-
Notifications
You must be signed in to change notification settings - Fork 335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540
Changes from 14 commits
49c66ba
ab0d642
5f78e67
aad7e7d
d9155f5
70441aa
fc8a883
68f343b
f562d08
d389848
64bfc9b
5958bed
b010b2c
b85ce4a
c465fbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
ReadOnlyOrCurrentUserRelationship) | ||
from api.users.serializers import (UserAddonSettingsSerializer, | ||
UserDetailSerializer, | ||
UserIdentitiesSerializer, | ||
UserInstitutionsRelationshipSerializer, | ||
UserSerializer, | ||
UserQuickFilesSerializer, | ||
|
@@ -445,6 +446,73 @@ def perform_destroy(self, instance): | |
user.save() | ||
|
||
|
||
class UserIdentitiesList(JSONAPIBaseView, generics.ListAPIView, UserMixin): | ||
""" | ||
The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/external_identities_list). | ||
""" | ||
permission_classes = ( | ||
base_permissions.TokenHasScope, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
CurrentUser, | ||
) | ||
|
||
serializer_class = UserIdentitiesSerializer | ||
|
||
required_read_scopes = [CoreScopes.USER_SETTINGS_READ] | ||
required_write_scopes = [CoreScopes.USER_SETTINGS_WRITE] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||
|
||
view_category = 'users' | ||
view_name = 'user-identities-list' | ||
|
||
# overrides ListAPIView | ||
def get_queryset(self): | ||
user = self.get_user() | ||
identities = [] | ||
for key, value in user.external_identity.iteritems(): | ||
identities.append({'_id': key, 'external_id': value.keys()[0], 'status': value.values()[0]}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: You can use a list comprehension to build |
||
|
||
return identities | ||
|
||
|
||
class UserIdentitiesDetail(JSONAPIBaseView, generics.RetrieveDestroyAPIView, UserMixin): | ||
""" | ||
The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/external_identities_detail). | ||
""" | ||
permission_classes = ( | ||
base_permissions.TokenHasScope, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
CurrentUser, | ||
) | ||
|
||
required_read_scopes = [CoreScopes.USER_SETTINGS_READ] | ||
required_write_scopes = [CoreScopes.USER_SETTINGS_WRITE] | ||
|
||
serializer_class = UserIdentitiesSerializer | ||
|
||
view_category = 'users' | ||
view_name = 'user-identities-detail' | ||
|
||
def get_object(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comments to UserIdentitiesList, |
||
user = self.get_user() | ||
identity_id = self.kwargs['identity_id'] | ||
try: | ||
identity = user.external_identity[identity_id] | ||
except KeyError: | ||
raise NotFound('Requested external identity could not be found.') | ||
|
||
return {'_id': identity_id, 'external_id': identity.keys()[0], 'status': identity.values()[0]} | ||
|
||
def perform_destroy(self, instance): | ||
user = self.get_user() | ||
identity_id = self.kwargs['identity_id'] | ||
try: | ||
user.external_identity.pop(identity_id) | ||
except KeyError: | ||
raise NotFound('Requested external identity could not be found.') | ||
|
||
user.save() | ||
|
||
|
||
class UserAccountExport(JSONAPIBaseView, generics.CreateAPIView, UserMixin): | ||
permission_classes = ( | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
# -*- coding: utf-8 -*- | ||
import pytest | ||
|
||
from osf_tests.factories import AuthUserFactory | ||
from api.base.settings.defaults import API_BASE | ||
|
||
|
||
@pytest.fixture() | ||
def user(): | ||
user = AuthUserFactory() | ||
user.external_identity = { | ||
'ORCID': { | ||
'0000-0001-9143-4653': 'VERIFIED' | ||
}, | ||
'LOTUS': { | ||
'0000-0001-9143-4652': 'LINK' | ||
} | ||
} | ||
user.save() | ||
return user | ||
|
||
|
||
@pytest.fixture() | ||
def unauthorized_user(): | ||
return AuthUserFactory() | ||
|
||
|
||
@pytest.mark.django_db | ||
class TestUserIdentitiesList: | ||
|
||
@pytest.fixture() | ||
def url(self, user): | ||
return '/{}users/{}/settings/identities/'.format(API_BASE, user._id) | ||
|
||
def test_authorized_gets_200(self, app, user, url): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expected response format is incorrect (doesn't adhere to JSONAPI spec). Recommended response: id is |
||
res = app.get(url, auth=user.auth) | ||
assert res.status_code == 200 | ||
assert res.content_type == 'application/vnd.api+json' | ||
assert res.json['data'][0]['attributes']['status'] == 'LINK' | ||
assert res.json['data'][0]['attributes']['external_id'] == '0000-0001-9143-4652' | ||
assert res.json['data'][0]['type'] == 'external-identities' | ||
assert res.json['data'][0]['id'] == 'LOTUS' | ||
|
||
assert res.json['data'][1]['attributes']['status'] == 'VERIFIED' | ||
assert res.json['data'][1]['attributes']['external_id'] == '0000-0001-9143-4653' | ||
assert res.json['data'][1]['type'] == 'external-identities' | ||
assert res.json['data'][1]['id'] == 'ORCID' | ||
|
||
def test_anonymous_gets_401(self, app, url): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add a test for a user trying to access someone else's identities which should 403. |
||
res = app.get(url, expect_errors=True) | ||
assert res.status_code == 401 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
def test_unauthorized_gets_403(self, app, url, unauthorized_user): | ||
res = app.get(url, auth=unauthorized_user.auth, expect_errors=True) | ||
assert res.status_code == 403 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
|
||
@pytest.mark.django_db | ||
class TestUserIdentitiesDetail: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you still add the tests for an identity that 404's and a patch test that 405's? I know that this code works, but it prevents someone down the line from inadvertently inheriting from a new generic view or something and no one catches this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
@pytest.fixture() | ||
def url(self, user): | ||
return '/{}users/{}/settings/identities/ORCID/'.format(API_BASE, user._id) | ||
|
||
@pytest.fixture() | ||
def bad_url(self, user): | ||
return '/{}users/{}/settings/identities/404-CID/'.format(API_BASE, user._id) | ||
|
||
def test_authorized_gets_200(self, app, user, url): | ||
res = app.get(url, auth=user.auth) | ||
assert res.status_code == 200 | ||
assert res.content_type == 'application/vnd.api+json' | ||
assert res.json['data']['attributes']['status'] == 'VERIFIED' | ||
assert res.json['data']['attributes']['external_id'] == '0000-0001-9143-4653' | ||
assert res.json['data']['type'] == 'external-identities' | ||
assert '/v2/users/{}/settings/identities/ORCID/'.format(user._id) in res.json['data']['links']['self'] | ||
|
||
def test_delete_204(self, app, user, url): | ||
res = app.delete(url, auth=user.auth) | ||
assert res.status_code == 204 | ||
|
||
user.refresh_from_db() | ||
assert user.external_identity == { | ||
'LOTUS': { | ||
'0000-0001-9143-4652': 'LINK' | ||
} | ||
} | ||
|
||
def test_anonymous_gets_401(self, app, url): | ||
res = app.get(url, expect_errors=True) | ||
assert res.status_code == 401 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
def test_unauthorized_delete_403(self, app, url, unauthorized_user): | ||
res = app.get(url, auth=unauthorized_user.auth, expect_errors=True) | ||
assert res.status_code == 403 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
def test_bad_request_gets_404(self, app, bad_url): | ||
res = app.get(bad_url, expect_errors=True) | ||
assert res.status_code == 404 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
def test_patch_405(self, app, user, url): | ||
res = app.patch(url, auth=user.auth, expect_errors=True) | ||
assert res.status_code == 405 | ||
assert res.content_type == 'application/vnd.api+json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yes, only logged in user should see their identities