Skip to content

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

Merged
merged 16 commits into from
Feb 23, 2023
Merged

Conversation

Kira-Pilot
Copy link
Member

@Kira-Pilot Kira-Pilot commented Feb 15, 2023

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:
Screen Shot 2023-02-17 at 12 41 02 PM

Screenshot 2023-02-20 at 10 23 55 AM

@Kira-Pilot Kira-Pilot changed the title Tokens ls all/kira pilot Add flag to see all tokens if owner Feb 15, 2023
@Kira-Pilot Kira-Pilot changed the title Add flag to see all tokens if owner Feat: add flag to see all tokens if owner Feb 15, 2023
@Kira-Pilot Kira-Pilot changed the title Feat: add flag to see all tokens if owner feat: add flag to see all tokens if owner Feb 16, 2023
@Kira-Pilot Kira-Pilot requested review from Emyrk and mafredri February 17, 2023 17:54
@Kira-Pilot
Copy link
Member Author

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.

@Kira-Pilot Kira-Pilot marked this pull request as ready for review February 17, 2023 18:06
Copy link
Member

@mafredri mafredri left a 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! 👍🏻

@Emyrk
Copy link
Member

Emyrk commented Feb 20, 2023

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.

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

@Kira-Pilot
Copy link
Member Author

@Emyrk

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 /authcheck endpoint.
The api_key model doesn't have a UUID. It just has a string ID. I could:

  1. Change the model to use a UUID instead. Nervous about this because it touches auth
  2. Add a new endpoint just to check for api_key permissions, modeling the handler off of api.checkAuthorization
  3. Something else?

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==
Copy link
Member Author

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.

@BrunoQuaresma BrunoQuaresma removed their request for review February 22, 2023 14:55
mutationFn: deleteAPIKey,
onSuccess: () => {
// Invalidate and refetch
void queryClient.invalidateQueries(queryKey)
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

FE LGTM

@Emyrk
Copy link
Member

Emyrk commented Feb 23, 2023

@Emyrk

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 /authcheck endpoint. The api_key model doesn't have a UUID. It just has a string ID. I could:

1. Change the model to use a UUID instead. Nervous about this because it touches auth

2. Add a new endpoint just to check for `api_key` permissions, modeling the handler off of `api.checkAuthorization`

3. Something else?

What do you think is the best path forward?

We do not need to specify an ID. That would ask "Can I read the API key 123". But you want to ask, "Can I read all api keys".

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"
		}
	}
}

@Kira-Pilot Kira-Pilot merged commit a32169c into main Feb 23, 2023
@Kira-Pilot Kira-Pilot deleted the tokens-ls-all/kira-pilot branch February 23, 2023 15:00
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokens: coder tokens ls is confusing for owners
4 participants