You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shiro.apache.org by Peter Ledbrook <pe...@cacoethes.co.uk> on 2010/02/22 21:45:18 UTC
Re: Issue with Grails / Shiro
>> It appears to be a Tomcat-specific issue :(
>
> This seems to be a problem with Shiro's OncePerRequestFilter and
> Tomcat. The trouble is, Tomcat completes the execution of the filters
> on the main request before then (apparently) forwarding to the error
> handler. So before the error handler is invoked, the Shiro filter
> clears the thread local variables, including the bound security
> manager. The security manager is never bound again because the Shiro
> filter extends OncePerRequestFilter which works out that this is still
> the same request (it's a forward, you see).
>
> Is this incorrect behaviour in Tomcat? I have no idea. The servlet
> specification does leave some holes, which means that it's not clear
> what the correct behaviour should be. Note that Tomcat only appears to
> perform a forward after completing the current request when it's
> forwarding to an error handler.
It seems that the servlet specification makes no guarantees in this
area. Tomcat's and Jetty's approaches are both valid. One thing that
is guaranteed by the spec is that the servlet container will pass the
original *unwrapped* request and response to the error handler, unless
there are any filters specifically attached to the ERROR dispatcher.
That means Shiro needs fixing.
I propose that we modify OncePerRequestFilter such that the *.FILTERED
request attribute is removed straight after the call to
doFilterInternal(). Users and the Grails plugin can then configure the
Shiro filter to trigger on the ERROR dispatcher if they want.
Admittedly the filter would no longer be once-per-request, but the
behaviour would be consistent between Tomcat and Jetty.
Cheers,
Peter
Re: Issue with Grails / Shiro
Posted by Les Hazlewood <lh...@apache.org>.
Sounds good - I just saw the commit. Thanks!
On Tue, Feb 23, 2010 at 1:28 AM, Peter Ledbrook <pe...@cacoethes.co.uk> wrote:
>> This is interesting. The OncePerRequestFilter was just copied from
>> Spring - did they not have the same issue then?
>
> Here's the latest code I just grabbed from src.springsource.org:
>
>
> public final void doFilter(ServletRequest request, ServletResponse
> response, FilterChain filterChain)
> throws ServletException, IOException {
>
> if (!(request instanceof HttpServletRequest) || !(response
> instanceof HttpServletResponse)) {
> throw new ServletException("OncePerRequestFilter just supports HTTP
> requests");
> }
> HttpServletRequest httpRequest = (HttpServletRequest) request;
> HttpServletResponse httpResponse = (HttpServletResponse) response;
>
> String alreadyFilteredAttributeName = getAlreadyFilteredAttributeName();
> if (request.getAttribute(alreadyFilteredAttributeName) != null ||
> shouldNotFilter(httpRequest)) {
> // Proceed without invoking this filter...
> filterChain.doFilter(request, response);
> }
> else {
> // Do invoke this filter...
> request.setAttribute(alreadyFilteredAttributeName, Boolean.TRUE);
> try {
> doFilterInternal(httpRequest, httpResponse, filterChain);
> }
> finally {
> // Remove the "already filtered" request attribute for this request.
> request.removeAttribute(alreadyFilteredAttributeName);
> }
> }
> }
>
> Note that they now remove the request attribute. I'll add that.
>
> Cheers,
>
> Peter
>
Re: Issue with Grails / Shiro
Posted by Peter Ledbrook <pe...@cacoethes.co.uk>.
> This is interesting. The OncePerRequestFilter was just copied from
> Spring - did they not have the same issue then?
Here's the latest code I just grabbed from src.springsource.org:
public final void doFilter(ServletRequest request, ServletResponse
response, FilterChain filterChain)
throws ServletException, IOException {
if (!(request instanceof HttpServletRequest) || !(response
instanceof HttpServletResponse)) {
throw new ServletException("OncePerRequestFilter just supports HTTP
requests");
}
HttpServletRequest httpRequest = (HttpServletRequest) request;
HttpServletResponse httpResponse = (HttpServletResponse) response;
String alreadyFilteredAttributeName = getAlreadyFilteredAttributeName();
if (request.getAttribute(alreadyFilteredAttributeName) != null ||
shouldNotFilter(httpRequest)) {
// Proceed without invoking this filter...
filterChain.doFilter(request, response);
}
else {
// Do invoke this filter...
request.setAttribute(alreadyFilteredAttributeName, Boolean.TRUE);
try {
doFilterInternal(httpRequest, httpResponse, filterChain);
}
finally {
// Remove the "already filtered" request attribute for this request.
request.removeAttribute(alreadyFilteredAttributeName);
}
}
}
Note that they now remove the request attribute. I'll add that.
Cheers,
Peter
Re: Issue with Grails / Shiro
Posted by Les Hazlewood <lh...@apache.org>.
This is interesting. The OncePerRequestFilter was just copied from
Spring - did they not have the same issue then?
I'm certainly in favor of consistent behavior!
+1 to Peter's proposed change.
On Mon, Feb 22, 2010 at 3:45 PM, Peter Ledbrook <pe...@cacoethes.co.uk> wrote:
>>> It appears to be a Tomcat-specific issue :(
>>
>> This seems to be a problem with Shiro's OncePerRequestFilter and
>> Tomcat. The trouble is, Tomcat completes the execution of the filters
>> on the main request before then (apparently) forwarding to the error
>> handler. So before the error handler is invoked, the Shiro filter
>> clears the thread local variables, including the bound security
>> manager. The security manager is never bound again because the Shiro
>> filter extends OncePerRequestFilter which works out that this is still
>> the same request (it's a forward, you see).
>>
>> Is this incorrect behaviour in Tomcat? I have no idea. The servlet
>> specification does leave some holes, which means that it's not clear
>> what the correct behaviour should be. Note that Tomcat only appears to
>> perform a forward after completing the current request when it's
>> forwarding to an error handler.
>
> It seems that the servlet specification makes no guarantees in this
> area. Tomcat's and Jetty's approaches are both valid. One thing that
> is guaranteed by the spec is that the servlet container will pass the
> original *unwrapped* request and response to the error handler, unless
> there are any filters specifically attached to the ERROR dispatcher.
> That means Shiro needs fixing.
>
> I propose that we modify OncePerRequestFilter such that the *.FILTERED
> request attribute is removed straight after the call to
> doFilterInternal(). Users and the Grails plugin can then configure the
> Shiro filter to trigger on the ERROR dispatcher if they want.
> Admittedly the filter would no longer be once-per-request, but the
> behaviour would be consistent between Tomcat and Jetty.
>
> Cheers,
>
> Peter
>