@ #4 I was also wondering about use case related to this. I agree with Shane that logout should be explicit operation because of possible side effects and not easy to foresee scenarios
Bolek On Mar 25, 2012, at 11:03 PM, 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. >> >> @ #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<[email protected]> >> >>> 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 >>> >>> >>> >
