OpenId Connect: Requesting acr Claims not possible

[furti]

There is already issue #7168 but it is only about using the "acr_values" parameter. But using this parameter makes the acr request "voluntary". The only way I see to access the acr Parameter as essential, is to use the method described below.

Summary

I need to request the acr claim within openid connect login. As far as I can see, there is no way to do this with the current implementation.

OpenID Connect Spec

https://openid.net/specs/openid-connect-core-1_0.html#ClaimsParameter

...
The claims parameter value is represented in an OAuth 2.0 request as UTF-8 encoded JSON (which ends up being form-urlencoded when passed as an OAuth parameter). When used in a Request Object value, per Section 6.1, the JSON is used as the value of the claims member.
...
An example Claims request is as follows:
{
"userinfo":
{
"given_name": {"essential": true},
"nickname": null,
"email": {"essential": true},
"email_verified": {"essential": true},
"picture": null,
"http://example.info/claims/groups": null
},
"id_token":
{
"auth_time": {"essential": true},
"acr": {"values": ["urn:mace:incommon:iap:silver"] }
}
}

The claims Parameter must be sent as a JSON String and is urlencoded.

Actual Behavior

When we have a custom implementation that adds the claims additional parameter, it is added to the authorization request. But it is not correclty encoded.

org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest.Builder.buildAuthorizationRequestUri() uses

return UriComponentsBuilder.fromHttpUrl(this.authorizationUri)
					.queryParams(parameters)
					.encode(StandardCharsets.UTF_8)
					.build()
					.toUriString();

under the hood. The way it is used, does encoding of the values, but does not encode template parameters. The claims parameter should be a JSON. JSON starts with "{" and ends with "}", so the UriComponentsBuilder thinks, it is a Template Parameter and does not encode it.

But we can't encode it before putting it in the builder, otherwise the %XX will be encoded twice, and will result in %25XX.

Expected Behavior

It should be possible to set parameters of the form "{...}"

Configuration

ClientRegistrationRepository clientRegistrationRepository = getClientRegistrationRepository();

        builder.oauth2Login(oauth2Login -> {
            oauth2Login.clientRegistrationRepository(clientRegistrationRepository);

            oauth2Login.authorizationEndpoint(authorizationEndpoint -> {
                authorizationEndpoint
                    .authorizationRequestResolver(
                        new PartnerNetOAuth2AuthorizationRequestResolver(clientRegistrationRepository,
                            OAuth2AuthorizationRequestRedirectFilter.DEFAULT_AUTHORIZATION_REQUEST_BASE_URI));
            });

            oauth2Login.userInfoEndpoint(userInfoEndpoint -> {
                userInfoEndpoint.oidcUserService(new PartnerNetOpenIdConnectUserService());
            });
        });

PartnerNetOAuth2AuthorizationRequestResolver is the custom implementation that simply added this line of code to the default implementation:
additionalParameters.put("claims", String.format("{\"id_token\":{\"acr\": {\"values\": [\"%s\"], \"essential\": true}}} ", acr));

Version

5.2.2

Sample

[jgrandja]

@furti I believe this has been fixed via #7871, which has been backported to 5.2.3. Can you please try and let me know.

[furti]

@jgrandja Sorry for that. Did not realize, that there was already a new version out.

#7871 already fixed it. But I'm not sure if the fix is a 100% correct.

Now everything is encoded before creating the URIComponentsBuilder, but afterwards the UriComponents are build like this:

return UriComponentsBuilder.fromHttpUrl(this.authorizationUri)
    .queryParams(parameters)
    .build()
    .toUriString();

build() is called without parameters.

Under the hood this calls

public UriComponents build() {
	return build(false);
}

Javadoc of this method says:

/**
 * Build a {@code UriComponents} instance from the various components
 * contained in this builder.
 * @param encoded whether all the components set in this builder are
 * encoded ({@code true}) or not ({@code false})
 * @return the URI components
 */
public UriComponents build(boolean encoded) {
	return buildInternal(encoded ?
				EncodingHint.FULLY_ENCODED :
			this.encodeTemplate ? EncodingHint.ENCODE_TEMPLATE : EncodingHint.NONE);
}

This means, now the code encodes everything correclty, but calls the method that tells the UriComponentsBuilder, that the values are not encoded. As nothing is done with the uri components afterwards, this makes not difference. But for clarity I would suggest to call .build(true) instead.

Maybe a better fix would have been to not encode everything manually but instead

return UriComponentsBuilder.fromHttpUrl(this.authorizationUri)
    .queryParams(parameters)
    .build()
    .encode()
    .toUriString();

This will save a lot of code in OAuth2AuthorizationRequest and should have the same effect I think.

[furti]

@jgrandja Feel free to close this issue if you think the UriComponentsBuilder is used correclty in this place.

Or tell me what version of the two fixes you prefer, and I can create a dedicated issue for this, so we can close this one, as the actual problem is already fixed.

[jgrandja]

@furti Good catch! Yes, it should be .build(true). Would you be interested in submitting a PR for this fix?

FYI, we cannot use .build().encode() as an alternative solution because the authorizationUri passed into .fromHttpUrl(this.authorizationUri) may contain encoded query parameters, which would result in double encoding.

[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.

[jgrandja]

@furti Regarding your suggestion that build() should be build(true) - this is no longer valid as the changes in this commit modified the implementation. So we are good.

Closing as duplicate of #7871

[furti]

Perfect. Using the UriBuilderFactory is even better than using the UriComponentsBuilder directly I think. :)