diff --git a/framework/auth/cas.py b/framework/auth/cas.py index 1084739fdc3..26da523e8e2 100644 --- a/framework/auth/cas.py +++ b/framework/auth/cas.py @@ -289,6 +289,19 @@ def make_response_from_ticket(ticket, service_url): if tos_checked_via_cas: user_updates['accepted_terms_of_service'] = timezone.now() print_cas_log(f'CAS TOS consent checked: {user.guids.first()._id}, {user.username}', LogLevel.INFO) + orcid_id = cas_resp.attributes.get('orcidId') + orcid_access_token = cas_resp.attributes.get('orcidAccessToken') + if orcid_id and orcid_access_token: + from osf.models.external import ExternalAccount + account, _ = ExternalAccount.objects.update_or_create( + provider='orcid', + provider_id=orcid_id, + defaults={ + 'provider_name': 'ORCID', + 'oauth_key': orcid_access_token, + }, + ) + user.external_accounts.add(account) # if we successfully authenticate and a verification key is present, invalidate it if user.verification_key: user_updates['verification_key'] = None diff --git a/framework/auth/tasks.py b/framework/auth/tasks.py index e0083798911..fb8a0959ff5 100644 --- a/framework/auth/tasks.py +++ b/framework/auth/tasks.py @@ -54,6 +54,7 @@ def update_affiliation_for_orcid_sso_users(user_id, orcid_id): logger.error(error_message) sentry.log_message(error_message) return + 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/models/user.py b/osf/models/user.py index 0218af34692..1fc9649eca8 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 @@ -2154,6 +2155,21 @@ def _clear_identifying_information(self): self.social = {} self.unclaimed_records = {} self.notifications_configured = {} + # Revoke ORCID OAuth grant before scrubbing the token below (best-effort, does not block deletion) + orcid_accounts = self.external_accounts.filter(provider='orcid') + for account in orcid_accounts: + try: + 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': account.oauth_key, + }, + timeout=5, + ) + except requests.exceptions.RequestException as e: + logger.warning(f'Failed to revoke ORCID token for user {self._id}: {e}') # Scrub all external accounts if self.external_accounts.exists(): logger.info('Clearing identifying information from external accounts...') @@ -2166,6 +2182,7 @@ def _clear_identifying_information(self): account.profile_url = None account.save() self.external_accounts.clear() + self.external_identity = {} self.deleted = timezone.now() diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 27a447d6681..543ad934751 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -12,6 +12,8 @@ from unittest import mock import itsdangerous import pytest +import requests +import responses from importlib import import_module from framework.auth.exceptions import ExpiredTokenError, InvalidTokenError, ChangePasswordError @@ -2242,6 +2244,57 @@ def test_can_gdpr_delete(self, user): assert user.is_disabled assert user.deleted is not None + @responses.activate + def test_gdpr_delete_revokes_orcid_token(self, user): + responses.add( + responses.POST, + settings.ORCID_OAUTH_REVOKE_URL, + status=200, + ) + account = ExternalAccountFactory(provider='orcid', oauth_key='fake-orcid-token') + user.external_accounts.add(account) + + user.gdpr_delete() + + assert len(responses.calls) == 1 + request_body = responses.calls[0].request.body + assert f'token={account.oauth_key}' in request_body + assert f'client_id={settings.ORCID_OAUTH_CLIENT_ID}' in request_body + assert f'client_secret={settings.ORCID_OAUTH_CLIENT_SECRET}' in request_body + account.reload() + assert account.oauth_key is None + + @responses.activate + def test_gdpr_delete_no_orcid_account_no_revoke_call(self, user): + responses.add( + responses.POST, + settings.ORCID_OAUTH_REVOKE_URL, + status=200, + ) + user.external_accounts.add(ExternalAccountFactory(provider='github')) + + user.gdpr_delete() + + assert len(responses.calls) == 0 + + @responses.activate + def test_gdpr_delete_orcid_revoke_failure_does_not_block_delete(self, user): + responses.add( + responses.POST, + settings.ORCID_OAUTH_REVOKE_URL, + body=requests.exceptions.ConnectionError('boom'), + ) + account = ExternalAccountFactory(provider='orcid', oauth_key='fake-orcid-token') + user.external_accounts.add(account) + + user.gdpr_delete() + + assert len(responses.calls) == 1 + assert user.deleted is not None + assert not user.external_accounts.exists() + account.reload() + assert account.oauth_key is None + def test_can_gdpr_delete_personal_nodes(self, user): user.gdpr_delete() diff --git a/tests/test_cas_authentication.py b/tests/test_cas_authentication.py index a272bfe4feb..2a5786da625 100644 --- a/tests/test_cas_authentication.py +++ b/tests/test_cas_authentication.py @@ -6,9 +6,10 @@ from framework.auth import cas +from osf.models import ExternalAccount from tests.base import OsfTestCase, fake from tests.utils import run_celery_tasks -from osf_tests.factories import UserFactory +from osf_tests.factories import ExternalAccountFactory, UserFactory def make_successful_response(user): @@ -32,6 +33,18 @@ def make_successful_response_with_tos_consent(user): ) +def make_successful_response_with_orcid_attrs(user, orcid_id=None, access_token=None): + return cas.CasResponse( + authenticated=True, + user=user._id, + attributes={ + 'accessToken': fake.md5(), + 'orcidId': orcid_id or fake.numerify('####-####-####-####'), + 'orcidAccessToken': access_token or fake.md5(), + } + ) + + def make_failure_response(): return cas.CasResponse( authenticated=False, @@ -261,6 +274,53 @@ def test_make_response_from_ticket_success_with_tos_consent(self, mock_service_v assert mock_service_validate.call_count == 1 assert mock_get_user_from_cas_resp.call_count == 1 + @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_orcid_attrs(self, mock_service_validate, mock_get_user_from_cas_resp): + orcid_id = fake.numerify('####-####-####-####') + access_token = fake.md5() + mock_service_validate.return_value = make_successful_response_with_orcid_attrs( + self.user, orcid_id=orcid_id, access_token=access_token + ) + mock_get_user_from_cas_resp.return_value = (self.user, None, 'authenticate') + ticket = fake.md5() + service_url = 'http://localhost:5000/' + resp = cas.make_response_from_ticket(ticket, service_url) + assert resp.status_code == 302 + account = ExternalAccount.objects.get(provider='orcid', provider_id=orcid_id) + assert account.oauth_key == access_token + assert account in self.user.external_accounts.all() + + @mock.patch('framework.auth.cas.get_user_from_cas_resp') + @mock.patch('framework.auth.cas.CasClient.service_validate') + def test_make_response_from_ticket_updates_existing_orcid_account(self, mock_service_validate, mock_get_user_from_cas_resp): + orcid_id = fake.numerify('####-####-####-####') + existing_account = ExternalAccountFactory(provider='orcid', provider_id=orcid_id, oauth_key='old-token') + self.user.external_accounts.add(existing_account) + new_access_token = fake.md5() + mock_service_validate.return_value = make_successful_response_with_orcid_attrs( + self.user, orcid_id=orcid_id, access_token=new_access_token + ) + mock_get_user_from_cas_resp.return_value = (self.user, None, 'authenticate') + ticket = fake.md5() + service_url = 'http://localhost:5000/' + resp = cas.make_response_from_ticket(ticket, service_url) + assert resp.status_code == 302 + assert ExternalAccount.objects.filter(provider='orcid', provider_id=orcid_id).count() == 1 + existing_account.reload() + assert existing_account.oauth_key == new_access_token + + @mock.patch('framework.auth.cas.get_user_from_cas_resp') + @mock.patch('framework.auth.cas.CasClient.service_validate') + def test_make_response_from_ticket_no_orcid_attrs_no_account_created(self, mock_service_validate, mock_get_user_from_cas_resp): + mock_service_validate.return_value = make_successful_response(self.user) + mock_get_user_from_cas_resp.return_value = (self.user, None, 'authenticate') + ticket = fake.md5() + service_url = 'http://localhost:5000/' + resp = cas.make_response_from_ticket(ticket, service_url) + assert resp.status_code == 302 + assert not ExternalAccount.objects.filter(provider='orcid').exists() + @mock.patch('framework.auth.cas.get_user_from_cas_resp') @mock.patch('framework.auth.cas.CasClient.service_validate') def test_make_response_from_ticket_failure(self, mock_service_validate, mock_get_user_from_cas_resp): diff --git a/website/settings/defaults.py b/website/settings/defaults.py index 026187948a5..88ba9464df7 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -796,6 +796,10 @@ class CeleryConfig: ORCID_RECORD_EMPLOYMENT_PATH = '/employments' ORCID_RECORD_EDUCATION_PATH = '/educations' +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://sandbox.orcid.org/oauth/revoke') + # Source: https://github.com/maxd/fake_email_validator/blob/master/config/fake_domains.list BLACKLISTED_DOMAINS = [ '0-mail.com',