Skip to content

[Test] Add resilience in TokenAuthIntegTests #64757

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 25, 2020
Merged

[Test] Add resilience in TokenAuthIntegTests #64757

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a0c67d
Add resilience in TokenAuthIntegTests
tvernum Nov 9, 2020
6d997b1
Merge branch 'master' into fix/56903-test-resilience
elasticmachine Nov 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions .../internalClusterTest/java/org/elasticsearch/xpack/security/authc/TokenAuthIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,26 @@ public void testExpiredTokensDeletedAfterExpiration() throws Exception {
assertThat(invalidateAccessTokenResponse.getPreviouslyInvalidatedTokens(), equalTo(0));
assertThat(invalidateAccessTokenResponse.getErrors(), empty());

// Weird testing behaviour ahead...
// invalidating by access token (above) is a Get, but invalidating by refresh token (below) is a Search
// In a multi node cluster, in a small % of cases, the search might find a document that has been invalidated but not yet deleted
// from that node's shard.
// Our assertion, therefore, is that an attempt to invalidate the (already invalidated) refresh token must not actually invalidate
// anything (concurrency controls must prevent that), nor may return any errors,
// but it might _temporarily_ find an "already invalidated" token.
final InvalidateTokenRequest invalidateRefreshTokenRequest = InvalidateTokenRequest.refreshToken(refreshToken);
InvalidateTokenResponse invalidateRefreshTokenResponse = restClient.security().invalidateToken(
InvalidateTokenRequest.refreshToken(refreshToken), SECURITY_REQUEST_OPTIONS);
invalidateRefreshTokenRequest, SECURITY_REQUEST_OPTIONS);
assertThat(invalidateRefreshTokenResponse.getInvalidatedTokens(), equalTo(0));
assertThat(invalidateRefreshTokenResponse.getPreviouslyInvalidatedTokens(), equalTo(0));
assertThat(invalidateRefreshTokenResponse.getErrors(), empty());
Copy link
Member

@ywangd ywangd Nov 9, 2020

Choose a reason for hiding this comment

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

It is worthwhile to link #64259 here because that is what actually fixed this assertion (original error of #56903).

Copy link
Contributor Author

@tvernum tvernum Nov 10, 2020

Choose a reason for hiding this comment

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

I don't follow.
Do you want this PR to reference another PR that is loosely related (they've both linked to the original issue)? Or do you want the code to reference the PR.

I didn't link these because they're separate pieces of work. One fixed a particular error in the code and the other fixes an incorrect assumption in the test. Both PRs link back to the original issue because the precised CI failure was a combination of the two, but I don't think the PRs themselves are that closely related.

Copy link
Member

@ywangd ywangd Nov 10, 2020

Choose a reason for hiding this comment

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

The former. This is mainly a note to myself. I am not suggesting you to do anything. I did not review the other PR. So I was a bit confused when trying to cross compare the build scan failure and the changes in this PR, which "resolves" the failure. Initially they seemed to be unrelated to me.

From the build scan, the exact error occured at the following line :

assertThat(invalidateRefreshTokenResponse.getErrors(), empty());

and technically speaking, the above error can be fixed by the other PR alone. By itself it will not be able to fix the "getPreviouslyInvalidatedTokens" assertion, but that was not the original error. So I added the comment as a reminder to my-future-self or maybe someone else who might need to look into it in future.


// 99% of the time, this will already be zero, but if not ensure it goes to zero within the allowed timeframe
if (invalidateRefreshTokenResponse.getPreviouslyInvalidatedTokens() > 0) {
assertBusy(() -> {
var newResponse = restClient.security().invalidateToken(invalidateRefreshTokenRequest, SECURITY_REQUEST_OPTIONS);
assertThat(newResponse.getPreviouslyInvalidatedTokens(), equalTo(0));
});
}
}

public void testInvalidateAllTokensForUser() throws Exception {
Expand Down