incubator-deltaspike-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gerhard Petracek <gerhard.petra...@gmail.com>
Subject Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout
Date Tue, 27 Mar 2012 21:33:15 GMT
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>
>>
>>
>>
>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>

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