From e0c5fa3f9861f8b1c51eb0a3d18f38054aa97611 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Mon, 7 Oct 2019 10:40:39 +1100 Subject: [PATCH] Fix for ApiKeyIntegTests related to Expired API keys remover (#43477) (#47547) (#47628) When API key is invalidated we do two things first it tries to trigger `ExpiredApiKeysRemover` task and second, we do index the invalidation for the API key. The index invalidation may happen before the `ExpiredApiKeysRemover` task is run and in that case, the API key invalidated will also get deleted. If the `ExpiredApiKeysRemover` runs before the API key invalidation is indexed then the API key is not deleted and will be deleted in the future run. This behavior was not captured in the tests related to `ExpiredApiKeysRemover` causing intermittent failures. This commit fixes those tests by checking if the API key invalidated is reported back when we get API keys after invalidation and perform the checks based on that. Closes #41747 --- .../security/authc/ApiKeyIntegTests.java | 50 +++++++++++++++---- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java index c661bb6c2b47c..79bc3a349102f 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java @@ -267,9 +267,26 @@ public void testInvalidatedApiKeysDeletedByRemover() throws Exception { assertThat(invalidateResponse.getPreviouslyInvalidatedApiKeys().size(), equalTo(0)); assertThat(invalidateResponse.getErrors().size(), equalTo(0)); + awaitApiKeysRemoverCompletion(); + refreshSecurityIndex(); + PlainActionFuture getApiKeyResponseListener = new PlainActionFuture<>(); securityClient.getApiKey(GetApiKeyRequest.usingRealmName("file"), getApiKeyResponseListener); - assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, is(2)); + Set expectedKeyIds = Sets.newHashSet(createdApiKeys.get(0).getId(), createdApiKeys.get(1).getId()); + boolean apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover = false; + for (ApiKey apiKey : getApiKeyResponseListener.get().getApiKeyInfos()) { + assertThat(apiKey.getId(), isIn(expectedKeyIds)); + if (apiKey.getId().equals(createdApiKeys.get(0).getId())) { + // has been invalidated but not yet deleted by ExpiredApiKeysRemover + assertThat(apiKey.isInvalidated(), is(true)); + apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover = true; + } else if (apiKey.getId().equals(createdApiKeys.get(1).getId())) { + // active api key + assertThat(apiKey.isInvalidated(), is(false)); + } + } + assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, + is((apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover) ? 2 : 1)); client = waitForExpiredApiKeysRemoverTriggerReadyAndGetClient().filterWithHeader( Collections.singletonMap("Authorization", UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, @@ -282,16 +299,24 @@ public void testInvalidatedApiKeysDeletedByRemover() throws Exception { assertThat(listener.get().getInvalidatedApiKeys().size(), is(1)); awaitApiKeysRemoverCompletion(); - refreshSecurityIndex(); - // Verify that 1st invalidated API key is deleted whereas the next one is not + // Verify that 1st invalidated API key is deleted whereas the next one may be or may not be as it depends on whether update was + // indexed before ExpiredApiKeysRemover ran getApiKeyResponseListener = new PlainActionFuture<>(); securityClient.getApiKey(GetApiKeyRequest.usingRealmName("file"), getApiKeyResponseListener); - assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, is(1)); - ApiKey apiKey = getApiKeyResponseListener.get().getApiKeyInfos()[0]; - assertThat(apiKey.getId(), is(createdApiKeys.get(1).getId())); - assertThat(apiKey.isInvalidated(), is(true)); + expectedKeyIds = Sets.newHashSet(createdApiKeys.get(1).getId()); + apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover = false; + for (ApiKey apiKey : getApiKeyResponseListener.get().getApiKeyInfos()) { + assertThat(apiKey.getId(), isIn(expectedKeyIds)); + if (apiKey.getId().equals(createdApiKeys.get(1).getId())) { + // has been invalidated but not yet deleted by ExpiredApiKeysRemover + assertThat(apiKey.isInvalidated(), is(true)); + apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover = true; + } + } + assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, + is((apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover) ? 1 : 0)); } private Client waitForExpiredApiKeysRemoverTriggerReadyAndGetClient() throws Exception { @@ -338,7 +363,7 @@ public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() .setDoc("expiration_time", dayBefore.toEpochMilli()).setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).get(); assertThat(expirationDateUpdatedResponse.getResult(), is(DocWriteResponse.Result.UPDATED)); - // Expire the 2nd key such that it cannot be deleted by the remover + // Expire the 2nd key such that it can be deleted by the remover // hack doc to modify the expiration time to the week before Instant weekBefore = created.minus(8L, ChronoUnit.DAYS); assertTrue(Instant.now().isAfter(weekBefore)); @@ -356,13 +381,13 @@ public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() refreshSecurityIndex(); - // Verify get API keys does not return expired and deleted key + // Verify get API keys does not return api keys deleted by ExpiredApiKeysRemover getApiKeyResponseListener = new PlainActionFuture<>(); securityClient.getApiKey(GetApiKeyRequest.usingRealmName("file"), getApiKeyResponseListener); - assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, is(3)); Set expectedKeyIds = Sets.newHashSet(createdApiKeys.get(0).getId(), createdApiKeys.get(2).getId(), createdApiKeys.get(3).getId()); + boolean apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover = false; for (ApiKey apiKey : getApiKeyResponseListener.get().getApiKeyInfos()) { assertThat(apiKey.getId(), isIn(expectedKeyIds)); if (apiKey.getId().equals(createdApiKeys.get(0).getId())) { @@ -370,9 +395,10 @@ public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() assertTrue(apiKey.getExpiration().isBefore(Instant.now())); assertThat(apiKey.isInvalidated(), is(false)); } else if (apiKey.getId().equals(createdApiKeys.get(2).getId())) { - // has not been expired as no expiration, but invalidated + // has not been expired as no expiration, is invalidated but not yet deleted by ExpiredApiKeysRemover assertThat(apiKey.getExpiration(), is(nullValue())); assertThat(apiKey.isInvalidated(), is(true)); + apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover = true; } else if (apiKey.getId().equals(createdApiKeys.get(3).getId())) { // has not been expired as no expiration, not invalidated assertThat(apiKey.getExpiration(), is(nullValue())); @@ -381,6 +407,8 @@ public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() fail("unexpected API key " + apiKey); } } + assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, + is((apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover) ? 3 : 2)); } private void refreshSecurityIndex() throws Exception {