Added support different servlet root paths for the Authn RequestFilter

[danielgpm]

The current AuthorizationRequestFilter assumes the APIs are always at the application root context: ("/authorize")

...
@Override
    public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException {

        HttpServletRequest request = (HttpServletRequest) req;
        HttpServletResponse response = (HttpServletResponse) res;
        HttpSession session = request.getSession();

        // skip everything that's not an authorize URL
        if (!request.getServletPath().startsWith("/authorize")) {
            chain.doFilter(req, res);
            return;
        }
...

In our use case, since we have to support other authentication protocols (such as SAML); the openid connect APIs are under the sub servlet context "/oidc" therefore the authorize url will be "/oidc/authorize" for our application. Because of this change, the current version of the filter never captures authentication request, so any authentication request containing prompt are max_age do not work.
This simple pull request changes that validation and includes the unit tests for such cases.

Notes:

• The extra test dependency: javax.servlet:javax.servlet-api:3.0.1 was added to be able to use the class MockHttpServletRequest in the unit test.

Thanks

[praseodym]

One problem I spot is that the filter will now also trigger on an URL such as "/somethingelse/authorize".

[danielgpm]

@praseodym but that's the purpose of the change: make this filter to capture authentication requests when the openid connect APIs are located on a sub context such as: "/oidc/authorize".

If the intention was to guarantee that "/authorize" is always at the start of the request even when there's is sub context present; I could modify the validation code to something like:

private static final String AUTHORIZE_URL = "/authorize";
...

/**
 * Checks is given request is a authentication request url
 *
 * @param request
 * @return
 */
private boolean isAuthorizeUrl(HttpServletRequest request) {
   // validates path is an authorize URL 
    if (AUTHORIZE_URL.equals(request.getPathInfo())) {
        return true;
    }
    // original validation
    if (request.getServletPath().startsWith(AUTHORIZE_URL)) {
        return true;
    }
    return false;
}

Now when the pathInfo is not null, it has to be exactly "/authorize", this will be fully compatible with the original implementation.
However, be aware that the original implementation triggers for URLs like: "/authorize/something/else"

[praseodym]

I see the reason behind the change, but it will break existing deployments. I think it would be best to do exact matching instead (i.e. getPathInfo).

[danielgpm]

I see. I updated the code accordingly.
Both validations are needed then; because getPathInfo will return null for the default case when the API is at the root context.

[jricher]

Just making sure I follow: the purpose of this change is to let you alter the paths of the server without changing the overall issuer? In most deployments so far, to have something live at /oidc/authorize, you have the issuer include the /oidc/ path component, and everything cascades fine, including all the other endpoints. Will that approach not work here?

[danielgpm]

@jricher no, we are also chaning the issuer; as you said we also though that by changing the issuer to something like: "http://app.domain/appname/oidc/", everything will work transparently however that is not the case for the requests with optional parameters options covered by this filter, namely: prompt and max_age.
All other cases of "/authrorize" requests are correctly handled by the spring's security/oauth2 filter that is also part of the chain.

[jricher]

Interesting, because that really should be the case here. The development setup uses a path component as part of the issuer and this filter definitely fires in those cases, so I'll look into it to see what's going on there.

[jricher]

I can't replicate this behavior. In my test environment, we deploy to openid-connect-server-webapp which sets the request.getContextPath() value to /openid-connect-server-webapp and on requests to the authorization endpoint at /openid-connect-server-webapp/authorize the value of request.getServletPath() is /authorize as this filter expects and handles properly.

When deployed as ROOT this sets request.getContextPath() to (empty string) and requests to /authorize have the value of request.getServletPath() set to /authorize, again as you'd expect.

What am I missing?

[danielgpm]

Rather than changing the root context name, we are registering the dispatcher for the oidc APIs under the subcontext /oidc. By doing that the request.getContextPath() is not changing but the request.getServletPath() will be '/oidc' (instead of /) and the pathInfo becomes /authorize

As a simple test you could modify the openid-connect-server-webapp/src/main/webapp/WEB-INF/web.xml by adding an additional pattern /oidc/*:

<servlet-mapping>
        <servlet-name>spring</servlet-name>
        <url-pattern>/</url-pattern>
        <url-pattern>/oidc/*</url-pattern>
    </servlet-mapping>

and then try any authn request using the new pattern, e.g:
/openid-connect-server-webapp/oidc/authorize?response_type=code&scope=openid%20profile&nonce=test&state=foo&prompt=login&client_id=client&redirect_uri=http://localhost/
(notice request parameter: prompt=login)

You will see that the call will be handled normally, however the AuthorizationRequestFilter will skip it since the servlet path no longer starts with "/authorize"
The end result is that the prompt parameter is ignored.

[jricher]

Interesting, that's definitely an off-label use and I can see why the filter would fail. The real question is why the @RequestMapping works in that use case but this doesn't, so I'll have to dig into this further. My gut tells me there's a more robust and comprehensive solution to this problem that won't have the side effects that @praseodym mentions above. Thanks for the additional info!

[danielgpm]

My guess is that the spring library uses pattern matching instead of exact matching to cover these subtleties.
My updated pull request already addressed @praseodym concerns. But to your point, if you want a 100% backward compatible change, then I would suggest just to add a simple Request Matcher like:

WEB-INF/application-context.xml

    ...
    <bean id="clientAuthMatcher" class="org.mitre.openid.connect.filter.MultiUrlRequestMatcher">
        <constructor-arg name="filterProcessesUrls">
            <set>
                <value>/introspect</value>
                <value>/revoke</value>
                <value>/token</value>
            </set>
        </constructor-arg>
    </bean>

    <bean id="authRequestMatcher" class="org.springframework.security.web.util.matcher.AntPathRequestMatcher">
        <constructor-arg value="/authorize"/>
    </bean>

AuthorizationRequestFilter:

    @Autowired(required = false)
    private LoginHintExtracter loginHintExtracter = new RemoveLoginHintsWithHTTP();

    @Autowired
    private RequestMatcher authRequestMatcher;

    @Override
    public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException {

        HttpServletRequest request = (HttpServletRequest) req;
        HttpServletResponse response = (HttpServletResponse) res;
        HttpSession session = request.getSession();

        // skip everything that's not an authorize URL
        if (!authRequestMatcher.matches(request)) {
            chain.doFilter(req, res);
            return;
        }
   ...
    }

This shouldn't break any existing code and the deployer will have the flexibility to change the pattern if needed without having to patch the code.

Thank you for your time

[jricher]

I like that approach much better, and I'll put that in this morning.

Looking through the Spring methods for @RequestMapping, it's a whole lot of logic for working in lots of different environments, and I can't see an easy way to simply plug into that.