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

implement basic auth cache #958

Merged
merged 5 commits into from
Nov 26, 2020
Merged

implement basic auth cache #958

merged 5 commits into from
Nov 26, 2020

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Nov 26, 2020

No description provided.

@fschade fschade requested a review from kulmann November 26, 2020 09:50
@fschade fschade requested a review from IljaN November 26, 2020 09:58
@owncloud owncloud deleted a comment from update-docs bot Nov 26, 2020
@phil-davis
Copy link
Contributor

phil-davis commented Nov 26, 2020

https://drone.owncloud.com/owncloud/ocis/1754/20/1

  Scenario: reset user password                                                                                                                      # /srv/app/testrunner/tests/acceptance/features/apiProvisioning-v1/resetUserPassword.feature:11
    Given these users have been created with skeleton files:                                                                                         # FeatureContext::theseUsersHaveBeenCreated()
      | username       | password  | displayname | email                    |
      | brand-new-user | %regular% | New user    | brand.new.user@oc.com.np |
    When the administrator resets the password of user "brand-new-user" to "%alt1%" using the provisioning API                                       # FeatureContext::adminResetsPasswordOfUserUsingTheProvisioningApi()
    Then the OCS status code should be "100"                                                                                                         # OCSContext::theOCSStatusCodeShouldBe()
    And the HTTP status code should be "200"                                                                                                         # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the content of file "textfile0.txt" for user "brand-new-user" using password "%alt1%" should be "ownCloud test text file 0" plus end-of-line # FeatureContext::contentOfFileForUserUsingPasswordShouldBePlusEndOfLine()
    But user "brand-new-user" using password "%regular%" should not be able to download file "textfile0.txt"                                         # FeatureContext::userUsingPasswordShouldNotBeAbleToDownloadFile()
      WebDav::userUsingPasswordShouldNotBeAbleToDownloadFile download must fail
      Failed asserting that 200 is equal to 400 or is greater than 400.

When the admin changes the password of a user, the cache needs to get invalidated.

It looks like the new password worked - textfile0.txt can be read iwth the new password. But the old password seems to also still work. Maybe both end up in the cache?

@fschade
Copy link
Contributor Author

fschade commented Nov 26, 2020

https://drone.owncloud.com/owncloud/ocis/1754/20/1

  Scenario: reset user password                                                                                                                      # /srv/app/testrunner/tests/acceptance/features/apiProvisioning-v1/resetUserPassword.feature:11
    Given these users have been created with skeleton files:                                                                                         # FeatureContext::theseUsersHaveBeenCreated()
      | username       | password  | displayname | email                    |
      | brand-new-user | %regular% | New user    | brand.new.user@oc.com.np |
    When the administrator resets the password of user "brand-new-user" to "%alt1%" using the provisioning API                                       # FeatureContext::adminResetsPasswordOfUserUsingTheProvisioningApi()
    Then the OCS status code should be "100"                                                                                                         # OCSContext::theOCSStatusCodeShouldBe()
    And the HTTP status code should be "200"                                                                                                         # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the content of file "textfile0.txt" for user "brand-new-user" using password "%alt1%" should be "ownCloud test text file 0" plus end-of-line # FeatureContext::contentOfFileForUserUsingPasswordShouldBePlusEndOfLine()
    But user "brand-new-user" using password "%regular%" should not be able to download file "textfile0.txt"                                         # FeatureContext::userUsingPasswordShouldNotBeAbleToDownloadFile()
      WebDav::userUsingPasswordShouldNotBeAbleToDownloadFile download must fail
      Failed asserting that 200 is equal to 400 or is greater than 400.

When the admin changes the password of a user, the cache needs to get invalidated.

It looks like the new password worked - textfile0.txt can be read iwth the new password. But the old password seems to also still work. Maybe both end up in the cache?

at this stage we won't notice any updates. As far as i understood it, basic auth should be non production only. But you`re right that could get us into trouble.

Maybe a better place could be the accounts service where the isPasswordValid func is... there we can react to password changes and invalidate it.

Florian Schade added 2 commits November 26, 2020 13:52
move cache to ocis-pkg
add password validation cache to accounts service
remove unused mux
cleanup k6 test
@sonarcloud
Copy link

sonarcloud bot commented Nov 26, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis
Copy link
Contributor

Run times of the API suites are typically less than half what they were before this change. That's amazing! The API tests do intensive API requests. As well as the obvious API requests that are done by the test steps in a scenario, there is lots of setup and teardown - creating users, uploading "skeleton" files... And every request has some basic auth that needs to be validated.

The UI tests do not show any obvious difference in run time. That is because most of their time is spent in rendering the UI and the selenium web driver querying the state of the DOM in the browser, waiting for browser state...

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works. There are test scenarios that try to access resources using invalid auth (wrong password, no password etc) and that change the user's password and try to use the old password... They are all passing - so the cache is not opening holes in the auth. From an external point-of-view this is "a great thing" - performance improves and behaviour is preserved.

I will let someone else review the detail of the structural code changes.

tests/k6/src/test-issue-162.ts Show resolved Hide resolved
@butonic butonic merged commit dbb52f2 into master Nov 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the basic-auth-cache branch November 26, 2020 16:33
ownclouders pushed a commit that referenced this pull request Nov 26, 2020
Merge: cda8aad cb2e2a3
Author: Jörn Friedrich Dreyer <jfd@butonic.de>
Date:   Thu Nov 26 17:33:47 2020 +0100

    Merge pull request #958 from owncloud/basic-auth-cache

    implement basic auth cache
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.

3 participants