From 1a373b0c2192720d9254ade2cd62ea0846ec99a8 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 3 Aug 2020 13:45:16 +1000 Subject: [PATCH] Only call listener once (SP template registration) (#60567) This fixes a bug in the IdP's template registration that would sometimes call the listener twice. Resolves: #54285 Resolves: #54423 Backport of: #60497 --- .../idp/saml/sp/SamlServiceProviderIndex.java | 1 + .../idp/action/SamlIdentityProviderTests.java | 4 +- .../sp/SamlServiceProviderIndexTests.java | 43 +++++++++++-------- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java index 3e02cbad47c..c5e10d9c6a2 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java @@ -205,6 +205,7 @@ public class SamlServiceProviderIndex implements Closeable { final ClusterState state = clusterService.state(); if (isTemplateUpToDate(state)) { listener.onResponse(false); + return; } final String template = TemplateUtils.loadTemplate(TEMPLATE_RESOURCE, Version.CURRENT.toString(), TEMPLATE_VERSION_SUBSTITUTE); final PutIndexTemplateRequest request = new PutIndexTemplateRequest(TEMPLATE_NAME).source(template, XContentType.JSON); diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlIdentityProviderTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlIdentityProviderTests.java index 0fd0185a1c0..75ec4da62af 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlIdentityProviderTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlIdentityProviderTests.java @@ -282,13 +282,13 @@ public class SamlIdentityProviderTests extends IdentityProviderIntegTestCase { assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(RestStatus.BAD_REQUEST.getStatus())); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/54285") public void testSpInitiatedSsoFailsForMalformedRequest() throws Exception { String acsUrl = "https://" + randomAlphaOfLength(12) + ".elastic-cloud.com/saml/acs"; String entityId = SP_ENTITY_ID; registerServiceProvider(entityId, acsUrl); registerApplicationPrivileges(); ensureGreen(SamlServiceProviderIndex.INDEX_NAME); + // Validate incoming authentication request Request validateRequest = new Request("POST", "/_idp/saml/validate"); validateRequest.setOptions(REQUEST_OPTIONS_AS_CONSOLE_USER); @@ -298,12 +298,14 @@ public class SamlIdentityProviderTests extends IdentityProviderIntegTestCase { final AuthnRequest authnRequest = buildAuthnRequest(entityId + randomAlphaOfLength(4), new URL(acsUrl), new URL("https://idp.org/sso/redirect"), nameIdFormat, forceAuthn); final String query = getQueryString(authnRequest, relayString, false, null); + // Skip http parameter name final String queryWithoutParam = query.substring(12); validateRequest.setJsonEntity("{\"authn_request_query\":\"" + queryWithoutParam + "\"}"); ResponseException e = expectThrows(ResponseException.class, () -> getRestClient().performRequest(validateRequest)); assertThat(e.getMessage(), containsString("does not contain a SAMLRequest parameter")); assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(RestStatus.BAD_REQUEST.getStatus())); + // arbitrarily trim the request final String malformedRequestQuery = query.substring(0, query.length() - randomIntBetween(10, 15)); validateRequest.setJsonEntity("{\"authn_request_query\":\"" + malformedRequestQuery + "\"}"); diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndexTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndexTests.java index e0cebae898d..13386896f41 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndexTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndexTests.java @@ -36,6 +36,7 @@ import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import static org.elasticsearch.xpack.idp.saml.idp.SamlIdentityProviderBuilder.IDP_ENTITY_ID; @@ -132,9 +133,7 @@ public class SamlServiceProviderIndexTests extends ESSingleNodeTestCase { } public void testWritesViaAliasIfItExists() { - final PlainActionFuture installTemplate = new PlainActionFuture<>(); - serviceProviderIndex.installIndexTemplate(installTemplate); - assertTrue(installTemplate.actionGet()); + assertTrue(installTemplate()); // Create an index that will trigger the template, but isn't the standard index name final String customIndexName = SamlServiceProviderIndex.INDEX_NAME + "-test"; @@ -172,9 +171,7 @@ public class SamlServiceProviderIndexTests extends ESSingleNodeTestCase { assertBusy(() -> assertThat("template should have been installed", templateMeta, notNullValue())); - final PlainActionFuture installTemplate = new PlainActionFuture<>(); - serviceProviderIndex.installIndexTemplate(installTemplate); - assertFalse("Template is already installed, should not install again", installTemplate.actionGet()); + assertFalse("Template is already installed, should not install again", installTemplate()); } public void testInstallTemplateAutomaticallyOnDocumentWrite() { @@ -186,45 +183,44 @@ public class SamlServiceProviderIndexTests extends ESSingleNodeTestCase { IndexTemplateMetadata templateMeta = clusterService.state().metadata().templates().get(SamlServiceProviderIndex.TEMPLATE_NAME); assertThat("template should have been installed", templateMeta, notNullValue()); - final PlainActionFuture installTemplate = new PlainActionFuture<>(); - serviceProviderIndex.installIndexTemplate(installTemplate); - assertFalse("Template is already installed, should not install again", installTemplate.actionGet()); + assertFalse("Template is already installed, should not install again", installTemplate()); } private boolean installTemplate() { final PlainActionFuture installTemplate = new PlainActionFuture<>(); - serviceProviderIndex.installIndexTemplate(installTemplate); + serviceProviderIndex.installIndexTemplate(assertListenerIsOnlyCalledOnce(installTemplate)); return installTemplate.actionGet(); } private Set getAllDocs() { final PlainActionFuture> future = new PlainActionFuture<>(); - serviceProviderIndex.findAll(ActionListener.wrap( + serviceProviderIndex.findAll(assertListenerIsOnlyCalledOnce(ActionListener.wrap( set -> future.onResponse(set.stream().map(doc -> doc.document.get()) .collect(Collectors.collectingAndThen(Collectors.toSet(), Collections::unmodifiableSet))), future::onFailure - )); + ))); return future.actionGet(); } private SamlServiceProviderDocument readDocument(String docId) { final PlainActionFuture future = new PlainActionFuture<>(); - serviceProviderIndex.readDocument(docId, future); + serviceProviderIndex.readDocument(docId, assertListenerIsOnlyCalledOnce(future)); final SamlServiceProviderIndex.DocumentSupplier supplier = future.actionGet(); return supplier == null ? null : supplier.getDocument(); } private void writeDocument(SamlServiceProviderDocument doc) { final PlainActionFuture future = new PlainActionFuture<>(); - serviceProviderIndex.writeDocument(doc, DocWriteRequest.OpType.INDEX, WriteRequest.RefreshPolicy.WAIT_UNTIL, future); + serviceProviderIndex.writeDocument(doc, DocWriteRequest.OpType.INDEX, WriteRequest.RefreshPolicy.WAIT_UNTIL, + assertListenerIsOnlyCalledOnce(future)); doc.setDocId(future.actionGet().getId()); } private DeleteResponse deleteDocument(SamlServiceProviderDocument doc) { final PlainActionFuture future = new PlainActionFuture<>(); - serviceProviderIndex.readDocument(doc.docId, ActionListener.wrap( + serviceProviderIndex.readDocument(doc.docId, assertListenerIsOnlyCalledOnce(ActionListener.wrap( info -> serviceProviderIndex.deleteDocument(info.version, WriteRequest.RefreshPolicy.IMMEDIATE, future), - future::onFailure)); + future::onFailure))); return future.actionGet(); } @@ -236,17 +232,17 @@ public class SamlServiceProviderIndexTests extends ESSingleNodeTestCase { private Set findAllByEntityId(String entityId) { final PlainActionFuture> future = new PlainActionFuture<>(); - serviceProviderIndex.findByEntityId(entityId, ActionListener.wrap( + serviceProviderIndex.findByEntityId(entityId, assertListenerIsOnlyCalledOnce(ActionListener.wrap( set -> future.onResponse(set.stream().map(doc -> doc.document.get()) .collect(Collectors.collectingAndThen(Collectors.toSet(), Collections::unmodifiableSet))), future::onFailure - )); + ))); return future.actionGet(); } private void refresh() { PlainActionFuture future = new PlainActionFuture<>(); - serviceProviderIndex.refresh(future); + serviceProviderIndex.refresh(assertListenerIsOnlyCalledOnce(future)); future.actionGet(); } @@ -303,4 +299,13 @@ public class SamlServiceProviderIndexTests extends ESSingleNodeTestCase { + randomAlphaOfLengthBetween(4, 8) + "." + randomAlphaOfLengthBetween(2, 4) + "/"; } + private static ActionListener assertListenerIsOnlyCalledOnce(ActionListener delegate) { + final AtomicInteger callCount = new AtomicInteger(0); + return ActionListener.runBefore(delegate, () -> { + if (callCount.incrementAndGet() != 1) { + fail("Listener was called twice"); + } + }); + } + }