Add new API endpoint to allow for create/edit/delete of tokens by user #648

calvinmclean · 2018-08-13T18:17:31Z

Description

This PR adds an API endpoint that allows users to create/edit/delete named tokens that they can use to access Atmosphere from something like the Atmosphere CLI.
The tokens are a new model that extends the existing cyverse-django-auth tokens and adds a name.
The tokens have a 5 year expiration date.

~~I still need to create the migration for this!~~

Related Troposphere PR

Checklist before merging Pull Requests

New test(s) included to reproduce the bug/verify the feature
Add an entry in the changelog
Documentation created/updated (include links)
If creating/modifying DB models which will contain secrets or sensitive information, PR to clank updating sanitation queries in roles/sanitary-sql-access/templates/sanitize-dump.sh.j2
- It was already in there because the access_token table existed but was empty/unused
Reviewed and approved by at least one other contributor.
New variables supported in Clank

coveralls · 2018-08-15T17:31:33Z

Coverage increased (+0.07%) to 37.363% when pulling 6e80a65 on calvinmclean:api-tokens into 89238ff on cyverse:master.

cdosborn · 2018-08-16T22:41:37Z

Api view needs to return expired tokens. I believe these are filtered out.

calvinmclean · 2018-08-20T22:13:08Z

I got the tests working just a few minutes before I have to leave today so I will polish and commit them when I am in the office on Wednesday!

cdosborn · 2018-08-21T22:29:03Z

Was talking with Tharon, no need to have these tokens expire.

calvinmclean · 2018-08-22T18:30:58Z

Removed token expireTime and finished the tests 👍

cdosborn · 2018-08-22T21:30:42Z

Creation of the d-c-a Token should pass an "issuer"

cdosborn · 2018-08-22T21:43:00Z

core/models/access_token.py

@@ -16,7 +16,7 @@ class Meta:
        app_label = "core"

 def create_access_token(user, token_name=None, token_expire=None, remote_ip=None, issuer=None):
-    token = Token(user=user)
+    token = Token(user=user, issue=issuer)


issue -> issuer

cdosborn · 2018-08-22T21:50:17Z

api/v2/views/access_token.py

+        return qs.filter(only_current_access_tokens())
+
+    def create(self, request):
+        issuer_backend = request.session.get('_auth_user_backend', '').split('.')[-1]


Curious, why are you looking this up in the session?

Issuer may just as well be "Personal-Access-Token"

It is the only way I knew how to get the issuer by copying what is done in auth.py

In this case you're the issuer. Do some searching for where we pass values for issuer in d-c-a, which should show its purpose.

The session is a key value store which is used in web-apps to track a user across multiple stateless requests. It's a bag of properties. If a user submits requests and passes their session as a cookie, we'll know which session they're using. _auth_user_backend is probably some relic of long ago

Ok, I will look into that

cdosborn · 2018-08-22T21:55:15Z

A user shouldn't be allowed to create access tokens with the same name

calvinmclean · 2018-08-22T22:14:40Z

I was able to whip up some changes to disallow creating tokens with the same name. I just need to improve the error message, make some tests, and make the query only look for tokens with the same name AND same user (I didn't implement the user part yet). The change you mentioned in Tharon's PR about having the NotificationController show the error from the API will show the error message I am creating here.

Unfortunately I can't finish this up until Friday. Class is so inconvenient 😢

cdosborn · 2018-08-24T15:46:27Z

I was thinking about this some more. There's nothing technically wrong with a user creating the same token from the api's perspective. Each token has a unique id, and the api response includes said ids. We don't really care if users do this for projects etc.

calvinmclean · 2018-08-24T17:20:47Z

Yeah that was my reasoning behind not enforcing it. But it also makes sense to enforce it because users can get confused if tokens have the same name (but they caused the confusion themselves). So it's a tricky decision to make. However, with projects, the users have other distinguishing information such as the creation date, description, and resources inside the project so it might make sense to enforce unique names with the tokens.

calvinmclean · 2018-08-24T18:06:11Z

In response to "Api view needs to return expired tokens. I believe these are filtered out.":

Should I still do this now that the tokens do not expire?

calvinmclean · 2018-08-24T18:09:24Z

I created the commit for enforcing unique names because I had it mostly finished anyways so now we can look at that when deciding what to do. However, users are still able to rename tokens to have identical names because, to be honest, I am not sure how the editing works on the API side of this. Maybe we need to enforce that on the Troposphere side (in collections or stores)?

Edit: I just did some searching and it looks like the edit is handled by the update() method inherited from rest_framework.viewsets.ModelViewSet so I believe I can override it

cdosborn · 2018-08-24T18:29:24Z

When you're enforcing some guarantee you want to enforce at the lowest level possible, a db constraint. If you can't enforce there, then you enforce at the api level, and hope your logic covers the edge cases (ex. you may have forgotten to replace update logic you mentioned above). The last place to add a constraint is in the ui, which is the weakest place, because no one has to use the frontend and they could just as easily change it to have different behavior.

A db constraint can only exist on a given table. You would have to add a user field to your token, and then you could add unique constraint between (user, name). This would imply that those two columns MUST uniquely identify a record, the db wouldn't allow any inserts/updates/changes which would break that guarantee. This isn't great because the token model has a user. So your access_token.user and access_token.token.user would be redundant and would open up potential for bugs. One path here would be to remove the user field from token, a quick glance showed that it was unnecessary.

Or you could just add it in the api, but none of our other apis do this checking. In each of those scenarios if the user is confused by making a duplicate, they can feel pretty responsible for doing something like that and not blame the application (so the weight isn't that important).

A middle ground solution would be to allow duplicate names, but to show the creation date of the token. With this information users could likely figure out which duplicate was which.

cdosborn · 2018-08-24T18:30:11Z

I don't think we need to worry about this issue. I still have some feedback on your pr i havn't mentioned, and functionally its good for MVP.

cdosborn · 2018-08-24T18:32:34Z

agh accidentally closed

cdosborn · 2018-08-24T18:34:39Z

Re: #648 (comment)
Yes. If the tokens do not expire, it is non-sensical to filter them. If they do expire at some point, we will also want the filter to be removed.

cdosborn · 2018-08-24T18:38:52Z

Another reason to enforce constraints at the database level. The python api code runs in parallel. So the code you added says something like "If the token exists already with this name, then error". However if two requests to create a token with the same name happened in parallel, they would both be created with the same name.

The database constraint would prevent this.

cdosborn · 2018-08-24T18:50:33Z

core/models/access_token.py

+
+class AccessToken(models.Model):
+    """
+    Extend the django_cyverse_auth Token to add a name


This is literally describing what the model is, may explain how this is used in the context of the api.

cdosborn · 2018-08-24T18:53:20Z

core/models/access_token.py

+def create_access_token(user, token_name=None, token_expire=None, remote_ip=None, issuer=None):
+    if AccessToken.objects.filter(name=token_name, token__user_id=user.id):
+        return None
+    token = Token(user=user, issuer=issuer)


This line and one below can be replaced by a single token = Token.objects.create(...)

Would this still check if a token with that name already exists? Or does it rely on the database enforcing that constraint?

I'm referring to line 21, 22 here

Oh okay. To make the name enforced by the database, I can probably add unique=True to the CharField and then I don't need 19 and 20 either

That would mean make the name unique across all tokens, so two users couldn't have token 'foobar'

cdosborn · 2018-08-24T18:54:12Z

core/models/access_token.py

+        app_label = "core"
+
+def create_access_token(user, token_name=None, token_expire=None, remote_ip=None, issuer=None):
+    if AccessToken.objects.filter(name=token_name, token__user_id=user.id):


AccessToken.objects.filter(name=token_name, token__user=user)

cdosborn · 2018-08-24T18:57:02Z

core/models/access_token.py

+        return None
+    token = Token(user=user, issuer=issuer)
+    token.save()
+    access_token, created = AccessToken.objects.update_or_create(token=token, name=token_name)


This may have been a recent change, but you don't want update_or_create. You just want create, a new AccessToken should always be returned, since a new Token is always created.

cdosborn · 2018-08-24T21:01:58Z

core/migrations/replace_0001_initial.py

+# RunPython operations to refer to the local versions:
+# core.migrations.0088_set_project_leaders_and_authors
+# core.migrations.0089_projects_to_project__alter_created_by
+# core.migrations.0092_set_unknown_size_instance_status_history


Would you replace this comment with a short summary of why this migration exists.

cdosborn · 2018-08-24T21:24:27Z

One last thing to look at, try using curl, and submitting gobbledigook:

curl -k -X POST 'https://local.atmo.cloud/api/v2/access_tokens' -H "Content-Type: application/json" -H 'Accept: application/json' -H 'Authorization: Token ...' --data '{ "name": {  "uh-oh": 1 } }'

I believe you're not using your serializer in your create method. You should have a line like serializer.validate(raise_errors=True)

calvinmclean · 2018-08-24T21:52:31Z

I created a validate method in the serializer following django docs

calvinmclean · 2018-08-24T21:53:20Z

Since the validate method returns the valid data, maybe I should use data = AccessTokenSerializer().validate(request.data) in the create method, but it does not seem necessary since the data is not changed at all

cdosborn · 2018-08-24T21:58:21Z

api/v2/views/access_token.py

+        return AccessToken.objects.filter(token__user=self.request.user)
+
+    def create(self, request):
+        AccessTokenSerializer().validate(request.data)


Try replacing this line with:

++ serializer = self.get_serializer(data=request.data) ++ serializer.is_valid(raise_exception=True)

Your class is implementing an AuthModelViewSet, which is based off of rest_framework's ModelViewSet, which provides a default implementation for def create. If you're going to override it, you just need to know what all its supposed to do. It basically takes serializer_class above does some shit and returns a json repsonse from the serializers data. You don't have to implement the serializers validate method. That method is based on the serializers fields (which in turn are based on the fields defined on your model).

In a manage.py shell run:

from api.v2.serializers.details import AccessTokenSerializer AccessTokenSerializer()

You'll see the serializer fields which are automatically populated and used in the serializers default validate method.

A helpful trick when you're replacing a method:

def create(self, request): import ipdb; ipdb.set_trace() super(self.__class__, self).create(request)

That will call the superclass implementation and you can step through and see what it does.

cdosborn · 2018-08-27T17:02:32Z

api/v2/views/access_token.py

@@ -22,7 +22,8 @@ def get_queryset(self):
        return AccessToken.objects.filter(token__user=self.request.user)

    def create(self, request):
-        AccessTokenSerializer().validate(request.data)
+        serializer = self.get_serializer(data=request.data)
+        serializer.is_valid(raise_exception=True)


👍 May seem a tad overkill for so simple an api, but this future proofs as more fields get added

Better to do it right the first time for sure. I appreciate all the feedback, I am learning a lot about APIs!

calvinmclean · 2018-08-27T17:53:08Z

Once this is approved, I would like a chance to rebase and squash some of these commits before merge

cdosborn · 2018-08-27T18:00:41Z

Troposphere expects error responses to be valid json.

For example this is what rest_framework returns when i send gobbledygook:

$ curl -k -X POST 'https://local.atmo.cloud/api/v2/access_tokens' -H "Content-Type: application/json" -H 'Accept: application/json' -H 'Authorization: Token ...' --data '{ {} }'      
{"detail":"JSON parse error - Expecting property name enclosed in double quotes: line 1 column 3 (char 2)"}

A raw string is not technically a json document, and tropo expects to see one. Fixed in a42fee9

calvinmclean · 2018-08-27T18:08:50Z

Oh yeah, there is still one problem: token names can be changed to match existing names without error. Any suggestions to fix this?

cdosborn · 2018-08-27T18:28:13Z

api/tests/v2/test_access_tokens.py

@@ -80,7 +80,7 @@ def test_create_same_name(self):
        force_authenticate(self.create_request, user=self.user)
        response = self.create_view(self.create_request)
        self.assertEquals(response.status_code, 400)
-        self.assertEquals(response.data, "Token with name \"Test Token Creation\" exists.")
+        self.assertEquals(response.data, {'detail': u'Token with name "Test Token Creation" exists.'})


cdosborn · 2018-08-27T18:30:06Z

It would be a little more work, but we could give up on checking if the names are unique, and instead just include a creation date (the serializer would pull this field off the Token)

cdosborn · 2018-08-27T18:30:29Z

The ui could list each token with its creation date, and the issue would no longer exist

cdosborn · 2018-08-27T18:33:40Z

I sort of like that route, because in the absence of an expiration date, it would be nice to show how old a token is

cdosborn · 2018-08-27T18:33:58Z

Another route would be to perform the validation in the update method

cdosborn · 2018-08-27T18:46:27Z

I'll wip this up real quick

cdosborn · 2018-08-27T21:27:47Z

@calvinmclean feel free to rebase, squash, make sure you refetch this branch, its history has been altered

calvinmclean · 2018-08-27T21:44:37Z

Rebase complete for beautiful git history 👍
However, I tried to drop the 'Update CHANGELOG' to be the last commit, but it doesn't show up like that here. It does show as the last commit with git log though

Created a new model, serializer, and APIv2 view for AccessTokens. These changes are intended to be compatible with a new Troposphere feature that allows users to manage tokens from the settings page that allow them to interact with the Atmosphere API.

calvinmclean mentioned this pull request Aug 13, 2018

New Feature - Create and manage "Personal Access Tokens" (API Tokens) from "Settings Page" cyverse/troposphere#789

Merged

4 tasks

calvinmclean force-pushed the api-tokens branch from 1404955 to 1e56cf5 Compare August 15, 2018 17:18

calvinmclean force-pushed the api-tokens branch from 69d7234 to b4fd095 Compare August 16, 2018 17:52

calvinmclean force-pushed the api-tokens branch from 3fb1ad5 to ecd34d2 Compare August 22, 2018 18:16

cdosborn reviewed Aug 22, 2018

View reviewed changes

calvinmclean force-pushed the api-tokens branch from f2906a4 to ecbcd31 Compare August 22, 2018 21:44

cdosborn reviewed Aug 22, 2018

View reviewed changes

cdosborn closed this Aug 24, 2018

cdosborn reopened this Aug 24, 2018

cdosborn suggested changes Aug 24, 2018

View reviewed changes

calvinmclean force-pushed the api-tokens branch 2 times, most recently from ad95e00 to 989ff50 Compare August 24, 2018 20:10

cdosborn reviewed Aug 24, 2018

View reviewed changes

calvinmclean force-pushed the api-tokens branch from 6514f37 to 9d42edf Compare August 24, 2018 21:50

cdosborn reviewed Aug 24, 2018

View reviewed changes

cdosborn reviewed Aug 27, 2018

View reviewed changes

Tharon-C mentioned this pull request Aug 27, 2018

WIP: Minimum Deploy Option cyverse/troposphere#781

Closed

8 tasks

cdosborn reviewed Aug 27, 2018

View reviewed changes

cdosborn force-pushed the api-tokens branch 2 times, most recently from 45d1457 to b68d84a Compare August 27, 2018 21:22

calvinmclean force-pushed the api-tokens branch 2 times, most recently from a753d7a to 2a4b0cc Compare August 27, 2018 21:43

calvinmclean force-pushed the api-tokens branch from 2a4b0cc to 5a3597d Compare August 27, 2018 21:47

Calvin Mclean added 4 commits August 27, 2018 15:12

Add migration for new access_token table

ba354ef

Add tests for the new AccessToken API

5f556f1

Update CHANGELOG

6e80a65

cdosborn force-pushed the api-tokens branch from 5a3597d to 6e80a65 Compare August 27, 2018 22:14

cdosborn approved these changes Aug 27, 2018

View reviewed changes

cdosborn merged commit 2ef315e into cyverse:master Aug 27, 2018

Add new API endpoint to allow for create/edit/delete of tokens by user #648

Add new API endpoint to allow for create/edit/delete of tokens by user #648

Conversation

calvinmclean commented Aug 13, 2018 • edited Loading

Description

Checklist before merging Pull Requests

coveralls commented Aug 15, 2018 • edited Loading

cdosborn commented Aug 16, 2018

calvinmclean commented Aug 20, 2018

cdosborn commented Aug 21, 2018

calvinmclean commented Aug 22, 2018

cdosborn commented Aug 22, 2018

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

cdosborn commented Aug 22, 2018

calvinmclean commented Aug 22, 2018

cdosborn commented Aug 24, 2018

calvinmclean commented Aug 24, 2018

calvinmclean commented Aug 24, 2018

calvinmclean commented Aug 24, 2018 • edited Loading

cdosborn commented Aug 24, 2018

cdosborn commented Aug 24, 2018 • edited Loading

cdosborn commented Aug 24, 2018

cdosborn commented Aug 24, 2018

cdosborn commented Aug 24, 2018

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

cdosborn Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdosborn commented Aug 24, 2018

calvinmclean commented Aug 24, 2018

calvinmclean commented Aug 24, 2018

cdosborn Aug 24, 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

calvinmclean commented Aug 27, 2018

cdosborn commented Aug 27, 2018 • edited Loading

calvinmclean commented Aug 27, 2018

Choose a reason for hiding this comment

cdosborn commented Aug 27, 2018

cdosborn commented Aug 27, 2018

cdosborn commented Aug 27, 2018

cdosborn commented Aug 27, 2018

cdosborn commented Aug 27, 2018

cdosborn commented Aug 27, 2018

calvinmclean commented Aug 27, 2018

calvinmclean commented Aug 13, 2018 •

edited

Loading

coveralls commented Aug 15, 2018 •

edited

Loading

calvinmclean commented Aug 24, 2018 •

edited

Loading

cdosborn commented Aug 24, 2018 •

edited

Loading

cdosborn Aug 24, 2018 •

edited

Loading

cdosborn Aug 24, 2018 •

edited

Loading

cdosborn commented Aug 27, 2018 •

edited

Loading