LtiLibrary / LtiAdvantage

LTI Advantage class library for .NET applications.
MIT License
34 stars 26 forks source link

Setting Roles to single value results in incorrect claim value #100

Open hisuwh opened 5 months ago

hisuwh commented 5 months ago

If I set the Roles property to a single value:

var request = new LtiResourceRequest
{
   // ...
   Roles = new[] { LtiAdvantage.Lti.Role.ContextLearner }
};

Then the claim that gets sent is a string value (which is not correct and causes errors downstream):

"https://purl.imsglobal.org/spec/lti/claim/roles": "http://purl.imsglobal.org/vocab/lis/v2/membership#Learner"

However, if I set multiple values:

var request = new LtiResourceRequest
{
   // ...
   Roles = new[] { LtiAdvantage.Lti.Role.InstitutionLearner, LtiAdvantage.Lti.Role.ContextLearner }
};

Then it correctly sends an array:

"https://purl.imsglobal.org/spec/lti/claim/roles": ["http://purl.imsglobal.org/vocab/lis/v2/institution/person#Learner", "http://purl.imsglobal.org/vocab/lis/v2/membership#Learner"]

I was working around this previously by setting multiple Roles but that is not always appropriate for all Role types

srijken commented 3 months ago

@hisuwh What type of request are you creating? I don't see LtiResourceRequest in the codebase, only LtiResourceLinkRequest and LtiDeepLinkingRequest?

I tried with this unit test, and this succeeds as well:

        [Fact]
        public void Roles_SingleRole_ReturnsArray()
        {
            var request = new LtiResourceLinkRequest
            {
                DeploymentId = "12345", 
                TargetLinkUri = "https://www.example.edu",
                ResourceLink = new ResourceLinkClaimValueType
                {
                    Id = "12345"
                },
                UserId = "12345",
                Roles = new[]{Role.ContextLearner}
            };

            Assert.True(request.TryGetValue("https://purl.imsglobal.org/spec/lti/claim/roles", out var rolesJson));
            var roles = ((JsonElement)rolesJson).ToStringList();
            Assert.Contains("http://purl.imsglobal.org/vocab/lis/v2/membership#Learner", roles);
        }

Maybe the NET6 merge fixed this?

hisuwh commented 3 months ago

Yes I meant LtiResourceLinkRequest.

It would seem the claims property returns this as multiple claims rather than a single claim with a JSON array value:

image image

srijken commented 3 months ago

I still don't really get it though:

image

hisuwh commented 3 months ago

I don't think the way of reading the claim as you have above is representative. I see the same as you using .TryGetValue but then as per the samples I'm using .Claims to get the claims to add to the payload

hisuwh commented 3 months ago

I've been using https://saltire.lti.app/ to test this which doesn't like it, neither does the tool provider I am working with.

hisuwh commented 3 months ago

I take that back. It works in saltire - this might be a customer issue.

JWT: image

Claims: image

hisuwh commented 3 months ago

If I send multiple roles it does look different:

JWT: image

srijken commented 3 months ago

~~The behavior you're seeing here is from JwtPayload, looks like here it changes from single value to array when adding the second claim of the same type: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for- dotnet/blob/dev/src/System.IdentityModel.Tokens.Jwt/JwtPayload.cs#L687-L702~~

The LTI 1.3 spec says this:

"https://purl.imsglobal.org/spec/lti/claim/roles" - Array of URI values for roles that the user has within the message's context. This claim is recommended to use the LTI-defined roles listed in Membership Roles and Types

So I think it's still a bug that needs fixing

srijken commented 3 months ago

I don't think the way of reading the claim as you have above is representative. I see the same as you using .TryGetValue but then as per the samples I'm using .Claims to get the claims to add to the payload

Can you explain, maybe with some code, what you mean by using .Claims to get the claims to add to the payload? I think the JwtPayload baseclass should do it's thing, and I can't find a way to prove it's not working yet. Only thing is that .Claims enumerates the roles separately, and TryGetValue, or just (JsonElement)request["https://purl.imsglobal.org/spec/lti/claim/roles"]; returns it as an array, even when there's only one role.

hisuwh commented 3 months ago

Can you explain, maybe with some code, what you mean by using .Claims to get the claims to add to the payload

As per your sample code here:

https://github.com/LtiLibrary/LtiAdvantagePlatform/blob/86f0cd4205d324128f80cfa664db884b3b08c1ec/src/Utility/LtiAdvantageProfileService.cs#L351