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

Decoding JWT with array of Claim causes exception #427

Closed
m1dst opened this issue Jul 8, 2022 · 18 comments
Closed

Decoding JWT with array of Claim causes exception #427

m1dst opened this issue Jul 8, 2022 · 18 comments
Assignees
Labels

Comments

@m1dst
Copy link

m1dst commented Jul 8, 2022

If I have an encoded JWT:

eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJPbmxpbmUgSldUIEJ1aWxkZXIiLCJpYXQiOjE2NTcyNzY0OTgsImV4cCI6MTY4ODgxMzAzMywiYXVkIjoid3d3LmV4YW1wbGUuY29tIiwic3ViIjoianJvY2tldEBleGFtcGxlLmNvbSIsIkdpdmVuTmFtZSI6IkpvaG5ueSIsIlN1cm5hbWUiOiJSb2NrZXQiLCJFbWFpbCI6Impyb2NrZXRAZXhhbXBsZS5jb20iLCJSb2xlIjpbIk1hbmFnZXIiLCJQcm9qZWN0IEFkbWluaXN0cmF0b3IiXX0.vHyzme52-ZQnrR6ub3nJNndqQo_C_lveLaJCSePXdsk

Which looks like this:

{
 typ: "JWT",
 alg: "HS256"
}.
{
 iss: "Online JWT Builder",
 iat: 1657276498,
 exp: 1688813033,
 aud: "www.example.com",
 sub: "jrocket@example.com",
 GivenName: "Johnny",
 Surname: "Rocket",
 Email: "jrocket@example.com",
 Role: [
  "Manager",
  "Project Administrator"
 ]
}.
[signature]

I can decode it successfully using the following code:

var payload = JwtBuilder.Create()
                        .WithAlgorithm(new HMACSHA256Algorithm())
                        .WithSecret(key)
                        .MustVerifySignature()
                        .Decode<IDictionary<string, object>>(token);

However, if I try to decode it as part of the ASPNetCore auth pipeline it fails with an exception.

info: JWT.Extensions.AspNetCore.JwtAuthenticationHandler[2]
      Error decoding JWT: Unexpected character encountered while parsing value: [. Path 'Role', line 1, position 192., returning failure
      Newtonsoft.Json.JsonReaderException: Unexpected character encountered while parsing value: [. Path 'Role', line 1, position 192.
         at Newtonsoft.Json.JsonTextReader.ReadStringValue(ReadType readType)
         at Newtonsoft.Json.JsonTextReader.ReadAsString()
         at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateDictionary(IDictionary dictionary, JsonReader reader, JsonDictionaryContract contract, JsonProperty containerProperty, String id)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
         at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
         at JWT.Serializers.JsonNetSerializer.Deserialize(Type type, String json)
         at JWT.Extensions.AspNetCore.JwtAuthenticationHandler.GetAuthenticationResult(String header)

If I have only a single claim called Role or remove it completely then it works fine. I saw something in an old issue that seemed similar and I'm wondering if that was fixed which is why I can decode it manually. Does something need fixing for the AspNetCore pipeline to make it work?

I suppose I could encode the array and decode it later but that just masks the issue.

@m1dst
Copy link
Author

m1dst commented Jul 8, 2022

I think it is working if I update to a prerelease nuget. So you might be able to close this.

@abatishchev abatishchev self-assigned this Jul 8, 2022
@abatishchev
Copy link
Member

Hi! Yes, support for a complex object (inside claims) was added only recently. Before it was a dictionary of string, string.

@m1dst
Copy link
Author

m1dst commented Jul 8, 2022

Whilst it doesn't raise an exception decoding, there is some other issue perhaps with the mapping of claims.

options.AddPolicy("myPolicy", builder => builder
        //.RequireClaim("Email", "jrocket@example.com")
        .RequireClaim("Role", "Manager")
        //.RequireRole("Manager")
        //.RequireClaim(ClaimTypes.Role, "Manager")
        .RequireAuthenticatedUser());

If I protect with a claim check on email, it works.
If I protect with a claim check on role with Manager, but only supply Manager as the value (not an array) it works.
If I supply multiple Role claims (as above) and then try to check if they have the Manager role with any of the following, it doesn't work,

.RequireRole("Manager")
or
.RequireClaim(ClaimTypes.Role, "Manager")

If I look at the ticket supplied in OnSuccessfulTicket I see....

image

This looks to me like nothing is getting mapped. Could it be because it was a string and now it is a complex object?

@abatishchev
Copy link
Member

It might have to be a comma-separated list. By default, claim value is a string, according to its constructor. Or might not be supported at all :(

Please let me know what you find out - happy to learn, make any changes if necessary.

Note that the screenshot shows claims that aren't present in the initial sample token. Or I'm missing something?

@m1dst
Copy link
Author

m1dst commented Jul 8, 2022

The screenshot is showing that rather than a List, it is a string with the contents of the type. Shouldn't it be a real List?

All the samples I've seen for Role seem to indicate it can be an array of strings.

@abatishchev
Copy link
Member

From quickly googling, I see it's always something like Roles = "Admin, User".

Can you share any docs when it's an array?

@abatishchev abatishchev reopened this Jul 8, 2022
@m1dst
Copy link
Author

m1dst commented Jul 11, 2022

I've also tried adding the role using the name "http://schemas.microsoft.com/ws/2008/06/identity/claims/role" too, e.g.:

eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzUxMiJ9.eyJleHAiOjE2NTc1NDA4NzIsImNsYWltMSI6MCwiY2xhaW0yIjoiY2xhaW0yLXZhbHVlIiwiaHR0cDovL3NjaGVtYXMubWljcm9zb2Z0LmNvbS93cy8yMDA4LzA2L2lkZW50aXR5L2NsYWltcy9yb2xlIjpbIk1hbmFnZXIiLCJNYW5hZ2VyMiJdfQ.31Xg-92E3uNYlcbp0weIaBWCCQt7_r4L__mRto4luKzrgcuho0nx9bV9RMRD7jvWx4JRaAX2-EO8MHz-Mp9y2w
{
  "typ": "JWT",
  "alg": "HS512"
}.{
  "exp": 1657540872,
  "claim1": 0,
  "claim2": "claim2-value",
  "http://schemas.microsoft.com/ws/2008/06/identity/claims/role": [
    "Manager",
    "Manager2"
  ]
}.[Signature]

@abatishchev
Copy link
Member

Sorry, I'm on vacation for another week, can't run any code to debug.

First things first. Let's identity the primary issue. What it is? Is something not working in JWT library itself or only in ASP.NET Core extension?

@abatishchev
Copy link
Member

Try to change options.PayloadType, by default it's a dictionary what means each claim name must be unique. What happens if you a list of key-value pairs? Let's say KeyValuePair<string,string>[]. But then it can't be an array of roles, each role should be specified separately.

@abatishchev
Copy link
Member

Another option is to continue using an array of roles but then you need to provide custom IIdentityFactory, see DefaultIdentityFactory.cs

@abatishchev
Copy link
Member

Upon rerun, I'll make DefaultIdentityFactory more friendly for inheritance and extension.

@m1dst
Copy link
Author

m1dst commented Jul 11, 2022

Yeah, I think the problem is the extension. Having just debugged the extension I find this is the line that maps the array to a string.

var claims = payload.Select(p => new Claim(p.Key, p.Value.ToString()));

There is so much conflicting information online vs APIs it is difficult to know how to proceed. I shall keep reading the MS docs as this is what we're trying to make work together. There may be a limitation in their implementation which restricts what is possible in an easy way.

@abatishchev
Copy link
Member

Using the DI container, register another version of the interface's implementation where you'll decode the payload and read the claims manually.

@abatishchev
Copy link
Member

@m1dst please see if #428 would help to achieve what you need.

@abatishchev
Copy link
Member

@m1dst ping. I can publish the extension package if the proposed change would help you.

@m1dst
Copy link
Author

m1dst commented Jul 14, 2022

Thanks for your speedy response.

I've been looking and thinking about this but haven't yet come to a conclusion. It seems there is a clear difference between the JWT spec and the way it has been implemented in Net Core. Or perhaps in the way we are understanding it.

There is ClaimValueType to consider. https://stackoverflow.com/questions/44677784/custom-claim-with-boolean-type
dotnet/runtime#18267

I think for the moment a full rethink of my approach is needed until I understand why there is so much conflicting information. I want to do things a standard way rather than just override what is there and make it do things in the way I think they should work.

@abatishchev
Copy link
Member

Hi, let me close the issue for now. Feel free to reopen or open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants