Make the LTI JWKS exchange more general for all LMSes

[rob-3]

Hopefully I'll be able to sort out the issues that resulted from the first time we merged this in.

[rob-3]

@ottenhoff this code, and specifically the $this->session->set() line, causes UDOIT to break in Canvas. Any idea why? Once this is resolved, I can merge this back in.

// Context is optional and contains a unique id, label, title, and type
if (!empty($token->{'https://purl.imsglobal.org/spec/lti/claim/context'})) {
    $contextFields = (array) $token->{'https://purl.imsglobal.org/spec/lti/claim/context'};
    foreach ($contextFields as $key => $val) {
        $this->session->set('lms_course_' . $key, $val);
    }
}

[ottenhoff]

I don't use Canvas and don't have a test account so I can only speculate:

• http://www.imsglobal.org/spec/lti/v1p3/#context-claim
• Context means the course information
• What is Canvas sending over in this context claim? What is the id they are sending?
• Does the lms_course_id set here interfere with the custom lms_course_id being set in src/Controller/LtiController.php?

I know I've mentioned this previously, but I recommend trying to remove reliance on custom Canvas API calls when the LTI 1.3 spec provides everything you need.

[rob-3]

Ok, thanks. I'll look into this some more. It seems likely that overwriting some of that custom canvas information is the root of the problem.

[rob-3]

You're right; this is caused by lms_course_id. Working on a fix

[rob-3]

@ottenhoff Is it reasonable to just choose a different prefix for these context variables?

The lms_course_id being set from canvas is just an integer that is being plugged into canvas urls (for example, it is 7 in my test configuration). Is canvas supposed to provide a way to use the long hash that comes from this context to get access to course content?

This is what it is being used for in CanvasLms.php

$lmsContentTypeUrls = [
    'announcement' => "courses/{$lmsCourseId}/discussion_topics/{$lmsContentId}",
    'assignment' => "courses/{$lmsCourseId}/assignments/{$lmsContentId}",
    'discussion_topic' => "courses/{$lmsCourseId}/discussion_topics/{$lmsContentId}",
    'file' => "courses/{$lmsCourseId}/files/{$lmsContentId}",
    'module' => "courses/{$lmsCourseId}/modules/{$lmsContentId}",
    'page' => "courses/{$lmsCourseId}/pages/{$lmsContentId}",
    'quiz' => "courses/{$lmsCourseId}/quizzes/{$lmsContentId}",
    'syllabus' => "courses/{$lmsCourseId}?include[]=syllabus_body",
];

Unfortunately, I'm not familiar with the LTI spec at all.

[ottenhoff]

Is canvas supposed to provide a way to use the long hash that comes from this context to get access to course content?

I don't know. The question I would ask first is what other data is coming from Canvas during the LTI post? Maybe what you need is already coming over. If not, then you need the custom code to overwrote the lms_course_id

[bagofarms]

@rob-3 The lms_course_id is being sent over from Canvas because we're asking for it in the "Custom Fields" of the LTI Developer Key. We're asking for:

lms_id=canvas
lms_user_id=$Canvas.user.id
lms_course_id=$Canvas.course.id
lms_api_domain=$Canvas.api.domain

Otherwise known as Variable Substitutions

The code is breaking because it's assuming that the lms_course_id will be the id of the course in the lms, and not a long hash.

As I understand it right now, I see two options:

1. This code should be moved back to the D2L interface and implemented in any other LMS interface that uses it, or
2. The session variable it gets stored to should be something like lti_context_* instead so that it does not interfere with the lms_course_id. After all, there is a separate context-id variable substitution we can use to get that value from Canvas, so it makes sense to store it in lti_context_id.

I'm leaning heavily toward option 2, but I don't know much about how D2L or Sakai handle course ids.

[rob-3]

This makes sense to me. I'll change the prefix for these variables to lti_context_ to avoid the naming collisions.

[rob-3]

My only concern is that this will break D2L or other LMSes that maybe do use the same variable in both places

[rob-3]

I want to rethink LtiController a bit before moving forward with this change. We have code like

if ('canvas' === $lms) {
    $output["custom_fields"] = [
        "lms_id" => 'canvas',
        "lms_user_id" => "\$Canvas.user.id",
        "lms_course_id" => "\$Canvas.course.id",
        "lms_account_id" => "\$Canvas.account.id",
        "lms_api_domain" => "\$Canvas.api.domain",
    ];
}
else {
    // D2L custom fields
}

that doesn't seem like the right way to be doing this. LMS-specific code and these concerns are better handled outside of the controllers.

[bagofarms]

Removing it from 3.3.0 because I don't have time to properly dive into it, and I don't want to delay 3.3.0 any further.

[dmols]

Sorry to bother, Rob! I'm noticing this PR is quite old and it's still in the draft stage so we're going to close it.