Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"audience" behaviour of JwtSecurityToken, JwtPayload confusing #1342

Closed
mgillis opened this issue Feb 29, 2020 · 0 comments · Fixed by #1343
Closed

"audience" behaviour of JwtSecurityToken, JwtPayload confusing #1342

mgillis opened this issue Feb 29, 2020 · 0 comments · Fixed by #1343
Labels
Customer reported Indicates issue was opened by customer Documentation The issue is related to adding documentation
Milestone

Comments

@mgillis
Copy link
Contributor

mgillis commented Feb 29, 2020

Due to these lines:

// if could be the case that some of the claims above had an 'aud' claim;
if (!string.IsNullOrEmpty(audience))
AddClaim(new Claim(JwtRegisteredClaimNames.Aud, audience, ClaimValueTypes.String));

The behaviour can be confusing, and some implementations of refresh tokens that I have seen (and is promulgated in a few tutorials out there) will pass in issuer, audience, etc., along with previous claims (from a deserialized JwtSecurityToken) to the JwtSecurityToken constructor. The resulting new token will have everything overwritten except for the audience claim, which will be a list with one new element added.

To be more clear:
If you call the constructor once with audience = "users", and claims = null, everything is fine;
If you call the constructor a second time with audience = "users" and claims including { "aud", "users" } you will end up with { "aud", ["users, "users'] }. etc.
After many refreshes of a long-lasting chain of tokens, the "aud" list can be very, very long, eventually causing problems (e.g. HTTP header size too big)

I am not saying the implementation is incorrect, and it's clear in the RFC that aud can be a list, but I suggest the documentation should be improved to clarify this behaviour. For example, instead of "a claim will be added", you could write "a claim will be added, appended to any 'aud' claims already in 'claims', if present."

The three places this documentation is copied are:

/// <param name="audience">If this value is not null, a { aud, 'audience' } claim will be added</param>

/// <param name="audience">If this value is not null, a { aud, 'audience' } claim will be added</param>

/// <param name="audience">If this value is not null, a { aud, 'audience' } claim will be added</param>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer reported Indicates issue was opened by customer Documentation The issue is related to adding documentation
Projects
None yet
2 participants