using <authorize> tag with @Secured annotation - help with submitting a patch

[Richard Bradley]

Short version:

I had a few problems with the <intercept-url> style of securing my web-app, so I switched to using @Secured on my controllers, but found that the <authorize> JSP tag no longer worked. So I fixed it. I'd like feedback on my approach, and some advice on how to get this fix polished up so I can submit it for inclusion in the codebase.

Long version:

I have numbered specific questions inline.

My app uses Spring 3 and Spring Security. I have role based permission, and REST / CRUD style controllers.
For example, I might have a FooController, with list(), show(), editForm(), createForm(), handleFormSubmission() actions.
These actions would map to "/foo" for the list, "/foo/{id}" for the show, "/foo/{id}?form" for the edit form, etc.
(This is how Spring Roo sets up a project, but I'm not currently using Roo).

Now, I want all the FooController actions to be restricted only to users with ROLE_FOO_MANAGER.
What should I do?

As far as I can tell, I need to add to my applicationContext-security.xml a line like:

<intercept-url pattern="/foo/**" access="hasRole('ROLE_FOO_MANAGER')" />

1. Is that right?

Now I found that this blocks access to /foo and /foo/3, but it didn't block access to /foo.html ! This is a pretty gaping security hole.

Did I need instead to add:

  <intercept-url pattern="/foo/**" access="hasRole('ROLE_FOO_MANAGER')" />
  <intercept-url pattern="/foo*" access="hasRole('ROLE_FOO_MANAGER')" />

?

Will there be any more holes I am not aware of?

I'm not that happy with the security config being specified by URL pattern, when the business security requirement is specified by controller (or action). Clearly the @Secured annotation on the controller is a better fit for what I'm trying to do.

So I switched to that. I had to turn on

<global-method-security secured-annotations="enabled" />

in both my applicationContext-security.xml and my applicationContext.xml files. I then converted my leaky URL patterns into @Secured annotations on the controllers and the actions, and I was much happier.

However, this change means that the <authorize> JSP tags no longer worked. Looking through the code, it seems their implementation is tied to the <intercept-url> tags.

2. Is there an already-implemented way to get <authorize> tags to work with @Secured annotations?

I was able to enhance the <authorize> tags to work with @Secured annotations on controllers by doing the following:

• Writing a custom WebInvocationPrivilegeEvaluator (code below)
• Adding my Privilege Evaluator as a bean to the applicationContext-security.xml before the <http> tag, causing the <authorize> tags to use my Evaluator instead of the Default evaluator.
• Writing a custom Spring DispatcherServlet which registers itself as a singleton in the application context, so my WebInvocationPrivilegeEvaluator can get a reference to it (code below)
• Making the (previously protected) getHandler() method public in my custom DispatcherServlet
• Tweaking the Spring Security DummyRequest class a bit (code below)

I have the following questions:

3. Is this sensible? Have I made any mistakes?
4. Would this enhancement be useful to others? Shall I try and get it committed to Spring Security?
5. In order to add this to Spring Security, I will need to either change the DispatcherServlet in Spring at the same time, or require that Security users of this feature use a custom DispatcherServlet. What's the best way around this?
6. It seems to me that the @Secured annotation is much better than the <intercept-url> patterns, due to the issues discussed above. Once I have this committed, shall I try and update the Spring Security documentation to recommend that instead of the patterns?

I need to resolve (5) before I can submit a patch. Any suggestions?

Thanks,



Rich


Here's the new WebInvocationPrivilegeEvaluator:

/**
 * A WebInvocationPrivilegeEvaluator which defers to the annotation-driven
 * MethodSecurityInterceptor instead of to the pattern-driven FilterInvocation
 * security interceptors.
 */
public class AnnotationAwareWebInvocationPrivilegeEvaluator implements
        WebInvocationPrivilegeEvaluator {

    @Autowired
    private MethodSecurityInterceptor security;
    @Autowired
    private ApplicationContext applicationContext;
    // Note: not autowired
    private VisibleDispatcherServlet dispatcherServlet;

    private VisibleDispatcherServlet getDispatcherServlet() {
        // When this bean is first constructed, the DispatcherServlet is not yet
        // registered in the application context. By the time we get called to evaluate
        // a privilege, it will be.
        if (dispatcherServlet == null) {
            dispatcherServlet = applicationContext.getBean(VisibleDispatcherServlet.class);
        }
        return dispatcherServlet;
    }

    @Override
    public boolean isAllowed(String uri, Authentication authentication) {
        return isAllowed(null, uri, null, authentication);
    }

    @Override
    public boolean isAllowed(
            String contextPath,
            String uri,
            String method,
            Authentication authentication) {

        if (authentication == null) {
            return false;
        }

        // Create a request which is sufficiently realistic to fool the
        // handler matchers in the Dispatcher
        DummyRequest2 request = new DummyRequest2();
        request.setContextPath(contextPath);
        request.setRequestURI(contextPath + uri);
        request.setMethod(StringUtils.defaultIfEmpty(method, "GET"));

        HandlerExecutionChain handler = getDispatcherServlet().getHandler(request);

        if (handler != null) {
            final HandlerMethod hMethod = (HandlerMethod) handler.getHandler();

            MethodInvocation mi = new MethodInvocation() {
                @Override
                public Object proceed() throws Throwable {
                    return null; // do nothing
                }
                @Override
                public Object getThis() {
                    return hMethod.getBean();
                }
                @Override
                public AccessibleObject getStaticPart() {
                    return null;
                }
                @Override
                public Object[] getArguments() {
                    return null;
                }
                @Override
                public Method getMethod() {
                    return hMethod.getMethod();
                }
            };

            try {
                security.invoke(mi);
            } catch (AccessDeniedException e) {
                return false;
            } catch (RuntimeException e) {
                throw e;
            } catch (Throwable t) {
                throw new RuntimeException(t);
            }

            return true;
        }

        return true;
    }
}

Here's the custom DispatcherServlet:

/**
 * A DispatcherServlet that registers itself in the application context,
 * so you can find it from beans which require a reference to the Dispatcher.
 *
 * Note that autowiring often won't work, as we can only register this
 * bean in the application context after most other beans have been autowired.
 *
 * However, you will be able to do appCtx.getBean(DispatcherServlet.class)
 */
public class VisibleDispatcherServlet extends DispatcherServlet {

    @Override
    protected void initStrategies(ApplicationContext context) {
        super.initStrategies(context);

        XmlWebApplicationContext parent = (XmlWebApplicationContext) context.getParent();
        ConfigurableListableBeanFactory bf = parent.getBeanFactory();
        bf.registerSingleton("dispatcherServlet", this);
    }

    @Override
    public HandlerExecutionChain getHandler(HttpServletRequest request) {
        try {
            return super.getHandler(request);
        } catch (RuntimeException e) {
            throw e;
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }
}

Here's the modified DummyRequest class (note that it needs to be in the Spring Security package, as the base class is package-private):

package org.springframework.security.web;

public class DummyRequest2 extends DummyRequest {

    @Override
    public Object getAttribute(String name) {
        return null;
    }

    @Override
    public void setAttribute(String name, Object o) {
        // ignore
    }

    @Override
    public String getCharacterEncoding() {
        return "utf8";
    }

    @Override
    public String getServletPath() {
        return "";
    }

    @Override
    public String getParameter(String name) {
        return null;
    }
}

[Rob Winch]

I have responded inline

As far as I can tell, I need to add to my applicationContext-security.xml a line like:

<intercept-url pattern="/foo/**" access="hasRole('ROLE_FOO_MANAGER')" />

1. Is that right?

It depends does that cover all your use cases? The comment below implies that it does not. You could use /foo*/** but of course that would also match /foo2/something. An easy way to test if the patterns work is to use write a JUnit test against AntUrlPathMatcher (in Spring Security 3.0.x) or AntPathRequestMatcher (in Spring Security 3.1.x). You can also @Autowire the springSecurityFilterChain into a JUnit and run the full request to validate that it is secured. Take a look at some of the Spring Security junits for examples of this.

If you like there is also a corresponding RegexUrlPathMatcher and RegexRequestMatcher that can be activated using the http@request-matcher attribute. This is more powerful, but keep in mind that by default it includes query params in the path so getting the pattern correct can be quite a bit more complicated.

You could also change Spring MVC to no longer match with the file extension. In 3.1 you can set useSuffixPatternMatch to false on RequestMappingHandlerMapping.

Will there be any more holes I am not aware of?

Since we cannot know all your URLs nor can we validate everyones configuraiton you should write tests to verify that all your URLs are secured.

However, this change means that the <authorize> JSP tags no longer worked. Looking through the code, it seems their implementation is tied to the <intercept-url> tags.

It is actually tied to the FilterInvocationSecurityMetadataSource that is used. The intercept-url elements simply create an instance of FilterInvocationSecurityMetadataSource (in your case it is using ExpressionBasedFilterInvocationSecurityMetadataSou rce). So I would create replace the FilterInvocationSecurityMetadataSource with a factory that creates an instance of ExpressionBasedFilterInvocationSecurityMetadataSou rce with the appropriate mappings.

To get an idea of how to do this you can see the FAQ which demonstrates how to create the mapping based upon a database. However, if you could look up the information in any way you like (i.e. Spring MVC configuration).

3. Is this sensible? Have I made any mistakes?

I would use a custom FilterInvocationSecurityMetadataSource as described above.

4. Would this enhancement be useful to others? Shall I try and get it committed to Spring Security?

Spring Security has no knowledge about the Spring MVC URL mappings because this would require knowledge of Spring MVC and Spring Security is intended to work independantly of Spring MVC. I doubt this would be included in Spring Securiyt, but you might post your solution on this thread if others wish to use it.

5. In order to add this to Spring Security, I will need to either change the DispatcherServlet in Spring at the same time, or require that Security users of this feature use a custom DispatcherServlet. What's the best way around this?

This may be a better question for Spring MVC forums. One option would be to replicate the logic for obtaining the DispatcherServlet's handlerMappings and then reproduce the logic in the getHandler method. This is probably even worse than your current solution.

It sounds as though you are using Spring 3, but if you updated to you can easily obtain a mapping of the classes by @Autowire RequestMappingHandlerMapping. You can see Rossen's Spring MVC 3.1 Update presentation or the GitHub project for an example. This still causes complications if multiple entries match knowing which one is best. Additionally if you have multiple handlerMappings it causes other problems.

You may be best may be creating the FilterInvocationSecurityMetadataSource as I mentioned, but producing it using your own syntax so it is more concise than the ant expressions. If you do this, then you would no longer need the annotations on your controllers since the URL's would already be protected. In that event the authorize tags would work also.

It seems to me that the @Secured annotation is much better than the <intercept-url> patterns, due to the issues discussed above. Once I have this committed, shall I try and update the Spring Security documentation to recommend that instead of the patterns?

In general it is considered best practice to secure your application in layers. It is good to secure the first entry point (i.e. by URL) and the Service layer using method layer security. If you wait until it gets to the controller the attack surface grows and the hacker may find a way to bypass things. This is why you should protect in layers.

[Richard Bradley]

Thanks very much, that was very helpful.

As far as I can tell, I need to add to my applicationContext-security.xml a line like: <intercept-url pattern="/foo/**"... Is that right?

It depends does that cover all your use cases? The comment below implies that it does not. You could use /foo*/** but of course that would also match /foo2/something.

Yes, it did not cover all my use cases (hence this discussion ).
My problem is that I'm trying to secure different actions, but Spring Security wants me to specify URLs. This mismatch of language seems to me to be likely to lead to security holes like those discussed above.

You can also @Autowire the springSecurityFilterChain into a JUnit and run the full request to validate that it is secured. Take a look at some of the Spring Security junits for examples of this.

This would rely on me thinking of all the security holes in advance. Our team had been writing our app for several weeks before we noticed that /foo.html would map to the index action on FooController, but wouldn't get caught by the Spring Security pattern /foo/** -- what other holes are lurking out there? We can't hope to catch them with a unit test, as they are unknown!

I would use a custom FilterInvocationSecurityMetadataSource as described above [instead of a custom WebInvocationPrivilegeEvaluator].

That doesn't seem quite right.

I don't want to use the spring filter, as it secures URLs, when I want to secure methods. So I shouldn't be filtering on FilterInvocations but on MethodInvocations. So it seems better to me to use a custom WebInvocationPrivilegeEvaluator.

Also, I can't quite see how to get a custom FilterInvocationSecurityMetadataSource to work in this case?

The FilterInvocationSecurityMetadataSource API maps from FilterInvocation (which contain a URL) to a List<ConfigAttribute>. How would I fetch the applicable List<ConfigAttribute> for a RequestHandler?

Spring Security has no knowledge about the Spring MVC URL mappings because this would require knowledge of Spring MVC and Spring Security is intended to work independantly of Spring MVC. I doubt this would be included in Spring Securiyt, but you might post your solution on this thread if others wish to use it.

I see. I didn't realise that -- I thought Spring Security was part of Spring / Spring MVC. Fair enough. Hopefully anyone with a similar problem will find this thread. I have posted all my code so far above.

It sounds as though you are using Spring 3, but if you updated [to 3.1] you can easily obtain a mapping of the classes by @Autowire RequestMappingHandlerMapping.

Great, that's ideal. I can autowire a RequestMappingHandlerMapping and do away with the custom DispatcherServlet altogether! Thanks.

(I'm having a little trouble with this at the moment -- the RMHM is only available in the web application context, but the spring-security stuff is currently in the parent application context. Do you know if I can move all the spring security declarations to my web context?)

You may be best may be creating the FilterInvocationSecurityMetadataSource as I mentioned, but producing it using your own syntax so it is more concise than the ant expressions. If you do this, then you would no longer need the annotations on your controllers since the URL's would already be protected. In that event the authorize tags would work also.

I don't like that solution -- the benefit of annotations on the controllers seems significant:

• There's no chance of an obscure mapping route bypassing my security
• All the config is in one place -- if I rename an action, I don't have to remember to also update an xml file hiding away somewhere (the same argument for all annotation based config, which definitely seems to be the way things are moving with Spring)
• (similar to the above) I can see what permissions are needed for an action on the action itself, without having to cross-reference an XML file and try and translate from @RequestMapping annotations into URLs and then into URL patterns.

In general it is considered best practice to secure your application in layers. It is good to secure the first entry point (i.e. by URL) and the Service layer using method layer security. If you wait until it gets to the controller the attack surface grows and the hacker may find a way to bypass things. This is why you should protect in layers.

That's interesting. There will be a larger attack surface once the request gets all the way to the method invocation interceptor, yes (all the spring injected method arguments, for example).

It's a tradeoff as always. For the reasons above and since I've just been bitten by URL mappings, I'm currently very much in favour of annotation based security.