MappedJwtClaimSetConverter should not mutate claim set

[jgrandja]

MappedJwtClaimSetConverter allows the ability to add/remove a claim and rename a claim name in the claim set. This use case does not seem valid. After the Authorization Server creates/signs the Jwt, it should never be altered by a Resource Server or Client. It should only be decoded and used as-is.

Ideally we should remove this capability but this will break compatibility since it was introduced in 5.1. We should figure out a strategy here.

[jzheaux]

I think this is a reasonable perspective. We should discuss the pros and cons.

First, we have added claims in the past, before this converter existed. For example, in NimbusJwtDecoderJwkSupport, the code indicated in the Jwt constructor a value for issuedAt that was never specified by the JWT. While it manifests itself in a slightly different way in MappedJwtClaimSetConverter, it was part of the initial thinking for how the converter should behave.

I don't mention this as a matter of precedent, but instead, it might be valuable to review why NimbusJwtDecoderJwkSupport's specifying an issuedAt is valid where this is not.

Second, insofar as this has already been released, it's not clear to me what we can do about it, except, if we agree that this is a bad practice, change our documentation accordingly. I'm not convinced that it is a bad practice, but I can see the value in not overtly recommending it in the reference docs. Perhaps we could also add a flag to turn off this behavior (it wouldn't add iat, it would reject converters that remove claims, and it would only work with the existing claim set).

Now, whether or not this is a bad practice I think is worth some discussion as well, though again, I see where you are coming from. I'm not certain if you are open to discuss this, but the rest of my comments are why I believe it's fine to leave this feature as it is.

First, the JWT itself isn't altered, so I see this as more of a semantics issue than something like broken access control or accidental misconfiguration. For any outbound communication, the application would need to look up the original JWT as always. This class doesn't change or compromise that. Additionally, the user must specifically configure these converters--it doesn't happen by accident.

Second, coercion itself is, indeed, mutating the claim set. That may sound a little pedantic, so let me explain. The original JWT is a signed and encoded JSON payload. It gets serialized into Java, and, at that point, we make certain choices about how to represent that. For example, the specification states that iat and exp are numeric. However, the code doesn't use Long to represent them. If we wanted a high-fidelity representation of the original value, we would leave these as Long or String or whatever they were originally. This actually is the very argument against doing any type coercion.

My point in bringing that up is that a user already cannot use Jwt to get a high-fidelity representation of the original, other than to decode the original JWT herself. Because of that, the true read-only boundary for a Jwt is JwtDecoder, which aligns with NimbusJwtDecoderJwkSupport specifying an issuedAt value.

So, the most reasonable seems to be that, if the user wants a guarantee that the claim names are the same (he doesn't have a guarantee that the values are the same since the code coerces them), he simply doesn't provide converters that do this, which is why I think that updating the documentation is what makes the most sense.

[jgrandja]

we have added claims in the past, before this converter existed. For example, in NimbusJwtDecoderJwkSupport, the code indicated in the Jwt constructor a value for issuedAt that was never specified by the JWT

Yes, this is one instance only where it has happened. However, the reasoning makes sense IMO. If the exp is provided in the JWT but the iat was not provided than we default to exp minus 1 sec. This logic is more defensive than anything as I believe if exp is provided than more than likely iat is provided. However, the logic is on the defensive side and I don't believe this will affect things downstream.

...but the rest of my comments are why I believe it's fine to leave this feature as it is

After reviewing the original issue #5223, there is no mention of the capability to add/remove/rename claims so this feature caught be by surprise

Second, coercion itself is, indeed, mutating the claim set.

This is not actually true. The sole purpose of this feature is all about type conversion. The types of each claim value may change but the true representation of the value should never change.

@rwinch Can you provide your feedback here as well.

[rwinch]

The issue is not intrinsic to MappedJwtClaimSetConverter, but to the fact that the JwtDecoder implementations allow a Converter<Map<String, Object>, Map<String, Object>> to convert the claims.

I agree with Josh that the JWT has not been manipulated, but just the Java representation of claims. We already mutate the result by converting it. Even though the implementation is operating on individual claims, the API contract is Map in and Map out so it shouldn't be a surprise that claims can be added and removed. This is beneficial because there may be the need to normalize claims from different providers within the Java representation.

[jgrandja]

...not intrinsic to MappedJwtClaimSetConverter, but to the fact that the JwtDecoder implementations allow a Converter<Map<String, Object>, Map<String, Object>> to convert the claims.

Very true. Essentially a user can completely convert any/all claims from the original JWT claim set, which provides quite a bit of control. I wonder if we allowed for too much control here?

the API contract is Map in and Map out so it shouldn't be a surprise that claims can be added and removed

Given the Map in Map out contract, I agree that it shouldn't be a surprise. However, my understanding of the original issue #5223 and the one currently in flight #6245 is all about type conversion. To be clear, these issues are to address type conversion of claim values in the Jwt Claim Set. The value representation should not change, just the type. So when I discovered that claim names can be renamed or new claims can be added....this surprised me. I guess the bottom line is that there is more than type conversion happening here.

This is beneficial because there may be the need to normalize claims from different providers within the Java representation

This can also be done in a custom JwtAuthenticationConverter, which seems more appropriate to me.

Either way, this doesn't seem to be of any concern so I'll close it.