No longer able to use "ArrayList" in "aud" claim

[selliera]

Describe the bug
Using 0.11.5, one is able to define a "aud" claim that is a list, as allowed by the standard.
With current main, it is no longer possible, resulting in an error.

To Reproduce
Steps to reproduce the behavior:
With 0.11.5, this code worked:

List resources = Arrays.asList("foo", "bar");
JwtBuilder tokenBuilder = Jwts.builder()
.signWith(pKey.getPrivate())
.setSubject(subject)
.claim(Claims.AUDIENCE, resources)
.build()

With current main, it produces:
"java.lang.IllegalArgumentException: Invalid JWT Claim 'aud' (Audience) value: [foo,bar]. Unsupported value type. Expected: java.lang.String, found: java.util.ArrayList

Expected behavior
Expected to be still able to use the "claim" method to set a custom but standard defined claim value for "aud".

[selliera]

However, it is still possible to obtain the result using setContent as documented, to the expense of not being able to use the convenient helper methods to set other claims.

[lhazlewood]

Hi there - when you say "current main", do you mean a build created from the master branch?

[lhazlewood]

I think I see - you were avoiding the previous Claims#setAudience API because it only supported a single String value, so you used claim to add multiple values as allowed per the spec.

Yes, the current main branch does tighter type assertions, and the current logic is incorrect - it must allow an array of strings or a single string.

So it seems that the Claims API for setAudience and getAudience should probably be changed to be a List<String>, and if finding a single string, wraps it in a list and returns it (since a Java method signature requires a type definition and Object isn't that valuable).

If this seems incorrect, please let me know.

Thanks for reporting this!!!

[selliera]

Yes, I guess having "aud" being a list, and falling back on a single string in case there is only one value is a good way to go. It will be compatible with RFC 7519 (even if the RFC does not make the fallback on single string in case of only one value mandatory).
The setAudience api could take a vararg as parameter, allowing existing code to still build, and seeming natural also in case there is only one value. But maybe that could be confusing too.
I agree on the getAudience: it is better to have type List<String> than Object. Maybe we could provide

List<String> getAudience()
String getFirstAudience()

to make it easier to manage audience when what we want is a single value.

[lhazlewood]

Good idea here - thanks for the idea and feedback! We'll definitely work on this before cutting a release

[selliera]

I can try to work on a pull request for it.
Thinking about it, it is probably better to give full control for the user building a claim, to chose between a single string, or a list with only one element.
So maybe on the "set" side, we could provide both:

public JwtBuilder setAudience(String aud);
public JwtBuilder setAudience(List<String> aud);

I saw that Fields support a stringSet, but unsure if that allow for a single string. If it is not the case, maybe I can add a stringOrStringSet in it ?

[bdemers]

Maybe getSingleAudience() and then fail if there is more than one? That way additional audiences are not ignored (potentially causing access issues elsewhere in the code)?

List<String> getAudience()
String getFirstAudience()

[lhazlewood]

I would hold off on a PR for a little bit - there's a bit of change I'm working on for immutable Header and Claims instances, and I wouldn't want either one of us to fight with much conflict. I'd definitely appreciate it after I merge though!

[lhazlewood]

Thinking out loud: I wonder if it makes sense to make setAudience(String) a convenience method that just wraps it in a List first? The RFC says that the value should be a JSON array, but a String value is supported (I think mostly for historic/legacy reasons before the RFC was finalized), so the array is preferred.

Also, List<String> getAudience() could be equally convenient: if it finds a single String value as a claim, it converts it to a List<String> first and then returns List<String> to the caller.

This way an app developer never needs to account for two data types in their code (which is undesirable IMO). Then you can do something like this:

Optional<String> firstAudience = Lists.first(jwt.getAudience())

One downside of this approach is that if the JWT creator needs to have it be a string claim (and not an array of strings) - perhaps because the JWT receiver blindly requires a String value and hasn't been updated to read arrays yet - there might need to be another way to just set a String and avoid conversion. I wonder if there's a clean alternative to do both...

[lhazlewood]

I saw that Fields support a stringSet, but unsure if that allow for a single string. If it is not the case, maybe I can add a stringOrStringSet in it ?

Yes, this is the tricky part I was hinting at in my potential merge conflict comment. This is the first time encountering a single-member-can-be-two-types case, so it might be forced to be Field<Object> which is not nice at all IMO. That's why I was hoping to coerce a string-to-list scenario and provide a single type.

[bdemers]

There are some older clients that assume the aud is a single value. So if JJWT is used on one end and an older lib on the other end that could cause a problem.

That said... Maybe that is something that is configured on the serializer? (and implement the api/impl as @lhazlewood suggests) 🤔

[selliera]

I had the same impression. I did experiment with a Field<List<String>>, keeping the getAudience and setAudience manipulating String for compatibility with existing code, and adding getAudienceList and setAudienceList (short for a better name).
Works fine: the only visible change is then that some automated tests do fail, detecting the added [] in some generated error messages.
I was worried however that this change imposes the use of a list, when the spec allows for a single string (and as you mention, we should make sure the library can interoperate with other implementations). Failed to do that without resorting to Field<Object>. With that, I had:
setAudience(String), setting audience to be a single string.
setAudience(List<String>), setting to an array.
getAudienceList did return the list, if it is a list
getAudience return the single value if there is one. If it is a list, it return the first item.

I'm not fond of the Field<Object>, but did not want to introduce another class StringOrStringList. Also, the "get first item for getAudience" may be too much.

[selliera]

Sorry I didn't notice your message about better work in that after your work on immutable instances. That's fine for me. I'm playing with the code to learn, and explore options.
In the meantime, I can make it work by managing the body using getPayload/setContent, so no need to rush. Better to have a satisfying long term solution, even if only in a future version.

[lhazlewood]

I realized we have a couple issues open relating to aud being an array. Closing this as a duplicate of #77 which we hope to have fixed in the 0.12.0 release