openwebwork / webwork2

Course management front end for WeBWorK
http://webwork.maa.org/wiki/Main_Page
Other
141 stars 164 forks source link

Separate the LTI 1.3 access token audience and URL. #2342

Closed drgrice1 closed 4 months ago

drgrice1 commented 5 months ago

This adds a new LTI 1.3 authentication parameter named $LTI{v1p3}{AccessTokenAUD}. This is used for the audience (aud) in the signed JWT that is sent when requesting an access token from the LMS. This access token is used for grade passback.

Previously the $LTI{v1p3}{AccessTokenURL} was used for both the audience and the actual URL that the access token request containing the signed JWT is sent to. I suspect that the audience and the URL may not be the same for all LMS's. They are the same for Moodle. These also needed to be the same for testing on my local Canvas instance. However, @Alex-Jordan showed me some information from D2L that indicates these are different there. I suspect these may need to be different for Canvas in production as well.

Alex-Jordan commented 5 months ago

I found a moment to try this. I cannot try it with a development WW server without coordination with the D2L people here. In our D2L test server, there is a deployment set up for our production WW server. OK, so here is what I did with the production WW server:

$debug_lti_parameters = 1; $debug_lti_grade_passback = 1;

as well as grade passback set to 'course' in `simple.conf`
* There is one assignment in the WW course. I have entered the WW course via the D2L test server course. (Single sign-on still works.)
* When I click to submit an answer to a question, I get:

Warning messages

Submitting all grades for user alex.jordan
Failed to obtain access token from LMS:
Bad Request

This is not different from what I had when I last looked into this, probably at this time:
https://webwork.maa.org/moodle/mod/forum/discuss.php?d=8333#p20580
* There is no grade data appearing in the D2L course Grades area.

Do you have any thoughts observations about it? Has `develop` branch had any other LTI related changes compared to `main` that I should include?
Alex-Jordan commented 5 months ago

I see what your updates are doing, and I will try it with them. But one thing is at the moment you still have v1p1 in the conf file instead of v1p3. But more importantly, I already used warn statements today to look at $c->url_for('root')->to_abs->to_string and it was indeed https. So I will try things again tonight, I'm just not optimistic that this is the issue.

drgrice1 commented 5 months ago

Hmm. Then I will remove that commit.

drgrice1 commented 5 months ago

It is odd that $c->url_for('root')->to_abs->to_string in lib/WeBWorK/Authen/LTIAdvantage/SubmitGrade.pm has https but when $c->url_for->to_abs is called on line 459 of lib/WeBWorK/Authen/LTIAdvanced.pm (for LTI 1.1) it does not?

Alex-Jordan commented 5 months ago

I checked, and at this time both are https. I'm sorry, I just reviewed emails from the summer and on or about August 9, we actually set up https for the WeBWorK server whilst grasping at straws. So I guess at that time, $LTI{v1p1}{OverrideSiteProtocolDomain} became unnecessary for me at that time. It is still set, but I suppose it's just replacing https with https now.

That message from the D2L vendor support team that said When launching the tool we see an error in the system log. Redirect Uri must match one of the registered Redirect URL values. The Redirect URI that is being sent is : RedirectUri http://webwork.pcc.edu/webwork2/ltiadvantage/launch came on August 4, before the change. It will be worth asking them to look at this again to see if they still see http or if now they see whatever the next issue is.

Alex-Jordan commented 5 months ago

I'm poring over the $request from after line 157 in LTIAdvantage/SubmitGrade.pm, using a warn statement to look for anything suspect. I only see two things, and don't know what to make of them.

drgrice1 commented 5 months ago

The $request object is a Mojo::Transaction::HTTP. The local_port is the local interface port (see https://docs.mojolicious.org/Mojo/Transaction#local_port). That is the port that the Mojolicious UserAget object for the request gets delegated to. That will be different for each each request. The $request->{req} is a Mojo::Message::Request which derives from a Mojo::Message. The version for that is the HTTP version (see https://docs.mojolicious.org/Mojo/Message#version) which defaults to 1.1. Those are really details of the request sent by webwork2 for the access token.

What we are really interested in is the $response. That is the response from D2L. One thing you could try is to add $c->log->info($c->dumper($response)) after line 177 of LTIAdvantage/SubmitGrade.pm. Although that will be a lot more than is really of interest. More specifically you could do $c->log->info($response->body) That will show the entire body of the response. Although, I suspect that all you will see is something like { "message": "Bad Request", ... } with a few more keys in that object. As I mentioned before, LMS's don't reveal much of what is going wrong here.

drgrice1 commented 5 months ago

Another thing you could try is to comment out line 143 (or maybe instead line 146) of LTIAdvantage/SubmitGrade.pm. That is the oddity between making this work for both Moodle and Canvas. Moodle needs line 146. Canvas needs line 143 (and I think also line 146 -- I don't recall exactly). Fortunately, having line 143 didn't cause a problem for Moodle, but maybe it does for D2L?

I am pretty sure that the problem is something not being quite right in the encoded JWT for D2L. For both Moodle and Canvas I had to delve into their code to figure out what was needed to get this to work. Unfortunately, no one seems to just follow the LTI Advantage specifications exactly (or they all have their own interpretation of those specifications that vary slightly).

Alex-Jordan commented 5 months ago

Ok, I've asked our local D2L team to ask the vendor to look into it again. By now we are probably past that old http issue they previously cited.

On Wed, Feb 28, 2024, 4:22 AM Glenn Rice @.***> wrote:

Another thing you could try is to comment out line 143 (or maybe instead line 146) of LTIAdvantage/SubmitGrade.pm. That is the oddity between making this work for both Moodle and Canvas. Moodle needs line 146. Canvas needs line 143 (and I think also line 146 -- I don't recall exactly). Fortunately, having line 143 didn't cause a problem for Moodle, but maybe it does for D2L?

I am pretty sure that the problem is something not being quite right in the encoded JWT for D2L. For both Moodle and Canvas I had to delve into their code to figure out what was needed to get this to work. Unfortunately, no one seems to just follow the LTI Advantage specifications exactly (or they all have their own interpretation of those specifications that vary slightly).

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-65e56a37bb06377d&q=1&e=c39fc843-b3c1-4b84-9e71-6f0db844aa7e&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2342%23issuecomment-1968870344, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-677a0f80bb82297a&q=1&e=c39fc843-b3c1-4b84-9e71-6f0db844aa7e&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOABWZXGTPSZ3DECPGZTYV4OQBAVCNFSM6AAAAABD3FGMG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRYHA3TAMZUGQ . You are receiving this because you were mentioned.Message ID: @.***>

drgrice1 commented 4 months ago

This now also switches from using the webwork server url for the iss claim to using the client id in the JWT sent when requesting an access token from the LMS for grade passback.

With this change this is confirmed to work for Moodle, Canvas, and D2L.

drgrice1 commented 4 months ago

Should this be made a hotfix?

Alex-Jordan commented 4 months ago

I guess it should be a hotfix. I'm thinking about a school that uses D2L, and is stuck with 2.18 for some reason.

Alex-Jordan commented 4 months ago

I think this is very safe to merge, if @pstaabp, @dlglin, @drdrew42, @somiaj, or @taniwallach want to look over the very small amount of code changes. There are three things here:

This has been tested with D2L, Moodle, and Canvas and it seems that each change is a good change.

drgrice1 commented 4 months ago

Could someone merge this? The hotfix version of this was already merged. So this should be as well.