Skip to content

Conversation

@neelvirdy
Copy link
Contributor

@neelvirdy neelvirdy commented Oct 26, 2022

Implements Phase 1/4 of #343, which tracks the switch to only store hashes of API Tokens in DB.

The end-user experiences no change in behavior from this PR, but newly created tokens will have a TokenHash and Label stored in DB. The Label is an arbitrary string that does not need to be unique, but allows the user to track the keys that are in use and know which to revoke when needed.

Allowing revoke by hash is necessary to ensure keys can still be revoked after we stop storing the raw token itself.

For info on follow-up PRs to complete the switch, see the issue linked above.


Demonstrating creating a new key with a label, as well as revoking keys by hash and by key. The listed tokens with the censored token are the keys that have no token value stored in the db, and are being revoked via their hash.

Screen.Recording.2022-11-07.at.12.08.59.PM.mov

@neelvirdy neelvirdy requested review from 10d9e and en0ma October 26, 2022 22:17
@en0ma
Copy link
Contributor

en0ma commented Oct 27, 2022

@neelvirdy can the label address this requirement #471 ?

@neelvirdy neelvirdy force-pushed the nvirdy/token-hash-label branch from 64c4c49 to b633f3b Compare October 27, 2022 17:04
@neelvirdy neelvirdy changed the base branch from master to dev October 27, 2022 17:08
@neelvirdy
Copy link
Contributor Author

@neelvirdy can the label address this requirement #471 ?

@en0ma yep! didn't see that issue till now but it seems identical except these 2 differences:

  1. Label vs name for field name - i have no strong preference with which way we go. Label is slightly more intuitive to me but clearly not for everyone! I don't see any convention from a quick google search. Open to adjusting it if someone shouts
  2. I intend to make the label a required input when generating a key, so the user is encouraged to provide a meaningful label and remember what the key is for, rather than auto-generating a random label and relying on the user to do their due diligence proactively to have good key management.

@neelvirdy neelvirdy requested a review from alvin-reyes October 27, 2022 17:31
@neelvirdy neelvirdy force-pushed the nvirdy/token-hash-label branch from 8eb200e to e229a07 Compare October 31, 2022 17:30
@en0ma
Copy link
Contributor

en0ma commented Nov 4, 2022

2. I intend to make the label a required input when generating a key, so the user is encouraged to provide a meaningful label and remember what the key is for, rather than auto-generating a random label and relying on the user to do their due diligence proactively to have good key management.

If this is done, I think it will completely address that issue. I am ok with using "label" too.

@en0ma
Copy link
Contributor

en0ma commented Nov 4, 2022

@alvin-reyes can you have a look at this PR

Copy link
Contributor

@alvin-reyes alvin-reyes left a comment

Choose a reason for hiding this comment

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

Great work @neelvirdy!

Can you add some verification tests and post it in this PR? Just add a new section on the description part

## Verification 
### revoke user by hash
<screen shot of the UI or the cli>
### revoke user by key
<screen shot of the UI or the cli>

@neelvirdy neelvirdy changed the base branch from dev to master November 7, 2022 17:39
@neelvirdy neelvirdy changed the base branch from master to dev November 7, 2022 17:39
@neelvirdy neelvirdy requested a review from alvin-reyes November 7, 2022 17:47
Copy link
Contributor

@alvin-reyes alvin-reyes left a comment

Choose a reason for hiding this comment

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

LGTM

@alvin-reyes alvin-reyes merged commit 7619fb2 into application-research:dev Nov 8, 2022
@neelvirdy neelvirdy deleted the nvirdy/token-hash-label branch November 8, 2022 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants