diff --git a/admin/users/views.py b/admin/users/views.py index 894efddfc58..94d93f5ba87 100644 --- a/admin/users/views.py +++ b/admin/users/views.py @@ -213,12 +213,13 @@ class UserGDPRDeleteView(UserMixin, View): permission_required = 'osf.change_osfuser' def post(self, request, *args, **kwargs): + user = self.get_object() try: - user = self.get_object() user.gdpr_delete() user.save() except UserStateError as e: messages.warning(request, str(e)) + return redirect(self.get_success_url()) messages.success(request, f'User {user._id} was successfully GDPR deleted') diff --git a/framework/auth/cas.py b/framework/auth/cas.py index 1084739fdc3..42a8a81743e 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 @@ -289,6 +292,29 @@ 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') + sentry.log_message( + f'CAS response ORCID attributes: user=[{user._id}], orcidId=[{orcid_id}], ' + f'orcidAccessToken=[{"present" if orcid_access_token else "missing"}]', + level=logging.INFO, + ) + if orcid_id and orcid_access_token: + from osf.models.external import ExternalAccount + account, created = ExternalAccount.objects.update_or_create( + provider='orcid', + provider_id=orcid_id, + defaults={ + 'provider_name': 'ORCID', + 'oauth_key': orcid_access_token, + }, + ) + sentry.log_message( + f'ORCID external account {"created" if created else "updated"}: ' + f'user=[{user._id}], account=[{account._id}], provider_id=[{orcid_id}]', + level=logging.INFO, + ) + 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..385d1fed1a0 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 @@ -2131,6 +2132,45 @@ def _clear_identifying_information(self): ''' This method ensures a user's info is deleted during a GDPR delete ''' + orcid_accounts = self.external_accounts.filter(provider='orcid') + sentry.log_message( + f'[GDPR delete; _clear_identifying_information] user={self._id}: ' + f'found {orcid_accounts.count()} ORCID account(s) to revoke', + level=logging.INFO, + ) + for account in orcid_accounts: + sentry.log_message( + f'[GDPR delete] user={self._id}: revoking ORCID account={account._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': account.oauth_key, + }, + timeout=5, + ) + sentry.log_message( + f'[GDPR delete] user={self._id}: ORCID account={account._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() @@ -2166,6 +2206,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..706e228062c 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,76 @@ 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_blocks_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) + + with pytest.raises(UserStateError): + user.gdpr_delete() + + assert len(responses.calls) == 1 + # Nothing else should have been touched: fail fast, before any other clearing happens. + assert user.deleted is None + assert not user.is_disabled + assert user.external_accounts.exists() + account.reload() + assert account.oauth_key == 'fake-orcid-token' + + @responses.activate + def test_gdpr_delete_orcid_revoke_http_error_blocks_delete(self, user): + responses.add( + responses.POST, + settings.ORCID_OAUTH_REVOKE_URL, + status=500, + ) + account = ExternalAccountFactory(provider='orcid', oauth_key='fake-orcid-token') + user.external_accounts.add(account) + + with pytest.raises(UserStateError): + user.gdpr_delete() + + assert len(responses.calls) == 1 + assert user.deleted 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',