[DISCUSS] [DELTASPIKE-76/127] security module - login/logout

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Gerhard Petracek
Administrator
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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Shane Bryzak-2
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
>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Gerhard Petracek
Administrator
hi shane,

@ #1 & #2:
i'm ok with it -> please provide a concrete suggestion for #1

@ #3:
imo we need a different event (and also a different term for "silent") -
rest see #5

@ #4:
that's usually true for gui-clients (see part 3), but not necessarily for
other clients.
imo it's ok for this first step of part 1 (because the prev. behaviour can
lead to side-effects which are hard/er to detect).
however, i knew from the beginning why you did it and imo it needs an
additional concept as soon as we discuss the corresponding use-case (and
the test for it fails).

@ #5:
this scenario isn't necessarily a topic for part 1 -> imo it's a topic for
part 3 and it needs further discussions.

regards,
gerhard



2012/3/24 Shane Bryzak <[hidden email]>

>  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
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Shane Bryzak-2
On 25/03/12 09:09, Gerhard Petracek wrote:
> hi shane,
>
> @ #1&  #2:
> i'm ok with it ->  please provide a concrete suggestion for #1

As I said previously I think that Identity should be in the base
package, i.e. org.apache.deltaspike.security.api.Identity.  This bean
deals with cross-cutting security concerns and it's a logical place for
it to be.
>
> @ #3:
> imo we need a different event (and also a different term for "silent") -
> rest see #5

Do you mean AlreadyLoggedInEvent?  What would you suggest as an
alternative?  Also, I think quietLogin() is an apt description for this
method, as its purpose is to attempt authentication if possible, and not
to throw any exceptions or the usual events upon success or failure.  
It's for non-explicit login where the credential information (or some
other identifying token) is available when an unauthenticated user has
attempted to invoke (directly or indirectly) either a privileged
operation or privilege check.
>
> @ #4:
> that's usually true for gui-clients (see part 3), but not necessarily for
> other clients.
> imo it's ok for this first step of part 1 (because the prev. behaviour can
> lead to side-effects which are hard/er to detect).
> however, i knew from the beginning why you did it and imo it needs an
> additional concept as soon as we discuss the corresponding use-case (and
> the test for it fails).

Do you have a use case for this?  I still think it's a really dangerous
thing to do, and I see no reason why a non-gui client couldn't just do
an explicit log out before logging in again.

>
> @ #5:
> this scenario isn't necessarily a topic for part 1 ->  imo it's a topic for
> part 3 and it needs further discussions.

No problem, we can put it on the backburner for now.

>
> regards,
> gerhard
>
>
>
> 2012/3/24 Shane Bryzak<[hidden email]>
>
>>   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
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Boleslaw Dawidowicz
@ #4

I was also wondering about use case related to this. I agree with Shane that logout should be explicit operation because of possible side effects and not easy to foresee scenarios

Bolek

On Mar 25, 2012, at 11:03 PM, Shane Bryzak wrote:

> On 25/03/12 09:09, Gerhard Petracek wrote:
>> hi shane,
>>
>> @ #1&  #2:
>> i'm ok with it ->  please provide a concrete suggestion for #1
>
> As I said previously I think that Identity should be in the base package, i.e. org.apache.deltaspike.security.api.Identity.  This bean deals with cross-cutting security concerns and it's a logical place for it to be.
>>
>> @ #3:
>> imo we need a different event (and also a different term for "silent") -
>> rest see #5
>
> Do you mean AlreadyLoggedInEvent?  What would you suggest as an alternative?  Also, I think quietLogin() is an apt description for this method, as its purpose is to attempt authentication if possible, and not to throw any exceptions or the usual events upon success or failure.  It's for non-explicit login where the credential information (or some other identifying token) is available when an unauthenticated user has attempted to invoke (directly or indirectly) either a privileged operation or privilege check.
>>
>> @ #4:
>> that's usually true for gui-clients (see part 3), but not necessarily for
>> other clients.
>> imo it's ok for this first step of part 1 (because the prev. behaviour can
>> lead to side-effects which are hard/er to detect).
>> however, i knew from the beginning why you did it and imo it needs an
>> additional concept as soon as we discuss the corresponding use-case (and
>> the test for it fails).
>
> Do you have a use case for this?  I still think it's a really dangerous thing to do, and I see no reason why a non-gui client couldn't just do an explicit log out before logging in again.
>
>>
>> @ #5:
>> this scenario isn't necessarily a topic for part 1 ->  imo it's a topic for
>> part 3 and it needs further discussions.
>
> No problem, we can put it on the backburner for now.
>
>>
>> regards,
>> gerhard
>>
>>
>>
>> 2012/3/24 Shane Bryzak<[hidden email]>
>>
>>>  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
>>>
>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Pete Muir
In reply to this post by Shane Bryzak-2

On 25 Mar 2012, at 22:03, Shane Bryzak wrote:

> On 25/03/12 09:09, Gerhard Petracek wrote:
>> hi shane,
>>
>> @ #1&  #2:
>> i'm ok with it ->  please provide a concrete suggestion for #1
>
> As I said previously I think that Identity should be in the base package, i.e. org.apache.deltaspike.security.api.Identity.  This bean deals with cross-cutting security concerns and it's a logical place for it to be.
>>
>> @ #3:
>> imo we need a different event (and also a different term for "silent") -
>> rest see #5
>
> Do you mean AlreadyLoggedInEvent?  What would you suggest as an alternative?  Also, I think quietLogin() is an apt description for this method, as its purpose is to attempt authentication if possible, and not to throw any exceptions or the usual events upon success or failure.  It's for non-explicit login where the credential information (or some other identifying token) is available when an unauthenticated user has attempted to invoke (directly or indirectly) either a privileged operation or privilege check.

Silent or quiet login are quite well accepted terms for "try to login but don't error if it fails". You can google these terms to see where they have been used in the past.

>>
>> @ #4:
>> that's usually true for gui-clients (see part 3), but not necessarily for
>> other clients.
>> imo it's ok for this first step of part 1 (because the prev. behaviour can
>> lead to side-effects which are hard/er to detect).
>> however, i knew from the beginning why you did it and imo it needs an
>> additional concept as soon as we discuss the corresponding use-case (and
>> the test for it fails).
>
> Do you have a use case for this?  I still think it's a really dangerous thing to do, and I see no reason why a non-gui client couldn't just do an explicit log out before logging in again.

I think this leads to a pretty odd situation. Throwing an exception if you try to login without logging out first seems much more explicit to me. Gerhard, can you expand on what you see as the problem here, and the use cases where it is a problem?

>
>>
>> @ #5:
>> this scenario isn't necessarily a topic for part 1 ->  imo it's a topic for
>> part 3 and it needs further discussions.
>
> No problem, we can put it on the backburner for now.
>
>>
>> regards,
>> gerhard
>>
>>
>>
>> 2012/3/24 Shane Bryzak<[hidden email]>
>>
>>>  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
>>>
>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Gerhard Petracek
Administrator
hi @ all,

@ #1:
+0.5 (which is enough for me to move it).

@ #3:
the current version on the master is:

IdentityImpl#login:
//...
if (isLoggedIn())
{
    if (requestSecurityState.get().isSilentLogin())
    {
        beanManager.fireEvent(new LoggedInEvent(user));
        return AuthenticationResult.success;
    }

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

LoggedInEvent isn't correct here imo and if we change it to
AlreadyLoggedInEvent, it's the same as the non-silent login.
however, since we postpone #5 we can discuss it later.

@ #4:
you can use the same argument for the login - just use:
if (!identity.isLoggedIn())
{
    identity.login();
}

the autom. logout in case of an unexpected constellation is a requirement
in some applications i know.
however, i'm also ok with e.g. an UnexpectedCredentialException

with the current version the new credentials are just ignored and you get
back 'success' as result -> -1

regards,
gerhard



2012/3/26 Pete Muir <[hidden email]>

>
> On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>
> > On 25/03/12 09:09, Gerhard Petracek wrote:
> >> hi shane,
> >>
> >> @ #1&  #2:
> >> i'm ok with it ->  please provide a concrete suggestion for #1
> >
> > As I said previously I think that Identity should be in the base
> package, i.e. org.apache.deltaspike.security.api.Identity.  This bean deals
> with cross-cutting security concerns and it's a logical place for it to be.
> >>
> >> @ #3:
> >> imo we need a different event (and also a different term for "silent") -
> >> rest see #5
> >
> > Do you mean AlreadyLoggedInEvent?  What would you suggest as an
> alternative?  Also, I think quietLogin() is an apt description for this
> method, as its purpose is to attempt authentication if possible, and not to
> throw any exceptions or the usual events upon success or failure.  It's for
> non-explicit login where the credential information (or some other
> identifying token) is available when an unauthenticated user has attempted
> to invoke (directly or indirectly) either a privileged operation or
> privilege check.
>
> Silent or quiet login are quite well accepted terms for "try to login but
> don't error if it fails". You can google these terms to see where they have
> been used in the past.
>
> >>
> >> @ #4:
> >> that's usually true for gui-clients (see part 3), but not necessarily
> for
> >> other clients.
> >> imo it's ok for this first step of part 1 (because the prev. behaviour
> can
> >> lead to side-effects which are hard/er to detect).
> >> however, i knew from the beginning why you did it and imo it needs an
> >> additional concept as soon as we discuss the corresponding use-case (and
> >> the test for it fails).
> >
> > Do you have a use case for this?  I still think it's a really dangerous
> thing to do, and I see no reason why a non-gui client couldn't just do an
> explicit log out before logging in again.
>
> I think this leads to a pretty odd situation. Throwing an exception if you
> try to login without logging out first seems much more explicit to me.
> Gerhard, can you expand on what you see as the problem here, and the use
> cases where it is a problem?
>
> >
> >>
> >> @ #5:
> >> this scenario isn't necessarily a topic for part 1 ->  imo it's a topic
> for
> >> part 3 and it needs further discussions.
> >
> > No problem, we can put it on the backburner for now.
> >
> >>
> >> regards,
> >> gerhard
> >>
> >>
> >>
> >> 2012/3/24 Shane Bryzak<[hidden email]>
> >>
> >>>  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
> >>>
> >>>
> >>>
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Jason Porter
I'm hoping I don't derail the great discussions we have going on with this
mail.

Having gone back and read this thread it seems like we're doing some
general hand waving about "stuff I don't like because ________" instead of
talking about actual use cases we've seen from various projects, clients
and users. IMO we'll get better discussions going around if we start
bringing requirements and use cases to the table from our various sources.
We may disagree about the appropriate implementation of something but we
should at least be able to agree about requirements and use cases if
they're discussed.

I know we have some from Seam 2 and 3 that we've been addressing and using,
and I'm sure there are things from other clients Gerhard and Mark have
addressed in the past. These may also not be the same kinds of clients, in
fact I'd wager they're not. Hopefully discussing the requirements a bit
more and getting back on the ground will help move the discussions along.

On Mon, Mar 26, 2012 at 05:34, Gerhard Petracek
<[hidden email]>wrote:

> hi @ all,
>
> @ #1:
> +0.5 (which is enough for me to move it).
>
> @ #3:
> the current version on the master is:
>
> IdentityImpl#login:
> //...
> if (isLoggedIn())
> {
>    if (requestSecurityState.get().isSilentLogin())
>    {
>        beanManager.fireEvent(new LoggedInEvent(user));
>        return AuthenticationResult.success;
>    }
>
>    beanManager.fireEvent(new AlreadyLoggedInEvent());
>    return AuthenticationResult.success;
> }
> //...
>
> LoggedInEvent isn't correct here imo and if we change it to
> AlreadyLoggedInEvent, it's the same as the non-silent login.
> however, since we postpone #5 we can discuss it later.
>
> @ #4:
> you can use the same argument for the login - just use:
> if (!identity.isLoggedIn())
> {
>    identity.login();
> }
>
> the autom. logout in case of an unexpected constellation is a requirement
> in some applications i know.
> however, i'm also ok with e.g. an UnexpectedCredentialException
>
> with the current version the new credentials are just ignored and you get
> back 'success' as result -> -1
>
> regards,
> gerhard
>
>
>
> 2012/3/26 Pete Muir <[hidden email]>
>
> >
> > On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
> >
> > > On 25/03/12 09:09, Gerhard Petracek wrote:
> > >> hi shane,
> > >>
> > >> @ #1&  #2:
> > >> i'm ok with it ->  please provide a concrete suggestion for #1
> > >
> > > As I said previously I think that Identity should be in the base
> > package, i.e. org.apache.deltaspike.security.api.Identity.  This bean
> deals
> > with cross-cutting security concerns and it's a logical place for it to
> be.
> > >>
> > >> @ #3:
> > >> imo we need a different event (and also a different term for
> "silent") -
> > >> rest see #5
> > >
> > > Do you mean AlreadyLoggedInEvent?  What would you suggest as an
> > alternative?  Also, I think quietLogin() is an apt description for this
> > method, as its purpose is to attempt authentication if possible, and not
> to
> > throw any exceptions or the usual events upon success or failure.  It's
> for
> > non-explicit login where the credential information (or some other
> > identifying token) is available when an unauthenticated user has
> attempted
> > to invoke (directly or indirectly) either a privileged operation or
> > privilege check.
> >
> > Silent or quiet login are quite well accepted terms for "try to login but
> > don't error if it fails". You can google these terms to see where they
> have
> > been used in the past.
> >
> > >>
> > >> @ #4:
> > >> that's usually true for gui-clients (see part 3), but not necessarily
> > for
> > >> other clients.
> > >> imo it's ok for this first step of part 1 (because the prev. behaviour
> > can
> > >> lead to side-effects which are hard/er to detect).
> > >> however, i knew from the beginning why you did it and imo it needs an
> > >> additional concept as soon as we discuss the corresponding use-case
> (and
> > >> the test for it fails).
> > >
> > > Do you have a use case for this?  I still think it's a really dangerous
> > thing to do, and I see no reason why a non-gui client couldn't just do an
> > explicit log out before logging in again.
> >
> > I think this leads to a pretty odd situation. Throwing an exception if
> you
> > try to login without logging out first seems much more explicit to me.
> > Gerhard, can you expand on what you see as the problem here, and the use
> > cases where it is a problem?
> >
> > >
> > >>
> > >> @ #5:
> > >> this scenario isn't necessarily a topic for part 1 ->  imo it's a
> topic
> > for
> > >> part 3 and it needs further discussions.
> > >
> > > No problem, we can put it on the backburner for now.
> > >
> > >>
> > >> regards,
> > >> gerhard
> > >>
> > >>
> > >>
> > >> 2012/3/24 Shane Bryzak<[hidden email]>
> > >>
> > >>>  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
> > >>>
> > >>>
> > >>>
> > >
> >
> >
>



--
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp

Software Engineer
Open Source Advocate
Author of Seam Catch - Next Generation Java Exception Handling

PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Shane Bryzak-2
In reply to this post by Gerhard Petracek
On 26/03/12 21:34, Gerhard Petracek wrote:

> hi @ all,
>
> @ #1:
> +0.5 (which is enough for me to move it).
>
> @ #3:
> the current version on the master is:
>
> IdentityImpl#login:
> //...
> if (isLoggedIn())
> {
>      if (requestSecurityState.get().isSilentLogin())
>      {
>          beanManager.fireEvent(new LoggedInEvent(user));
>          return AuthenticationResult.success;
>      }
>
>      beanManager.fireEvent(new AlreadyLoggedInEvent());
>      return AuthenticationResult.success;
> }
> //...
>
> LoggedInEvent isn't correct here imo and if we change it to
> AlreadyLoggedInEvent, it's the same as the non-silent login.
> however, since we postpone #5 we can discuss it later.

The business logic here is correct - if the user has been silently
authenticated during an explicit attempt to log in, then the
LoggedInEvent should still be fired and a "success" value returned.  
What happens next in the case where a silent login *hasn't* occurred I
agree, the "success" may not be the most appropriate result.  My
proposal here is to introduce a new enum value for the scenario where an
already authenticated user attempts to log in again - possible examples
for this may be something like LOGGED_IN, UNKNOWN or UNDETERMINED.

>
> @ #4:
> you can use the same argument for the login - just use:
> if (!identity.isLoggedIn())
> {
>      identity.login();
> }
>
> the autom. logout in case of an unexpected constellation is a requirement
> in some applications i know.
> however, i'm also ok with e.g. an UnexpectedCredentialException

I'm +1 on throwing either an UnexpectedCredentialException here, or
returning one of the enum values I suggested above and allowing the
developer to deal with the return code.

>
> with the current version the new credentials are just ignored and you get
> back 'success' as result ->  -1

See my suggestion above.

>
> regards,
> gerhard
>
>
>
> 2012/3/26 Pete Muir<[hidden email]>
>
>> On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>>
>>> On 25/03/12 09:09, Gerhard Petracek wrote:
>>>> hi shane,
>>>>
>>>> @ #1&   #2:
>>>> i'm ok with it ->   please provide a concrete suggestion for #1
>>> As I said previously I think that Identity should be in the base
>> package, i.e. org.apache.deltaspike.security.api.Identity.  This bean deals
>> with cross-cutting security concerns and it's a logical place for it to be.
>>>> @ #3:
>>>> imo we need a different event (and also a different term for "silent") -
>>>> rest see #5
>>> Do you mean AlreadyLoggedInEvent?  What would you suggest as an
>> alternative?  Also, I think quietLogin() is an apt description for this
>> method, as its purpose is to attempt authentication if possible, and not to
>> throw any exceptions or the usual events upon success or failure.  It's for
>> non-explicit login where the credential information (or some other
>> identifying token) is available when an unauthenticated user has attempted
>> to invoke (directly or indirectly) either a privileged operation or
>> privilege check.
>>
>> Silent or quiet login are quite well accepted terms for "try to login but
>> don't error if it fails". You can google these terms to see where they have
>> been used in the past.
>>
>>>> @ #4:
>>>> that's usually true for gui-clients (see part 3), but not necessarily
>> for
>>>> other clients.
>>>> imo it's ok for this first step of part 1 (because the prev. behaviour
>> can
>>>> lead to side-effects which are hard/er to detect).
>>>> however, i knew from the beginning why you did it and imo it needs an
>>>> additional concept as soon as we discuss the corresponding use-case (and
>>>> the test for it fails).
>>> Do you have a use case for this?  I still think it's a really dangerous
>> thing to do, and I see no reason why a non-gui client couldn't just do an
>> explicit log out before logging in again.
>>
>> I think this leads to a pretty odd situation. Throwing an exception if you
>> try to login without logging out first seems much more explicit to me.
>> Gerhard, can you expand on what you see as the problem here, and the use
>> cases where it is a problem?
>>
>>>> @ #5:
>>>> this scenario isn't necessarily a topic for part 1 ->   imo it's a topic
>> for
>>>> part 3 and it needs further discussions.
>>> No problem, we can put it on the backburner for now.
>>>
>>>> regards,
>>>> gerhard
>>>>
>>>>
>>>>
>>>> 2012/3/24 Shane Bryzak<[hidden email]>
>>>>
>>>>>   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
>>>>>
>>>>>
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Gerhard Petracek
Administrator
hi shane,

i changed #1, #2 and #4 (see [1]).

i agree with jason that we should discuss the rest based on further
use-cases.
since we already agreed on postponing the rest, we should be fine for now.

the last topic i found for this first step is AuthenticationResult.EXCEPTION
we could think about using the trick we have in our ExceptionUtils (which
need to be discussed in any case) instead of using
AuthenticationResult.EXCEPTION (if it is only used to avoid the issue
with catched checked exceptions).

regards,
gerhard

[1] https://issues.apache.org/jira/browse/DELTASPIKE-127



2012/3/27 Shane Bryzak <[hidden email]>

> On 26/03/12 21:34, Gerhard Petracek wrote:
>
>> hi @ all,
>>
>> @ #1:
>> +0.5 (which is enough for me to move it).
>>
>> @ #3:
>> the current version on the master is:
>>
>> IdentityImpl#login:
>> //...
>> if (isLoggedIn())
>> {
>>     if (requestSecurityState.get().**isSilentLogin())
>>     {
>>         beanManager.fireEvent(new LoggedInEvent(user));
>>         return AuthenticationResult.success;
>>     }
>>
>>     beanManager.fireEvent(new AlreadyLoggedInEvent());
>>     return AuthenticationResult.success;
>> }
>> //...
>>
>> LoggedInEvent isn't correct here imo and if we change it to
>> AlreadyLoggedInEvent, it's the same as the non-silent login.
>> however, since we postpone #5 we can discuss it later.
>>
>
> The business logic here is correct - if the user has been silently
> authenticated during an explicit attempt to log in, then the LoggedInEvent
> should still be fired and a "success" value returned.  What happens next in
> the case where a silent login *hasn't* occurred I agree, the "success" may
> not be the most appropriate result.  My proposal here is to introduce a new
> enum value for the scenario where an already authenticated user attempts to
> log in again - possible examples for this may be something like LOGGED_IN,
> UNKNOWN or UNDETERMINED.
>
>
>> @ #4:
>> you can use the same argument for the login - just use:
>> if (!identity.isLoggedIn())
>> {
>>     identity.login();
>> }
>>
>> the autom. logout in case of an unexpected constellation is a requirement
>> in some applications i know.
>> however, i'm also ok with e.g. an UnexpectedCredentialException
>>
>
> I'm +1 on throwing either an UnexpectedCredentialException here, or
> returning one of the enum values I suggested above and allowing the
> developer to deal with the return code.
>
>
>
>> with the current version the new credentials are just ignored and you get
>> back 'success' as result ->  -1
>>
>
> See my suggestion above.
>
>
>
>> regards,
>> gerhard
>>
>>
>>
>> 2012/3/26 Pete Muir<[hidden email]>
>>
>>  On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>>>
>>>  On 25/03/12 09:09, Gerhard Petracek wrote:
>>>>
>>>>> hi shane,
>>>>>
>>>>> @ #1&   #2:
>>>>> i'm ok with it ->   please provide a concrete suggestion for #1
>>>>>
>>>> As I said previously I think that Identity should be in the base
>>>>
>>> package, i.e. org.apache.deltaspike.**security.api.Identity.  This bean
>>> deals
>>> with cross-cutting security concerns and it's a logical place for it to
>>> be.
>>>
>>>> @ #3:
>>>>> imo we need a different event (and also a different term for "silent")
>>>>> -
>>>>> rest see #5
>>>>>
>>>> Do you mean AlreadyLoggedInEvent?  What would you suggest as an
>>>>
>>> alternative?  Also, I think quietLogin() is an apt description for this
>>> method, as its purpose is to attempt authentication if possible, and not
>>> to
>>> throw any exceptions or the usual events upon success or failure.  It's
>>> for
>>> non-explicit login where the credential information (or some other
>>> identifying token) is available when an unauthenticated user has
>>> attempted
>>> to invoke (directly or indirectly) either a privileged operation or
>>> privilege check.
>>>
>>> Silent or quiet login are quite well accepted terms for "try to login but
>>> don't error if it fails". You can google these terms to see where they
>>> have
>>> been used in the past.
>>>
>>>  @ #4:
>>>>> that's usually true for gui-clients (see part 3), but not necessarily
>>>>>
>>>> for
>>>
>>>> other clients.
>>>>> imo it's ok for this first step of part 1 (because the prev. behaviour
>>>>>
>>>> can
>>>
>>>> lead to side-effects which are hard/er to detect).
>>>>> however, i knew from the beginning why you did it and imo it needs an
>>>>> additional concept as soon as we discuss the corresponding use-case
>>>>> (and
>>>>> the test for it fails).
>>>>>
>>>> Do you have a use case for this?  I still think it's a really dangerous
>>>>
>>> thing to do, and I see no reason why a non-gui client couldn't just do an
>>> explicit log out before logging in again.
>>>
>>> I think this leads to a pretty odd situation. Throwing an exception if
>>> you
>>> try to login without logging out first seems much more explicit to me.
>>> Gerhard, can you expand on what you see as the problem here, and the use
>>> cases where it is a problem?
>>>
>>>  @ #5:
>>>>> this scenario isn't necessarily a topic for part 1 ->   imo it's a
>>>>> topic
>>>>>
>>>> for
>>>
>>>> part 3 and it needs further discussions.
>>>>>
>>>> No problem, we can put it on the backburner for now.
>>>>
>>>>  regards,
>>>>> gerhard
>>>>>
>>>>>
>>>>>
>>>>> 2012/3/24 Shane Bryzak<[hidden email]>
>>>>>
>>>>>   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 (**isAuthenticationRequestWithDif**ferentUserId())
>>>>>>             {
>>>>>>
>>>>>>                 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Shane Bryzak-2
On 28/03/12 03:21, Gerhard Petracek wrote:
> hi shane,
>
> i changed #1, #2 and #4 (see [1]).
>
> i agree with jason that we should discuss the rest based on further
> use-cases.
> since we already agreed on postponing the rest, we should be fine for now.

+1, I've reviewed the patch and it looks ok for Part 1.
>
> the last topic i found for this first step is AuthenticationResult.EXCEPTION
> we could think about using the trick we have in our ExceptionUtils (which
> need to be discussed in any case) instead of using
> AuthenticationResult.EXCEPTION (if it is only used to avoid the issue
> with catched checked exceptions).

I'm not sure in which circumstances it would make sense to return
AuthenticationResult.EXCEPTION instead of just throwing an exception,
however we can certainly discuss this further.

>
> regards,
> gerhard
>
> [1] https://issues.apache.org/jira/browse/DELTASPIKE-127
>
>
>
> 2012/3/27 Shane Bryzak<[hidden email]>
>
>> On 26/03/12 21:34, Gerhard Petracek wrote:
>>
>>> hi @ all,
>>>
>>> @ #1:
>>> +0.5 (which is enough for me to move it).
>>>
>>> @ #3:
>>> the current version on the master is:
>>>
>>> IdentityImpl#login:
>>> //...
>>> if (isLoggedIn())
>>> {
>>>      if (requestSecurityState.get().**isSilentLogin())
>>>      {
>>>          beanManager.fireEvent(new LoggedInEvent(user));
>>>          return AuthenticationResult.success;
>>>      }
>>>
>>>      beanManager.fireEvent(new AlreadyLoggedInEvent());
>>>      return AuthenticationResult.success;
>>> }
>>> //...
>>>
>>> LoggedInEvent isn't correct here imo and if we change it to
>>> AlreadyLoggedInEvent, it's the same as the non-silent login.
>>> however, since we postpone #5 we can discuss it later.
>>>
>> The business logic here is correct - if the user has been silently
>> authenticated during an explicit attempt to log in, then the LoggedInEvent
>> should still be fired and a "success" value returned.  What happens next in
>> the case where a silent login *hasn't* occurred I agree, the "success" may
>> not be the most appropriate result.  My proposal here is to introduce a new
>> enum value for the scenario where an already authenticated user attempts to
>> log in again - possible examples for this may be something like LOGGED_IN,
>> UNKNOWN or UNDETERMINED.
>>
>>
>>> @ #4:
>>> you can use the same argument for the login - just use:
>>> if (!identity.isLoggedIn())
>>> {
>>>      identity.login();
>>> }
>>>
>>> the autom. logout in case of an unexpected constellation is a requirement
>>> in some applications i know.
>>> however, i'm also ok with e.g. an UnexpectedCredentialException
>>>
>> I'm +1 on throwing either an UnexpectedCredentialException here, or
>> returning one of the enum values I suggested above and allowing the
>> developer to deal with the return code.
>>
>>
>>
>>> with the current version the new credentials are just ignored and you get
>>> back 'success' as result ->   -1
>>>
>> See my suggestion above.
>>
>>
>>
>>> regards,
>>> gerhard
>>>
>>>
>>>
>>> 2012/3/26 Pete Muir<[hidden email]>
>>>
>>>   On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>>>>   On 25/03/12 09:09, Gerhard Petracek wrote:
>>>>>> hi shane,
>>>>>>
>>>>>> @ #1&    #2:
>>>>>> i'm ok with it ->    please provide a concrete suggestion for #1
>>>>>>
>>>>> As I said previously I think that Identity should be in the base
>>>>>
>>>> package, i.e. org.apache.deltaspike.**security.api.Identity.  This bean
>>>> deals
>>>> with cross-cutting security concerns and it's a logical place for it to
>>>> be.
>>>>
>>>>> @ #3:
>>>>>> imo we need a different event (and also a different term for "silent")
>>>>>> -
>>>>>> rest see #5
>>>>>>
>>>>> Do you mean AlreadyLoggedInEvent?  What would you suggest as an
>>>>>
>>>> alternative?  Also, I think quietLogin() is an apt description for this
>>>> method, as its purpose is to attempt authentication if possible, and not
>>>> to
>>>> throw any exceptions or the usual events upon success or failure.  It's
>>>> for
>>>> non-explicit login where the credential information (or some other
>>>> identifying token) is available when an unauthenticated user has
>>>> attempted
>>>> to invoke (directly or indirectly) either a privileged operation or
>>>> privilege check.
>>>>
>>>> Silent or quiet login are quite well accepted terms for "try to login but
>>>> don't error if it fails". You can google these terms to see where they
>>>> have
>>>> been used in the past.
>>>>
>>>>   @ #4:
>>>>>> that's usually true for gui-clients (see part 3), but not necessarily
>>>>>>
>>>>> for
>>>>> other clients.
>>>>>> imo it's ok for this first step of part 1 (because the prev. behaviour
>>>>>>
>>>>> can
>>>>> lead to side-effects which are hard/er to detect).
>>>>>> however, i knew from the beginning why you did it and imo it needs an
>>>>>> additional concept as soon as we discuss the corresponding use-case
>>>>>> (and
>>>>>> the test for it fails).
>>>>>>
>>>>> Do you have a use case for this?  I still think it's a really dangerous
>>>>>
>>>> thing to do, and I see no reason why a non-gui client couldn't just do an
>>>> explicit log out before logging in again.
>>>>
>>>> I think this leads to a pretty odd situation. Throwing an exception if
>>>> you
>>>> try to login without logging out first seems much more explicit to me.
>>>> Gerhard, can you expand on what you see as the problem here, and the use
>>>> cases where it is a problem?
>>>>
>>>>   @ #5:
>>>>>> this scenario isn't necessarily a topic for part 1 ->    imo it's a
>>>>>> topic
>>>>>>
>>>>> for
>>>>> part 3 and it needs further discussions.
>>>>> No problem, we can put it on the backburner for now.
>>>>>
>>>>>   regards,
>>>>>> gerhard
>>>>>>
>>>>>>
>>>>>>
>>>>>> 2012/3/24 Shane Bryzak<[hidden email]>
>>>>>>
>>>>>>    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 (**isAuthenticationRequestWithDif**ferentUserId())
>>>>>>>              {
>>>>>>>
>>>>>>>                  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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Gerhard Petracek
Administrator
hi shane,

@ AuthenticationResult.EXCEPTION:
we are talking about the same.
however, in the master we have:

public AuthenticationResult login()
{
    try
    {
        //login logic
    }
    catch (Exception ex)
    {
        beanManager.fireEvent(new LoginFailedEvent(ex));
        return AuthenticationResult.exception;
    }
}

and that was/is my objection.

since we agree on it, i'll also change it (for now) to show what i've in
mind.
it's a quite small part -> we can discuss the result after the commit.

-> i'll commit it and add information about the removed parts to the jira
issue - so we can find and check out the previous state easily.

regards,
gerhard



2012/3/27 Shane Bryzak <[hidden email]>

> On 28/03/12 03:21, Gerhard Petracek wrote:
>
>> hi shane,
>>
>> i changed #1, #2 and #4 (see [1]).
>>
>> i agree with jason that we should discuss the rest based on further
>> use-cases.
>> since we already agreed on postponing the rest, we should be fine for now.
>>
>
> +1, I've reviewed the patch and it looks ok for Part 1.
>
>
>> the last topic i found for this first step is
>> AuthenticationResult.EXCEPTION
>> we could think about using the trick we have in our ExceptionUtils (which
>> need to be discussed in any case) instead of using
>> AuthenticationResult.EXCEPTION (if it is only used to avoid the issue
>> with catched checked exceptions).
>>
>
> I'm not sure in which circumstances it would make sense to return
> AuthenticationResult.EXCEPTION instead of just throwing an exception,
> however we can certainly discuss this further.
>
>>
>> regards,
>> gerhard
>>
>> [1] https://issues.apache.org/**jira/browse/DELTASPIKE-127<https://issues.apache.org/jira/browse/DELTASPIKE-127>
>>
>>
>>
>> 2012/3/27 Shane Bryzak<[hidden email]>
>>
>>  On 26/03/12 21:34, Gerhard Petracek wrote:
>>>
>>>  hi @ all,
>>>>
>>>> @ #1:
>>>> +0.5 (which is enough for me to move it).
>>>>
>>>> @ #3:
>>>> the current version on the master is:
>>>>
>>>> IdentityImpl#login:
>>>> //...
>>>> if (isLoggedIn())
>>>> {
>>>>     if (requestSecurityState.get().****isSilentLogin())
>>>>
>>>>     {
>>>>         beanManager.fireEvent(new LoggedInEvent(user));
>>>>         return AuthenticationResult.success;
>>>>     }
>>>>
>>>>     beanManager.fireEvent(new AlreadyLoggedInEvent());
>>>>     return AuthenticationResult.success;
>>>> }
>>>> //...
>>>>
>>>> LoggedInEvent isn't correct here imo and if we change it to
>>>> AlreadyLoggedInEvent, it's the same as the non-silent login.
>>>> however, since we postpone #5 we can discuss it later.
>>>>
>>>>  The business logic here is correct - if the user has been silently
>>> authenticated during an explicit attempt to log in, then the
>>> LoggedInEvent
>>> should still be fired and a "success" value returned.  What happens next
>>> in
>>> the case where a silent login *hasn't* occurred I agree, the "success"
>>> may
>>> not be the most appropriate result.  My proposal here is to introduce a
>>> new
>>> enum value for the scenario where an already authenticated user attempts
>>> to
>>> log in again - possible examples for this may be something like
>>> LOGGED_IN,
>>> UNKNOWN or UNDETERMINED.
>>>
>>>
>>>  @ #4:
>>>> you can use the same argument for the login - just use:
>>>> if (!identity.isLoggedIn())
>>>> {
>>>>     identity.login();
>>>> }
>>>>
>>>> the autom. logout in case of an unexpected constellation is a
>>>> requirement
>>>> in some applications i know.
>>>> however, i'm also ok with e.g. an UnexpectedCredentialException
>>>>
>>>>  I'm +1 on throwing either an UnexpectedCredentialException here, or
>>> returning one of the enum values I suggested above and allowing the
>>> developer to deal with the return code.
>>>
>>>
>>>
>>>  with the current version the new credentials are just ignored and you
>>>> get
>>>> back 'success' as result ->   -1
>>>>
>>>>  See my suggestion above.
>>>
>>>
>>>
>>>  regards,
>>>> gerhard
>>>>
>>>>
>>>>
>>>> 2012/3/26 Pete Muir<[hidden email]>
>>>>
>>>>  On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>>>>
>>>>>  On 25/03/12 09:09, Gerhard Petracek wrote:
>>>>>
>>>>>> hi shane,
>>>>>>>
>>>>>>> @ #1&    #2:
>>>>>>> i'm ok with it ->    please provide a concrete suggestion for #1
>>>>>>>
>>>>>>>  As I said previously I think that Identity should be in the base
>>>>>>
>>>>>>  package, i.e. org.apache.deltaspike.****security.api.Identity.
>>>>>  This bean
>>>>>
>>>>> deals
>>>>> with cross-cutting security concerns and it's a logical place for it to
>>>>> be.
>>>>>
>>>>>  @ #3:
>>>>>>
>>>>>>> imo we need a different event (and also a different term for
>>>>>>> "silent")
>>>>>>> -
>>>>>>> rest see #5
>>>>>>>
>>>>>>>  Do you mean AlreadyLoggedInEvent?  What would you suggest as an
>>>>>>
>>>>>>  alternative?  Also, I think quietLogin() is an apt description for
>>>>> this
>>>>> method, as its purpose is to attempt authentication if possible, and
>>>>> not
>>>>> to
>>>>> throw any exceptions or the usual events upon success or failure.  It's
>>>>> for
>>>>> non-explicit login where the credential information (or some other
>>>>> identifying token) is available when an unauthenticated user has
>>>>> attempted
>>>>> to invoke (directly or indirectly) either a privileged operation or
>>>>> privilege check.
>>>>>
>>>>> Silent or quiet login are quite well accepted terms for "try to login
>>>>> but
>>>>> don't error if it fails". You can google these terms to see where they
>>>>> have
>>>>> been used in the past.
>>>>>
>>>>>  @ #4:
>>>>>
>>>>>> that's usually true for gui-clients (see part 3), but not necessarily
>>>>>>>
>>>>>>>  for
>>>>>> other clients.
>>>>>>
>>>>>>> imo it's ok for this first step of part 1 (because the prev.
>>>>>>> behaviour
>>>>>>>
>>>>>>>  can
>>>>>> lead to side-effects which are hard/er to detect).
>>>>>>
>>>>>>> however, i knew from the beginning why you did it and imo it needs an
>>>>>>> additional concept as soon as we discuss the corresponding use-case
>>>>>>> (and
>>>>>>> the test for it fails).
>>>>>>>
>>>>>>>  Do you have a use case for this?  I still think it's a really
>>>>>> dangerous
>>>>>>
>>>>>>  thing to do, and I see no reason why a non-gui client couldn't just
>>>>> do an
>>>>> explicit log out before logging in again.
>>>>>
>>>>> I think this leads to a pretty odd situation. Throwing an exception if
>>>>> you
>>>>> try to login without logging out first seems much more explicit to me.
>>>>> Gerhard, can you expand on what you see as the problem here, and the
>>>>> use
>>>>> cases where it is a problem?
>>>>>
>>>>>  @ #5:
>>>>>
>>>>>> this scenario isn't necessarily a topic for part 1 ->    imo it's a
>>>>>>> topic
>>>>>>>
>>>>>>>  for
>>>>>> part 3 and it needs further discussions.
>>>>>> No problem, we can put it on the backburner for now.
>>>>>>
>>>>>>  regards,
>>>>>>
>>>>>>> gerhard
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2012/3/24 Shane Bryzak<[hidden email]>
>>>>>>>
>>>>>>>   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 (****isAuthenticationRequestWithDif**
>>>>>>>> **ferentUserId())
>>>>>>>>
>>>>>>>>             {
>>>>>>>>
>>>>>>>>                 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Shane Bryzak-2
Ah yes, I see the code now.  From memory, this was requested so that
users could write navigation rules to redirect to some pretty error page
if there was an authentication exception.  I'm happy if you change it
for now, and we can discuss it further later.

On 28/03/12 07:33, Gerhard Petracek wrote:

> hi shane,
>
> @ AuthenticationResult.EXCEPTION:
> we are talking about the same.
> however, in the master we have:
>
> public AuthenticationResult login()
> {
>      try
>      {
>          //login logic
>      }
>      catch (Exception ex)
>      {
>          beanManager.fireEvent(new LoginFailedEvent(ex));
>          return AuthenticationResult.exception;
>      }
> }
>
> and that was/is my objection.
>
> since we agree on it, i'll also change it (for now) to show what i've in
> mind.
> it's a quite small part ->  we can discuss the result after the commit.
>
> ->  i'll commit it and add information about the removed parts to the jira
> issue - so we can find and check out the previous state easily.
>
> regards,
> gerhard
>
>
>
> 2012/3/27 Shane Bryzak<[hidden email]>
>
>> On 28/03/12 03:21, Gerhard Petracek wrote:
>>
>>> hi shane,
>>>
>>> i changed #1, #2 and #4 (see [1]).
>>>
>>> i agree with jason that we should discuss the rest based on further
>>> use-cases.
>>> since we already agreed on postponing the rest, we should be fine for now.
>>>
>> +1, I've reviewed the patch and it looks ok for Part 1.
>>
>>
>>> the last topic i found for this first step is
>>> AuthenticationResult.EXCEPTION
>>> we could think about using the trick we have in our ExceptionUtils (which
>>> need to be discussed in any case) instead of using
>>> AuthenticationResult.EXCEPTION (if it is only used to avoid the issue
>>> with catched checked exceptions).
>>>
>> I'm not sure in which circumstances it would make sense to return
>> AuthenticationResult.EXCEPTION instead of just throwing an exception,
>> however we can certainly discuss this further.
>>
>>> regards,
>>> gerhard
>>>
>>> [1] https://issues.apache.org/**jira/browse/DELTASPIKE-127<https://issues.apache.org/jira/browse/DELTASPIKE-127>
>>>
>>>
>>>
>>> 2012/3/27 Shane Bryzak<[hidden email]>
>>>
>>>   On 26/03/12 21:34, Gerhard Petracek wrote:
>>>>   hi @ all,
>>>>> @ #1:
>>>>> +0.5 (which is enough for me to move it).
>>>>>
>>>>> @ #3:
>>>>> the current version on the master is:
>>>>>
>>>>> IdentityImpl#login:
>>>>> //...
>>>>> if (isLoggedIn())
>>>>> {
>>>>>      if (requestSecurityState.get().****isSilentLogin())
>>>>>
>>>>>      {
>>>>>          beanManager.fireEvent(new LoggedInEvent(user));
>>>>>          return AuthenticationResult.success;
>>>>>      }
>>>>>
>>>>>      beanManager.fireEvent(new AlreadyLoggedInEvent());
>>>>>      return AuthenticationResult.success;
>>>>> }
>>>>> //...
>>>>>
>>>>> LoggedInEvent isn't correct here imo and if we change it to
>>>>> AlreadyLoggedInEvent, it's the same as the non-silent login.
>>>>> however, since we postpone #5 we can discuss it later.
>>>>>
>>>>>   The business logic here is correct - if the user has been silently
>>>> authenticated during an explicit attempt to log in, then the
>>>> LoggedInEvent
>>>> should still be fired and a "success" value returned.  What happens next
>>>> in
>>>> the case where a silent login *hasn't* occurred I agree, the "success"
>>>> may
>>>> not be the most appropriate result.  My proposal here is to introduce a
>>>> new
>>>> enum value for the scenario where an already authenticated user attempts
>>>> to
>>>> log in again - possible examples for this may be something like
>>>> LOGGED_IN,
>>>> UNKNOWN or UNDETERMINED.
>>>>
>>>>
>>>>   @ #4:
>>>>> you can use the same argument for the login - just use:
>>>>> if (!identity.isLoggedIn())
>>>>> {
>>>>>      identity.login();
>>>>> }
>>>>>
>>>>> the autom. logout in case of an unexpected constellation is a
>>>>> requirement
>>>>> in some applications i know.
>>>>> however, i'm also ok with e.g. an UnexpectedCredentialException
>>>>>
>>>>>   I'm +1 on throwing either an UnexpectedCredentialException here, or
>>>> returning one of the enum values I suggested above and allowing the
>>>> developer to deal with the return code.
>>>>
>>>>
>>>>
>>>>   with the current version the new credentials are just ignored and you
>>>>> get
>>>>> back 'success' as result ->    -1
>>>>>
>>>>>   See my suggestion above.
>>>>
>>>>
>>>>   regards,
>>>>> gerhard
>>>>>
>>>>>
>>>>>
>>>>> 2012/3/26 Pete Muir<[hidden email]>
>>>>>
>>>>>   On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>>>>>
>>>>>>   On 25/03/12 09:09, Gerhard Petracek wrote:
>>>>>>
>>>>>>> hi shane,
>>>>>>>> @ #1&     #2:
>>>>>>>> i'm ok with it ->     please provide a concrete suggestion for #1
>>>>>>>>
>>>>>>>>   As I said previously I think that Identity should be in the base
>>>>>>>   package, i.e. org.apache.deltaspike.****security.api.Identity.
>>>>>>   This bean
>>>>>>
>>>>>> deals
>>>>>> with cross-cutting security concerns and it's a logical place for it to
>>>>>> be.
>>>>>>
>>>>>>   @ #3:
>>>>>>>> imo we need a different event (and also a different term for
>>>>>>>> "silent")
>>>>>>>> -
>>>>>>>> rest see #5
>>>>>>>>
>>>>>>>>   Do you mean AlreadyLoggedInEvent?  What would you suggest as an
>>>>>>>   alternative?  Also, I think quietLogin() is an apt description for
>>>>>> this
>>>>>> method, as its purpose is to attempt authentication if possible, and
>>>>>> not
>>>>>> to
>>>>>> throw any exceptions or the usual events upon success or failure.  It's
>>>>>> for
>>>>>> non-explicit login where the credential information (or some other
>>>>>> identifying token) is available when an unauthenticated user has
>>>>>> attempted
>>>>>> to invoke (directly or indirectly) either a privileged operation or
>>>>>> privilege check.
>>>>>>
>>>>>> Silent or quiet login are quite well accepted terms for "try to login
>>>>>> but
>>>>>> don't error if it fails". You can google these terms to see where they
>>>>>> have
>>>>>> been used in the past.
>>>>>>
>>>>>>   @ #4:
>>>>>>
>>>>>>> that's usually true for gui-clients (see part 3), but not necessarily
>>>>>>>>   for
>>>>>>> other clients.
>>>>>>>
>>>>>>>> imo it's ok for this first step of part 1 (because the prev.
>>>>>>>> behaviour
>>>>>>>>
>>>>>>>>   can
>>>>>>> lead to side-effects which are hard/er to detect).
>>>>>>>
>>>>>>>> however, i knew from the beginning why you did it and imo it needs an
>>>>>>>> additional concept as soon as we discuss the corresponding use-case
>>>>>>>> (and
>>>>>>>> the test for it fails).
>>>>>>>>
>>>>>>>>   Do you have a use case for this?  I still think it's a really
>>>>>>> dangerous
>>>>>>>
>>>>>>>   thing to do, and I see no reason why a non-gui client couldn't just
>>>>>> do an
>>>>>> explicit log out before logging in again.
>>>>>>
>>>>>> I think this leads to a pretty odd situation. Throwing an exception if
>>>>>> you
>>>>>> try to login without logging out first seems much more explicit to me.
>>>>>> Gerhard, can you expand on what you see as the problem here, and the
>>>>>> use
>>>>>> cases where it is a problem?
>>>>>>
>>>>>>   @ #5:
>>>>>>
>>>>>>> this scenario isn't necessarily a topic for part 1 ->     imo it's a
>>>>>>>> topic
>>>>>>>>
>>>>>>>>   for
>>>>>>> part 3 and it needs further discussions.
>>>>>>> No problem, we can put it on the backburner for now.
>>>>>>>
>>>>>>>   regards,
>>>>>>>
>>>>>>>> gerhard
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 2012/3/24 Shane Bryzak<[hidden email]>
>>>>>>>>
>>>>>>>>    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 (****isAuthenticationRequestWithDif**
>>>>>>>>> **ferentUserId())
>>>>>>>>>
>>>>>>>>>              {
>>>>>>>>>
>>>>>>>>>                  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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout

Jason Porter
That's exactly why it's in there. Came from the ability of Seam 2 to
redirect to a different page on errors. Not using the web.xml method.

On Tue, Mar 27, 2012 at 15:46, Shane Bryzak <[hidden email]> wrote:

> Ah yes, I see the code now.  From memory, this was requested so that users
> could write navigation rules to redirect to some pretty error page if there
> was an authentication exception.  I'm happy if you change it for now, and
> we can discuss it further later.
>
>
> On 28/03/12 07:33, Gerhard Petracek wrote:
>
>> hi shane,
>>
>> @ AuthenticationResult.**EXCEPTION:
>> we are talking about the same.
>> however, in the master we have:
>>
>> public AuthenticationResult login()
>> {
>>     try
>>     {
>>         //login logic
>>     }
>>     catch (Exception ex)
>>     {
>>         beanManager.fireEvent(new LoginFailedEvent(ex));
>>         return AuthenticationResult.**exception;
>>     }
>> }
>>
>> and that was/is my objection.
>>
>> since we agree on it, i'll also change it (for now) to show what i've in
>> mind.
>> it's a quite small part ->  we can discuss the result after the commit.
>>
>> ->  i'll commit it and add information about the removed parts to the jira
>> issue - so we can find and check out the previous state easily.
>>
>> regards,
>> gerhard
>>
>>
>>
>> 2012/3/27 Shane Bryzak<[hidden email]>
>>
>>  On 28/03/12 03:21, Gerhard Petracek wrote:
>>>
>>>  hi shane,
>>>>
>>>> i changed #1, #2 and #4 (see [1]).
>>>>
>>>> i agree with jason that we should discuss the rest based on further
>>>> use-cases.
>>>> since we already agreed on postponing the rest, we should be fine for
>>>> now.
>>>>
>>>>  +1, I've reviewed the patch and it looks ok for Part 1.
>>>
>>>
>>>  the last topic i found for this first step is
>>>> AuthenticationResult.EXCEPTION
>>>> we could think about using the trick we have in our ExceptionUtils
>>>> (which
>>>> need to be discussed in any case) instead of using
>>>> AuthenticationResult.EXCEPTION (if it is only used to avoid the issue
>>>> with catched checked exceptions).
>>>>
>>>>  I'm not sure in which circumstances it would make sense to return
>>> AuthenticationResult.EXCEPTION instead of just throwing an exception,
>>> however we can certainly discuss this further.
>>>
>>>  regards,
>>>> gerhard
>>>>
>>>> [1] https://issues.apache.org/****jira/browse/DELTASPIKE-127<https://issues.apache.org/**jira/browse/DELTASPIKE-127>
>>>> <htt**ps://issues.apache.org/jira/**browse/DELTASPIKE-127<https://issues.apache.org/jira/browse/DELTASPIKE-127>
>>>> >
>>>>
>>>>
>>>>
>>>> 2012/3/27 Shane Bryzak<[hidden email]>
>>>>
>>>>  On 26/03/12 21:34, Gerhard Petracek wrote:
>>>>
>>>>>  hi @ all,
>>>>>
>>>>>> @ #1:
>>>>>> +0.5 (which is enough for me to move it).
>>>>>>
>>>>>> @ #3:
>>>>>> the current version on the master is:
>>>>>>
>>>>>> IdentityImpl#login:
>>>>>> //...
>>>>>> if (isLoggedIn())
>>>>>> {
>>>>>>     if (requestSecurityState.get().******isSilentLogin())
>>>>>>
>>>>>>     {
>>>>>>         beanManager.fireEvent(new LoggedInEvent(user));
>>>>>>         return AuthenticationResult.success;
>>>>>>     }
>>>>>>
>>>>>>     beanManager.fireEvent(new AlreadyLoggedInEvent());
>>>>>>     return AuthenticationResult.success;
>>>>>> }
>>>>>> //...
>>>>>>
>>>>>> LoggedInEvent isn't correct here imo and if we change it to
>>>>>> AlreadyLoggedInEvent, it's the same as the non-silent login.
>>>>>> however, since we postpone #5 we can discuss it later.
>>>>>>
>>>>>>  The business logic here is correct - if the user has been silently
>>>>>>
>>>>> authenticated during an explicit attempt to log in, then the
>>>>> LoggedInEvent
>>>>> should still be fired and a "success" value returned.  What happens
>>>>> next
>>>>> in
>>>>> the case where a silent login *hasn't* occurred I agree, the "success"
>>>>> may
>>>>> not be the most appropriate result.  My proposal here is to introduce a
>>>>> new
>>>>> enum value for the scenario where an already authenticated user
>>>>> attempts
>>>>> to
>>>>> log in again - possible examples for this may be something like
>>>>> LOGGED_IN,
>>>>> UNKNOWN or UNDETERMINED.
>>>>>
>>>>>
>>>>>  @ #4:
>>>>>
>>>>>> you can use the same argument for the login - just use:
>>>>>> if (!identity.isLoggedIn())
>>>>>> {
>>>>>>     identity.login();
>>>>>> }
>>>>>>
>>>>>> the autom. logout in case of an unexpected constellation is a
>>>>>> requirement
>>>>>> in some applications i know.
>>>>>> however, i'm also ok with e.g. an UnexpectedCredentialException
>>>>>>
>>>>>>  I'm +1 on throwing either an UnexpectedCredentialException here, or
>>>>>>
>>>>> returning one of the enum values I suggested above and allowing the
>>>>> developer to deal with the return code.
>>>>>
>>>>>
>>>>>
>>>>>  with the current version the new credentials are just ignored and you
>>>>>
>>>>>> get
>>>>>> back 'success' as result ->    -1
>>>>>>
>>>>>>  See my suggestion above.
>>>>>>
>>>>>
>>>>>
>>>>>  regards,
>>>>>
>>>>>> gerhard
>>>>>>
>>>>>>
>>>>>>
>>>>>> 2012/3/26 Pete Muir<[hidden email]>
>>>>>>
>>>>>>  On 25 Mar 2012, at 22:03, Shane Bryzak wrote:
>>>>>>
>>>>>>   On 25/03/12 09:09, Gerhard Petracek wrote:
>>>>>>>
>>>>>>>  hi shane,
>>>>>>>>
>>>>>>>>> @ #1&     #2:
>>>>>>>>> i'm ok with it ->     please provide a concrete suggestion for #1
>>>>>>>>>
>>>>>>>>>  As I said previously I think that Identity should be in the base
>>>>>>>>>
>>>>>>>>  package, i.e. org.apache.deltaspike.******security.api.Identity.
>>>>>>>>
>>>>>>>  This bean
>>>>>>>
>>>>>>> deals
>>>>>>> with cross-cutting security concerns and it's a logical place for it
>>>>>>> to
>>>>>>> be.
>>>>>>>
>>>>>>>  @ #3:
>>>>>>>
>>>>>>>> imo we need a different event (and also a different term for
>>>>>>>>> "silent")
>>>>>>>>> -
>>>>>>>>> rest see #5
>>>>>>>>>
>>>>>>>>>  Do you mean AlreadyLoggedInEvent?  What would you suggest as an
>>>>>>>>>
>>>>>>>>  alternative?  Also, I think quietLogin() is an apt description for
>>>>>>>>
>>>>>>> this
>>>>>>> method, as its purpose is to attempt authentication if possible, and
>>>>>>> not
>>>>>>> to
>>>>>>> throw any exceptions or the usual events upon success or failure.
>>>>>>>  It's
>>>>>>> for
>>>>>>> non-explicit login where the credential information (or some other
>>>>>>> identifying token) is available when an unauthenticated user has
>>>>>>> attempted
>>>>>>> to invoke (directly or indirectly) either a privileged operation or
>>>>>>> privilege check.
>>>>>>>
>>>>>>> Silent or quiet login are quite well accepted terms for "try to login
>>>>>>> but
>>>>>>> don't error if it fails". You can google these terms to see where
>>>>>>> they
>>>>>>> have
>>>>>>> been used in the past.
>>>>>>>
>>>>>>>  @ #4:
>>>>>>>
>>>>>>>  that's usually true for gui-clients (see part 3), but not
>>>>>>>> necessarily
>>>>>>>>
>>>>>>>>>  for
>>>>>>>>>
>>>>>>>> other clients.
>>>>>>>>
>>>>>>>>  imo it's ok for this first step of part 1 (because the prev.
>>>>>>>>> behaviour
>>>>>>>>>
>>>>>>>>>  can
>>>>>>>>>
>>>>>>>> lead to side-effects which are hard/er to detect).
>>>>>>>>
>>>>>>>>  however, i knew from the beginning why you did it and imo it needs
>>>>>>>>> an
>>>>>>>>> additional concept as soon as we discuss the corresponding use-case
>>>>>>>>> (and
>>>>>>>>> the test for it fails).
>>>>>>>>>
>>>>>>>>>  Do you have a use case for this?  I still think it's a really
>>>>>>>>>
>>>>>>>> dangerous
>>>>>>>>
>>>>>>>>  thing to do, and I see no reason why a non-gui client couldn't just
>>>>>>>>
>>>>>>> do an
>>>>>>> explicit log out before logging in again.
>>>>>>>
>>>>>>> I think this leads to a pretty odd situation. Throwing an exception
>>>>>>> if
>>>>>>> you
>>>>>>> try to login without logging out first seems much more explicit to
>>>>>>> me.
>>>>>>> Gerhard, can you expand on what you see as the problem here, and the
>>>>>>> use
>>>>>>> cases where it is a problem?
>>>>>>>
>>>>>>>  @ #5:
>>>>>>>
>>>>>>>  this scenario isn't necessarily a topic for part 1 ->     imo it's a
>>>>>>>>
>>>>>>>>> topic
>>>>>>>>>
>>>>>>>>>  for
>>>>>>>>>
>>>>>>>> part 3 and it needs further discussions.
>>>>>>>> No problem, we can put it on the backburner for now.
>>>>>>>>
>>>>>>>>  regards,
>>>>>>>>
>>>>>>>>  gerhard
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2012/3/24 Shane Bryzak<[hidden email]>
>>>>>>>>>
>>>>>>>>>   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 (******isAuthenticationRequestWithDif****
>>>>>>>>>> **ferentUserId())
>>>>>>>>>>
>>>>>>>>>>             {
>>>>>>>>>>
>>>>>>>>>>                 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>


--
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp

Software Engineer
Open Source Advocate
Author of Seam Catch - Next Generation Java Exception Handling

PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu