Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions admin_tests/users/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion api_tests/users/views/test_user_detail.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions framework/auth/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ def external_first_login_authenticate(user_dict, response):
data = {
'auth_user_external_id_provider': user_dict['external_id_provider'],
'auth_user_external_id': user_dict['external_id'],
'auth_user_external_id_access_token': user_dict.get('external_id_access_token'),
'auth_user_external_id_refresh_token': user_dict.get('external_id_refresh_token'),
Comment on lines +63 to +64

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of the scope of PR but is fine to have it here.

'auth_user_fullname': user_dict['fullname'],
'auth_user_external_first_login': True,
'service_url': user_dict['service_url'],
Expand Down
33 changes: 33 additions & 0 deletions framework/auth/cas.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -253,6 +256,26 @@ def get_profile_url():

return get_client().get_profile_url()

def save_orcid_access_token_to_user(user, orcid_id: str, access_token: str, refresh_token: str = None):
sentry.log_message(
f'CAS response ORCID attributes: user=[{user._id}], orcidId=[{orcid_id}], '
f'orcidAccessToken=[{"present" if access_token else "missing"}], '
f'orcidRefreshToken=[{"present" if refresh_token else "missing"}]',
level=logging.WARNING,
)
Comment thread
Vlad0n20 marked this conversation as resolved.
Comment on lines +260 to +265

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future (new ticket), change this to debug log and only log empty access token error after we dev tested on staging1.

if orcid_id and access_token:
provider = settings.EXTERNAL_IDENTITY_PROFILE['OrcidProfile']
token_entry = {'access_token': access_token}
# Refresh token is optional: not all providers/users release one, depending on ORCID privacy settings.
if refresh_token:
token_entry['refresh_token'] = refresh_token
user.external_identity_tokens.setdefault(provider, {})[orcid_id] = token_entry
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"}], '
f'refresh_token=[{"present" if refresh_token else "missing"}]',
level=logging.INFO,
)
Comment on lines +273 to +278

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto


def make_response_from_ticket(ticket, service_url):
"""
Expand Down Expand Up @@ -297,7 +320,15 @@ 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)
Comment thread
cslzchen marked this conversation as resolved.
refresh_token = cas_resp.attributes.get('orcidRefreshToken', None)
user.verification_key = generate_verification_key()
save_orcid_access_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}]',
Expand Down Expand Up @@ -325,6 +356,8 @@ 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),
Comment thread
cslzchen marked this conversation as resolved.
'external_id_refresh_token': cas_resp.attributes.get('orcidRefreshToken', None),
'fullname': fullname,
'service_url': service_furl.url,
}
Expand Down
1 change: 1 addition & 0 deletions framework/auth/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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: '
Expand Down
4 changes: 4 additions & 0 deletions framework/auth/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,8 @@ def external_login_email_post():

external_id_provider = session.get('auth_user_external_id_provider', None)
external_id = session.get('auth_user_external_id', None)
external_id_access_token = session.get('auth_user_external_id_access_token', None)
external_id_refresh_token = session.get('auth_user_external_id_refresh_token', None)
Comment on lines +1053 to +1054

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, out of scope but it's ok

fullname = session.get('auth_user_fullname', None) or form.name.data
service_url = session.get('service_url', None)

Expand Down Expand Up @@ -1103,6 +1105,7 @@ def external_login_email_post():
user.accepted_terms_of_service = timezone.now()
# 2. add unconfirmed email and send confirmation email
user.add_unconfirmed_email(clean_email, external_identity=external_identity)
cas.save_orcid_access_token_to_user(user, external_id, external_id_access_token, external_id_refresh_token)
Comment thread
Vlad0n20 marked this conversation as resolved.
user.save()
send_confirm_email_async(
user,
Expand Down Expand Up @@ -1131,6 +1134,7 @@ def external_login_email_post():
campaign=None,
accepted_terms_of_service=accepted_terms_of_service
)
cas.save_orcid_access_token_to_user(user, external_id, external_id_access_token, external_id_refresh_token)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

user.save()
# 3. send confirmation email
send_confirm_email_async(
Expand Down
18 changes: 18 additions & 0 deletions osf/migrations/0053_gdpr_delete_and_orcid_revoke.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
from django.db import migrations
import osf.utils.datetime_aware_jsonfield
import osf.utils.fields


class Migration(migrations.Migration):
Comment thread
cslzchen marked this conversation as resolved.

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),
),
]
68 changes: 68 additions & 0 deletions osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# OSF imports
import itsdangerous
import pytz
import requests
from dirtyfields import DirtyFieldsMixin

from django.conf import settings
Expand Down Expand Up @@ -319,6 +320,17 @@ class OSFUser(DirtyFieldsMixin, GuidMixin, BaseModel, AbstractBaseUser, Permissi
# },
# ...
# }
external_identity_tokens = DateTimeAwareJSONField(default=dict, blank=True)
# Format: {
# <external_id_provider>: {
# <external_id>: {
# "access_token" : <token>,
# "refresh_token": <token>, # optional: not all providers/users release a refresh token
# }
Comment thread
cslzchen marked this conversation as resolved.
# ...
# },
# ...
# }

# Employment history
jobs = DateTimeAwareJSONField(default=list, blank=True, validators=[validate_history_item])
Expand Down Expand Up @@ -817,7 +829,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))
Expand Down Expand Up @@ -2137,6 +2154,55 @@ 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))
Comment thread
Vlad0n20 marked this conversation as resolved.
orcid_access_token = token_entry.get('access_token') if token_entry else None
orcid_refresh_token = token_entry.get('refresh_token') if token_entry else None
# Per ORCID (https://info.orcid.org/ufaqs/how-can-i-revoke-tokens/), revoking either the access or the
# refresh token invalidates both, so only one needs to be sent. Prefer the access token, since the
# refresh token is optional and may not have been released depending on the user's ORCID privacy settings.
orcid_token = orcid_access_token or orcid_refresh_token
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'
)
Comment thread
Vlad0n20 marked this conversation as resolved.

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()

Expand Down Expand Up @@ -2172,7 +2238,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
Expand Down
26 changes: 26 additions & 0 deletions osf_tests/test_merging_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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()
Expand Down
Loading
Loading