Add support for target page.

[chamathns]

Describe the issue:

Currently, upon successful authentication, the SSO agent redirects the user to the page registered in the ACS URL (eg: app/home).

However, if a user tries to access another secured page (eg: app/myAccount) without having an authenticated session with the IdP, the user is first prompted for authentication. Then upon successful authentication, the user is redirected to the app/home where the user should have been redirected to the original page he tried to access; app/myAccount.

Expected behaviour

The Agent should keep track of the original page the user tried to access (target page), and redirect the user to the target page upon successful authentication.

[codebling]

I think this could be done by appending the original URL (e.g. app/myAccount) as a query parameter to the callback URI (e.g. app/oauth2client) in DefaultOIDCManager.java:96, then the query parameter could be used for the redirect in OIDCAgentFilter.java:115.

What do you think? I could potentially create a PR for this.

[chamathns]

Hi,

Thank you very much for your suggestion. I think it is a valid approach but I have a minor concern with sending out the target page URI (app/myAccount) as a query parameter. What if the URI in the query parameter is tampered with when it is returned in the callback URI? Couldn't it pose a security vulnerability?

Thinking about this, another way could keep track of the app/myAccount URI would be to store it against the state param in the session storage when sending out the login request at HTTPSessionBasedOIDCProcessor.java:65. Then we can retrieve the app/myAccount URI when handling the callback URI at OIDCAgentFilter.java:108.

What do you think about the two approaches?

cc: @darshanasbg

[codebling]

@chamathns I'm trying to think of whether that scenario is plausible - tampering with the query parameter would require the ability to break TLS and to install a MiTM between the client and the IdP (at least in every scenario I can imagine). If that was achieved, in the worst case it would provide a redirect to a relative path (on the Relying Party site) of the attacker's choice. While not a vulnerability in itself, it would provide a greater attack surface, though if the attacker can break TLS and install a MiTM near the client, there is probably a very large attack surface already (e.g. they could rewrite any request with a redirect to any site). Let me know if you can think of another scenario or if there's a problem with my reasoning - I am by no means an expert here.

That being said, there's nothing really wrong with the session approach, either. I think there are some minor edge cases when using multiple tabs, but probably nothing that anyone would complain about.

[darshanasbg]

Hi,

Thank you very much for your suggestion. I think it is a valid approach but I have a minor concern with sending out the target page URI (app/myAccount) as a query parameter. What if the URI in the query parameter is tampered with when it is returned in the callback URI? Couldn't it pose a security vulnerability?

Thinking about this, another way could keep track of the app/myAccount URI would be to store it against the state param in the session storage when sending out the login request at HTTPSessionBasedOIDCProcessor.java:65. Then we can retrieve the app/myAccount URI when handling the callback URI at OIDCAgentFilter.java:108.

What do you think about the two approaches?

cc: @darshanasbg

I prefer to use the state parameter to cater this rather sending through the callback url itself. If we send this on the callback url, now the callback URL will be dynamic and it will be depend on IDPs callback URL validation logic and capabilities which will negatively effects the interoperability of the SDK.

[chamathns]

Adding to this, in the spec, 3.1.2.1. Authentication Request says,

This URI MUST exactly match one of the Redirection URI values for the Client pre-registered at the OpenID Provider, with the matching performed as described in Section 6.2.1 of [RFC3986] (Simple String Comparison)

An exact string matching at the IDP would mean that the callback URIs should be static. And, the current draft recommendation for OAuth 2.0 Best Security Practice confirms that a dynamic redirect_uri must not be used by enforcing the exact matching of pre-registered URIs with the request URI. And it advises against forwarding to URIs obtained from query strings.

Authorization servers MUST utilize exact matching of client redirect URIs against pre-registered URIs. Clients SHOULD avoid forwarding the user's browser to a URI obtained from a query parameter.

Hence, I believe storing the target page to the session with the help of state param would be the more favorable approach.

What do you think?

ref: https://stackoverflow.com/questions/55524480/should-dynamic-query-parameters-be-present-in-the-redirection-uri-for-an-oauth2

[codebling]

Of course! In my head, I was thinking that the query parameter is not part of the matching, but of course it is. Thanks for the SO link, I'll read up on this a bit more.

So we're looking at adding the redirection URI to the request parameters? Something like this? (relevant code)

    public void sendForLogin(HttpServletRequest request, HttpServletResponse response)
            throws SSOAgentException {

        HttpSession session = request.getSession();
        RequestContext requestContext = defaultOIDCManager.sendForLogin(request, response);
        requestContext.setParameter(SSOAgentContants.REDIRECT_URI_KEY, redirectURI); //<-- this line added
        session.setAttribute(SSOAgentConstants.REQUEST_CONTEXT, requestContext);

[chamathns]

@codebling Yes. That's the idea. Then, when handling the callback response in the agent filter, we can retrieve the redirectURI from requestContext obj that's in the session storage and redirect to that.