Use refresh token for any access token error with 401 status code and re-authenticate user for any refresh token error with 400 status code. #33777
Conversation
azasypkin
added
blocked
Team:Security
|
Pinging @elastic/kibana-security |
This comment has been minimized.
This comment has been minimized.
azasypkin
force-pushed
the
issue-33646-400-and-401
branch
from
b1e6272 to
a62a9b2
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Need to merge master and rewrite this import before merging: #33761
There was a problem hiding this comment.
If I'm following along correctly, the following tests should be updated as well:
fails if state contains invalid credentials.line 179fails when session contains a rejected tokenline 285
When submitting a request to _security/_authenticate with an invalid Bearer header, we're getting back a 401 as well, which will match the new isAccessTokenExpiredError and attempt to use the refresh token to get a new access token. Previously, we weren't trying to use the refresh tokens in this situation.
There was a problem hiding this comment.
fails if state contains invalid credentials. line 179
fails when session contains a rejected token line 285
Hmm, right, but If we change these tests, then they will be exactly the same as succeeds with valid session even if requiring a token refresh, line 134 or redirects non-AJAX requests to /login and clears session if token refresh fails with 400 error' line 360, depending on what happens during refreshing.
It feels like we can either remove these tests completely or change them to test non-401 errors (e.g. 500), what do you think?
There was a problem hiding this comment.
I like the idea of adding tests for the 500 situation
There was a problem hiding this comment.
Save question as above...
Is it possible that when we do the access token logic, we get back the 401 because the token document exists and it's expired, and then before we get here the refresh token is deleted and we get back a 500 with the message body denoting the token being deleted?
There was a problem hiding this comment.
See reply above, tl;dr it shouldn't be possible.
There was a problem hiding this comment.
Thanks for looking into this!
There was a problem hiding this comment.
Same comment as above about logging the error message.
There was a problem hiding this comment.
We log all error messages right at the beginning of the catch :) Or you think just logging message isn't enough? Potentially the error can have body.error.reason or body.error.error_description, but I'm not sure if we want to somehow extract and log these....
There was a problem hiding this comment.
I think just err.message is fine, I completely missed that we did so above. Please ignore!
x-pack/plugins/security/server/lib/authentication/providers/saml.ts
Outdated
Show resolved
Hide resolved
… re-initiate SAML handshake for any refresh token error with 400 status code.
azasypkin
force-pushed
the
issue-33646-400-and-401
branch
from
a62a9b2 to
8830007
Compare
|
Had to force-push after rebase on master, but nothing has changed and won't do it for this PR anymore. |
This comment has been minimized.
This comment has been minimized.
| expect(authenticationResult.notHandled()).to.be(true); | ||
| }); | ||
|
|
||
| it('fails if state contains invalid credentials.', async () => { |
There was a problem hiding this comment.
note: we had a similar one below, that I've repurposed to test 500 errors.
💚 Build Succeeded |
azasypkin
added
the
Feature:Security/Authentication
… re-authentication user for any refresh token error with 400 status code. (elastic#33777) * Use refresh token for any access token error with 401 status code and re-initiate SAML handshake for any refresh token error with 400 status code. * Switch from `expect.js` to `@kbn/expect`. * Review#1: repurpose redundant tests to test token failures with 500 code.
… re-authentication user for any refresh token error with 400 status code. (#33777) * Use refresh token for any access token error with 401 status code and re-initiate SAML handshake for any refresh token error with 400 status code. * Switch from `expect.js` to `@kbn/expect`. * Review#1: repurpose redundant tests to test token failures with 500 code.
azasypkin
changed the title
4 participants
Always refresh access token if authentication fails with
401(and temporarily with500: 'token document is missing and must be present), and always re-initiate authentication if refresh token fails with400, no matter what the underlying reason is.Blocked by #33774Fixes #33646 and #22905
"Release Note: Kibana now automatically re-initiates login when session access/refresh token pair used for Token and SAML authentication is removed from Elasticsearch (typically after 24 hours of user inactivity)."