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

Tharon-C · 2018-08-13T17:17:00Z

Create and manage "Personal Access Tokens" (API Tokens) from "Settings Page"

This PR is dependent on changes to Atmosphere API cyverse/atmosphere#648

This feature adds a section to the advanced settings page for creating and managing "Personal Access Tokens" or "API Auth Tokens" used to give other applications like the CLI access to the user's account.

This uses a new endpoint "<root>/access_tokens" and so a new store, actions, model, and collection have been added to the data layer.

Actions the user can take:

Create
Edit Name
Delete

Actions are submitted through modals:

APITokenCreate (two views)

Assign a name to Token

Success and Copy Token Hash

Transition, Assign and submit -> Success

APITokenEdit

APITokenDelete

Checklist before merging Pull Requests

Fix APITokenDelete modal icon size to flex: "1 0 24px".
Add an entry in the changelog
Documentation created/updated (include links)
Reviewed and approved by at least one other contributor.

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

cyverse/atmosphere#648

calvinmclean · 2018-08-13T20:30:17Z

I am testing how this works if the Atmosphere API is unreachable. When a token is being created, it will fail and the user will see that it is removed from the list, but the modal will stay open and it will keep saying 'Creating...'

I am experimenting with passing two callback functions to the create action, one for failure and one for success. Initially I just made the failure one change the state to not show creating anymore, and it worked out well. It just returned the user to the modal's initial state. Should we add an error message that shows up as part of the modal? Or we could use a NotificationController like we do with the icon select and guacamole color select

here is what I experimented with: calvinmclean@2983c70

If the Atmosphere API fails to create a token, the modal would still act as if it was trying to create the token. Now, it will go back to the initial state of the modal (while still showing the entered name), and will present a notification with an error message.

calvinmclean · 2018-08-13T21:15:00Z

Also, should we display the expireTime in the UI? The Atmosphere API makes it available.

cdosborn · 2018-08-15T18:28:05Z

Pr is failing the formatter
If you want to level up your git fu:
git rebase origin/master --exec 'npm run format -- -l'

This will replay your pr on top of origin/master, and after each commit is brought over, it will be tested with the formatter. If the formatter fails, it'll stop to let you amend the commit, then you can continue rebasing.

Tharon-C · 2018-08-15T19:08:08Z

Thanks, there are some unused variables. Am building with the backend changes now. After fixing the lint errors and updating change log I'll remove the WIP status for review.

…

On Wed, Aug 15, 2018, 11:28 AM Connor Osborn ***@***.***> wrote: Pr is failing the formatter — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#789 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHBmws_8em6tqobt1nAdpOJ1a3RjuvOPks5uRGg2gaJpZM4V65cg> .

cdosborn · 2018-08-16T22:46:18Z

My thought is that we need to include the expiration time (and it should probably be 6mo). If it expired w/o the user knowing, they would experience this currently as the token just disappearing from their settings page. (left a comment on atmo pr, because it doesn't include expired tokens in api response).

cdosborn · 2018-08-16T21:53:30Z

troposphere/static/js/actions/APITokenActions.js

+import Utils from "./Utils";
+
+export default {
+    create: ({name, atmo_user}, successCallback, failCallback) => {


Simplify this signature to

create(tokenName, user) => Promise

Since create is returning a promise, anyone who has the promise can promise.then(successCallback) or promise.catch(failCallback)

cdosborn · 2018-08-16T21:55:19Z

troposphere/static/js/actions/APITokenActions.js

+export default {
+    create: ({name, atmo_user}, successCallback, failCallback) => {
+        if (!name) throw new Error("Missing Token name");
+        if (!atmo_user) throw new Error("Missing Token author");


cdosborn · 2018-08-16T22:33:20Z

troposphere/static/js/components/modals/api_token/APITokenCreate.jsx

+            errorMsg: "",
+            name: this.props.name || "",
+            successView: false
+        };


all state needs to be included here

cdosborn · 2018-08-16T22:34:00Z

troposphere/static/js/components/modals/api_token/APITokenCreate.jsx

+    mixins: [BootstrapModalMixin],
+    propTypes: {
+        user: React.PropTypes.number.isRequired
+    },


all props need to be included here

In getInitialState we were assigning to name the name prop and letting in fall through to empty string if not defined name: name || "" . I removed the use of the name prop from Create modal as it was left over from when the Edit modal and Create modal were sharing code. Since separating these experiences into their own files we no longer need a name prop on the Create modal.

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

troposphere/static/js/components/modals/api_token/APITokenCreate.jsx

+        });
+        actions.APITokenActions.create(
+            attributes,
+            this.successCallback,


w.r.t to my prior comment,

let promise = actions.APITokenActions.create(...); promise.then(...); promise.catch(...):

I'm having trouble getting this interface to work in a way that doesn't feel kludgy. Could you elaborate? The model returned doesn't seem to be a promise unless I make it one in the action and that is where things start to get awkward. I do think it is better than using callbacks just not sure the best way to do it.

I can do let promise = Promise.resolve(...) here but then I don't get the server response as an argument.

What does actions.APITokenActions.create return?

cdosborn · 2018-08-16T22:54:01Z

troposphere/static/js/components/modals/api_token/APITokenDelete.jsx

+        APITokenStore.remove(token);
+        token.destroy({
+            success: function() {
+                APITokenStore.emitChange();


emitChange is a store implementation detail, nothing outside the store should call this. It's an implementation detail, because its part of the inner-mechanics of how the store works that no user of the store need know about.

Alternatively you can dispatch to the store, and let the store do store stuff

cdosborn · 2018-08-16T22:55:39Z

troposphere/static/js/components/settings/advanced/TokenListView.jsx

+        };
+    },
+
+    updateState() {


cdosborn · 2018-08-16T22:56:36Z

troposphere/static/js/components/settings/advanced/TokenListView.jsx

+    render() {
+        let {APITokenStore} = this.props.subscriptions,
+            profile = this.state.profile,
+            api_token = APITokenStore.getAll();


camelCase, and variable doesn't make sense for what should be a collection

cdosborn · 2018-08-16T22:58:01Z

troposphere/static/js/components/settings/advanced/TokenListView.jsx

+            {
+                token
+            },
+            () => {}


You don't need to pass empty callbacks, leave off that arg

cdosborn · 2018-08-16T22:59:00Z

troposphere/static/js/components/settings/advanced/TokenListView.jsx

+                        <tbody>
+                            {api_token
+                                ? api_token.map(this.renderTokenRow)
+                                : []}


Suggestion: (tokens || []).map(this.renderTokenRow)

Tharon-C · 2018-08-17T19:22:07Z

RE: Showing the expiration date
I had made mockups and sought approval from Edwin on the feature before starting work. The lack of expiration date did come up but was decided to treat them as persistent for now as an MVP.

We are modeling the functionality of GH's implementation of "Personal Access Tokens".

cdosborn · 2018-08-17T20:29:29Z

troposphere/static/js/actions/APITokenActions.js

+        // Add token optimistically
+        Utils.dispatch(APITokenConstants.ADD_TOKEN, {apiToken});
+
+        apiToken


Change this method so that a promise is returned.

let promise = apiToken.save() promise.done(...) promise.fail(...) return promise

This does prove that the model is indeed a promise and removes the then is not a function error I was getting before. then however isn't getting called... I'll keep digging.

The model is not a promise

All I can think of is...

https://www.youtube.com/watch?v=9iSlPoQm2XY

cdosborn · 2018-08-21T22:14:05Z

Okay, then lets not have the tokens expire, it was just an edge case I was considering.

cdosborn · 2018-08-22T20:05:13Z

Optimistic logic isn't ideal. When the user hits create, the token is added, and shows in the table, but the modal continues to show in progress. So while the table add is optimistic the modal is not optimistic. We do not need to optimistically add the token, since the use will have to wait to copy the token key.

cdosborn · 2018-08-22T20:08:07Z

troposphere/static/js/actions/APITokenActions.js

+            Utils.dispatch(APITokenConstants.REMOVE_TOKEN, {apiToken});
+            NotificationController.error(
+                "Error creating token.",
+                "Your login might be expired. If you continue to see this error " +


Notification controller needs to include the error from the create response. Take a look at the arg to catch.

I can add this change

Tharon-C · 2018-08-22T21:03:02Z

RE: Optimistic logic
Yeah, I can agree with that.

cdosborn · 2018-08-22T22:31:16Z

user_id attribute doesn't need to be included in the create call. Since the user must be authenticated (i.e. the api knows exactly who is trying to create the token) to create the token, the api already knows who to create the token for.

Tharon-C · 2018-08-23T18:57:43Z

RE: user_id
OK, I removed the user from the api request and "all related things"
Changes to Atmo are required calvinmclean/atmosphere#1

Tharon-C · 2018-08-27T17:34:15Z

All requested changes have been made. Once cyverse/atmosphere#648 has been merged this can be as well. Thanks @cdosborn and @calvinmclean

Since these tokens do not expire, it's convenient to at least see when the tokens were created. Also, this additional info assists the edge case where someone creates two access tokens with the same name.

cdosborn · 2018-08-27T20:58:54Z

Added "Issued" column re: cyverse/atmosphere#648 (comment)

cdosborn · 2018-08-27T21:04:11Z

I tested against an api throwing errors in create/update/delete

Tharon-C · 2018-08-27T21:23:10Z

Thanks @cdosborn the updates look good!

Tharon-C and others added 4 commits August 9, 2018 18:35

Stub out API Token feature

0ee80d9

Enable APIToken viewing and creating

722cec8

Enable APIToken editing and deleting

4930e1c

Return token to the user without using setTimeout

a0f4060

calvinmclean mentioned this pull request Aug 13, 2018

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

Merged

6 tasks

Tharon-C added 2 commits August 15, 2018 20:48

Fix lint errors and format

fa2b6f8

Update CHANGELOG.md

8c5f1a2

Tharon-C changed the title ~~WIP: New Feature - Create and manage "Personal Access Tokens" (API Tokens) from "Settings Page"~~ New Feature - Create and manage "Personal Access Tokens" (API Tokens) from "Settings Page" Aug 15, 2018

hide APITokenCreate modal if error

42a8db5

cdosborn suggested changes Aug 16, 2018

View reviewed changes

refactor for readability and code style

d26a397

Use actions over accessing store directly

dce7e13

cdosborn reviewed Aug 17, 2018

View reviewed changes

Use Promise over Callback

5dfcbf8

cdosborn reviewed Aug 22, 2018

View reviewed changes

Don't update optimistically

f96c654

Don't send user in request

1f87a6b

cdosborn added 6 commits August 27, 2018 11:14

If optimistic delete fails, re-add the token

267e5f5

Prefer browser Promise api, consistently return promises

badae22

Prefer function expressions

35dab4a

Use displayError utilities to handle error variations

0e886f7

Start imports with external libraries separated by a new line

c9117f9

Add issued column to access token table

7c9e821

Since these tokens do not expire, it's convenient to at least see when the tokens were created. Also, this additional info assists the edge case where someone creates two access tokens with the same name.

cdosborn approved these changes Aug 27, 2018

View reviewed changes

cdosborn merged commit 657ddec into cyverse:master Aug 27, 2018

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

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

Conversation

Tharon-C commented Aug 13, 2018 • edited Loading

Create and manage "Personal Access Tokens" (API Tokens) from "Settings Page"

Actions the user can take:

Actions are submitted through modals:

APITokenCreate (two views)

Assign a name to Token

Success and Copy Token Hash

Transition, Assign and submit -> Success

APITokenEdit

APITokenDelete

Checklist before merging Pull Requests

calvinmclean commented Aug 13, 2018

calvinmclean commented Aug 13, 2018 • edited Loading

calvinmclean commented Aug 13, 2018 • edited Loading

cdosborn commented Aug 15, 2018 • edited Loading

Tharon-C commented Aug 15, 2018 via email

cdosborn commented Aug 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

Tharon-C Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tharon-C Aug 17, 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

cdosborn Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Tharon-C commented Aug 17, 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

cdosborn commented Aug 21, 2018

cdosborn commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tharon-C commented Aug 22, 2018

cdosborn commented Aug 22, 2018 • edited Loading

Tharon-C commented Aug 23, 2018

Tharon-C commented Aug 27, 2018

cdosborn commented Aug 27, 2018 • edited Loading

cdosborn commented Aug 27, 2018

Tharon-C commented Aug 27, 2018

Tharon-C commented Aug 13, 2018 •

edited

Loading

calvinmclean commented Aug 13, 2018 •

edited

Loading

calvinmclean commented Aug 13, 2018 •

edited

Loading

cdosborn commented Aug 15, 2018 •

edited

Loading

cdosborn commented Aug 16, 2018 •

edited

Loading

Tharon-C Aug 17, 2018 •

edited

Loading

Tharon-C Aug 17, 2018 •

edited

Loading

cdosborn Aug 16, 2018 •

edited

Loading

cdosborn commented Aug 22, 2018 •

edited

Loading

cdosborn commented Aug 27, 2018 •

edited

Loading