From 58bfb76afe13480e8c1bfbda12648d42b400b086 Mon Sep 17 00:00:00 2001 From: catherine Date: Mon, 25 May 2026 12:59:45 -0700 Subject: [PATCH 1/3] Link LTI user to existing record in user table if exists by also checking student number Users created via csv import did not populate the global_unique_identifier column in user table. If user logs in via LTI, lti_user unable to link to existing record in user table. It's also unable to create a new user record due to duplicate student number. --- compair/api/classlist.py | 1 + compair/api/login.py | 4 +++ compair/models/lti_models/lti_user.py | 7 ++++ compair/tests/test_models.py | 50 ++++++++++++++++++++++++++- 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/compair/api/classlist.py b/compair/api/classlist.py index 14015f5a9..7bc2d3d52 100644 --- a/compair/api/classlist.py +++ b/compair/api/classlist.py @@ -191,6 +191,7 @@ def import_users(import_type, course, users): ) if import_type == ThirdPartyType.cas.value or import_type == ThirdPartyType.saml.value: # CAS/SAML login + u.global_unique_identifier = username u.third_party_auths.append(ThirdPartyUser( unique_identifier=username, third_party_type=ThirdPartyType(import_type) diff --git a/compair/api/login.py b/compair/api/login.py index 942af45b4..624da6f11 100644 --- a/compair/api/login.py +++ b/compair/api/login.py @@ -279,6 +279,10 @@ def saml_auth(): thirdpartyuser.generate_or_link_user_account() db.session.commit() + if thirdpartyuser.user and thirdpartyuser.user.global_unique_identifier is None: + thirdpartyuser.user.global_unique_identifier = unique_identifier + db.session.commit() + authenticate(thirdpartyuser.user, login_method=thirdpartyuser.third_party_type.value) thirdpartyuser.params = attributes diff --git a/compair/models/lti_models/lti_user.py b/compair/models/lti_models/lti_user.py index 558b48518..6d33b1d9f 100644 --- a/compair/models/lti_models/lti_user.py +++ b/compair/models/lti_models/lti_user.py @@ -49,6 +49,13 @@ def generate_or_link_user_account(self): .filter_by(global_unique_identifier=self.global_unique_identifier) \ .one_or_none() + if not self.compair_user and self.student_number: + self.compair_user = User.query \ + .filter_by(student_number=self.student_number) \ + .one_or_none() + if self.compair_user: + self.compair_user.global_unique_identifier = self.global_unique_identifier + if not self.compair_user: self.compair_user = User( username=None, diff --git a/compair/tests/test_models.py b/compair/tests/test_models.py index 6ea8a4f37..029aa438e 100644 --- a/compair/tests/test_models.py +++ b/compair/tests/test_models.py @@ -7,13 +7,16 @@ from compair import db from compair.models import User, Comparison, AnswerScore, \ - AnswerCriterionScore, LTIOutcome, SystemRole + AnswerCriterionScore, LTIOutcome, SystemRole, ThirdPartyUser +from compair.models.lti_models import LTIUser +from compair.models import ThirdPartyType from compair.models.comparison import update_answer_scores, \ update_answer_criteria_scores from compair.tests.test_compair import ComPAIRTestCase from compair.algorithms import ComparisonPair, ComparisonWinner from compair.algorithms.score import calculate_score from data.fixtures.test_data import TestFixture, LTITestData +from data.factories import LTIConsumerFactory, UserFactory class TestUsersModel(ComPAIRTestCase): user = User() @@ -115,6 +118,51 @@ def test_update_answer_criteria_scores(self): scores = update_answer_criteria_scores([score], 1, criterion_comparison_results) self.assertEqual(len(scores), 4) +class TestLTIUserGenerateOrLinkAccount(ComPAIRTestCase): + def setUp(self): + super(TestLTIUserGenerateOrLinkAccount, self).setUp() + self.lti_consumer = LTIConsumerFactory( + global_unique_identifier_param='custom_puid', + student_number_param='custom_student_number' + ) + db.session.commit() + + def test_links_existing_saml_user_by_student_number_when_global_unique_identifier_missing(self): + # user created via SAML - has student number but no global_unique_identifier + existing_user = UserFactory( + system_role=SystemRole.student, + student_number='12345678', + global_unique_identifier=None, + username=None, + password=None + ) + ThirdPartyUser( + unique_identifier='saml_identifier', + third_party_type=ThirdPartyType.saml, + user=existing_user + ) + db.session.commit() + + user_count_before = User.query.count() + + lti_user = LTIUser( + lti_consumer=self.lti_consumer, + user_id='canvas_user_123', + system_role=SystemRole.student, + global_unique_identifier='puid_abc', + student_number='12345678' + ) + db.session.add(lti_user) + lti_user.generate_or_link_user_account() + + # no new user should be created + self.assertEqual(User.query.count(), user_count_before) + # lti_user should be linked to the existing user + self.assertEqual(lti_user.compair_user_id, existing_user.id) + # global_unique_identifier should be backfilled on the existing user + self.assertEqual(existing_user.global_unique_identifier, 'puid_abc') + + class TestLTIOutcome(ComPAIRTestCase): def setUp(self): From 641234d164059c21f686d2f1d199fd195da36e95 Mon Sep 17 00:00:00 2001 From: catherine Date: Mon, 25 May 2026 13:27:58 -0700 Subject: [PATCH 2/3] Remove backlinking global_unique_identifier on third party user login --- compair/api/login.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/compair/api/login.py b/compair/api/login.py index 624da6f11..942af45b4 100644 --- a/compair/api/login.py +++ b/compair/api/login.py @@ -279,10 +279,6 @@ def saml_auth(): thirdpartyuser.generate_or_link_user_account() db.session.commit() - if thirdpartyuser.user and thirdpartyuser.user.global_unique_identifier is None: - thirdpartyuser.user.global_unique_identifier = unique_identifier - db.session.commit() - authenticate(thirdpartyuser.user, login_method=thirdpartyuser.third_party_type.value) thirdpartyuser.params = attributes From 27bcf573f3d1b10faa3508250932cf5f44c459ad Mon Sep 17 00:00:00 2001 From: catherine Date: Mon, 25 May 2026 15:11:29 -0700 Subject: [PATCH 3/3] Guard global_unique_identifier overwrites and backfill on re-import --- compair/api/classlist.py | 3 +++ compair/models/lti_models/lti_user.py | 2 +- compair/tests/test_models.py | 35 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/compair/api/classlist.py b/compair/api/classlist.py index 7bc2d3d52..55797f44f 100644 --- a/compair/api/classlist.py +++ b/compair/api/classlist.py @@ -180,6 +180,9 @@ def import_users(import_type, course, users): # overwrite password if user has not logged in yet if u.last_online == None and not password in [None, '*']: set_user_passwords.append((u, password)) + if (import_type == ThirdPartyType.cas.value or import_type == ThirdPartyType.saml.value) \ + and u.global_unique_identifier is None: + u.global_unique_identifier = username else: u = User( username=None, diff --git a/compair/models/lti_models/lti_user.py b/compair/models/lti_models/lti_user.py index 6d33b1d9f..4259d27c3 100644 --- a/compair/models/lti_models/lti_user.py +++ b/compair/models/lti_models/lti_user.py @@ -53,7 +53,7 @@ def generate_or_link_user_account(self): self.compair_user = User.query \ .filter_by(student_number=self.student_number) \ .one_or_none() - if self.compair_user: + if self.compair_user and self.compair_user.global_unique_identifier is None: self.compair_user.global_unique_identifier = self.global_unique_identifier if not self.compair_user: diff --git a/compair/tests/test_models.py b/compair/tests/test_models.py index 029aa438e..9e4d39f83 100644 --- a/compair/tests/test_models.py +++ b/compair/tests/test_models.py @@ -162,6 +162,41 @@ def test_links_existing_saml_user_by_student_number_when_global_unique_identifie # global_unique_identifier should be backfilled on the existing user self.assertEqual(existing_user.global_unique_identifier, 'puid_abc') + def test_does_not_overwrite_existing_global_unique_identifier_when_linking_by_student_number(self): + # user already has a global_unique_identifier set from a prior SAML/CAS login + existing_user = UserFactory( + system_role=SystemRole.student, + student_number='12345678', + global_unique_identifier='existing_puid', + username=None, + password=None + ) + ThirdPartyUser( + unique_identifier='saml_identifier', + third_party_type=ThirdPartyType.saml, + user=existing_user + ) + db.session.commit() + + user_count_before = User.query.count() + + lti_user = LTIUser( + lti_consumer=self.lti_consumer, + user_id='canvas_user_456', + system_role=SystemRole.student, + global_unique_identifier='different_puid', + student_number='12345678' + ) + db.session.add(lti_user) + lti_user.generate_or_link_user_account() + + # no new user should be created + self.assertEqual(User.query.count(), user_count_before) + # lti_user should be linked to the existing user + self.assertEqual(lti_user.compair_user_id, existing_user.id) + # existing global_unique_identifier must not be overwritten + self.assertEqual(existing_user.global_unique_identifier, 'existing_puid') + class TestLTIOutcome(ComPAIRTestCase):