Spring Security
  1. Spring Security
  2. SEC-1187

AbstractUserDetailsAuthenticationProvider allows login of disabled, locked or expired users

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 3.0.0 M2
    • Component/s: Core
    • Labels:
      None
    • Environment:
      -

      Description

      If I look into AbstractUserDetailsAuthenticationProvider, I see the followng code:
      try

      { user = retrieveUser(username, (UsernamePasswordAuthenticationToken) authentication); }

      catch (UsernameNotFoundException notFound) {
      if (hideUserNotFoundExceptions)

      { throw new BadCredentialsException(messages.getMessage( "AbstractUserDetailsAuthenticationProvider.badCredentials", "Bad credentials")); }

      else

      { throw notFound; }

      }

      Assert.notNull(user, "retrieveUser returned null - a violation of the interface contract");
      }

      preAuthenticationChecks.check(user);

      try

      { additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); }

      catch (AuthenticationException exception) {
      if (cacheWasUsed)

      { // There was a problem, so try again after checking // we're using latest data (ie not from the cache) cacheWasUsed = false; user = retrieveUser(username, (UsernamePasswordAuthenticationToken) authentication); ### HERE ### additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); }

      else

      { throw exception; }

      }

      Isn't it necessary to execute the preAuthenticationChecks at ### HERE ### again?
      I don't want that disabled, locked or expired users are able to log in.

        Activity

        Hide
        Luke Taylor added a comment - - edited

        The re-loading of the user at this point only occurs if an AuthenticationException has taken place which might imply that a user has changed their password. It won't make any difference if you've disabled their account and they authenticate correctly against the cache. If you have a separate admin application and you want the ability to lock out users, then it must be linked to the cache and remove the user entry when the data is changed. Either that or don't use a cache - you only really need one for stateless applications.

        Show
        Luke Taylor added a comment - - edited The re-loading of the user at this point only occurs if an AuthenticationException has taken place which might imply that a user has changed their password. It won't make any difference if you've disabled their account and they authenticate correctly against the cache. If you have a separate admin application and you want the ability to lock out users, then it must be linked to the cache and remove the user entry when the data is changed. Either that or don't use a cache - you only really need one for stateless applications.
        Hide
        Ingo added a comment -

        Hi, thanks for your comment.
        Of course, the correct way is that a separate admin application cleans the cache.
        You are right, but I could also say that if the password of the user is changed, the password change code has to remove the user in the authentication user cache.
        But the whole "cacheWasUsed" logic means, my cache isn't notified and that's why I re-retrieve the user again. And we don't know what a customized preAuthenticationChecks does. Doesn't exist a single scenario where we have to execute the preAuthenticationChecks again. I'm not 100% sure.

        Show
        Ingo added a comment - Hi, thanks for your comment. Of course, the correct way is that a separate admin application cleans the cache. You are right, but I could also say that if the password of the user is changed, the password change code has to remove the user in the authentication user cache. But the whole "cacheWasUsed" logic means, my cache isn't notified and that's why I re-retrieve the user again. And we don't know what a customized preAuthenticationChecks does. Doesn't exist a single scenario where we have to execute the preAuthenticationChecks again. I'm not 100% sure.
        Hide
        Luke Taylor added a comment -

        You could indeed say that password-changing code ought to clear the cache too, but if it did the cacheWasUsed logic would not be executed, so there would still be no real reason to call the preAuthenticationChecks again in that scenario. It is most likely that the cacheWasUsed logic just pre-dates the status checking of the UserDetails.

        Probably the most sensible thing would be to move the status check into the try/catch block

        try

        { preAuthenticationChecks.check(user); additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); }

        catch (AuthenticationException exception) {

        since it will generally throw an AuthenticationException (AccountStatusException is a subclass of AuthenticationException) and then call the status check as you have suggested.

        That way if the cached version of the UserDetails has an invalid status, but it has actually been cleared, the user will then be able to log in.

        Show
        Luke Taylor added a comment - You could indeed say that password-changing code ought to clear the cache too, but if it did the cacheWasUsed logic would not be executed, so there would still be no real reason to call the preAuthenticationChecks again in that scenario. It is most likely that the cacheWasUsed logic just pre-dates the status checking of the UserDetails. Probably the most sensible thing would be to move the status check into the try/catch block try { preAuthenticationChecks.check(user); additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication); } catch (AuthenticationException exception) { since it will generally throw an AuthenticationException (AccountStatusException is a subclass of AuthenticationException) and then call the status check as you have suggested. That way if the cached version of the UserDetails has an invalid status, but it has actually been cleared, the user will then be able to log in.
        Hide
        Luke Taylor added a comment -

        Not critical.

        Show
        Luke Taylor added a comment - Not critical.
        Hide
        Luke Taylor added a comment -

        Change made as suggested.

        Show
        Luke Taylor added a comment - Change made as suggested.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: