4 Replies Latest reply on Feb 5, 2008 7:40 PM by shane.bryzak

    Security questions to the Seam team

      Hi,

      I have 2 questions about the authentication process.

      #1
      Would you agree if I opened a JIRA issue to change the SeamLoginModule class to properly chain exceptions when throwing LoginExceptions in the login method? See code below:

      public boolean login()
       throws LoginException
       {
       try
       {
       NameCallback cbName = new NameCallback("Enter username");
       PasswordCallback cbPassword = new PasswordCallback("Enter password", false);
      
       // Get the username and password from the callback handler
       callbackHandler.handle(new Callback[] { cbName, cbPassword });
       username = cbName.getName();
       }
       catch (Exception ex)
       {
       log.error("Error logging in", ex);
       throw new LoginException(ex.getMessage());
       }
      
       MethodExpression mb = Identity.instance().getAuthenticateMethod();
       if (mb==null)
       {
       throw new IllegalStateException("No authentication method defined - please define <security:authenticate-method/> for <security:identity/> in components.xml");
       }
      
       try
       {
       return (Boolean) mb.invoke();
       }
       catch (Exception ex)
       {
       log.error("Error invoking login method", ex);
       throw new LoginException(ex.getMessage());
       }
       }
      


      In both instances where a LoginException is thrown, only the message is passed to the constructor of the LoginException. I know that the LoginException does not overload the constructor to pass a cause directly but we can use the initCause method instead:

      LoginException loginException = new LoginException();
      loginException.initCause(originalException);
      throw loginException;
      


      If people use typed exceptions in their authenticator method to express different reasons why the login attempt failed, it's impossible right now to get that original exception.

      #2
      The default behaviour of the isLoggedIn method in the Identity class is to pass the attemptLogin flag to true. Because of that, when authentication fails, it always calls the authenticator method twice. See the code of the authenticate() method below:

      public void authenticate()
       throws LoginException
       {
       // If we're already authenticated, then don't authenticate again
       if (!isLoggedIn())
       {
       authenticate( getLoginContext() );
       }
       }
      
      
       public boolean isLoggedIn(boolean attemptLogin)
       {
       if (!authenticating && attemptLogin && getPrincipal() == null && isCredentialsSet() &&
       Contexts.isEventContextActive() &&
       !Contexts.getEventContext().isSet(LOGIN_TRIED))
       {
       Contexts.getEventContext().set(LOGIN_TRIED, true);
       quietLogin();
       }
      
       // If there is a principal set, then the user is logged in.
       return getPrincipal() != null;
       }
      
      public void authenticate(LoginContext loginContext)
       throws LoginException
       {
       try
       {
       authenticating = true;
       preAuthenticate();
       loginContext.login();
       postAuthenticate();
       }
       finally
       {
       authenticating = false;
       }
       }
      


      The first reference to isLoggedIn tries to log the user. When it fails, it goes in the if block and tries to authenticate the user for a second time before failing again. I could fix this on my end by overriding the isLoggedIn() method in my own Identity component and passing the attemptLogin flag to false. Before doing so, I thought that perhaps a fix could be done at a higher level, i.e. in the Identity class of Seam itself. The way I see it, 2 things could be done:

      1. In the authenticate() method, invoke the isLoggedIn method with false.

      2. Look into the management of the authenticating class member; there might be something wrong. It's only set to true at the beginning of the authenticate(LoginContext) method. If you look at the logic in the isLoggedIn(boolean) method, when it winds up being invoked at the beginning of the authenticate(), the authenticating flag is false, the attemptLogin flag is true, I don't have a principal yet (I'm trying to login for the first time) and my credentials are set (the user just provided his username and password).

      Thanks for your help.

        • 1. Re: Security questions to the Seam team
          shane.bryzak

          Both of these suggestions are good, could you please raise these in JIRA and assign to me. They won't make it into Seam 2.0.0GA however I'll try to get them into 2.0.1.

          • 2. Re: Security questions to the Seam team

            Thanks for your reply.

            I created the following feature request for #1 (I didn't consider this as a bug): http://jira.jboss.org/jira/browse/JBSEAM-2164.

            For #2, I created the following bug fix request: http://jira.jboss.org/jira/browse/JBSEAM-2165.

            How do I assign them to you? I couldn't find a way to do it.

            • 3. Re: Security questions to the Seam team
              b.reeve

              I downloaded the latest Seam 2.0.1.GA release and I still have issues with Authenticator method being invoked twice.

              Based on the solution here, the authenticate() method is invoked with isLoggedIn(false), but then when there is a navigation rule like this

              <page view-id="/login.xhtml" >
               <navigation>
               <rule if="#{identity.loggedIn}">
               <redirect view-id="/home.xhtml" />
               </rule>
               </navigation>
              </page>
              


              the isLoggedIn(true) is being invoked and so there is another attempt to login.

              I see that this is the same configuration in all the example applications too.

              Is there a way to prevent the authenticator from invoking the login twice.

              • 4. Re: Security questions to the Seam team
                shane.bryzak

                The issue described in JBSEAM-2165 was a valid issue which is now fixed, however there is still no guarantee as to how many times the authenticator method will be called. From the ref docs:

                When writing an authenticator method, it is important that it is kept minimal and free from any side-effects. This is because there is no guarantee as to how many times the authenticator method will be called by the security API, and as such it may be invoked multiple times during a single request. Because of this, any special code that should execute upon a successful or failed authentication should be written by implementing an event observer. See the section on Security Events further down in this chapter for more information about which events are raised by Seam Security.