Skip to content

[Test] More resilience for token invalidation test #75319

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 5 commits into from
Aug 20, 2021
Merged

[Test] More resilience for token invalidation test #75319

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb87eed
[Test] More resilience for token invalidation test
ywangd Jul 14, 2021
1673512
tweak
ywangd Jul 14, 2021
542260f
Update x-pack/plugin/security/src/internalClusterTest/java/org/elasti…
ywangd Jul 14, 2021
5c38f37
Merge remote-tracking branch 'origin/master' into es-67347-token-inva…
ywangd Aug 3, 2021
41d596e
address feedback
ywangd Aug 3, 2021
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
42 changes: 34 additions & 8 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 @@ -197,26 +197,52 @@ public void testExpiredTokensDeletedAfterExpiration() throws Exception {

// 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
// In a multi node cluster, in a small % of cases, the search might find a document that has been deleted but not yet refreshed
// in that node's shard.
// Our assertion, therefore, is that an attempt to invalidate the 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.
// but it might _temporarily_ find an "already deleted" token.
final InvalidateTokenRequest invalidateRefreshTokenRequest = InvalidateTokenRequest.refreshToken(refreshToken);
InvalidateTokenResponse invalidateRefreshTokenResponse = restClient.security().invalidateToken(
invalidateRefreshTokenRequest, SECURITY_REQUEST_OPTIONS);
assertThat(invalidateRefreshTokenResponse.getInvalidatedTokens(), equalTo(0));
assertThat(invalidateRefreshTokenResponse.getErrors(), empty());
Copy link
Contributor

@tvernum tvernum Jul 14, 2021

Choose a reason for hiding this comment

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

One of the problems here is that we have this very complex test, but nothing testing the more normal cases.
As far as I can tell there is nothing to test what happens if you try to invalidate an access token and then invalidate its refresh token. What's that behaviour supposed to be?

And this test is trying to test a behaviour that we mostly don't care about - specifically what happens if you try to invalidate a refresh token after it's been deleted from the index? The answer is it should either behave like a token that never existed, or it should behave like a token that was recently invalidated. We don't really mind which.

I don't think we should ever return errors though - it's not the caller's problem that the cleanup job just ran.

Copy link
Member Author

@ywangd ywangd Jul 14, 2021

Choose a reason for hiding this comment

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

but nothing testing the more normal cases

We can (and should) add more tests. But I'd prefer a separate PR. This one is just to tame the number of CI failures.

And this test is trying to test a behaviour that we mostly don't care about - specifically what happens if you try to invalidate a refresh token after it's been deleted from the index?

If we go by the test name literally, to me, testExpiredTokensDeletedAfterExpiration means it is sufficient to ensure that the expired token document is deleted once the cleanup job runs. It has nothing to do with subsuquent invalidations after the deletion. And like you said, any subsequent invalidation should just behave like the document never exists and we do have a separate test (testInvalidateNotValidRefreshTokens) for that. If this is the case, I'd say the test is sufficient after the second assertBusy block which asserts the document no longer exists after triggering the cleanup job.

So a simpler fix is to just drop all the code after it. We can then have separate PRs to add tests for more common use cases, e.g. invalidate refresh token after access token is invalidated etc.

Copy link
Contributor

@tvernum tvernum Jul 29, 2021

Choose a reason for hiding this comment

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

Part of my concern here is that this test says it's OK to return errors for a period of time after the deletion occurs, as long as they eventually go away.

Why is that? That seems like it's just accepting that there is a bug that we don't want to fix.
And due to the weirdness in these tests overall, it's hard to tell whether that's a bug that manifests more generally, or whether it's just an edge case that we don't care about and should stop testing for.

Because it sounds like we don't care, and we're just trying to get the test to run.
Why do these assertions model the behaviour that we actually want from the code?

Copy link
Member Author

@ywangd ywangd Aug 3, 2021

Choose a reason for hiding this comment

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

I added a new test for "deleting access and refresh tokens one after the other with randomness" to cover the more realistic use case as you suggested.

As for this test, the elaborated code indeed suggests for a bug. There are also other bugs in TokenService and they are all more or less caused by the same dirty read problem. All of them are quite rare and this one is even less a practical issue. So we didn't get time around to tackle them, plus they all have some complexities. Even for this one, code wise it is likely easy to skip any DocumentMissingException and just returns normally. But I am not sure whether this would be covering up other subtle and likely very rare problems. In my opinion, it is at least safer to report a false error instead of silently skip the error (unless we are perfectly sure). All these sounds like a bit too much for a PR that is meant to reduce our CI backlog. I can raise an issue to track the bug, but I'd prefer to keep it separate from this PR.

assertThat(invalidateRefreshTokenResponse.getPreviouslyInvalidatedTokens(), equalTo(0));

// 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) {
// 99% of the time, this will already be empty, but if not ensure it goes to empty within the allowed timeframe
if (false == invalidateRefreshTokenResponse.getErrors().isEmpty()) {
assertBusy(() -> {
var newResponse = restClient.security().invalidateToken(invalidateRefreshTokenRequest, SECURITY_REQUEST_OPTIONS);
assertThat(newResponse.getPreviouslyInvalidatedTokens(), equalTo(0));
assertThat(newResponse.getErrors(), empty());
Copy link
Member Author

@ywangd ywangd Jul 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I was the reviewer for the previous change (#64757). But somehow I didn't realise that we should wrap getErrors() here instead of getPreviouslyInvalidatedTokens() because:

  1. The original failure (TokenAuthIntegTests testExpiredTokensDeletedAfterExpiration failure #56903) was about getErrors().
  2. getPreviouslyInvalidatedTokens() should always return 0 since the refersh token is never invalidated before. If the cluster behaves really weirdly, we could probably expect getInvalidatedTokens() to be 1, but not getPreviouslyInvalidatedTokens().

});
}
}

public void testAccessTokenAndRefreshTokenCanBeInvalidatedIndependently() throws IOException {
final RestHighLevelClient restClient = new TestRestHighLevelClient();
CreateTokenResponse response = restClient.security().createToken(CreateTokenRequest.passwordGrant(
SecuritySettingsSource.TEST_USER_NAME, SecuritySettingsSourceField.TEST_PASSWORD.toCharArray()), SECURITY_REQUEST_OPTIONS);
final InvalidateTokenRequest invalidateRequest1, invalidateRequest2;
if (randomBoolean()) {
invalidateRequest1 = InvalidateTokenRequest.accessToken(response.getAccessToken());
invalidateRequest2 = InvalidateTokenRequest.refreshToken(response.getRefreshToken());
} else {
invalidateRequest1 = InvalidateTokenRequest.refreshToken(response.getRefreshToken());
invalidateRequest2 = InvalidateTokenRequest.accessToken(response.getAccessToken());
}

final InvalidateTokenResponse response1 =
restClient.security().invalidateToken(invalidateRequest1, SECURITY_REQUEST_OPTIONS);
assertThat(response1.getInvalidatedTokens(), equalTo(1));
assertThat(response1.getPreviouslyInvalidatedTokens(), equalTo(0));
assertThat(response1.getErrors(), empty());

final InvalidateTokenResponse response2 =
restClient.security().invalidateToken(invalidateRequest2, SECURITY_REQUEST_OPTIONS);
assertThat(response2.getInvalidatedTokens(), equalTo(1));
assertThat(response2.getPreviouslyInvalidatedTokens(), equalTo(0));
assertThat(response2.getErrors(), empty());
}

public void testInvalidateAllTokensForUser() throws Exception {
final RestHighLevelClient restClient = new TestRestHighLevelClient();
final int numOfRequests = randomIntBetween(5, 10);
Expand Down