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

Reenable auth provider tests #32565

Merged
merged 5 commits into from
Mar 6, 2019

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Mar 6, 2019

With the huge improvements which elastic/elasticsearch#39631 gives us, we had to temporarily skip some of our tests. This PR re-enables the tests to assert the new behavior.

Refresh tokens can now be re-used for up to 60 seconds, and they'll return the same access/refresh token as before. These access/refresh tokens are encrypted within the session cookie, and the tests don't attempt to decrypt the cookie to ensure that the auth/refresh tokens are the same, but instead ensure that both continue working to authenticate subsequent requests.

The time limit for reusing refresh tokens is hard-coded at 60 seconds, and as such makes us have some rather long-running tests to ensure that we can't reuse refresh tokens after this window. It bothers me that we have to introduce tests that end up taking right over 85 seconds to execute, but I'd prefer to have the coverage.

Resolves: #26744

@kobelb kobelb added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Mar 6, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@epixa
Copy link
Contributor

epixa commented Mar 6, 2019

I don't know if we need to test in Kibana that refresh tokens can't be used after 60 seconds. We have no control at all over that behavior, and we should be able to assume that Elasticsearch has tests on its own features.

If we have unit tests in place for the expected behavior when refresh fails and the expected behavior when an unknown error response is encountered, that should be pretty solid coverage. Your call though.

@kobelb
Copy link
Contributor Author

kobelb commented Mar 6, 2019

@epixa I was torn on this as well and almost didn't add them. I'll go ahead and remove them.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

@kobelb kobelb merged commit 1a1f4b7 into elastic:master Mar 6, 2019
@kobelb kobelb deleted the reenable-auth-provider-tests branch March 6, 2019 17:30
kobelb added a commit to kobelb/kibana that referenced this pull request Mar 6, 2019
* Reenabling token auth provider test

* Re-enabling saml test

* Adding a SAML test to ensure we can't reuse refresh token after 60 seconds

* Adding token auth provider re-use refresh token after 60 seconds test

* Removing the reusuing refresh tokens after timeout
PhilippBaranovskiy pushed a commit to PhilippBaranovskiy/kibana that referenced this pull request Mar 7, 2019
* Reenabling token auth provider test

* Re-enabling saml test

* Adding a SAML test to ensure we can't reuse refresh token after 60 seconds

* Adding token auth provider re-use refresh token after 60 seconds test

* Removing the reusuing refresh tokens after timeout
kobelb added a commit that referenced this pull request Mar 7, 2019
* Reenabling token auth provider test

* Re-enabling saml test

* Adding a SAML test to ensure we can't reuse refresh token after 60 seconds

* Adding token auth provider re-use refresh token after 60 seconds test

* Removing the reusuing refresh tokens after timeout
@kobelb kobelb added the release_note:skip Skip the PR/issue when compiling release notes label May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SAML/token Concurrent XHR Requests
3 participants