On Sat, Feb 5, 2011 at 10:24 PM, Kalle Korhonen <[email protected]> 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
