Improve cache invalidation in IdP SP cache

tvernum · tvernum · commit d9158d5e7fe0 · 2025-06-06T14:46:01.000+10:00
The Identity Provider's Service Provider cache had two issues: 1. It checked for identity based on sequence numbers, but didn't include the `seq_no_primary_term` parameter on searches, which means the sequence would always by `-2` 2. It didn't track whether the index was deleted, which means it could be caching values from an old version of the index This commit fixes both of these issues. In practice neither issue was a problem because there are no deployments that use index-based service providers, however the 2nd issue did cause some challenges for testing. Backport of: elastic#128890
diff --git a/docs/changelog/128890.yaml b/docs/changelog/128890.yaml
@@ -0,0 +1,5 @@
+pr: 128890
+summary: Improve cache invalidation in IdP SP cache
+area: IdentityProvider
+type: bug
+issues: []
diff --git a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java
@@ -35,6 +35,7 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
 
 public class IdentityProviderAuthenticationIT extends IdpRestTestCase {
 
@@ -74,6 +75,52 @@ public void testRegistrationAndIdpInitiatedSso() throws Exception {
         authenticateWithSamlResponse(samlResponse, null);
     }
 
+    public void testUpdateExistingServiceProvider() throws Exception {
+        final Map<String, Object> request1 = Map.ofEntries(
+            Map.entry("name", "Test SP [v1]"),
+            Map.entry("acs", SP_ACS),
+            Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))),
+            Map.entry(
+                "attributes",
+                Map.ofEntries(
+                    Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/principal"),
+                    Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
+                    Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
+                    Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
+                )
+            )
+        );
+        final SamlServiceProviderIndex.DocumentVersion docVersion1 = createServiceProvider(SP_ENTITY_ID, request1);
+        checkIndexDoc(docVersion1);
+        ensureGreen(SamlServiceProviderIndex.INDEX_NAME);
+
+        final String samlResponse1 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null);
+        assertThat(samlResponse1, containsString("https://idp.test.es.elasticsearch.org/attribute/principal"));
+        assertThat(samlResponse1, not(containsString("https://idp.test.es.elasticsearch.org/attribute/username")));
+
+        final Map<String, Object> request = Map.ofEntries(
+            Map.entry("name", "Test SP [v2]"),
+            Map.entry("acs", SP_ACS),
+            Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))),
+            Map.entry(
+                "attributes",
+                Map.ofEntries(
+                    Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/username"),
+                    Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
+                    Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
+                    Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
+                )
+            )
+        );
+        final SamlServiceProviderIndex.DocumentVersion docVersion2 = createServiceProvider(SP_ENTITY_ID, request);
+        checkIndexDoc(docVersion2);
+        ensureGreen(SamlServiceProviderIndex.INDEX_NAME);
+
+        final String samlResponse2 = generateSamlResponse(SP_ENTITY_ID, SP_ACS, null);
+        assertThat(samlResponse2, containsString("https://idp.test.es.elasticsearch.org/attribute/username"));
+        assertThat(samlResponse2, not(containsString("https://idp.test.es.elasticsearch.org/attribute/principal")));
+    }
+
     public void testRegistrationAndSpInitiatedSso() throws Exception {
         final Map<String, Object> request = Map.ofEntries(
             Map.entry("name", "Test SP"),
diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/IdentityProviderPlugin.java
@@ -108,6 +108,8 @@ public Collection<?> createComponents(PluginServices services) {
             index,
             serviceProviderFactory
         );
+        services.clusterService().addListener(registeredServiceProviderResolver);
+
         final WildcardServiceProviderResolver wildcardServiceProviderResolver = WildcardServiceProviderResolver.create(
             services.environment(),
             services.resourceWatcherService(),
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
@@ -33,6 +33,7 @@
 import org.elasticsearch.common.util.CachedSupplier;
 import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
 import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.index.get.GetResult;
 import org.elasticsearch.index.query.QueryBuilder;
@@ -51,6 +52,7 @@
 import java.util.Arrays;
 import java.util.Objects;
 import java.util.Set;
+import java.util.SortedMap;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -152,6 +154,20 @@ private void checkForAliasStateChange(ClusterState state) {
         }
     }
 
+    Index getIndex(ClusterState state) {
+        final SortedMap<String, IndexAbstraction> indicesLookup = state.metadata().getIndicesLookup();
+
+        IndexAbstraction indexAbstraction = indicesLookup.get(ALIAS_NAME);
+        if (indexAbstraction == null) {
+            indexAbstraction = indicesLookup.get(INDEX_NAME);
+        }
+        if (indexAbstraction == null) {
+            return null;
+        } else {
+            return indexAbstraction.getWriteIndex();
+        }
+    }
+
     @Override
     public void close() {
         logger.debug("Closing ... removing cluster state listener");
@@ -255,7 +271,12 @@ public void refresh(ActionListener<Void> listener) {
 
     private void findDocuments(QueryBuilder query, ActionListener<Set<DocumentSupplier>> listener) {
         logger.trace("Searching [{}] for [{}]", ALIAS_NAME, query);
-        final SearchRequest request = client.prepareSearch(ALIAS_NAME).setQuery(query).setSize(1000).setFetchSource(true).request();
+        final SearchRequest request = client.prepareSearch(ALIAS_NAME)
+            .setQuery(query)
+            .setSize(1000)
+            .setFetchSource(true)
+            .seqNoAndPrimaryTerm(true)
+            .request();
         client.search(request, ActionListener.wrap(response -> {
             if (logger.isTraceEnabled()) {
                 logger.trace("Search hits: [{}] [{}]", response.getHits().getTotalHits(), Arrays.toString(response.getHits().getHits()));
diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolver.java
@@ -7,16 +7,22 @@
 
 package org.elasticsearch.xpack.idp.saml.sp;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.cluster.ClusterChangedEvent;
+import org.elasticsearch.cluster.ClusterStateListener;
 import org.elasticsearch.common.cache.Cache;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.iterable.Iterables;
+import org.elasticsearch.index.Index;
 import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentSupplier;
 import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentVersion;
 
+import java.util.Objects;
 import java.util.stream.Collectors;
 
-public class SamlServiceProviderResolver {
+public class SamlServiceProviderResolver implements ClusterStateListener {
 
     private final Cache<String, CachedServiceProvider> cache;
     private final SamlServiceProviderIndex index;
@@ -32,6 +38,8 @@ public SamlServiceProviderResolver(
         this.serviceProviderFactory = serviceProviderFactory;
     }
 
+    private final Logger logger = LogManager.getLogger(getClass());
+
     /**
      * Find a {@link SamlServiceProvider} by entity-id.
      *
@@ -75,6 +83,16 @@ private void populateCacheAndReturn(String entityId, DocumentSupplier doc, Actio
         listener.onResponse(serviceProvider);
     }
 
+    @Override
+    public void clusterChanged(ClusterChangedEvent event) {
+        final Index previousIndex = index.getIndex(event.previousState());
+        final Index currentIndex = index.getIndex(event.state());
+        if (Objects.equals(previousIndex, currentIndex) == false) {
+            logger.info("Index has changed [{}] => [{}], clearing cache", previousIndex, currentIndex);
+            this.cache.invalidateAll();
+        }
+    }
+
     private class CachedServiceProvider {
         private final String entityId;
         private final DocumentVersion documentVersion;
diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderResolverTests.java
@@ -9,7 +9,11 @@
 
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.support.PlainActionFuture;
+import org.elasticsearch.cluster.ClusterChangedEvent;
+import org.elasticsearch.cluster.ClusterName;
+import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.index.Index;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xpack.idp.saml.idp.SamlIdentityProvider;
 import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex.DocumentVersion;
@@ -135,6 +139,37 @@ public void testResolveIgnoresCacheWhenDocumentVersionChanges() throws Exception
         assertThat(serviceProvider2.getPrivileges().getResource(), equalTo(document2.privileges.resource));
     }
 
+    public void testCacheIsClearedWhenIndexChanges() throws Exception {
+        final SamlServiceProviderDocument document1 = SamlServiceProviderTestUtils.randomDocument(1);
+        final SamlServiceProviderDocument document2 = SamlServiceProviderTestUtils.randomDocument(2);
+        document2.entityId = document1.entityId;
+
+        final DocumentVersion docVersion = new DocumentVersion(randomAlphaOfLength(12), 1, 1);
+
+        mockDocument(document1.entityId, docVersion, document1);
+        final SamlServiceProvider serviceProvider1a = resolveServiceProvider(document1.entityId);
+        final SamlServiceProvider serviceProvider1b = resolveServiceProvider(document1.entityId);
+        assertThat(serviceProvider1b, sameInstance(serviceProvider1a));
+
+        final ClusterState oldState = ClusterState.builder(ClusterName.DEFAULT).build();
+        final ClusterState newState = ClusterState.builder(ClusterName.DEFAULT).build();
+        when(index.getIndex(oldState)).thenReturn(new Index(SamlServiceProviderIndex.INDEX_NAME, randomUUID()));
+        when(index.getIndex(newState)).thenReturn(new Index(SamlServiceProviderIndex.INDEX_NAME, randomUUID()));
+        resolver.clusterChanged(new ClusterChangedEvent(getTestName(), newState, oldState));
+
+        mockDocument(document1.entityId, docVersion, document2);
+        final SamlServiceProvider serviceProvider2 = resolveServiceProvider(document1.entityId);
+
+        assertThat(serviceProvider2, not(sameInstance(serviceProvider1a)));
+        assertThat(serviceProvider2.getEntityId(), equalTo(document2.entityId));
+        assertThat(serviceProvider2.getAssertionConsumerService().toString(), equalTo(document2.acs));
+        assertThat(serviceProvider2.getAttributeNames().principal, equalTo(document2.attributeNames.principal));
+        assertThat(serviceProvider2.getAttributeNames().name, equalTo(document2.attributeNames.name));
+        assertThat(serviceProvider2.getAttributeNames().email, equalTo(document2.attributeNames.email));
+        assertThat(serviceProvider2.getAttributeNames().roles, equalTo(document2.attributeNames.roles));
+        assertThat(serviceProvider2.getPrivileges().getResource(), equalTo(document2.privileges.resource));
+    }
+
     private SamlServiceProvider resolveServiceProvider(String entityId) {
         final PlainActionFuture<SamlServiceProvider> future = new PlainActionFuture<>();
         resolver.resolve(entityId, future);
