Spring Security
  1. Spring Security
  2. SEC-1641

DefaultLdapAuthoritiesPopulator does not accept groupSearchBase as null

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: None
    • Fix Version/s: 3.1.0.M2, 3.0.6
    • Component/s: Docs and Website, LDAP
    • Labels:
      None
    • Environment:
      using spring 3.0.3

      Description

      The DefaultLdapAuthoritiesPopulator Javadocs indicate a null groupSearchBase means that no searching will be performed. The implementation code, in particular getGroupMembershipRoles(), looks like it would handle that just fine, except there is an Assert.notNull in the groupSearchBase setter function. Being able to set the groupSearchBase to null is handy in situations where you don't really want to search but want to assign a defaultRole to all users. If the Assert goes away, one could do that very nicely with this class.

        Activity

        Hide
        Luke Taylor added a comment -

        I've inlined the setter method into the constructor, removing the null check. In 3.1 the preferred approach for creating a default role, converting case, adding prefixes etc. will be to use a GrantedAuthoritiesMapper, so the equivalent functinality in other classes wil be deprecated. I've updated the DefaultLdapAuthoritiesPopulator to indicate this.

        Show
        Luke Taylor added a comment - I've inlined the setter method into the constructor, removing the null check. In 3.1 the preferred approach for creating a default role, converting case, adding prefixes etc. will be to use a GrantedAuthoritiesMapper, so the equivalent functinality in other classes wil be deprecated. I've updated the DefaultLdapAuthoritiesPopulator to indicate this.
        Hide
        Michael Grove added a comment -

        Thanks Luke. I think I see a problem though in the update. Won't the below result in a NPE if groupSearchBase is null?

        this.groupSearchBase = groupSearchBase;
        if (groupSearchBase.length() == 0)

        { logger.info("groupSearchBase is empty. Searches will be performed from the context source base"); }
        Show
        Michael Grove added a comment - Thanks Luke. I think I see a problem though in the update. Won't the below result in a NPE if groupSearchBase is null? this.groupSearchBase = groupSearchBase; if (groupSearchBase.length() == 0) { logger.info("groupSearchBase is empty. Searches will be performed from the context source base"); }
        Hide
        Luke Taylor added a comment -

        Well spotted. That's what happens when you don't ensure a test fails before it passes . It also needs to return a mutable empty set in the case of a null search base. I've committed a fix that hopefully works this time.

        Show
        Luke Taylor added a comment - Well spotted. That's what happens when you don't ensure a test fails before it passes . It also needs to return a mutable empty set in the case of a null search base. I've committed a fix that hopefully works this time.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Michael Grove
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: