fix: add external saml groups with empty whitelist (#3061)

To be consistent with external OIDC logins, we should add external SAML groups if the whitelist is empty. Change-Id: I0981440b1986c6ff107ea39d7e141b6eb6b683e5 Co-authored-by: d036670 <[email protected]>
cloudfoundry · Sep 26, 2024 · be32b11 · be32b11
1 parent c034b9a
commit be32b11
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 1 deletion.
diff --git a/...ain/java/org/cloudfoundry/identity/uaa/provider/saml/LoginSamlAuthenticationProvider.java b/...ain/java/org/cloudfoundry/identity/uaa/provider/saml/LoginSamlAuthenticationProvider.java
@@ -215,6 +215,9 @@ protected void publish(ApplicationEvent event) {
     protected Set<String> filterSamlAuthorities(SamlIdentityProviderDefinition definition, Collection<? extends GrantedAuthority> samlAuthorities) {
         List<String> whiteList = of(definition.getExternalGroupsWhitelist()).orElse(Collections.EMPTY_LIST);
         Set<String> authorities = samlAuthorities.stream().map(s -> s.getAuthority()).collect(Collectors.toSet());
+        if (whiteList.isEmpty()) {
+            return authorities;
+        }
         Set<String> result = retainAllMatches(authorities, whiteList);
         logger.debug(String.format("White listed external SAML groups:'%s'", result));
         return result;

diff --git a/...ava/org/cloudfoundry/identity/uaa/provider/saml/LoginSamlAuthenticationProviderTests.java b/...ava/org/cloudfoundry/identity/uaa/provider/saml/LoginSamlAuthenticationProviderTests.java
@@ -139,6 +139,7 @@ class LoginSamlAuthenticationProviderTests {
     private static final String SAML_ADMIN = "saml.admin";
     private static final String SAML_TEST = "saml.test";
     private static final String SAML_NOT_MAPPED = "saml.unmapped";
+    private static final String SAML_NOT_ASSERTED = "saml.unasserted";
     private static final String UAA_USER = "uaa.user";
     private static final String UAA_SAML_USER = "uaa.saml.user";
     private static final String UAA_SAML_ADMIN = "uaa.saml.admin";
@@ -428,14 +429,25 @@ void test_group_attribute_not_set() {
     }
 
     @Test
-    void dontAdd_external_groups_to_authentication_without_whitelist() {
+    void dontAdd_external_groups_to_authentication_without_matching_whitelist() {
         providerDefinition.addAttributeMapping(GROUP_ATTRIBUTE_NAME, "groups");
+        providerDefinition.addWhiteListedGroup(SAML_NOT_ASSERTED);
         provider.setConfig(providerDefinition);
         providerProvisioning.update(provider, identityZoneManager.getCurrentIdentityZone().getId());
 
         UaaAuthentication authentication = getAuthentication(authprovider);
         assertEquals(Collections.EMPTY_SET, authentication.getExternalGroups());
     }
+
+    @Test
+    void add_external_groups_to_authentication_with_empty_whitelist() {
+        providerDefinition.addAttributeMapping(GROUP_ATTRIBUTE_NAME, "groups");
+        provider.setConfig(providerDefinition);
+        providerProvisioning.update(provider, identityZoneManager.getCurrentIdentityZone().getId());
+
+        UaaAuthentication authentication = getAuthentication(authprovider);
+        assertThat(authentication.getExternalGroups(), containsInAnyOrder(SAML_USER, SAML_ADMIN, SAML_NOT_MAPPED));
+    }
 
     @Test
     void add_external_groups_to_authentication_with_whitelist() {