From 985cbe9887ff47b219e3a8fbe420c5b3bea8f793 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Wed, 2 Oct 2019 16:55:58 +1000 Subject: [PATCH] Fix for ApiKeyIntegTests related to Expired API keys remover (#43477) 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 | 52 ++++++++++++++----- 1 file changed, 40 insertions(+), 12 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 9e6e496fda37c..5974678fdd320 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 @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.security.authc; import com.google.common.collect.Sets; + import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; @@ -292,9 +293,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, @@ -307,16 +325,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 { @@ -339,7 +365,6 @@ private Client waitForExpiredApiKeysRemoverTriggerReadyAndGetClient() throws Exc return internalCluster().client(nodeWithMostRecentRun); } - @AwaitsFix( bugUrl = "https://github.com/elastic/elasticsearch/issues/41747") public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() throws Exception { Client client = waitForExpiredApiKeysRemoverTriggerReadyAndGetClient().filterWithHeader( Collections.singletonMap("Authorization", UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, @@ -366,7 +391,7 @@ public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() .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)); @@ -385,13 +410,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())) { @@ -399,9 +424,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())); @@ -410,6 +436,8 @@ public void testExpiredApiKeysBehaviorWhenKeysExpired1WeekBeforeAnd1DayBefore() fail("unexpected API key " + apiKey); } } + assertThat(getApiKeyResponseListener.get().getApiKeyInfos().length, + is((apiKeyInvalidatedButNotYetDeletedByExpiredApiKeysRemover) ? 3 : 2)); } private void refreshSecurityIndex() throws Exception {