Describe the bug
DelegatingOAuth2UserService stream mechanism that can cause oauth2 exceptions
To Reproduce
DelegatingOAuth2UserService:
this.userServices.stream().map((userService) -> userService.loadUser(userRequest))
.filter(Objects::nonNull)
.findFirst()
.orElse(null);
u see , the first stream element maybe not be the right one. when unsuited OAuth2UserService that will cause some exception like that
[missing_openid_attribute] Missing required "some param" attribute in UserInfoEndpoint for Client Registration : some provider
and it will request all IDP user endpoint:
try wrong 1 try wrong 2 try wrong 3 oh right here
Expected behavior
accurate OAuth2UserService
Comment From: sjohnr
@NotFound403, thanks for reaching out. I'm not quite clear on what your current vs expected behavior is. For example, are you receiving an error after the first IDP user info endpoint is tried, and the next one is not tried?
Can you perhaps provide a MockMvc test that demonstrates the issue? Or could you provide a suggestion for how DelegatingOAuth2UserService can be improved that would help clarify the issue?
Comment From: NotFound403
okey, this a the mocktest
/***
* just like loop
*/
@Test
public void streamAlwaysOrdered() {
// idp1 service
OAuth2UserService<OAuth2UserRequest, OAuth2User> idp1CustomService = userRequest -> {
String registrationId = userRequest.getClientRegistration().getRegistrationId();
// request to auth server
if (!registrationId.equals("idp1")) {
// not suitable request
// this is a simulation . required param missing http 405
throw new RuntimeException(" this is an exception in auth server ");
}
List<GrantedAuthority> authorities = AuthorityUtils.createAuthorityList("USER");
Map<String, Object> attributes = new HashMap<>();
attributes.put("username", "idp1");
attributes.put("age", 22);
attributes.put("avatar", "https://avatars.githubusercontent.com/u/22664375?s=80&v=4");
String nameAttributeKey = "username";
return new DefaultOAuth2User(authorities, attributes, nameAttributeKey);
};
// idp2 service
OAuth2UserService<OAuth2UserRequest, OAuth2User> idp2CustomService = userRequest -> {
String registrationId = userRequest.getClientRegistration().getRegistrationId();
// request to auth server
if (!registrationId.equals("idp2")) {
// not suitable request
// this is a simulation required param missing http 405
throw new RuntimeException(" this is an exception in auth server ");
}
List<GrantedAuthority> authorities = AuthorityUtils.createAuthorityList("USER");
Map<String, Object> attributes = new HashMap<>();
attributes.put("username", "idp2");
attributes.put("age", 22);
attributes.put("avatar", "https://avatars.githubusercontent.com/u/22664375?s=80&v=4");
String nameAttributeKey = "username";
return new DefaultOAuth2User(authorities, attributes, nameAttributeKey);
};
// default service
DefaultOAuth2UserService defaultOAuth2UserService = new DefaultOAuth2UserService();
List<OAuth2UserService<OAuth2UserRequest, OAuth2User>> oAuth2UserServices = Arrays.asList(idp1CustomService,
idp2CustomService,
defaultOAuth2UserService);
OAuth2UserService<OAuth2UserRequest, OAuth2User> oAuth2UserService = new DelegatingOAuth2UserService<>(oAuth2UserServices);
Instant now = Instant.now();
OAuth2AccessToken accessToken = new OAuth2AccessToken(OAuth2AccessToken.TokenType.BEARER,"xxxxx", now, now.plusSeconds(180));
// when i use idp1’s userRequest ignore some params
ClientRegistration idp1 = ClientRegistration.withRegistrationId("idp1")
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.clientId("idp_client")
.clientSecret("noop")
.redirectUri("{baseUrl}/login/oauth2/code/{registrationId}")
.authorizationUri("https://idp1.com/authorize/oauth2")
.tokenUri("https://idp1.com/token/oauth2")
.userInfoUri("https://idp1.com/user/oauth2")
.build();
OAuth2UserRequest idp1Request = new OAuth2UserRequest(idp1, accessToken);
Assertions.assertNotNull(oAuth2UserService.loadUser(idp1Request));
// when i use idp2’s userRequest ignore some params
ClientRegistration idp2 = ClientRegistration.withRegistrationId("idp2")
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.clientId("idp_client")
.clientSecret("noop")
.redirectUri("{baseUrl}/login/oauth2/code/{registrationId}")
.authorizationUri("https://idp2.com/authorize/oauth2")
.tokenUri("https://idp2.com/token/oauth2")
.userInfoUri("https://idp2.com/user/oauth2")
.build();
OAuth2UserRequest idp2Request = new OAuth2UserRequest(idp2, accessToken);
// always
Assertions.assertThrows(RuntimeException.class,()->oAuth2UserService.loadUser(idp2Request));
}
Comment From: NotFound403
this is my delegate,just like DelegatingPasswordEncoder.
public class DelegatingOAuth2UserService<R extends OAuth2UserRequest, U extends OAuth2User>
implements OAuth2UserService<R, U> {
private final OAuth2UserService<OAuth2UserRequest, OAuth2User> defaultOAuth2UserService = new DefaultOAuth2UserService();
private final List<OAuth2UserService<R, U>> userServices;
private final Map<String, OAuth2UserService<R, U>> userServiceMap;
/**
* Constructs a {@code DelegatingOAuth2UserService} using the provided parameters.
* @deprecated
* @param userServices a {@code List} of {@link OAuth2UserService}(s)
*/
@Deprecated
public DelegatingOAuth2UserService(List<OAuth2UserService<R, U>> userServices) {
Assert.notEmpty(userServices, "userServices cannot be empty");
this.userServices = Collections.unmodifiableList(new ArrayList<>(userServices));
this.userServiceMap = Collections.emptyMap();
}
/**
* Constructs a {@code DelegatingOAuth2UserService} using the provided parameters.
*
* @param userServiceMap a {@code Map} of {@link OAuth2UserService}(s) k -> registrationId ，v-> {@code OAuth2UserService}
*/
public DelegatingOAuth2UserService(Map<String, OAuth2UserService<R, U>> userServiceMap) {
Assert.notEmpty(userServiceMap, "userServiceMap cannot be empty");
this.userServiceMap = Collections.unmodifiableMap(userServiceMap);
this.userServices = Collections.emptyList();
}
@Override
public U loadUser(R userRequest) throws OAuth2AuthenticationException {
Assert.notNull(userRequest, "userRequest cannot be null");
if (CollectionUtils.isEmpty(userServiceMap)) {
// @formatter:off
return this.userServices.stream()
.map((userService) -> userService.loadUser(userRequest))
.filter(Objects::nonNull)
.findFirst()
.orElse(null);
// @formatter:on
} else {
String registrationId = userRequest.getClientRegistration().getRegistrationId();
OAuth2UserService<R, U> oAuth2UserService = userServiceMap.get(registrationId);
if (oAuth2UserService == null) {
oAuth2UserService = (OAuth2UserService<R, U>) defaultOAuth2UserService;
}
return oAuth2UserService.loadUser(userRequest);
}
}
}
Comment From: sjohnr
@NotFound403, thanks so much for the mock test, that is extremely helpful in illustrating your reported issue. Here are my thoughts on this:
The DelegatingOAuth2UserService is a very simple composite implementation that tries a list of delegates in order, and simply expects null as the return value. This is a pretty simple contract to understand: as long as your requests succeed in such a way that the delegate provides null, you will not have an issue.
When the requests being issued through the composite are not valid for some of the providers, a legitimate OAuth2 error could be returned. I'm not sure it would make sense for this implementation to try and suppress or handle those errors. In this case, I feel it would be up to your application to provide adequate error handling for your use case. For example, you could wrap your delegates with another delegate that catches and handles the exception triggered by the 405 response, if that is appropriate. You can log it or do whatever you like, and then return null. Something like:
public class ErrorHandlingOAuth2UserService implements OAuth2UserService<OAuth2UserRequest, OAuth2User> {
private final OAuth2UserService<OAuth2UserRequest, OAuth2User> delegate;
public ErrorHandlingOAuth2UserService(OAuth2UserService<OAuth2UserRequest, OAuth2User> delegate) {
this.delegate = delegate;
}
@Override
public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException {
try {
return this.delegate.loadUser(userRequest);
} catch (Exception ex) {
// TODO: Handle exception...
return null;
}
}
}
What do you think of this approach?
Comment From: NotFound403
@sjohnr thanks for ur reply.
In this way it seems to be working properly, but we just handled the exception and didn't stop it. Why? The IDP1 said that the IDP2's request wasn't mine and i had to respond it, so i responded an exception.
I think we should skip the inappropriate OAuth2UserService, because that:
- The deleagate needs to try it many times, but only one is actually needed.
- Mismatched requests may carry sensitive parameters to inappropriate servers, such as bearer token or oauth2 client registration etc, that's not safe.
- In addition, the setting of returning 'null' is an implicit condition that is difficult to be found.
Expectly
idp2 oauth2 userinfo request -> idp2 try
Not
idp2 oauth2 userinfo request -> idp1 try (invalid predictably), idp2 try
The best way, DelegatingOAuth2UserService can checkout a suitable OAuth2UserService for the request, if not, provides a default one or throw an exception with there is no suitable OAuth2UserService implementation. We shouldnt to give attempt for each OAuth2UserService that delegated.
What's your opinion?
Comment From: sjohnr
Thanks for the dialogue and feedback, @NotFound403!
I think I see what you're saying, and I agree that in your situation, the DelegatingOAuth2UserService is probably not suitable. However, you provided your own implementation above that could be simplified to only use the registrationId to lookup a suitable OAuth2UserService from a Map. This seems to be the way your use case needs it to work. I'd recommend just using that implementation as a custom composite OAuth2UserService. The interface is pretty easy to implement, as you've already done it! I would actually advise using that same pattern of building composites and using delegation whenever you need to in Spring Security, there's nothing wrong with that (in my opinion).
Having said all of that, I'm not sure it's extremely important for that example implementation you provided to be contributed back into Spring Security, nor does it seem desirable to change the existing DelegatingOAuth2UserService to work that way as it unnecessarily complicates the existing code.
I think I've gathered enough information to close this issue, as the recommended solution is to build your own delegating implementation. I'm hoping that this conversation will be helpful to others looking to do something similar. As always, if I've missed anything or you have more to add, please add additional comments to keep the conversation going.