PathMatchingFilter and configuration

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

PathMatchingFilter and configuration

Les Hazlewood-2
On Fri, Feb 11, 2011 at 10:48 AM, Les Hazlewood <[hidden email]> wrote:

> On Thu, Feb 10, 2011 at 8:06 PM, Kalle Korhonen
> <[hidden email]> wrote:
>> On Thu, Feb 10, 2011 at 5:48 PM, Les Hazlewood <[hidden email]> wrote:
>>> As it currently stands (at least until 2.0), I don't believe this is possible.
>>> PathMatchingFilter does more with the paths beyond just seeing if it
>>> should execute.  It uses the configured paths to determine
>>> path-specific configuration as well as support the
>>> isFilterChainContinued method (which allows inspection of
>>> path-specific config).
>>> Being able to dynamically react to path-specific configuration during
>>> a request is a big feature of many of the Shiro filters, as well as
>>> many custom subclasses out of our control.
>>> Just out of curiosity, what is the push to refactor the hierarchy?
>>
>> Perhaps I didn't explain the issue properly, but re-read the issue
>> description again. Since the path specific configuration is the only
>> actual attribute of the filter, it should be part of each specific
>> filter instance. If we just created a separate filter instance for
>> each chain, it'd simplify the logic, make it perform better and divide
>> the responsibilities better.
>
> I'm still not sure this would be ideal (at least until we find a
> suitable alternative):
>
> As you know, Filters are currently maintained in a 'pool' from which
> they can be referenced multiple times for separate chains.  In every
> web app I've ever used with Shiro, I rely on that to know that I can
> configure any object in the pool and that configuration applies in all
> chains where I use it.  I then use path-specific config for
> finer-grained control.
>
> For example, I can turn on or off the SSL filter application wide, but
> then turn it on or off for an individual chain *without* removing it
> from the chain:
>
> /some/feature/** = ssl[enabled=${someFeature.ssl.enabled}], authc
>
> I leverage this a lot in my apps and it allows for an extremely
> powerful/flexible security control experience.
>
> Creating a new filter instance per chain would be very frustrating if
> I would have to configure the same filter type _every_ time it needs
> to go in a chain.  web.xml uses a single filter instance for multiple
> chains, and I think this is still probably the most intuitive for
> people IMO.
>
> As an alternative, I suppose you could use a factory approach (where
> you configure the Factory instance and it creates a new Filter each
> time you create a chain) but that is far more complex than configuring
> a 'global' filter directly.  I don't believe we should force the
> Factory approach on all Shiro users when the alternative is much
> simpler.  I think the Factory approach can co-exist, but they're not
> mutually exclusive IMO.
>
> Now if we can find a slick way of supporting this without being
> painful to configure or implement for end-users, I'm totally open to
> that.  Any ideas?  Any examples of how you think this could work (code
> or config or otherwise)?
>
> Best,
>
> Les

I'm just switching the thread to a new title - the Jira title was
distracting in the midst of all the other Jira notifications.

Les
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

kaosko
On Fri, Feb 11, 2011 at 11:11 AM, Les Hazlewood <[hidden email]> wrote:
> On Fri, Feb 11, 2011 at 10:48 AM, Les Hazlewood <[hidden email]> wrote:
>> As you know, Filters are currently maintained in a 'pool' from which
>> they can be referenced multiple times for separate chains.  In every

Yes, and I'm saying there's no benefit pooling them.

>> For example, I can turn on or off the SSL filter application wide, but
>> then turn it on or off for an individual chain *without* removing it
>> from the chain:
>> /some/feature/** = ssl[enabled=${someFeature.ssl.enabled}], authc
>> I leverage this a lot in my apps and it allows for an extremely
>> powerful/flexible security control experience.
>> Creating a new filter instance per chain would be very frustrating if
>> I would have to configure the same filter type _every_ time it needs
>> to go in a chain.  web.xml uses a single filter instance for multiple

Right, we have the default configuration per filter type and then
configuration per filter instance. Obviously the enabled bit is part
of the filter instance configuration.

Java's fast, but for a filter chain with five filters in it, we are
currently doing the same comparison five times unnecessarily. This
adds up very quick since it's done for every request. Just for fun, do
a few quick performance tests. In the meantime, I rescheduled the
issue for 2.0 and made it unassigned. For now, I'll instead implement
the same just for tapestry-security.

Kalle
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

Les Hazlewood-2
On Fri, Feb 11, 2011 at 9:58 PM, Kalle Korhonen
<[hidden email]> wrote:

> On Fri, Feb 11, 2011 at 11:11 AM, Les Hazlewood <[hidden email]> wrote:
>> On Fri, Feb 11, 2011 at 10:48 AM, Les Hazlewood <[hidden email]> wrote:
>>> As you know, Filters are currently maintained in a 'pool' from which
>>> they can be referenced multiple times for separate chains.  In every
>
> Yes, and I'm saying there's no benefit pooling them.
>
>>> For example, I can turn on or off the SSL filter application wide, but
>>> then turn it on or off for an individual chain *without* removing it
>>> from the chain:
>>> /some/feature/** = ssl[enabled=${someFeature.ssl.enabled}], authc
>>> I leverage this a lot in my apps and it allows for an extremely
>>> powerful/flexible security control experience.
>>> Creating a new filter instance per chain would be very frustrating if
>>> I would have to configure the same filter type _every_ time it needs
>>> to go in a chain.  web.xml uses a single filter instance for multiple
>
> Right, we have the default configuration per filter type and then
> configuration per filter instance. Obviously the enabled bit is part
> of the filter instance configuration.

The crux of this discussion for me lies in the following 2 questions:

How would a user configure filters such that each filter instance
created reflects that configuration.  Then, how would they provide
chain-specific configuration overrides on a particular filter
instance?

That is, how would either scenario look in shiro.ini?

I don't have any problems with new filter instances being created for
each filter chain.  I'd just like to see how this can be configured
simply without users having to repeat themselves.  Any ideas?

Best,

Les
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

kaosko
On Fri, Feb 11, 2011 at 10:34 PM, Les Hazlewood <[hidden email]> wrote:
> On Fri, Feb 11, 2011 at 9:58 PM, Kalle Korhonen
> The crux of this discussion for me lies in the following 2 questions:
> How would a user configure filters such that each filter instance
> created reflects that configuration.  Then, how would they provide
> chain-specific configuration overrides on a particular filter
> instance?

No, each chain would have its own filter instances.

Kalle
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

Les Hazlewood-2
In reply to this post by Les Hazlewood-2
On Feb 12, 2011 6:34 AM, "Kalle Korhonen" <[hidden email]>
wrote:
>
> On Fri, Feb 11, 2011 at 10:34 PM, Les Hazlewood <[hidden email]>
wrote:
> > On Fri, Feb 11, 2011 at 9:58 PM, Kalle Korhonen
> > The crux of this discussion for me lies in the following 2 questions:
> > How would a user configure filters such that each filter instance
> > created reflects that configuration.  Then, how would they provide
> > chain-specific configuration overrides on a particular filter
> > instance?
>
> No, each chain would have its own filter instances.
>

I understand the concept :)

Can you please show an example of how one would configure the instances in
shiro.ini?

Let's say I want to use the SSL filter to enforce SSL on port 8443.  I use
that filter in 6 different filter chains in the [ulrs] section.  What does
shiro.ini look like under your proposed mechanism?

Thanks,

Les
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

kaosko
On Sat, Feb 12, 2011 at 2:10 PM, Les Hazlewood <[hidden email]> wrote:

>> On Fri, Feb 11, 2011 at 10:34 PM, Les Hazlewood <[hidden email]>
> wrote:
>> > On Fri, Feb 11, 2011 at 9:58 PM, Kalle Korhonen
>> > The crux of this discussion for me lies in the following 2 questions:
>> > How would a user configure filters such that each filter instance
>> > created reflects that configuration.  Then, how would they provide
>> > chain-specific configuration overrides on a particular filter
>> > instance?
>> No, each chain would have its own filter instances.
> Can you please show an example of how one would configure the instances in
> shiro.ini?
> Let's say I want to use the SSL filter to enforce SSL on port 8443.  I use
> that filter in 6 different filter chains in the [ulrs] section.  What does
> shiro.ini look like under your proposed mechanism?

Not proposing any changes to the current ini format, this is just
about how and when to apply that configuration.

Kalle
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

Les Hazlewood-2
Actually, in thinking about this more, I don't think we should create
new Filter instances per chain by default, and here's why:

1.  Shiro filter chains can contain _any_ javax.servlet.Filter
instance - not just Shiro PathMatching ones.  Just like web.xml, we
create a single Filter instance used in many chains.  This ensures
that we remain consistent with the Servlet specification and we retain
the singleton nature that everyone expects.

2.  Creating a new Filter instance per chain can be a dangerous thing
to do - there are many filters that, upon startup/init, go through
sometimes significant initialization logic resulting in final state
used when filtering requests.

For example, consider the Spring DispatcherServlet - it creates an
ApplicationContext at filter startup.  While many instances of this
could be created (and it probably would't be too expensive), it is not
expected that this would ever occur, and it could even result in
problems for some applications.

This is just one example.  There are a ton of other framework filters
that assume the same 'init once' behavior.  Because we can't know
before hand what Filters would be used in Shiro's chain definitions,
we can't just assume to create a new instance for each chain.

I don't know what Tapestry Security supports, but if you're allowed to
configure any javax.servlet.Filter like you can in Shiro's INI, I
think you'd be opening yourself to many potential problems since
creating new instances per chain is definitely not standard Serlvet
container behavior.

Finally, and please correct me if I'm wrong, but your main concern in
this thread isn't really about new instances vs pooled instances (if
it was, I'm assuming you'd argue that the Servlet containers are doing
it incorrectly as well, since we just retain the same behavior).  It
is almost entirely driven by your concern about performance due to
executing path matching too often, correct?

Les
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

kaosko
On Sat, Feb 12, 2011 at 3:25 PM, Les Hazlewood <[hidden email]> wrote:
> Actually, in thinking about this more, I don't think we should create
> new Filter instances per chain by default, and here's why:
> 1.  Shiro filter chains can contain _any_ javax.servlet.Filter

Sure, which may the be exact problem. By the servlet spec, the filters
are configured in web.xml. We've talked about that before and Shiro
would also be a better fit for Play framework if these filters weren't
servlet filters.

> 2.  Creating a new Filter instance per chain can be a dangerous thing
> to do - there are many filters that, upon startup/init, go through

Sure it could, but that's certainly addressable if there were any such
filters with significant initialization logic. Even then, it's
one-time cost instead of recurring cost.

> I don't know what Tapestry Security supports, but if you're allowed to
> configure any javax.servlet.Filter like you can in Shiro's INI, I

Currently it does, but I just opened an issue for it and specifically
noted that they shouldn't be allowed anymore
(http://jira.codehaus.org/browse/TYNAMO-76). BTW, Tapestry is
implemented as a filter as well, so obviously you can use *standard*
servlet filters together with Tapestry as you see fit.

> Finally, and please correct me if I'm wrong, but your main concern in
> this thread isn't really about new instances vs pooled instances (if
> it was, I'm assuming you'd argue that the Servlet containers are doing
> it incorrectly as well, since we just retain the same behavior).  It
> is almost entirely driven by your concern about performance due to
> executing path matching too often, correct?

Right about the performance, that's what the PathMatchingFilter issue
all is about. Neither issue I opened says anything about the pool
either way. Absolutely wrong assumption on the servlet container
behavior: that comparison is off - there's no behavior such as filter
chains. By the spec, each filter includes the mapping configuration in
itself and they are all being compared to, similar to executing them
all as separate chains.

As is, please provide closing comments to
https://issues.apache.org/jira/browse/SHIRO-256 for future reference.

Kalle
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

kaosko
Just FYI, I implemented the simplified logic for Tapestry/Shiro
integration as described. I'm quite happy how it turned out and in the
process I was able to obsolete big chunks of path handling code (for
example, see http://svn.codehaus.org/tynamo/trunk/tapestry-security/src/test/java/org/tynamo/security/testapp/services/AppModule.java).
I think the same approach would be quite valuable to the Shiro
community at large as well, though I personally have perhaps less
incentive now to push the changes upstream. Since the path matching
and path specific configuration is specific to Shiro filters, the
concerns Les raised earlier don't quite apply - obviously you can
share filter instances between different filter chains, it's just that
chain specific configuration is much simpler (and will perform better)
if you don't. If we decide to keep the (Shiro) issue open, perhaps we
can bring the topic up again when planning for 2.0.

Kalle


On Sat, Feb 12, 2011 at 5:10 PM, Kalle Korhonen
<[hidden email]> wrote:

> On Sat, Feb 12, 2011 at 3:25 PM, Les Hazlewood <[hidden email]> wrote:
>> Actually, in thinking about this more, I don't think we should create
>> new Filter instances per chain by default, and here's why:
>> 1.  Shiro filter chains can contain _any_ javax.servlet.Filter
>
> Sure, which may the be exact problem. By the servlet spec, the filters
> are configured in web.xml. We've talked about that before and Shiro
> would also be a better fit for Play framework if these filters weren't
> servlet filters.
>
>> 2.  Creating a new Filter instance per chain can be a dangerous thing
>> to do - there are many filters that, upon startup/init, go through
>
> Sure it could, but that's certainly addressable if there were any such
> filters with significant initialization logic. Even then, it's
> one-time cost instead of recurring cost.
>
>> I don't know what Tapestry Security supports, but if you're allowed to
>> configure any javax.servlet.Filter like you can in Shiro's INI, I
>
> Currently it does, but I just opened an issue for it and specifically
> noted that they shouldn't be allowed anymore
> (http://jira.codehaus.org/browse/TYNAMO-76). BTW, Tapestry is
> implemented as a filter as well, so obviously you can use *standard*
> servlet filters together with Tapestry as you see fit.
>
>> Finally, and please correct me if I'm wrong, but your main concern in
>> this thread isn't really about new instances vs pooled instances (if
>> it was, I'm assuming you'd argue that the Servlet containers are doing
>> it incorrectly as well, since we just retain the same behavior).  It
>> is almost entirely driven by your concern about performance due to
>> executing path matching too often, correct?
>
> Right about the performance, that's what the PathMatchingFilter issue
> all is about. Neither issue I opened says anything about the pool
> either way. Absolutely wrong assumption on the servlet container
> behavior: that comparison is off - there's no behavior such as filter
> chains. By the spec, each filter includes the mapping configuration in
> itself and they are all being compared to, similar to executing them
> all as separate chains.
>
> As is, please provide closing comments to
> https://issues.apache.org/jira/browse/SHIRO-256 for future reference.
>
> Kalle
>
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

Les Hazlewood
Administrator
Sounds good to me - can you please add this to the 2.0 brainstorming page?

I've got some ideas around how to eliminate the path matching behavior
per filter that also makes filter logic more 'pluggable' as well
(without forcing subclassing).

Best,

Les

On Tue, May 31, 2011 at 4:57 PM, Kalle Korhonen
<[hidden email]> wrote:

> Just FYI, I implemented the simplified logic for Tapestry/Shiro
> integration as described. I'm quite happy how it turned out and in the
> process I was able to obsolete big chunks of path handling code (for
> example, see http://svn.codehaus.org/tynamo/trunk/tapestry-security/src/test/java/org/tynamo/security/testapp/services/AppModule.java).
> I think the same approach would be quite valuable to the Shiro
> community at large as well, though I personally have perhaps less
> incentive now to push the changes upstream. Since the path matching
> and path specific configuration is specific to Shiro filters, the
> concerns Les raised earlier don't quite apply - obviously you can
> share filter instances between different filter chains, it's just that
> chain specific configuration is much simpler (and will perform better)
> if you don't. If we decide to keep the (Shiro) issue open, perhaps we
> can bring the topic up again when planning for 2.0.
>
> Kalle
>
>
> On Sat, Feb 12, 2011 at 5:10 PM, Kalle Korhonen
> <[hidden email]> wrote:
>> On Sat, Feb 12, 2011 at 3:25 PM, Les Hazlewood <[hidden email]> wrote:
>>> Actually, in thinking about this more, I don't think we should create
>>> new Filter instances per chain by default, and here's why:
>>> 1.  Shiro filter chains can contain _any_ javax.servlet.Filter
>>
>> Sure, which may the be exact problem. By the servlet spec, the filters
>> are configured in web.xml. We've talked about that before and Shiro
>> would also be a better fit for Play framework if these filters weren't
>> servlet filters.
>>
>>> 2.  Creating a new Filter instance per chain can be a dangerous thing
>>> to do - there are many filters that, upon startup/init, go through
>>
>> Sure it could, but that's certainly addressable if there were any such
>> filters with significant initialization logic. Even then, it's
>> one-time cost instead of recurring cost.
>>
>>> I don't know what Tapestry Security supports, but if you're allowed to
>>> configure any javax.servlet.Filter like you can in Shiro's INI, I
>>
>> Currently it does, but I just opened an issue for it and specifically
>> noted that they shouldn't be allowed anymore
>> (http://jira.codehaus.org/browse/TYNAMO-76). BTW, Tapestry is
>> implemented as a filter as well, so obviously you can use *standard*
>> servlet filters together with Tapestry as you see fit.
>>
>>> Finally, and please correct me if I'm wrong, but your main concern in
>>> this thread isn't really about new instances vs pooled instances (if
>>> it was, I'm assuming you'd argue that the Servlet containers are doing
>>> it incorrectly as well, since we just retain the same behavior).  It
>>> is almost entirely driven by your concern about performance due to
>>> executing path matching too often, correct?
>>
>> Right about the performance, that's what the PathMatchingFilter issue
>> all is about. Neither issue I opened says anything about the pool
>> either way. Absolutely wrong assumption on the servlet container
>> behavior: that comparison is off - there's no behavior such as filter
>> chains. By the spec, each filter includes the mapping configuration in
>> itself and they are all being compared to, similar to executing them
>> all as separate chains.
>>
>> As is, please provide closing comments to
>> https://issues.apache.org/jira/browse/SHIRO-256 for future reference.
>>
>> Kalle
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: PathMatchingFilter and configuration

kaosko
On Tue, May 31, 2011 at 8:01 PM, Les Hazlewood <[hidden email]> wrote:
> Sounds good to me - can you please add this to the 2.0 brainstorming page?

Done.