deltaspike-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shane Bryzak <sbry...@redhat.com>
Subject Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout
Date Sat, 24 Mar 2012 21:48:26 GMT
A few points:

1) Identity and DefaultIdentity should not be in the authentication package.

2) DefaultLoginCredential should be in the credential package, not 
authentication.

3) The following code has been modified in the login() method of the 
Identity implementation.  This code is important, it ensures that the 
authentication events are still fired and the login() method returns a 
success in the situation where the user performs an explicit login but a 
silent login occurs during the same request.

             if (isLoggedIn())
             {
                 // If authentication has already occurred during this 
request via a silent login,
                 // and login() is explicitly called then we still want 
to raise the LOGIN_SUCCESSFUL event,
                 // and then return.
                 if (requestSecurityState.get().isSilentLogin())
                 {
                     beanManager.fireEvent(new LoggedInEvent(user));
                     return AuthenticationResult.success;
                 }

                 beanManager.fireEvent(new AlreadyLoggedInEvent());
                 return AuthenticationResult.success;
             }

4) I'm not so sure this is a good idea:

//force a logout if a different user-id is provided
if (isAuthenticationRequestWithDifferentUserId())
{
logout(false);
}


There's many reasons I'm -1 on this, here's a few of them:

a) In most typical applications the login form won't even be visible to 
the user after they have logged in already.

b) It's important to keep a clean separation between operations 
performed within different authentication contexts (I don't mean CDI 
context here, I mean the context of being logged in as a certain user).  
For example, things like auditing can be potentially affected by this, 
where an application is logging what happens during each request under 
each user's user ID.

c) The test for determining if the user logging in is a different user 
is problematic - there's no guarantee that the username/password they 
provide is going to be equal to the current User.userID value, and it 
doesn't take into account authentication by means other than a 
username/password.

d) Automatically throwing away the user's session as a side effect of 
this functionality (by calling logout()) could potentially be dangerous, 
as there may be important state that the user can lose.  I'm of the 
strong opinion that logging out should always be an explicit operation 
performed intentionally by the user.

In general, I think if a user that is already authenticated tries to log 
in with a different username it should throw an exception.

5) The quietLogin() method is missing.  This method is a non-functional 
requirement of the "Remember Me" use case.



On 23/03/12 22:46, Gerhard Petracek wrote:
> hi @ all,
>
> as mentioned in [1] we switched to a step by step discussion for the
> security module.
> the first step of part 1 (see [2]) is a>simple<  credential based
> login(/logout) use-case.
>
> some of us reviewed and improved the current draft [3] already.
> you can see the result at [3] (and not in our repository). [3] also
> contains a link to the refactored api (+ new tests).
> this version includes what we need for the>simple<  login/logout scenario
> mentioned by part 1.
> that means the api and spi you can see at [3] is just a first step and will
> be changed based on further scenarios (of the other parts).
> (e.g. right now "User" is a class and will be refactored to an interface as
> soon as we need to change it.)
>
> if there are no basic objections, i'll push those changes to our repository
> on sunday (and i'll add further tests afterwards).
> furthermore, everything in [3] which is marked as "agreed" will be added to
> our temporary documentation and will be part of v0.2-incubating.
> (as mentioned before: even those parts might be changed later on, but not
> before the release of v0.2-incubating which should be available soon.)
>
> regards,
> gerhard
>
> [1] http://s.apache.org/uc6
> [2] http://s.apache.org/HC1
> [3] http://s.apache.org/6OE
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message