Adds userinfo_endpoint to authorization server metadata #489
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
jgrandja
added
status: duplicate
jgrandja
left a comment
jgrandja
left a comment
There was a problem hiding this comment.
Thanks for the PR @enlightenedmouse. Please see review comment.
| .tokenEndpoint(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getTokenEndpoint())) | ||
| .tokenEndpointAuthenticationMethods(clientAuthenticationMethods()) | ||
| .jwkSetUrl(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getJwkSetEndpoint())) | ||
| .userInfoEndpoint(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getOidcUserInfoEndpoint())) |
There was a problem hiding this comment.
The UserInfo endpoint is not defined in RFC 8414 2. Authorization Server Metadata and instead defined only in 3. OpenID Provider Metadata.
Therefore, the update should only be accessible in OidcProviderConfiguration not in OAuth2AuthorizationServerMetadata
There was a problem hiding this comment.
Hi @jgrandja ... Got it thanks : ) I added it because of the comment at the end of Section 2. "Additional authorization server metadata parameters MAY also be used" and the sample in section 3.2. Authorization Server Metadata Response shows userinfo_endpoint. But more so, I added it because I wasn't sure if the ClientRegistration within oauth2-client would be configured using the openid configuration endpoint or an authorization server metadata endpoint. After looking into the docs, it appears it tries both. So your call ; ) Let me know.
sjohnr
left a comment
sjohnr
left a comment
There was a problem hiding this comment.
Hi @enlightenedmouse, thanks for the PR! Sorry for the long delay (over the holidays), but I wanted to pick this thread up again and help move it forward. I have provided some feedback for your PR below. Would you be able to take a look?
| .tokenEndpoint(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getTokenEndpoint())) | ||
| .tokenEndpointAuthenticationMethods(clientAuthenticationMethods()) | ||
| .jwkSetUrl(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getJwkSetEndpoint())) | ||
| .userInfoEndpoint(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getOidcUserInfoEndpoint())) |
There was a problem hiding this comment.
I think the preference at the moment would be to remove this from OAuth2AuthorizationServerMetadataEndpointFilter at this time, as we want to align as closely to the spec as possible (and not support optional features/items such as this).
Since the OAuth2AuthorizationServerMetadataEndpointFilter is not very customizable at the moment, we should see if anyone requires this, and open an enhancement for customization options instead if needed.
Would you be able to provide an update to this PR to address this?
| if (getClaims().get(OAuth2AuthorizationServerMetadataClaimNames.JWKS_URI) != null) { | ||
| validateURL(getClaims().get(OAuth2AuthorizationServerMetadataClaimNames.JWKS_URI), "jwksUri must be a valid URL"); | ||
| } | ||
| if (getClaims().get(OAuth2AuthorizationServerMetadataClaimNames.USER_INFO_ENDPOINT) != null) { |
There was a problem hiding this comment.
Related to supporting only oidc-configuration, please move this to OidcProviderConfiguration. Also, I think we should add a test to ensure this is covered.
| * @param userInfoEndpoint the {@code URL} of the OAuth 2.0 UserInfo Endpoint | ||
| * @return the {@link AbstractBuilder} for further configuration | ||
| */ | ||
| public B userInfoEndpoint(String userInfoEndpoint) { |
There was a problem hiding this comment.
Related to supporting only oidc-configuration, please move this to OidcProviderConfiguration and update the javadoc accordingly.
| /** | ||
| * {@code userinfo_endpoint} - the {@code URL} of the OAuth 2.0 UserInfo Endpoint | ||
| */ | ||
| String USER_INFO_ENDPOINT = "userinfo_endpoint"; |
There was a problem hiding this comment.
Related to supporting only oidc-configuration, please move this to OidcProviderMetadataClaimNames.
| .tokenEndpointAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC.getValue()) | ||
| .tokenEndpointAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_POST.getValue()) | ||
| .jwkSetUrl(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getJwkSetEndpoint())) | ||
| .userInfoEndpoint(asUrl(this.providerSettings.getIssuer(), this.providerSettings.getOidcUserInfoEndpoint())) |
There was a problem hiding this comment.
I think we should add to an existing test to ensure this is covered.
|
Just a friendly reminder also that it is now 2022 ❗😱, so you will want to think about updating changed files with 2022 in the copyright headers. 😄 |
|
@enlightenedmouse have you had a chance to review the above comments? |
|
@enlightenedmouse I went ahead and merged this into |
4 participants
Fixes gh-488