ClaimAccessor#getClaimAsMap doesn't return null as documented

[shartte]

Describe the bug
org.springframework.security.oauth2.core.ClaimAccessor#getClaimAsMap's Javadoc reads as follows:

	 * @return the claim value or {@code null} if it does not exist or cannot be assigned
	 * to a {@code Map}

The actual implementation throws an IllegalArgumentException instead, if the claim value cannot be converted to a map:

Assert.isTrue(convertedValue != null,
  () -> "Unable to convert claim '" + claim + "' of type '" + claimValue.getClass() + "' to Map.");

This may affect other methods on this class as well, but I only tested this one.

To Reproduce
Use a claim value such as:

{
   "claim": "string_value"
}

Then call getClaimAsMap("claim") and observe that the following exception will be thrown:

java.lang.IllegalArgumentException: Unable to convert claim 'claim' of type 'class java.lang.String' to Map.

Expected behavior
The Javadoc should match the actual implementation, or vice versa.

[jzheaux]

Thanks for pointing this out, @shartte.

It would be odd for accessor.getClaimAsMap("myclaim") to return null for existing claims. As such, I believe the Javadoc should be updated as opposed to the implementations.

Are you able to submit a PR to update the Javadoc for getClaimAsMap and getClaimAsStringList?

[shartte]

Sure, I can do that. Are you sure they shouldn't return null?

Depends on what an app should do with claims they can't process. There's no X.509-style "MustUnderstand" in JWT, so I am honestly unsure.

[jzheaux]

If it returns null for both failed conversions and for when the claim is not present, how would the caller tell the difference?

If you want to do a trial conversion, you can always get the claim yourself and try to convert:

Object value = this.accessor.getClaim("myclaim");
TypeDescriptor type = TypeDescriptor.map(Map.class, 
    TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(Object.class))
Map<String, Object> map = ClaimConversionService.getSharedInstance().convert(value, type);
if (map == null) {
    // try something else
}

[shartte]

Well, there's still hasClaim which would allow one to check beforehand if it's relevant to them.

edit: But this is largely a theoretical discussion :)
I'll just PR a fix to the javadoc so it's at least consistent.

[jzheaux]

I'll just PR a fix to the javadoc so it's at least consistent.

sounds good!

In case it's helpful, I'll share a couple more reasons why Spring Security favors returning an error in situations like these.

If coercion fails, it's probably due to a coding or configuration error. In security-related code, we'd prefer that devs find out about those problems early in the dev cycle, and returning an error is a reliable way to do that. It also means that for a production error, devs have fewer steps to finding out what's wrong, and they didn't need to do anything special to get that benefit.

It's also quite similar to how casting and autoboxing work in Java. If Java fails to do it, you get an error. Consider the ClaimAccessor#getClaim call:

Map<String, Object> claims = Collections.singletonMap("myclaim", Instant.now());
ClaimAccessor accessor = () -> claims;
String value = accessor.getClaim("myclaim");

Which is the least astonishing: For the above to error (since the value isn't a String), or for it to succeed but return null? Like String value = (String) claims.get("myclaim") would error, I believe that getClaim should, too. And because getClaim does, it seems reasonable for the remaining getClaimXXX methods to do so as well.

You can see another example of this preference in MappedJwtClaimSetConverter which also fails when coercion fails.

[shartte]

No need to convince me, I am using Nimbus JSONObjectUtil for conversion of the remaining claims anyhow, and they decided to use checked exceptions to communicate this.
It may however be somewhat surprising that getClaimAsString will return {} when the claim is a JSON object, instead of failing. From a quick test, getClaimAsBoolean will also silently convert a HashMap<>() (to simulate {} in the claim) to false.

[jgrandja]

@shartte Regarding your original comment, I don't see an issue with ClaimAccessor.getClaimAsMap().

Given the claim:

{
   "claim": "string_value"
}

A call to getClaimAsMap("claim") will throw :

java.lang.IllegalArgumentException: Unable to convert claim 'claim' of type 'class java.lang.String' to Map.

This is the correct behaviour.

However the javadoc can be confusing:

Returns the claim value as a {@code Map<String, Object>} or {@code null} if it does
not exist or cannot be assigned to a {@code Map}.

The text in bold is where the confusion may be.

The following javadoc should clear things up:

/**
 * Returns the claim value as a {@code Map<String, Object>} or {@code null} if it does not exist.
 * @param claim the name of the claim
 * @return the claim value or {@code null} if it does not exist
 * @throws IllegalArgumentException if the claim value cannot be converted to a {@code Map<String, Object>}
 */

What do you think?

[qavid]

@jzheaux, @jgrandja I have created PR #10357. Please take look.