Skip to content

feat: add 'Show all tokens' toggle for owners #6325

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 12 commits into from
Mar 1, 2023

Conversation

Kira-Pilot
Copy link
Member

resolves #6214

Adds a new 'Show all tokens' toggle which only appears if the user has read all permissions.

Screenshot 2023-02-23 at 1 33 16 PM

Screenshot 2023-02-23 at 1 33 10 PM

@Kira-Pilot
Copy link
Member Author

I tried to write a test for this switch:

import { rest } from "msw"
import { server } from "testHelpers/server"
import { render } from "testHelpers/renderHelpers"
import { TokensPage } from "./TokensPage"
import { AuthorizationResponse } from "api/typesGenerated"
import { renderHook, waitFor, screen } from "@testing-library/react"
import { useCheckTokenPermissions } from "./hooks"

describe("TokensPage", () => {
  it("shows a toggle switch if user has read all permissions", async () => {
    const MockReadAllTokenPermissions: AuthorizationResponse = {
      readAllApiKeys: true,
    }

    server.use(
      rest.get("/api/v2/authcheck", (req, res, ctx) => {
        return res(ctx.status(200), ctx.json(MockReadAllTokenPermissions))
      }),
    )

    const { container } = render(<TokensPage />)

    const { result } = renderHook(() => useCheckTokenPermissions(), {
      container,
    })

    await waitFor(() => expect(result.current.isSuccess).toBe(true))
    expect(result.current.data).toEqual(MockReadAllTokenPermissions)

    // https://v4.mui.com/components/switches/#accessibility
    await screen.findByRole("checkbox")
  })
})

But I keep getting this error:
Screenshot 2023-02-24 at 11 17 20 AM

I believe QueryClient should be set in the render function, which wraps the <TokensPage /> component in AppProviders. @BrunoQuaresma any idea where I'm going astray?

@Kira-Pilot Kira-Pilot marked this pull request as ready for review February 24, 2023 16:28
@BrunoQuaresma
Copy link
Collaborator

I tried to write a test for this switch:

import { rest } from "msw"
import { server } from "testHelpers/server"
import { render } from "testHelpers/renderHelpers"
import { TokensPage } from "./TokensPage"
import { AuthorizationResponse } from "api/typesGenerated"
import { renderHook, waitFor, screen } from "@testing-library/react"
import { useCheckTokenPermissions } from "./hooks"

describe("TokensPage", () => {
  it("shows a toggle switch if user has read all permissions", async () => {
    const MockReadAllTokenPermissions: AuthorizationResponse = {
      readAllApiKeys: true,
    }

    server.use(
      rest.get("/api/v2/authcheck", (req, res, ctx) => {
        return res(ctx.status(200), ctx.json(MockReadAllTokenPermissions))
      }),
    )

    const { container } = render(<TokensPage />)

    const { result } = renderHook(() => useCheckTokenPermissions(), {
      container,
    })

    await waitFor(() => expect(result.current.isSuccess).toBe(true))
    expect(result.current.data).toEqual(MockReadAllTokenPermissions)

    // https://v4.mui.com/components/switches/#accessibility
    await screen.findByRole("checkbox")
  })
})

But I keep getting this error: Screenshot 2023-02-24 at 11 17 20 AM

I believe QueryClient should be set in the render function, which wraps the <TokensPage /> component in AppProviders. @BrunoQuaresma any idea where I'm going astray?

If it is in the authenticated route, I would use renderWithAuth. But if the render helper has the query client, it should work... maybe we are missing something

@Kira-Pilot
Copy link
Member Author

maybe we are missing something

I did try renderWithAuth with no luck. I have tried posting on their Discord and we'll see how they respond.

coderd/apikey.go Outdated
for _, key := range keys {
apiKeys = append(apiKeys, convertAPIKey(key))
user, err := api.Database.GetUserByID(ctx, key.UserID)
Copy link
Member

Choose a reason for hiding this comment

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

We should use GetUsersByIDs here instead, otherwise, you could execute hundreds of separate queries if there are hundreds of users.

Generally, we should never query in for loops!

@@ -90,6 +90,11 @@ type TokensFilter struct {
IncludeAll bool `json:"include_all"`
}

type ConvertedAPIKey struct {
Copy link
Member

Choose a reason for hiding this comment

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

I think APIKeyWithOwner or APIKeyWithUsername might make more sense here.

@kylecarbs
Copy link
Member

Oops, I meant to click request changes, but feel free to re-tag me if you need!

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

@Kira-Pilot Kira-Pilot merged commit 6304bfb into main Mar 1, 2023
@Kira-Pilot Kira-Pilot deleted the add-token-switch/kira-pilot branch March 1, 2023 16:35
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 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
3 participants