Override PathMatchingFilter

[Barry Books <tr...@gmail.com>]

I'd like to change Shiro to do case insensitive matches on urls. I've
figured out how to override the matching in authc by creating a filter
and overriding the pathsmatch method it appears I also need to
override whatever calls the filter in the first place. I think that's
the PathMatchingFilter but I can't figure out how to plug a different
one in.


I'm trying to use Shiro with Tapestry and the problem is Tapestry will
send /admin, /Admin, /admiN etc all to the same place. I'd like to
match them all with

[urls]
/admin/** = authc, roles[administrator]

but that config only handles /admin

Thanks
Barry

Re: Override PathMatchingFilter

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

On Sat, Feb 5, 2011 at 10:24 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> Thanks Barry. Been looking into it since I saw your report and there's
> a lot of stuff that is rotten there. First of all, initializing
> IniShiroFilter, creating the IniFilterChainResolverFactory, setting
> the FilterChainResolver and instantiating the default filters are all
> strongly coupled together so there's no easy way to override the
> default behavior. PathMatchingFilterChainResolver allows setting a
> different PatternMatcher but a PathMatchingFilter doesn't and worse
> yet, they are using different pattern matchers.

"Rotten"? "Worse yet"?  That's a bit harsh regarding something that
has been working well for the vast number of our end users IMHO.  This
is the first such issue, long since the code has existed, so I would
see that as an indicator of it's success.  Also the code in question
was available to you for peer review the entire time it was being
built, so why not raise the concern then instead of criticizing a year
later?  In my own personal opinion, I believe we're an open, helpful
and benevolent community, and criticism should be constructive without
being derisive.

Anyway, the IniShiroFilter was not initially intended to be easily
'overridable' - it was created as a means for web.xml users to set up
Shiro and not necessarily as a framework component intended to be
overridden by end-users or other frameworks (it's JavaDoc makes no
mention of overriding concerns on purpose).  Instead, the
AbstractShiroFilter (the IniShiroFilter's parent class) describes
itself as the extension point.

We can make IniShiroFilter more flexible to accomodate overrides if we
want, but I respectfully submit that this would be considered a
feature request in the spirit of catering to something beyond the
original design concerns and not an effect of 'rotten' code.

> Perhaps we could patch it together one way or another to allow case
> insensitive processing, but we might just as well fix it properly.

Agreed - please open a feature request.

> Considering that a filter would only need to carry around one
> attribute, it's configuration, it'd make it perform far better and
> greatly simplify processing the filter chains if we'd create separate
> instances of the same filter for each filter chain rather than trying
> to use the same filter in different chains. I'll open an issue
> accordingly.

I'm confused here - do you mean multiple instances of the
IniShiroFilter?  Or the filters that it references in its filter
chain?

If it is the latter, I think the appropriate approach for this is to
enable a Factory mechanism that allows users to specify a filter's
configuration and it is expected that the Factory will create new
instances as is necessary.  When not using a Factory, the default
singleton semantics should remain in place IMO (pretty common behavior
in most IoC environments).  In fact, there is an issue very closely
related to this:  https://issues.apache.org/jira/browse/SHIRO-217
that could provide the same consistent solution.

> I'll fix the tapestry-security library separately with an ugly but
> small and efficient hack. I've been toying with the idea of just
> rewriting the request processing part and the filters for simplicity
> and to make it align better with Tapestry, but I'll save that for
> later.

Of course if there are suggestions for making the request processing
smoother, better, etc, we should definitely look in to this if it
makes it better for everyone.  I'm very open to making as many
improvements as we can.  If need be, we can add this to the 2.0
feature list.

It also sounds like there are a few concerns here that are very dev
related, so I've CC'd the dev list so we don't bog down the user list
with implementation/design details.

Best,

Les

Re: Override PathMatchingFilter

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

On Sat, Feb 5, 2011 at 10:24 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> Thanks Barry. Been looking into it since I saw your report and there's
> a lot of stuff that is rotten there. First of all, initializing
> IniShiroFilter, creating the IniFilterChainResolverFactory, setting
> the FilterChainResolver and instantiating the default filters are all
> strongly coupled together so there's no easy way to override the
> default behavior. PathMatchingFilterChainResolver allows setting a
> different PatternMatcher but a PathMatchingFilter doesn't and worse
> yet, they are using different pattern matchers.

"Rotten"? "Worse yet"?  That's a bit harsh regarding something that
has been working well for the vast number of our end users IMHO.  This
is the first such issue, long since the code has existed, so I would
see that as an indicator of it's success.  Also the code in question
was available to you for peer review the entire time it was being
built, so why not raise the concern then instead of criticizing a year
later?  In my own personal opinion, I believe we're an open, helpful
and benevolent community, and criticism should be constructive without
being derisive.

Anyway, the IniShiroFilter was not initially intended to be easily
'overridable' - it was created as a means for web.xml users to set up
Shiro and not necessarily as a framework component intended to be
overridden by end-users or other frameworks (it's JavaDoc makes no
mention of overriding concerns on purpose).  Instead, the
AbstractShiroFilter (the IniShiroFilter's parent class) describes
itself as the extension point.

We can make IniShiroFilter more flexible to accomodate overrides if we
want, but I respectfully submit that this would be considered a
feature request in the spirit of catering to something beyond the
original design concerns and not an effect of 'rotten' code.

> Perhaps we could patch it together one way or another to allow case
> insensitive processing, but we might just as well fix it properly.

Agreed - please open a feature request.

> Considering that a filter would only need to carry around one
> attribute, it's configuration, it'd make it perform far better and
> greatly simplify processing the filter chains if we'd create separate
> instances of the same filter for each filter chain rather than trying
> to use the same filter in different chains. I'll open an issue
> accordingly.

I'm confused here - do you mean multiple instances of the
IniShiroFilter?  Or the filters that it references in its filter
chain?

If it is the latter, I think the appropriate approach for this is to
enable a Factory mechanism that allows users to specify a filter's
configuration and it is expected that the Factory will create new
instances as is necessary.  When not using a Factory, the default
singleton semantics should remain in place IMO (pretty common behavior
in most IoC environments).  In fact, there is an issue very closely
related to this:  https://issues.apache.org/jira/browse/SHIRO-217
that could provide the same consistent solution.

> I'll fix the tapestry-security library separately with an ugly but
> small and efficient hack. I've been toying with the idea of just
> rewriting the request processing part and the filters for simplicity
> and to make it align better with Tapestry, but I'll save that for
> later.

Of course if there are suggestions for making the request processing
smoother, better, etc, we should definitely look in to this if it
makes it better for everyone.  I'm very open to making as many
improvements as we can.  If need be, we can add this to the 2.0
feature list.

It also sounds like there are a few concerns here that are very dev
related, so I've CC'd the dev list so we don't bog down the user list
with implementation/design details.

Best,

Les

Re: Override PathMatchingFilter

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

Thanks Barry. Been looking into it since I saw your report and there's
a lot of stuff that is rotten there. First of all, initializing
IniShiroFilter, creating the IniFilterChainResolverFactory, setting
the FilterChainResolver and instantiating the default filters are all
strongly coupled together so there's no easy way to override the
default behavior. PathMatchingFilterChainResolver allows setting a
different PatternMatcher but a PathMatchingFilter doesn't and worse
yet, they are using different pattern matchers.

Perhaps we could patch it together one way or another to allow case
insensitive processing, but we might just as well fix it properly.
Considering that a filter would only need to carry around one
attribute, it's configuration, it'd make it perform far better and
greatly simplify processing the filter chains if we'd create separate
instances of the same filter for each filter chain rather than trying
to use the same filter in different chains. I'll open an issue
accordingly.

I'll fix the tapestry-security library separately with an ugly but
small and efficient hack. I've been toying with the idea of just
rewriting the request processing part and the filters for simplicity
and to make it align better with Tapestry, but I'll save that for
later.

Kalle


On Fri, Feb 4, 2011 at 1:16 PM, Barry Books <tr...@gmail.com> wrote:
> I'd like to change Shiro to do case insensitive matches on urls. I've
> figured out how to override the matching in authc by creating a filter
> and overriding the pathsmatch method it appears I also need to
> override whatever calls the filter in the first place. I think that's
> the PathMatchingFilter but I can't figure out how to plug a different
> one in.
>
>
> I'm trying to use Shiro with Tapestry and the problem is Tapestry will
> send /admin, /Admin, /admiN etc all to the same place. I'd like to
> match them all with
>
> [urls]
> /admin/** = authc, roles[administrator]
>
> but that config only handles /admin
>
> Thanks
> Barry
>