Fix SAML private attributes predicate

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlPrivateAttributePredicate.java

@@ -0,0 +1,103 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.security.authc.saml;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.elasticsearch.xpack.core.security.authc.RealmConfig;
+import org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings;
+import org.elasticsearch.xpack.security.authc.saml.SamlAttributes.SamlPrivateAttribute;
+import org.opensaml.saml.saml2.core.Attribute;
+
+import java.util.List;
+import java.util.Set;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+
+import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRIVATE_ATTRIBUTES;
+
+/**
+ * The predicate which is constructed based on the values of {@link SamlRealmSettings#PRIVATE_ATTRIBUTES} setting.
+ * When the setting is configured, the attributes whose {@code Attribute#getName} or {@code Attribute#getFriendlyName} match,
+ * will be treated as private ({@link SamlPrivateAttribute}).
+ */
+class SamlPrivateAttributePredicate implements Predicate<Attribute> {
+
+    private static final Logger logger = LogManager.getLogger(SamlPrivateAttributePredicate.class);
+
+    private static final Predicate<Attribute> MATCH_NONE = new Predicate<Attribute>() {
+        @Override
+        public boolean test(Attribute attribute) {
+            return false;
+        }
+
+        @Override
+        public String toString() {
+            return "<matching no SAML private attributes>";
+        }
+    };
+
+    private final Predicate<Attribute> predicate;
+
+    SamlPrivateAttributePredicate(RealmConfig config) {
+        this.predicate = buildPrivateAttributesPredicate(config);
+    }
+
+    private static Predicate<Attribute> buildPrivateAttributesPredicate(RealmConfig config) {
+
+        if (false == config.hasSetting(PRIVATE_ATTRIBUTES)) {
+            logger.trace("No SAML private attributes setting configured.");
+            return MATCH_NONE;
+        }
+
+        final List<String> attributesList = config.getSetting(PRIVATE_ATTRIBUTES);
+        if (attributesList == null || attributesList.isEmpty()) {
+            logger.trace("No SAML private attributes configured for setting [{}].", PRIVATE_ATTRIBUTES);
+            return MATCH_NONE;
+        }
+
+        final Set<String> attributesSet = attributesList.stream()
+            .filter(name -> name != null && false == name.isBlank())
+            .collect(Collectors.toUnmodifiableSet());
+
+        if (attributesSet.isEmpty()) {
+            return MATCH_NONE;
+        }
+
+        logger.trace("SAML private attributes configured: {}", attributesSet);
+        return new Predicate<>() {
+
+            @Override
+            public boolean test(Attribute attribute) {
+                if (attribute == null) {
+                    return false;
+                }
+                if (attribute.getName() != null && attributesSet.contains(attribute.getName())) {
+                    return true;
+                }
+                return attribute.getFriendlyName() != null && attributesSet.contains(attribute.getFriendlyName());
+            }
+
+            @Override
+            public String toString() {
+                return "<matching " + attributesSet + " SAML private attributes>";
+            }
+        };
+    }
+
+    @Override
+    public boolean test(Attribute attribute) {
+        return predicate.test(attribute);
+    }
+
+    @Override
+    public String toString() {
+        return this.getClass().getSimpleName() + " {predicate=" + predicate + "}";
+    }
+
+}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java

@@ -135,7 +135,6 @@
 import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.NAME_ATTRIBUTE;
 import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.POPULATE_USER_METADATA;
 import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRINCIPAL_ATTRIBUTE;
-import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRIVATE_ATTRIBUTES;
 import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.SIGNING_KEY_ALIAS;
 import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.SIGNING_MESSAGE_TYPES;
 import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.SIGNING_SETTING_KEY;
@@ -223,13 +222,13 @@ public static SamlRealm create(
         final Clock clock = Clock.systemUTC();
         final IdpConfiguration idpConfiguration = getIdpConfiguration(config, metadataResolver, idpDescriptor);
         final TimeValue maxSkew = config.getSetting(CLOCK_SKEW);
-        final Predicate<Attribute> secureAttributePredicate = secureAttributePredicate(config);
+        final Predicate<Attribute> privateAttributePredicate = new SamlPrivateAttributePredicate(config);
         final SamlAuthenticator authenticator = new SamlAuthenticator(
             clock,
             idpConfiguration,
             serviceProvider,
             maxSkew,
-            secureAttributePredicate
+            privateAttributePredicate
         );
         final SamlLogoutRequestHandler logoutHandler = new SamlLogoutRequestHandler(clock, idpConfiguration, serviceProvider, maxSkew);
         final SamlLogoutResponseHandler logoutResponseHandler = new SamlLogoutResponseHandler(
@@ -258,20 +257,6 @@ public static SamlRealm create(
         return realm;
     }
 
-    static Predicate<Attribute> secureAttributePredicate(RealmConfig config) {
-        if (false == config.hasSetting(PRIVATE_ATTRIBUTES)) {
-            return attribute -> false;
-        }
-        final List<String> secureAttributeNames = config.getSetting(PRIVATE_ATTRIBUTES);
-        if (secureAttributeNames == null || secureAttributeNames.isEmpty()) {
-            return attribute -> false;
-        }
-
-        final Set<String> secureAttributeNamesSet = Set.copyOf(secureAttributeNames);
-        return attribute -> attribute != null
-            && (secureAttributeNamesSet.contains(attribute.getName()) || secureAttributeNamesSet.contains(attribute.getFriendlyName()));
-    }
-
     public SpConfiguration getServiceProvider() {
         return serviceProvider;
     }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlSecureAttributePredicateTests.java

@@ -0,0 +1,77 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.security.authc.saml;
+
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.core.security.authc.RealmConfig;
+import org.mockito.Mockito;
+import org.opensaml.saml.saml2.core.Attribute;
+
+import java.util.List;
+
+import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRIVATE_ATTRIBUTES;
+import static org.hamcrest.Matchers.is;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class SamlSecureAttributePredicateTests extends ESTestCase {
+
+    public void testPredicateWithSettingConfigured() {
+
+        final List<String> privateAttributes = List.of("private", "http://elastic.co/confidential");
+        final RealmConfig config = realmConfig(privateAttributes);
+        final SamlPrivateAttributePredicate predicate = new SamlPrivateAttributePredicate(config);
+
+        final String privateAttribute = randomFrom(privateAttributes);
+        final String nonPrivateAttribute = randomFrom(new String[] { null, " ", randomAlphaOfLengthBetween(0, 3) });
+
+        assertThat(predicate.test(attribute("private", "http://elastic.co/confidential")), is(true));
+        assertThat(predicate.test(attribute(privateAttribute, nonPrivateAttribute)), is(true));
+        assertThat(predicate.test(attribute(nonPrivateAttribute, privateAttribute)), is(true));
+
+        assertThat(predicate.test(attribute(privateAttribute, null)), is(true));
+        assertThat(predicate.test(attribute(null, privateAttribute)), is(true));
+
+        assertThat(predicate.test(attribute(nonPrivateAttribute, null)), is(false));
+        assertThat(predicate.test(attribute(null, nonPrivateAttribute)), is(false));
+        assertThat(predicate.test(attribute(null, null)), is(false));
+
+        assertThat(predicate.test(attribute("something", "else")), is(false));
+        assertThat(predicate.test(attribute("", "")), is(false));
+
+    }
+
+    public void testPredicateWhenSettingIsNotConfigured() {
+
+        List<String> privateAttributes = randomBoolean() ? List.of() : null;
+        RealmConfig config = realmConfig(privateAttributes);
+        SamlPrivateAttributePredicate predicate = new SamlPrivateAttributePredicate(config);
+
+        String name = randomFrom(randomAlphaOfLengthBetween(0, 5), null);
+        String friendlyName = randomFrom(randomAlphaOfLengthBetween(0, 5), null);
+
+        assertThat(predicate.test(attribute(name, friendlyName)), is(false));
+
+    }
+
+    private static Attribute attribute(String name, String friendlyName) {
+        Attribute attribute = mock(Attribute.class);
+        when(attribute.getName()).thenReturn(name);
+        when(attribute.getFriendlyName()).thenReturn(friendlyName);
+        return attribute;
+    }
+
+    private static RealmConfig realmConfig(List<String> privateAttributeNames) {
+        RealmConfig config = Mockito.mock(RealmConfig.class);
+        when(config.hasSetting(PRIVATE_ATTRIBUTES)).thenReturn(privateAttributeNames != null);
+        doReturn(privateAttributeNames).when(config).getSetting(PRIVATE_ATTRIBUTES);
+        return config;
+    }
+
+}