From 68da146a0bac98dfd15cc8449f5e0157239a9df0 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Fri, 31 Jul 2026 16:21:28 +0200 Subject: [PATCH 1/5] Add revoke endpoint request for ORCID --- admin/templates/users/user.html | 12 +++++++ framework/auth/cas.py | 16 +++++++++ framework/auth/tasks.py | 11 ++++++ .../0045_osfuser_orcid_token_tracking.py | 27 ++++++++++++++ osf/models/user.py | 24 ++++++++++++- osf_tests/test_user.py | 35 +++++++++++++++++-- website/settings/defaults.py | 1 + 7 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 osf/migrations/0045_osfuser_orcid_token_tracking.py diff --git a/admin/templates/users/user.html b/admin/templates/users/user.html index 5cc9244d484..640bf0f9924 100644 --- a/admin/templates/users/user.html +++ b/admin/templates/users/user.html @@ -126,6 +126,18 @@

User: {{ user.username }} ({{user. {% endif %} + + Initial ORCID Authorization + {{ user.date_orcid_initial_authorized }} + + + Last ORCID Authorization + {{ user.date_orcid_last_authorized }} + + + ORCID Token Reserved + {{ user.orcid_token_stored }} + Registered {{ user.is_registered }} [{{ user.date_registered }}] diff --git a/framework/auth/cas.py b/framework/auth/cas.py index 1084739fdc3..a4d576868a9 100644 --- a/framework/auth/cas.py +++ b/framework/auth/cas.py @@ -28,6 +28,7 @@ class CasHTTPError(CasError): def __init__(self, code, message, headers, content): super().__init__(code, message) + self.message = message self.headers = headers self.content = content @@ -97,6 +98,10 @@ def get_auth_token_revocation_url(self): url = furl(self.BASE_URL).add(path=['oauth2', 'revoke']) return url.url + def get_orcid_token_revocation_url(self): + url = furl(self.BASE_URL).add(path=['osf', 'orcid', 'revoke']) + return url.url + def service_validate(self, ticket, service_url): """ Send request to CAS to validate ticket. @@ -198,6 +203,17 @@ def revoke_tokens(self, payload): else: self._handle_error(resp) + def revoke_orcid_token(self, orcid_id): + url = self.get_orcid_token_revocation_url() + headers = { + 'Authorization': f'Bearer {settings.CAS_ORCID_REVOKE_SHARED_SECRET}', + } + resp = requests.post(url, json={'orcid_id': orcid_id}, headers=headers) + if resp.status_code == 204: + return True + else: + self._handle_error(resp) + def parse_auth_header(header): """ diff --git a/framework/auth/tasks.py b/framework/auth/tasks.py index e0083798911..640003fd994 100644 --- a/framework/auth/tasks.py +++ b/framework/auth/tasks.py @@ -2,6 +2,7 @@ import itertools import logging +from django.utils import timezone from lxml import etree import pytz import requests @@ -54,6 +55,16 @@ def update_affiliation_for_orcid_sso_users(user_id, orcid_id): logger.error(error_message) sentry.log_message(error_message) return + + # Best-effort tracking of ORCID (re)authorization for admin visibility and GDPR-delete triage. + # This reflects that OSF observed a completed ORCID login, not a confirmed CAS-side token write. + now = timezone.now() + if not user.date_orcid_initial_authorized: + user.date_orcid_initial_authorized = now + user.date_orcid_last_authorized = now + user.orcid_token_stored = True + user.save() + institution = check_institution_affiliation(orcid_id) if institution: logger.info(f'Eligible institution affiliation has been found for ORCiD SSO user: ' diff --git a/osf/migrations/0045_osfuser_orcid_token_tracking.py b/osf/migrations/0045_osfuser_orcid_token_tracking.py new file mode 100644 index 00000000000..c4519083a1a --- /dev/null +++ b/osf/migrations/0045_osfuser_orcid_token_tracking.py @@ -0,0 +1,27 @@ +from django.db import migrations, models +import osf.utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0044_notification_scheduled'), + ] + + operations = [ + migrations.AddField( + model_name='osfuser', + name='date_orcid_initial_authorized', + field=osf.utils.fields.NonNaiveDateTimeField(blank=True, null=True), + ), + migrations.AddField( + model_name='osfuser', + name='date_orcid_last_authorized', + field=osf.utils.fields.NonNaiveDateTimeField(blank=True, null=True), + ), + migrations.AddField( + model_name='osfuser', + name='orcid_token_stored', + field=models.BooleanField(default=False), + ), + ] diff --git a/osf/models/user.py b/osf/models/user.py index 98461a3cf0a..92dbed60148 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -26,7 +26,7 @@ from django.utils import timezone from framework import sentry -from framework.auth import Auth, signals, utils +from framework.auth import Auth, cas, signals, utils from framework.auth.core import generate_verification_key from framework.auth.exceptions import ( ChangePasswordError, @@ -408,6 +408,12 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi chronos_user_id = models.TextField(null=True, blank=True, db_index=True) + date_orcid_initial_authorized = NonNaiveDateTimeField(null=True, blank=True) + + date_orcid_last_authorized = NonNaiveDateTimeField(null=True, blank=True) + + orcid_token_stored = models.BooleanField(default=False) + allow_indexing = models.BooleanField(null=True, blank=True, default=None) objects = OSFUserManager() @@ -2172,6 +2178,22 @@ def _clear_identifying_information(self): account.profile_url = None account.save() self.external_accounts.clear() + + # Revoke any ORCID OAuth token CAS holds for this user, so OSF no longer shows as a + # trusted party on the user's ORCID account. Best-effort: never blocks GDPR delete. + orcid_ids = self.external_identity.get('ORCID', {}) + if orcid_ids: + for orcid_id in orcid_ids: + try: + cas.get_client().revoke_orcid_token(orcid_id) + except cas.CasHTTPError as e: + logger.error(f'Unable to revoke ORCID token via CAS for user {self._id}, orcid_id={orcid_id}: {e}') + sentry.log_exception(e) + except Exception as e: + logger.error(f'Unexpected error revoking ORCID token via CAS for user {self._id}, orcid_id={orcid_id}: {e}') + sentry.log_exception(e) + self.orcid_token_stored = False + self.external_identity = {} self.deleted = timezone.now() diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 9d2e8b12628..41de62fdae2 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -2224,11 +2224,15 @@ def test_gdpr_delete_triggers_share_update_for_public_shared_preprints( assert mock_update_search.called - def test_can_gdpr_delete(self, user): + @mock.patch('framework.auth.cas.CasClient.revoke_orcid_token') + def test_can_gdpr_delete(self, mock_revoke_orcid_token, user): + user.external_identity = {'ORCID': {'fake-orcid-id': 'VERIFIED'}} + user.orcid_token_stored = True + user.save() + user.social = ['fake social'] user.schools = ['fake schools'] user.jobs = ['fake jobs'] - user.external_identity = ['fake external identity'] user.external_accounts.add(ExternalAccountFactory()) user.gdpr_delete() @@ -2243,6 +2247,33 @@ def test_can_gdpr_delete(self, user): assert not user.external_accounts.exists() assert user.is_disabled assert user.deleted is not None + mock_revoke_orcid_token.assert_called_once_with('fake-orcid-id') + assert user.orcid_token_stored is False + + @mock.patch('framework.auth.cas.CasClient.revoke_orcid_token') + def test_gdpr_delete_no_orcid_no_cas_call(self, mock_revoke_orcid_token, user): + assert user.external_identity == {} + + user.gdpr_delete() + + mock_revoke_orcid_token.assert_not_called() + + @mock.patch('framework.auth.cas.CasClient.revoke_orcid_token') + def test_gdpr_delete_orcid_revoke_failure_does_not_block_delete(self, mock_revoke_orcid_token, user): + from framework.auth import cas + mock_revoke_orcid_token.side_effect = cas.CasHTTPError( + code=400, message='Bad Request', headers={}, content=b'', + ) + user.external_identity = {'ORCID': {'fake-orcid-id': 'VERIFIED'}} + user.orcid_token_stored = True + user.save() + + user.gdpr_delete() + + mock_revoke_orcid_token.assert_called_once_with('fake-orcid-id') + assert user.external_identity == {} + assert user.orcid_token_stored is False + assert user.deleted is not None def test_can_gdpr_delete_personal_nodes(self, user): diff --git a/website/settings/defaults.py b/website/settings/defaults.py index c247e2e24db..a23f036624c 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -393,6 +393,7 @@ def parent_dir(path): SPAM_SUBMIT_TASK_HARD_TIME_LIMIT = 90 CAS_SERVER_URL = 'http://localhost:8080' +CAS_ORCID_REVOKE_SHARED_SECRET = os.environ.get('CAS_ORCID_REVOKE_SHARED_SECRET', 'changeme') MFR_SERVER_URL = 'http://localhost:7778' ###### ARCHIVER ########### From 4c482224e146ef565d809cd96cdc93f128fa5fbd Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Thu, 27 Aug 2026 20:33:41 +0200 Subject: [PATCH 2/5] Added external_identity_access_token field and func to save it --- admin/templates/users/user.html | 12 --- framework/auth/cas.py | 36 ++++++++ framework/auth/tasks.py | 10 --- .../0045_osfuser_orcid_token_tracking.py | 27 ------ .../0053_osfuser_orcid_token_tracking.py | 18 ++++ osf/models/user.py | 84 ++++++++++++++----- osf_tests/test_merging_users.py | 25 ++++++ osf_tests/test_user.py | 50 ++++++----- tests/test_cas_authentication.py | 82 ++++++++++++++++++ 9 files changed, 250 insertions(+), 94 deletions(-) delete mode 100644 osf/migrations/0045_osfuser_orcid_token_tracking.py create mode 100644 osf/migrations/0053_osfuser_orcid_token_tracking.py diff --git a/admin/templates/users/user.html b/admin/templates/users/user.html index 640bf0f9924..5cc9244d484 100644 --- a/admin/templates/users/user.html +++ b/admin/templates/users/user.html @@ -126,18 +126,6 @@

User: {{ user.username }} ({{user. {% endif %} - - Initial ORCID Authorization - {{ user.date_orcid_initial_authorized }} - - - Last ORCID Authorization - {{ user.date_orcid_last_authorized }} - - - ORCID Token Reserved - {{ user.orcid_token_stored }} - Registered {{ user.is_registered }} [{{ user.date_registered }}] diff --git a/framework/auth/cas.py b/framework/auth/cas.py index a4d576868a9..6b41d59f188 100644 --- a/framework/auth/cas.py +++ b/framework/auth/cas.py @@ -8,6 +8,9 @@ from lxml import etree import requests +import logging + +from framework import sentry from framework.auth import authenticate, external_first_login_authenticate from framework.auth.core import get_user, generate_verification_key from framework.auth.utils import print_cas_log, LogLevel @@ -269,6 +272,24 @@ def get_profile_url(): return get_client().get_profile_url() +def save_orcid_access_and_refresh_token_to_user(user, orcid_id: str, access_token: str, refresh_token: str): + sentry.log_message( + f'CAS response ORCID attributes: user=[{user._id}], orcidId=[{orcid_id}], ' + f'orcidAccessToken=[{"present" if access_token else "missing"}]', + level=logging.INFO, + ) + if orcid_id and access_token: + provider = settings.EXTERNAL_IDENTITY_PROFILE['OrcidProfile'] + user.external_identity_access_token.setdefault(provider, {})[orcid_id] = { + 'access_token': access_token, + 'refresh_token': refresh_token, + } + sentry.log_message( + f'ORCID token stored on external_identity_access_token: user=[{user._id}], ' + f'provider_id=[{orcid_id}], access_token=[{access_token if access_token else "missing"}]' + f'refresh_token=[{refresh_token if refresh_token else "missing"}]', + level=logging.INFO, + ) def make_response_from_ticket(ticket, service_url): """ @@ -291,6 +312,8 @@ def make_response_from_ticket(ticket, service_url): user_updates = {} # serialize updates to user to be applied async # user found and authenticated if user and action == 'authenticate': + access_token = cas_resp.attributes.get('orcidAccessToken', None) + refresh_token = cas_resp.attributes.get('orcidRefreshToken', None) print_cas_log( f'CAS response - authenticating user: user=[{user._id}], ' f'external=[{external_credential}], action=[{action}]', @@ -306,6 +329,13 @@ def make_response_from_ticket(ticket, service_url): user_updates['accepted_terms_of_service'] = timezone.now() print_cas_log(f'CAS TOS consent checked: {user.guids.first()._id}, {user.username}', LogLevel.INFO) # if we successfully authenticate and a verification key is present, invalidate it + if external_credential and access_token and refresh_token: + save_orcid_access_and_refresh_token_to_user( + user, + external_credential['id'], + access_token, + refresh_token, + ) if user.verification_key: user_updates['verification_key'] = None @@ -314,6 +344,12 @@ def make_response_from_ticket(ticket, service_url): # current CAS session created by external login must be cleared first before authentication if external_credential: user.verification_key = generate_verification_key() + save_orcid_access_and_refresh_token_to_user( + user, + external_credential['id'], + access_token, + refresh_token, + ) user.save() print_cas_log( f'CAS response - redirect existing external IdP login to verification key login: user=[{user._id}]', diff --git a/framework/auth/tasks.py b/framework/auth/tasks.py index 640003fd994..fb8a0959ff5 100644 --- a/framework/auth/tasks.py +++ b/framework/auth/tasks.py @@ -2,7 +2,6 @@ import itertools import logging -from django.utils import timezone from lxml import etree import pytz import requests @@ -56,15 +55,6 @@ def update_affiliation_for_orcid_sso_users(user_id, orcid_id): sentry.log_message(error_message) return - # Best-effort tracking of ORCID (re)authorization for admin visibility and GDPR-delete triage. - # This reflects that OSF observed a completed ORCID login, not a confirmed CAS-side token write. - now = timezone.now() - if not user.date_orcid_initial_authorized: - user.date_orcid_initial_authorized = now - user.date_orcid_last_authorized = now - user.orcid_token_stored = True - user.save() - institution = check_institution_affiliation(orcid_id) if institution: logger.info(f'Eligible institution affiliation has been found for ORCiD SSO user: ' diff --git a/osf/migrations/0045_osfuser_orcid_token_tracking.py b/osf/migrations/0045_osfuser_orcid_token_tracking.py deleted file mode 100644 index c4519083a1a..00000000000 --- a/osf/migrations/0045_osfuser_orcid_token_tracking.py +++ /dev/null @@ -1,27 +0,0 @@ -from django.db import migrations, models -import osf.utils.fields - - -class Migration(migrations.Migration): - - dependencies = [ - ('osf', '0044_notification_scheduled'), - ] - - operations = [ - migrations.AddField( - model_name='osfuser', - name='date_orcid_initial_authorized', - field=osf.utils.fields.NonNaiveDateTimeField(blank=True, null=True), - ), - migrations.AddField( - model_name='osfuser', - name='date_orcid_last_authorized', - field=osf.utils.fields.NonNaiveDateTimeField(blank=True, null=True), - ), - migrations.AddField( - model_name='osfuser', - name='orcid_token_stored', - field=models.BooleanField(default=False), - ), - ] diff --git a/osf/migrations/0053_osfuser_orcid_token_tracking.py b/osf/migrations/0053_osfuser_orcid_token_tracking.py new file mode 100644 index 00000000000..4582ac4b891 --- /dev/null +++ b/osf/migrations/0053_osfuser_orcid_token_tracking.py @@ -0,0 +1,18 @@ +from django.db import migrations +import osf.utils.datetime_aware_jsonfield +import osf.utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0052_downloadevent_download_channel'), + ] + + operations = [ + migrations.AddField( + model_name='osfuser', + name='external_identity_access_token', + field=osf.utils.datetime_aware_jsonfield.DateTimeAwareJSONField(blank=True, default=dict, encoder=osf.utils.datetime_aware_jsonfield.DateTimeAwareJSONEncoder), + ), + ] diff --git a/osf/models/user.py b/osf/models/user.py index 92dbed60148..1f908975b28 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -12,6 +12,7 @@ # OSF imports import itsdangerous import pytz +import requests from dirtyfields import DirtyFieldsMixin from django.conf import settings @@ -26,7 +27,7 @@ from django.utils import timezone from framework import sentry -from framework.auth import Auth, cas, signals, utils +from framework.auth import Auth, signals, utils from framework.auth.core import generate_verification_key from framework.auth.exceptions import ( ChangePasswordError, @@ -319,6 +320,17 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi # }, # ... # } + external_identity_access_token = DateTimeAwareJSONField(default=dict, blank=True) + # Format: { + # : { + # : { + # "access_token" : , + # "refresh_token" : , + # } + # ... + # }, + # ... + # } # Employment history jobs = DateTimeAwareJSONField(default=list, blank=True, validators=[validate_history_item]) @@ -408,12 +420,6 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi chronos_user_id = models.TextField(null=True, blank=True, db_index=True) - date_orcid_initial_authorized = NonNaiveDateTimeField(null=True, blank=True) - - date_orcid_last_authorized = NonNaiveDateTimeField(null=True, blank=True) - - orcid_token_stored = models.BooleanField(default=False) - allow_indexing = models.BooleanField(null=True, blank=True, default=None) objects = OSFUserManager() @@ -823,7 +829,12 @@ def merge_user(self, user): self.external_identity[service] = { service_id: status } + + token_entry = user.external_identity_access_token.get(service, {}).get(service_id) + if token_entry: + self.external_identity_access_token.setdefault(service, {})[service_id] = token_entry user.external_identity = {} + user.external_identity_access_token = {} # FOREIGN FIELDS self.external_accounts.add(*user.external_accounts.values_list('pk', flat=True)) @@ -2143,6 +2154,49 @@ def _clear_identifying_information(self): ''' This method ensures a user's info is deleted during a GDPR delete ''' + orcid_tokens = { + orcid_id: token_entry.get('access_token') + for orcid_id, token_entry in self.external_identity_access_token.get('ORCID', {}).items() + if token_entry.get('access_token') + } + sentry.log_message( + f'[GDPR delete; _clear_identifying_information] user={self._id}: ' + f'found {len(orcid_tokens)} ORCID token(s) to revoke', + level=logging.INFO, + ) + for orcid_id, orcid_token in orcid_tokens.items(): + sentry.log_message( + f'[GDPR delete] user={self._id}: revoking ORCID id={orcid_id} ' + f'via {website_settings.ORCID_OAUTH_REVOKE_URL}', + level=logging.INFO, + ) + try: + response = requests.post( + website_settings.ORCID_OAUTH_REVOKE_URL, + data={ + 'client_id': website_settings.ORCID_OAUTH_CLIENT_ID, + 'client_secret': website_settings.ORCID_OAUTH_CLIENT_SECRET, + 'token': orcid_token, + }, + timeout=5, + ) + sentry.log_message( + f'[GDPR delete] user={self._id}: ORCID id={orcid_id} revoked, ' + f'status_code={response.status_code}, response_text={response.text}, response={response}', + level=logging.INFO, + ) + response.raise_for_status() + except requests.exceptions.RequestException as e: + sentry.log_message( + f'[GDPR delete] Failed to revoke ORCID token for user {self._id}: {e}', + level=logging.ERROR, + ) + sentry.log_exception(e) + raise UserStateError( + 'Unable to revoke this user\'s ORCID access right now because ORCID\'s ' + 'service could not be reached. Please try the GDPR delete again later.' + ) + # This doesn't remove identifying info, but ensures other users can't see the deleted user's profile etc. self.deactivate_account() @@ -2179,22 +2233,8 @@ def _clear_identifying_information(self): account.save() self.external_accounts.clear() - # Revoke any ORCID OAuth token CAS holds for this user, so OSF no longer shows as a - # trusted party on the user's ORCID account. Best-effort: never blocks GDPR delete. - orcid_ids = self.external_identity.get('ORCID', {}) - if orcid_ids: - for orcid_id in orcid_ids: - try: - cas.get_client().revoke_orcid_token(orcid_id) - except cas.CasHTTPError as e: - logger.error(f'Unable to revoke ORCID token via CAS for user {self._id}, orcid_id={orcid_id}: {e}') - sentry.log_exception(e) - except Exception as e: - logger.error(f'Unexpected error revoking ORCID token via CAS for user {self._id}, orcid_id={orcid_id}: {e}') - sentry.log_exception(e) - self.orcid_token_stored = False - self.external_identity = {} + self.external_identity_access_token = {} self.deleted = timezone.now() @property diff --git a/osf_tests/test_merging_users.py b/osf_tests/test_merging_users.py index e505b5580e3..1d144ee6b2c 100644 --- a/osf_tests/test_merging_users.py +++ b/osf_tests/test_merging_users.py @@ -273,6 +273,31 @@ def test_merge_preserves_external_identity(self): assert linking_user.external_identity == {} assert no_provider_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}} + def test_merge_transfers_external_identity_access_token(self): + surviving_user = UserFactory( + external_identity={'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}}, + ) + merged_user = UserFactory( + external_identity={'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}}, + external_identity_access_token={ + 'ORCID': { + '1234-1234-1234-1234': {'access_token': 'token-1234', 'refresh_token': None}, + '4321-4321-4321-4321': {'access_token': 'token-4321', 'refresh_token': None}, + }, + }, + ) + + with override_flag(ENABLE_GV, active=True): + surviving_user.merge_user(merged_user) + + assert surviving_user.external_identity_access_token == { + 'ORCID': { + '1234-1234-1234-1234': {'access_token': 'token-1234', 'refresh_token': None}, + '4321-4321-4321-4321': {'access_token': 'token-4321', 'refresh_token': None}, + }, + } + assert merged_user.external_identity_access_token == {} + def test_merge_unregistered(self): # test only those behaviors that are not tested with unconfirmed users self._add_unregistered_user() diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 41de62fdae2..a18eab8e815 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -12,6 +12,7 @@ from unittest import mock import itsdangerous import pytest +import requests from importlib import import_module from waffle.testutils import override_flag @@ -2224,10 +2225,13 @@ def test_gdpr_delete_triggers_share_update_for_public_shared_preprints( assert mock_update_search.called - @mock.patch('framework.auth.cas.CasClient.revoke_orcid_token') - def test_can_gdpr_delete(self, mock_revoke_orcid_token, user): + @mock.patch('osf.models.user.requests.post') + def test_can_gdpr_delete(self, mock_post, user): + mock_post.return_value = mock.Mock(status_code=200) user.external_identity = {'ORCID': {'fake-orcid-id': 'VERIFIED'}} - user.orcid_token_stored = True + user.external_identity_access_token = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token', 'refresh_token': None}}, + } user.save() user.social = ['fake social'] @@ -2243,37 +2247,37 @@ def test_can_gdpr_delete(self, mock_revoke_orcid_token, user): assert user.schools == [] assert user.jobs == [] assert user.external_identity == {} + assert user.external_identity_access_token == {} assert not user.emails.exists() assert not user.external_accounts.exists() assert user.is_disabled assert user.deleted is not None - mock_revoke_orcid_token.assert_called_once_with('fake-orcid-id') - assert user.orcid_token_stored is False - - @mock.patch('framework.auth.cas.CasClient.revoke_orcid_token') - def test_gdpr_delete_no_orcid_no_cas_call(self, mock_revoke_orcid_token, user): - assert user.external_identity == {} + mock_post.assert_called_once() + assert mock_post.call_args.kwargs['data']['token'] == 'fake-orcid-token' + @mock.patch('osf.models.user.requests.post') + def test_gdpr_delete_no_orcid_token_no_revoke_call(self, mock_post, user): user.gdpr_delete() - mock_revoke_orcid_token.assert_not_called() + mock_post.assert_not_called() - @mock.patch('framework.auth.cas.CasClient.revoke_orcid_token') - def test_gdpr_delete_orcid_revoke_failure_does_not_block_delete(self, mock_revoke_orcid_token, user): - from framework.auth import cas - mock_revoke_orcid_token.side_effect = cas.CasHTTPError( - code=400, message='Bad Request', headers={}, content=b'', - ) - user.external_identity = {'ORCID': {'fake-orcid-id': 'VERIFIED'}} - user.orcid_token_stored = True + @mock.patch('osf.models.user.requests.post') + def test_gdpr_delete_orcid_revoke_failure_blocks_delete(self, mock_post, user): + mock_post.side_effect = requests.exceptions.ConnectionError('boom') + user.external_identity_access_token = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token', 'refresh_token': None}}, + } user.save() - user.gdpr_delete() + with pytest.raises(UserStateError): + user.gdpr_delete() - mock_revoke_orcid_token.assert_called_once_with('fake-orcid-id') - assert user.external_identity == {} - assert user.orcid_token_stored is False - assert user.deleted is not None + mock_post.assert_called_once() + assert user.deleted is None + assert not user.is_disabled + assert user.external_identity_access_token == { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token', 'refresh_token': None}}, + } def test_can_gdpr_delete_personal_nodes(self, user): diff --git a/tests/test_cas_authentication.py b/tests/test_cas_authentication.py index a272bfe4feb..2bd029a27c3 100644 --- a/tests/test_cas_authentication.py +++ b/tests/test_cas_authentication.py @@ -55,6 +55,19 @@ def make_external_response(release=True, unicode=False): ) +def make_external_response_with_orcid_access_token(access_token=None): + return cas.CasResponse( + authenticated=True, + user='OrcidProfile#{}'.format(fake.numerify('####-####-####-####')), + attributes={ + 'accessToken': fake.md5(), + 'given-names': fake.first_name(), + 'family-name': fake.last_name(), + 'orcidAccessToken': access_token or fake.md5(), + } + ) + + def generate_external_user_with_resp(service_url, user=True, release=True): """ Generate mock user, external credential and cas response for tests. @@ -371,6 +384,75 @@ def test_make_response_from_ticket_generates_new_verification_key(self, mock_ser self.user.reload() assert self.user.verification_key != verification_key + @mock.patch('framework.auth.cas.CasClient.service_validate') + def test_make_response_from_ticket_stores_orcid_access_token(self, mock_service_validate): + access_token = fake.md5() + mock_response = make_external_response_with_orcid_access_token(access_token=access_token) + validated_creds = cas.validate_external_credential(mock_response.user) + self.user.external_identity = { + validated_creds['provider']: { + validated_creds['id']: 'VERIFIED' + } + } + self.user.save() + mock_service_validate.return_value = mock_response + ticket = fake.md5() + service_url = 'http://localhost:5000/' + resp = cas.make_response_from_ticket(ticket, service_url) + assert resp.status_code == 302 + self.user.reload() + assert self.user.external_identity_access_token == { + validated_creds['provider']: { + validated_creds['id']: {'access_token': access_token, 'refresh_token': None}, + }, + } + + @mock.patch('framework.auth.cas.CasClient.service_validate') + def test_make_response_from_ticket_updates_existing_orcid_access_token(self, mock_service_validate): + mock_response = make_external_response_with_orcid_access_token() + validated_creds = cas.validate_external_credential(mock_response.user) + self.user.external_identity = { + validated_creds['provider']: { + validated_creds['id']: 'VERIFIED' + } + } + self.user.external_identity_access_token = { + validated_creds['provider']: { + validated_creds['id']: {'access_token': 'old-token', 'refresh_token': None}, + }, + } + self.user.save() + mock_service_validate.return_value = mock_response + ticket = fake.md5() + service_url = 'http://localhost:5000/' + resp = cas.make_response_from_ticket(ticket, service_url) + assert resp.status_code == 302 + self.user.reload() + new_access_token = mock_response.attributes['orcidAccessToken'] + assert self.user.external_identity_access_token == { + validated_creds['provider']: { + validated_creds['id']: {'access_token': new_access_token, 'refresh_token': None}, + }, + } + + @mock.patch('framework.auth.cas.CasClient.service_validate') + def test_make_response_from_ticket_no_orcid_access_token_no_entry_created(self, mock_service_validate): + mock_response = make_external_response() + validated_creds = cas.validate_external_credential(mock_response.user) + self.user.external_identity = { + validated_creds['provider']: { + validated_creds['id']: 'VERIFIED' + } + } + self.user.save() + mock_service_validate.return_value = mock_response + ticket = fake.md5() + service_url = 'http://localhost:5000/' + resp = cas.make_response_from_ticket(ticket, service_url) + assert resp.status_code == 302 + self.user.reload() + assert self.user.external_identity_access_token == {} + @mock.patch('framework.auth.cas.CasClient.service_validate') def test_make_response_from_ticket_handles_unicode(self, mock_service_validate): mock_response = make_external_response(unicode=True) From eebddd608152865a48b6944305a9bc633ffb8588 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Thu, 27 Aug 2026 22:19:50 +0200 Subject: [PATCH 3/5] Update column name, fix comments --- framework/auth/cas.py | 32 +++---------------- .../0053_osfuser_orcid_token_tracking.py | 2 +- osf/models/user.py | 27 ++++++++-------- website/settings/defaults.py | 5 +++ 4 files changed, 25 insertions(+), 41 deletions(-) diff --git a/framework/auth/cas.py b/framework/auth/cas.py index 6b41d59f188..314ec5b3ee5 100644 --- a/framework/auth/cas.py +++ b/framework/auth/cas.py @@ -31,7 +31,6 @@ class CasHTTPError(CasError): def __init__(self, code, message, headers, content): super().__init__(code, message) - self.message = message self.headers = headers self.content = content @@ -101,9 +100,6 @@ def get_auth_token_revocation_url(self): url = furl(self.BASE_URL).add(path=['oauth2', 'revoke']) return url.url - def get_orcid_token_revocation_url(self): - url = furl(self.BASE_URL).add(path=['osf', 'orcid', 'revoke']) - return url.url def service_validate(self, ticket, service_url): """ @@ -206,17 +202,6 @@ def revoke_tokens(self, payload): else: self._handle_error(resp) - def revoke_orcid_token(self, orcid_id): - url = self.get_orcid_token_revocation_url() - headers = { - 'Authorization': f'Bearer {settings.CAS_ORCID_REVOKE_SHARED_SECRET}', - } - resp = requests.post(url, json={'orcid_id': orcid_id}, headers=headers) - if resp.status_code == 204: - return True - else: - self._handle_error(resp) - def parse_auth_header(header): """ @@ -276,7 +261,7 @@ def save_orcid_access_and_refresh_token_to_user(user, orcid_id: str, access_toke sentry.log_message( f'CAS response ORCID attributes: user=[{user._id}], orcidId=[{orcid_id}], ' f'orcidAccessToken=[{"present" if access_token else "missing"}]', - level=logging.INFO, + level=logging.WARNING, ) if orcid_id and access_token: provider = settings.EXTERNAL_IDENTITY_PROFILE['OrcidProfile'] @@ -286,8 +271,8 @@ def save_orcid_access_and_refresh_token_to_user(user, orcid_id: str, access_toke } sentry.log_message( f'ORCID token stored on external_identity_access_token: user=[{user._id}], ' - f'provider_id=[{orcid_id}], access_token=[{access_token if access_token else "missing"}]' - f'refresh_token=[{refresh_token if refresh_token else "missing"}]', + f'provider_id=[{orcid_id}], access_token=[{"present" if access_token else "missing"}]' + f'refresh_token=[{"present" if refresh_token else "missing"}]', level=logging.INFO, ) @@ -312,8 +297,6 @@ def make_response_from_ticket(ticket, service_url): user_updates = {} # serialize updates to user to be applied async # user found and authenticated if user and action == 'authenticate': - access_token = cas_resp.attributes.get('orcidAccessToken', None) - refresh_token = cas_resp.attributes.get('orcidRefreshToken', None) print_cas_log( f'CAS response - authenticating user: user=[{user._id}], ' f'external=[{external_credential}], action=[{action}]', @@ -329,13 +312,6 @@ def make_response_from_ticket(ticket, service_url): user_updates['accepted_terms_of_service'] = timezone.now() print_cas_log(f'CAS TOS consent checked: {user.guids.first()._id}, {user.username}', LogLevel.INFO) # if we successfully authenticate and a verification key is present, invalidate it - if external_credential and access_token and refresh_token: - save_orcid_access_and_refresh_token_to_user( - user, - external_credential['id'], - access_token, - refresh_token, - ) if user.verification_key: user_updates['verification_key'] = None @@ -343,6 +319,8 @@ def make_response_from_ticket(ticket, service_url): # this extra step will guarantee that 2FA are enforced # current CAS session created by external login must be cleared first before authentication if external_credential: + access_token = cas_resp.attributes.get('orcidAccessToken', None) + refresh_token = cas_resp.attributes.get('orcidRefreshToken', None) user.verification_key = generate_verification_key() save_orcid_access_and_refresh_token_to_user( user, diff --git a/osf/migrations/0053_osfuser_orcid_token_tracking.py b/osf/migrations/0053_osfuser_orcid_token_tracking.py index 4582ac4b891..ec61974a6ea 100644 --- a/osf/migrations/0053_osfuser_orcid_token_tracking.py +++ b/osf/migrations/0053_osfuser_orcid_token_tracking.py @@ -12,7 +12,7 @@ class Migration(migrations.Migration): operations = [ migrations.AddField( model_name='osfuser', - name='external_identity_access_token', + name='external_identity_tokens', field=osf.utils.datetime_aware_jsonfield.DateTimeAwareJSONField(blank=True, default=dict, encoder=osf.utils.datetime_aware_jsonfield.DateTimeAwareJSONEncoder), ), ] diff --git a/osf/models/user.py b/osf/models/user.py index 1f908975b28..e464c050a2e 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -320,7 +320,7 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi # }, # ... # } - external_identity_access_token = DateTimeAwareJSONField(default=dict, blank=True) + external_identity_tokens = DateTimeAwareJSONField(default=dict, blank=True) # Format: { # : { # : { @@ -2154,17 +2154,15 @@ def _clear_identifying_information(self): ''' This method ensures a user's info is deleted during a GDPR delete ''' - orcid_tokens = { - orcid_id: token_entry.get('access_token') - for orcid_id, token_entry in self.external_identity_access_token.get('ORCID', {}).items() - if token_entry.get('access_token') - } + # A user has at most one ORCID identity, so there is at most one token entry to revoke. + orcid_id, token_entry = next(iter(self.external_identity_access_token.get('ORCID', {}).items()), (None, None)) + orcid_token = token_entry.get('access_token') if token_entry else None sentry.log_message( f'[GDPR delete; _clear_identifying_information] user={self._id}: ' - f'found {len(orcid_tokens)} ORCID token(s) to revoke', + f'{"found" if orcid_token else "no"} ORCID token to revoke', level=logging.INFO, ) - for orcid_id, orcid_token in orcid_tokens.items(): + if orcid_id and orcid_token: sentry.log_message( f'[GDPR delete] user={self._id}: revoking ORCID id={orcid_id} ' f'via {website_settings.ORCID_OAUTH_REVOKE_URL}', @@ -2178,24 +2176,27 @@ def _clear_identifying_information(self): 'client_secret': website_settings.ORCID_OAUTH_CLIENT_SECRET, 'token': orcid_token, }, - timeout=5, + timeout=website_settings.ORCID_OAUTH_REVOKE_REQUEST_TIMEOUT, ) sentry.log_message( f'[GDPR delete] user={self._id}: ORCID id={orcid_id} revoked, ' - f'status_code={response.status_code}, response_text={response.text}, response={response}', + f'status_code={response.status_code}, response_text={response.text}', level=logging.INFO, ) response.raise_for_status() except requests.exceptions.RequestException as e: sentry.log_message( - f'[GDPR delete] Failed to revoke ORCID token for user {self._id}: {e}', + f'[GDPR delete] Failed to revoke ORCID token for user {self._id} ORCID id {orcid_id}: {e}', level=logging.ERROR, ) sentry.log_exception(e) raise UserStateError( - 'Unable to revoke this user\'s ORCID access right now because ORCID\'s ' - 'service could not be reached. Please try the GDPR delete again later.' + 'Fail to revoke ORCID\'s service could not be reached' ) + else: + raise UserStateError( + 'User do not have connected ORCID' + ) # This doesn't remove identifying info, but ensures other users can't see the deleted user's profile etc. self.deactivate_account() diff --git a/website/settings/defaults.py b/website/settings/defaults.py index a23f036624c..97f5242107b 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -394,6 +394,11 @@ def parent_dir(path): CAS_SERVER_URL = 'http://localhost:8080' CAS_ORCID_REVOKE_SHARED_SECRET = os.environ.get('CAS_ORCID_REVOKE_SHARED_SECRET', 'changeme') +ORCID_OAUTH_CLIENT_ID = os.environ.get('ORCID_OAUTH_CLIENT_ID', 'changeme') +ORCID_OAUTH_CLIENT_SECRET = os.environ.get('ORCID_OAUTH_CLIENT_SECRET', 'changeme') +ORCID_OAUTH_REVOKE_URL = os.environ.get('ORCID_OAUTH_REVOKE_URL', 'https://orcid.org/oauth/revoke') +ORCID_OAUTH_REVOKE_REQUEST_TIMEOUT = os.environ.get('ORCID_OAUTH_REVOKE_REQUEST_TIMEOUT', 15) + MFR_SERVER_URL = 'http://localhost:7778' ###### ARCHIVER ########### From b364f1ff6e42e43f1c0fc894e677d0357a4d79e2 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Fri, 28 Aug 2026 15:46:33 +0200 Subject: [PATCH 4/5] Remove orcid refresh token --- api_tests/users/views/test_user_detail.py | 9 ++- framework/auth/cas.py | 16 ++--- osf/models/user.py | 21 +++--- osf_tests/test_merging_users.py | 17 ++--- osf_tests/test_user.py | 87 ++++++++++++++++++++--- tests/test_cas_authentication.py | 16 +++-- 6 files changed, 120 insertions(+), 46 deletions(-) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 70bd2cfe810..1a6b1f1f42a 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -1216,8 +1216,15 @@ def test_requesting_deactivated_user_returns_410_response_and_meta_info( res.json['errors'][0]['meta']['profile_image']).netloc == 'secure.gravatar.com' assert res.json['errors'][0]['detail'] == 'The requested user is no longer available.' + @mock.patch('osf.models.user.requests.post') def test_gdpr_deleted_user_returns_404_and_no_meta_info( - self, app, user_one, user_two): + self, mock_post, app, user_one, user_two): + mock_post.return_value = mock.Mock(status_code=200) + user_one.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + user_one.save() + url = f'/{API_BASE}users/{user_one._id}/' res = app.get(url, auth=user_two.auth, expect_errors=False) assert res.status_code == 200 diff --git a/framework/auth/cas.py b/framework/auth/cas.py index 314ec5b3ee5..a8860538bee 100644 --- a/framework/auth/cas.py +++ b/framework/auth/cas.py @@ -100,7 +100,6 @@ def get_auth_token_revocation_url(self): url = furl(self.BASE_URL).add(path=['oauth2', 'revoke']) return url.url - def service_validate(self, ticket, service_url): """ Send request to CAS to validate ticket. @@ -257,7 +256,7 @@ def get_profile_url(): return get_client().get_profile_url() -def save_orcid_access_and_refresh_token_to_user(user, orcid_id: str, access_token: str, refresh_token: str): +def save_orcid_access_token_to_user(user, orcid_id: str, access_token: str): sentry.log_message( f'CAS response ORCID attributes: user=[{user._id}], orcidId=[{orcid_id}], ' f'orcidAccessToken=[{"present" if access_token else "missing"}]', @@ -265,14 +264,12 @@ def save_orcid_access_and_refresh_token_to_user(user, orcid_id: str, access_toke ) if orcid_id and access_token: provider = settings.EXTERNAL_IDENTITY_PROFILE['OrcidProfile'] - user.external_identity_access_token.setdefault(provider, {})[orcid_id] = { + user.external_identity_tokens.setdefault(provider, {})[orcid_id] = { 'access_token': access_token, - 'refresh_token': refresh_token, } sentry.log_message( - f'ORCID token stored on external_identity_access_token: user=[{user._id}], ' - f'provider_id=[{orcid_id}], access_token=[{"present" if access_token else "missing"}]' - f'refresh_token=[{"present" if refresh_token else "missing"}]', + f'ORCID token stored on external_identity_tokens: user=[{user._id}], ' + f'provider_id=[{orcid_id}], access_token=[{"present" if access_token else "missing"}]', level=logging.INFO, ) @@ -320,13 +317,11 @@ def make_response_from_ticket(ticket, service_url): # current CAS session created by external login must be cleared first before authentication if external_credential: access_token = cas_resp.attributes.get('orcidAccessToken', None) - refresh_token = cas_resp.attributes.get('orcidRefreshToken', None) user.verification_key = generate_verification_key() - save_orcid_access_and_refresh_token_to_user( + save_orcid_access_token_to_user( user, external_credential['id'], access_token, - refresh_token, ) user.save() print_cas_log( @@ -355,6 +350,7 @@ def make_response_from_ticket(ticket, service_url): user = { 'external_id_provider': external_credential['provider'], 'external_id': external_credential['id'], + 'external_id_access_token': cas_resp.attributes.get('orcidAccessToken', None), 'fullname': fullname, 'service_url': service_furl.url, } diff --git a/osf/models/user.py b/osf/models/user.py index e464c050a2e..81add518d28 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -325,7 +325,6 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi # : { # : { # "access_token" : , - # "refresh_token" : , # } # ... # }, @@ -830,11 +829,11 @@ def merge_user(self, user): service_id: status } - token_entry = user.external_identity_access_token.get(service, {}).get(service_id) + token_entry = user.external_identity_tokens.get(service, {}).get(service_id) if token_entry: - self.external_identity_access_token.setdefault(service, {})[service_id] = token_entry + self.external_identity_tokens.setdefault(service, {})[service_id] = token_entry user.external_identity = {} - user.external_identity_access_token = {} + user.external_identity_tokens = {} # FOREIGN FIELDS self.external_accounts.add(*user.external_accounts.values_list('pk', flat=True)) @@ -2155,14 +2154,18 @@ def _clear_identifying_information(self): This method ensures a user's info is deleted during a GDPR delete ''' # A user has at most one ORCID identity, so there is at most one token entry to revoke. - orcid_id, token_entry = next(iter(self.external_identity_access_token.get('ORCID', {}).items()), (None, None)) + orcid_id, token_entry = next(iter(self.external_identity_tokens.get('ORCID', {}).items()), (None, None)) orcid_token = token_entry.get('access_token') if token_entry else None sentry.log_message( f'[GDPR delete; _clear_identifying_information] user={self._id}: ' f'{"found" if orcid_token else "no"} ORCID token to revoke', level=logging.INFO, ) - if orcid_id and orcid_token: + if not (orcid_id and orcid_token): + raise UserStateError( + 'User do not have connected ORCID' + ) + else: sentry.log_message( f'[GDPR delete] user={self._id}: revoking ORCID id={orcid_id} ' f'via {website_settings.ORCID_OAUTH_REVOKE_URL}', @@ -2193,10 +2196,6 @@ def _clear_identifying_information(self): raise UserStateError( 'Fail to revoke ORCID\'s service could not be reached' ) - else: - raise UserStateError( - 'User do not have connected ORCID' - ) # This doesn't remove identifying info, but ensures other users can't see the deleted user's profile etc. self.deactivate_account() @@ -2235,7 +2234,7 @@ def _clear_identifying_information(self): self.external_accounts.clear() self.external_identity = {} - self.external_identity_access_token = {} + self.external_identity_tokens = {} self.deleted = timezone.now() @property diff --git a/osf_tests/test_merging_users.py b/osf_tests/test_merging_users.py index 1d144ee6b2c..b27167f382b 100644 --- a/osf_tests/test_merging_users.py +++ b/osf_tests/test_merging_users.py @@ -56,6 +56,7 @@ def _add_unregistered_user(self): self.project_with_unreg_contrib.save() @pytest.mark.enable_enqueue_task + @pytest.mark.enable_search @mock.patch('website.mailchimp_utils.get_mailchimp_api') def test_merge(self, mock_get_mailchimp_api): def is_mrm_field(value): @@ -273,16 +274,16 @@ def test_merge_preserves_external_identity(self): assert linking_user.external_identity == {} assert no_provider_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}} - def test_merge_transfers_external_identity_access_token(self): + def test_merge_transfers_external_identity_tokens(self): surviving_user = UserFactory( external_identity={'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}}, ) merged_user = UserFactory( external_identity={'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}}, - external_identity_access_token={ + external_identity_tokens={ 'ORCID': { - '1234-1234-1234-1234': {'access_token': 'token-1234', 'refresh_token': None}, - '4321-4321-4321-4321': {'access_token': 'token-4321', 'refresh_token': None}, + '1234-1234-1234-1234': {'access_token': 'token-1234'}, + '4321-4321-4321-4321': {'access_token': 'token-4321'}, }, }, ) @@ -290,13 +291,13 @@ def test_merge_transfers_external_identity_access_token(self): with override_flag(ENABLE_GV, active=True): surviving_user.merge_user(merged_user) - assert surviving_user.external_identity_access_token == { + assert surviving_user.external_identity_tokens == { 'ORCID': { - '1234-1234-1234-1234': {'access_token': 'token-1234', 'refresh_token': None}, - '4321-4321-4321-4321': {'access_token': 'token-4321', 'refresh_token': None}, + '1234-1234-1234-1234': {'access_token': 'token-1234'}, + '4321-4321-4321-4321': {'access_token': 'token-4321'}, }, } - assert merged_user.external_identity_access_token == {} + assert merged_user.external_identity_tokens == {} def test_merge_unregistered(self): # test only those behaviors that are not tested with unconfirmed users diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index a18eab8e815..f98d83fb698 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -2125,7 +2125,13 @@ class TestUserGdprDelete: @pytest.fixture() def user(self): - return AuthUserFactory() + user = AuthUserFactory() + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + user.save() + with mock.patch('osf.models.user.requests.post', return_value=mock.Mock(status_code=200)): + yield user @pytest.fixture() def project_with_two_admins(self, user): @@ -2229,8 +2235,8 @@ def test_gdpr_delete_triggers_share_update_for_public_shared_preprints( def test_can_gdpr_delete(self, mock_post, user): mock_post.return_value = mock.Mock(status_code=200) user.external_identity = {'ORCID': {'fake-orcid-id': 'VERIFIED'}} - user.external_identity_access_token = { - 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token', 'refresh_token': None}}, + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, } user.save() @@ -2247,7 +2253,7 @@ def test_can_gdpr_delete(self, mock_post, user): assert user.schools == [] assert user.jobs == [] assert user.external_identity == {} - assert user.external_identity_access_token == {} + assert user.external_identity_tokens == {} assert not user.emails.exists() assert not user.external_accounts.exists() assert user.is_disabled @@ -2257,15 +2263,22 @@ def test_can_gdpr_delete(self, mock_post, user): @mock.patch('osf.models.user.requests.post') def test_gdpr_delete_no_orcid_token_no_revoke_call(self, mock_post, user): + mock_post.return_value = mock.Mock(status_code=200) + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + user.save() + user.gdpr_delete() - mock_post.assert_not_called() + mock_post.assert_called_once() + assert mock_post.call_args.kwargs['data']['token'] == 'fake-orcid-token' @mock.patch('osf.models.user.requests.post') def test_gdpr_delete_orcid_revoke_failure_blocks_delete(self, mock_post, user): mock_post.side_effect = requests.exceptions.ConnectionError('boom') - user.external_identity_access_token = { - 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token', 'refresh_token': None}}, + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, } user.save() @@ -2275,9 +2288,65 @@ def test_gdpr_delete_orcid_revoke_failure_blocks_delete(self, mock_post, user): mock_post.assert_called_once() assert user.deleted is None assert not user.is_disabled - assert user.external_identity_access_token == { - 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token', 'refresh_token': None}}, + assert user.external_identity_tokens == { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + + @mock.patch('osf.models.user.requests.post') + def test_gdpr_delete_orcid_revoke_invalid_client_credentials_blocks_delete(self, mock_post, user): + mock_response = mock.Mock(status_code=401, text='invalid_client') + mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError('401 Client Error') + mock_post.return_value = mock_response + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, } + user.save() + + with pytest.raises(UserStateError): + user.gdpr_delete() + + mock_post.assert_called_once() + assert mock_post.call_args.kwargs['data']['token'] == 'fake-orcid-token' + assert user.deleted is None + assert not user.is_disabled + assert user.external_identity_tokens == { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + + @mock.patch('osf.models.user.requests.post') + def test_gdpr_delete_orcid_revoke_invalid_token_blocks_delete(self, mock_post, user): + mock_response = mock.Mock(status_code=400, text='invalid_token') + mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError('400 Client Error') + mock_post.return_value = mock_response + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'bad-orcid-token'}}, + } + user.save() + + with pytest.raises(UserStateError): + user.gdpr_delete() + + mock_post.assert_called_once() + assert mock_post.call_args.kwargs['data']['token'] == 'bad-orcid-token' + assert user.deleted is None + assert not user.is_disabled + assert user.external_identity_tokens == { + 'ORCID': {'fake-orcid-id': {'access_token': 'bad-orcid-token'}}, + } + + @mock.patch('osf.models.user.requests.post') + def test_gdpr_delete_orcid_empty_token_no_revoke_call(self, mock_post, user): + mock_post.return_value = mock.Mock(status_code=200) + user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + user.save() + + user.gdpr_delete() + + mock_post.assert_called_once() + assert user.deleted is not None + assert user.is_disabled def test_can_gdpr_delete_personal_nodes(self, user): diff --git a/tests/test_cas_authentication.py b/tests/test_cas_authentication.py index 2bd029a27c3..7a8b18f7a1f 100644 --- a/tests/test_cas_authentication.py +++ b/tests/test_cas_authentication.py @@ -258,6 +258,7 @@ def test_make_response_from_ticket_success(self, mock_service_validate, mock_get assert mock_get_user_from_cas_resp.call_count == 1 @pytest.mark.enable_enqueue_task + @pytest.mark.enable_search @mock.patch('framework.auth.cas.get_user_from_cas_resp') @mock.patch('framework.auth.cas.CasClient.service_validate') def test_make_response_from_ticket_success_with_tos_consent(self, mock_service_validate, mock_get_user_from_cas_resp): @@ -287,6 +288,7 @@ def test_make_response_from_ticket_failure(self, mock_service_validate, mock_get assert mock_get_user_from_cas_resp.call_count == 0 @pytest.mark.enable_enqueue_task + @pytest.mark.enable_search @mock.patch('framework.auth.cas.CasClient.service_validate') def test_make_response_from_ticket_invalidates_verification_key(self, mock_service_validate): self.user.verification_key = fake.md5() @@ -401,9 +403,9 @@ def test_make_response_from_ticket_stores_orcid_access_token(self, mock_service_ resp = cas.make_response_from_ticket(ticket, service_url) assert resp.status_code == 302 self.user.reload() - assert self.user.external_identity_access_token == { + assert self.user.external_identity_tokens == { validated_creds['provider']: { - validated_creds['id']: {'access_token': access_token, 'refresh_token': None}, + validated_creds['id']: {'access_token': access_token}, }, } @@ -416,9 +418,9 @@ def test_make_response_from_ticket_updates_existing_orcid_access_token(self, moc validated_creds['id']: 'VERIFIED' } } - self.user.external_identity_access_token = { + self.user.external_identity_tokens = { validated_creds['provider']: { - validated_creds['id']: {'access_token': 'old-token', 'refresh_token': None}, + validated_creds['id']: {'access_token': 'old-token'}, }, } self.user.save() @@ -429,9 +431,9 @@ def test_make_response_from_ticket_updates_existing_orcid_access_token(self, moc assert resp.status_code == 302 self.user.reload() new_access_token = mock_response.attributes['orcidAccessToken'] - assert self.user.external_identity_access_token == { + assert self.user.external_identity_tokens == { validated_creds['provider']: { - validated_creds['id']: {'access_token': new_access_token, 'refresh_token': None}, + validated_creds['id']: {'access_token': new_access_token}, }, } @@ -451,7 +453,7 @@ def test_make_response_from_ticket_no_orcid_access_token_no_entry_created(self, resp = cas.make_response_from_ticket(ticket, service_url) assert resp.status_code == 302 self.user.reload() - assert self.user.external_identity_access_token == {} + assert self.user.external_identity_tokens == {} @mock.patch('framework.auth.cas.CasClient.service_validate') def test_make_response_from_ticket_handles_unicode(self, mock_service_validate): From f5aa332f37dd59bf4367fddf6ea0b63e1e2c32a9 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Fri, 28 Aug 2026 16:17:48 +0200 Subject: [PATCH 5/5] fix tests --- admin_tests/users/test_views.py | 7 +++++++ tests/test_cas_authentication.py | 1 + 2 files changed, 8 insertions(+) diff --git a/admin_tests/users/test_views.py b/admin_tests/users/test_views.py index 6c86d9ed923..05d2b140e4c 100644 --- a/admin_tests/users/test_views.py +++ b/admin_tests/users/test_views.py @@ -113,6 +113,13 @@ def test_correct_view_permissions(self): class TestGDPRDeleteUser(AdminTestCase): def setUp(self): self.user = UserFactory() + self.user.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + self.user.save() + post_patcher = mock.patch('osf.models.user.requests.post', return_value=mock.Mock(status_code=200)) + self.mock_post = post_patcher.start() + self.addCleanup(post_patcher.stop) self.request = RequestFactory().post('/fake_path') self.view = views.UserGDPRDeleteView self.view = setup_log_view(self.view, self.request, guid=self.user._id) diff --git a/tests/test_cas_authentication.py b/tests/test_cas_authentication.py index 7a8b18f7a1f..e7967d4e132 100644 --- a/tests/test_cas_authentication.py +++ b/tests/test_cas_authentication.py @@ -92,6 +92,7 @@ def generate_external_user_with_resp(service_url, user=True, release=True): user = { 'external_id_provider': validated_credentials['provider'], 'external_id': validated_credentials['id'], + 'external_id_access_token': cas_resp.attributes.get('orcidAccessToken', None), 'fullname': '', 'service_url': service_url, }