Skip to content
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

Merged
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
23 changes: 23 additions & 0 deletions api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,29 @@ class Meta:
type_ = 'institutions'


class UserIdentitiesSerializer(JSONAPISerializer):
id = IDField(source='_id', read_only=True)
type = TypeField()
external_id = ser.CharField(read_only=True)
status = ser.CharField(read_only=True)

links = LinksField({
'self': 'get_absolute_url',
})

def get_absolute_url(self, obj):
return absolute_reverse(
'users:user-identities-detail',
kwargs={
'user_id': self.context['request'].parser_context['kwargs']['user_id'],
'version': self.context['request'].parser_context['kwargs']['version'],
'identity_id': obj['_id']
}
)

class Meta:
type_ = 'external-identities'

class UserAccountExportSerializer(BaseAPISerializer):
type = TypeField()

Expand Down
2 changes: 2 additions & 0 deletions api/users/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
url(r'^(?P<user_id>\w+)/registrations/$', views.UserRegistrations.as_view(), name=views.UserRegistrations.view_name),
url(r'^(?P<user_id>\w+)/quickfiles/$', views.UserQuickFiles.as_view(), name=views.UserQuickFiles.view_name),
url(r'^(?P<user_id>\w+)/relationships/institutions/$', views.UserInstitutionsRelationship.as_view(), name=views.UserInstitutionsRelationship.view_name),
url(r'^(?P<user_id>\w+)/settings/identities/$', views.UserIdentitiesList.as_view(), name=views.UserIdentitiesList.view_name),
url(r'^(?P<user_id>\w+)/settings/identities/(?P<identity_id>\w+)/$', views.UserIdentitiesDetail.as_view(), name=views.UserIdentitiesDetail.view_name),
url(r'^(?P<user_id>\w+)/settings/export/$', views.UserAccountExport.as_view(), name=views.UserAccountExport.view_name),
url(r'^(?P<user_id>\w+)/settings/deactivate/$', views.UserAccountDeactivate.as_view(), name=views.UserAccountDeactivate.view_name),
]
68 changes: 68 additions & 0 deletions api/users/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
ReadOnlyOrCurrentUserRelationship)
from api.users.serializers import (UserAddonSettingsSerializer,
UserDetailSerializer,
UserIdentitiesSerializer,
UserInstitutionsRelationshipSerializer,
UserSerializer,
UserQuickFilesSerializer,
Expand Down Expand Up @@ -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,
Copy link
Contributor

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

)

serializer_class = UserIdentitiesSerializer

required_read_scopes = [CoreScopes.USER_SETTINGS_READ]
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]})
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: You can use a list comprehension to build identities.


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):
Copy link
Contributor

@pattisdr pattisdr Jul 16, 2018

Choose a reason for hiding this comment

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

Similar comments to UserIdentitiesList, get_object should return the object you're trying to serialize, in this case, the individual external identity, rather than attempting to format the presentation of the data.

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,
Expand Down
109 changes: 109 additions & 0 deletions api_tests/users/views/test_user_external_identities.py
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):
Copy link
Contributor

@pattisdr pattisdr Jul 16, 2018

Choose a reason for hiding this comment

The 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 external_id_provider, type is external_identities, and then you have two attributes that should be nested under attributes. The self link refers to itself for each item in the list, which is the detail link, not the list link.

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):
Copy link
Contributor

Choose a reason for hiding this comment

The 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:
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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'