Skip to content

Final redirect in OAuth2AuthorizationCodeGrantFilter needs to be configurable #7324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Final redirect in OAuth2AuthorizationCodeGrantFilter needs to be configurable #7324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions .../security/oauth2/client/web/DefaultOAuth2AuthorizationCodeGrantFinalRedirectStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.springframework.security.oauth2.client.web;

import java.io.IOException;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse;
import org.springframework.security.web.RedirectStrategy;
import org.springframework.security.web.savedrequest.RequestCache;
import org.springframework.security.web.savedrequest.SavedRequest;

/**
* Default implementation of {@link OAuth2AuthorizationCodeGrantFinalRedirectStrategy}.
*
* @author Tadaya Tsuyukubo
* @since 5.2
* @see OAuth2AuthorizationCodeGrantFilter
*/
public class DefaultOAuth2AuthorizationCodeGrantFinalRedirectStrategy implements OAuth2AuthorizationCodeGrantFinalRedirectStrategy {

private final RedirectStrategy redirectStrategy;
private final RequestCache requestCache;

public DefaultOAuth2AuthorizationCodeGrantFinalRedirectStrategy(RedirectStrategy redirectStrategy, RequestCache requestCache) {
this.redirectStrategy = redirectStrategy;
this.requestCache = requestCache;
}

@Override
public void sendRedirect(HttpServletRequest request, HttpServletResponse response,
OAuth2AuthorizationRequest authorizationRequest, OAuth2AuthorizationResponse authorizationResponse) throws IOException {

String redirectUrl = authorizationResponse.getRedirectUri();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in this implementation is the exact existing code found in OAuth2AuthorizationCodeGrantFilter, that is also removed in this PR. It's simply moved around and adds the new interface OAuth2AuthorizationCodeGrantFinalRedirectStrategy. It's not clear to me why this is needed.

Based on your main comment:

This makes it difficult to modify the final redirect uri based on state parameter, cookie, etc.
Thus, it would be nice that the OAuth2AuthorizationCodeGrantFilter takes a strategy that determines the final destination to redirect.

I don't understand why a user would want to change the final redirect based on the state parameter? Can you please provide a more concrete use case? There hasn't been any issue logged asking for this flexibility so I'm unsure at this point if this is even needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end goal is making the logic that determines final redirect to be customizable per user's need.

WRT state parameter, it can be used to store final redirect url.

from https://www.oauth.com/oauth2-servers/redirect-uris/redirect-uri-registration/

If a client wishes to include request-specific data in the redirect URL, it can instead use the “state” parameter to store data that will be included after the user is redirected. It can either encode the data in the state parameter itself, or use the state parameter as a session ID to store the state on the server.

Per-Request Customization
If a client wishes to include request-specific data in the redirect URL, it can instead use the “state” parameter to store data that will be included after the user is redirected. It can either encode the data in the state parameter itself, or use the state parameter as a session ID to store the state on the server.

from https://www.oauth.com/oauth2-servers/server-side-apps/authorization-code/

state (recommended)
... This may be used to indicate what action in the app to perform after authorization is complete, for example, indicating which of your app’s pages to redirect to after authorization. ...

For example, I do see in some of our app uses state parameter as random string + final redirect url.

Alternative to state parameter, such final redirect url might be carried by cookie, additional params in OAuth2AuthorizationRequest, or determined by server side logic.

For the concrete case I have is that I need to redirect to different landing page per oauth grant request instead of the fixed redirect_uri page that has registered to the service in authorization server.

The initial request(one that triggers oauth2 code grant flow) contains a query parameter to indicate the final landing page after oauth2 code grant. (e.g.: final_redirect_uri param)
This information can be kept in OAuth2AuthorizationRequest#additionalParameters, for example.

With current implementation, the final redirect url is fixed to where the auth server has redirected to. (Also, current handling has problem with query parameter)
Instead, I'd like to resolve where to redirect reading from OAuth2AuthorizationRequest#additionalParameters.

If the logic is injectable, it is possible to replace the default logic to the one that achievers per-request customization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ttddyy

WRT state parameter, it can be used to store final redirect url.

The state parameter is used internally to correlate the OAuth2AuthorizationRequest to the OAuth2AuthorizationResponse. It should not be used in an application. It is strictly used to protect against CSRF attacks:

state
RECOMMENDED. An opaque value used by the client to maintain
state between the request and callback. The authorization
server includes this value when redirecting the user-agent back
to the client. The parameter SHOULD be used for preventing
cross-site request forgery as described in Section 10.12.

Regarding...

For the concrete case I have is that I need to redirect to different landing page per oauth grant request instead of the fixed redirect_uri page that has registered to the service in authorization server.

Here is a much simpler solution that you can implement:

Given the following ClientRegistration configuration:

spring:
  security:
    oauth2:
      client:
        registration:
          okta:
            client-id: okta-client-id
            client-secret: okta-client-secret
            authorization-grant-type: authorization_code
            redirect-uri: "{baseUrl}/authorized/okta"
            ...

You may register a @Controller handler at /authorized/okta and perform the redirect there to the specific landing page based on your needs:

@Controller
public class OAuth2ClientController {

	@GetMapping("/authorized/okta")
	public String okta(@RegisteredOAuth2AuthorizedClient("okta") OAuth2AuthorizedClient authorizedClient) {
		return "redirect:/your-landing-page";
	}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may register a @Controller handler at /authorized/okta and perform the redirect there to the specific landing page based on your needs:

And how to tell this controller which url should be used for redirect?

If I understand correctly, @ttddyy (and me too) wants to redirect to different landing pages based on logic, ruled by a client, that initiates the authentication. So, the controller should receive that url from somewhere, right?

P.S. This url also preferably should be checked for validity (whitelist) somewhere because authentication server will not check it, as it will check only the redirect_url (/authorized/okta in this example) but not the next/final redirect url, which will be nothing more than opaque query parameter from the authentication server point of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jgrandja and @xak2000

I understand the state parameter is used to prevent CSRF.
On the other hand, that's why our current state parameter mixes landing url and random string to make the state parameter unique. (It was somebody else's impl, so that's my guess...)

If I understand correctly, @ttddyy (and me too) wants to redirect to different landing pages based on logic, ruled by a client, that initiates the authentication. So, the controller should receive that url from somewhere, right?

Yes, that's the idea.

Actually, in my current interim implementation, I use controller and passing the final-redirect-url in cookie.(also does whitelist check in controller).

This works, but I wanted to remove this additional round-trip request to the controller.
If server side directly redirects to the landing page, this request is not necessary. (Also removes cluttered code to pass the final-redirect-url to cookie.)

Copy link
Contributor

@xak2000 xak2000 Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please provide more details on the flow you are using? What is triggering the Authorization Request? Is your JS client redirecting directly to the Authorization Server or are you going through OAuth2AuthorizationRequestRedirectFilter?

@jgrandja I provided more details of the flow in the comment below. Do you understand the case? If not, I can provide more details.

JS client just opens (changes window.location) an url of configured oauth2Login() to initiate an authorization code flow. Then spring-security + authentication server (Keycloak in my case) do their job and in the end Keycloak redirects back to spring-security (OAuth2LoginAuthenticationFilter if I remember correctly), that exchanges the auth code (from query parameters) to tokens (sending POST request to Keycloak). After it received and saved access/refresh tokens, I want to send 302 redirect to the location of where user's browser been before JS changed the window.location the first time (to initiate an authentication flow). And this URL of course can't be stored in any configuration file or something. This URL (the location of the browser at the moment when JS client changed window.location to initiate an authentication flow) is dynamic. This can be /my-site/users or /my-site/main-page or anything else. And when normal (non-ajax) request requires an authentication (e.g. /my-site/users page requires authentication), then this case handled by spring-security using saved request cache, if I remember correctly. This works because when you try to open /my-site/users in the browser, the request received by spring-security filter is already at URL where you want to redirect the user after successful authentication. But when AJAX request requires an authentication (and normal request doesn't), then this is not the case.

Imagine that you can freely open /my-site/users page in the browser (so, this URL doesn't require any auth), and when JS client loads, it tries to load JSON from /api/users URL (this URL requires auth). When JS Client tries to send request to this URL, you don't want to save this request to replay after successful authentication. You don't want to respond with 302 redirect also. It's XHR request from JS Client. The redirect is useless here. So, the solution is to respond with 401 and let the JS Client to do "redirect" itself. This is not a real redirect of course. Just changing of the current window.location. But you still want to return to the current page after successful authentication. At this time server doesn't know current page at all. The only request it saw was an AJAX request to /api/users URL, but you want to open /my-site/users page after successful authentication. Opening of this page will send the AJAX request to the /api/users URL again and this time it will be authenticated (because of session cookie, that spring-security uses to store an Authentication object).

So, as you see, what is required in this case is an ability to provide an url from JS client to the spring-security (when JS client changes window.location to initiate an authentication flow), and that spring-security will use after successful authentication to send 302 redirect. This can be passed as query param to /oauth2/authorization/{resourceId}?target_url=http://my-site/users for example.

Of course, I not meaning that spring-security should support that custom query parameter out of the box. This is not generic use case I think, but a very specific one. But spring-security should not prevent or make it too hard to implement this. Now at least the problem that #7329 should solve prevents to implement this solution. Another option is to not ping-pong that target URL to the Keycloak and back (as opaque query param), but to save some state in the OAuth2AuthorizationRequest and an ability to use it for redirect after tokens will be exchanged. I didn't look into that second option and can't say what prevents (if any) to implement it now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation @xak2000.

The option I provided in this comment won't work for you since target_url is dynamic. The following solution should work for your case.

  1. Implement and configure a custom OAuth2AuthorizationRequestResolver that saves target_url to HttpSession. You have access to HttpServletRequest so this can be achieved.
  2. Implement and configure a custom RedirectStrategy in OAuth2AuthorizationCodeGrantFilter that reads target_url from HttpSession and performs the redirect to target_url instead of the default redirect-uri.

The prerequisites to this working is that #7329 needs to be resolved (maybe not for your case) and OAuth2AuthorizationCodeGrantFilter needs to expose a setter for RedirectStrategy.

...but to save some state in the OAuth2AuthorizationRequest and an ability to use it for redirect after tokens will be exchanged

Saving custom state to OAuth2AuthorizationRequest simply does not make sense. The OAuth2AuthorizationRequest is a representation of the OAuth 2.0 Authorization Request sent to the provider and target_url has nothing to do with the Authorization Request. The default AuthorizationRequestRepository saves OAuth2AuthorizationRequest to HttpSession so you can do the same with custom parameters via a custom OAuth2AuthorizationRequestResolver as outlined in Point 1.

spring-security should not prevent or make it too hard to implement this

Totally agreed. We need to ensure users have the right hooks so they can customize special use cases as is your case. I believe the 2 points will work for you without introducing a new interface that is similar to RedirectStrategy but accepts OAuth2AuthorizationRequest.

Keep in mind that when we introduce new API's, we will need to maintain this for a long period. And if the new API is introduced for special use cases and consequently not used as much than the value is simply not there. My goal is to help you realize your solution without introducing new API's (if possible). The way I see it now, we would need to introduce a new setter for RedirectStrategy which would be simply to do.

Let me know if my proposal will work for you. If you still feel it won't than the next step is for you to prepare a minimal sample that we can use to come up with a solution that will work. From this we will know what API's are needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ttddyy @xak2000 I've been giving this some further thought and I'm not sure exposing a setter for RedirectStrategy in OAuth2AuthorizationCodeGrantFilter is the best way to go. I'm starting to think that it might be better and more flexible to define a new API that handles a successful and unsuccessful authorization, eg. OAuth2AuthorizationSuccessHandler and OAuth2AuthorizationFailureHandler. Similar to how authentication success/failure is handled in AbstractAuthenticationProcessingFilter.

OAuth2AuthorizationCodeGrantFinalRedirectStrategy in this PR is specific to authorization_code so I think we might need an API that is more generic that would work for authorization grants that require explicit user authorization/consent.

Let me think about this one for a bit. We have SpringOne next week so I'm busy preparing for it this week. I'll circle back to this shortly.

Copy link
Contributor

@xak2000 xak2000 Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jgrandja !

The idea of saving target_url to HttpSession is good. I think it can work!

Of course, we still need some API to make use of this saved URL when authentication will succeed. I like the idea of success/failure handler. It will be more flexible than just an overriding of redirection strategy, as some may want to do some more sophisticated things than just redirect to a custom url.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ttddyy I'm circling back to this now and would like to know if you tried my suggestion as per comment.

SavedRequest savedRequest = this.requestCache.getRequest(request, response);
if (savedRequest != null) {
redirectUrl = savedRequest.getRedirectUrl();
this.requestCache.removeRequest(request, response);
}

this.redirectStrategy.sendRedirect(request, response, redirectUrl);
}

}
22 changes: 14 additions & 8 deletions ...va/org/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ public class OAuth2AuthorizationCodeGrantFilter extends OncePerRequestFilter {
private final AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource = new WebAuthenticationDetailsSource();
private final RedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
private final RequestCache requestCache = new HttpSessionRequestCache();
private OAuth2AuthorizationCodeGrantFinalRedirectStrategy finalRedirectStrategy =
new DefaultOAuth2AuthorizationCodeGrantFinalRedirectStrategy(this.redirectStrategy, this.requestCache);

/**
* Constructs an {@code OAuth2AuthorizationCodeGrantFilter} using the provided parameters.
Expand Down Expand Up @@ -128,6 +130,17 @@ public final void setAuthorizationRequestRepository(AuthorizationRequestReposito
this.authorizationRequestRepository = authorizationRequestRepository;
}

/**
* Sets the final redirect strategy.
*
* @param finalRedirectStrategy the final redirect strategy
* @since 5.2
*/
public final void setFinalRedirectStrategy(OAuth2AuthorizationCodeGrantFinalRedirectStrategy finalRedirectStrategy) {
Assert.notNull(finalRedirectStrategy, "finalRedirectStrategy cannot be null");
this.finalRedirectStrategy = finalRedirectStrategy;
}

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {
Expand Down Expand Up @@ -206,13 +219,6 @@ private void processAuthorizationResponse(HttpServletRequest request, HttpServle

this.authorizedClientRepository.saveAuthorizedClient(authorizedClient, currentAuthentication, request, response);

String redirectUrl = authorizationResponse.getRedirectUri();
SavedRequest savedRequest = this.requestCache.getRequest(request, response);
if (savedRequest != null) {
redirectUrl = savedRequest.getRedirectUrl();
this.requestCache.removeRequest(request, response);
}

this.redirectStrategy.sendRedirect(request, response, redirectUrl);
this.finalRedirectStrategy.sendRedirect(request, response, authorizationRequest, authorizationResponse);
}
}
23 changes: 23 additions & 0 deletions ...amework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFinalRedirectStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.springframework.security.oauth2.client.web;

import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;

/**
* Strategy interface to perform final redirect at the end of OAuth2 Authorization Code Grant Flow.
*
* @author Tadaya Tsuyukubo
* @since 5.2
* @see OAuth2AuthorizationCodeGrantFilter
*/
public interface OAuth2AuthorizationCodeGrantFinalRedirectStrategy {

void sendRedirect(HttpServletRequest request, HttpServletResponse response,
OAuth2AuthorizationRequest authorizationRequest,
OAuth2AuthorizationResponse authorizationResponse) throws IOException;

}