Do not trim SAML roles (#1207)

* Comment out trimRoles * Remove trim() in splitRoles * Remove else statement * Address comments * Change samlRolesSeparator to compiled pattern * Resolve comments, add test for roles_seperator * Check null before pattern.compile, rename pattern variable * Add test cases
opensearch-project · Jun 1, 2021 · 2ecb088 · 2ecb088
1 parent 6a86ae2
commit 2ecb088
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 31 deletions.
diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java b/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java
@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPathExpressionException;
@@ -78,13 +79,13 @@ class AuthTokenProcessorHandler {
     private String jwtRolesKey;
     private String samlSubjectKey;
     private String samlRolesKey;
-    private String samlRolesSeparator;
     private String kibanaRootUrl;
 
     private long expiryOffset = 0;
     private ExpiryBaseValue expiryBaseValue = ExpiryBaseValue.AUTO;
     private JsonWebKey signingKey;
     private JsonMapObjectReaderWriter jsonMapReaderWriter = new JsonMapObjectReaderWriter();
+    private Pattern samlRolesSeparatorPattern;
 
     AuthTokenProcessorHandler(Settings settings, Settings jwtSettings, Saml2SettingsProvider saml2SettingsProvider)
             throws Exception {
@@ -95,8 +96,11 @@ class AuthTokenProcessorHandler {
 
         this.samlRolesKey = settings.get("roles_key");
         this.samlSubjectKey = settings.get("subject_key");
-        this.samlRolesSeparator = settings.get("roles_seperator");
+        String samlRolesSeparator = settings.get("roles_seperator");
         this.kibanaRootUrl = settings.get("kibana_url");
+        if (samlRolesSeparator != null) {
+            this.samlRolesSeparatorPattern = Pattern.compile(samlRolesSeparator);
+        }
 
         if (samlRolesKey == null || samlRolesKey.length() == 0) {
             log.warn("roles_key is not configured, will only extract subject from SAML");
@@ -408,39 +412,18 @@ private String[] extractRoles(SamlResponse samlResponse) throws XPathExpressionE
             return null;
         }
 
-        if (samlRolesSeparator != null) {
+        if (samlRolesSeparatorPattern != null) {
             values = splitRoles(values);
-        } else {
-            values = trimRoles(values);
         }
 
         return values.toArray(new String[values.size()]);
     }
 
     private List<String> splitRoles(List<String> values) {
-        ArrayList<String> result = new ArrayList<String>(values.size() * 5);
-
-        for (String role : values) {
-            if (role != null) {
-                for (String splitRole : role.split(samlRolesSeparator)) {
-                    result.add(splitRole.trim());
-                }
-            }
-        }
-
-        return result;
-    }
-
-    private List<String> trimRoles(List<String> values) {
-        ArrayList<String> result = new ArrayList<>(values);
-
-        for (int i = 0; i < result.size(); i++) {
-            if (result.get(i) != null) {
-                result.set(i, result.get(i).trim());
-            }
-        }
-
-        return result;
+        return values.stream()
+                .flatMap(v -> samlRolesSeparatorPattern.splitAsStream(v))
+                .filter(r -> !Strings.isNullOrEmpty(r))
+                .collect(Collectors.toList());
     }
 
     private String getAbsoluteAcsEndpoint(String acsEndpoint) {

diff --git a/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java b/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java
@@ -371,12 +371,12 @@ public void rolesTest() throws Exception {
         mockSamlIdpServer.setSignResponses(true);
         mockSamlIdpServer.loadSigningKeys("saml/kirk-keystore.jks", "kirk");
         mockSamlIdpServer.setAuthenticateUser("horst");
-        mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("a", "b"));
+        mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("a ,c", "b   ,d,   e", "f", "g,,h, ,i"));
         mockSamlIdpServer.setEndpointQueryString(null);
 
         Settings settings = Settings.builder().put(IDP_METADATA_URL, mockSamlIdpServer.getMetadataUri())
                 .put("kibana_url", "http://wherever").put("idp.entity_id", mockSamlIdpServer.getIdpEntityId())
-                .put("exchange_key", "abc").put("roles_key", "roles").put("path.home", ".").build();
+                .put("exchange_key", "abc").put("roles_key", "roles").put("path.home", ".").put("roles_seperator", ",").build();
 
         HTTPSamlAuthenticator samlAuthenticator = new HTTPSamlAuthenticator(settings, null);
 
@@ -401,7 +401,7 @@ public void rolesTest() throws Exception {
         JwtToken jwt = jwtConsumer.getJwtToken();
 
         Assert.assertEquals("horst", jwt.getClaim("sub"));
-        Assert.assertArrayEquals(new String[] { "a", "b" },
+        Assert.assertArrayEquals(new String[] { "a ", "c", "b   ", "d", "   e", "f", "g", "h", " ", "i" },
                 ((List<String>) jwt.getClaim("roles")).toArray(new String[0]));
     }