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 6 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
14 changes: 14 additions & 0 deletions api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,17 @@ def get_absolute_url(self, obj):

class Meta:
type_ = 'institutions'


class UserIdentitiesSerializer(BaseAPISerializer):
Copy link
Contributor

Choose a reason for hiding this comment

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

We follow the JSONAPI specification to format our API responses, and your response here does not follow the spec. (The ticket was a little confusing here, and I can see why it made you think the data should be formatted this way). If you inherit from the JSONAPISerializer instead of BaseAPISerializer a lot of this is taken care of for you.

My recommendation, provided you change your views as well:

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'

data = ser.DictField()
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of defining a data DictField, inherit from the JSONAPISerializer which will automatically place your information inside of a data dict. In this serializer you should define the individual fields you want to display for each identity, which are id, type, external_id, and status. If you are returning a list of all identities, the JSONAPISerializer takes care of returning a list of dictionaries. If you request a single identity, the serializer returns a data dictionary.

links = LinksField({'self': 'get_self_url'})

def get_self_url(self, obj):
Copy link
Contributor

Choose a reason for hiding this comment

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

The self url we want to return here should be the detail url, not the list url.

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

class Meta:
type_ = 'external_identities'
Copy link
Contributor

Choose a reason for hiding this comment

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

In ticket, type is specified as needing a hyphen instead of an underscore. Looks like we are inconsistent as to the format here in other serializers though.

2 changes: 2 additions & 0 deletions api/users/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@
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),
]
64 changes: 64 additions & 0 deletions api/users/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
ReadOnlyOrCurrentUserRelationship)
from api.users.serializers import (UserAddonSettingsSerializer,
UserDetailSerializer,
UserIdentitiesSerializer,
UserInstitutionsRelationshipSerializer,
UserSerializer,
UserQuickFilesSerializer,
Expand Down Expand Up @@ -436,3 +437,66 @@ def perform_destroy(self, instance):
if val['id'] in current_institutions:
user.remove_institution(val['id'])
user.save()


class UserIdentitiesList(JSONAPIBaseView, generics.RetrieveAPIView, UserMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

List views should return a queryset(ideally) or a list of items. In our case, we have to return a list of user identities since this information is stored as a dict on the User. I would inherit from the ListAPIView, and override get_queryset to build the list of items. You are attempting to format the user identities that the user would see under get_object, but this is the serializer's job. Just return the items you want to serialize here.

My recommendation:

class UserIdentitiesList(JSONAPIBaseView, generics.ListAPIView, UserMixin):
    permission_classes = (
        base_permissions.TokenHasScope,
        drf_permissions.IsAuthenticatedOrReadOnly,
        CurrentUser,
    )

    serializer_class = UserIdentitiesSerializer

    required_read_scopes = [CoreScopes.USERS_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]})

        return identities

"""
REMEMBER TO WRITE DEV DOCS!!!
Copy link
Contributor

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 👍

The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/...).
"""
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.USERS_READ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

def get_object(self):
user = self.get_user()

return {'data': user.external_identity, 'self': user, 'type': 'external-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):
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:
return {'data': {identity_id: user.external_identity[identity_id]}, 'self': user}
except KeyError:
raise NotFound('Requested external identity could not be found.')

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()
76 changes: 76 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,76 @@
# -*- 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.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'] == {
'ORCID': {
'0000-0001-9143-4653': 'VERIFIED'
},
'LOTUS': {
'0000-0001-9143-4652': 'LINK'
}
}

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'


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

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'] == {'ORCID': {'0000-0001-9143-4653': 'VERIFIED'}}

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 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_authorized_delete_204(self, app, user, url):
Copy link
Contributor

Choose a reason for hiding this comment

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

Test unauthorized delete as well. I would also add a couple of tests trying to use unauthorized request methods, such as trying to update an identity which should 405.

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