Expected Behavior

DefaultOAuth2AuthorizationRequestResolver should provide a way to customize authorizationRequestMatcher and provide a customizable way to extract the registrationId on custom scenarios where we may need to store registrationId on headers or cookies but also want to leverage the default implementation since current class is final we can't even extend it.

private Function<HttpServletRequest, String> registrationIdResolver;

public DefaultOAuth2AuthorizationRequestResolver(ClientRegistrationRepository clientRegistrationRepository, String authorizationRequestBaseUri) {
    this(clientRegistrationRepository, new AntPathRequestMatcher(authorizationRequestBaseUri + "/{" + REGISTRATION_ID_URI_VARIABLE_NAME + "}")));
}

public DefaultOAuth2AuthorizationRequestResolver(ClientRegistrationRepository clientRegistrationRepository, AntPathRequestMatcher authorizationRequestMatcher) {
    this.clientRegistrationRepository = clientRegistrationRepository;
    this.authorizationRequestMatcher = authorizationRequestMatcher;
    this.registrationIdResolver = request -> {
        return authorizationRequestMatcher.matcher(request)
                .getVariables()
                .get(REGISTRATION_ID_URI_VARIABLE_NAME);
    };
}

public void setRegistrationIdResolver(Function<HttpServletRequest, String> registrationIdResolver) {
    this.registrationIdResolver = registrationIdResolver;
}

private String resolveRegistrationId(HttpServletRequest request) {
    if (!this.authorizationRequestMatcher.matches(request)) {
        return null;
    }

    return this.registrationIdResolver.apply(request);
}

Current Behavior

Current behavior does not enable providing a custom matcher nor an alternative way to extract the registrationId value from the request. With the suggested changes we can have it customizable by possibly: 1. matching with different URIs, 2. being able to extract registrationId from request cookies, headers, etc.

Context

Currently implementing multi-tenant login support and need to recover from request context which registrationId to use. From the way that DefaultOAuth2AuthorizationRequestResolver is implemented, I had to basically copy & paste its source code to add the desired behavior since it is final and no way is provided to add custom behavior to it.

Comment From: marcusdacoregio

Hi @imerljak, thanks for the report.

Based on the javadoc, I would say that the implementation was designed to work only in a scenario where the registrationId is on the request path:

An implementation of an OAuth2AuthorizationRequestResolver that attempts to resolve an OAuth2AuthorizationRequest from the provided HttpServletRequest using the default request URI pattern /oauth2/authorization/{registrationId}.

A simple way to avoid rewriting too much code is to resolve the registrationId in your implementation and then delegating to the DefaultOauth2AuthorizationRequestResolver, something like:

public class MyResolver implements OAuth2AuthorizationRequestResolver {

    private final OAuth2AuthorizationRequestResolver delegate;

    public MyResolver(ClientRegistrationRepository repository) {
        this.delegate = new DefaultOAuth2AuthorizationRequestResolver(repository, "not-used");
    }

    @Override
    public OAuth2AuthorizationRequest resolve(HttpServletRequest request) {
    String registrationId = resolveRegistrationIdFollowingMyRequirements(request);
        return resolve(request, registrationId);
    }

    @Override
    public OAuth2AuthorizationRequest resolve(HttpServletRequest request, String registrationId) {
    return this.delegate.resolve(request, registrationId);
    }

}

Does that make sense to you?

Comment From: spring-projects-issues

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

Comment From: imerljak

@marcusdacoregio Thanks for the suggestion.. At first sight it does look like it would be sufficient but if we look a DefaultOAuth2AuthorizationRequestResolver implementation there's a difference between both methods. Where resolve(HttpServletRequest) assumes a login action and resolve(HttpServletRequest, String) assumes an authorisation action.

As seem here:

@Override
public OAuth2AuthorizationRequest resolve(HttpServletRequest request) {
  String registrationId = resolveRegistrationId(request);
  if (registrationId == null) {
      return null;
  }
  String redirectUriAction = getAction(request, "login");
  return resolve(request, registrationId, redirectUriAction);
}

@Override
public OAuth2AuthorizationRequest resolve(HttpServletRequest request, String registrationId) {
  if (registrationId == null) {
      return null;
  }
  String redirectUriAction = getAction(request, "authorize");
  return resolve(request, registrationId, redirectUriAction);
}

Having no way to directly call the private method resolve(HttpServletRequest, String, String). By following your suggestion I would always be following the 'authorisation' path. I am not sure if that would be the desired outcome.

Anyways, I happened to drop the usage of that class in my project. But regardless, the enhancement would be beneficial for those looking to add some custom behaviour into it.

Comment From: marcusdacoregio

Hi @imerljak, thanks for the feedback.

Let's leave this open and see if there is more interest from the community in getting this changed. @sjohnr do you have any other point of view on this?

Comment From: sjohnr

If I recall correct, the only thing the "login" vs "authorize" action is used for is as an optional path variable {action} which can be used in the redirect_uri that is constructed from the ClientRegistration and used to build the OAuth2AuthorizationRequest. I'd guess most deployments don't use it, and to my knowledge, none of our examples or samples use it. So, I'd venture to guess @marcusdacoregio's example above would work perfectly for you.

That being said, I'd be in favor of an additional (separate) implementation of the OAuth2AuthorizationRequestResolver that takes a Function to resolve the registrationId. Ideally, it would return a Map<String, String> and not just a String, so that numerous variables can be resolved and injected into the redirect_uri that is built.

This would be far more flexible and allow total freedom in what URL is used for the OAuth 2.0 Login endpoint (defaults to /oauth2/authorization/{registrationId} -> extracts registrationId and hard-coded action of login or authorize) and the resulting redirect_uri value (defaults to {baseUrl}/login/oauth2/code/{registrationId} with those variables expanded).

Comment From: jzheaux

Ideally, it would return a Map and not just a String, so that numerous variables can be resolved and injected into the redirect_uri that is built.

Would RequestMatcher address this? RequestMatcher#matcher returns a MatchResult with a Map of variables it resolved from the request.

I think this would also address @imerljak's initial request:

DefaultOAuth2AuthorizationRequestResolver should provide a way to customize authorizationRequestMatcher

Comment From: sjohnr

Would RequestMatcher address this? RequestMatcher#matcher returns a MatchResult with a Map of variables it resolved from the request.

I think it could, yes. In that case, I wonder if it would be possible to just do:

DefaultOAuth2AuthorizationRequestResolver should provide a way to customize authorizationRequestMatcher

and refactor to use it for resolving variables?