[jboss-user] [JBoss Seam] - Security questions to the Seam team

[ylazzari]

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.	

View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4098803#4098803

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4098803