deltaspike-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jason Porter <lightguard...@gmail.com>
Subject Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout
Date Tue, 27 Mar 2012 22:37:22 GMT
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 <sbryzak@redhat.com> 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<sbryzak@redhat.com>
>>
>>  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<sbryzak@redhat.com>
>>>>
>>>>  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<pmuir@redhat.com>
>>>>>>
>>>>>>  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<sbryzak@redhat.com>
>>>>>>>>>
>>>>>>>>>   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

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