gplazma: ldap throw exception if no principal is added

Motivation: The gPlazma Mapping plugin contract is that a mapping plugin should either add (at least) one principal or it has failed. A failed mapping plugin throws an AuthenticationException to indicate why it failed. Currently, the ldap plugin never throws an AuthenticationException, even when it has added no principals. This makes it hard to use the ldap plugin with other mapping plugins. Modification: Update plugin to throw the expected exception if it fails to add any principals. The exception's message provides a short explanation on why the mapping request failed. Result: The LDAP plugin behaviuor now more closely follows that of other mapping plugins. This allows deployments where LDAP is tried first and, if that fails to identify the user, fall-back strategies are used. Target: master Request: 7.0 Request: 6.2 Request: 6.1 Request: 6.1 Request: 6.0 Request: 5.2 Requires-notes: yes Requires-book: no Patch: https://rb.dcache.org/r/13000/ Acked-by: Albert Rossi Acked-by: Tigran Mkrtchyan Acked-by: Lea Morschel
dCache · May 5, 2021 · 9e9125c · 9e9125c
1 parent e608775
commit 9e9125c
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 35 deletions.
diff --git a/modules/gplazma2-ldap/src/main/java/org/dcache/gplazma/plugins/Ldap.java b/modules/gplazma2-ldap/src/main/java/org/dcache/gplazma/plugins/Ldap.java
@@ -421,47 +421,53 @@ public void map(Set<Principal> principals) throws AuthenticationException {
         boolean isUsernameMissing = false;
 
         Optional<Principal> principal = findFirst(principals, UserNamePrincipal.class::isInstance);
-        if (!principal.isPresent() && tryUidMapping) {
+
+        if (principal.isEmpty()) {
+            checkAuthentication(tryUidMapping, "no username");
+
             principal = findFirst(principals, UidPrincipal.class::isInstance);
+            checkAuthentication(principal.isPresent(), "no username or uid");
+
             filter = "(uidNumber=%s)";
             isUsernameMissing = true;
         }
 
-        if (principal.isPresent()) {
+        assert principal.isPresent();
 
-            //REVISIT: if we query LDAP server to user record, then we probably have to respect the provided primary GID
-            boolean hasPrimaryGid = principals.stream()
-                    .filter(GidPrincipal.class::isInstance)
-                    .map(GidPrincipal.class::cast)
-                    .anyMatch(GidPrincipal::isPrimaryGroup);
+        //REVISIT: if we query LDAP server to user record, then we probably have to respect the provided primary GID
+        boolean hasPrimaryGid = principals.stream()
+                .filter(GidPrincipal.class::isInstance)
+                .map(GidPrincipal.class::cast)
+                .anyMatch(GidPrincipal::isPrimaryGroup);
 
-            try (AutoCloseableLdapContext ctx = new AutoCloseableLdapContext()) {
-                NamingEnumeration<SearchResult> sResult = ctx.search(peopleOU,
-                        String.format(filter, principal.get().getName()),
-                        SC_UID_GID_NUMBER);
+        try (AutoCloseableLdapContext ctx = new AutoCloseableLdapContext()) {
+            NamingEnumeration<SearchResult> sResult = ctx.search(peopleOU,
+                    String.format(filter, principal.get().getName()),
+                    SC_UID_GID_NUMBER);
 
-                try {
-                    if (sResult.hasMore()) {
-                        Attributes userAttr = sResult.next().getAttributes();
-
-                        Principal usernamePrincipal;
-                        if (isUsernameMissing) {
-                            usernamePrincipal = new UserNamePrincipal((String) userAttr.get(USER_ID_ATTRIBUTE).get());
-                            principals.add(usernamePrincipal);
-                        } else {
-                            usernamePrincipal = principal.get();
-                            principals.add(new UidPrincipal((String) userAttr.get(UID_NUMBER_ATTRIBUTE).get()));
-                        }
-
-                        principals.add(new GidPrincipal((String) userAttr.get(GID_NUMBER_ATTRIBUTE).get(), !hasPrimaryGid));
-                        principals.addAll(getGroupsByUid.searchGroup(ctx, usernamePrincipal, peopleOU, groupOU));
-                    }
-                } finally {
-                    sResult.close();
+            try {
+                checkAuthentication(sResult.hasMore(), "unknown %s",
+                        isUsernameMissing ? "uid" : "username");
+
+                Attributes userAttr = sResult.next().getAttributes();
+
+                Principal usernamePrincipal;
+                if (isUsernameMissing) {
+                    usernamePrincipal = new UserNamePrincipal((String) userAttr.get(USER_ID_ATTRIBUTE).get());
+                    principals.add(usernamePrincipal);
+                } else {
+                    usernamePrincipal = principal.get();
+                    principals.add(new UidPrincipal((String) userAttr.get(UID_NUMBER_ATTRIBUTE).get()));
                 }
-            } catch (NamingException e) {
-                LOGGER.warn("Failed to get mapping: {}", e.toString());
+
+                principals.add(new GidPrincipal((String) userAttr.get(GID_NUMBER_ATTRIBUTE).get(), !hasPrimaryGid));
+                principals.addAll(getGroupsByUid.searchGroup(ctx, usernamePrincipal, peopleOU, groupOU));
+            } finally {
+                sResult.close();
             }
+        } catch (NamingException e) {
+            LOGGER.warn("Failed to get mapping: {}", e.toString());
+            throw new AuthenticationException("problem with LDAP server");
         }
     }
 

diff --git a/modules/gplazma2-ldap/src/test/java/org/dcache/gplazma/plugins/LdapTest.java b/modules/gplazma2-ldap/src/test/java/org/dcache/gplazma/plugins/LdapTest.java
@@ -141,12 +141,10 @@ public void shouldReturnMatchingUidGidWithExistingPrimaryGid() throws Authentica
         assertThat("expected GID not found", principals, hasItem(ACTOR_GID_PRINCIPAL));
     }
 
-    @Test
-    public void shouldDoNothingForNonExisting() throws AuthenticationException {
+    @Test(expected=AuthenticationException.class)
+    public void shouldThrowExceptionForNonExisting() throws AuthenticationException {
         Set<Principal> principals = Sets.newHashSet(NON_EXISTING_PRINCIPAL);
         plugin.map(principals);
-        assertThat("unexpected number of returned principals", principals, hasSize(1));
-        assertThat("expected USERNAME not found", principals, hasItem(NON_EXISTING_PRINCIPAL));
     }
 
     @Test