Summary

Carefully propose changes to the specification of the OAuth2AuthorizationRequestResolver interface. I feel that the DefaultOAuth2AuthorizationRequestResolver is now given many responsibilities.

  1. Matches the pattern of requestUri in HttpServletRequest.
  2. Create an OAuth2AuthorizationRequest using ClientRegistration.
  3. Determines Redirect by whether the return value is null. (via.. OAuth2AuthorizationRequestRedirectFilter)

I think OAuth2AuthorizationRequestRedirectFilter is responsible for # 1 and # 3. OAuth2AuthorizationRequestResolver should only be responsible for creating OAuth2AuthorizationRequest.

All requests go through the OAuth2AuthorizationRequestResolver.

  • All requests that are not related to OAuth2 authentication call the resolver. And it will pass through this filter only if the response is null.
  • The OAuth2AuthorizationRequestResolver is determining whether to pass the filter.
OAuth2AuthorizationRequest authorizationRequest = this.authorizationRequestResolver.resolve(request);
if (authorizationRequest != null) {
    this.sendRedirectForAuthorization(request, response, authorizationRequest);
    return;
}

filterChain.doFilter(request, response);

->

  • In OAuth2AuthorizationRequestRedirectFilter, match requestUri and determine whether to proceed with the filter.
  • OAuth2AuthorizationRequestResolver is only responsible for creating OAuth2AuthorizationRequest using HttpServletRequest and registrationId.
String registrationId = this.resolveRegistrationId(request);
if (registrationId != null) {
    OAuth2AuthorizationRequest authorizationRequest = this.authorizationRequestResolver.resolve(request, registrationId);
    if (authorizationRequest != null) {
        this.sendRedirectForAuthorization(request, response, authorizationRequest);
        return;
    }
}

filterChain.doFilter(request.response);

There may be confusion in java config.

  • In the following java config, it can be confusing what the value of redirect-base-uri will be. ("/oauth2/authorization-1/{registrationId}" or "/oauth2/authorization-2/{registrationId}")
  • The actual behavior is overridden by the setting of authorizationRequestResolver, but it is generally expected that baseUri will determine the processingUrl of the filter.
http
    .oauth2Login()
    .authorizationEndpoint()
        .baseUri("/oauth2/authorization-1")
        .authorizationRequestResolver(new DefaultOAuth2AuthorizationRequestResolver(
                           clientRegistrationRepository, "/oauth2/authorization-2"));

->

  • The configuration below is expected to be baseUri will be filter processing url without any reason.
http
    .oauth2Login()
    .authorizationEndpoint()
        .baseUri("/oauth2/authorization-1")
        .authorizationRequestResolver(new DefaultOAuth2AuthorizationRequestResolver(
                           clientRegistrationRepository));

Exception Handling

  • When a ClientAuthorizationRequiredException occurs, the DefaultOAuth2AuthorizationRequestResolver is doing a work-around to get the registrationId.
DefaultOAuth2AuthorizationRequestResolver.java

private String resolveRegistrationId(HttpServletRequest request) {
    // Check for ClientAuthorizationRequiredException which may have been set
    // in the request by OAuth2AuthorizationRequestRedirectFilter
    ClientAuthorizationRequiredException authzEx =
            (ClientAuthorizationRequiredException) request.getAttribute(AUTHORIZATION_REQUIRED_EXCEPTION_ATTR_NAME);
    if (authzEx != null) {
        return authzEx.getClientRegistrationId();
    }
    if (this.authorizationRequestMatcher.matches(request)) {
        return this.authorizationRequestMatcher
        .extractUriTemplateVariables(request).get(REGISTRATION_ID_URI_VARIABLE_NAME);
    }
    return null;
}

->

  • When a ClientAuthorizationRequiredException occurs, you can provide the registrationId directly to the correct OAuth2AuthorizationRequestResolver.
OAuth2AuthorizationRequestRedirectFilter.java

String registrationId = authzEx.getClientRegistrationId();
OAuth2AuthorizationRequest authorizationRequest = this.authorizationRequestResolver
                                      .resolve(request, registrationId);
  • Since the OAuth2AuthorizationRequestResolver is not released, I think there is a chance of a design change.
  • It would be nice if you could think of a better way.
  • Thank you. I am waiting for 5.1.0 GA.

Comment From: jgrandja

@mhyeon-lee Thank you for your feedback!

Let me address each of your main points:

I feel that the DefaultOAuth2AuthorizationRequestResolver is now given many responsibilities. 1. Matches the pattern of requestUri in HttpServletRequest. 2. Create an OAuth2AuthorizationRequest using ClientRegistration. 3. Determines Redirect by whether the return value is null. (via.. OAuth2AuthorizationRequestRedirectFilter)

I think OAuth2AuthorizationRequestRedirectFilter is responsible for # 1 and # 3. OAuth2AuthorizationRequestResolver should only be responsible for creating OAuth2AuthorizationRequest.

The OAuth2AuthorizationRequestResolver is a very simple interface with one operation. It's sole purpose is to return an OAuth2AuthorizationRequest if it's able to resolve one from the provided HttpServletRequest or null if it can't resolve.

As per your 1st point:

  1. Matches the pattern of requestUri in HttpServletRequest. and I think OAuth2AuthorizationRequestRedirectFilter is responsible for # 1

If OAuth2AuthorizationRequestRedirectFilter is responsible for matching and extracting the clientRegistrationId than how will the OAuth2AuthorizationRequestResolver receive the clientRegistrationId? Based on your proposal, you are suggesting to change OAuth2AuthorizationRequestResolver to:

OAuth2AuthorizationRequest resolve(HttpServletRequest request, String registrationId)

Adding registrationId as a parameter argument reduces the flexibility and it's very possible some implementations will not resolve based on registrationId. For example, there may be a cookie-based implementation of AuthorizationRequestRepository that writes a cookie to the response via AuthorizationRequestRepository.saveAuthorizationRequest and than the OAuth2AuthorizationRequestResolver reads the cookie and constructs the OAuth2AuthorizationRequest from the attributes stored in the cookie. This would result in the registrationId argument not being used for this implementation.

Furthermore, I think it makes sense to keep the request matching in the OAuth2AuthorizationRequestResolver because it can only resolve and construct the OAuth2AuthorizationRequest using it's own strategy for matching.

We need to keep the OAuth2AuthorizationRequestResolver.resolve() operation as is with only the HttpServletRequest parameter to keep things flexible.

As per your 3rd point:

  1. Determines Redirect by whether the return value is null. (via.. OAuth2AuthorizationRequestRedirectFilter) I think OAuth2AuthorizationRequestRedirectFilter is responsible for # 3.

Again, the sole purpose of the OAuth2AuthorizationRequestResolver is to return an OAuth2AuthorizationRequest if it's able to resolve one from the provided HttpServletRequest or null if it can't resolve. The OAuth2AuthorizationRequestRedirectFilter determines the control flow based on the return value of OAuth2AuthorizationRequestResolver. So if the resolver returns a OAuth2AuthorizationRequest, than this signals to the filter that the current request is an Authorization Request and therefore it will perform the redirect - as the current request is targeted for this filter.

Does this make sense?

Comment From: mhyeon-lee

@jgrandja

I understood your intentions. Thank you for your detailed explanation.

I will close this issue and related PR.

Comment From: jgrandja

@mhyeon-lee Thanks again for your feedback and your efforts!