Yep, that was my TODO :) This is a bit tricky and is related to the SubjectAccessor comment I related before - one mechanism should bind it in some way to the application for further use. Another pulls it some way to be used during code execution. I'd be nice to figure out a solution that feels clean and is valuable moving forward.
The Binder implementation hierarchy exists because there is always the crowd of people that may not want to save the subject state to the session to minimize or eliminate the need for sessions entirely (completely stateless architectures). That is why the SessionSubjectBinder is a subclass of the ThreadContextSubjectBinder - to allow an end-user to configure whether they want session binding to occur or not based on which binder they configure. Maybe for a 1.0 release we just move the session-binding implementation directly into the DefaultSecurityManager implementation as you suggested Kalle. Then we can focus on a cleaner way to enable this (or turn it off) in a way that's backwards compatible. The map-based approach for the SubjectFactory, while ignoring compile-time method argument safety, allows us a lot of flexibility to determine how this construction process might work in the future. Finally there is a way to have the Subject.Binder indicate this session binding is desired any time a Subject is constructed. We could have something like Subject.Binder().bindToSession(true);. That call would translate into a flag that is set in the subject context map sent to the SubjectFactory. The SubjectFactory implementation would return a DelegatingSubject instance that could execute the bind-to-session-after-login logic based on that flag (i.e. that logic wouldn't be in the SecurityManager implementation - it'd be in the Subject implementation). If we decide this is a good idea, it is definitely possible. - Les On Thu, Feb 18, 2010 at 12:50 AM, Kalle Korhonen <[email protected]> wrote: > On Tue, Feb 16, 2010 at 7:55 PM, Kalle Korhonen > <[email protected]> wrote: >> On Tue, Feb 16, 2010 at 7:12 AM, Les Hazlewood <[email protected]> wrote: >>> Yes, in my opinion, the SecurityManager#getSubject() call is redundant >>> and should be removed and SecurityUtils should be updated to have the >>> fallback - it could probably just use the Subject.Builder() directly. > > Integration tests save the day! Since I would have already broken it > had I gone ahead with the changes I had prepared. The issue is that > eventually the principals need to be bound to the session. Les, I > assume it was you who had added this TODO to > DefaultSecurityManager.login(): > //TODO - is binding necessary anymore? Shouldn't the Builders > or Builder callers do this now? > > Unfortunately it's still needed since in the builder we are still > building the subject so we cannot just move the > SessionSubjectBinder.bindToSession implementation to the builder. We > could move it to DefaultSecurityManager.bind() pretty much as is but > then we'd still need to pass the session around in several > DefaultSubjectFactory calls; getPrincipal() being the most critical > one (as we check the session for principals there). Soo... anybody > (Les?) have any better ideas for this or should I just go ahead with > what I suggested above and we can improve later? Overall, it still > feels to me that the internals for this could be simplified. > > Kalle >
