Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent duplicate accounts #587

Closed
wants to merge 3 commits into from
Closed

Conversation

kulmann
Copy link
Contributor

@kulmann kulmann commented Sep 16, 2020

No description provided.

@kulmann kulmann requested review from C0rby and IljaN September 16, 2020 10:07
@phil-davis
Copy link
Contributor

apiProvisioning-v2/addUser.feature:32 starts passing - that is great. That scenario purposely tries to create an existing user, and now the expected status 400 is returned.

apiProvisioning-v2/addUser.feature:102 - similar, that scenario tries to create the user again, with a different case username. That also now returns the expected 400 status.

So those can be removed from the expected-failures files in this PR.

@phil-davis
Copy link
Contributor

But other tests are now failing. I suspect that is because:
https://cloud.drone.io/owncloud/ocis/1624/5/7

  Scenario: admin tries to create an existing user but with username containing capital letters                                 # /srv/app/testrunner/tests/acceptance/features/apiProvisioning-v2/addUser.feature:102
    Given user "brand-new-user" has been created with default attributes and skeleton files                                     # FeatureContext::userHasBeenCreatedWithDefaultAttributes()
    When the administrator sends a user creation request for user "BRAND-NEW-USER" password "%alt1%" using the provisioning API # FeatureContext::adminSendsUserCreationRequestUsingTheProvisioningApi()
    Then the OCS status code should be "400"                                                                                    # OCSContext::theOCSStatusCodeShouldBe()
    And the HTTP status code should be "400"                                                                                    # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the API should not return any data                                                                                      # FeatureContext::theApiShouldNotReturnAnyData()
INFORMATION: could not delete user 'BRAND-NEW-USER' 404 <?xml version="1.0" encoding="UTF-8"?>
<ocs><meta><status>error</status><statuscode>998</statuscode><message>The requested user could not be found</message></meta></ocs>

The test code creates brand-new-user then tries to create BRAND-NEW-USER, which correctly fails.

But then the afterScenario tries to delete user BRAND-NEW-USER
Actually that should work (it works in oC10). When an API request is received to delete a user, it should not matter what mixture of upper/lower case is in the request.

But to make the test suite more forgiving there, I should fix that up so that the test suite remember correctly the exact user that was first created.

@kulmann kulmann force-pushed the prevent-duplicate-accounts branch from 10cd37a to 0ec3cd7 Compare September 17, 2020 07:03
@kulmann
Copy link
Contributor Author

kulmann commented Sep 17, 2020

@phil-davis how much effort would it be to - like you mentioned - make the test suite more forgiving? I think playing around with username vs userid replacements and generating an internal id (see owncloud/ocis-ocs#64) might create followup issues...

@phil-davis
Copy link
Contributor

phil-davis commented Sep 18, 2020

But to make the test suite more forgiving there, I should fix that up so that the test suite remember correctly the exact user that was first created.

core PR owncloud/core#37917 - the test code will be as nice as it ccan be in the afterSccenario cleanups, making sure to use the same upper-lower case mix in the username (userid) when deleting as was used when the user was originally created. When that PR is merged, we can bump the core commit id and see what improves.

@micbar
Copy link
Contributor

micbar commented Sep 22, 2020

@kulmann ocis accounts on master branch is broken.

Steps to reproduce:

  • Start ocis server
  • Create a user using the provisioning API curl -X POST http://localhost:9110/ocs/v1.php/cloud/users -d userid=michael -d password="spaces in my password" -d username="michael" -d displayname="display name" -d email="michael@email.com" -d enabled="true" | xmllint --format -
  • Delete the user using a case sensitive ID curl -X DELETE http://localhost:9110/ocs/v1.php/cloud/users/MiChAEl
  • Create the user again using the Command from step 2

Expected

  • no error

Actual

  • 2020-09-22T14:48:50+02:00 ERR could not add user error="{\"id\":\"com.owncloud.api.accounts\",\"code\":400,\"detail\":\"account already exists\",\"status\":\"Bad Request\"}" service=ocs userid=michael

@kulmann
Copy link
Contributor Author

kulmann commented Oct 8, 2020

Superseded by #665

@kulmann kulmann closed this Oct 8, 2020
@kulmann kulmann mentioned this pull request Oct 8, 2020
1 task
@C0rby C0rby deleted the prevent-duplicate-accounts branch February 25, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants