[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540

Johnetordoff · 2018-07-12T16:33:10Z

Purpose

As part of out on going Emberification, we our exposing the external identity information to our V2 API.

Changes

Adds UserIdentitiesDetail view
Adds UserIdentitiesList view
Adds UserIdentitiesSerializer
Adds tests

QA Notes

Go to a the identitites endpoint such as http://localhost:8000/v2/users/<user_id>/settings/identities/ check to make sure it's cool
Check the detail make sure it's cool too at http://localhost:8000/v2/users/<user_id>/settings/identities/<identity_id> like http://localhost:8000/v2/users/<user_id>/settings/identities/ORCID/ is probably the only valid one for now.
With the proper creds issue a DELETE against the http://localhost:8000/v2/users/<user_id>/settings/identities/<identity_id> make sure its is gone.

Documentation

Docs will be add when this gets approved.

Side Effects

None that I know of.

Ticket

https://openscience.atlassian.net/projects/PLAT/issues/PLAT-931

… into get-delete-external-ids * 'master' of https://github.com/CenterForOpenScience/osf.io: (27 commits) Update entity id / login url for IIT Add institution Illinois Institute of Technology (IIT) Update CHANGELOG, package.json Allow "upload" action to registrations being archived Fix institutions order in `scripts/populate_institutions.py` Add new institution CMU to test server only Fix None subjects bug Add 'provider' to ES CGM docs fix prob fail for TestNodeDetail.test_node_shows_wiki_relationship_based_on_disabled_status_and_version Add SearchParser - update perms and views Add test for expected search POST behavior Remove deprecated kwarg -Breaking change from "master" Update changed method signatures -Broke with "master" changes Add merge migration Remove duplicative Addr/Privacy info -Already in base template -Also fix <br/> tag Add form validation for asset naming conflicts Include comment in "edit-comment" email Add Choices to ProviderAssetFile.name Add merge migration Prevent outgoing blob requests when getting URLs ...

into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: Integrate django-silk for profiling avoid failure on coveralls client add code coverage to tests

pattisdr

Hey @Johnetordoff, several changes requested, I went ahead and made a PR to your branch Johnetordoff#5 since I'd tested a lot of this myself locally. That has a lot of what I mentioned here, except for tests.

pattisdr · 2018-07-16T16:57:49Z

api/users/views.py

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


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

pattisdr · 2018-07-16T17:00:48Z

api/users/views.py

+    view_category = 'users'
+    view_name = 'user-identities-detail'
+
+    def get_object(self):


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.

pattisdr · 2018-07-16T17:05:59Z

api/users/serializers.py

@@ -252,3 +252,17 @@ def get_absolute_url(self, obj):

    class Meta:
        type_ = 'institutions'
+
+
+class UserIdentitiesSerializer(BaseAPISerializer):


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'

pattisdr · 2018-07-16T17:09:12Z

api/users/serializers.py

+
+
+class UserIdentitiesSerializer(BaseAPISerializer):
+    data = ser.DictField()


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.

pattisdr · 2018-07-16T17:09:32Z

api/users/serializers.py

+    data = ser.DictField()
+    links = LinksField({'self': 'get_self_url'})
+
+    def get_self_url(self, obj):


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

pattisdr · 2018-07-16T17:17:18Z

api_tests/users/views/test_user_external_identities.py

+        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):


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.

pattisdr · 2018-07-16T17:18:02Z

api_tests/users/views/test_user_external_identities.py

+        assert res.status_code == 401
+        assert res.content_type == 'application/vnd.api+json'
+
+    def test_authorized_delete_204(self, app, user, url):


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.

pattisdr · 2018-07-16T17:28:08Z

api/users/serializers.py

+        })
+
+    class Meta:
+        type_ = 'external_identities'


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.

pattisdr · 2018-07-16T17:30:20Z

api/users/views.py

+    permission_classes = (
+        base_permissions.TokenHasScope,
+        drf_permissions.IsAuthenticatedOrReadOnly,
+        CurrentUser,


👍 Yes, only logged in user should see their identities

pattisdr · 2018-07-16T17:49:26Z

api/users/views.py

+
+    serializer_class = UserIdentitiesSerializer
+
+    required_read_scopes = [CoreScopes.USERS_READ]


Wondering if we should define more specific scopes for user settings, i.e. CoreScopes.USER_SETTINGS_READ, CoreScopes.USER_SETTINGS_WRITE.

Agreed. I'll plan to merge #8527 first so the CoreScopes added in that PR can be used here.

coveralls · 2018-07-16T22:09:21Z

Pull Request Test Coverage Report for Build 30557

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at ?%

Totals
Change from base Build 30553:	0.0%
Covered Lines:
Relevant Lines:	0

💛 - Coveralls

pattisdr

Just a couple little things @Johnetordoff !

pattisdr · 2018-07-17T19:05:56Z

api_tests/users/views/test_user_external_identities.py

+
+    def test_authorized_gets_200(self, app, user, url):
+        res = app.get(url, auth=user.auth)
+        print res.json['data']


Remove print statement.

pattisdr · 2018-07-17T19:06:32Z

api_tests/users/views/test_user_external_identities.py

+        print res.json['data']
+        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'}


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'

pattisdr · 2018-07-17T19:37:40Z

api_tests/users/views/test_user_external_identities.py

+
+
+@pytest.mark.django_db
+class TestUserIdentitiesDetail:


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.

pattisdr · 2018-07-17T19:40:44Z

api_tests/users/views/test_user_external_identities.py

+        res = app.get(url, auth=user.auth)
+        assert res.status_code == 200
+        assert res.content_type == 'application/vnd.api+json'
+        assert res.json['data'] == {


Prefer comparing individual attributes instead of whole dictionaries for readability.

pattisdr · 2018-07-19T00:17:10Z

Thanks for the new tests @Johnetordoff. Looks good.

caseyrollins

Just the smallest of changes requested. Looks good, @Johnetordoff!

caseyrollins · 2018-07-23T20:53:36Z

api_tests/users/views/test_user_external_identities.py

+        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 res.json['data']['links']['self'] == 'http://localhost:8000/v2/users/{}/settings/identities/ORCID/'.format(user._id)


Could you change this to assert that /v2/users/{}/settings/identities/ORCID/ is in the self link? We don't want tests to be dependent on a specific port.

caseyrollins · 2018-07-23T21:02:46Z

api/users/views.py

+
+    serializer_class = UserIdentitiesSerializer
+
+    required_read_scopes = [CoreScopes.USERS_READ]


Agreed. I'll plan to merge #8527 first so the CoreScopes added in that PR can be used here.

caseyrollins · 2018-07-23T21:04:40Z

api/users/views.py

+
+class UserIdentitiesList(JSONAPIBaseView, generics.ListAPIView, UserMixin):
+    """
+    REMEMBER TO WRITE DEV DOCS!!!


This implementation is good to go. Go ahead and document 👍

@caseyrollins

into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: (98 commits) Bump version and update changelog fix tests for non-dict based client attributes Update gitlab serializers for fangorn config js Add enforce_csrf waffle switch so that this can be merged earlier Update markdown-it-video with infinite loop fix Fix indentation Improve tests by using user cookie method and faster user creation Tests to ensure API requests with Session auth fail without CSRF protection Travis and Test Speed ups Rename --quick to --all Don't transact to avoid deadlocks Add script to fix poor mapping logic -h/t @caseyrollins Add same enforce_csrf method as drf's SessionAuthentication replace bad reverse migration with warning Use timezone.now() when checking for failed registrations Remove duplicate psyarxiv custom subject. Use fork of celery 4.2.1 with celery/celery#4839 merged Upgrade celery and kombu Set TIME_ZONE in settings add scheme to match UI ...

… into get-delete-external-ids * 'master' of https://github.com/CenterForOpenScience/osf.io:

into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: fix mocking for gitlab

caseyrollins · 2018-07-25T15:40:52Z

@Johnetordoff, can you go ahead and get this PR up to date with develop and use the USER_SETTINGS_READ and USER_SETTINGS_WRITE scopes added in #8527?

into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: Allow removing stuck registrations with addons Add write scopes instead of read scopes to write-only user endpoints. Add USER_SETTINGS scopes, mock send_email in throttling tests, and move user fixtures out so they can be shared between tests. Handle account export and deactivation requests in APIv2. Signed-off-by: John Tordoff <john@cos.io> # Conflicts: # api/users/serializers.py # api/users/urls.py # api/users/views.py

Signed-off-by: John Tordoff <john@cos.io>

caseyrollins · 2018-07-25T17:42:25Z

api/users/views.py

+    serializer_class = UserIdentitiesSerializer
+
+    required_read_scopes = [CoreScopes.USER_SETTINGS_READ]
+    required_write_scopes = [CoreScopes.USER_SETTINGS_WRITE]


Should be CoreScopes.NULL since this endpoint is read-only

Signed-off-by: John Tordoff <john@cos.io>

sloria

Just one non-blocking nit. This is good to merge.

sloria · 2018-07-25T20:45:58Z

api/users/views.py

+        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]})


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

Johnetordoff added 4 commits July 12, 2018 09:25

add GET/DELETE endpoint for external identities with tests

49c66ba

add proper permissions

5f78e67

Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io

aad7e7d

into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: Integrate django-silk for profiling avoid failure on coveralls client add code coverage to tests

pattisdr suggested changes Jul 16, 2018

View reviewed changes

Make UserIdentities responses adhere to JSONAPI.

d9155f5

pattisdr mentioned this pull request Jul 16, 2018

Feature/Make User Identities responses adhere to JSONAPI [PLAT-931] Johnetordoff/osf.io#5

Merged

add and improve tests

70441aa

Johnetordoff force-pushed the get-delete-external-ids branch from ad0be16 to 70441aa Compare July 16, 2018 20:36

pattisdr reviewed Jul 17, 2018

View reviewed changes

clean up test and add more

fc8a883

Johnetordoff force-pushed the get-delete-external-ids branch from 773dbe1 to fc8a883 Compare July 18, 2018 12:44

caseyrollins suggested changes Jul 23, 2018

View reviewed changes

caseyrollins reviewed Jul 23, 2018

View reviewed changes

Johnetordoff mentioned this pull request Jul 24, 2018

[PLAT-931] Add External Identities endpoints to docs CenterForOpenScience/developer.osf.io#25

Merged

Johnetordoff added 5 commits July 24, 2018 11:10

update docstrings for new dev docs

f562d08

fix external identities test link

d389848

Merge branch 'master' of https://github.com/CenterForOpenScience/osf.io…

64bfc9b

… into get-delete-external-ids * 'master' of https://github.com/CenterForOpenScience/osf.io:

Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io

5958bed

into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: fix mocking for gitlab

Johnetordoff added 2 commits July 25, 2018 12:23

change scope to user_settings_read/user_settings_write

b85ce4a

Signed-off-by: John Tordoff <john@cos.io>

caseyrollins reviewed Jul 25, 2018

View reviewed changes

change write scope to null user external identities list

c465fbf

Signed-off-by: John Tordoff <john@cos.io>

caseyrollins approved these changes Jul 25, 2018

View reviewed changes

caseyrollins changed the title ~~[PLAT-931] Create/Delete User External Identities in APIv2~~ [PLAT-931] Retrieve/Delete User External Identities in APIv2 Jul 25, 2018

sloria approved these changes Jul 25, 2018

View reviewed changes

sloria merged commit eb482b1 into CenterForOpenScience:develop Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540

[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540

Johnetordoff commented Jul 12, 2018

pattisdr left a comment •

edited

Loading

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018 •

edited

Loading

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

pattisdr Jul 16, 2018

caseyrollins Jul 23, 2018

coveralls commented Jul 16, 2018 •

edited

Loading

pattisdr left a comment

pattisdr Jul 17, 2018

pattisdr Jul 17, 2018

pattisdr Jul 17, 2018

Johnetordoff Jul 17, 2018

pattisdr Jul 17, 2018

pattisdr commented Jul 19, 2018

caseyrollins left a comment

caseyrollins Jul 23, 2018

caseyrollins Jul 23, 2018

caseyrollins Jul 23, 2018

caseyrollins commented Jul 25, 2018

caseyrollins Jul 25, 2018

sloria left a comment

sloria Jul 25, 2018



		class UserIdentitiesSerializer(BaseAPISerializer):
		data = ser.DictField()


		serializer_class = UserIdentitiesSerializer

		required_read_scopes = [CoreScopes.USERS_READ]

[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540

[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540

Conversation

Johnetordoff commented Jul 12, 2018

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

pattisdr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 16, 2018 • edited Loading

Pull Request Test Coverage Report for Build 30557

💛 - Coveralls

pattisdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr commented Jul 19, 2018

caseyrollins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseyrollins commented Jul 25, 2018

Choose a reason for hiding this comment

sloria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr left a comment •

edited

Loading

pattisdr Jul 16, 2018 •

edited

Loading

coveralls commented Jul 16, 2018 •

edited

Loading