-
Notifications
You must be signed in to change notification settings - Fork 327
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 8 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 |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
ReadOnlyOrCurrentUserRelationship) | ||
from api.users.serializers import (UserAddonSettingsSerializer, | ||
UserDetailSerializer, | ||
UserIdentitiesSerializer, | ||
UserInstitutionsRelationshipSerializer, | ||
UserSerializer, | ||
UserQuickFilesSerializer, | ||
|
@@ -436,3 +437,72 @@ def perform_destroy(self, instance): | |
if val['id'] in current_institutions: | ||
user.remove_institution(val['id']) | ||
user.save() | ||
|
||
|
||
class UserIdentitiesList(JSONAPIBaseView, generics.ListAPIView, UserMixin): | ||
""" | ||
REMEMBER TO WRITE DEV DOCS!!! | ||
The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/...). | ||
""" | ||
permission_classes = ( | ||
base_permissions.TokenHasScope, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
CurrentUser, | ||
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. 👍 Yes, only logged in user should see their identities |
||
) | ||
|
||
serializer_class = UserIdentitiesSerializer | ||
|
||
required_read_scopes = [CoreScopes.USERS_READ] | ||
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. Wondering if we should define more specific scopes for user settings, i.e. CoreScopes.USER_SETTINGS_READ, 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. Agreed. I'll plan to merge #8527 first so the CoreScopes added in that PR can be used here. |
||
required_write_scopes = [CoreScopes.NULL] | ||
|
||
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): | ||
""" | ||
REMEMBER TO WRITE DEV DOCS!!! | ||
The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/...). | ||
""" | ||
permission_classes = ( | ||
base_permissions.TokenHasScope, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
CurrentUser, | ||
) | ||
|
||
required_read_scopes = [CoreScopes.USERS_READ] | ||
required_write_scopes = [CoreScopes.USERS_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() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
# -*- 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) | ||
print res.json['data'] | ||
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. Remove print statement. |
||
assert res.status_code == 200 | ||
assert res.content_type == 'application/vnd.api+json' | ||
assert res.json['data'][0]['attributes'] == {u'status': u'LINK', u'external_id': u'0000-0001-9143-4652'} | ||
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. You could define data = res.json['data']. Also, for all these tests, I prefer comparing individual attributes instead of comparing dictionaries for readability. assert data[0]['attributes']['status'] == 'LINK' |
||
assert res.json['data'][0]['type'] == 'external-identities' | ||
assert res.json['data'][0]['id'] == 'LOTUS' | ||
|
||
assert res.json['data'][1]['attributes'] == {u'status': u'VERIFIED', u'external_id': u'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) | ||
|
||
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'] == { | ||
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. Prefer comparing individual attributes instead of whole dictionaries for readability. |
||
u'attributes': { | ||
u'external_id': u'0000-0001-9143-4653', | ||
u'status': u'VERIFIED' | ||
}, | ||
u'id': u'ORCID', | ||
u'links': { | ||
u'self': u'http://localhost:8000/v2/users/{}/settings/identities/ORCID/'.format(user._id) | ||
}, | ||
u'type': u'external-identities' | ||
} | ||
|
||
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 permission test for someone trying to access someone else's identity. Also a test where the identify doesn't exist that should 404. |
||
res = app.get(url, expect_errors=True) | ||
assert res.status_code == 401 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
def test_no_creds_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_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' |
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.
This implementation is good to go. Go ahead and document 👍