Store the LTI1.3 user_id

lms/security.py

@@ -363,7 +363,7 @@ def get_lti_user(request) -> LTIUser | None:
     if lti_user:
         # Make a record of the user for analytics so we can map from the
         # LTI users and the corresponding user in H
-        request.find_service(UserService).upsert_user(lti_user)
+        request.find_service(UserService).upsert_user(lti_user, request.lti_params)
 
         # Attach useful information to sentry in case we get an exception further down the line
         sentry_sdk.set_tag("application_instance_id", lti_user.application_instance_id)

lms/services/roster.py

@@ -182,6 +182,7 @@ def _get_roster_users(self, roster, tool_consumer_instance_guid):
         values = []
         for member in roster:
             lti_user_id = member.get("lti11_legacy_user_id") or member["user_id"]
+            lti_v13_user_id = member["user_id"]
             name = display_name(
                 given_name=member.get("name", ""),
                 family_name=member.get("family_name", ""),
@@ -198,6 +199,7 @@ def _get_roster_users(self, roster, tool_consumer_instance_guid):
                 {
                     "tool_consumer_instance_guid": tool_consumer_instance_guid,
                     "lti_user_id": lti_user_id,
+                    "lti_v13_user_id": lti_v13_user_id,
                     "h_userid": h_userid,
                     "display_name": name,
                 }
@@ -208,7 +210,11 @@ def _get_roster_users(self, roster, tool_consumer_instance_guid):
             LMSUser,
             values=values,
             index_elements=["h_userid"],
-            update_columns=["updated"],
+            update_columns=[
+                "updated",
+                # lti_v13_user_id is not going to change but we want to backfill it for existing users.
+                "lti_v13_user_id",
+            ],
         )
 
     def _get_roster_roles(self, roster) -> list[LTIRole]:

lms/services/user.py

@@ -9,6 +9,7 @@
     AssignmentMembership,
     LMSUser,
     LMSUserApplicationInstance,
+    LTIParams,
     LTIRole,
     LTIUser,
     RoleScope,
@@ -34,7 +35,7 @@ def __init__(self, db, h_authority: str):
         self._db = db
         self._h_authority = h_authority
 
-    def upsert_user(self, lti_user: LTIUser) -> User:
+    def upsert_user(self, lti_user: LTIUser, lti_params: LTIParams) -> User:
         """Store a record of having seen a particular user."""
 
         # Note! - Storing a user in our DB currently has an implication for
@@ -64,11 +65,10 @@ def upsert_user(self, lti_user: LTIUser) -> User:
             # We are only storing emails for teachers now.
             user.email = lti_user.email
 
-        self._upsert_lms_user(user)
-
+        self._upsert_lms_user(user, lti_params)
         return user
 
-    def _upsert_lms_user(self, user: User) -> LMSUser:
+    def _upsert_lms_user(self, user: User, lti_params: LTIParams) -> LMSUser:
         """Upsert LMSUser based on a User object."""
         self._db.flush()  # Make sure User has hit the DB on the current transaction
 
@@ -79,13 +79,14 @@ def _upsert_lms_user(self, user: User) -> LMSUser:
                 {
                     "tool_consumer_instance_guid": user.application_instance.tool_consumer_instance_guid,
                     "lti_user_id": user.user_id,
+                    "lti_v13_user_id": lti_params.v13.get("sub"),
                     "h_userid": user.h_userid,
                     "email": user.email,
                     "display_name": user.display_name,
                 }
             ],
             index_elements=["h_userid"],
-            update_columns=["updated", "display_name", "email"],
+            update_columns=["updated", "display_name", "email", "lti_v13_user_id"],
         ).one()
         bulk_upsert(
             self._db,

tests/unit/lms/security_test.py

@@ -651,7 +651,9 @@ def test_it(self, pyramid_request, user_service, lti_user, sentry_sdk):
         result = get_lti_user(pyramid_request)
 
         assert result == lti_user
-        user_service.upsert_user.assert_called_once_with(result)
+        user_service.upsert_user.assert_called_once_with(
+            result, pyramid_request.lti_params
+        )
         sentry_sdk.set_tag.assert_called_once_with(
             "application_instance_id", lti_user.application_instance_id
         )

tests/unit/lms/services/user_test.py

@@ -13,8 +13,8 @@
 
 class TestUserService:
     @pytest.mark.usefixtures("user_is_instructor")
-    def test_upsert_user(self, service, lti_user, db_session):
-        user = service.upsert_user(lti_user)
+    def test_upsert_user(self, service, lti_user, db_session, pyramid_request):
+        user = service.upsert_user(lti_user, pyramid_request.lti_params)
 
         saved_user = db_session.query(User).order_by(User.id.desc()).first()
         assert saved_user == Any.instance_of(User).with_attrs(
@@ -31,41 +31,43 @@ def test_upsert_user(self, service, lti_user, db_session):
             }
         )
         assert saved_user == user
-        self.assert_lms_user(db_session, user)
+        self.assert_lms_user(db_session, user, pyramid_request.lti_params)
 
     @pytest.mark.usefixtures("user_is_learner")
     def test_upsert_user_doesnt_save_email_for_students(
-        self, service, lti_user, db_session
+        self, service, lti_user, db_session, pyramid_request
     ):
-        service.upsert_user(lti_user)
+        service.upsert_user(lti_user, pyramid_request.lti_params)
 
         saved_user = db_session.query(User).order_by(User.id.desc()).first()
         assert saved_user.roles == lti_user.roles
         assert not saved_user.email
-        self.assert_lms_user(db_session, saved_user)
+        self.assert_lms_user(db_session, saved_user, pyramid_request.lti_params)
 
     @pytest.mark.usefixtures("user")
-    def test_upsert_user_with_an_existing_user(self, service, lti_user, db_session):
-        user = service.upsert_user(lti_user)
+    def test_upsert_user_with_an_existing_user(
+        self, service, lti_user, db_session, pyramid_request
+    ):
+        user = service.upsert_user(lti_user, pyramid_request.lti_params)
 
         saved_user = db_session.get(User, user.id)
         assert saved_user.id == user.id
         assert saved_user.roles == lti_user.roles
         assert user == saved_user
-        self.assert_lms_user(db_session, user)
+        self.assert_lms_user(db_session, saved_user, pyramid_request.lti_params)
 
     @pytest.mark.usefixtures("user")
     def test_upsert_user_doesnt_save_email_for_existing_students(
-        self, service, lti_user, db_session
+        self, service, lti_user, db_session, pyramid_request
     ):
         lti_user.roles = "Student"
 
-        service.upsert_user(lti_user)
+        service.upsert_user(lti_user, pyramid_request.lti_params)
 
         saved_user = db_session.query(User).order_by(User.id.desc()).first()
         assert saved_user.roles == lti_user.roles
         assert not saved_user.email
-        self.assert_lms_user(db_session, saved_user)
+        self.assert_lms_user(db_session, saved_user, pyramid_request.lti_params)
 
     def test_get(self, user, service):
         db_user = service.get(user.application_instance, user.user_id)
@@ -174,7 +176,7 @@ def test_get_users_by_instructor_h_userid(
 
         assert db_session.scalars(query).all() == [student_in_assigment]
 
-    def assert_lms_user(self, db_session, user):
+    def assert_lms_user(self, db_session, user, lti_params):
         """Assert the corresponding LMSUser to user exists in the DB with the same attributes."""
 
         lms_user = db_session.scalars(
@@ -184,6 +186,7 @@ def assert_lms_user(self, db_session, user):
         assert lms_user.display_name == user.display_name
         assert lms_user.email == user.email
         assert lms_user.updated == user.updated
+        assert lms_user.lti_v13_user_id == lti_params.v13.get("sub")
 
     @pytest.fixture
     def course(self, application_instance, db_session):