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

Add ui for managing api clients #888

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@JackTLi
Copy link
Member

JackTLi commented Apr 4, 2019

This adds a UI for managing api clients

image

image

image

@JackTLi JackTLi force-pushed the api-client-provisioning branch from 5c774a0 to a368972 Apr 4, 2019

@JackTLi JackTLi requested review from DazWorrall, esnunes and casperisfine Apr 4, 2019

@DazWorrall
Copy link
Member

DazWorrall left a comment

Rubocop errors look legit, but this LGTM - far nicer than needing to boot a Rails console!

@casperisfine

This comment has been minimized.

Copy link
Contributor

casperisfine commented Apr 5, 2019

Wow, this has been a long awaited feature, great idea.

Before I actually review the code, here's a little questionnaire to help you make sure every concern is accounted for:

  • ApiClient have a creator field. Should this UI be global, hence list all clients from everyone, or scoped by user ?
  • Should the auth token be displayed after the initial confirmation page ? If yes, should we use some kind of reveal button ?
  • Shipit tokens are a bit special as in we can't regenerate them, they are derived from the record id using a private key. Hence to regenerate a token we need to delete and recreate the record. Is this something we'd like to facilitate ?
  • ApiClient can optionally be scoped to a single stack via the stack_id attributes. Is that something we want to add to the UI.

Feel free to dismiss some of these concerns as suitable for a followup, but just want to makesure everything is accounted for.

@JackTLi

This comment has been minimized.

Copy link
Member Author

JackTLi commented Apr 5, 2019

Thanks @casperisfine ! Appreciate you prompting the discussion

ApiClient have a creator field. Should this UI be global, hence list all clients from everyone, or scoped by user ?

Good question, I thought about this and I believe a global list is better, since it makes it easier for collaboration or helping someone out (e.g. they are missing a permission on an existing key that one of their app is using). A filter or search functionality here could be pretty useful though 🤔

Should the auth token be displayed after the initial confirmation page ? If yes, should we use some kind of reveal button ?

I agree on the reveal button. I was thinking about adding that along with some auditing as well as a followup. Both for viewing the key, and also for when the key was last used.

Shipit tokens are a bit special as in we can't regenerate them, they are derived from the record id using a private key. Hence to regenerate a token we need to delete and recreate the record. Is this something we'd like to facilitate ?

I was thinking about adding a revoke (soft-delete) feature as a followup-item. Assuming we could "unrevoke", I think this would have the same effect?

ApiClient can optionally be scoped to a single stack via the stack_id attributes. Is that something we want to add to the UI.

Yeah that's a good point, I can probably patch that into this PR 👍

@casperisfine

This comment has been minimized.

Copy link
Contributor

casperisfine commented Apr 5, 2019

I was thinking about adding a revoke (soft-delete) feature as a followup-item. Assuming we could "unrevoke", I think this would have the same effect?

Not exactly the same effect. If you revoke / unrevoke you are making an existing token unusable.

Regenerating mean getting a new, valid token, while revoking the old one.

So basically for shipit it would mean creating a new record, will all identical attributes (except id), and then deleting the old one (or soft delete but WTV).

@JackTLi

This comment has been minimized.

Copy link
Member Author

JackTLi commented Apr 5, 2019

Ahh I see what you mean. I feel like we don't need a specific regenerate feature.

The case I'm thinking of in my head is if a key needs to be rotated. In most key rotation cases, both keys need to be supported for a period of time anyways. So for a regeneration of key 1, key 2 needs to be created first, app moves to use key 2, and then key 1 is deleted, which lends itself better to a manual process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.