Fix responses for the token APIs #54532
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ | |
| import org.elasticsearch.license.XPackLicenseState; | ||
| import org.elasticsearch.rest.RestStatus; | ||
| import org.elasticsearch.search.SearchHit; | ||
| import org.elasticsearch.search.SearchHits; | ||
| import org.elasticsearch.xpack.core.XPackField; | ||
| import org.elasticsearch.xpack.core.XPackPlugin; | ||
| import org.elasticsearch.xpack.core.XPackSettings; | ||
|
|
@@ -584,7 +585,8 @@ public void invalidateAccessToken(String accessToken, ActionListener<TokensInval | |
| final Iterator<TimeValue> backoff = DEFAULT_BACKOFF.iterator(); | ||
| decodeToken(accessToken, ActionListener.wrap(userToken -> { | ||
| if (userToken == null) { | ||
| logger.trace("The access token [{}] is expired and already deleted", accessToken); | ||
| listener.onResponse(TokensInvalidationResult.emptyResult()); | ||
|
There was a problem hiding this comment. With this change, a 200 response will be returned if the token index does not exist. This behaviour is different from the refresh token invalidation, which returns 400 invalidGrant when the index does not exist. |
||
| } else { | ||
| indexInvalidation(Collections.singleton(userToken), backoff, "access_token", null, listener); | ||
| } | ||
|
|
@@ -623,9 +625,17 @@ public void invalidateRefreshToken(String refreshToken, ActionListener<TokensInv | |
| maybeStartTokenRemover(); | ||
| final Iterator<TimeValue> backoff = DEFAULT_BACKOFF.iterator(); | ||
| findTokenFromRefreshToken(refreshToken, | ||
| backoff, ActionListener.wrap(searchHits -> { | ||
| if (searchHits.getHits().length < 1) { | ||
| logger.debug("could not find token document for refresh token"); | ||
| listener.onResponse(TokensInvalidationResult.emptyResult()); | ||
| } else if (searchHits.getHits().length > 1) { | ||
| listener.onFailure(new IllegalStateException("multiple tokens share the same refresh token")); | ||
| } else { | ||
| final Tuple<UserToken, String> parsedTokens = | ||
| parseTokensFromDocument(searchHits.getAt(0).getSourceAsMap(), null); | ||
| indexInvalidation(Collections.singletonList(parsedTokens.v1()), backoff, "refresh_token", null, listener); | ||
| } | ||
| }, listener::onFailure)); | ||
| } | ||
| } | ||
|
|
@@ -827,21 +837,30 @@ public void refreshToken(String refreshToken, ActionListener<Tuple<String, Strin | |
| ensureEnabled(); | ||
| final Instant refreshRequested = clock.instant(); | ||
| final Iterator<TimeValue> backoff = DEFAULT_BACKOFF.iterator(); | ||
| final Consumer<Exception> onFailure = ex -> listener.onFailure(traceLog("find token by refresh token", refreshToken, ex)); | ||
| findTokenFromRefreshToken(refreshToken, | ||
| backoff, | ||
| ActionListener.wrap(searchHits -> { | ||
| if (searchHits.getHits().length < 1) { | ||
| logger.warn("could not find token document for refresh token"); | ||
| onFailure.accept(invalidGrantException("could not refresh the requested token")); | ||
| } else if (searchHits.getHits().length > 1) { | ||
| onFailure.accept(new IllegalStateException("multiple tokens share the same refresh token")); | ||
| } else { | ||
| final SearchHit tokenDocHit = searchHits.getAt(0); | ||
| final Authentication clientAuth = securityContext.getAuthentication(); | ||
| innerRefresh(refreshToken, tokenDocHit.getId(), tokenDocHit.getSourceAsMap(), tokenDocHit.getSeqNo(), | ||
| tokenDocHit.getPrimaryTerm(), | ||
| clientAuth, backoff, refreshRequested, listener); | ||
| } | ||
| }, listener::onFailure)); | ||
| } | ||
|
|
||
| /** | ||
| * Infers the format and version of the passed in {@code refreshToken}. Delegates the actual search of the token document to | ||
| * {@code #findTokenFromRefreshToken(String, SecurityIndexManager, Iterator, ActionListener)} . | ||
| */ | ||
| private void findTokenFromRefreshToken(String refreshToken, Iterator<TimeValue> backoff, ActionListener<SearchHits> listener) { | ||
| if (refreshToken.length() == TOKEN_LENGTH) { | ||
| // first check if token has the old format before the new version-prepended one | ||
| logger.debug("Assuming an unversioned refresh token [{}], generated for node versions" | ||
|
|
@@ -860,7 +879,7 @@ private void findTokenFromRefreshToken(String refreshToken, Iterator<TimeValue> | |
| if (refreshTokenVersion.before(VERSION_TOKENS_INDEX_INTRODUCED) || unencodedRefreshToken.length() != TOKEN_LENGTH) { | ||
| logger.debug("Decoded refresh token [{}] with version [{}] is invalid.", unencodedRefreshToken, | ||
| refreshTokenVersion); | ||
| listener.onResponse(SearchHits.empty()); | ||
| } else { | ||
| // TODO Remove this conditional after backporting to 7.x | ||
| if (refreshTokenVersion.onOrAfter(VERSION_HASHED_TOKENS)) { | ||
|
|
@@ -872,7 +891,7 @@ private void findTokenFromRefreshToken(String refreshToken, Iterator<TimeValue> | |
| } | ||
| } catch (IOException e) { | ||
|
There was a problem hiding this comment. This kinda reasonates on what we have discussed the other day around how many statements should be surrounded by a try-catch block. The IOException is only thrown by There was a problem hiding this comment. I see your point, took a swing at it |
||
| logger.debug(() -> new ParameterizedMessage("Could not decode refresh token [{}].", refreshToken), e); | ||
| listener.onResponse(SearchHits.empty()); | ||
|
There was a problem hiding this comment. All usage of |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -884,7 +903,7 @@ private void findTokenFromRefreshToken(String refreshToken, Iterator<TimeValue> | |
| * backoff policy. This method requires the tokens index where the token document, pointed to by the refresh token, resides. | ||
| */ | ||
| private void findTokenFromRefreshToken(String refreshToken, SecurityIndexManager tokensIndexManager, Iterator<TimeValue> backoff, | ||
| ActionListener<SearchHits> listener) { | ||
| final Consumer<Exception> onFailure = ex -> listener.onFailure(traceLog("find token by refresh token", refreshToken, ex)); | ||
| final Consumer<Exception> maybeRetryOnFailure = ex -> { | ||
| if (backoff.hasNext()) { | ||
|
|
@@ -917,13 +936,8 @@ private void findTokenFromRefreshToken(String refreshToken, SecurityIndexManager | |
| if (searchResponse.isTimedOut()) { | ||
| logger.debug("find token from refresh token response timed out, retrying"); | ||
| maybeRetryOnFailure.accept(invalidGrantException("could not refresh the requested token")); | ||
| } else { | ||
| listener.onResponse(searchResponse.getHits()); | ||
| } | ||
| }, e -> { | ||
| if (isShardNotAvailableException(e)) { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The (expected for this test) 500 was masked by the
401that was thrown inelasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutAction.java
Line 55 in d029a13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because the response changes to 200 and error now happens in the OIDC part as 500?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is necessary because we don't do the login flow so we don't have tokens to invalidate for the logout. We expect that this call would fail for that reason - but it failing with a
401was a mistake