-
Notifications
You must be signed in to change notification settings - Fork 888
feat: add flag to see all tokens if owner #6227
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
Conversation
We'd like to include a toggle on the tokens page in the UI that one can switch to show 'all' tokens, or just tokens created by that user. This toggle should only appear if the user is allowed to read tokens created by other users. @Emyrk Perhaps we can chat about implementation on Monday given you recently touched RBAC? I looked at the code but wasn't sure as to the best path forward given we don't have roles on the FE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, nice work! 👍🏻
We should not check roles. We should do an authorization check like we do for other FE elements. We should do a "read" check on an api token owner by a random uuid, which indicates reading any other user's api key |
I assume we do this check with the
What do you think is the best path forward? |
site/yarn.lock
Outdated
integrity sha512-4Hbzei7ZyBp+1aw0874YWpKOubZd/jc53/XU+gkYry1QV+VvrbO8icLM5CUtm4F0hyXn85DXYKEMIS26gitD3A== | ||
version "4.0.3" | ||
resolved "https://registry.yarnpkg.com/minipass/-/minipass-4.0.3.tgz#00bfbaf1e16e35e804f4aa31a7c1f6b8d9f0ee72" | ||
integrity sha512-OW2r4sQ0sI+z5ckEt5c1Tri4xTgZwYDxpE54eqWlQloQRoWtXjqt9udJ5Z4dSv7wK+nfFI7FRXyCpBSft+gpFw== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn.lock
changes because I had to remove and re-add canvas
again.
mutationFn: deleteAPIKey, | ||
onSuccess: () => { | ||
// Invalidate and refetch | ||
void queryClient.invalidateQueries(queryKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need void
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. We have to add it to escape the following eslint error:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the
void operator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FE LGTM
We do not need to specify an The payload should be: {
"canReadAllAPIKeys": {
"action": "read",
"object": {
// By not specifying an owner, this checks if all api keys
// can be read by the user in the `site` scope.
"resource_type": "api_key"
}
}
} |
partially resolves #6214
Some remaining work to be done in the UI. In preparation for this work, I have ripped out the tokens machine and instead I'm using React Query to handle state for the page.
We now have a new

--all
flag for listing tokens: