Mailing-List: contact deltaspike-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: deltaspike-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of sbryzak@redhat.com designates
 209.132.183.28 as permitted sender)
Message-ID: <4F6F8834.1010006@redhat.com>
Date: Mon, 26 Mar 2012 07:03:48 +1000
From: Shane Bryzak <sbryzak@redhat.com>
User-Agent: Mozilla/5.0 (X11; Linux i686;
 rv:9.0) Gecko/20111222 Thunderbird/9.0
MIME-Version: 1.0
To: deltaspike-dev@incubator.apache.org
CC: Gerhard Petracek <gerhard.petracek@gmail.com>
Subject: Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout
References: 
 <CAGJtJfGMM2weQX1BKkfHUfC0AOBBJaBYozmk_5nNJCfXYbc2jA@mail.gmail.com>
 <4F6E412A.6070202@redhat.com>
 <CAGJtJfGqa_mh-XmXst91zdcbZqDs6g3NYBFXw8GqSbGFp1t+nw@mail.gmail.com>
In-Reply-To: 
 <CAGJtJfGqa_mh-XmXst91zdcbZqDs6g3NYBFXw8GqSbGFp1t+nw@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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<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 (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
>>
>>
>>