Check for LMS ID first when authenticating from LMS

[Alex-Jordan]

This is an attempt to deal with situations where a user in the LMS undergoes a name change. So now when then try to authenticate form the LMS, we first look at their LMS ID and see if there is a WW user with that same lis_source_did.

For now I have only made changes for LTI 1.3. But it turns out I can't test this at all for LTI 1.3 because the WW server I have for testing LTI with our D2L test server is not set up for https. So I can only test LTI 1.1 stuff.

Any feedback on this code so far?

[drgrice1]

This really shouldn't be called the lis_source_did. That isn't what it is. It is the sub claim which is a stable locally unique identifier for the user in the LMS. See https://www.imsglobal.org/spec/lti/v1p3#message-claims. Nowhere in the LTI 1.3 specification do they even refer to an lis_sourced_id. That is a LTI 1.1 thing.

[drgrice1]

I think that the $type_of_source should be "database user" in this case. Note that this is only used for the debugging message on line 217 below, so it can contain spaces and be informative. The type of source means the source of the webwork2 user_id. That is from the user_id column of the user table in the database in this case, and not the misnamed lis_source_did column.

[drgrice1]

Okay, looking at this closer I also see that $user_id_source = $c->stash->{lti_lms_user_id} is not good. The values of $user_id_source and $type_of_source need to be set to something that will work well in the sentence "User id is |$self->{user_id}| (obtained from $user_id_source which was $type_of_source)\n". That is all that those variables are used for (other than the usage of $user_id_source in the case that it is equal to "email").

To be honest, I don't think anything will work in that sentence well in this case. So instead I think the $type_of_source should just be set to "database user" in this case, and "$user_id_source which was ..." in the other two cases. Then the debugging message changed to "User id is |$self->{user_id}| (obtained from $type_of_source)\n";

[Alex-Jordan]

So, what I was doing here was the best I could think of for User id is ____ (obtained from ____ which was ____). Limiting to that structure, User id is ____ (obtained from lis_source_did which was xxx-yyy-zzz) seemed OK, assuming someone reading the log would know what that means. But having a different sentence altogether is better.

[drgrice1]

The $c->stash->{lti_lms_user_id} can never be set at this point. It isn't set until line 236 later in this same method. You will need to use $claims->{sub} at this point instead.

[drgrice1]

Why is this set? It will never be needed.

[Alex-Jordan]

This was my attempt at recording the LMS ID as a new user is created. I didn't track the code, but I assumed the previous block was setting similar things like first_name, and that be setting this here it would trickle down to the lis_source_did column being filled out for the new user. I'm not surprised if that's wrong, but I couldn't test.

[drgrice1]

I added a pull request to this branch with some suggested changes. I tested it with Moodle, and it works. I created a user, and signed in. Then I changed the user's email address in the LMS (I have $LTI{v1p3}{preferred_source_of_username} = 'email'), and then signed in again, and the user was signed in with the original webwork2 user. No new user was created.

[drgrice1]

I have now also successfully tested my suggested changes with Canvas.

[Alex-Jordan]

Thanks for looking at this, and I'll merge your PR soon. I hope to get this in my PCC production server for our winter term. For now at PCC, doing it with LTI 1.3 is enough.

[Alex-Jordan]

I merged your PR, thanks again. I may take a stab at the parallel development for LTI 1.1.

[drgrice1]

Unfortunately, I don't think this is going to work for LTI 1.1. At least not without adding a new column to the user table in the database.

The thing is that the lis_sourced_id is not an LMS user identifier. In fact it is a resource identifier for the external tool link (or grade item really) in the LMS. That is saved in the lis_source_did column for the user in the case that grade pass back mode is "course", but it would not be reliable to use it for this even in that case. It simply isn't a user identifier. Looking at what Moodle and Canvas send for this in particular it would be a bad idea to use. Moodle sends something like {"data":{"instanceid":"62","userid":"4","typeid":"1","launchid":69624527},"hash":"c0284102b6441f01028d784af97c9695b61e181852b18d0ceb155f1b767dfc9e"} for this. Canvas sends something like 2712-36676-392282-120885-1b74b9b323c2b11a439870b54f6f3ebac1bc6ceb.

Looking at the LTI 1.1 specification (see section 3 on basic launch data in https://www.imsglobal.org/specs/ltiv1p1p1/implementation-guide) and what Moodle and Canvas send, it seems the only reliable thing to use would be the user_id parameter. In fact that is literally the only thing in common between what Moodle and Canvas send that could possibly be used for this. The specification doesn't technically require that parameter, it is only recommended, but both Moodle and Canvas send it, and it is in fact the same thing that they send for the sub claim for LTI 1.3. The problem is that you can't save that in the lis_source_did column of the user table in the database because that would conflict with the value that is needed there for grade pass back in the case that the grade pass back mode is "course". I suppose if the grade pass back mode is unset or is "homework" you could, but I don't think that would be a good idea. Certainly the capability of webwork2 to support this shouldn't depend on the grade pass back mode.

[Alex-Jordan]

OK, then I guess it's time to lift the "draft" status from this?

[drgrice1]

I think it is.

[Alex-Jordan]

I applied this to my production server and ran the following test with D2L. Working directly with the database, for one WW course, I removed my user's lis_source_did, and I set a different user to have my lis_source_did. Then I entered the WW course using an LTI link from D2L, and it entered me as that other user. In other words, the test was a success.

I have not tested the part of this that records the lis_source_did when a new user is created.

[drgrice1]

Now that this is not a draft anymore, I will add my approval. I have already tested it with Moodle and Canvas as stated previously.

[somiaj]

I can test this, but it looks good so I can throw my approval. I'm assuming you both have also tested that creation of new users still works with this?

@drgrice1 you comment that this won't be reliable for LTI 1.1, because what is being saved is not a user id but more a resource id. But how often would this actually change for a single user? And I assume any change could never match a different existing user? So could we use it to try and catch name changes in the case the LMS doesn't update this resource id for the updated user?

[drgrice1]

If you are using grade pass back mode, then it will change for every set you log in with, and there is no guarantee it won't also work for another user. It simply will not work, and trying to use it for this would be completely wrong in any case.

Also, yes I have tested it with user creation.

[Alex-Jordan]

I just tested with account creation too (with D2L). It worked. And checking the database, the account creation was enough to get the lis_source_did in there without any attempt at grade passback.

[somiaj]

Oh nevermind, I see what is going on now, didn't fully understand your previous statement.

One thought if we wanted to try to support this in 1.1, could we just make the lis_source_did contain both the user_id and the resource_id, something like user_id;lis_source_did. It would make things more complicated but could allow this change without another column in the DB. Though maybe another column would be better for this instead of just stashing the 1.3 user_id in the 1.1 resource_id column, then we could support both 1.3 and 1.1 with this feature.

[somiaj]

Anyways here is my approval, I was just trying to think of ways to support 1.1 because I'm a dinosaur who doesn't want to deal with the headache of moving to 1.3 here. Though I guess 1.1 probably going to be dropped in the future at some point anyways by the LMSes?

[Alex-Jordan]

My opinion: it would be OK to start treating LTI 1.1 as basically frozen.

[dlglin]

My opinion: it would be OK to start treating LTI 1.1 as basically frozen.

LTI 1.1 is not officially deprecated, but hasn't been supported since June 30, 2021: https://www.1edtech.org/lti-security-announcement-and-deprecation-schedule

I'm fine with not adding new functionality to LTI 1.1, especially since some new features might not be possible in 1.1.