fixed aud validation and changed aud field type to Option<String>

[Dowwie]

Validation type's aud field and the aud validation logic requires reconsideration.

The Validation type should use aud: Option<String>, not Option<Value>. The jwt spec states that aud can be one or more strings, and this remains true for the aud claim within a jwt. However, this rule does not apply to the Validation config's aud field. The Validation aud field is a name (who am I -- a service named "one"). This name is used to check audience membership with the jwt's aud claim.
the aud field of a jwt claim

Suppose a token server creates a subject a new token that is valid for use with server's "one" and "two". One way to accomplish this is by setting the aud claim within the jwt to ["one", "two"].

Consider server "one" registers its name, "one", for the aud field within its Validation config (directly, without the need for set_audience). When it authenticates a request that contains a jwt token where the aud claim is ["one", "two"], server "one" ought to consider this token valid because "one" is a member of the intended audience. Further, server "two" ought to validate the same, assuming it registered "two" within its Validation config. Yet, the current validation logic does not take this into consideration. The current validation logic is all-or-nothing. What I propose instead is a check for audience membership.

The jwt spec seems to require the use of a serde_json::Value type for the claim's aud field so as to consider String or Vec. Consequently, the new aud validation logic will need to convert to compatible types that can then check Validation.aud for membership.

[Keats]

I am not sure the audience validation is always ever a single string, see jpadilla/pyjwt#205 for example.
The Ruby library also allows checking for an array: https://github.com/jwt/ruby-jwt/blob/master/README.md#audience-claim and same for the JS library.
I would prefer keeping it as it is right now since there are clearly users needing that to be a list.

[Dowwie]

@Keats
My PR follows the jwt specification precisely: "Each principal intended to process the JWT MUST identify itself with a value in the audience claim."

This is a test of membership. Note that the specification refers to a principal, not a plural principals. The singular principal name is what is saved in the Validation's aud String . The validation test is for membership: is this String in the list presented in the jwt?

As the jsonwebtoken stands today, a system can only validate aud that directly matches that provided in the jwt. I am advancing a PR that considers a jwt created for authentication across a number of systems.

In other words, my PR supports the following:

Token Server to Recipient: "Here's a token that you can use to authenticate with systems A, B, or C."
Token Recipient to System A: "Here's my jwt. It is valid for authentication with systems A, B, or C (see my aud)"
System A: "I recognize myself as system A (see my Validation config). I see that my name is in your aud claim. Therefore, access granted."

Where as the current jsonwebtoken validation requires many identity tokens be created:

Token Server to Recipient: "Here's a token that you can use to authenticate with systems A"
Token Server to Recipient: "Here's a token that you can use to authenticate with systems B"
Token Server to Recipient: "Here's a token that you can use to authenticate with systems C"
Token Recipient to System A: "Here's my jwt that only works with System A. It is valid for authentication with systems A.(see my aud)"
System A: "I recognize myself as system A (see my Validation config). I see that my name is in your aud claim. Therefore, access granted."

[Dowwie]

@Keats this requires your consideration, still..

[Keats]

Right, it's closer to the spec, but libraries in other languages do allow validation against an array even if they didn't before, as seen in the PR linked above.
Changing that later if people needs it (eg moving a current service from python/ruby/node/.net, I haven't checked the other languages but I assume they also allow passing an array to the audience validation) as an array would be a breaking change.
Since this is a Value, a user can add only a string if they want to and the somewhat niche usecase for array validation is still covered.

[Dowwie]

@Keats Are you in agreement about the need for membership test and is it clear what I'm trying to achieve with it?

The idea is that the token could have a comprehensive list of audience members that is a superset to Validation's aud field. I have no issues with sticking to the serde_json::Value for the aud field within Validation. This just requires a set operation instead of what I've initially provided. Also, it requires consideration of whether to verify that ALL or ANY of the list members be present in the token's aud.

Supposing that Validation.aud: <Option> = ["one", "two"], the membership test is EITHER that A) both are present within the token's aud or B) any are present within the token's aud

Thoughts?

[Dowwie]

@Keats yes?

[Keats]

I see the issue now.
I think in the case of lists, it should look for ANY match in it so it is indeed buggy right now.

[Dowwie]

@Keats I've updated the PR. All tests pass. The latest involves no breaking change on set_audience public api. However, aud field is public and the type is now HashSet so this will introduce breaks for those who have not used set_audience. Using HashSet is preferable to deriving a HashSet for every Validation computation. Let me know what you think..

[Dowwie]

@Keats ready for review

[Keats]

Can you do the PR on the v7 branch? Since it does contain a breaking change.

This means we can also change set_audience to something like add_audience that takes a &str and not expose Value at all.

[Keats]

this definitely should not panic should I would just replace the unwrap_or_else with expect

[Dowwie]

unwrap_or_else is the idiomatic recommendation by clippy for expect, which panics as does the unwrap that is there now

[Dowwie]

I don't understand what you're trying to achieve with add_audience(&str). Audience is set in one call. It isn't incrementally built.

I can resubmit this as 0.7..

[Dowwie]

moved to next branch, submitted a new PR: #105