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 17:21:44 GMT
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 <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