From 737cf16c31d8d1621f307056eb9e33a16cc835c0 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 10 Sep 2024 13:58:31 +0200 Subject: [PATCH] Store the LTI1.3 user_id --- lms/security.py | 2 +- lms/services/roster.py | 8 +++++++- lms/services/user.py | 11 ++++++----- tests/unit/lms/security_test.py | 4 +++- tests/unit/lms/services/user_test.py | 29 +++++++++++++++------------- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/lms/security.py b/lms/security.py index 36dfa8efbe..0f4de46721 100644 --- a/lms/security.py +++ b/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) diff --git a/lms/services/roster.py b/lms/services/roster.py index 517bdba245..f8e75dc2c6 100644 --- a/lms/services/roster.py +++ b/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]: diff --git a/lms/services/user.py b/lms/services/user.py index 99eeab5f44..9a69aff1d5 100644 --- a/lms/services/user.py +++ b/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, diff --git a/tests/unit/lms/security_test.py b/tests/unit/lms/security_test.py index 92eb84f187..44ee60f903 100644 --- a/tests/unit/lms/security_test.py +++ b/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 ) diff --git a/tests/unit/lms/services/user_test.py b/tests/unit/lms/services/user_test.py index 52d6ad49d5..25eb58c1b4 100644 --- a/tests/unit/lms/services/user_test.py +++ b/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):