From 1a6efce501844f56e87608b18afb5da39814f734 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 1 Sep 2025 11:45:07 -0400 Subject: [PATCH 1/6] Implement SSO attributes de-duplication for email and names --- api/institutions/authentication.py | 23 ++++++++++++++++++++++- framework/auth/__init__.py | 21 ++++++++++++++++++--- framework/auth/exceptions.py | 4 ++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/api/institutions/authentication.py b/api/institutions/authentication.py index a5588c2b034..c7e8aab4303 100644 --- a/api/institutions/authentication.py +++ b/api/institutions/authentication.py @@ -16,7 +16,8 @@ from api.waffle.utils import flag_is_active from framework import sentry -from framework.auth import get_or_create_institutional_user +from framework.auth import get_or_create_institutional_user, deduplicate_sso_attributes +from framework.auth.exceptions import MultipleSSOEmailError from osf import features from osf.exceptions import InstitutionAffiliationStateError @@ -223,8 +224,13 @@ def authenticate(self, request): f'sso_email={sso_email}, sso_identity={sso_identity}]', ) + # Deduplicate full name first if it is provided + if fullname: + fullname = deduplicate_sso_attributes(institution, sso_identity, 'fullname', fullname) # Use given name and family name to build full name if it is not provided if given_name and family_name and not fullname: + given_name = deduplicate_sso_attributes(institution, sso_identity, 'given_name', given_name) + family_name = deduplicate_sso_attributes(institution, sso_identity, 'family_name', family_name) fullname = given_name + ' ' + family_name # Non-empty full name is required. Fail the auth and inform sentry if not provided. @@ -235,6 +241,21 @@ def authenticate(self, request): sentry.log_message(message) raise PermissionDenied(detail='InstitutionSsoMissingUserNames') + # Deduplicate sso email, currently we only handle duplicate sso email instead of multiple sso email + try: + sso_email = deduplicate_sso_attributes( + institution, + sso_identity, + 'sso_email', + sso_email, + ignore_errors=False, + ) + except MultipleSSOEmailError: + message = f'Institution SSO Error: multiple SSO email [sso_email={sso_email}, sso_identity={sso_identity}, institution={institution._id}]' + sentry.log_message(message) + logger.error(message) + # TODO: this requires a CAS hotfix to handle `detail='InstitutionMultipleSSOEmails'` + raise PermissionDenied(detail='InstitutionMultipleSSOEmails') # Attempt to find an existing user that matches the email(s) provided via SSO. Create a new one if not found. # If a user is found, it is possible that the user is inactive (e.g. unclaimed, disabled, unconfirmed, etc.). # If a new user is created, the user object is confirmed but not registered (i.e. inactive until registered). diff --git a/framework/auth/__init__.py b/framework/auth/__init__.py index 9df43069936..e840ac8e66e 100644 --- a/framework/auth/__init__.py +++ b/framework/auth/__init__.py @@ -2,11 +2,11 @@ from django.utils import timezone -from framework import bcrypt +from framework import bcrypt, sentry from framework.auth import signals from framework.auth.core import Auth from framework.auth.core import get_user, generate_verification_key -from framework.auth.exceptions import DuplicateEmailError +from framework.auth.exceptions import DuplicateEmailError, MultipleSSOEmailError from framework.auth.tasks import update_user_from_activity from framework.auth.utils import LogLevel, print_cas_log from framework.celery_tasks.handlers import enqueue_task @@ -154,11 +154,26 @@ def get_or_create_institutional_user(fullname, sso_email, sso_identity, primary_ # CASE 5/5: If no user is found, create a confirmed user and return the user and sso identity. # Note: Institution users are created as confirmed with a strong and random password. Users don't need the # password since they sign in via SSO. They can reset their password to enable email/password login. - user = OSFUser.create_confirmed(sso_email, str(uuid.uuid4()), fullname) + # user = OSFUser.create_confirmed(sso_email, str(uuid.uuid4()), fullname) + user = OSFUser.create_confirmed(sso_email, 'abCD12#$', fullname) user.add_system_tag(institution_source_tag(primary_institution._id)) return user, True, None, None, sso_identity +def deduplicate_sso_attributes(institution, sso_identity, attr_name, attr_value, delimiter=';', ignore_errors=True): + if delimiter not in attr_value: + return attr_value + value_set = set(attr_value.split(delimiter)) + if len(value_set) != 1: + message = (f'Multiple values {attr_value} found for SSO attribute {attr_name}: ' + f'[institution_id={institution._id}, sso_identity={sso_identity}]') + if ignore_errors: + sentry.log_message(message) + return attr_value + raise MultipleSSOEmailError(message) + return value_set.pop() + + def get_or_create_user(fullname, address, reset_password=True, is_spam=False): """ Get or create user by fullname and email address. diff --git a/framework/auth/exceptions.py b/framework/auth/exceptions.py index 4d58a551f4e..d13c6925e47 100644 --- a/framework/auth/exceptions.py +++ b/framework/auth/exceptions.py @@ -65,3 +65,7 @@ class MergeConflictError(EmailConfirmTokenError): """Raised if a merge is not possible due to a conflict""" message_short = language.CANNOT_MERGE_ACCOUNTS_SHORT message_long = language.CANNOT_MERGE_ACCOUNTS_LONG + + +class MultipleSSOEmailError(AuthError): + pass From 92bdf23a2b2f9410643630349ac4fbcde3093577 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 1 Sep 2025 16:31:38 -0400 Subject: [PATCH 2/6] Sync API error detail with CAS --- api/institutions/authentication.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/institutions/authentication.py b/api/institutions/authentication.py index c7e8aab4303..bb328b32abe 100644 --- a/api/institutions/authentication.py +++ b/api/institutions/authentication.py @@ -254,8 +254,7 @@ def authenticate(self, request): message = f'Institution SSO Error: multiple SSO email [sso_email={sso_email}, sso_identity={sso_identity}, institution={institution._id}]' sentry.log_message(message) logger.error(message) - # TODO: this requires a CAS hotfix to handle `detail='InstitutionMultipleSSOEmails'` - raise PermissionDenied(detail='InstitutionMultipleSSOEmails') + raise PermissionDenied(detail='InstitutionSsoMultipleEmailsNotSupported') # Attempt to find an existing user that matches the email(s) provided via SSO. Create a new one if not found. # If a user is found, it is possible that the user is inactive (e.g. unclaimed, disabled, unconfirmed, etc.). # If a new user is created, the user object is confirmed but not registered (i.e. inactive until registered). From 72dae64cf3ec71db6e4e503ba0e7f0cf20a6096e Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Fri, 5 Sep 2025 10:11:32 -0400 Subject: [PATCH 3/6] Remove local dedebugging code --- framework/auth/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/framework/auth/__init__.py b/framework/auth/__init__.py index e840ac8e66e..502060bea40 100644 --- a/framework/auth/__init__.py +++ b/framework/auth/__init__.py @@ -154,8 +154,7 @@ def get_or_create_institutional_user(fullname, sso_email, sso_identity, primary_ # CASE 5/5: If no user is found, create a confirmed user and return the user and sso identity. # Note: Institution users are created as confirmed with a strong and random password. Users don't need the # password since they sign in via SSO. They can reset their password to enable email/password login. - # user = OSFUser.create_confirmed(sso_email, str(uuid.uuid4()), fullname) - user = OSFUser.create_confirmed(sso_email, 'abCD12#$', fullname) + user = OSFUser.create_confirmed(sso_email, str(uuid.uuid4()), fullname) user.add_system_tag(institution_source_tag(primary_institution._id)) return user, True, None, None, sso_identity From cbca42951fc5e5dc253553ab1f63b4d6e74070d1 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Fri, 5 Sep 2025 10:31:00 -0400 Subject: [PATCH 4/6] CR response: simplify duduping and add docstring --- api/institutions/authentication.py | 14 ++++---------- framework/auth/__init__.py | 13 ++++++------- framework/auth/exceptions.py | 1 + 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/api/institutions/authentication.py b/api/institutions/authentication.py index bb328b32abe..55686334c90 100644 --- a/api/institutions/authentication.py +++ b/api/institutions/authentication.py @@ -226,11 +226,11 @@ def authenticate(self, request): # Deduplicate full name first if it is provided if fullname: - fullname = deduplicate_sso_attributes(institution, sso_identity, 'fullname', fullname) + fullname = deduplicate_sso_attributes('fullname', fullname) # Use given name and family name to build full name if it is not provided if given_name and family_name and not fullname: - given_name = deduplicate_sso_attributes(institution, sso_identity, 'given_name', given_name) - family_name = deduplicate_sso_attributes(institution, sso_identity, 'family_name', family_name) + given_name = deduplicate_sso_attributes('given_name', given_name) + family_name = deduplicate_sso_attributes('family_name', family_name) fullname = given_name + ' ' + family_name # Non-empty full name is required. Fail the auth and inform sentry if not provided. @@ -243,13 +243,7 @@ def authenticate(self, request): # Deduplicate sso email, currently we only handle duplicate sso email instead of multiple sso email try: - sso_email = deduplicate_sso_attributes( - institution, - sso_identity, - 'sso_email', - sso_email, - ignore_errors=False, - ) + sso_email = deduplicate_sso_attributes('sso_email', sso_email) except MultipleSSOEmailError: message = f'Institution SSO Error: multiple SSO email [sso_email={sso_email}, sso_identity={sso_identity}, institution={institution._id}]' sentry.log_message(message) diff --git a/framework/auth/__init__.py b/framework/auth/__init__.py index 502060bea40..4d449730302 100644 --- a/framework/auth/__init__.py +++ b/framework/auth/__init__.py @@ -159,17 +159,16 @@ def get_or_create_institutional_user(fullname, sso_email, sso_identity, primary_ return user, True, None, None, sso_identity -def deduplicate_sso_attributes(institution, sso_identity, attr_name, attr_value, delimiter=';', ignore_errors=True): +def deduplicate_sso_attributes(attr_name, attr_value, delimiter=';'): if delimiter not in attr_value: return attr_value value_set = set(attr_value.split(delimiter)) if len(value_set) != 1: - message = (f'Multiple values {attr_value} found for SSO attribute {attr_name}: ' - f'[institution_id={institution._id}, sso_identity={sso_identity}]') - if ignore_errors: - sentry.log_message(message) - return attr_value - raise MultipleSSOEmailError(message) + message = f'Multiple values found for SSO attribute: [{attr_name}={attr_value}]' + sentry.log_message(message) + if attr_name == 'sso_email': + raise MultipleSSOEmailError(message) + return attr_value return value_set.pop() diff --git a/framework/auth/exceptions.py b/framework/auth/exceptions.py index d13c6925e47..c1bffe721e7 100644 --- a/framework/auth/exceptions.py +++ b/framework/auth/exceptions.py @@ -68,4 +68,5 @@ class MergeConflictError(EmailConfirmTokenError): class MultipleSSOEmailError(AuthError): + """Raised if institution SSO provides multiple emails which OSF cannot deduplicate.""" pass From ed05e0bfa9e96d805c5acf0cb6ce1572108972bf Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 9 Sep 2025 17:47:01 -0400 Subject: [PATCH 5/6] Deduplicate names seperately before building fullname --- api/institutions/authentication.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/api/institutions/authentication.py b/api/institutions/authentication.py index 55686334c90..6748d58af61 100644 --- a/api/institutions/authentication.py +++ b/api/institutions/authentication.py @@ -224,15 +224,16 @@ def authenticate(self, request): f'sso_email={sso_email}, sso_identity={sso_identity}]', ) - # Deduplicate full name first if it is provided + # Deduplicate names first if it is provided if fullname: fullname = deduplicate_sso_attributes('fullname', fullname) - # Use given name and family name to build full name if it is not provided - if given_name and family_name and not fullname: + if given_name: given_name = deduplicate_sso_attributes('given_name', given_name) + if family_name: family_name = deduplicate_sso_attributes('family_name', family_name) + # Use given name and family name to build full name if it is not provided + if given_name and family_name and not fullname: fullname = given_name + ' ' + family_name - # Non-empty full name is required. Fail the auth and inform sentry if not provided. if not fullname: message = f'Institution SSO Error: missing full name ' \ From 4b8ac5bbc5368235e4a4a532c442b9a040744c27 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 9 Sep 2025 17:49:03 -0400 Subject: [PATCH 6/6] Add unit tests --- .../views/test_institution_auth.py | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/api_tests/institutions/views/test_institution_auth.py b/api_tests/institutions/views/test_institution_auth.py index 670a6ee31b4..11c187fbff6 100644 --- a/api_tests/institutions/views/test_institution_auth.py +++ b/api_tests/institutions/views/test_institution_auth.py @@ -494,6 +494,135 @@ def test_user_external_unconfirmed(self, app, institution, url_auth_institution) assert accepted_terms_of_service == user.accepted_terms_of_service assert not user.has_usable_password() + def test_duplicate_emails_and_names_success_existing_user(self, app, institution, url_auth_institution): + username, fullname, password = 'user_deanseu@user.edu', 'Foo Bar', 'FuAsKeEr' + exiting_user = make_user(username, fullname) + exiting_user.set_password(password) + exiting_user.save() + sso_email = f'{username};{username}' + with capture_signals() as mock_signals: + res = app.post( + url_auth_institution, + make_payload( + institution, + sso_email, + family_name='User;User', + given_name='Fake;Fake', + fullname='Fake User;Fake User', + ) + ) + assert res.status_code == 204 + assert not mock_signals.signals_sent() + user = OSFUser.objects.filter(username=username).first() + assert user + assert user.fullname == fullname + affiliation = user.get_institution_affiliation(institution._id) + assert affiliation.sso_mail == username + assert user.has_usable_password() + assert user.check_password(password) + assert institution in user.get_affiliated_institutions() + + def test_duplicate_emails_and_names_success_new_user(self, app, institution, url_auth_institution): + username, fullname, family_name, given_name = 'user_deansnu@user.edu', 'Fake User', 'User', 'Fake' + sso_email = f'{username};{username}' + with capture_signals() as mock_signals: + res = app.post( + url_auth_institution, + make_payload( + institution, + sso_email, + family_name=f'{family_name};{family_name}', + given_name=f'{given_name};{given_name}', + fullname=f'{fullname};{fullname}', + ) + ) + assert res.status_code == 204 + assert mock_signals.signals_sent() + user = OSFUser.objects.filter(username=username).first() + assert user + assert user.fullname == fullname + assert user.family_name == family_name + assert user.given_name == given_name + affiliation = user.get_institution_affiliation(institution._id) + assert affiliation.sso_mail == username + assert user.has_usable_password() + assert institution in user.get_affiliated_institutions() + + def test_multiple_names_warning_exiting_user(self, app, institution, url_auth_institution): + username, fullname, password = 'user_mnweu@user.edu', 'Foo Bar', 'FuAsKeEr' + exiting_user = make_user(username, fullname) + exiting_user.set_password(password) + exiting_user.save() + with capture_signals() as mock_signals: + res = app.post( + url_auth_institution, + make_payload( + institution, + username, + family_name='User1;User2', + given_name='Fake1;Fake2', + fullname='Fake1 User1;Fake2 User2', + ) + ) + assert res.status_code == 204 + assert not mock_signals.signals_sent() + user = OSFUser.objects.filter(username=username).first() + assert user + assert user.fullname == fullname + affiliation = user.get_institution_affiliation(institution._id) + assert affiliation.sso_mail == username + assert user.has_usable_password() + assert user.check_password(password) + assert institution in user.get_affiliated_institutions() + + def test_multiple_names_warning_new_user(self, app, institution, url_auth_institution): + sso_email, fullname, family_name, given_name = 'user_deansnu@user.edu', 'Fake User;Foo Bar', 'User;Bar', 'Fake;Foo' + with capture_signals() as mock_signals: + res = app.post( + url_auth_institution, + make_payload(institution, sso_email, family_name=family_name, given_name=given_name, fullname=fullname), + ) + assert res.status_code == 204 + assert mock_signals.signals_sent() + user = OSFUser.objects.filter(username=sso_email).first() + assert user + assert user.fullname == fullname + assert user.family_name == family_name + assert user.given_name == given_name + affiliation = user.get_institution_affiliation(institution._id) + assert affiliation.sso_mail == sso_email + assert user.has_usable_password() + assert institution in user.get_affiliated_institutions() + + def test_multiple_emails_failure_existing_user(self, app, institution, url_auth_institution): + username, second_email, fullname, password = 'user_mefeu_a', 'user_mefeu_b@user.edu', 'Fake User', 'FuAsKeEr' + existing_uesr = make_user(username, fullname) + existing_uesr.set_password(password) + existing_uesr.save() + sso_email = f'{username};{second_email}' + with capture_signals() as mock_signals: + res = app.post( + url_auth_institution, + make_payload(institution, sso_email=sso_email, fullname=fullname), + expect_errors=True, + ) + assert res.status_code == 403 + assert res.json['errors'][0]['detail'] == 'InstitutionSsoMultipleEmailsNotSupported' + assert not mock_signals.signals_sent() + + def test_multiple_emails_failure_new_user(self, app, institution, url_auth_institution): + first_email, second_email, family_name, given_name = 'user_mefeu_a', 'user_mefeu_b@user.edu', 'User', 'Fake' + sso_email = f'{first_email};{second_email}' + with capture_signals() as mock_signals: + res = app.post( + url_auth_institution, + make_payload(institution, sso_email, family_name=family_name, given_name=given_name), + expect_errors=True, + ) + assert res.status_code == 403 + assert res.json['errors'][0]['detail'] == 'InstitutionSsoMultipleEmailsNotSupported' + assert not mock_signals.signals_sent() + @pytest.mark.django_db class TestInstitutionStorageRegion: