Return-Path: X-Original-To: apmail-incubator-deltaspike-dev-archive@minotaur.apache.org Delivered-To: apmail-incubator-deltaspike-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id E8CE7907E for ; Tue, 27 Mar 2012 22:38:10 +0000 (UTC) Received: (qmail 18446 invoked by uid 500); 27 Mar 2012 22:38:10 -0000 Delivered-To: apmail-incubator-deltaspike-dev-archive@incubator.apache.org Received: (qmail 18406 invoked by uid 500); 27 Mar 2012 22:38:10 -0000 Mailing-List: contact deltaspike-dev-help@incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: deltaspike-dev@incubator.apache.org Delivered-To: mailing list deltaspike-dev@incubator.apache.org Received: (qmail 18398 invoked by uid 99); 27 Mar 2012 22:38:10 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 27 Mar 2012 22:38:10 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of lightguard.jp@gmail.com designates 209.85.215.47 as permitted sender) Received: from [209.85.215.47] (HELO mail-lpp01m010-f47.google.com) (209.85.215.47) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 27 Mar 2012 22:38:03 +0000 Received: by lagw12 with SMTP id w12so494679lag.6 for ; Tue, 27 Mar 2012 15:37:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=wJ1+eDFF0KwMfyWGrudv7RMTSKPYVBevU22TdSI1fSo=; b=VWSjzIlPjVDUsYxeCw/5THI4pt5wiVJ6SO6fefsz3sd002PtNBGH7dWs7vVYUDoqKd clriLNWlbwJls2F6unen/+4MGlqqUVa53TtXqKX1BB9PNsVDjHWlwEntCbEgF126umAf nJGOn/UkV0zeZMACri/hEq5L4pRNvh/AeR5UVaTj6InycuAYgq56fA3LMUsA2iKCYdva Crz4uQM5eGkjDFgCHkKlvN0yq9MF2IYXayIRgr8EYwWiHaec6e+lraHkFQGrjEazrC5T 1Jr4I6RQO0EdgGPpIEq0xLoKZr+r0ltXerr5IatSK37OQCJJhdLYwtdHsppSNAfgfluS 7LAA== Received: by 10.152.110.116 with SMTP id hz20mr20033018lab.33.1332887862435; Tue, 27 Mar 2012 15:37:42 -0700 (PDT) MIME-Version: 1.0 Received: by 10.112.65.21 with HTTP; Tue, 27 Mar 2012 15:37:22 -0700 (PDT) In-Reply-To: <4F723546.9000705@redhat.com> References: <4F6E412A.6070202@redhat.com> <4F6F8834.1010006@redhat.com> <4F70F380.4010301@redhat.com> <4F722A24.3050204@redhat.com> <4F723546.9000705@redhat.com> From: Jason Porter Date: Tue, 27 Mar 2012 16:37:22 -0600 Message-ID: Subject: Re: [DISCUSS] [DELTASPIKE-76/127] security module - login/logout To: deltaspike-dev@incubator.apache.org Cc: Gerhard Petracek Content-Type: multipart/alternative; boundary=bcaec548581090ded404bc41239d --bcaec548581090ded404bc41239d Content-Type: text/plain; charset=UTF-8 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 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 >> >> 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 >>>> >>>> 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 >>>>>> >>>>>> 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 >>>>>>>>> >>>>>>>>> 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 --bcaec548581090ded404bc41239d--