Spring Security
  1. Spring Security
  2. SEC-1949

ProviderManager should not query additional authentication providers when a BadCredentialsException is thrown

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 3.1.0
    • Fix Version/s: 3.1.2
    • Component/s: Core
    • Labels:
      None
    • Environment:
      n/a

      Description

      If a BadCredentialsException is thrown, the provider manager should break its loop and prevent calling further authentication providers.

      Per the javadocs:
      BadCredentialsException - Thrown if an authentication request is rejected because the credentials are invalid.

      There is no point to call additional providers if the credentials are invalid.

      This is somewhat related to SEC-546.

        Activity

        Hide
        Rob Winch added a comment -

        Can I ask how this is causing you a problem. This is intentional behavior so that multiple AuthenticationProviders can be used. For example, the user may not have valid credentials in LDAP (tried first) and do have valid credentials in a database (tried second).

        Show
        Rob Winch added a comment - Can I ask how this is causing you a problem. This is intentional behavior so that multiple AuthenticationProviders can be used. For example, the user may not have valid credentials in LDAP (tried first) and do have valid credentials in a database (tried second).
        Hide
        Phil Krasko added a comment -

        If a bad credentials exception is thrown it means the user was found, is neither locked or disabled, but the provided password is incorrect. Why would you go to another authentication provider? I would expect to try another provider if a UsernameNotFoundException or AuthenticationServiceException was thrown.

        In my scenario I know which provider the user should be authenticating with.
        In each provider I immediately check to see the the user type is supported by the provider. If not, I throw a AuthenticationServiceException to short circuit and skip to the next provider.

        I don't want to continue down the chain of providers when a bad credentials exception is thrown because the provider I selected is the only one the user can authenticate with.

        Show
        Phil Krasko added a comment - If a bad credentials exception is thrown it means the user was found, is neither locked or disabled, but the provided password is incorrect. Why would you go to another authentication provider? I would expect to try another provider if a UsernameNotFoundException or AuthenticationServiceException was thrown. In my scenario I know which provider the user should be authenticating with. In each provider I immediately check to see the the user type is supported by the provider. If not, I throw a AuthenticationServiceException to short circuit and skip to the next provider. I don't want to continue down the chain of providers when a bad credentials exception is thrown because the provider I selected is the only one the user can authenticate with.
        Hide
        Rob Winch added a comment -

        Thank you for explaining this in more detail. Unfortunately, I do not think we can make the proposed changes. I am closing this ticket as Won't Fix for the following reasons:

        • BadCredentialException means that the "credentials are invalid" (see javadoc). It does not necessarily mean that the user was found. There are often times the difference cannot be determined. For example, LDAP bind authentication cannot distinguish between the two scenarios. This means we cannot use BadCredentials vs UsernameNotFoundException to determine if the ProviderManager should continue.
        • UserNotFoundException is intended for UserDetailsService implementations and for AuthenticationProviders that use a UserDetailsService (see javadoc). It is not intended for general purpose AuthenticationProvider implementations and we do not want to leak this abstraction. For example, we do not want AuthenticationProvider implementations to throw it when they want the ProviderManager to continue to the next AuthenticationProvider.
        • Many, if not all, AuthenticationProviders convert the UsernameNotFoundException to a BadCredentialsException to ensure phishing is not done. You can see an example of this on AbstractUserDetailsAuthenticationProvider which has the hideUserNotFoundException property set to true by default. When the property is true it transforms the UserNotFoundException into BadCredentialsException. This means the ProviderManager will not be able to distinguish between an invalid username (UserNameNotFoundException) and an invalid username/password combination (BadCredentialsException). Users could set this property to false, but we do not want to encourage them to since this will allow for phishing.
        • There are others relying on ProviderManager to continue looping through the AuthenticationProviders

        This means you will need to extend Spring Security to meet this requirement. If you need the ability to select a specific provider and it cannot be determined by the supports method, you have at least a few options:

        • Since AuthenticationManager is an interface, you can implement the logic yourself. The logic is essentially just looping over the AuthenticationProvider implementations so it should not be difficult to implement.
        • Allow ProviderManager to iterate over all the AuthenticationProviders. If the AuthenticationProvider cannot authenticate the user, it can return null. This should be rather low cost in terms of performance.

        If you need further guidance, I'd be happy to assist on the forums.

        Show
        Rob Winch added a comment - Thank you for explaining this in more detail. Unfortunately, I do not think we can make the proposed changes. I am closing this ticket as Won't Fix for the following reasons: BadCredentialException means that the "credentials are invalid" ( see javadoc ). It does not necessarily mean that the user was found. There are often times the difference cannot be determined. For example, LDAP bind authentication cannot distinguish between the two scenarios. This means we cannot use BadCredentials vs UsernameNotFoundException to determine if the ProviderManager should continue. UserNotFoundException is intended for UserDetailsService implementations and for AuthenticationProviders that use a UserDetailsService ( see javadoc ). It is not intended for general purpose AuthenticationProvider implementations and we do not want to leak this abstraction. For example, we do not want AuthenticationProvider implementations to throw it when they want the ProviderManager to continue to the next AuthenticationProvider. Many, if not all, AuthenticationProviders convert the UsernameNotFoundException to a BadCredentialsException to ensure phishing is not done. You can see an example of this on AbstractUserDetailsAuthenticationProvider which has the hideUserNotFoundException property set to true by default. When the property is true it transforms the UserNotFoundException into BadCredentialsException. This means the ProviderManager will not be able to distinguish between an invalid username (UserNameNotFoundException) and an invalid username/password combination (BadCredentialsException). Users could set this property to false, but we do not want to encourage them to since this will allow for phishing. There are others relying on ProviderManager to continue looping through the AuthenticationProviders This means you will need to extend Spring Security to meet this requirement. If you need the ability to select a specific provider and it cannot be determined by the supports method, you have at least a few options: Since AuthenticationManager is an interface, you can implement the logic yourself. The logic is essentially just looping over the AuthenticationProvider implementations so it should not be difficult to implement. Allow ProviderManager to iterate over all the AuthenticationProviders. If the AuthenticationProvider cannot authenticate the user, it can return null. This should be rather low cost in terms of performance. If you need further guidance, I'd be happy to assist on the forums .

          People

          • Assignee:
            Rob Winch
            Reporter:
            Phil Krasko
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1d
              1d
              Remaining:
              Remaining Estimate - 1d
              1d
              Logged:
              Time Spent - Not Specified
              Not Specified