NPE in DefaultOAuth2User.getName function about spring-security

[HyoJongPark]

@jzheaux I have made a PR, can you check and merge please?

I wanted to treat it as a more meaningful exception, but I thought the preferred method in Spring Security was a check using assertion, so I handled it the same way.

If you think it is necessary to deal with a more meaningful exception, i will supplement the work later.

[Eccenux]

So, do you mean that instead of NPE or RuntimeException, we should handle null value using more meaningful exceptions such as 'UserNotFoundException'?

Yes, any meaningful Exception extending an AuthenticationException or more specifically OAuth2AuthenticationException.

[Eccenux]

I hope you know that using Assert.notNull throws IllegalArgumentException which in turn will cause 500:

I think it's not well documented that one should do:

        try {
		OAuth2User oauth2user = super.loadUser(userRequest);
        } catch (IllegalArgumentException e) {
...
        }

DefaultOAuth2UserService.loadUser(.) is only document to throw OAuth2AuthenticationException, see:
https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.html#loadUser(org.springframework.security.oauth2.client.userinfo.OAuth2UserRequest)

[injae-kim]

I think it's not well documented that one should do:

	public DefaultOAuth2User(..) {
		Assert.notEmpty(attributes, "attributes cannot be empty");
		Assert.hasText(nameAttributeKey, "nameAttributeKey cannot be empty");
		if (!attributes.containsKey(nameAttributeKey)) {
			throw new IllegalArgumentException("Missing attribute '" + nameAttributeKey + "' in attributes");
		}

On DefaultOAuth2User, it already can throw IllegalArgumentException when attribute is empty or null nameAttributeKey, which is not documeneted.

IllegalArgumentException is common exception on java and usually can be thrown on other class's constructor too, so it's okay to not add it on document?

I understand your point but not sure we need to add IllegalArgumentException on document in this case 🤔

Let's wait maintainer's opinion too~!

[jzheaux]

Thanks, @Eccenux, for the report. It sounds like the reason there is an NPE is because the application is placing a null value into the map for the nameAttributeKey. Does that sound right?

If so, I think it's reasonable to check for the DefaultOAuth2User constructor to, instead of doing this:

if (!attributes.containsKey(nameAttributeKey)) {
	throw new IllegalArgumentException("Missing attribute '" + nameAttributeKey + "' in attributes");
}

do this:

Assert.notNull(attributes.get(nameAttributeKey), "Missing attribute '" + nameAttributeKey + "' in attributes"), 

Are you able to create a pull request to make this change?

[Eccenux]

Would that Assert.notNull be caught by Spring Security though?

I mean it's safe to assume this is true:

		Assert.notEmpty(attributes, "attributes cannot be empty");
		Assert.hasText(nameAttributeKey, "nameAttributeKey cannot be empty");

It's something that should be true for valid implementation.

On the other hand, what is in attributes.get(nameAttributeKey) depends on idP, so an external entity.

[jzheaux]

No, it would not be caught by Spring Security.

My suggested change means that passing a map that has a key with a null value is an illegal usage of the class. I wouldn't want business logic to continue in that case. Instead, I'd want the application to address the illegal usage.

AuthenticationPrincipal#getName should not return null as can be seen in the JavaDoc:

/**
  * Returns the name of the authenticated <code>Principal</code>. Never
  * <code>null</code>.
  * @return the name of the authenticated <code>Principal</code>
*/

Because of that, if the value is null, there is nothing reasonable to return from getName(). This means that the constructor should fail in that circumstance.

depends on idP, so an external entity

Only you know what a key with a null value means in your arrangement, so it would be up to you to check for that condition and then throw a UserNotFoundException or whatever is the correct interpretation.

[Eccenux]

AuthenticationPrincipal#getName should not return null as can be seen in the JavaDoc:...

So if that happens it's a bug in a different place of Spring Security. My code is not parsing attributes. Spring Security is parsing them.

[Eccenux]

For those that stumble upon this I can offer this solution:

	public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException {
		final OAuth2User oauth2user = super.loadUser(userRequest);
		final String providerId = userRequest.getClientRegistration().getRegistrationId();
		// extra validation of user profile data (as returned by idP)
		validateProfile(providerId, oauth2user);
	}

	/**
	 * Validate user profile data.
	 *
	 * @param providerId code name of the provider (e.g. "google").
	 * @param oauth2user OAuth profile.
	 * @throws UsernameNotFoundException invalid profile.
	 */
	private void validateProfile(final String providerId, final OAuth2User oauth2user) throws UsernameNotFoundException {

			String email = oauth2user.getAttribute("Email"); // same as `userNameAttributeName` in  the providers config (ClientRegistrationFactoryBean)
			if (email == null || email.isEmpty()) {
				final String message = "Unexpected profile value (providerId: %s, email: [%s]".formatted(
						providerId, email
				);
				LOG.warn(message);
				throw new UsernameNotFoundException(message);
			}
	}

I guess that works too.

[HyoJongPark]

@jzheaux, @Eccenux I'm interested in doing this issue.
May I handle this issue? I will open PR if possible.

For those that stumble upon this I can offer this solution:

I think this is a good way to delegate null check to the user and I agree that the subject of the error is Spring Security, not my code, but I think the user should be able to get a response for giving the wrong value.

Because users will not know the need for the null check code until the NPE occurs in DefaultOauth2User#getName, and only then will they feel the need for the code and handle the error in some form.

I think this is a code that can generate unnecessary resources, and as long as we are aware of this issue, i would like to give users a response to the wrong value through exception.

---

Suggestion:

1. Let's create an environment where NPE doesn't occur?

AuthenticationPrincipal#getName should not return null as can be seen in the JavaDoc:

If the issue is oriented in the direction of delegating null check to the user, I think we can consider modifying it like the format below, but as mentioned in JavaDoc, this is not a good way to do it.

public String getName() {
    return Objects.isNull(this.getAttribute(this.nameAttributeKey)) ? null : this.getAttribute(this.nameAttributeKey).toString();
}

2. Provide exception for environments where NPE may occur

As @jzheaux said, I think it would be a good way to cause an error to the user by null checking in the constructor or getter.

I think I'll probably work on it by null checking through theconstructor, what do you think?

[Eccenux]

Correct me if I'm wrong but if the check is done in a constructor this will actually fail work:

public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException {
	final OAuth2User oauth2user = super.loadUser(userRequest); // <-- constructor makes it fail here
	final String providerId = userRequest.getClientRegistration().getRegistrationId();
	// extra validation of user profile data (as returned by idP)
	validateProfile(providerId, oauth2user); // <---- never got to the validation part
}

AFAIK that 500 error with NPE skips all your configuration within the application and will display full stack even when you disable presenting it within your application (which can be considered a security problem as it display implementation details). Not sure what happens when the assertion fails.

[Eccenux]

For me a good solution would be to throw an error that can be caught. So not a RuntimeException like NPE. I mean NPE can technically be caught, but NPE doesn't have a meaning and is typically not something that is declared in documentation.

[HyoJongPark]

validateProfile(providerId, oauth2user); // <---- never got to the validation part

I also thought if the return value for 'getName()' is null, then the constructor of 'DefaultOauth2User' would do null check and throw exception. and then the user handle the result (exception) instead of the null check.

If the return value for 'getName()' is null, I thought of doing a null check in the constructor of 'DefaultOuth2User' and throw an exception.

Therefore, Our code performs the handle of the results (exceptions) instead of null checks(validateProfile).

do this in constructor:

Assert.notNull(attributes.get(nameAttributeKey), "Missing attribute '" + nameAttributeKey + "' in attributes"), 

check this in user code:

public class OAuth2UserServiceImpl extends DefaultOAuth2UserService {
    private static final Logger LOG = LoggerFactory.getLogger(OAuth2UserServiceImpl.class);
    // ...

    @Override
    public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException {
        try {
            OAuth2User oauth2user = super.loadUser(userRequest);
            final String providerId = userRequest.getClientRegistration().getRegistrationId();
            final String username = oauth2user.getName();
             …
        } catch (IllegalArgumentException e) {
            //this part up to user
            throw new UsernameNotFoundException(message);
        }
}

---

So not a RuntimeException like NPE. I mean NPE can technically be caught, but NPE doesn't have a meaning and is typically not something that is declared in documentation.

So, do you mean that instead of NPE or RuntimeException, we should handle null value using more meaningful exceptions such as 'UserNotFoundException'?

[jzheaux]

If you think it is necessary to deal with a more meaningful exception, i will supplement the work later.

Thanks, no, I think IllegalArgumentException is the most meaningful exception for when an application gives invalid input to a component.

Yes, any meaningful Exception extending an AuthenticationException or more specifically OAuth2AuthenticationException.

These would be less meaningful because they would intimate that there is some problem with authentication, which there isn't, at least not in the constructor. The problem is that an application is handing an invalid value to a component. The additional meaning that you are deriving here is usage-specific.

For example, consider that DefaultOAuth2User can be constructed without it being in the context of an authentication. IOW, an authentication might not be happening at the time. It would be a misleading assumption to throw an authentication exception from DefaultOAuth2User.

I hope you know that using Assert.notNull throws IllegalArgumentException which in turn will cause 500:

This is a result of how you are using the class. It should throw a 500 because that indicates that it is the server's responsibility to address the issue. It ensures that you address the issue quickly by making sure that your application does not hand null values to DefaultOAuth2User.

The way I would recommend an application address the fact that the IdP gave a null value for the user name would be handle things before they reach DefaultOAuth2User. While I don't know the meaning of an IdP sending an invalid response instead of returning an error code, it seems that this is equivalent to an error code. So, I'd probably create a custom HttpMessageConverter that adapts the null-username response into an error response and then do:

DefaultOAuth2UserService service = new DefaultOAuth2UserService();
RestTemplate rest = new RestTemplate();
rest.setErrorHandler(new OAuth2ErrorResponseErrorHandler());
rest.setMessageConverters(List.of(new MyMissingUsernameAdaptingHttpMessageConverter()));
service.setRestOperations(rest);