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/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 1084739fdc3..a8860538bee 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 @@ -253,6 +256,22 @@ def get_profile_url(): return get_client().get_profile_url() +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"}]', + level=logging.WARNING, + ) + if orcid_id and access_token: + provider = settings.EXTERNAL_IDENTITY_PROFILE['OrcidProfile'] + user.external_identity_tokens.setdefault(provider, {})[orcid_id] = { + 'access_token': access_token, + } + sentry.log_message( + 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, + ) def make_response_from_ticket(ticket, service_url): """ @@ -297,7 +316,13 @@ 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) user.verification_key = generate_verification_key() + save_orcid_access_token_to_user( + user, + external_credential['id'], + access_token, + ) user.save() print_cas_log( f'CAS response - redirect existing external IdP login to verification key login: user=[{user._id}]', @@ -325,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/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/migrations/0053_osfuser_orcid_token_tracking.py b/osf/migrations/0053_osfuser_orcid_token_tracking.py new file mode 100644 index 00000000000..ec61974a6ea --- /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_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 98461a3cf0a..81add518d28 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 @@ -319,6 +320,16 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi # }, # ... # } + external_identity_tokens = DateTimeAwareJSONField(default=dict, blank=True) + # Format: { + # : { + # : { + # "access_token" : , + # } + # ... + # }, + # ... + # } # Employment history jobs = DateTimeAwareJSONField(default=list, blank=True, validators=[validate_history_item]) @@ -817,7 +828,12 @@ def merge_user(self, user): self.external_identity[service] = { service_id: status } + + token_entry = user.external_identity_tokens.get(service, {}).get(service_id) + if token_entry: + self.external_identity_tokens.setdefault(service, {})[service_id] = token_entry user.external_identity = {} + user.external_identity_tokens = {} # FOREIGN FIELDS self.external_accounts.add(*user.external_accounts.values_list('pk', flat=True)) @@ -2137,6 +2153,50 @@ 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_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 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}', + 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=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}', + 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} ORCID id {orcid_id}: {e}', + level=logging.ERROR, + ) + sentry.log_exception(e) + raise UserStateError( + 'Fail to revoke ORCID\'s service could not be reached' + ) + # This doesn't remove identifying info, but ensures other users can't see the deleted user's profile etc. self.deactivate_account() @@ -2172,7 +2232,9 @@ def _clear_identifying_information(self): account.profile_url = None account.save() self.external_accounts.clear() + self.external_identity = {} + 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 e505b5580e3..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,6 +274,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_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_tokens={ + 'ORCID': { + '1234-1234-1234-1234': {'access_token': 'token-1234'}, + '4321-4321-4321-4321': {'access_token': 'token-4321'}, + }, + }, + ) + + with override_flag(ENABLE_GV, active=True): + surviving_user.merge_user(merged_user) + + assert surviving_user.external_identity_tokens == { + 'ORCID': { + '1234-1234-1234-1234': {'access_token': 'token-1234'}, + '4321-4321-4321-4321': {'access_token': 'token-4321'}, + }, + } + assert merged_user.external_identity_tokens == {} + 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 9d2e8b12628..f98d83fb698 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 @@ -2124,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): @@ -2224,11 +2231,18 @@ 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('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.external_identity_tokens = { + 'ORCID': {'fake-orcid-id': {'access_token': 'fake-orcid-token'}}, + } + 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() @@ -2239,10 +2253,100 @@ def test_can_gdpr_delete(self, user): assert user.schools == [] assert user.jobs == [] assert user.external_identity == {} + assert user.external_identity_tokens == {} assert not user.emails.exists() assert not user.external_accounts.exists() assert user.is_disabled assert user.deleted is not None + 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): + 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 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_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 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_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 a272bfe4feb..e7967d4e132 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. @@ -79,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, } @@ -245,6 +259,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): @@ -274,6 +289,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() @@ -371,6 +387,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_tokens == { + validated_creds['provider']: { + validated_creds['id']: {'access_token': access_token}, + }, + } + + @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_tokens = { + validated_creds['provider']: { + validated_creds['id']: {'access_token': 'old-token'}, + }, + } + 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_tokens == { + validated_creds['provider']: { + validated_creds['id']: {'access_token': new_access_token}, + }, + } + + @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_tokens == {} + @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) diff --git a/website/settings/defaults.py b/website/settings/defaults.py index c247e2e24db..97f5242107b 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -393,6 +393,12 @@ 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') +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 ###########