Spring Security
  1. Spring Security
  2. SEC-1371

LdapUserDetailsManager - createUser method should not wipe out all Authorities before bind

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 3.0.0
    • Fix Version/s: 3.0.2
    • Component/s: LDAP
    • Labels:
      None

      Description

      We have run across an issue when attempting to create a new user.

      First, all authorities are wiped out for the user, THEN the bind is attempted.

      If the bind fails because the user already exists, this leaves us with a user with no authorities.

      Here is the code in LdapUserDetailsManager:

      public void createUser(UserDetails user) {
      DirContextAdapter ctx = new DirContextAdapter();
      copyToContext(user, ctx);
      DistinguishedName dn = usernameMapper.buildDn(user.getUsername());
      // Check for any existing authorities which might be set for this DN
      GrantedAuthority[] authorities = getUserAuthorities(dn, user.getUsername());

      if (authorities.length > 0)

      { removeAuthorities(dn, authorities); }

      logger.debug("Creating new user '"+ user.getUsername() + "' with DN '" + dn + "'");

      template.bind(dn, ctx, null);

      addAuthorities(dn, user.getAuthorities());
      }

      Would there be any harm in moving the removeAuthorities call after the template.bind call ?

        Activity

        Hide
        Luke Taylor added a comment -

        The userExists() method is there to allow you to check whether an account already exists prior to attempting to create it, so you would typically use this as part of your user creation workflow.

        It still makes more sense to attempt the bind first though.

        Show
        Luke Taylor added a comment - The userExists() method is there to allow you to check whether an account already exists prior to attempting to create it, so you would typically use this as part of your user creation workflow. It still makes more sense to attempt the bind first though.
        Hide
        Luke Taylor added a comment -

        OK, I've made the change. You should probably still delete an existing user and their authorities before creating one with the same name though.

        Show
        Luke Taylor added a comment - OK, I've made the change. You should probably still delete an existing user and their authorities before creating one with the same name though.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Kenny West
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: