NPE in DefaultOAuth2User.getName function

[Eccenux]

Describe the bug
Problem is a possible NullPointerException (NPE) in DefaultOAuth2User.getName.

To Reproduce
Get null value on the name attribute during after OAuth authentaction.
Or simply create attributes for DefaultOAuth2User as show in the Sample.

Expected behavior
NPE shouldn't be going around Spring Security. It should maybe use throw new UsernameNotFoundException(.), not sure.

Sample

This is a minimal example to reproduce (without setting up an oauth2 server):

		Collection<GrantedAuthority> authorities = Arrays.asList(
			new SimpleGrantedAuthority("ROLE_USER")
		);
		var attributes = new HashMap<String, Object>();
		var nameAttributeKey = "Email";
		attributes.put("UserName", "");
		// notice this is in the attributes, but is null
		attributes.put(nameAttributeKey, null);
		var test = new DefaultOAuth2User(authorities, attributes, nameAttributeKey);
		test.getName();

This code is a problem:

	@Override
	public String getName() {
		return this.getAttribute(this.nameAttributeKey).toString();
	}

Real code
In a real use case this was the attributes of the DefaultOAuth2User:

User Attributes: [{UserName=, Email=null, Authenticated=false, Claims=null}]

The name field was "Email", but I guess it could be anything that just so happens to be null. Note that attributes are from an external service. The service is defined by userInfoUri from FactoryBean<T>. So the attributes are essentially attributes of a user profile as returned by the identity provider (idP).

This is how getName was originally called:

public class OAuth2UserServiceImpl extends DefaultOAuth2UserService {
	private static final Logger LOG = LoggerFactory.getLogger(OAuth2UserServiceImpl.class);
	// ...
	@Override
	public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException {
		OAuth2User oauth2user = super.loadUser(userRequest);

		final String providerId = userRequest.getClientRegistration().getRegistrationId();
		// this results in NPE
		final String username = oauth2user.getName();
		// ...
}

[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.