Implement User Info Endpoint #303
Conversation
…spring-authorization-server into spring-projects-experimental-main
|
@idosal Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
spring-projects-issues
added
the
status: waiting-for-triage
|
@idosal Thank you for signing the Contributor License Agreement! |
|
Apologies for the delay @idosal. I will review within the next few days. |
jgrandja
added
type: enhancement
|
@idosal I'm pushing this out for the |
There was a problem hiding this comment.
@idosal, this is indeed a great start! Sorry for the wait time, I'm going to jump in and help move this forward.
First off, you will need to rebase on main before you start working on these changes.
I would suggest going back to commit e4df09b (e.g. git reset --hard e4df09b40e8da43eb30eb110cbb15b09181b7695), squash your commits (e.g. git rebase -i HEAD~2) and rebase the result on main.
You will have conflicts to resolve in the configurer, unfortunately. 😞 If you have trouble with this, let me know.
Once we get through the below round of changes, we can look at adding tests and testing using the samples project to make sure this is working.
| @@ -0,0 +1,169 @@ | |||
| /* | |||
| * Copyright 2020 the original author or authors. | |||
| * @author Ido Salomon | ||
| * @see AbstractHttpMessageConverter | ||
| * @see OidcUserInfo | ||
| * @since 0.1.1 |
There was a problem hiding this comment.
Move below @author and change to 0.2.0, since that's the release we're targeting now.
| import java.util.Map; | ||
|
|
||
| /** | ||
| * A {@link HttpMessageConverter} for an {@link OidcUserInfo OIDC User Info Response}. |
There was a problem hiding this comment.
Please change OIDC to OpenID. I'll tag some other places as well, but go ahead and change all comment references to OpenID.
|
|
||
| private static final ParameterizedTypeReference<Map<String, Object>> STRING_OBJECT_MAP = | ||
| new ParameterizedTypeReference<Map<String, Object>>() { | ||
| }; |
There was a problem hiding this comment.
Please remove whitespace, as in {}.
|
|
||
| private final GenericHttpMessageConverter<Object> jsonMessageConverter = HttpMessageConverters.getJsonMessageConverter(); | ||
|
|
||
| private Converter<Map<String, Object>, OidcUserInfo> oidcUserInfoConverter = new OidcUserInfoConverter(); |
There was a problem hiding this comment.
I think we should prefer to remove oidc prefix from internal variable and parameter names. I'll tag some other places as well.
| */ | ||
| public OAuth2AuthorizationServerConfigurer<B> userInfoClaimsMapper(UserInfoClaimsMapper userInfoClaimsMapper) { | ||
| Assert.notNull(userInfoClaimsMapper, "userInfoClaimsMapper cannot be null"); | ||
| this.getBuilder().setSharedObject(UserInfoClaimsMapper.class, userInfoClaimsMapper); |
There was a problem hiding this comment.
With reduced visibility of the mapper class and moving it to this package, this would now wrap the given Function<Object, OidcUserInfo>, e.g.
this.getBuilder().setSharedObject(UserInfoClaimsMapper.class, new OidcUserInfoClaimsMapper(userInfoClaimsMapper));This is so the shared object mechanism works internally, but the class is not exposed in the public API.
| @@ -314,6 +334,13 @@ private void initEndpointMatchers(ProviderSettings providerSettings) { | |||
| OAuth2AuthorizationServerMetadataEndpointFilter.DEFAULT_OAUTH2_AUTHORIZATION_SERVER_METADATA_ENDPOINT_URI, HttpMethod.GET.name()); | |||
| this.oidcClientRegistrationEndpointMatcher = new AntPathRequestMatcher( | |||
| providerSettings.oidcClientRegistrationEndpoint(), HttpMethod.POST.name()); | |||
| this.oidcUserInfoEndpointMatcher = new OrRequestMatcher( | |||
| new AntPathRequestMatcher( | |||
| OidcUserInfoEndpointFilter.DEFAULT_OIDC_USER_INFO_ENDPOINT_URI, | |||
There was a problem hiding this comment.
Please add a new setting to ProviderSettings called oidcUserInfoEndpoint and use it here. It would default to /userinfo though ProviderSettings does not use constants from anywhere else in the codebase. Change the visibility of this constant in the filter to private.
| @@ -384,6 +411,18 @@ private static void validateProviderSettings(ProviderSettings providerSettings) | |||
| return jwtCustomizer; | |||
| } | |||
|
|
|||
| private static <B extends HttpSecurityBuilder<B>> UserInfoClaimsMapper getUserInfoClaimsMapper(B builder) { | |||
There was a problem hiding this comment.
It looks like when resolving conflicts, the code that configures the filter got dropped. See note on review.
After you rebase on main, this should be moved to OAuth2ConfigurerUtils.
| private static <B extends HttpSecurityBuilder<B>> UserInfoClaimsMapper getUserInfoClaimsMapper(B builder) { | ||
| UserInfoClaimsMapper userInfoClaimsMapper = builder.getSharedObject(UserInfoClaimsMapper.class); | ||
| if (userInfoClaimsMapper == null) { | ||
| userInfoClaimsMapper = getOptionalBean(builder, UserInfoClaimsMapper.class); |
There was a problem hiding this comment.
Since I suggested reducing the visibility of this class, the user cannot provide a bean of this type. I haven't tested it, but here's a suggested alternative implementation of this method using ResolvableType that would allow a user to provide a bean of type Function<Object, OidcUserInfo>:
static <B extends HttpSecurityBuilder<B>> Function<Object, OidcUserInfo> getUserInfoClaimsMapper(B builder) {
OidcUserInfoClaimsMapper userInfoClaimsMapper = builder.getSharedObject(OidcUserInfoClaimsMapper.class);
if (userInfoClaimsMapper == null) {
ResolvableType type = ResolvableType.forClassWithGenerics(Function.class, Object.class, OidcUserInfo.class);
Function<Object, OidcUserInfo> mapperFunction = getOptionalBean(builder, type);
if (mapperFunction == null) {
mapperFunction = (principal) -> OidcUserInfo.builder()
.subject(principal.toString())
.build();
}
userInfoClaimsMapper = new OidcUserInfoClaimsMapper(mapperFunction);
builder.setSharedObject(OidcUserInfoClaimsMapper.class, userInfoClaimsMapper);
}
return userInfoClaimsMapper;
}|
|
||
| import org.springframework.security.oauth2.core.oidc.OidcUserInfo; | ||
|
|
||
| public class DefaultUserInfoClaimsMapper implements UserInfoClaimsMapper { |
There was a problem hiding this comment.
Based on other suggestions, I think you can delete this class.
|
Closing in favor of #441 |
sjohnr
added
status: declined
|
@sjohnr Damn, I completely missed the review. Sorry about that. Please let me know if there's any way I can contribute. |
|
@sjohnr Thanks, I'm glad it helped. I'll review it ASAP. |
Hey @jgrandja,
this is a very rough draft to verify I'm in the right direction. I'd love to get your feedback to see how I can deliver this ASAP.
Thanks