[HOTFIX] Strip quotes from LDAP role names and fix defaultRole example

zeppelin-server/src/main/java/org/apache/zeppelin/realm/RoleMappingLdapRealm.java

@@ -33,11 +33,15 @@
 
 /**
  * RoleMappingLdapRealm is a simple LDAP realm that returns configured role for specific users.
- * Multiple roles(quoted string, comma separated) can be specified for a user.
+ * Multiple roles (comma separated) can be specified for a user.
  * Example configurations:
  * ldapRealm = org.apache.zeppelin.realm.RoleMappingLdapRealm
  * ldapRealm.rolesByUsername = "user1":"admin","user2":"role1,role2","user3":"role3"
- * ldapRealm.defaultRole = "role1,role2"
+ * ldapRealm.defaultRole = role1,role2
+ *
+ * <p>Quotes are required around a {@code rolesByUsername} value that contains a comma, because
+ * the map entries themselves are comma separated. Quotes are not needed for {@code defaultRole}
+ * and are stripped if present.
  */
 public class RoleMappingLdapRealm extends DefaultLdapRealm {
 
@@ -79,9 +83,21 @@ public Set<String> getRoleNamesForUser(String username, LdapContext ldapContext,
     String roleString = rolesByUsername.getOrDefault(username, defaultRole);
 
     Set<String> roleNames = Arrays.stream(roleString.split(ROLE_NAMES_DELIMITER))
-        .map(String::trim).collect(Collectors.toSet());
+        .map(RoleMappingLdapRealm::stripQuotes).collect(Collectors.toSet());
 
     LOGGER.debug("For user {}, RoleMappingLdapRealm returns {} as the role", username, roleNames);
     return roleNames;
   }
+
+  // Shiro's Ini parser does not strip quotes for String properties such as defaultRole.
+  private static String stripQuotes(String role) {
+    String stripped = role.trim();
+    if (stripped.startsWith("\"")) {
+      stripped = stripped.substring(1);
+    }
+    if (stripped.endsWith("\"")) {
+      stripped = stripped.substring(0, stripped.length() - 1);
+    }
+    return stripped.trim();
+  }
 }

zeppelin-server/src/test/java/org/apache/zeppelin/realm/RoleMappingLdapRealmTest.java

@@ -18,10 +18,15 @@
  */
 package org.apache.zeppelin.realm;
 
+import org.apache.shiro.config.Ini;
+import org.apache.shiro.config.ogdl.ReflectionBuilder;
 import org.junit.jupiter.api.Test;
 
 import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class RoleMappingLdapRealmTest {
@@ -59,4 +64,51 @@ public void testGetRolesByUsernameWithMultipleRoles() {
     assertTrue(realm.getRoleNamesForUser("user3", null, null).contains("guest1"));
     assertTrue(realm.getRoleNamesForUser("user3", null, null).contains("guest2"));
   }
+
+  @Test
+  public void testDefaultRoleWithQuotes() {
+    RoleMappingLdapRealm realm = new RoleMappingLdapRealm();
+    realm.setDefaultRole("\"guest1,guest2\"");
+
+    Set<String> roleNames = realm.getRoleNamesForUser("user3", null, null);
+
+    assertEquals(Set.of("guest1", "guest2"), roleNames);
+  }
+
+  @Test
+  public void testDefaultRoleWithIndividuallyQuotedRoles() {
+    RoleMappingLdapRealm realm = new RoleMappingLdapRealm();
+    realm.setDefaultRole("\"guest1\",\"guest2\"");
+
+    Set<String> roleNames = realm.getRoleNamesForUser("user3", null, null);
+
+    assertEquals(Set.of("guest1", "guest2"), roleNames);
+  }
+
+  @Test
+  public void testRolesByUsernameWithQuotedRoles() {
+    RoleMappingLdapRealm realm = new RoleMappingLdapRealm();
+    Map<String, String> rolesByUsername = new LinkedHashMap<>();
+    rolesByUsername.put("user1", "\"role1,role2\"");
+    realm.setRolesByUsername(rolesByUsername);
+
+    Set<String> roleNames = realm.getRoleNamesForUser("user1", null, null);
+
+    assertEquals(Set.of("role1", "role2"), roleNames);
+  }
+
+  @Test
+  public void testQuotedRolesParsedFromIni() {
+    Ini ini = new Ini();
+    ini.load("ldapRealm = org.apache.zeppelin.realm.RoleMappingLdapRealm\n"
+        + "ldapRealm.defaultRole = \"role1,role2\"\n"
+        + "ldapRealm.rolesByUsername = \"user1\":\"admin\",\"user2\":\"role1,role2\"\n");
+    Map<String, ?> objects = new ReflectionBuilder().buildObjects(
+        ini.getSection(Ini.DEFAULT_SECTION_NAME));
+    RoleMappingLdapRealm realm = (RoleMappingLdapRealm) objects.get("ldapRealm");
+
+    assertEquals(Set.of("admin"), realm.getRoleNamesForUser("user1", null, null));
+    assertEquals(Set.of("role1", "role2"), realm.getRoleNamesForUser("user2", null, null));
+    assertEquals(Set.of("role1", "role2"), realm.getRoleNamesForUser("user3", null, null));
+  }
 }