Open knoobie opened 3 years ago
@knoobie To support signed and/or encrypted UserInfo Response, we would need to add a new implementation of OAuth2UserService
or maybe enhance the existing OidcUserService
. The solution (workaround) provided in the SO post is the correct approach - register a custom OAuth2UserService
that is capable of handling a signed and/or encrypted UserInfo Response.
Let's leave this issue open and see if there is demand for this feature, via upvotes.
@jgrandja Thanks for your quick look at it!
I'm wondering if it would be still possible to improve the current implementation of DefaultOAuth2UserService
to reduce the amount of boilerplate / copy paste we have to implement - like e.g. changing the getResponse
method from private
to protected
and add RestOperations restOperations
as parameter. The OAuth2UserRequest
should have all the information regarding the used ClientRegistration
. I'm happy to create a PR for that, if you think it's worth it.
Before:
private ResponseEntity<Map<String, Object>> getResponse(OAuth2UserRequest userRequest,
RequestEntity<?> request) {
// ...
}
After:
protected ResponseEntity<Map<String, Object>> getResponse(RestOperations restOperations,
OAuth2UserRequest userRequest, RequestEntity<?> request) {
// ...
}
@knoobie I'm not sure it makes sense to pass RestOperations
as a parameter, given that it's a member of DefaultOAuth2UserService
.
@jgrandja The idea behind it was to allow the usage of the already configured Restoperations
(private member) within the protected
overwritten getResponse
method. I could change the field to protected as well, but wasn't sure if that's something you would go along with.
Edit: But allowing getResponse
to be protected should be enough to reduce the whole copy paste of the class - the Restoperations
is just a nice to have / the icing on the cake.
@knoobie
allowing
getResponse()
to beprotected
I still don't see the value in exposing this since all it does is catch Exception
's and format the message
before throwing.
We typically only "open" things up when there is value in reuse. The reason we keep things closed to start with is because it's fewer public API's we need to support and it allows us to make changes passively.
If there is a good reason to open things up because it allows greater reuse than we can consider it. But as far as I see it now, opening up getResponse()
doesn't really provide that much benefit of reuse.
Maybe you could provide a code snippet of what you are trying to achieve?
Also, can you please open a new ticket so we can continue the discussion there since this is not related to the main issue.
@jgrandja Created https://github.com/spring-projects/spring-security/issues/9629 for it. Thanks for your feedback here!
Hi,
Upvoted because I encountered a same issue. But how to make OAuth2UserService smarter ? I mean: how to keep existing implementation and add application/jwt the implementation dependant of the received content type ? My use-case needs to support multiple OpenID connect with various configurations.
In my case, I prefered using a JwtDecoderFactory
rather JwtDecoder
.
An idea (replaces getResponse
) :
private Map<String, Object> getUserAttributes(OAuth2UserRequest userRequest, RequestEntity<?> request) {
try {
ResponseEntity<String> response = this.restOperations.exchange(request, String.class);
// Special case - JWT decoding
if (response.getHeaders() != null && response.getHeaders().getContentType() == JwtHttpMessageConverter.MEDIA_TYPE_JWT) {
Jwt jwt = getJwt(userRequest, response.getBody());
return jwt.getClaims();
}
try {
return OBJECT_MAPPER.readValue(response.getBody(), TYPE_REFERENCE_RESPONSE);
} catch (JsonProcessingException ex) {
OAuth2Error invalidUserInfoJsonError = new OAuth2Error(INVALID_USER_INFO_JSON_ERROR_CODE,
"An error occurred while attempting to retrieve the UserInfo Resource: " + ex.getMessage(), null);
throw new OAuth2AuthenticationException(invalidUserInfoJsonError, invalidUserInfoJsonError.toString(), ex);
}
}
catch (OAuth2AuthorizationException ex) {
// ...
}
}
private Jwt getJwt(OAuth2UserRequest userRequest, String userInfoToken) {
JwtDecoder jwtDecoder = jwtDecoderFactory.createDecoder(userRequest.getClientRegistration());
try {
return jwtDecoder.decode(userInfoToken);
} catch (JwtException ex) {
OAuth2Error invalidUserInfoJwtError = new OAuth2Error(INVALID_USER_INFO_JWT_ERROR_CODE,
"An error occurred while attempting to decode the UserInfo Resource: " + ex.getMessage(), null);
throw new OAuth2AuthenticationException(invalidUserInfoJwtError, invalidUserInfoJwtError.toString(), ex);
}
}
@florianberthe See this comment on the initial work required before this feature can be implemented.
@jgrandja I've read it. It was a suggestion to handle multiple clients with encrypted userinfo (or not).
Expected Behavior
The implementations of
OAuth2UserService
support theapplication/jwt
content type when fetching the UserInfo resource.Related Spec Info:
OpenID Connect Core 1.0 - 5.3.2. Successful UserInfo Response
Current Behavior
The implementations of
OAuth2UserService
areDefaultOAuth2UserService
andOidcUserService
. These implementations only support theapplication/json
content type when fetching the UserInfo resource. Resulting in406 Not Acceptable
when the given identity provider only allowsapplication/jwt
or this error when provided with a customrestOperations
which allowsapplication/jwt
:https://github.com/spring-projects/spring-security/blob/eff4cdc9241d264376e06ce0117c5a6715f77094/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java#L139-L147
Context
The identity provider we have to use is build with "high security in mind" and forces the use of signed JWT.
More in depth information and a workaround I found while developing: https://stackoverflow.com/questions/59876435/spring-fails-for-userinfo-endpoint-returning-signed-jwt