Expose getter for nameAttributeKey in OAuth2AuthenticatedPrincipal

[andreblanke]

Given a DefaultOidcUser or DefaultOAuth2User, it is currently not possible to create a faithful copy of the instance since the constructors require a nameAttributeKey for the all-arg constructor:

public DefaultOidcUser(Collection<? extends GrantedAuthority> authorities, OidcIdToken idToken,
		String nameAttributeKey)

public DefaultOAuth2User(Collection<? extends GrantedAuthority> authorities, Map<String, Object> attributes,
		String nameAttributeKey)

which is not accessible.

This issue has been mentioned before in #14461 (comment) where a custom ExtendedOidcUser decorator class is used to work around it. While that works, I feel that this should be possible without introducing a separate class using just the public API.

The PR aims to change this by adding the OAuth2AuthenticatedPrincipal.getNameAttributeKey method in 464b078. This is a breaking change for classes implementing the interface.

With the nameAttributeKey now available from within the interface, I figured it also makes sense to provide a default implementation for OAuth2AuthenticatedPrincipal.getName in c7bcf86.

[jzheaux]

Thanks, @andreblanke! I've left some feedback inline.

[jzheaux]

Hi, @andreblanke! Are you able to make the requested changes? No rush, if I don't hear from you in about a week, I'm happy to make the changes myself.

[andreblanke]

Hi there @jzheaux. First of all thank you for your feedback. Sorry, I've been postponing this. I'd like to finish the changes in the next few days once we've decided on an implementation (getter for nameAttributeKey vs. new constructors).

[jzheaux]

Thanks for the updates, @andreblanke! I've left some feedback inilne.

[jzheaux]

Is it guaranteed at this point that attribute.get(userNameAttributeName) is non-null? If not, please do a null check so that the code can offer a more informative error message.

[andreblanke]

You're right, attributes is nullable and an assertion is appropriate here, basically what my newly introduced DefaultOAuth2User.getNameFromAttributes is doing.

However, I can't help but feel that "get name from attributes using nameAttributeKey with assertions" is a frequent enough operation (4 mostly similar occurrences have been touched by this PR) to maybe warrant a separate method.

What would you think about changing the constructors to use name instead of nameAttributeKey (as done by the PR) but introducing static factory methods for DefaultOidcUser and DefaultOAuth2User taking nameAttributeKey as parameter instead of name?
That would get rid of the old, inconsistent constructors (in terms of both the parameter order and the usage of nameAttributeKey instead of name) while giving a clear upgrade path. Additionally, OidcUserAuthority.collectClaims mentioned above would not need to be duplicated or made accessible.

[jzheaux]

Yes, I think that could work. Perhaps a builder in that case, though, as that's more typical of Spring Security classes. It's also a bit more resilient against member additional member variables getting added over time.

Perhaps:

public static DefaultOAuth2User.Builder withNameAttributeKey(String nameAttributeKey);

[andreblanke]

I've added builders for DefaultOAuth2User and DefaultOidcUser. Right now the new "canonical constructor" is still public, but it might be better to make it private to be in line with, e.g., OAuth2AuthorizeRequest?

[jzheaux]

Is this correct? It seems like it should be idToken.getAttribute(IdTokenClaimNames.SUB).

Same question for DefaultOidcUser(OidcIdToken, OidcUserInfo, Collection<? extends GrantedAuthority>)

[andreblanke]

Both constructors ultimately delegate to

public DefaultOAuth2User(String name, Map<String, Object> attributes,
			Collection<? extends GrantedAuthority> authorities) {
    // ...
    this.name = (name != null) ? name : (String) this.attributes.get("sub");
}

which is consistent with the constructor behavior of DefaultOAuth2AuthenticatedPrincipal, although using IdTokenClaimNames.SUB instead of "sub" would be more appropriate (nevermind, OAuth2 core classes should likely not access OpenId Connect-related classes).

[andreblanke]

However, for that to succeed I'd still need to remove the Assert.notNull(name, "name cannot be null"); from the constructor which is still present at the moment.

[jzheaux]

Will you please add DefaultOAuth2UserMixinTests and DefaultOidcUserMixinTests to ensure that serializations of either arrangement can correctly deserialize?

[andreblanke]

It seems that Jackson only supports a single @JsonCreator for each class, meaning the backwards compatibility would be a bit more involved (perhaps using a custom deserializer?).

[jzheaux]

Yes, the next evolution of that mixin would be to use a deserializer.