From ba7873346ef5739c26a6488baa11458b41ac6d65 Mon Sep 17 00:00:00 2001 From: Paul Millar Date: Tue, 20 Apr 2021 00:16:21 +0200 Subject: [PATCH] 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 --- .../java/org/dcache/gplazma/plugins/Ldap.java | 68 ++++++++++--------- .../org/dcache/gplazma/plugins/LdapTest.java | 6 +- 2 files changed, 39 insertions(+), 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 index 03fef926b2c..0fc6747d457 100644 --- 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 principals) throws AuthenticationException { boolean isUsernameMissing = false; Optional principal = findFirst(principals, UserNamePrincipal.class::isInstance); - if (!principal.isPresent() && tryUidMapping) { + + if (!principal.isPresent()) { + 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 sResult = ctx.search(peopleOU, - String.format(filter, principal.get().getName()), - SC_UID_GID_NUMBER); + try (AutoCloseableLdapContext ctx = new AutoCloseableLdapContext()) { + NamingEnumeration 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 index a27210098d8..669f0893825 100644 --- 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 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