Configuration overhaul update

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

I'm just about wrapping up my config modifications, and it is pretty
promising.  I want to outline what I'm doing before I commit to see if
anyone has any comments.  I'll probably commit sometime tomorrow
unless anyone has any objections.

This is long, but explains everything that I've done.  Review and
speak now or forever hold your peace! ;)

1. I added an org.apache.shiro.config.Ini class that works similarly
to java.util.Properties - it stands alone on its own and doesn't need
to extend any of the various *Resource classes - definitely nicer to
use and could even be used outside of Shiro for any INI-type access -
much cleaner than the previous org.apache.shiro.config.IniResource
implementation.  The test cases added for this class are great too and
are easier to understand.

2. I created an IniSecurityManagerFactory that implements
SecurityManagerFactory directly that takes in an Ini instance and
builds up the SecurityManager object graph as necessary.  This
replaces the old IniConfiguration class.

I like this because *Configuration classes were ambiguous - you don't
know by looking at the name if it is actually Configuration (i.e. text
or something else) or a factory.  I still don't know if
'IniSecurityManagerFactory' is an ideal name - any better ideas?

3. The IniSecurityManagerFactory only deals with SecurityManager
construction - it has no knowledge of how to construct filter chains
or resolve configured chains - something that I consider to be a
separate concern.  Previously the IniConfiguration class was
subclassed by WebIniConfiguration to provide this behavior, which in
retrospect, probably shouldn't have been done with inheritance.

To support composition-over-inheritance in this case, I'll need to
create a similar component for filter chain construction - maybe like
IniFilterChainResolverFactory that takes in the Ini instance and
builds a FilterChainResolver.

3.  The ShiroFilter implementation was changed to reference 2 internal
class attributes - a SecurityManager and a FilterChainResolver - both
of which can be constructed by the above Ini mechanism (or anything
else that anyone wants to use).  They are separate concerns, so this
continues the composition-over-inheritance design.

The big question left is do we keep ShiroFilter the same as it is and
backwards-compatible by having it use INI configuration by default?

Or do we make it an abstract class and then have, say, an
IniShiroFilter that knows how to work with INI and BeanUtils APIs for
object graph construction?  The abstract class would provide the
Subject creation and thread binding duties as normal and leave only
SecurityManager/FilterChainResolver building or acquisition to
subclasses.

- Les

Re: Configuration overhaul update

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

Just committed my change.

Start with the AbstractShiroFilter class and its only child,
IniShiroFilter.  Notice how much cleaner IniShiroFilter is, especially
by delegating to the WebIniSecurityManagerFactory.

Now I need to create an IniFilterChainResolverFactory to work in the same way.

On Fri, Oct 9, 2009 at 4:25 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> On Fri, Oct 9, 2009 at 12:43 PM, Les Hazlewood <lh...@apache.org> wrote:
>> On Fri, Oct 9, 2009 at 2:37 PM, Kalle Korhonen
>> <ka...@gmail.com> wrote:
>>> Regarding the topic #3, should ShiroFilter be abstract or not, I'm
>>> strongly for keeping it as is or even renaming it to IniShiroFilter
>>> (or ShiroIniFilter) without any abstract classes. The reason is that
>> I agree.  My idea about the abstract class was more to consolidate the
>> what-would-be-shared behavior of the current ShiroFilter (subject
>> creation, thread binding) to a parent class and leave
>> configuration-specific details to the children classes.
>> So maybe, call the abstract parent class SubjectFilter or something
>> like that.  Then have a (Ini)ShiroFilter that implements INI
>> configuration parsing & construction.
>> That way, if you're supporting a different configuration mechanism,
>> you would probably want to subclass SubjectFilter instead of
>> ShiroFilter.
>> Does this make sense?
>
> Agree, you do have a point there. If we want to allow extending Shiro
> with non-ini configuration mechanism while still using functionality
> in ShiroFilter, this is the way to do it.
>
>> I think it is probably necessary since both web and non-web
>> environments will need to parse INI and construct a SecurityManager.
>> By delegating this logic to a component, both the (Ini)ShiroFilter and
>> any other mechanism (Java code, IoC config, etc) could leverage the
>> same component.
>
> Yes, makes sense for the non-web use case.
>
> Kalle
>

Re: Configuration overhaul update

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

On Fri, Oct 9, 2009 at 12:43 PM, Les Hazlewood <lh...@apache.org> wrote:
> On Fri, Oct 9, 2009 at 2:37 PM, Kalle Korhonen
> <ka...@gmail.com> wrote:
>> Regarding the topic #3, should ShiroFilter be abstract or not, I'm
>> strongly for keeping it as is or even renaming it to IniShiroFilter
>> (or ShiroIniFilter) without any abstract classes. The reason is that
> I agree.  My idea about the abstract class was more to consolidate the
> what-would-be-shared behavior of the current ShiroFilter (subject
> creation, thread binding) to a parent class and leave
> configuration-specific details to the children classes.
> So maybe, call the abstract parent class SubjectFilter or something
> like that.  Then have a (Ini)ShiroFilter that implements INI
> configuration parsing & construction.
> That way, if you're supporting a different configuration mechanism,
> you would probably want to subclass SubjectFilter instead of
> ShiroFilter.
> Does this make sense?

Agree, you do have a point there. If we want to allow extending Shiro
with non-ini configuration mechanism while still using functionality
in ShiroFilter, this is the way to do it.

> I think it is probably necessary since both web and non-web
> environments will need to parse INI and construct a SecurityManager.
> By delegating this logic to a component, both the (Ini)ShiroFilter and
> any other mechanism (Java code, IoC config, etc) could leverage the
> same component.

Yes, makes sense for the non-web use case.

Kalle

Re: Configuration overhaul update

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

Hi Kalle,

Thanks for the feedback - much appreciated as always.  I'll answer in-line.

On Fri, Oct 9, 2009 at 2:37 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> But going forward, it'd be better to provide a more formal
> roadmap to our users with version and branching plans and sort out the
> changes to different release trains and development branches (and I
> don't mean it needs to be anything heavy, but something like open bug
> fix branch for 1.0.x and reserve trunk for 1.1.0).

I agree - I like doing the trunk/branch versions exactly as you've
specified, and I think we definitely need to make this clearer for the
end-users.  I think the first step to doing that is make Jira reflect
this.  I think once the first release is out it will be better in that
it will start to become habit.

> Regarding the topic #3, should ShiroFilter be abstract or not, I'm
> strongly for keeping it as is or even renaming it to IniShiroFilter
> (or ShiroIniFilter) without any abstract classes. The reason is that
> every user needs to make a conscious decision anyway to configure it
> in his web.xml and to configure the ini file as well. It's much more
> transparent if you configure a ShiroIniFilter, you'll know what you
> get. Another layer of abstraction to configure ShiroFilter as a
> filter, then provide configuration for it to instantiate a specific
> implementation doesn't add any value.

I agree.  My idea about the abstract class was more to consolidate the
what-would-be-shared behavior of the current ShiroFilter (subject
creation, thread binding) to a parent class and leave
configuration-specific details to the children classes.

So maybe, call the abstract parent class SubjectFilter or something
like that.  Then have a (Ini)ShiroFilter that implements INI
configuration parsing & construction.

That way, if you're supporting a different configuration mechanism,
you would probably want to subclass SubjectFilter instead of
ShiroFilter.

Does this make sense?

> if ShiroFilter or a base Shiro implementation would be
> auto-discovered, and you'd need to (re-)configure it to your specific
> environment. Along the same lines, I'm not even sure if
> IniSecurityManagerFactory is needed - could ShiroFilter not handle
> that directly?

I think it is probably necessary since both web and non-web
environments will need to parse INI and construct a SecurityManager.
By delegating this logic to a component, both the (Ini)ShiroFilter and
any other mechanism (Java code, IoC config, etc) could leverage the
same component.

> My opinion in short, go ahead and check in and we'll tackle the rest later.

Sounds good - I'll do that shortly.

Cheers,

Les

Re: Configuration overhaul update

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

Hi Erik - don't worry just yet ;)

I committed my change, but I haven't changed any of the existing
ShiroFilter classes or how they perform configuration.  The stuff I
added is entirely new and not referenced by existing configuration
mechanisms, so you won't see any side affects.

I'm ok with making a pre-release without deleting any of the old stuff
- we can deprecate what will be removed as of 1.0 final and just move
forward with the new configuration mechanism.  But of course we need
to discuss this with our Mentors and other team members to see what is
involved.

If you see any funny stuff at all, let me know and I'll jump right on
it.  I'd be surprised if there were any trouble though due to the fact
that it is all unreferenced new code.

Cheers,

Les

On Fri, Oct 9, 2009 at 7:07 PM, Erik Beeson <er...@gmail.com> wrote:
> On that note, any chance of a release before making major changes? I'm about
> to go into production with a snapshot on the basis that JSecurity is
> adequately battle-tested, but it would be nice if there were some sort of
> release, even a pre-release or RC or something.
>
> --Erik
>
>
> On Fri, Oct 9, 2009 at 11:37 AM, Kalle Korhonen
> <ka...@gmail.com>wrote:
>
>> I don't have any detailed, specific comments to any of your points
>> except for the last one so I'll just top post. The changes overall
>> look good and are inline with what we've discussed in different
>> threads and what you've indicated before. I'll have to see the code to
>> provide more substantial comments. One of the issues is that by now
>> since there hasn't been releases after the move to Incubator, there
>> are multiple consumers of the snapshots. I'm sure these can be worked
>> in without disturbing the most typical servlet/ini file configuration
>> use case but it'll force changes to the more esoteric implementations
>> (and judging from the posts to the user list I'm not the only one with
>> a non-standard use for Shiro). Anyway, I don't think its a huge issue
>> since these are snapshots after all, so you get what you ask for when
>> using them. But going forward, it'd be better to provide a more formal
>> roadmap to our users with version and branching plans and sort out the
>> changes to different release trains and development branches (and I
>> don't mean it needs to be anything heavy, but something like open bug
>> fix branch for 1.0.x and reserve trunk for 1.1.0). We can revisit that
>> after we have our first incubator release out (separate topic as
>> well).
>>
>> Regarding the topic #3, should ShiroFilter be abstract or not, I'm
>> strongly for keeping it as is or even renaming it to IniShiroFilter
>> (or ShiroIniFilter) without any abstract classes. The reason is that
>> every user needs to make a conscious decision anyway to configure it
>> in his web.xml and to configure the ini file as well. It's much more
>> transparent if you configure a ShiroIniFilter, you'll know what you
>> get. Another layer of abstraction to configure ShiroFilter as a
>> filter, then provide configuration for it to instantiate a specific
>> implementation doesn't add any value. The situation would be different
>> if ShiroFilter or a base Shiro implementation would be
>> auto-discovered, and you'd need to (re-)configure it to your specific
>> environment. Along the same lines, I'm not even sure if
>> IniSecurityManagerFactory is needed - could ShiroFilter not handle
>> that directly? But as said, we can take a further look into it once
>> all of us have the code in front of us.
>>
>> My opinion in short, go ahead and check in and we'll tackle the rest later.
>>
>> Kalle
>>
>>
>>
>> On Wed, Oct 7, 2009 at 1:29 PM, Les Hazlewood <lh...@apache.org>
>> wrote:
>> > I'm just about wrapping up my config modifications, and it is pretty
>> > promising.  I want to outline what I'm doing before I commit to see if
>> > anyone has any comments.  I'll probably commit sometime tomorrow
>> > unless anyone has any objections.
>> >
>> > This is long, but explains everything that I've done.  Review and
>> > speak now or forever hold your peace! ;)
>> >
>> > 1. I added an org.apache.shiro.config.Ini class that works similarly
>> > to java.util.Properties - it stands alone on its own and doesn't need
>> > to extend any of the various *Resource classes - definitely nicer to
>> > use and could even be used outside of Shiro for any INI-type access -
>> > much cleaner than the previous org.apache.shiro.config.IniResource
>> > implementation.  The test cases added for this class are great too and
>> > are easier to understand.
>> >
>> > 2. I created an IniSecurityManagerFactory that implements
>> > SecurityManagerFactory directly that takes in an Ini instance and
>> > builds up the SecurityManager object graph as necessary.  This
>> > replaces the old IniConfiguration class.
>> >
>> > I like this because *Configuration classes were ambiguous - you don't
>> > know by looking at the name if it is actually Configuration (i.e. text
>> > or something else) or a factory.  I still don't know if
>> > 'IniSecurityManagerFactory' is an ideal name - any better ideas?
>> >
>> > 3. The IniSecurityManagerFactory only deals with SecurityManager
>> > construction - it has no knowledge of how to construct filter chains
>> > or resolve configured chains - something that I consider to be a
>> > separate concern.  Previously the IniConfiguration class was
>> > subclassed by WebIniConfiguration to provide this behavior, which in
>> > retrospect, probably shouldn't have been done with inheritance.
>> >
>> > To support composition-over-inheritance in this case, I'll need to
>> > create a similar component for filter chain construction - maybe like
>> > IniFilterChainResolverFactory that takes in the Ini instance and
>> > builds a FilterChainResolver.
>> >
>> > 3.  The ShiroFilter implementation was changed to reference 2 internal
>> > class attributes - a SecurityManager and a FilterChainResolver - both
>> > of which can be constructed by the above Ini mechanism (or anything
>> > else that anyone wants to use).  They are separate concerns, so this
>> > continues the composition-over-inheritance design.
>> >
>> > The big question left is do we keep ShiroFilter the same as it is and
>> > backwards-compatible by having it use INI configuration by default?
>> >
>> > Or do we make it an abstract class and then have, say, an
>> > IniShiroFilter that knows how to work with INI and BeanUtils APIs for
>> > object graph construction?  The abstract class would provide the
>> > Subject creation and thread binding duties as normal and leave only
>> > SecurityManager/FilterChainResolver building or acquisition to
>> > subclasses.
>> >
>> > - Les
>> >
>>
>

Re: Configuration overhaul update

[Erik Beeson <er...@gmail.com>]

On that note, any chance of a release before making major changes? I'm about
to go into production with a snapshot on the basis that JSecurity is
adequately battle-tested, but it would be nice if there were some sort of
release, even a pre-release or RC or something.

--Erik


On Fri, Oct 9, 2009 at 11:37 AM, Kalle Korhonen
<ka...@gmail.com>wrote:

> I don't have any detailed, specific comments to any of your points
> except for the last one so I'll just top post. The changes overall
> look good and are inline with what we've discussed in different
> threads and what you've indicated before. I'll have to see the code to
> provide more substantial comments. One of the issues is that by now
> since there hasn't been releases after the move to Incubator, there
> are multiple consumers of the snapshots. I'm sure these can be worked
> in without disturbing the most typical servlet/ini file configuration
> use case but it'll force changes to the more esoteric implementations
> (and judging from the posts to the user list I'm not the only one with
> a non-standard use for Shiro). Anyway, I don't think its a huge issue
> since these are snapshots after all, so you get what you ask for when
> using them. But going forward, it'd be better to provide a more formal
> roadmap to our users with version and branching plans and sort out the
> changes to different release trains and development branches (and I
> don't mean it needs to be anything heavy, but something like open bug
> fix branch for 1.0.x and reserve trunk for 1.1.0). We can revisit that
> after we have our first incubator release out (separate topic as
> well).
>
> Regarding the topic #3, should ShiroFilter be abstract or not, I'm
> strongly for keeping it as is or even renaming it to IniShiroFilter
> (or ShiroIniFilter) without any abstract classes. The reason is that
> every user needs to make a conscious decision anyway to configure it
> in his web.xml and to configure the ini file as well. It's much more
> transparent if you configure a ShiroIniFilter, you'll know what you
> get. Another layer of abstraction to configure ShiroFilter as a
> filter, then provide configuration for it to instantiate a specific
> implementation doesn't add any value. The situation would be different
> if ShiroFilter or a base Shiro implementation would be
> auto-discovered, and you'd need to (re-)configure it to your specific
> environment. Along the same lines, I'm not even sure if
> IniSecurityManagerFactory is needed - could ShiroFilter not handle
> that directly? But as said, we can take a further look into it once
> all of us have the code in front of us.
>
> My opinion in short, go ahead and check in and we'll tackle the rest later.
>
> Kalle
>
>
>
> On Wed, Oct 7, 2009 at 1:29 PM, Les Hazlewood <lh...@apache.org>
> wrote:
> > I'm just about wrapping up my config modifications, and it is pretty
> > promising.  I want to outline what I'm doing before I commit to see if
> > anyone has any comments.  I'll probably commit sometime tomorrow
> > unless anyone has any objections.
> >
> > This is long, but explains everything that I've done.  Review and
> > speak now or forever hold your peace! ;)
> >
> > 1. I added an org.apache.shiro.config.Ini class that works similarly
> > to java.util.Properties - it stands alone on its own and doesn't need
> > to extend any of the various *Resource classes - definitely nicer to
> > use and could even be used outside of Shiro for any INI-type access -
> > much cleaner than the previous org.apache.shiro.config.IniResource
> > implementation.  The test cases added for this class are great too and
> > are easier to understand.
> >
> > 2. I created an IniSecurityManagerFactory that implements
> > SecurityManagerFactory directly that takes in an Ini instance and
> > builds up the SecurityManager object graph as necessary.  This
> > replaces the old IniConfiguration class.
> >
> > I like this because *Configuration classes were ambiguous - you don't
> > know by looking at the name if it is actually Configuration (i.e. text
> > or something else) or a factory.  I still don't know if
> > 'IniSecurityManagerFactory' is an ideal name - any better ideas?
> >
> > 3. The IniSecurityManagerFactory only deals with SecurityManager
> > construction - it has no knowledge of how to construct filter chains
> > or resolve configured chains - something that I consider to be a
> > separate concern.  Previously the IniConfiguration class was
> > subclassed by WebIniConfiguration to provide this behavior, which in
> > retrospect, probably shouldn't have been done with inheritance.
> >
> > To support composition-over-inheritance in this case, I'll need to
> > create a similar component for filter chain construction - maybe like
> > IniFilterChainResolverFactory that takes in the Ini instance and
> > builds a FilterChainResolver.
> >
> > 3.  The ShiroFilter implementation was changed to reference 2 internal
> > class attributes - a SecurityManager and a FilterChainResolver - both
> > of which can be constructed by the above Ini mechanism (or anything
> > else that anyone wants to use).  They are separate concerns, so this
> > continues the composition-over-inheritance design.
> >
> > The big question left is do we keep ShiroFilter the same as it is and
> > backwards-compatible by having it use INI configuration by default?
> >
> > Or do we make it an abstract class and then have, say, an
> > IniShiroFilter that knows how to work with INI and BeanUtils APIs for
> > object graph construction?  The abstract class would provide the
> > Subject creation and thread binding duties as normal and leave only
> > SecurityManager/FilterChainResolver building or acquisition to
> > subclasses.
> >
> > - Les
> >
>

Re: Configuration overhaul update

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

I don't have any detailed, specific comments to any of your points
except for the last one so I'll just top post. The changes overall
look good and are inline with what we've discussed in different
threads and what you've indicated before. I'll have to see the code to
provide more substantial comments. One of the issues is that by now
since there hasn't been releases after the move to Incubator, there
are multiple consumers of the snapshots. I'm sure these can be worked
in without disturbing the most typical servlet/ini file configuration
use case but it'll force changes to the more esoteric implementations
(and judging from the posts to the user list I'm not the only one with
a non-standard use for Shiro). Anyway, I don't think its a huge issue
since these are snapshots after all, so you get what you ask for when
using them. But going forward, it'd be better to provide a more formal
roadmap to our users with version and branching plans and sort out the
changes to different release trains and development branches (and I
don't mean it needs to be anything heavy, but something like open bug
fix branch for 1.0.x and reserve trunk for 1.1.0). We can revisit that
after we have our first incubator release out (separate topic as
well).

Regarding the topic #3, should ShiroFilter be abstract or not, I'm
strongly for keeping it as is or even renaming it to IniShiroFilter
(or ShiroIniFilter) without any abstract classes. The reason is that
every user needs to make a conscious decision anyway to configure it
in his web.xml and to configure the ini file as well. It's much more
transparent if you configure a ShiroIniFilter, you'll know what you
get. Another layer of abstraction to configure ShiroFilter as a
filter, then provide configuration for it to instantiate a specific
implementation doesn't add any value. The situation would be different
if ShiroFilter or a base Shiro implementation would be
auto-discovered, and you'd need to (re-)configure it to your specific
environment. Along the same lines, I'm not even sure if
IniSecurityManagerFactory is needed - could ShiroFilter not handle
that directly? But as said, we can take a further look into it once
all of us have the code in front of us.

My opinion in short, go ahead and check in and we'll tackle the rest later.

Kalle



On Wed, Oct 7, 2009 at 1:29 PM, Les Hazlewood <lh...@apache.org> wrote:
> I'm just about wrapping up my config modifications, and it is pretty
> promising.  I want to outline what I'm doing before I commit to see if
> anyone has any comments.  I'll probably commit sometime tomorrow
> unless anyone has any objections.
>
> This is long, but explains everything that I've done.  Review and
> speak now or forever hold your peace! ;)
>
> 1. I added an org.apache.shiro.config.Ini class that works similarly
> to java.util.Properties - it stands alone on its own and doesn't need
> to extend any of the various *Resource classes - definitely nicer to
> use and could even be used outside of Shiro for any INI-type access -
> much cleaner than the previous org.apache.shiro.config.IniResource
> implementation.  The test cases added for this class are great too and
> are easier to understand.
>
> 2. I created an IniSecurityManagerFactory that implements
> SecurityManagerFactory directly that takes in an Ini instance and
> builds up the SecurityManager object graph as necessary.  This
> replaces the old IniConfiguration class.
>
> I like this because *Configuration classes were ambiguous - you don't
> know by looking at the name if it is actually Configuration (i.e. text
> or something else) or a factory.  I still don't know if
> 'IniSecurityManagerFactory' is an ideal name - any better ideas?
>
> 3. The IniSecurityManagerFactory only deals with SecurityManager
> construction - it has no knowledge of how to construct filter chains
> or resolve configured chains - something that I consider to be a
> separate concern.  Previously the IniConfiguration class was
> subclassed by WebIniConfiguration to provide this behavior, which in
> retrospect, probably shouldn't have been done with inheritance.
>
> To support composition-over-inheritance in this case, I'll need to
> create a similar component for filter chain construction - maybe like
> IniFilterChainResolverFactory that takes in the Ini instance and
> builds a FilterChainResolver.
>
> 3.  The ShiroFilter implementation was changed to reference 2 internal
> class attributes - a SecurityManager and a FilterChainResolver - both
> of which can be constructed by the above Ini mechanism (or anything
> else that anyone wants to use).  They are separate concerns, so this
> continues the composition-over-inheritance design.
>
> The big question left is do we keep ShiroFilter the same as it is and
> backwards-compatible by having it use INI configuration by default?
>
> Or do we make it an abstract class and then have, say, an
> IniShiroFilter that knows how to work with INI and BeanUtils APIs for
> object graph construction?  The abstract class would provide the
> Subject creation and thread binding duties as normal and leave only
> SecurityManager/FilterChainResolver building or acquisition to
> subclasses.
>
> - Les
>