Re: Regarding SHIRO-110: Remove org.apache.shiro.mgt.SubjectBinder and its usages

[Les Hazlewood <lh...@apache.org>]

Just catching up after the long weekend off...

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.

We're ultimately pushing the responsibility of managing the runtime
Subject instance based on the client technology, which I think is a
smart idea - the runtime environment could vary dramatically based on
client and it is best managed based on the deployment environment.
ThreadLocal binding is not always the best approach (for example,
maybe not necessary or even desirable in a standalone client
application).

In reviewing this code a month ago, I thought heavily about
introducing something like a SubjectAccessor interface that would be
used by SecurityUtils
(SecurityUtils.setSubjectAccessor/getSubjectAccessor) to allow
SecurityUtils to be configured with the lookup strategy appropriate
for the application.

Thoughts on this?

On Sun, Feb 14, 2010 at 12:55 AM, Kalle Korhonen
<ka...@gmail.com> wrote:
> As we are marching towards the 1.0.0 I decided assign a few more
> issues to myself, one of them being Regarding SHIRO-110: Remove
> org.apache.shiro.mgt.SubjectBinder and its usages. There really
> weren't too many places left so it's an easy removal. The only only
> area where I'm not completely sure what to do is
> SecurityManager.getSubject(). Supposedly an implementation could
> always create a subject but there's createSubject for that purpose so
> getSubject() might be redundant and somewhat misleading.
> SecurityManager.getSubject was added in 0.9.  Should we deprecate
> SecurityManager.getSubject() or outright remove it? If we leave it
> alone or just deprecate, DefaultSecurityManager.getSubject() could
> call ThreadContext.getSubject() - same as SecurityUtils and as a
> fallback createSubject(new HashMap()). SecurityManager.getSubject() is
> only called from SecurityUtils within Shiro but SecurityManager is a
> fairly visible interface.
>
> Les, do you think we should getSubject() as is and provide the default
> implementation as suggested? If it's simpler to talk when you see the
> code, I could commit the suggested implementation and we could review
> afterwards.
>
> Kalle
>

Re: Regarding SHIRO-110: Remove org.apache.shiro.mgt.SubjectBinder and its usages

[Les Hazlewood <lh...@apache.org>]

>> In reviewing this code a month ago, I thought heavily about
>> introducing something like a SubjectAccessor interface that would be
>> used by SecurityUtils
>> (SecurityUtils.setSubjectAccessor/getSubjectAccessor) to allow
>> SecurityUtils to be configured with the lookup strategy appropriate
>> for the application.
>> Thoughts on this?
>
> An improvement we can make later. If you don't absolutely need it and
> I don't absolutely need it, the chances are that the community as well
> can get by without it for 1.0.0.

+1

I agree.

Les

Re: Regarding SHIRO-110: Remove org.apache.shiro.mgt.SubjectBinder and its usages

[Les Hazlewood <lh...@apache.org>]

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
<ka...@gmail.com> wrote:
> On Tue, Feb 16, 2010 at 7:55 PM, Kalle Korhonen
> <ka...@gmail.com> wrote:
>> On Tue, Feb 16, 2010 at 7:12 AM, Les Hazlewood <lh...@apache.org> 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
>

Re: Regarding SHIRO-110: Remove org.apache.shiro.mgt.SubjectBinder and its usages

[Kalle Korhonen <ka...@gmail.com>]

On Tue, Feb 16, 2010 at 7:55 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> On Tue, Feb 16, 2010 at 7:12 AM, Les Hazlewood <lh...@apache.org> 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

Re: Regarding SHIRO-110: Remove org.apache.shiro.mgt.SubjectBinder and its usages

[Kalle Korhonen <ka...@gmail.com>]

On Tue, Feb 16, 2010 at 7:12 AM, Les Hazlewood <lh...@apache.org> 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.

Great, then we are perfectly in sync - that's what I expected and was
reading from between the lines.

> We're ultimately pushing the responsibility of managing the runtime
> Subject instance based on the client technology, which I think is a
> smart idea - the runtime environment could vary dramatically based on
> client and it is best managed based on the deployment environment.
> ThreadLocal binding is not always the best approach (for example,
> maybe not necessary or even desirable in a standalone client
> application).

Attempting to manage state is always risky business.

> In reviewing this code a month ago, I thought heavily about
> introducing something like a SubjectAccessor interface that would be
> used by SecurityUtils
> (SecurityUtils.setSubjectAccessor/getSubjectAccessor) to allow
> SecurityUtils to be configured with the lookup strategy appropriate
> for the application.
> Thoughts on this?

An improvement we can make later. If you don't absolutely need it and
I don't absolutely need it, the chances are that the community as well
can get by without it for 1.0.0.

Kalle


> On Sun, Feb 14, 2010 at 12:55 AM, Kalle Korhonen
> <ka...@gmail.com> wrote:
>> As we are marching towards the 1.0.0 I decided assign a few more
>> issues to myself, one of them being Regarding SHIRO-110: Remove
>> org.apache.shiro.mgt.SubjectBinder and its usages. There really
>> weren't too many places left so it's an easy removal. The only only
>> area where I'm not completely sure what to do is
>> SecurityManager.getSubject(). Supposedly an implementation could
>> always create a subject but there's createSubject for that purpose so
>> getSubject() might be redundant and somewhat misleading.
>> SecurityManager.getSubject was added in 0.9.  Should we deprecate
>> SecurityManager.getSubject() or outright remove it? If we leave it
>> alone or just deprecate, DefaultSecurityManager.getSubject() could
>> call ThreadContext.getSubject() - same as SecurityUtils and as a
>> fallback createSubject(new HashMap()). SecurityManager.getSubject() is
>> only called from SecurityUtils within Shiro but SecurityManager is a
>> fairly visible interface.
>>
>> Les, do you think we should getSubject() as is and provide the default
>> implementation as suggested? If it's simpler to talk when you see the
>> code, I could commit the suggested implementation and we could review
>> afterwards.
>>
>> Kalle
>>
>