From 2b5e02f5b2356c6c73957ebce66e7511783a6046 Mon Sep 17 00:00:00 2001 From: Kayla Washburn Date: Tue, 17 Oct 2023 16:11:42 -0600 Subject: [PATCH 01/26] refactor: improve e2e test reporting (#10304) --- site/.eslintrc.yaml | 3 + site/e2e/playwright.config.ts | 2 +- site/e2e/reporter.ts | 159 +++++++++++++++++++++++----------- 3 files changed, 114 insertions(+), 50 deletions(-) diff --git a/site/.eslintrc.yaml b/site/.eslintrc.yaml index ac7c6936ff5a6..478c1e070415e 100644 --- a/site/.eslintrc.yaml +++ b/site/.eslintrc.yaml @@ -91,6 +91,9 @@ rules: - allowSingleExtends: true "brace-style": "off" "curly": ["error", "all"] + "eslint-comments/disable-enable-pair": + - error + - allowWholeFile: true "eslint-comments/require-description": "error" eqeqeq: error import/default: "off" diff --git a/site/e2e/playwright.config.ts b/site/e2e/playwright.config.ts index 792944f26dde9..dcd82a6dfd999 100644 --- a/site/e2e/playwright.config.ts +++ b/site/e2e/playwright.config.ts @@ -6,7 +6,7 @@ export const port = process.env.CODER_E2E_PORT ? Number(process.env.CODER_E2E_PORT) : defaultPort; -const coderMain = path.join(__dirname, "../../enterprise/cmd/coder/main.go"); +const coderMain = path.join(__dirname, "../../enterprise/cmd/coder"); export const STORAGE_STATE = path.join(__dirname, ".auth.json"); diff --git a/site/e2e/reporter.ts b/site/e2e/reporter.ts index 1cb5e34c6619a..6be742b02bb55 100644 --- a/site/e2e/reporter.ts +++ b/site/e2e/reporter.ts @@ -1,4 +1,5 @@ -import fs from "fs"; +/* eslint-disable no-console -- Logging is sort of the whole point here */ +import * as fs from "fs/promises"; import type { FullConfig, Suite, @@ -6,80 +7,140 @@ import type { TestResult, FullResult, Reporter, + TestError, } from "@playwright/test/reporter"; import axios from "axios"; +import type { Writable } from "stream"; class CoderReporter implements Reporter { + config: FullConfig | null = null; + testOutput = new Map>(); + passedCount = 0; + failedTests: TestCase[] = []; + timedOutTests: TestCase[] = []; + onBegin(config: FullConfig, suite: Suite) { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log(`Starting the run with ${suite.allTests().length} tests`); + this.config = config; + console.log(`==> Running ${suite.allTests().length} tests`); } onTestBegin(test: TestCase) { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log(`Starting test ${test.title}`); + this.testOutput.set(test.id, []); + console.log(`==> Starting test ${test.title}`); } - onStdOut(chunk: string, test: TestCase, _: TestResult): void { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log( - `[stdout] [${test ? test.title : "unknown"}]: ${chunk.replace( - /\n$/g, - "", - )}`, - ); + onStdOut(chunk: string, test?: TestCase, _?: TestResult): void { + if (!test) { + for (const line of filteredServerLogLines(chunk)) { + console.log(`[stdout] ${line}`); + } + return; + } + this.testOutput.get(test.id)!.push([process.stdout, chunk]); } - onStdErr(chunk: string, test: TestCase, _: TestResult): void { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log( - `[stderr] [${test ? test.title : "unknown"}]: ${chunk.replace( - /\n$/g, - "", - )}`, - ); + onStdErr(chunk: string, test?: TestCase, _?: TestResult): void { + if (!test) { + for (const line of filteredServerLogLines(chunk)) { + console.error(`[stderr] ${line}`); + } + return; + } + this.testOutput.get(test.id)!.push([process.stderr, chunk]); } async onTestEnd(test: TestCase, result: TestResult) { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log(`Finished test ${test.title}: ${result.status}`); + console.log(`==> Finished test ${test.title}: ${result.status}`); + + if (result.status === "passed") { + this.passedCount++; + } + + if (result.status === "failed") { + this.failedTests.push(test); + } + + if (result.status === "timedOut") { + this.timedOutTests.push(test); + } + + const fsTestTitle = test.title.replaceAll(" ", "-"); + const outputFile = `test-results/debug-pprof-goroutine-${fsTestTitle}.txt`; + await exportDebugPprof(outputFile); if (result.status !== "passed") { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log("errors", result.errors, "attachments", result.attachments); + console.log(`Data from pprof has been saved to ${outputFile}`); + console.log("==> Output"); + const output = this.testOutput.get(test.id)!; + for (const [target, chunk] of output) { + target.write(`${chunk.replace(/\n$/g, "")}\n`); + } + + if (result.errors.length > 0) { + console.log("==> Errors"); + for (const error of result.errors) { + reportError(error); + } + } + + if (result.attachments.length > 0) { + console.log("==> Attachments"); + for (const attachment of result.attachments) { + console.log(attachment); + } + } } - await exportDebugPprof(test.title); + this.testOutput.delete(test.id); } onEnd(result: FullResult) { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log(`Finished the run: ${result.status}`); + console.log(`==> Tests ${result.status}`); + console.log(`${this.passedCount} passed`); + if (this.failedTests.length > 0) { + console.log(`${this.failedTests.length} failed`); + for (const test of this.failedTests) { + console.log(` ${test.location.file} › ${test.title}`); + } + } + if (this.timedOutTests.length > 0) { + console.log(`${this.timedOutTests.length} timed out`); + for (const test of this.timedOutTests) { + console.log(` ${test.location.file} › ${test.title}`); + } + } } } -const exportDebugPprof = async (testName: string) => { - const url = "http://127.0.0.1:6060/debug/pprof/goroutine?debug=1"; - const outputFile = `test-results/debug-pprof-goroutine-${testName}.txt`; +const shouldPrintLine = (line: string) => + [" error=EOF", "coderd: audit_log"].every((noise) => !line.includes(noise)); - await axios - .get(url) - .then((response) => { - if (response.status !== 200) { - throw new Error(`Error: Received status code ${response.status}`); - } +const filteredServerLogLines = (chunk: string): string[] => + chunk.trimEnd().split("\n").filter(shouldPrintLine); - fs.writeFile(outputFile, response.data, (err) => { - if (err) { - throw new Error(`Error writing to ${outputFile}: ${err.message}`); - } else { - // eslint-disable-next-line no-console -- Helpful for debugging - console.log(`Data from ${url} has been saved to ${outputFile}`); - } - }); - }) - .catch((error) => { - throw new Error(`Error: ${error.message}`); - }); +const exportDebugPprof = async (outputFile: string) => { + const response = await axios.get( + "http://127.0.0.1:6060/debug/pprof/goroutine?debug=1", + ); + if (response.status !== 200) { + throw new Error(`Error: Received status code ${response.status}`); + } + + await fs.writeFile(outputFile, response.data); +}; + +const reportError = (error: TestError) => { + if (error.location) { + console.log(`${error.location.file}:${error.location.line}:`); + } + if (error.snippet) { + console.log(error.snippet); + } + + if (error.message) { + console.log(error.message); + } else { + console.log(error); + } }; // eslint-disable-next-line no-unused-vars -- Playwright config uses it From fe05fd1e6e9760c4e6af262625c177aeca917b9a Mon Sep 17 00:00:00 2001 From: Muhammad Atif Ali Date: Wed, 18 Oct 2023 15:13:44 +0300 Subject: [PATCH 02/26] docs: update vscode web docs (#10327) --- docs/ides/web-ides.md | 90 +++++++++++++++++++++--------------- site/src/theme/icons.json | 1 + site/static/icon/discord.svg | 1 + 3 files changed, 55 insertions(+), 37 deletions(-) create mode 100644 site/static/icon/discord.svg diff --git a/docs/ides/web-ides.md b/docs/ides/web-ides.md index ca5463e5b91b6..aa466e54c432e 100644 --- a/docs/ides/web-ides.md +++ b/docs/ides/web-ides.md @@ -45,7 +45,7 @@ resource "coder_app" "pubslack" { display_name = "Coder Public Slack" slug = "pubslack" url = "https://coder-com.slack.com/" - icon = "https://cdn2.hubspot.net/hubfs/521324/slack-logo.png" + icon = "/icon/slack.svg" external = true } @@ -54,7 +54,7 @@ resource "coder_app" "discord" { display_name = "Coder Discord" slug = "discord" url = "https://discord.com/invite/coder" - icon = "https://logodix.com/logo/573024.png" + icon = "/icon/discord.svg" external = true } ``` @@ -133,43 +133,59 @@ resource "coder_app" "code-server" { ![code-server in a workspace](../images/code-server-ide.png) -## VS Code Server +## VS Code Web VS Code supports launching a local web client using the `code serve-web` -command. To add VS Code web as a web IDE, Install and start this in your -`startup_script` and create a corresponding `coder_app` - -```hcl -resource "coder_agent" "main" { - arch = "amd64" - os = "linux" - startup_script = </tmp/vscode-web.log 2>&1 & - EOF -} -``` - -> `code serve-web` was introduced in version 1.82.0 (August 2023). - -You also need to add a `coder_app` resource for this. - -```hcl -# VS Code Web -resource "coder_app" "vscode-web" { - agent_id = coder_agent.coder.id - slug = "vscode-web" - display_name = "VS Code Web" - icon = "/icon/code.svg" - url = "http://localhost:13338?folder=/home/coder" - subdomain = true # VS Code Web does currently does not work with a subpath https://github.com/microsoft/vscode/issues/192947 - share = "owner" -} -``` +command. To add VS Code web as a web IDE, you have two options. + +1. Install using the + [vscode-web module](https://registry.coder.com/modules/vscode-web) from the + coder registry. + + ```hcl + module "vscode-web" { + source = "https://registry.coder.com/modules/vscode-web" + agent_id = coder_agent.main.id + accept_license = true + } + ``` + +2. Install and start in your `startup_script` and create a corresponding + `coder_app` + + ```hcl + resource "coder_agent" "main" { + arch = "amd64" + os = "linux" + startup_script = </tmp/vscode-web.log 2>&1 & + EOF + } + ``` + + > `code serve-web` was introduced in version 1.82.0 (August 2023). + + You also need to add a `coder_app` resource for this. + + ```hcl + # VS Code Web + resource "coder_app" "vscode-web" { + agent_id = coder_agent.coder.id + slug = "vscode-web" + display_name = "VS Code Web" + icon = "/icon/code.svg" + url = "http://localhost:13338?folder=/home/coder" + subdomain = true # VS Code Web does currently does not work with a subpath https://github.com/microsoft/vscode/issues/192947 + share = "owner" + } + ``` ## JupyterLab diff --git a/site/src/theme/icons.json b/site/src/theme/icons.json index 12fbaec3f2805..db38a793c52ab 100644 --- a/site/src/theme/icons.json +++ b/site/src/theme/icons.json @@ -14,6 +14,7 @@ "datagrip.svg", "dataspell.svg", "debian.svg", + "discord.svg", "do.png", "docker.png", "dotfiles.svg", diff --git a/site/static/icon/discord.svg b/site/static/icon/discord.svg new file mode 100644 index 0000000000000..ca65400760907 --- /dev/null +++ b/site/static/icon/discord.svg @@ -0,0 +1 @@ + \ No newline at end of file From c93fe8ddbe7efecd45f45a648fc949446b0f766e Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Wed, 18 Oct 2023 09:18:03 -0300 Subject: [PATCH 03/26] chore(site): remove template version machine (#10315) --- site/src/api/queries/templateVersions.ts | 15 -- site/src/api/queries/templates.ts | 26 +++ site/src/components/TemplateFiles/hooks.ts | 68 +++++++ .../DuplicateTemplateView.tsx | 2 +- .../ImportStarterTemplateView.tsx | 2 +- .../CreateTemplatePage/UploadTemplateView.tsx | 2 +- .../CreateWorkspacePage.tsx | 2 +- .../TemplateFilesPage/TemplateFilesPage.tsx | 67 +------ .../TemplateVersionPage.tsx | 26 ++- .../TemplateVersionPageView.stories.tsx | 34 ++-- .../TemplateVersionPageView.tsx | 18 +- site/src/utils/templateVersion.ts | 9 +- .../templateVersionXService.ts | 179 ------------------ 13 files changed, 143 insertions(+), 307 deletions(-) delete mode 100644 site/src/api/queries/templateVersions.ts create mode 100644 site/src/components/TemplateFiles/hooks.ts delete mode 100644 site/src/xServices/templateVersion/templateVersionXService.ts diff --git a/site/src/api/queries/templateVersions.ts b/site/src/api/queries/templateVersions.ts deleted file mode 100644 index e90e762e810a0..0000000000000 --- a/site/src/api/queries/templateVersions.ts +++ /dev/null @@ -1,15 +0,0 @@ -import * as API from "api/api"; - -export const templateVersionLogs = (versionId: string) => { - return { - queryKey: ["templateVersion", versionId, "logs"], - queryFn: () => API.getTemplateVersionLogs(versionId), - }; -}; - -export const richParameters = (versionId: string) => { - return { - queryKey: ["templateVersion", versionId, "richParameters"], - queryFn: () => API.getTemplateVersionRichParameters(versionId), - }; -}; diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index f4082e362fd29..e5a39a015b661 100644 --- a/site/src/api/queries/templates.ts +++ b/site/src/api/queries/templates.ts @@ -50,6 +50,18 @@ export const templateVersion = (versionId: string) => { }; }; +export const templateVersionByName = ( + orgId: string, + templateName: string, + versionName: string, +) => { + return { + queryKey: ["templateVersion", orgId, templateName, versionName], + queryFn: () => + API.getTemplateVersionByName(orgId, templateName, versionName), + }; +}; + export const templateVersions = (templateId: string) => { return { queryKey: ["templateVersions", templateId], @@ -127,6 +139,20 @@ const createTemplateFn = async (options: { }); }; +export const templateVersionLogs = (versionId: string) => { + return { + queryKey: ["templateVersion", versionId, "logs"], + queryFn: () => API.getTemplateVersionLogs(versionId), + }; +}; + +export const richParameters = (versionId: string) => { + return { + queryKey: ["templateVersion", versionId, "richParameters"], + queryFn: () => API.getTemplateVersionRichParameters(versionId), + }; +}; + const waitBuildToBeFinished = async (version: TemplateVersion) => { let data: TemplateVersion; let jobStatus: ProvisionerJobStatus; diff --git a/site/src/components/TemplateFiles/hooks.ts b/site/src/components/TemplateFiles/hooks.ts new file mode 100644 index 0000000000000..6d21538f8c3db --- /dev/null +++ b/site/src/components/TemplateFiles/hooks.ts @@ -0,0 +1,68 @@ +import { TemplateVersion } from "api/typesGenerated"; +import { useTab } from "hooks/useTab"; +import { useEffect } from "react"; +import { useQuery } from "react-query"; +import { + TemplateVersionFiles, + getTemplateVersionFiles, +} from "utils/templateVersion"; +import * as API from "api/api"; + +export const useFileTab = (templateFiles: TemplateVersionFiles | undefined) => { + // Tabs The default tab is the tab that has main.tf but until we loads the + // files and check if main.tf exists we don't know which tab is the default + // one so we just use empty string + const tab = useTab("file", ""); + const isLoaded = tab.value !== ""; + useEffect(() => { + if (templateFiles && !isLoaded) { + const terraformFileIndex = Object.keys(templateFiles).indexOf("main.tf"); + // If main.tf exists use the index if not just use the first tab + tab.set(terraformFileIndex !== -1 ? terraformFileIndex.toString() : "0"); + } + }, [isLoaded, tab, templateFiles]); + + return { + ...tab, + isLoaded, + }; +}; + +export const useTemplateFiles = ( + templateName: string, + version: TemplateVersion | undefined, +) => { + return useQuery({ + queryKey: ["templateFiles", templateName, version], + queryFn: () => { + if (!version) { + return; + } + return getTemplateFilesWithDiff(templateName, version); + }, + enabled: version !== undefined, + }); +}; + +const getTemplateFilesWithDiff = async ( + templateName: string, + version: TemplateVersion, +) => { + const previousVersion = await API.getPreviousTemplateVersionByName( + version.organization_id!, + templateName, + version.name, + ); + const loadFilesPromises: ReturnType[] = []; + loadFilesPromises.push(getTemplateVersionFiles(version.job.file_id)); + if (previousVersion) { + loadFilesPromises.push( + getTemplateVersionFiles(previousVersion.job.file_id), + ); + } + const [currentFiles, previousFiles] = await Promise.all(loadFilesPromises); + return { + currentFiles, + previousFiles, + }; +}; diff --git a/site/src/pages/CreateTemplatePage/DuplicateTemplateView.tsx b/site/src/pages/CreateTemplatePage/DuplicateTemplateView.tsx index 96567948d97ca..3fcdb594989fb 100644 --- a/site/src/pages/CreateTemplatePage/DuplicateTemplateView.tsx +++ b/site/src/pages/CreateTemplatePage/DuplicateTemplateView.tsx @@ -1,6 +1,6 @@ import { useQuery, useMutation } from "react-query"; -import { templateVersionLogs } from "api/queries/templateVersions"; import { + templateVersionLogs, templateByName, templateVersion, templateVersionVariables, diff --git a/site/src/pages/CreateTemplatePage/ImportStarterTemplateView.tsx b/site/src/pages/CreateTemplatePage/ImportStarterTemplateView.tsx index 9854a62cae2d0..38e596b9081e1 100644 --- a/site/src/pages/CreateTemplatePage/ImportStarterTemplateView.tsx +++ b/site/src/pages/CreateTemplatePage/ImportStarterTemplateView.tsx @@ -1,6 +1,6 @@ import { useQuery, useMutation } from "react-query"; -import { templateVersionLogs } from "api/queries/templateVersions"; import { + templateVersionLogs, JobError, createTemplate, templateExamples, diff --git a/site/src/pages/CreateTemplatePage/UploadTemplateView.tsx b/site/src/pages/CreateTemplatePage/UploadTemplateView.tsx index a337b82a12404..b3eb11a7c1c3c 100644 --- a/site/src/pages/CreateTemplatePage/UploadTemplateView.tsx +++ b/site/src/pages/CreateTemplatePage/UploadTemplateView.tsx @@ -1,6 +1,6 @@ import { useQuery, useMutation } from "react-query"; -import { templateVersionLogs } from "api/queries/templateVersions"; import { + templateVersionLogs, JobError, createTemplate, templateVersionVariables, diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 94e8a55f16800..2c6cff4807e81 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -22,11 +22,11 @@ import { useMutation, useQuery, useQueryClient } from "react-query"; import { templateByName, templateVersionExternalAuth, + richParameters, } from "api/queries/templates"; import { autoCreateWorkspace, createWorkspace } from "api/queries/workspaces"; import { checkAuthorization } from "api/queries/authCheck"; import { CreateWSPermissions, createWorkspaceChecks } from "./permissions"; -import { richParameters } from "api/queries/templateVersions"; import { paramsUsedToCreateWorkspace } from "utils/workspace"; import { useEffectEvent } from "hooks/hookPolyfills"; diff --git a/site/src/pages/TemplatePage/TemplateFilesPage/TemplateFilesPage.tsx b/site/src/pages/TemplatePage/TemplateFilesPage/TemplateFilesPage.tsx index d5398d7bc3b03..1a1cd9b244b10 100644 --- a/site/src/pages/TemplatePage/TemplateFilesPage/TemplateFilesPage.tsx +++ b/site/src/pages/TemplatePage/TemplateFilesPage/TemplateFilesPage.tsx @@ -1,77 +1,14 @@ -import { useQuery } from "react-query"; -import { getPreviousTemplateVersionByName } from "api/api"; -import { TemplateVersion } from "api/typesGenerated"; import { Loader } from "components/Loader/Loader"; import { TemplateFiles } from "components/TemplateFiles/TemplateFiles"; import { useTemplateLayoutContext } from "pages/TemplatePage/TemplateLayout"; -import { useOrganizationId } from "hooks/useOrganizationId"; -import { useTab } from "hooks/useTab"; -import { FC, useEffect } from "react"; +import { FC } from "react"; import { Helmet } from "react-helmet-async"; -import { - getTemplateVersionFiles, - TemplateVersionFiles, -} from "utils/templateVersion"; import { getTemplatePageTitle } from "../utils"; - -const fetchTemplateFiles = async ( - organizationId: string, - templateName: string, - activeVersion: TemplateVersion, -) => { - const previousVersion = await getPreviousTemplateVersionByName( - organizationId, - templateName, - activeVersion.name, - ); - const loadFilesPromises: ReturnType[] = []; - loadFilesPromises.push(getTemplateVersionFiles(activeVersion)); - if (previousVersion) { - loadFilesPromises.push(getTemplateVersionFiles(previousVersion)); - } - const [currentFiles, previousFiles] = await Promise.all(loadFilesPromises); - return { - currentFiles, - previousFiles, - }; -}; - -const useTemplateFiles = ( - organizationId: string, - templateName: string, - activeVersion: TemplateVersion, -) => - useQuery({ - queryKey: ["templateFiles", templateName], - queryFn: () => - fetchTemplateFiles(organizationId, templateName, activeVersion), - }); - -const useFileTab = (templateFiles: TemplateVersionFiles | undefined) => { - // Tabs The default tab is the tab that has main.tf but until we loads the - // files and check if main.tf exists we don't know which tab is the default - // one so we just use empty string - const tab = useTab("file", ""); - const isLoaded = tab.value !== ""; - useEffect(() => { - if (templateFiles && !isLoaded) { - const terraformFileIndex = Object.keys(templateFiles).indexOf("main.tf"); - // If main.tf exists use the index if not just use the first tab - tab.set(terraformFileIndex !== -1 ? terraformFileIndex.toString() : "0"); - } - }, [isLoaded, tab, templateFiles]); - - return { - ...tab, - isLoaded, - }; -}; +import { useFileTab, useTemplateFiles } from "components/TemplateFiles/hooks"; const TemplateFilesPage: FC = () => { const { template, activeVersion } = useTemplateLayoutContext(); - const orgId = useOrganizationId(); const { data: templateFiles } = useTemplateFiles( - orgId, template.name, activeVersion, ); diff --git a/site/src/pages/TemplateVersionPage/TemplateVersionPage.tsx b/site/src/pages/TemplateVersionPage/TemplateVersionPage.tsx index 7567efd502e04..491e57a576571 100644 --- a/site/src/pages/TemplateVersionPage/TemplateVersionPage.tsx +++ b/site/src/pages/TemplateVersionPage/TemplateVersionPage.tsx @@ -1,13 +1,13 @@ -import { useMachine } from "@xstate/react"; import { usePermissions } from "hooks/usePermissions"; import { useOrganizationId } from "hooks/useOrganizationId"; -import { useTab } from "hooks/useTab"; import { type FC, useMemo } from "react"; import { Helmet } from "react-helmet-async"; import { useParams } from "react-router-dom"; import { pageTitle } from "utils/page"; -import { templateVersionMachine } from "xServices/templateVersion/templateVersionXService"; import TemplateVersionPageView from "./TemplateVersionPageView"; +import { useQuery } from "react-query"; +import { templateVersionByName } from "api/queries/templates"; +import { useFileTab, useTemplateFiles } from "components/TemplateFiles/hooks"; type Params = { version: string; @@ -18,13 +18,16 @@ export const TemplateVersionPage: FC = () => { const { version: versionName, template: templateName } = useParams() as Params; const orgId = useOrganizationId(); - const [state] = useMachine(templateVersionMachine, { - context: { templateName, versionName, orgId }, - }); - const tab = useTab("file", "0"); + const templateVersionQuery = useQuery( + templateVersionByName(orgId, templateName, versionName), + ); + const { data: templateFiles, error: templateFilesError } = useTemplateFiles( + templateName, + templateVersionQuery.data, + ); + const tab = useFileTab(templateFiles?.currentFiles); const permissions = usePermissions(); - - const versionId = state.context.currentVersion?.id; + const versionId = templateVersionQuery.data?.id; const createWorkspaceUrl = useMemo(() => { const params = new URLSearchParams(); if (versionId) { @@ -41,7 +44,10 @@ export const TemplateVersionPage: FC = () => { = { @@ -59,13 +55,11 @@ export const Default: Story = {}; export const Error: Story = { args: { - context: { - ...defaultArgs.context, - currentVersion: undefined, - currentFiles: undefined, - error: mockApiError({ - message: "Error on loading the template version", - }), - }, + ...defaultArgs, + currentVersion: undefined, + currentFiles: undefined, + error: mockApiError({ + message: "Error on loading the template version", + }), }, }; diff --git a/site/src/pages/TemplateVersionPage/TemplateVersionPageView.tsx b/site/src/pages/TemplateVersionPage/TemplateVersionPageView.tsx index 2238f9ccddfec..44107caffeef6 100644 --- a/site/src/pages/TemplateVersionPage/TemplateVersionPageView.tsx +++ b/site/src/pages/TemplateVersionPage/TemplateVersionPageView.tsx @@ -16,29 +16,31 @@ import { UseTabResult } from "hooks/useTab"; import { type FC } from "react"; import { Link as RouterLink } from "react-router-dom"; import { createDayString } from "utils/createDayString"; -import { TemplateVersionMachineContext } from "xServices/templateVersion/templateVersionXService"; import { ErrorAlert } from "components/Alert/ErrorAlert"; +import { TemplateVersion } from "api/typesGenerated"; +import { TemplateVersionFiles } from "utils/templateVersion"; export interface TemplateVersionPageViewProps { - /** - * Used to display the version name before loading the version in the API - */ versionName: string; templateName: string; tab: UseTabResult; - context: TemplateVersionMachineContext; createWorkspaceUrl?: string; + error: unknown; + currentVersion: TemplateVersion | undefined; + currentFiles: TemplateVersionFiles | undefined; + previousFiles: TemplateVersionFiles | undefined; } export const TemplateVersionPageView: FC = ({ - context, tab, versionName, templateName, createWorkspaceUrl, + currentVersion, + currentFiles, + previousFiles, + error, }) => { - const { currentFiles, error, currentVersion, previousFiles } = context; - return ( ; export const getTemplateVersionFiles = async ( - version: TemplateVersion, + fileId: string, ): Promise => { const files: TemplateVersionFiles = {}; - const tarFile = await API.getFile(version.job.file_id); + const tarFile = await API.getFile(fileId); const tarReader = new TarReader(); await tarReader.readFile(tarFile); for (const file of tarReader.fileInfo) { diff --git a/site/src/xServices/templateVersion/templateVersionXService.ts b/site/src/xServices/templateVersion/templateVersionXService.ts deleted file mode 100644 index 4cad1e061c0af..0000000000000 --- a/site/src/xServices/templateVersion/templateVersionXService.ts +++ /dev/null @@ -1,179 +0,0 @@ -import { - getPreviousTemplateVersionByName, - GetPreviousTemplateVersionByNameResponse, - getTemplateByName, - getTemplateVersionByName, -} from "api/api"; -import { Template, TemplateVersion } from "api/typesGenerated"; -import { - getTemplateVersionFiles, - TemplateVersionFiles, -} from "utils/templateVersion"; -import { assign, createMachine } from "xstate"; - -export interface TemplateVersionMachineContext { - orgId: string; - templateName: string; - versionName: string; - template?: Template; - currentVersion?: TemplateVersion; - currentFiles?: TemplateVersionFiles; - error?: unknown; - // Get file diffs - previousVersion?: TemplateVersion; - previousFiles?: TemplateVersionFiles; -} - -export const templateVersionMachine = createMachine( - { - predictableActionArguments: true, - id: "templateVersion", - schema: { - context: {} as TemplateVersionMachineContext, - services: {} as { - loadVersions: { - data: { - currentVersion: GetPreviousTemplateVersionByNameResponse; - previousVersion: GetPreviousTemplateVersionByNameResponse; - }; - }; - loadTemplate: { - data: { - template: Template; - }; - }; - loadFiles: { - data: { - currentFiles: TemplateVersionFiles; - previousFiles: TemplateVersionFiles; - }; - }; - }, - }, - tsTypes: {} as import("./templateVersionXService.typegen").Typegen0, - initial: "initialInfo", - states: { - initialInfo: { - type: "parallel", - states: { - versions: { - initial: "loadingVersions", - states: { - loadingVersions: { - invoke: { - src: "loadVersions", - onDone: [ - { - actions: "assignVersions", - target: "success", - }, - ], - }, - }, - success: { - type: "final", - }, - }, - }, - template: { - initial: "loadingTemplate", - states: { - loadingTemplate: { - invoke: { - src: "loadTemplate", - onDone: [ - { - actions: "assignTemplate", - target: "success", - }, - ], - }, - }, - success: { - type: "final", - }, - }, - }, - }, - onDone: { - target: "loadingFiles", - }, - }, - loadingFiles: { - invoke: { - src: "loadFiles", - onDone: { - target: "done.ok", - actions: ["assignFiles"], - }, - onError: { - target: "done.error", - actions: ["assignError"], - }, - }, - }, - done: { - states: { - ok: { type: "final" }, - error: { type: "final" }, - }, - }, - }, - }, - { - actions: { - assignError: assign({ - error: (_, { data }) => data, - }), - assignTemplate: assign({ - template: (_, { data }) => data.template, - }), - assignVersions: assign({ - currentVersion: (_, { data }) => data.currentVersion, - previousVersion: (_, { data }) => data.previousVersion, - }), - assignFiles: assign({ - currentFiles: (_, { data }) => data.currentFiles, - previousFiles: (_, { data }) => data.previousFiles, - }), - }, - services: { - loadVersions: async ({ orgId, templateName, versionName }) => { - const [currentVersion, previousVersion] = await Promise.all([ - getTemplateVersionByName(orgId, templateName, versionName), - getPreviousTemplateVersionByName(orgId, templateName, versionName), - ]); - - return { - currentVersion, - previousVersion, - }; - }, - loadTemplate: async ({ orgId, templateName }) => { - const template = await getTemplateByName(orgId, templateName); - - return { - template, - }; - }, - loadFiles: async ({ currentVersion, previousVersion }) => { - if (!currentVersion) { - throw new Error("Version is not defined"); - } - const loadFilesPromises: ReturnType[] = - []; - loadFilesPromises.push(getTemplateVersionFiles(currentVersion)); - if (previousVersion) { - loadFilesPromises.push(getTemplateVersionFiles(previousVersion)); - } - const [currentFiles, previousFiles] = await Promise.all( - loadFilesPromises, - ); - return { - currentFiles, - previousFiles, - }; - }, - }, - }, -); From 9b73020f111d068c142e96bba6c4a5490e2b9c5e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 18 Oct 2023 20:07:52 +0300 Subject: [PATCH 04/26] ci(.github): set DataDog upload timeout (#10328) --- .github/workflows/ci.yaml | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 20a7da094d14e..3a42b67c75f8e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -291,14 +291,9 @@ jobs: gotestsum --junitfile="gotests.xml" --jsonfile="gotests.json" \ --packages="./..." -- $PARALLEL_FLAG -short -failfast $COVERAGE_FLAGS - - name: Print test stats - if: success() || failure() - run: | - # Artifacts are not available after rerunning a job, - # so we need to print the test stats to the log. - go run ./scripts/ci-report/main.go gotests.json | tee gotests_stats.json - - name: Upload test stats to Datadog + timeout-minutes: 1 + continue-on-error: true uses: ./.github/actions/upload-datadog if: success() || failure() with: @@ -343,14 +338,9 @@ jobs: export TS_DEBUG_DISCO=true make test-postgres - - name: Print test stats - if: success() || failure() - run: | - # Artifacts are not available after rerunning a job, - # so we need to print the test stats to the log. - go run ./scripts/ci-report/main.go gotests.json | tee gotests_stats.json - - name: Upload test stats to Datadog + timeout-minutes: 1 + continue-on-error: true uses: ./.github/actions/upload-datadog if: success() || failure() with: @@ -391,6 +381,8 @@ jobs: gotestsum --junitfile="gotests.xml" -- -race ./... - name: Upload test stats to Datadog + timeout-minutes: 1 + continue-on-error: true uses: ./.github/actions/upload-datadog if: always() with: From 504cedf15a640daeae379ae03a0c8ba8978a1046 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 18 Oct 2023 14:20:30 -0500 Subject: [PATCH 05/26] feat: add telemetry for external provisioners (#10322) --- coderd/telemetry/telemetry.go | 42 ++++++++++++++++++++----- codersdk/provisionerdaemons.go | 3 ++ enterprise/cli/provisionerdaemons.go | 3 ++ enterprise/coderd/provisionerdaemons.go | 17 +++++++++- 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index 6c2b3eb78b9ba..39f3b892c2150 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -698,6 +698,23 @@ func ConvertWorkspaceProxy(proxy database.WorkspaceProxy) WorkspaceProxy { } } +func ConvertExternalProvisioner(id uuid.UUID, tags map[string]string, provisioners []database.ProvisionerType) ExternalProvisioner { + tagsCopy := make(map[string]string, len(tags)) + for k, v := range tags { + tagsCopy[k] = v + } + strProvisioners := make([]string, 0, len(provisioners)) + for _, prov := range provisioners { + strProvisioners = append(strProvisioners, string(prov)) + } + return ExternalProvisioner{ + ID: id.String(), + Tags: tagsCopy, + Provisioners: strProvisioners, + StartedAt: time.Now(), + } +} + // Snapshot represents a point-in-time anonymized database dump. // Data is aggregated by latest on the server-side, so partial data // can be sent without issue. @@ -705,20 +722,21 @@ type Snapshot struct { DeploymentID string `json:"deployment_id"` APIKeys []APIKey `json:"api_keys"` - ProvisionerJobs []ProvisionerJob `json:"provisioner_jobs"` + CLIInvocations []clitelemetry.Invocation `json:"cli_invocations"` + ExternalProvisioners []ExternalProvisioner `json:"external_provisioners"` Licenses []License `json:"licenses"` - Templates []Template `json:"templates"` + ProvisionerJobs []ProvisionerJob `json:"provisioner_jobs"` TemplateVersions []TemplateVersion `json:"template_versions"` + Templates []Template `json:"templates"` Users []User `json:"users"` - Workspaces []Workspace `json:"workspaces"` - WorkspaceApps []WorkspaceApp `json:"workspace_apps"` - WorkspaceAgents []WorkspaceAgent `json:"workspace_agents"` WorkspaceAgentStats []WorkspaceAgentStat `json:"workspace_agent_stats"` + WorkspaceAgents []WorkspaceAgent `json:"workspace_agents"` + WorkspaceApps []WorkspaceApp `json:"workspace_apps"` WorkspaceBuilds []WorkspaceBuild `json:"workspace_build"` - WorkspaceResources []WorkspaceResource `json:"workspace_resources"` - WorkspaceResourceMetadata []WorkspaceResourceMetadata `json:"workspace_resource_metadata"` WorkspaceProxies []WorkspaceProxy `json:"workspace_proxies"` - CLIInvocations []clitelemetry.Invocation `json:"cli_invocations"` + WorkspaceResourceMetadata []WorkspaceResourceMetadata `json:"workspace_resource_metadata"` + WorkspaceResources []WorkspaceResource `json:"workspace_resources"` + Workspaces []Workspace `json:"workspaces"` } // Deployment contains information about the host running Coder. @@ -900,6 +918,14 @@ type WorkspaceProxy struct { UpdatedAt time.Time `json:"updated_at"` } +type ExternalProvisioner struct { + ID string `json:"id"` + Tags map[string]string `json:"tags"` + Provisioners []string `json:"provisioners"` + StartedAt time.Time `json:"started_at"` + ShutdownAt *time.Time `json:"shutdown_at"` +} + type noopReporter struct{} func (*noopReporter) Report(_ *Snapshot) {} diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index ce2dd08758b8c..0b321e51662ab 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -174,6 +174,8 @@ func (c *Client) provisionerJobLogsAfter(ctx context.Context, path string, after // ServeProvisionerDaemonRequest are the parameters to call ServeProvisionerDaemon with // @typescript-ignore ServeProvisionerDaemonRequest type ServeProvisionerDaemonRequest struct { + // ID is a unique ID for a provisioner daemon. + ID uuid.UUID `json:"id" format:"uuid"` // Organization is the organization for the URL. At present provisioner daemons ARE NOT scoped to organizations // and so the organization ID is optional. Organization uuid.UUID `json:"organization" format:"uuid"` @@ -194,6 +196,7 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione return nil, xerrors.Errorf("parse url: %w", err) } query := serverURL.Query() + query.Add("id", req.ID.String()) for _, provisioner := range req.Provisioners { query.Add("provisioner", string(provisioner)) } diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index 837cb2e671766..623368487ff68 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -9,6 +9,7 @@ import ( "os/signal" "time" + "github.com/google/uuid" "golang.org/x/xerrors" "cdr.dev/slog" @@ -127,8 +128,10 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { connector := provisionerd.LocalProvisioners{ string(database.ProvisionerTypeTerraform): proto.NewDRPCProvisionerClient(terraformClient), } + id := uuid.New() srv := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { return client.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: id, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeTerraform, }, diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 70f59f40308f0..e10489f45a6c7 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -10,6 +10,7 @@ import ( "net" "net/http" "strings" + "time" "github.com/google/uuid" "github.com/hashicorp/yamux" @@ -27,6 +28,8 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/provisionerdserver" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/telemetry" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionerd/proto" ) @@ -155,6 +158,11 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) return } + id, _ := uuid.Parse(r.URL.Query().Get("id")) + if id == uuid.Nil { + id = uuid.New() + } + provisionersMap := map[codersdk.ProvisionerType]struct{}{} for _, provisioner := range r.URL.Query()["provisioner"] { switch provisioner { @@ -210,6 +218,13 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) api.AGPL.WebsocketWaitMutex.Unlock() defer api.AGPL.WebsocketWaitGroup.Done() + tep := telemetry.ConvertExternalProvisioner(id, tags, provisioners) + api.Telemetry.Report(&telemetry.Snapshot{ExternalProvisioners: []telemetry.ExternalProvisioner{tep}}) + defer func() { + tep.ShutdownAt = ptr.Ref(time.Now()) + api.Telemetry.Report(&telemetry.Snapshot{ExternalProvisioners: []telemetry.ExternalProvisioner{tep}}) + }() + conn, err := websocket.Accept(rw, r, &websocket.AcceptOptions{ // Need to disable compression to avoid a data-race. CompressionMode: websocket.CompressionDisabled, @@ -245,7 +260,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) srv, err := provisionerdserver.NewServer( api.ctx, api.AccessURL, - uuid.New(), + id, logger, provisioners, tags, From 1ad998ee3af7db5fdaf3eef3936cc74fb2b37d76 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 18 Oct 2023 15:08:02 -0500 Subject: [PATCH 06/26] fix: add requester IP to workspace build audit logs (#10242) --- cli/server.go | 9 + cli/templates.go | 1 + cli/templateversionarchive.go | 1 + cli/templateversions.go | 1 + cli/user_delete_test.go | 1 - coderd/audit/request.go | 67 +++++- coderd/audit/request_test.go | 33 +++ coderd/autobuild/lifecycle_executor.go | 3 +- coderd/database/dbfake/dbfake.go | 1 + coderd/externalauth.go | 3 +- .../provisionerdserver/provisionerdserver.go | 6 + coderd/workspacebuilds.go | 2 + coderd/workspacebuilds_test.go | 199 +++++++++++++++++- coderd/workspaces.go | 4 +- coderd/workspaces_test.go | 149 ------------- coderd/wsbuilder/wsbuilder.go | 9 +- coderd/wsbuilder/wsbuilder_test.go | 66 +++++- 17 files changed, 381 insertions(+), 174 deletions(-) create mode 100644 coderd/audit/request_test.go diff --git a/cli/server.go b/cli/server.go index 9f33ced438f84..6b6b218fb7d6a 100644 --- a/cli/server.go +++ b/cli/server.go @@ -41,6 +41,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/collectors" "github.com/prometheus/client_golang/prometheus/promhttp" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" "golang.org/x/mod/semver" "golang.org/x/oauth2" @@ -2020,6 +2022,13 @@ func ConfigureTraceProvider( sqlDriver = "postgres" ) + otel.SetTextMapPropagator( + propagation.NewCompositeTextMapPropagator( + propagation.TraceContext{}, + propagation.Baggage{}, + ), + ) + if cfg.Trace.Enable.Value() || cfg.Trace.DataDog.Value() || cfg.Trace.HoneycombAPIKey != "" { sdkTracerProvider, _closeTracing, err := tracing.TracerProvider(ctx, "coderd", tracing.TracerOpts{ Default: cfg.Trace.Enable.Value(), diff --git a/cli/templates.go b/cli/templates.go index 391ac7700870a..2dec46d226b5c 100644 --- a/cli/templates.go +++ b/cli/templates.go @@ -11,6 +11,7 @@ import ( "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" + "github.com/coder/pretty" ) func (r *RootCmd) templates() *clibase.Cmd { diff --git a/cli/templateversionarchive.go b/cli/templateversionarchive.go index d334bdb83fb4b..740de869ff9c2 100644 --- a/cli/templateversionarchive.go +++ b/cli/templateversionarchive.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" + "github.com/coder/pretty" ) func (r *RootCmd) unarchiveTemplateVersion() *clibase.Cmd { diff --git a/cli/templateversions.go b/cli/templateversions.go index 4ccba09b63ba8..5b99a46310996 100644 --- a/cli/templateversions.go +++ b/cli/templateversions.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" + "github.com/coder/pretty" ) func (r *RootCmd) templateVersions() *clibase.Cmd { diff --git a/cli/user_delete_test.go b/cli/user_delete_test.go index d8a6956577550..9ee546ca7a925 100644 --- a/cli/user_delete_test.go +++ b/cli/user_delete_test.go @@ -121,7 +121,6 @@ func TestUserDelete(t *testing.T) { // pw, err := cryptorand.String(16) // require.NoError(t, err) - // fmt.Println(aUser.OrganizationID) // toDelete, err := client.CreateUser(ctx, codersdk.CreateUserRequest{ // Email: "colin5@coder.com", // Username: "coolin", diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 812dc1e5c555f..cc1f60779a7dc 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -11,6 +11,8 @@ import ( "github.com/google/uuid" "github.com/sqlc-dev/pqtype" + "go.opentelemetry.io/otel/baggage" + "golang.org/x/xerrors" "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" @@ -54,6 +56,7 @@ type BuildAuditParams[T Auditable] struct { Status int Action database.AuditAction OrganizationID uuid.UUID + IP string AdditionalFields json.RawMessage New T @@ -248,9 +251,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request // WorkspaceBuildAudit creates an audit log for a workspace build. // The audit log is committed upon invocation. func WorkspaceBuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) { - // As the audit request has not been initiated directly by a user, we omit - // certain user details. - ip := parseIP("") + ip := parseIP(p.IP) diff := Diff(p.Audit, p.Old, p.New) var err error @@ -280,16 +281,70 @@ func WorkspaceBuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T RequestID: p.JobID, AdditionalFields: p.AdditionalFields, } - exportErr := p.Audit.Export(ctx, auditLog) - if exportErr != nil { + err = p.Audit.Export(ctx, auditLog) + if err != nil { p.Log.Error(ctx, "export audit log", slog.F("audit_log", auditLog), slog.Error(err), ) - return } } +type WorkspaceBuildBaggage struct { + IP string +} + +func (b WorkspaceBuildBaggage) Props() ([]baggage.Property, error) { + ipProp, err := baggage.NewKeyValueProperty("ip", b.IP) + if err != nil { + return nil, xerrors.Errorf("create ip kv property: %w", err) + } + + return []baggage.Property{ipProp}, nil +} + +func WorkspaceBuildBaggageFromRequest(r *http.Request) WorkspaceBuildBaggage { + return WorkspaceBuildBaggage{IP: r.RemoteAddr} +} + +type Baggage interface { + Props() ([]baggage.Property, error) +} + +func BaggageToContext(ctx context.Context, d Baggage) (context.Context, error) { + props, err := d.Props() + if err != nil { + return ctx, xerrors.Errorf("create baggage properties: %w", err) + } + + m, err := baggage.NewMember("audit", "baggage", props...) + if err != nil { + return ctx, xerrors.Errorf("create new baggage member: %w", err) + } + + b, err := baggage.New(m) + if err != nil { + return ctx, xerrors.Errorf("create new baggage carrier: %w", err) + } + + return baggage.ContextWithBaggage(ctx, b), nil +} + +func BaggageFromContext(ctx context.Context) WorkspaceBuildBaggage { + d := WorkspaceBuildBaggage{} + b := baggage.FromContext(ctx) + props := b.Member("audit").Properties() + for _, prop := range props { + switch prop.Key() { + case "ip": + d.IP, _ = prop.Value() + default: + } + } + + return d +} + func either[T Auditable, R any](old, new T, fn func(T) R, auditAction database.AuditAction) R { if ResourceID(new) != uuid.Nil { return fn(new) diff --git a/coderd/audit/request_test.go b/coderd/audit/request_test.go new file mode 100644 index 0000000000000..e0040425d4683 --- /dev/null +++ b/coderd/audit/request_test.go @@ -0,0 +1,33 @@ +package audit_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/propagation" + + "github.com/coder/coder/v2/coderd/audit" +) + +func TestBaggage(t *testing.T) { + t.Parallel() + prop := propagation.NewCompositeTextMapPropagator( + propagation.TraceContext{}, + propagation.Baggage{}, + ) + + expected := audit.WorkspaceBuildBaggage{ + IP: "127.0.0.1", + } + + ctx, err := audit.BaggageToContext(context.Background(), expected) + require.NoError(t, err) + + carrier := propagation.MapCarrier{} + prop.Inject(ctx, carrier) + bCtx := prop.Extract(ctx, carrier) + got := audit.BaggageFromContext(bCtx) + + require.Equal(t, expected, got) +} diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index 07c586b2331bb..f3b3ff58460b0 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -184,7 +184,8 @@ func (e *Executor) runOnce(t time.Time) Stats { builder = builder.ActiveVersion() } - build, job, err = builder.Build(e.ctx, tx, nil) + build, job, err = builder.Build(e.ctx, tx, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"}) + if err != nil { log.Error(e.ctx, "unable to transition workspace", slog.F("transition", nextTransition), diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index bffd855da6b6b..e138584498d37 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -4589,6 +4589,7 @@ func (q *FakeQuerier) InsertProvisionerJob(_ context.Context, arg database.Inser Type: arg.Type, Input: arg.Input, Tags: arg.Tags, + TraceMetadata: arg.TraceMetadata, } job.JobStatus = provisonerJobStatus(job) q.provisionerJobs = append(q.provisionerJobs, job) diff --git a/coderd/externalauth.go b/coderd/externalauth.go index 31dff667c28e7..774a5f860397d 100644 --- a/coderd/externalauth.go +++ b/coderd/externalauth.go @@ -6,9 +6,8 @@ import ( "fmt" "net/http" - "golang.org/x/sync/errgroup" - "github.com/sqlc-dev/pqtype" + "golang.org/x/sync/errgroup" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 5afb85565c50b..38038df49dd90 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -912,12 +912,15 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. s.Logger.Error(ctx, "marshal workspace resource info for failed job", slog.Error(err)) } + bag := audit.BaggageFromContext(ctx) + audit.WorkspaceBuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ Audit: *auditor, Log: s.Logger, UserID: job.InitiatorID, OrganizationID: workspace.OrganizationID, JobID: job.ID, + IP: bag.IP, Action: auditAction, Old: previousBuild, New: build, @@ -1259,12 +1262,15 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob) s.Logger.Error(ctx, "marshal resource info for successful job", slog.Error(err)) } + bag := audit.BaggageFromContext(ctx) + audit.WorkspaceBuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ Audit: *auditor, Log: s.Logger, UserID: job.InitiatorID, OrganizationID: workspace.OrganizationID, JobID: job.ID, + IP: bag.IP, Action: auditAction, Old: previousBuild, New: workspaceBuild, diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 16326f9945fb2..5025d778d83ad 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -17,6 +17,7 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" @@ -372,6 +373,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { func(action rbac.Action, object rbac.Objecter) bool { return api.Authorize(r, action, object) }, + audit.WorkspaceBuildBaggageFromRequest(r), ) var buildErr wsbuilder.BuildError if xerrors.As(err, &buildErr) { diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index c5c1d353d2b95..1f487e6915d40 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -12,6 +12,8 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" "golang.org/x/xerrors" "cdr.dev/slog" @@ -29,11 +31,22 @@ import ( func TestWorkspaceBuild(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + otel.SetTextMapPropagator( + propagation.NewCompositeTextMapPropagator( + propagation.TraceContext{}, + propagation.Baggage{}, + ), + ) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + Auditor: auditor, + }) user := coderdtest.CreateFirstUser(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + auditor.ResetLogs() workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) @@ -41,6 +54,8 @@ func TestWorkspaceBuild(t *testing.T) { _, err := client.WorkspaceBuild(ctx, workspace.LatestBuild.ID) require.NoError(t, err) + require.Len(t, auditor.AuditLogs(), 1) + require.Equal(t, auditor.AuditLogs()[0].Ip.IPNet.IP.String(), "127.0.0.1") } func TestWorkspaceBuildByBuildNumber(t *testing.T) { @@ -854,3 +869,185 @@ func TestWorkspaceBuildDebugMode(t *testing.T) { require.Equal(t, 2, logsProcessed) }) } + +func TestPostWorkspaceBuild(t *testing.T) { + t.Parallel() + t.Run("NoTemplateVersion", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: uuid.New(), + Transition: codersdk.WorkspaceTransitionStart, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("TemplateVersionFailedImport", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + ProvisionApply: []*proto.Response{{}}, + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.CreateWorkspace(ctx, user.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: "workspace", + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("AlreadyActive", func(t *testing.T) { + t.Parallel() + client, closer := coderdtest.NewWithProvisionerCloser(t, nil) + defer closer.Close() + + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + closer.Close() + // Close here so workspace build doesn't process! + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: codersdk.WorkspaceTransitionStart, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusConflict, apiErr.StatusCode()) + }) + + t.Run("Audit", func(t *testing.T) { + t.Parallel() + + otel.SetTextMapPropagator( + propagation.NewCompositeTextMapPropagator( + propagation.TraceContext{}, + propagation.Baggage{}, + ), + ) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + auditor.ResetLogs() + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: codersdk.WorkspaceTransitionStart, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) + + require.Len(t, auditor.AuditLogs(), 1) + require.Equal(t, auditor.AuditLogs()[0].Ip.IPNet.IP.String(), "127.0.0.1") + }) + + t.Run("IncrementBuildNumber", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: codersdk.WorkspaceTransitionStart, + }) + require.NoError(t, err) + require.Equal(t, workspace.LatestBuild.BuildNumber+1, build.BuildNumber) + }) + + t.Run("WithState", func(t *testing.T) { + t.Parallel() + client, closeDaemon := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + wantState := []byte("something") + _ = closeDaemon.Close() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: codersdk.WorkspaceTransitionStart, + ProvisionerState: wantState, + }) + require.NoError(t, err) + gotState, err := client.WorkspaceBuildState(ctx, build.ID) + require.NoError(t, err) + require.Equal(t, wantState, gotState) + }) + + t.Run("Delete", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionDelete, + }) + require.NoError(t, err) + require.Equal(t, workspace.LatestBuild.BuildNumber+1, build.BuildNumber) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) + + res, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ + Owner: user.UserID.String(), + }) + require.NoError(t, err) + require.Len(t, res.Workspaces, 0) + }) +} diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 56cfc8e2436b4..c495d250287c0 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -512,7 +512,9 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req db, func(action rbac.Action, object rbac.Objecter) bool { return api.Authorize(r, action, object) - }) + }, + audit.WorkspaceBuildBaggageFromRequest(r), + ) return err }, nil) var bldErr wsbuilder.BuildError diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 74a91c658ed7a..a12c262cb14f5 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1563,155 +1563,6 @@ func TestOffsetLimit(t *testing.T) { require.Len(t, ws.Workspaces, 0) } -func TestPostWorkspaceBuild(t *testing.T) { - t.Parallel() - t.Run("NoTemplateVersion", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - _, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: uuid.New(), - Transition: codersdk.WorkspaceTransitionStart, - }) - require.Error(t, err) - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) - }) - - t.Run("TemplateVersionFailedImport", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ - ProvisionApply: []*proto.Response{{}}, - }) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - _, err := client.CreateWorkspace(ctx, user.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{ - TemplateID: template.ID, - Name: "workspace", - }) - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) - }) - - t.Run("AlreadyActive", func(t *testing.T) { - t.Parallel() - client, closer := coderdtest.NewWithProvisionerCloser(t, nil) - defer closer.Close() - - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - closer.Close() - // Close here so workspace build doesn't process! - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - _, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: template.ActiveVersionID, - Transition: codersdk.WorkspaceTransitionStart, - }) - require.Error(t, err) - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusConflict, apiErr.StatusCode()) - }) - - t.Run("IncrementBuildNumber", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: template.ActiveVersionID, - Transition: codersdk.WorkspaceTransitionStart, - }) - require.NoError(t, err) - require.Equal(t, workspace.LatestBuild.BuildNumber+1, build.BuildNumber) - }) - - t.Run("WithState", func(t *testing.T) { - t.Parallel() - client, closeDaemon := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{ - IncludeProvisionerDaemon: true, - }) - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) - wantState := []byte("something") - _ = closeDaemon.Close() - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: template.ActiveVersionID, - Transition: codersdk.WorkspaceTransitionStart, - ProvisionerState: wantState, - }) - require.NoError(t, err) - gotState, err := client.WorkspaceBuildState(ctx, build.ID) - require.NoError(t, err) - require.Equal(t, wantState, gotState) - }) - - t.Run("Delete", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - Transition: codersdk.WorkspaceTransitionDelete, - }) - require.NoError(t, err) - require.Equal(t, workspace.LatestBuild.BuildNumber+1, build.BuildNumber) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) - - res, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ - Owner: user.UserID.String(), - }) - require.NoError(t, err) - require.Len(t, res.Workspaces, 0) - }) -} - func TestWorkspaceUpdateAutostart(t *testing.T) { t.Parallel() dublinLoc := mustLocation(t, "Europe/Dublin") diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 008bc88ab72ab..66e784d8462b6 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -15,6 +15,7 @@ import ( "github.com/sqlc-dev/pqtype" "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -201,16 +202,20 @@ func (b *Builder) Build( ctx context.Context, store database.Store, authFunc func(action rbac.Action, object rbac.Objecter) bool, + auditBaggage audit.WorkspaceBuildBaggage, ) ( *database.WorkspaceBuild, *database.ProvisionerJob, error, ) { - b.ctx = ctx + var err error + b.ctx, err = audit.BaggageToContext(ctx, auditBaggage) + if err != nil { + return nil, nil, xerrors.Errorf("create audit baggage: %w", err) + } // Run the build in a transaction with RepeatableRead isolation, and retries. // RepeatableRead isolation ensures that we get a consistent view of the database while // computing the new build. This simplifies the logic so that we do not need to worry if // later reads are consistent with earlier ones. - var err error for retries := 0; retries < 5; retries++ { var workspaceBuild *database.WorkspaceBuild var provisionerJob *database.ProvisionerJob diff --git a/coderd/wsbuilder/wsbuilder_test.go b/coderd/wsbuilder/wsbuilder_test.go index 224a40cfb82d8..c55d440ac2c28 100644 --- a/coderd/wsbuilder/wsbuilder_test.go +++ b/coderd/wsbuilder/wsbuilder_test.go @@ -12,7 +12,10 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -88,7 +91,7 @@ func TestBuilder_NoOptions(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -123,7 +126,48 @@ func TestBuilder_Initiator(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Initiator(otherUserID) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + req.NoError(err) +} + +func TestBuilder_Baggage(t *testing.T) { + t.Parallel() + req := require.New(t) + asrt := assert.New(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + otel.SetTextMapPropagator( + propagation.NewCompositeTextMapPropagator( + propagation.TraceContext{}, + propagation.Baggage{}, + ), + ) + + mDB := expectDB(t, + // Inputs + withTemplate, + withInactiveVersion(nil), + withLastBuildFound, + withRichParameters(nil), + withParameterSchemas(inactiveJobID, nil), + + // Outputs + expectProvisionerJob(func(job database.InsertProvisionerJobParams) { + asrt.Contains(string(job.TraceMetadata.RawMessage), "ip=127.0.0.1") + }), + withInTx, + expectBuild(func(bld database.InsertWorkspaceBuildParams) { + }), + expectBuildParameters(func(params database.InsertWorkspaceBuildParametersParams) { + }), + withBuild, + ) + + ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} + uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Initiator(otherUserID) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"}) req.NoError(err) } @@ -157,7 +201,7 @@ func TestBuilder_Reason(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Reason(database.BuildReasonAutostart) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -196,7 +240,7 @@ func TestBuilder_ActiveVersion(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).ActiveVersion() - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -274,7 +318,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(nextBuildParameters) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) t.Run("UsePreviousParameterValues", func(t *testing.T) { @@ -317,7 +361,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(nextBuildParameters) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -357,7 +401,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) bldErr := wsbuilder.BuildError{} req.ErrorAs(err, &bldErr) asrt.Equal(http.StatusBadRequest, bldErr.Status) @@ -394,7 +438,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(nextBuildParameters) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) bldErr := wsbuilder.BuildError{} req.ErrorAs(err, &bldErr) asrt.Equal(http.StatusBadRequest, bldErr.Status) @@ -456,7 +500,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). RichParameterValues(nextBuildParameters). VersionID(activeVersionID) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -516,7 +560,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). RichParameterValues(nextBuildParameters). VersionID(activeVersionID) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -574,7 +618,7 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). RichParameterValues(nextBuildParameters). VersionID(activeVersionID) - _, _, err := uut.Build(ctx, mDB, nil) + _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) } From 997493d4aea5e1864ad83bcaeb4ec70bc4e4ee71 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Wed, 18 Oct 2023 17:07:21 -0500 Subject: [PATCH 07/26] feat: add template setting to require active template version (#10277) --- cli/server.go | 2 +- cli/templates.go | 2 - cli/templateversionarchive.go | 2 - cli/templateversions.go | 2 - coderd/apidoc/docs.go | 8 + coderd/apidoc/swagger.json | 8 + coderd/autobuild/lifecycle_executor.go | 16 +- coderd/autobuild/lifecycle_executor_test.go | 50 +++++++ coderd/coderd.go | 14 ++ coderd/coderdtest/coderdtest.go | 19 ++- coderd/database/dbauthz/accesscontrol.go | 37 +++++ coderd/database/dbauthz/dbauthz.go | 39 ++++- coderd/database/dbauthz/dbauthz_test.go | 67 ++++++++- coderd/database/dbauthz/setup_test.go | 24 ++- coderd/database/dbfake/dbfake.go | 19 +++ coderd/database/dbmetrics/dbmetrics.go | 7 + coderd/database/dbmock/dbmock.go | 14 ++ coderd/database/dump.sql | 4 +- .../000166_template_active_version.down.sql | 25 ++++ .../000166_template_active_version.up.sql | 23 +++ coderd/database/modelmethods.go | 2 +- coderd/database/modelqueries.go | 1 + coderd/database/models.go | 2 + coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 31 +++- coderd/database/queries/templates.sql | 9 ++ coderd/templates.go | 22 ++- coderd/wsbuilder/wsbuilder.go | 39 ++--- codersdk/deployment.go | 2 + codersdk/organizations.go | 4 + codersdk/templates.go | 8 + docs/admin/audit-logs.md | 26 ++-- docs/api/schemas.md | 4 + docs/api/templates.md | 7 + enterprise/audit/table.go | 1 + enterprise/coderd/coderd.go | 14 +- enterprise/coderd/dbauthz/accesscontrol.go | 30 ++++ enterprise/coderd/templates_test.go | 46 ++++++ enterprise/coderd/workspacebuilds_test.go | 139 ++++++++++++++++++ enterprise/coderd/workspaces_test.go | 90 ++++++++++++ site/src/api/typesGenerated.ts | 5 + site/src/pages/CreateTemplatePage/utils.ts | 1 + .../TemplateSettingsForm.tsx | 1 + .../TemplateSettingsPage.test.tsx | 1 + .../TemplateScheduleForm.tsx | 2 + .../TemplateSchedulePage.test.tsx | 1 + site/src/testHelpers/entities.ts | 1 + 47 files changed, 802 insertions(+), 70 deletions(-) create mode 100644 coderd/database/dbauthz/accesscontrol.go create mode 100644 coderd/database/migrations/000166_template_active_version.down.sql create mode 100644 coderd/database/migrations/000166_template_active_version.up.sql create mode 100644 enterprise/coderd/dbauthz/accesscontrol.go create mode 100644 enterprise/coderd/workspacebuilds_test.go diff --git a/cli/server.go b/cli/server.go index 6b6b218fb7d6a..a6a08a0b947fd 100644 --- a/cli/server.go +++ b/cli/server.go @@ -940,7 +940,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. autobuildTicker := time.NewTicker(vals.AutobuildPollInterval.Value()) defer autobuildTicker.Stop() autobuildExecutor := autobuild.NewExecutor( - ctx, options.Database, options.Pubsub, coderAPI.TemplateScheduleStore, &coderAPI.Auditor, logger, autobuildTicker.C) + ctx, options.Database, options.Pubsub, coderAPI.TemplateScheduleStore, &coderAPI.Auditor, coderAPI.AccessControlStore, logger, autobuildTicker.C) autobuildExecutor.Run() hangDetectorTicker := time.NewTicker(vals.JobHangDetectorInterval.Value()) diff --git a/cli/templates.go b/cli/templates.go index 2dec46d226b5c..4f5b4f8f36d0b 100644 --- a/cli/templates.go +++ b/cli/templates.go @@ -6,8 +6,6 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" - "github.com/coder/pretty" - "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" diff --git a/cli/templateversionarchive.go b/cli/templateversionarchive.go index 740de869ff9c2..63c9d8a3de212 100644 --- a/cli/templateversionarchive.go +++ b/cli/templateversionarchive.go @@ -8,8 +8,6 @@ import ( "golang.org/x/xerrors" - "github.com/coder/pretty" - "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" diff --git a/cli/templateversions.go b/cli/templateversions.go index 5b99a46310996..a27d6a6d65af3 100644 --- a/cli/templateversions.go +++ b/cli/templateversions.go @@ -8,8 +8,6 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" - "github.com/coder/pretty" - "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index b5041179736d2..4151c62b40202 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -7816,6 +7816,10 @@ const docTemplate = `{ "description": "Name is the name of the template.", "type": "string" }, + "require_active_version": { + "description": "RequireActiveVersion mandates that workspaces are built with the active\ntemplate version.", + "type": "boolean" + }, "template_version_id": { "description": "VersionID is an in-progress or completed job to use as an initial version\nof the template.\n\nThis is required on creation to enable a user-flow of validating a\ntemplate works. There is no reason the data-model cannot support empty\ntemplates, but it doesn't make sense for users.", "type": "string", @@ -9994,6 +9998,10 @@ const docTemplate = `{ "terraform" ] }, + "require_active_version": { + "description": "RequireActiveVersion mandates that workspaces are built with the active\ntemplate version.", + "type": "boolean" + }, "time_til_dormant_autodelete_ms": { "type": "integer" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 6e2e5c0902ddb..df54a31a85a54 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -6969,6 +6969,10 @@ "description": "Name is the name of the template.", "type": "string" }, + "require_active_version": { + "description": "RequireActiveVersion mandates that workspaces are built with the active\ntemplate version.", + "type": "boolean" + }, "template_version_id": { "description": "VersionID is an in-progress or completed job to use as an initial version\nof the template.\n\nThis is required on creation to enable a user-flow of validating a\ntemplate works. There is no reason the data-model cannot support empty\ntemplates, but it doesn't make sense for users.", "type": "string", @@ -9028,6 +9032,10 @@ "type": "string", "enum": ["terraform"] }, + "require_active_version": { + "description": "RequireActiveVersion mandates that workspaces are built with the active\ntemplate version.", + "type": "boolean" + }, "time_til_dormant_autodelete_ms": { "type": "integer" }, diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index f3b3ff58460b0..d2fb16b033946 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -32,6 +32,7 @@ type Executor struct { db database.Store ps pubsub.Pubsub templateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore] + accessControlStore *atomic.Pointer[dbauthz.AccessControlStore] auditor *atomic.Pointer[audit.Auditor] log slog.Logger tick <-chan time.Time @@ -46,7 +47,7 @@ type Stats struct { } // New returns a new wsactions executor. -func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, tss *atomic.Pointer[schedule.TemplateScheduleStore], auditor *atomic.Pointer[audit.Auditor], log slog.Logger, tick <-chan time.Time) *Executor { +func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, tss *atomic.Pointer[schedule.TemplateScheduleStore], auditor *atomic.Pointer[audit.Auditor], acs *atomic.Pointer[dbauthz.AccessControlStore], log slog.Logger, tick <-chan time.Time) *Executor { le := &Executor{ //nolint:gocritic // Autostart has a limited set of permissions. ctx: dbauthz.AsAutostart(ctx), @@ -56,6 +57,7 @@ func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, tss * tick: tick, log: log.Named("autobuild"), auditor: auditor, + accessControlStore: acs, } return le } @@ -159,6 +161,12 @@ func (e *Executor) runOnce(t time.Time) Stats { return nil } + template, err := tx.GetTemplateByID(e.ctx, ws.TemplateID) + if err != nil { + log.Warn(e.ctx, "get template by id", slog.Error(err)) + } + accessControl := (*(e.accessControlStore.Load())).GetTemplateAccessControl(template) + latestJob, err := tx.GetProvisionerJobByID(e.ctx, latestBuild.JobID) if err != nil { log.Warn(e.ctx, "get last provisioner job for workspace %q: %w", slog.Error(err)) @@ -179,7 +187,7 @@ func (e *Executor) runOnce(t time.Time) Stats { Reason(reason) log.Debug(e.ctx, "auto building workspace", slog.F("transition", nextTransition)) if nextTransition == database.WorkspaceTransitionStart && - ws.AutomaticUpdates == database.AutomaticUpdatesAlways { + useActiveVersion(accessControl, ws) { log.Debug(e.ctx, "autostarting with active version") builder = builder.ActiveVersion() } @@ -470,3 +478,7 @@ func auditBuild(ctx context.Context, log slog.Logger, auditor audit.Auditor, par AdditionalFields: raw, }) } + +func useActiveVersion(opts dbauthz.TemplateAccessControl, ws database.Workspace) bool { + return opts.RequireActiveVersion || ws.AutomaticUpdates == database.AutomaticUpdatesAlways +} diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index fd37166ea86db..81bbf80603898 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -783,6 +783,56 @@ func TestExecutorAutostopTemplateDisabled(t *testing.T) { assert.Len(t, stats.Transitions, 0) } +// Test that an AGPL AccessControlStore properly disables +// functionality. +func TestExecutorRequireActiveVersion(t *testing.T) { + t.Parallel() + + var ( + sched = mustSchedule(t, "CRON_TZ=UTC 0 * * * *") + ticker = make(chan time.Time) + statCh = make(chan autobuild.Stats) + + ownerClient = coderdtest.New(t, &coderdtest.Options{ + AutobuildTicker: ticker, + IncludeProvisionerDaemon: true, + AutobuildStats: statCh, + TemplateScheduleStore: schedule.NewAGPLTemplateScheduleStore(), + }) + ) + owner := coderdtest.CreateFirstUser(t, ownerClient) + + // Create an active and inactive template version. We'll + // build a regular member's workspace using a non-active + // template version and assert that the field is not abided + // since there is no enterprise license. + activeVersion := coderdtest.CreateTemplateVersion(t, ownerClient, owner.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, ownerClient, owner.OrganizationID, activeVersion.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.RequireActiveVersion = true + ctr.VersionID = activeVersion.ID + }) + inactiveVersion := coderdtest.CreateTemplateVersion(t, ownerClient, owner.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.TemplateID = template.ID + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, ownerClient, activeVersion.ID) + memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) + ws := coderdtest.CreateWorkspace(t, memberClient, owner.OrganizationID, uuid.Nil, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.TemplateVersionID = inactiveVersion.ID + cwr.AutostartSchedule = ptr.Ref(sched.String()) + }) + _ = coderdtest.AwaitWorkspaceBuildJobCompleted(t, ownerClient, ws.LatestBuild.ID) + ws = coderdtest.MustTransitionWorkspace(t, memberClient, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop, func(req *codersdk.CreateWorkspaceBuildRequest) { + req.TemplateVersionID = inactiveVersion.ID + }) + require.Equal(t, inactiveVersion.ID, ws.LatestBuild.TemplateVersionID) + ticker <- sched.Next(ws.LatestBuild.CreatedAt) + stats := <-statCh + require.Len(t, stats.Transitions, 1) + + ws = coderdtest.MustWorkspace(t, memberClient, ws.ID) + require.Equal(t, inactiveVersion.ID, ws.LatestBuild.TemplateVersionID) +} + // TestExecutorFailedWorkspace test AGPL functionality which mainly // ensures that autostop actions as a result of a failed workspace // build do not trigger. diff --git a/coderd/coderd.go b/coderd/coderd.go index 7ab2e578462b1..81ec1e87e6b18 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -131,6 +131,7 @@ type Options struct { SetUserSiteRoles func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, roles []string) error TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore] UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore] + AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore] // AppSecurityKey is the crypto key used to sign and encrypt tokens related to // workspace applications. It consists of both a signing and encryption key. AppSecurityKey workspaceapps.SecurityKey @@ -208,11 +209,20 @@ func New(options *Options) *API { if options.Authorizer == nil { options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) } + + if options.AccessControlStore == nil { + options.AccessControlStore = &atomic.Pointer[dbauthz.AccessControlStore]{} + var tacs dbauthz.AccessControlStore = dbauthz.AGPLTemplateAccessControlStore{} + options.AccessControlStore.Store(&tacs) + } + options.Database = dbauthz.New( options.Database, options.Authorizer, options.Logger.Named("authz_querier"), + options.AccessControlStore, ) + experiments := ReadExperiments( options.Logger, options.DeploymentValues.Experiments.Value(), ) @@ -369,6 +379,7 @@ func New(options *Options) *API { Auditor: atomic.Pointer[audit.Auditor]{}, TemplateScheduleStore: options.TemplateScheduleStore, UserQuietHoursScheduleStore: options.UserQuietHoursScheduleStore, + AccessControlStore: options.AccessControlStore, Experiments: experiments, healthCheckGroup: &singleflight.Group[string, *healthcheck.Report]{}, Acquirer: provisionerdserver.NewAcquirer( @@ -1008,6 +1019,9 @@ type API struct { UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore] // DERPMapper mutates the DERPMap to include workspace proxies. DERPMapper atomic.Pointer[func(derpMap *tailcfg.DERPMap) *tailcfg.DERPMap] + // AccessControlStore is a pointer to an atomic pointer since it is + // passed to dbauthz. + AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore] HTTPAuth *HTTPAuthorizer diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 4eea44a226bfa..6310a48d04e2e 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -218,7 +218,6 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can if options.Database == nil { options.Database, options.Pubsub = dbtestutil.NewDB(t) - options.Database = dbauthz.New(options.Database, options.Authorizer, options.Logger.Leveled(slog.LevelDebug)) } // Some routes expect a deployment ID, so just make sure one exists. @@ -260,6 +259,10 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can t.Cleanup(closeBatcher) } + accessControlStore := &atomic.Pointer[dbauthz.AccessControlStore]{} + var acs dbauthz.AccessControlStore = dbauthz.AGPLTemplateAccessControlStore{} + accessControlStore.Store(&acs) + var templateScheduleStore atomic.Pointer[schedule.TemplateScheduleStore] if options.TemplateScheduleStore == nil { options.TemplateScheduleStore = schedule.NewAGPLTemplateScheduleStore() @@ -279,6 +282,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can options.Pubsub, &templateScheduleStore, &auditor, + accessControlStore, *options.Logger, options.AutobuildTicker, ).WithStatsChannel(options.AutobuildStats) @@ -416,6 +420,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can Authorizer: options.Authorizer, Telemetry: telemetry.NewNoop(), TemplateScheduleStore: &templateScheduleStore, + AccessControlStore: accessControlStore, TLSCertificates: options.TLSCertificates, TrialGenerator: options.TrialGenerator, TailnetCoordinator: options.Coordinator, @@ -915,7 +920,7 @@ func CreateWorkspace(t testing.TB, client *codersdk.Client, organization uuid.UU } // TransitionWorkspace is a convenience method for transitioning a workspace from one state to another. -func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition) codersdk.Workspace { +func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition, muts ...func(req *codersdk.CreateWorkspaceBuildRequest)) codersdk.Workspace { t.Helper() ctx := context.Background() workspace, err := client.Workspace(ctx, workspaceID) @@ -925,10 +930,16 @@ func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID template, err := client.Template(ctx, workspace.TemplateID) require.NoError(t, err, "fetch workspace template") - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + req := codersdk.CreateWorkspaceBuildRequest{ TemplateVersionID: template.ActiveVersionID, Transition: codersdk.WorkspaceTransition(to), - }) + } + + for _, mut := range muts { + mut(&req) + } + + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, req) require.NoError(t, err, "unexpected error transitioning workspace to %s", to) _ = AwaitWorkspaceBuildJobCompleted(t, client, build.ID) diff --git a/coderd/database/dbauthz/accesscontrol.go b/coderd/database/dbauthz/accesscontrol.go new file mode 100644 index 0000000000000..92417ff4114ba --- /dev/null +++ b/coderd/database/dbauthz/accesscontrol.go @@ -0,0 +1,37 @@ +package dbauthz + +import ( + "context" + + "github.com/google/uuid" + + "github.com/coder/coder/v2/coderd/database" +) + +// AccessControlStore fetches access control-related configuration +// that is used when determining whether an actor is authorized +// to interact with an RBAC object. +type AccessControlStore interface { + GetTemplateAccessControl(t database.Template) TemplateAccessControl + SetTemplateAccessControl(ctx context.Context, store database.Store, id uuid.UUID, opts TemplateAccessControl) error +} + +type TemplateAccessControl struct { + RequireActiveVersion bool +} + +// AGPLTemplateAccessControlStore always returns the defaults for access control +// settings. +type AGPLTemplateAccessControlStore struct{} + +var _ AccessControlStore = AGPLTemplateAccessControlStore{} + +func (AGPLTemplateAccessControlStore) GetTemplateAccessControl(database.Template) TemplateAccessControl { + return TemplateAccessControl{ + RequireActiveVersion: false, + } +} + +func (AGPLTemplateAccessControlStore) SetTemplateAccessControl(context.Context, database.Store, uuid.UUID, TemplateAccessControl) error { + return nil +} diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 038f4e0c92807..796c68a5031f8 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "sync/atomic" "time" "github.com/google/uuid" @@ -101,9 +102,10 @@ type querier struct { db database.Store auth rbac.Authorizer log slog.Logger + acs *atomic.Pointer[AccessControlStore] } -func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) database.Store { +func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger, acs *atomic.Pointer[AccessControlStore]) database.Store { // If the underlying db store is already a querier, return it. // Do not double wrap. if slices.Contains(db.Wrappers(), wrapname) { @@ -113,6 +115,7 @@ func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) data db: db, auth: authorizer, log: logger, + acs: acs, } } @@ -507,7 +510,7 @@ func (q *querier) Ping(ctx context.Context) (time.Duration, error) { func (q *querier) InTx(function func(querier database.Store) error, txOpts *sql.TxOptions) error { return q.db.InTx(func(tx database.Store) error { // Wrap the transaction store in a querier. - wrapped := New(tx, q.auth, q.log) + wrapped := New(tx, q.auth, q.log, q.acs) return function(wrapped) }, txOpts) } @@ -2200,7 +2203,7 @@ func (q *querier) InsertWorkspaceAppStats(ctx context.Context, arg database.Inse func (q *querier) InsertWorkspaceBuild(ctx context.Context, arg database.InsertWorkspaceBuildParams) error { w, err := q.db.GetWorkspaceByID(ctx, arg.WorkspaceID) if err != nil { - return err + return xerrors.Errorf("get workspace by id: %w", err) } var action rbac.Action = rbac.ActionUpdate @@ -2209,7 +2212,28 @@ func (q *querier) InsertWorkspaceBuild(ctx context.Context, arg database.InsertW } if err = q.authorizeContext(ctx, action, w.WorkspaceBuildRBAC(arg.Transition)); err != nil { - return err + return xerrors.Errorf("authorize context: %w", err) + } + + // If we're starting a workspace we need to check the template. + if arg.Transition == database.WorkspaceTransitionStart { + t, err := q.db.GetTemplateByID(ctx, w.TemplateID) + if err != nil { + return xerrors.Errorf("get template by id: %w", err) + } + + accessControl := (*q.acs.Load()).GetTemplateAccessControl(t) + + // If the template requires the active version we need to check if + // the user is a template admin. If they aren't and are attempting + // to use a non-active version then we must fail the request. + if accessControl.RequireActiveVersion { + if arg.TemplateVersionID != t.ActiveVersionID { + if err = q.authorizeContext(ctx, rbac.ActionUpdate, t); err != nil { + return xerrors.Errorf("cannot use non-active version: %w", err) + } + } + } } return q.db.InsertWorkspaceBuild(ctx, arg) @@ -2442,6 +2466,13 @@ func (q *querier) UpdateTemplateACLByID(ctx context.Context, arg database.Update return fetchAndExec(q.log, q.auth, rbac.ActionCreate, fetch, q.db.UpdateTemplateACLByID)(ctx, arg) } +func (q *querier) UpdateTemplateAccessControlByID(ctx context.Context, arg database.UpdateTemplateAccessControlByIDParams) error { + fetch := func(ctx context.Context, arg database.UpdateTemplateAccessControlByIDParams) (database.Template, error) { + return q.db.GetTemplateByID(ctx, arg.ID) + } + return update(q.log, q.auth, fetch, q.db.UpdateTemplateAccessControlByID)(ctx, arg) +} + func (q *querier) UpdateTemplateActiveVersionByID(ctx context.Context, arg database.UpdateTemplateActiveVersionByIDParams) error { fetch := func(ctx context.Context, arg database.UpdateTemplateActiveVersionByIDParams) (database.Template, error) { return q.db.GetTemplateByID(ctx, arg.ID) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 28866396c8b86..a5847839b2478 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -21,6 +21,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/slice" + "github.com/coder/coder/v2/testutil" ) func TestAsNoActor(t *testing.T) { @@ -61,7 +62,7 @@ func TestAsNoActor(t *testing.T) { func TestPing(t *testing.T) { t.Parallel() - q := dbauthz.New(dbfake.New(), &coderdtest.RecordingAuthorizer{}, slog.Make()) + q := dbauthz.New(dbfake.New(), &coderdtest.RecordingAuthorizer{}, slog.Make(), accessControlStorePointer()) _, err := q.Ping(context.Background()) require.NoError(t, err, "must not error") } @@ -73,7 +74,7 @@ func TestInTX(t *testing.T) { db := dbfake.New() q := dbauthz.New(db, &coderdtest.RecordingAuthorizer{ Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: xerrors.New("custom error")}, - }, slog.Make()) + }, slog.Make(), accessControlStorePointer()) actor := rbac.Subject{ ID: uuid.NewString(), Roles: rbac.RoleNames{rbac.RoleOwner()}, @@ -109,8 +110,8 @@ func TestNew(t *testing.T) { // Double wrap should not cause an actual double wrap. So only 1 rbac call // should be made. - az := dbauthz.New(db, rec, slog.Make()) - az = dbauthz.New(az, rec, slog.Make()) + az := dbauthz.New(db, rec, slog.Make(), accessControlStorePointer()) + az = dbauthz.New(az, rec, slog.Make(), accessControlStorePointer()) w, err := az.GetWorkspaceByID(ctx, exp.ID) require.NoError(t, err, "must not error") @@ -127,7 +128,7 @@ func TestDBAuthzRecursive(t *testing.T) { t.Parallel() q := dbauthz.New(dbfake.New(), &coderdtest.RecordingAuthorizer{ Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil}, - }, slog.Make()) + }, slog.Make(), accessControlStorePointer()) actor := rbac.Subject{ ID: uuid.NewString(), Roles: rbac.RoleNames{rbac.RoleOwner()}, @@ -1213,13 +1214,67 @@ func (s *MethodTestSuite) TestWorkspace() { }).Asserts(rbac.ResourceWorkspace.WithOwner(u.ID.String()).InOrg(o.ID), rbac.ActionCreate) })) s.Run("Start/InsertWorkspaceBuild", s.Subtest(func(db database.Store, check *expects) { - w := dbgen.Workspace(s.T(), db, database.Workspace{}) + t := dbgen.Template(s.T(), db, database.Template{}) + w := dbgen.Workspace(s.T(), db, database.Workspace{ + TemplateID: t.ID, + }) check.Args(database.InsertWorkspaceBuildParams{ WorkspaceID: w.ID, Transition: database.WorkspaceTransitionStart, Reason: database.BuildReasonInitiator, }).Asserts(w.WorkspaceBuildRBAC(database.WorkspaceTransitionStart), rbac.ActionUpdate) })) + s.Run("Start/RequireActiveVersion/VersionMismatch/InsertWorkspaceBuild", s.Subtest(func(db database.Store, check *expects) { + t := dbgen.Template(s.T(), db, database.Template{}) + ctx := testutil.Context(s.T(), testutil.WaitShort) + err := db.UpdateTemplateAccessControlByID(ctx, database.UpdateTemplateAccessControlByIDParams{ + ID: t.ID, + RequireActiveVersion: true, + }) + require.NoError(s.T(), err) + v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: t.ID}, + }) + w := dbgen.Workspace(s.T(), db, database.Workspace{ + TemplateID: t.ID, + }) + check.Args(database.InsertWorkspaceBuildParams{ + WorkspaceID: w.ID, + Transition: database.WorkspaceTransitionStart, + Reason: database.BuildReasonInitiator, + TemplateVersionID: v.ID, + }).Asserts( + w.WorkspaceBuildRBAC(database.WorkspaceTransitionStart), rbac.ActionUpdate, + t, rbac.ActionUpdate, + ) + })) + s.Run("Start/RequireActiveVersion/VersionsMatch/InsertWorkspaceBuild", s.Subtest(func(db database.Store, check *expects) { + v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{}) + t := dbgen.Template(s.T(), db, database.Template{ + ActiveVersionID: v.ID, + }) + + ctx := testutil.Context(s.T(), testutil.WaitShort) + err := db.UpdateTemplateAccessControlByID(ctx, database.UpdateTemplateAccessControlByIDParams{ + ID: t.ID, + RequireActiveVersion: true, + }) + require.NoError(s.T(), err) + + w := dbgen.Workspace(s.T(), db, database.Workspace{ + TemplateID: t.ID, + }) + // Assert that we do not check for template update permissions + // if versions match. + check.Args(database.InsertWorkspaceBuildParams{ + WorkspaceID: w.ID, + Transition: database.WorkspaceTransitionStart, + Reason: database.BuildReasonInitiator, + TemplateVersionID: v.ID, + }).Asserts( + w.WorkspaceBuildRBAC(database.WorkspaceTransitionStart), rbac.ActionUpdate, + ) + })) s.Run("Delete/InsertWorkspaceBuild", s.Subtest(func(db database.Store, check *expects) { w := dbgen.Workspace(s.T(), db, database.Workspace{}) check.Args(database.InsertWorkspaceBuildParams{ diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go index 9efcf5ef9418e..968b882f2d263 100644 --- a/coderd/database/dbauthz/setup_test.go +++ b/coderd/database/dbauthz/setup_test.go @@ -6,6 +6,7 @@ import ( "reflect" "sort" "strings" + "sync/atomic" "testing" "github.com/golang/mock/gomock" @@ -59,7 +60,7 @@ func (s *MethodTestSuite) SetupSuite() { mockStore := dbmock.NewMockStore(ctrl) // We intentionally set no expectations apart from this. mockStore.EXPECT().Wrappers().Return([]string{}).AnyTimes() - az := dbauthz.New(mockStore, nil, slog.Make()) + az := dbauthz.New(mockStore, nil, slog.Make(), accessControlStorePointer()) // Take the underlying type of the interface. azt := reflect.TypeOf(az).Elem() s.methodAccounting = make(map[string]int) @@ -110,7 +111,7 @@ func (s *MethodTestSuite) Subtest(testCaseF func(db database.Store, check *expec rec := &coderdtest.RecordingAuthorizer{ Wrapped: fakeAuthorizer, } - az := dbauthz.New(db, rec, slog.Make()) + az := dbauthz.New(db, rec, slog.Make(), accessControlStorePointer()) actor := rbac.Subject{ ID: uuid.NewString(), Roles: rbac.RoleNames{rbac.RoleOwner()}, @@ -398,3 +399,22 @@ func (emptyPreparedAuthorized) Authorize(_ context.Context, _ rbac.Object) error func (emptyPreparedAuthorized) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) { return "", nil } + +func accessControlStorePointer() *atomic.Pointer[dbauthz.AccessControlStore] { + acs := &atomic.Pointer[dbauthz.AccessControlStore]{} + var tacs dbauthz.AccessControlStore = fakeAccessControlStore{} + acs.Store(&tacs) + return acs +} + +type fakeAccessControlStore struct{} + +func (fakeAccessControlStore) GetTemplateAccessControl(t database.Template) dbauthz.TemplateAccessControl { + return dbauthz.TemplateAccessControl{ + RequireActiveVersion: t.RequireActiveVersion, + } +} + +func (fakeAccessControlStore) SetTemplateAccessControl(context.Context, database.Store, uuid.UUID, dbauthz.TemplateAccessControl) error { + panic("not implemented") +} diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index e138584498d37..2d46bbe76ec9a 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -5643,6 +5643,25 @@ func (q *FakeQuerier) UpdateTemplateACLByID(_ context.Context, arg database.Upda return sql.ErrNoRows } +func (q *FakeQuerier) UpdateTemplateAccessControlByID(_ context.Context, arg database.UpdateTemplateAccessControlByIDParams) error { + if err := validateDatabaseType(arg); err != nil { + return err + } + + q.mutex.Lock() + defer q.mutex.Unlock() + + for idx, tpl := range q.templates { + if tpl.ID != arg.ID { + continue + } + q.templates[idx].RequireActiveVersion = arg.RequireActiveVersion + return nil + } + + return sql.ErrNoRows +} + func (q *FakeQuerier) UpdateTemplateActiveVersionByID(_ context.Context, arg database.UpdateTemplateActiveVersionByIDParams) error { if err := validateDatabaseType(arg); err != nil { return err diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index ece7020139b0f..ed3ff65b18378 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -1523,6 +1523,13 @@ func (m metricsStore) UpdateTemplateACLByID(ctx context.Context, arg database.Up return err } +func (m metricsStore) UpdateTemplateAccessControlByID(ctx context.Context, arg database.UpdateTemplateAccessControlByIDParams) error { + start := time.Now() + r0 := m.s.UpdateTemplateAccessControlByID(ctx, arg) + m.queryLatencies.WithLabelValues("UpdateTemplateAccessControlByID").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) UpdateTemplateActiveVersionByID(ctx context.Context, arg database.UpdateTemplateActiveVersionByIDParams) error { start := time.Now() err := m.s.UpdateTemplateActiveVersionByID(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 31614be3ae919..491937c6aeb6d 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -3213,6 +3213,20 @@ func (mr *MockStoreMockRecorder) UpdateTemplateACLByID(arg0, arg1 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateTemplateACLByID", reflect.TypeOf((*MockStore)(nil).UpdateTemplateACLByID), arg0, arg1) } +// UpdateTemplateAccessControlByID mocks base method. +func (m *MockStore) UpdateTemplateAccessControlByID(arg0 context.Context, arg1 database.UpdateTemplateAccessControlByIDParams) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateTemplateAccessControlByID", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateTemplateAccessControlByID indicates an expected call of UpdateTemplateAccessControlByID. +func (mr *MockStoreMockRecorder) UpdateTemplateAccessControlByID(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateTemplateAccessControlByID", reflect.TypeOf((*MockStore)(nil).UpdateTemplateAccessControlByID), arg0, arg1) +} + // UpdateTemplateActiveVersionByID mocks base method. func (m *MockStore) UpdateTemplateActiveVersionByID(arg0 context.Context, arg1 database.UpdateTemplateActiveVersionByIDParams) error { m.ctrl.T.Helper() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index ab6eb95252f2d..0e79c875f93bc 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -755,7 +755,8 @@ CREATE TABLE templates ( time_til_dormant_autodelete bigint DEFAULT 0 NOT NULL, autostop_requirement_days_of_week smallint DEFAULT 0 NOT NULL, autostop_requirement_weeks bigint DEFAULT 0 NOT NULL, - autostart_block_days_of_week smallint DEFAULT 0 NOT NULL + autostart_block_days_of_week smallint DEFAULT 0 NOT NULL, + require_active_version boolean DEFAULT false NOT NULL ); COMMENT ON COLUMN templates.default_ttl IS 'The default duration for autostop for workspaces created from this template.'; @@ -800,6 +801,7 @@ CREATE VIEW template_with_users AS templates.autostop_requirement_days_of_week, templates.autostop_requirement_weeks, templates.autostart_block_days_of_week, + templates.require_active_version, COALESCE(visible_users.avatar_url, ''::text) AS created_by_avatar_url, COALESCE(visible_users.username, ''::text) AS created_by_username FROM (public.templates diff --git a/coderd/database/migrations/000166_template_active_version.down.sql b/coderd/database/migrations/000166_template_active_version.down.sql new file mode 100644 index 0000000000000..d3b4bba305e02 --- /dev/null +++ b/coderd/database/migrations/000166_template_active_version.down.sql @@ -0,0 +1,25 @@ +BEGIN; + +-- Update the template_with_users view; +DROP VIEW template_with_users; + +ALTER TABLE templates DROP COLUMN require_active_version; + +-- If you need to update this view, put 'DROP VIEW template_with_users;' before this. +CREATE VIEW + template_with_users +AS + SELECT + templates.*, + coalesce(visible_users.avatar_url, '') AS created_by_avatar_url, + coalesce(visible_users.username, '') AS created_by_username + FROM + templates + LEFT JOIN + visible_users + ON + templates.created_by = visible_users.id; + +COMMENT ON VIEW template_with_users IS 'Joins in the username + avatar url of the created by user.'; + +COMMIT; diff --git a/coderd/database/migrations/000166_template_active_version.up.sql b/coderd/database/migrations/000166_template_active_version.up.sql new file mode 100644 index 0000000000000..a9255505eede7 --- /dev/null +++ b/coderd/database/migrations/000166_template_active_version.up.sql @@ -0,0 +1,23 @@ +BEGIN; + +DROP VIEW template_with_users; + +ALTER TABLE templates ADD COLUMN require_active_version boolean NOT NULL DEFAULT 'f'; + +CREATE VIEW + template_with_users +AS + SELECT + templates.*, + coalesce(visible_users.avatar_url, '') AS created_by_avatar_url, + coalesce(visible_users.username, '') AS created_by_username + FROM + templates + LEFT JOIN + visible_users + ON + templates.created_by = visible_users.id; + +COMMENT ON VIEW template_with_users IS 'Joins in the username + avatar url of the created by user.'; + +COMMIT; diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 66e1618bf570f..ed5bf76b784d7 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -179,7 +179,7 @@ func (w Workspace) ApplicationConnectRBAC() rbac.Object { } func (w Workspace) WorkspaceBuildRBAC(transition WorkspaceTransition) rbac.Object { - // If a workspace is locked it cannot be built. + // If a workspace is dormant it cannot be built. // However we need to allow stopping a workspace by a caller once a workspace // is locked (e.g. for autobuild). Additionally, if a user wants to delete // a locked workspace, they shouldn't have to have it unlocked first. diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index bb380a8293bdc..5c78600237e1d 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -86,6 +86,7 @@ func (q *sqlQuerier) GetAuthorizedTemplates(ctx context.Context, arg GetTemplate &i.AutostopRequirementDaysOfWeek, &i.AutostopRequirementWeeks, &i.AutostartBlockDaysOfWeek, + &i.RequireActiveVersion, &i.CreatedByAvatarURL, &i.CreatedByUsername, ); err != nil { diff --git a/coderd/database/models.go b/coderd/database/models.go index 1684e4fe5ebeb..c67afa74faa9a 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1892,6 +1892,7 @@ type Template struct { AutostopRequirementDaysOfWeek int16 `db:"autostop_requirement_days_of_week" json:"autostop_requirement_days_of_week"` AutostopRequirementWeeks int64 `db:"autostop_requirement_weeks" json:"autostop_requirement_weeks"` AutostartBlockDaysOfWeek int16 `db:"autostart_block_days_of_week" json:"autostart_block_days_of_week"` + RequireActiveVersion bool `db:"require_active_version" json:"require_active_version"` CreatedByAvatarURL sql.NullString `db:"created_by_avatar_url" json:"created_by_avatar_url"` CreatedByUsername string `db:"created_by_username" json:"created_by_username"` } @@ -1930,6 +1931,7 @@ type TemplateTable struct { AutostopRequirementWeeks int64 `db:"autostop_requirement_weeks" json:"autostop_requirement_weeks"` // A bitmap of days of week that autostart of a workspace is not allowed. Default allows all days. This is intended as a cost savings measure to prevent auto start on weekends (for example). AutostartBlockDaysOfWeek int16 `db:"autostart_block_days_of_week" json:"autostart_block_days_of_week"` + RequireActiveVersion bool `db:"require_active_version" json:"require_active_version"` } // Joins in the username + avatar url of the created by user. diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 99503ba40e3d6..bb2e95d17c309 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -301,6 +301,7 @@ type sqlcQuerier interface { UpdateProvisionerJobWithCompleteByID(ctx context.Context, arg UpdateProvisionerJobWithCompleteByIDParams) error UpdateReplica(ctx context.Context, arg UpdateReplicaParams) (Replica, error) UpdateTemplateACLByID(ctx context.Context, arg UpdateTemplateACLByIDParams) error + UpdateTemplateAccessControlByID(ctx context.Context, arg UpdateTemplateAccessControlByIDParams) error UpdateTemplateActiveVersionByID(ctx context.Context, arg UpdateTemplateActiveVersionByIDParams) error UpdateTemplateDeletedByID(ctx context.Context, arg UpdateTemplateDeletedByIDParams) error UpdateTemplateMetaByID(ctx context.Context, arg UpdateTemplateMetaByIDParams) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index fc301d427fa8d..d4978e5d141e8 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -4726,7 +4726,7 @@ func (q *sqlQuerier) GetTemplateAverageBuildTime(ctx context.Context, arg GetTem const getTemplateByID = `-- name: GetTemplateByID :one SELECT - id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, created_by_avatar_url, created_by_username + id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, require_active_version, created_by_avatar_url, created_by_username FROM template_with_users WHERE @@ -4764,6 +4764,7 @@ func (q *sqlQuerier) GetTemplateByID(ctx context.Context, id uuid.UUID) (Templat &i.AutostopRequirementDaysOfWeek, &i.AutostopRequirementWeeks, &i.AutostartBlockDaysOfWeek, + &i.RequireActiveVersion, &i.CreatedByAvatarURL, &i.CreatedByUsername, ) @@ -4772,7 +4773,7 @@ func (q *sqlQuerier) GetTemplateByID(ctx context.Context, id uuid.UUID) (Templat const getTemplateByOrganizationAndName = `-- name: GetTemplateByOrganizationAndName :one SELECT - id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, created_by_avatar_url, created_by_username + id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, require_active_version, created_by_avatar_url, created_by_username FROM template_with_users AS templates WHERE @@ -4818,6 +4819,7 @@ func (q *sqlQuerier) GetTemplateByOrganizationAndName(ctx context.Context, arg G &i.AutostopRequirementDaysOfWeek, &i.AutostopRequirementWeeks, &i.AutostartBlockDaysOfWeek, + &i.RequireActiveVersion, &i.CreatedByAvatarURL, &i.CreatedByUsername, ) @@ -4825,7 +4827,7 @@ func (q *sqlQuerier) GetTemplateByOrganizationAndName(ctx context.Context, arg G } const getTemplates = `-- name: GetTemplates :many -SELECT id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, created_by_avatar_url, created_by_username FROM template_with_users AS templates +SELECT id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, require_active_version, created_by_avatar_url, created_by_username FROM template_with_users AS templates ORDER BY (name, id) ASC ` @@ -4864,6 +4866,7 @@ func (q *sqlQuerier) GetTemplates(ctx context.Context) ([]Template, error) { &i.AutostopRequirementDaysOfWeek, &i.AutostopRequirementWeeks, &i.AutostartBlockDaysOfWeek, + &i.RequireActiveVersion, &i.CreatedByAvatarURL, &i.CreatedByUsername, ); err != nil { @@ -4882,7 +4885,7 @@ func (q *sqlQuerier) GetTemplates(ctx context.Context) ([]Template, error) { const getTemplatesWithFilter = `-- name: GetTemplatesWithFilter :many SELECT - id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, created_by_avatar_url, created_by_username + id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, default_ttl, created_by, icon, user_acl, group_acl, display_name, allow_user_cancel_workspace_jobs, max_ttl, allow_user_autostart, allow_user_autostop, failure_ttl, time_til_dormant, time_til_dormant_autodelete, autostop_requirement_days_of_week, autostop_requirement_weeks, autostart_block_days_of_week, require_active_version, created_by_avatar_url, created_by_username FROM template_with_users AS templates WHERE @@ -4958,6 +4961,7 @@ func (q *sqlQuerier) GetTemplatesWithFilter(ctx context.Context, arg GetTemplate &i.AutostopRequirementDaysOfWeek, &i.AutostopRequirementWeeks, &i.AutostartBlockDaysOfWeek, + &i.RequireActiveVersion, &i.CreatedByAvatarURL, &i.CreatedByUsername, ); err != nil { @@ -5054,6 +5058,25 @@ func (q *sqlQuerier) UpdateTemplateACLByID(ctx context.Context, arg UpdateTempla return err } +const updateTemplateAccessControlByID = `-- name: UpdateTemplateAccessControlByID :exec +UPDATE + templates +SET + require_active_version = $2 +WHERE + id = $1 +` + +type UpdateTemplateAccessControlByIDParams struct { + ID uuid.UUID `db:"id" json:"id"` + RequireActiveVersion bool `db:"require_active_version" json:"require_active_version"` +} + +func (q *sqlQuerier) UpdateTemplateAccessControlByID(ctx context.Context, arg UpdateTemplateAccessControlByIDParams) error { + _, err := q.db.ExecContext(ctx, updateTemplateAccessControlByID, arg.ID, arg.RequireActiveVersion) + return err +} + const updateTemplateActiveVersionByID = `-- name: UpdateTemplateActiveVersionByID :exec UPDATE templates diff --git a/coderd/database/queries/templates.sql b/coderd/database/queries/templates.sql index e89c245b0391d..c5bc72d7911d6 100644 --- a/coderd/database/queries/templates.sql +++ b/coderd/database/queries/templates.sql @@ -169,3 +169,12 @@ SELECT coalesce((PERCENTILE_DISC(0.95) WITHIN GROUP(ORDER BY exec_time_sec) FILTER (WHERE transition = 'delete')), -1)::FLOAT AS delete_95 FROM build_times ; + +-- name: UpdateTemplateAccessControlByID :exec +UPDATE + templates +SET + require_active_version = $2 +WHERE + id = $1 +; diff --git a/coderd/templates.go b/coderd/templates.go index 63fec1c63c9d6..c1f3bc97a01c3 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -347,6 +347,15 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque return xerrors.Errorf("insert template: %s", err) } + if createTemplate.RequireActiveVersion { + err = (*api.AccessControlStore.Load()).SetTemplateAccessControl(ctx, tx, id, dbauthz.TemplateAccessControl{ + RequireActiveVersion: createTemplate.RequireActiveVersion, + }) + if err != nil { + return xerrors.Errorf("set template access control: %w", err) + } + } + dbTemplate, err = tx.GetTemplateByID(ctx, id) if err != nil { return xerrors.Errorf("get template by id: %s", err) @@ -614,7 +623,8 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { req.AutostopRequirement.Weeks == scheduleOpts.AutostopRequirement.Weeks && req.FailureTTLMillis == time.Duration(template.FailureTTL).Milliseconds() && req.TimeTilDormantMillis == time.Duration(template.TimeTilDormant).Milliseconds() && - req.TimeTilDormantAutoDeleteMillis == time.Duration(template.TimeTilDormantAutoDelete).Milliseconds() { + req.TimeTilDormantAutoDeleteMillis == time.Duration(template.TimeTilDormantAutoDelete).Milliseconds() && + req.RequireActiveVersion == template.RequireActiveVersion { return nil } @@ -638,6 +648,15 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { return xerrors.Errorf("update template metadata: %w", err) } + if template.RequireActiveVersion != req.RequireActiveVersion { + err = (*api.AccessControlStore.Load()).SetTemplateAccessControl(ctx, tx, template.ID, dbauthz.TemplateAccessControl{ + RequireActiveVersion: req.RequireActiveVersion, + }) + if err != nil { + return xerrors.Errorf("set template access control: %w", err) + } + } + updated, err = tx.GetTemplateByID(ctx, template.ID) if err != nil { return xerrors.Errorf("fetch updated template metadata: %w", err) @@ -824,5 +843,6 @@ func (api *API) convertTemplate( AutostartRequirement: codersdk.TemplateAutostartRequirement{ DaysOfWeek: codersdk.BitmapToWeekdays(template.AutostartAllowedDays()), }, + RequireActiveVersion: template.RequireActiveVersion, } } diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 66e784d8462b6..80d5cca80f313 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -11,7 +11,6 @@ import ( "time" "github.com/google/uuid" - "github.com/lib/pq" "github.com/sqlc-dev/pqtype" "golang.org/x/xerrors" @@ -216,28 +215,18 @@ func (b *Builder) Build( // RepeatableRead isolation ensures that we get a consistent view of the database while // computing the new build. This simplifies the logic so that we do not need to worry if // later reads are consistent with earlier ones. - for retries := 0; retries < 5; retries++ { - var workspaceBuild *database.WorkspaceBuild - var provisionerJob *database.ProvisionerJob - err := store.InTx(func(store database.Store) error { - b.store = store - workspaceBuild, provisionerJob, err = b.buildTx(authFunc) - return err - }, &sql.TxOptions{Isolation: sql.LevelRepeatableRead}) - var pqe *pq.Error - if xerrors.As(err, &pqe) { - if pqe.Code == "40001" { - // serialization error, retry - continue - } - } - if err != nil { - // Other (hard) error - return nil, nil, err - } - return workspaceBuild, provisionerJob, nil + var workspaceBuild *database.WorkspaceBuild + var provisionerJob *database.ProvisionerJob + err = database.ReadModifyUpdate(store, func(tx database.Store) error { + var err error + b.store = tx + workspaceBuild, provisionerJob, err = b.buildTx(authFunc) + return err + }) + if err != nil { + return nil, nil, xerrors.Errorf("build tx: %w", err) } - return nil, nil, xerrors.Errorf("too many errors; last error: %w", err) + return workspaceBuild, provisionerJob, nil } // buildTx contains the business logic of computing a new build. Attributes of the new database objects are computed @@ -360,7 +349,11 @@ func (b *Builder) buildTx(authFunc func(action rbac.Action, object rbac.Objecter MaxDeadline: time.Time{}, // set by provisioner upon completion }) if err != nil { - return BuildError{http.StatusInternalServerError, "insert workspace build", err} + code := http.StatusInternalServerError + if rbac.IsUnauthorizedError(err) { + code = http.StatusUnauthorized + } + return BuildError{code, "insert workspace build", err} } names, values, err := b.getParameters() diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 4622808853aa5..bfcead815cf7c 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -50,6 +50,7 @@ const ( FeatureExternalTokenEncryption FeatureName = "external_token_encryption" FeatureTemplateAutostopRequirement FeatureName = "template_autostop_requirement" FeatureWorkspaceBatchActions FeatureName = "workspace_batch_actions" + FeatureAccessControl FeatureName = "access_control" ) // FeatureNames must be kept in-sync with the Feature enum above. @@ -70,6 +71,7 @@ var FeatureNames = []FeatureName{ FeatureExternalTokenEncryption, FeatureTemplateAutostopRequirement, FeatureWorkspaceBatchActions, + FeatureAccessControl, } // Humanize returns the feature name in a human-readable format. diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 6c10bc2c91abe..cc206180f81ae 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -124,6 +124,10 @@ type CreateTemplateRequest struct { // and must be explicitly granted to users or groups in the permissions settings // of the template. DisableEveryoneGroupAccess bool `json:"disable_everyone_group_access"` + + // RequireActiveVersion mandates that workspaces are built with the active + // template version. + RequireActiveVersion bool `json:"require_active_version"` } // CreateWorkspaceRequest provides options for creating a new workspace. diff --git a/codersdk/templates.go b/codersdk/templates.go index d0ee400a29b20..3a3240ca711b2 100644 --- a/codersdk/templates.go +++ b/codersdk/templates.go @@ -52,6 +52,10 @@ type Template struct { FailureTTLMillis int64 `json:"failure_ttl_ms"` TimeTilDormantMillis int64 `json:"time_til_dormant_ms"` TimeTilDormantAutoDeleteMillis int64 `json:"time_til_dormant_autodelete_ms"` + + // RequireActiveVersion mandates that workspaces are built with the active + // template version. + RequireActiveVersion bool `json:"require_active_version"` } // WeekdaysToBitmap converts a list of weekdays to a bitmap in accordance with @@ -221,6 +225,10 @@ type UpdateTemplateMeta struct { // from the template. This is useful for preventing dormant workspaces being immediately // deleted when updating the dormant_ttl field to a new, shorter value. UpdateWorkspaceDormantAt bool `json:"update_workspace_dormant_at"` + // RequireActiveVersion mandates workspaces built using this template + // use the active version of the template. This option has no + // effect on template admins. + RequireActiveVersion bool `json:"require_active_version"` } type TemplateExample struct { diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index c7da38bec58d1..af7a5724458d7 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -8,19 +8,19 @@ We track the following resources: -| Resource | | -| -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
login, logout, register, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idtrue
| -| AuditOAuthConvertState
|
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
to_login_typetrue
user_idtrue
| -| Group
create, write, delete |
FieldTracked
avatar_urltrue
display_nametrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
sourcefalse
| -| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| -| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
autostart_block_days_of_weektrue
autostop_requirement_days_of_weektrue
autostop_requirement_weekstrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_usernamefalse
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
time_til_dormanttrue
time_til_dormant_autodeletetrue
updated_atfalse
user_acltrue
| -| TemplateVersion
create, write |
FieldTracked
archivedtrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_usernamefalse
external_auth_providersfalse
idtrue
job_idfalse
messagefalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| -| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typetrue
quiet_hours_scheduletrue
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| -| Workspace
create, write, delete |
FieldTracked
automatic_updatestrue
autostart_scheduletrue
created_atfalse
deletedfalse
deleting_attrue
dormant_attrue
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| -| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_by_avatar_urlfalse
initiator_by_usernamefalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| -| WorkspaceProxy
|
FieldTracked
created_attrue
deletedfalse
derp_enabledtrue
derp_onlytrue
display_nametrue
icontrue
idtrue
nametrue
region_idtrue
token_hashed_secrettrue
updated_atfalse
urltrue
wildcard_hostnametrue
| +| Resource | | +| -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| APIKey
login, logout, register, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idtrue
| +| AuditOAuthConvertState
|
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
to_login_typetrue
user_idtrue
| +| Group
create, write, delete |
FieldTracked
avatar_urltrue
display_nametrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
sourcefalse
| +| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| +| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| +| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
autostart_block_days_of_weektrue
autostop_requirement_days_of_weektrue
autostop_requirement_weekstrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_usernamefalse
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
require_active_versiontrue
time_til_dormanttrue
time_til_dormant_autodeletetrue
updated_atfalse
user_acltrue
| +| TemplateVersion
create, write |
FieldTracked
archivedtrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_usernamefalse
external_auth_providersfalse
idtrue
job_idfalse
messagefalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| +| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typetrue
quiet_hours_scheduletrue
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| Workspace
create, write, delete |
FieldTracked
automatic_updatestrue
autostart_scheduletrue
created_atfalse
deletedfalse
deleting_attrue
dormant_attrue
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| +| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_by_avatar_urlfalse
initiator_by_usernamefalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| +| WorkspaceProxy
|
FieldTracked
created_attrue
deletedfalse
derp_enabledtrue
derp_onlytrue
display_nametrue
icontrue
idtrue
nametrue
region_idtrue
token_hashed_secrettrue
updated_atfalse
urltrue
wildcard_hostnametrue
| diff --git a/docs/api/schemas.md b/docs/api/schemas.md index cd09260f59546..da6715839647a 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1584,6 +1584,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "icon": "string", "max_ttl_ms": 0, "name": "string", + "require_active_version": true, "template_version_id": "0ba39c92-1f1b-4c32-aa3e-9925d7713eb1" } ``` @@ -1607,6 +1608,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `icon` | string | false | | Icon is a relative path or external URL that specifies an icon to be displayed in the dashboard. | | `max_ttl_ms` | integer | false | | Max ttl ms remove max_ttl once autostop_requirement is matured | | `name` | string | true | | Name is the name of the template. | +| `require_active_version` | boolean | false | | Require active version mandates that workspaces are built with the active template version. | | `template_version_id` | string | true | | Template version ID is an in-progress or completed job to use as an initial version of the template. | | This is required on creation to enable a user-flow of validating a template works. There is no reason the data-model cannot support empty templates, but it doesn't make sense for users. | @@ -4370,6 +4372,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "provisioner": "terraform", + "require_active_version": true, "time_til_dormant_autodelete_ms": 0, "time_til_dormant_ms": 0, "updated_at": "2019-08-24T14:15:22Z" @@ -4401,6 +4404,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `name` | string | false | | | | `organization_id` | string | false | | | | `provisioner` | string | false | | | +| `require_active_version` | boolean | false | | Require active version mandates that workspaces are built with the active template version. | | `time_til_dormant_autodelete_ms` | integer | false | | | | `time_til_dormant_ms` | integer | false | | | | `updated_at` | string | false | | | diff --git a/docs/api/templates.md b/docs/api/templates.md index a6cc2d4b5a367..08de540e2a7f2 100644 --- a/docs/api/templates.md +++ b/docs/api/templates.md @@ -61,6 +61,7 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/templat "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "provisioner": "terraform", + "require_active_version": true, "time_til_dormant_autodelete_ms": 0, "time_til_dormant_ms": 0, "updated_at": "2019-08-24T14:15:22Z" @@ -109,6 +110,7 @@ Status Code **200** | `» name` | string | false | | | | `» organization_id` | string(uuid) | false | | | | `» provisioner` | string | false | | | +| `» require_active_version` | boolean | false | | Require active version mandates that workspaces are built with the active template version. | | `» time_til_dormant_autodelete_ms` | integer | false | | | | `» time_til_dormant_ms` | integer | false | | | | `» updated_at` | string(date-time) | false | | | @@ -159,6 +161,7 @@ curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/templa "icon": "string", "max_ttl_ms": 0, "name": "string", + "require_active_version": true, "template_version_id": "0ba39c92-1f1b-4c32-aa3e-9925d7713eb1" } ``` @@ -211,6 +214,7 @@ curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/templa "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "provisioner": "terraform", + "require_active_version": true, "time_til_dormant_autodelete_ms": 0, "time_til_dormant_ms": 0, "updated_at": "2019-08-24T14:15:22Z" @@ -346,6 +350,7 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/templat "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "provisioner": "terraform", + "require_active_version": true, "time_til_dormant_autodelete_ms": 0, "time_til_dormant_ms": 0, "updated_at": "2019-08-24T14:15:22Z" @@ -657,6 +662,7 @@ curl -X GET http://coder-server:8080/api/v2/templates/{template} \ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "provisioner": "terraform", + "require_active_version": true, "time_til_dormant_autodelete_ms": 0, "time_til_dormant_ms": 0, "updated_at": "2019-08-24T14:15:22Z" @@ -775,6 +781,7 @@ curl -X PATCH http://coder-server:8080/api/v2/templates/{template} \ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "provisioner": "terraform", + "require_active_version": true, "time_til_dormant_autodelete_ms": 0, "time_til_dormant_ms": 0, "updated_at": "2019-08-24T14:15:22Z" diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 49f10d117057c..f272354d649ac 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -85,6 +85,7 @@ var auditableResourcesTypes = map[any]map[string]Action{ "failure_ttl": ActionTrack, "time_til_dormant": ActionTrack, "time_til_dormant_autodelete": ActionTrack, + "require_active_version": ActionTrack, }, &database.TemplateVersion{}: { "id": ActionTrack, diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index b49890589a6c3..55defaf9b964d 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -24,12 +24,13 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd" agplaudit "github.com/coder/coder/v2/coderd/audit" - "github.com/coder/coder/v2/coderd/database/dbauthz" + agpldbauthz "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/rbac" agplschedule "github.com/coder/coder/v2/coderd/schedule" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/dbauthz" "github.com/coder/coder/v2/enterprise/coderd/license" "github.com/coder/coder/v2/enterprise/coderd/proxyhealth" "github.com/coder/coder/v2/enterprise/coderd/schedule" @@ -113,7 +114,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { api.AGPL.SiteHandler.RegionsFetcher = func(ctx context.Context) (any, error) { // If the user can read the workspace proxy resource, return that. // If not, always default to the regions. - actor, ok := dbauthz.ActorFromContext(ctx) + actor, ok := agpldbauthz.ActorFromContext(ctx) if ok && api.Authorizer.Authorize(ctx, actor, rbac.ActionRead, rbac.ResourceWorkspaceProxy) == nil { return api.fetchWorkspaceProxies(ctx) } @@ -482,6 +483,7 @@ func (api *API) updateEntitlements(ctx context.Context) error { codersdk.FeatureTemplateAutostopRequirement: api.AGPL.Experiments.Enabled(codersdk.ExperimentTemplateAutostopRequirement) && api.DefaultQuietHoursSchedule != "", codersdk.FeatureWorkspaceProxy: true, codersdk.FeatureUserRoleManagement: true, + codersdk.FeatureAccessControl: true, }) if err != nil { return err @@ -654,6 +656,14 @@ func (api *API) updateEntitlements(ctx context.Context) error { } } + if initial, changed, enabled := featureChanged(codersdk.FeatureAccessControl); shouldUpdate(initial, changed, enabled) { + var acs agpldbauthz.AccessControlStore = agpldbauthz.AGPLTemplateAccessControlStore{} + if enabled { + acs = dbauthz.EnterpriseTemplateAccessControlStore{} + } + api.AGPL.AccessControlStore.Store(&acs) + } + // External token encryption is soft-enforced featureExternalTokenEncryption := entitlements.Features[codersdk.FeatureExternalTokenEncryption] featureExternalTokenEncryption.Enabled = len(api.ExternalTokenEncryption) > 0 diff --git a/enterprise/coderd/dbauthz/accesscontrol.go b/enterprise/coderd/dbauthz/accesscontrol.go new file mode 100644 index 0000000000000..454f416ab8736 --- /dev/null +++ b/enterprise/coderd/dbauthz/accesscontrol.go @@ -0,0 +1,30 @@ +package dbauthz + +import ( + "context" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/database" + agpldbz "github.com/coder/coder/v2/coderd/database/dbauthz" +) + +type EnterpriseTemplateAccessControlStore struct{} + +func (EnterpriseTemplateAccessControlStore) GetTemplateAccessControl(t database.Template) agpldbz.TemplateAccessControl { + return agpldbz.TemplateAccessControl{ + RequireActiveVersion: t.RequireActiveVersion, + } +} + +func (EnterpriseTemplateAccessControlStore) SetTemplateAccessControl(ctx context.Context, store database.Store, id uuid.UUID, opts agpldbz.TemplateAccessControl) error { + err := store.UpdateTemplateAccessControlByID(ctx, database.UpdateTemplateAccessControlByIDParams{ + ID: id, + RequireActiveVersion: opts.RequireActiveVersion, + }) + if err != nil { + return xerrors.Errorf("update template access control: %w", err) + } + return nil +} diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 50bd6ecf0798b..6b51e706e2d81 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/v2/cryptorand" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/enterprise/coderd/schedule" "github.com/coder/coder/v2/provisioner/echo" "github.com/coder/coder/v2/testutil" ) @@ -571,6 +572,51 @@ func TestTemplates(t *testing.T) { require.Equal(t, updatedDormantWS.DormantAt, dormantWorkspace.DormantAt) require.True(t, updatedDormantWS.LastUsedAt.After(dormantWorkspace.LastUsedAt)) }) + + t.Run("RequireActiveVersion", func(t *testing.T) { + t.Parallel() + client, user := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + TemplateScheduleStore: schedule.NewEnterpriseTemplateScheduleStore(agplUserQuietHoursScheduleStore()), + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAccessControl: 1, + }, + }, + }) + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.RequireActiveVersion = true + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + require.True(t, template.RequireActiveVersion) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Update the field and assert it persists. + updatedTemplate, err := client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + RequireActiveVersion: false, + }) + require.NoError(t, err) + require.False(t, updatedTemplate.RequireActiveVersion) + + // Flip it back to ensure we aren't hardcoding to a default value. + updatedTemplate, err = client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + RequireActiveVersion: true, + }) + require.NoError(t, err) + require.True(t, updatedTemplate.RequireActiveVersion) + + // Assert that fetching a template is no different from the response + // when updating. + template, err = client.Template(ctx, template.ID) + require.NoError(t, err) + require.Equal(t, updatedTemplate, template) + }) } func TestTemplateACL(t *testing.T) { diff --git a/enterprise/coderd/workspacebuilds_test.go b/enterprise/coderd/workspacebuilds_test.go new file mode 100644 index 0000000000000..615991a65dec9 --- /dev/null +++ b/enterprise/coderd/workspacebuilds_test.go @@ -0,0 +1,139 @@ +package coderd_test + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/testutil" +) + +func TestWorkspaceBuild(t *testing.T) { + t.Parallel() + t.Run("TemplateRequiresActiveVersion", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAccessControl: 1, + codersdk.FeatureTemplateRBAC: 1, + codersdk.FeatureAdvancedTemplateScheduling: 1, + }, + }, + }) + + // Create an initial version. + oldVersion := coderdtest.CreateTemplateVersion(t, ownerClient, owner.OrganizationID, nil) + // Create a template that mandates the promoted version. + // This should be enforced for everyone except template admins. + template := coderdtest.CreateTemplate(t, ownerClient, owner.OrganizationID, oldVersion.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, ownerClient, oldVersion.ID) + require.Equal(t, oldVersion.ID, template.ActiveVersionID) + template, err := ownerClient.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + RequireActiveVersion: true, + }) + require.NoError(t, err) + require.True(t, template.RequireActiveVersion) + + // Create a new version that we will promote. + activeVersion := coderdtest.CreateTemplateVersion(t, ownerClient, owner.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.TemplateID = template.ID + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, ownerClient, activeVersion.ID) + err = ownerClient.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{ + ID: activeVersion.ID, + }) + require.NoError(t, err) + + templateAdminClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + templateACLAdminClient, templateACLAdmin := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) + templateGroupACLAdminClient, templateGroupACLAdmin := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) + memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) + + // Create a group so we can also test group template admin ownership. + group, err := ownerClient.CreateGroup(ctx, owner.OrganizationID, codersdk.CreateGroupRequest{ + Name: "test", + }) + require.NoError(t, err) + + // Add the user who gains template admin via group membership. + group, err = ownerClient.PatchGroup(ctx, group.ID, codersdk.PatchGroupRequest{ + AddUsers: []string{templateGroupACLAdmin.ID.String()}, + }) + require.NoError(t, err) + + // Update the template for both users and groups. + err = ownerClient.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{ + UserPerms: map[string]codersdk.TemplateRole{ + templateACLAdmin.ID.String(): codersdk.TemplateRoleAdmin, + }, + GroupPerms: map[string]codersdk.TemplateRole{ + group.ID.String(): codersdk.TemplateRoleAdmin, + }, + }) + require.NoError(t, err) + + type testcase struct { + Name string + Client *codersdk.Client + ExpectedStatusCode int + } + + cases := []testcase{ + { + Name: "OwnerOK", + Client: ownerClient, + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "TemplateAdminOK", + Client: templateAdminClient, + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "TemplateACLAdminOK", + Client: templateACLAdminClient, + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "TemplateGroupACLAdminOK", + Client: templateGroupACLAdminClient, + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "MemberFails", + Client: memberClient, + ExpectedStatusCode: http.StatusUnauthorized, + }, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + _, err = c.Client.CreateWorkspace(ctx, owner.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateVersionID: oldVersion.ID, + Name: "abc123", + AutomaticUpdates: codersdk.AutomaticUpdatesNever, + }) + if c.ExpectedStatusCode == http.StatusOK { + require.NoError(t, err) + } else { + require.Error(t, err) + cerr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, c.ExpectedStatusCode, cerr.StatusCode()) + } + }) + } + }) +} diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index e38d1874e764d..a80107d379e74 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -734,6 +734,96 @@ func TestWorkspaceAutobuild(t *testing.T) { require.Len(t, stats.Transitions, 1) require.Equal(t, database.WorkspaceTransitionDelete, stats.Transitions[ws.ID]) }) + + t.Run("RequireActiveVersion", func(t *testing.T) { + t.Parallel() + + var ( + tickCh = make(chan time.Time) + statsCh = make(chan autobuild.Stats) + ctx = testutil.Context(t, testutil.WaitMedium) + ) + + client, user := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + AutobuildTicker: tickCh, + IncludeProvisionerDaemon: true, + AutobuildStats: statsCh, + TemplateScheduleStore: schedule.NewEnterpriseTemplateScheduleStore(agplUserQuietHoursScheduleStore()), + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{codersdk.FeatureAccessControl: 1}, + }, + }) + + sched, err := cron.Weekly("CRON_TZ=UTC 0 * * * *") + require.NoError(t, err) + + // Create a template version1 that passes to get a functioning workspace. + version1 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID) + + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version1.ID) + require.Equal(t, version1.ID, template.ActiveVersionID) + + ws := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.AutostartSchedule = ptr.Ref(sched.String()) + }) + + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + + // Create a new version so that we can assert we don't update + // to the latest by default. + version2 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.TemplateID = template.ID + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version2.ID) + + // Make sure to promote it. + err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{ + ID: version2.ID, + }) + require.NoError(t, err) + + // Kick of an autostart build. + tickCh <- sched.Next(ws.LatestBuild.CreatedAt) + stats := <-statsCh + require.NoError(t, stats.Error) + require.Len(t, stats.Transitions, 1) + require.Contains(t, stats.Transitions, ws.ID) + require.Equal(t, database.WorkspaceTransitionStart, stats.Transitions[ws.ID]) + + // Validate that we didn't update to the promoted version. + started := coderdtest.MustWorkspace(t, client, ws.ID) + firstBuild := coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, started.LatestBuild.ID) + require.Equal(t, version1.ID, firstBuild.TemplateVersionID) + + // Update the template to require the promoted version. + _, err = client.UpdateTemplateMeta(ctx, template.ID, codersdk.UpdateTemplateMeta{ + RequireActiveVersion: true, + AllowUserAutostart: true, + }) + require.NoError(t, err) + + // Reset the workspace to the stopped state so we can try + // to autostart again. + coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop, func(req *codersdk.CreateWorkspaceBuildRequest) { + req.TemplateVersionID = ws.LatestBuild.TemplateVersionID + }) + + // Force an autostart transition again. + tickCh <- sched.Next(firstBuild.CreatedAt) + stats = <-statsCh + require.NoError(t, stats.Error) + require.Len(t, stats.Transitions, 1) + require.Contains(t, stats.Transitions, ws.ID) + require.Equal(t, database.WorkspaceTransitionStart, stats.Transitions[ws.ID]) + + // Validate that we are using the promoted version. + ws = coderdtest.MustWorkspace(t, client, ws.ID) + require.Equal(t, version2.ID, ws.LatestBuild.TemplateVersionID) + }) } // Blocked by autostart requirements diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 29fbd56f5e316..805ab0d2bacf9 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -221,6 +221,7 @@ export interface CreateTemplateRequest { readonly dormant_ttl_ms?: number; readonly delete_ttl_ms?: number; readonly disable_everyone_group_access: boolean; + readonly require_active_version: boolean; } // From codersdk/templateversions.go @@ -916,6 +917,7 @@ export interface Template { readonly failure_ttl_ms: number; readonly time_til_dormant_ms: number; readonly time_til_dormant_autodelete_ms: number; + readonly require_active_version: boolean; } // From codersdk/templates.go @@ -1166,6 +1168,7 @@ export interface UpdateTemplateMeta { readonly time_til_dormant_autodelete_ms?: number; readonly update_workspace_last_used_at: boolean; readonly update_workspace_dormant_at: boolean; + readonly require_active_version: boolean; } // From codersdk/users.go @@ -1715,6 +1718,7 @@ export const Experiments: Experiment[] = [ // From codersdk/deployment.go export type FeatureName = + | "access_control" | "advanced_template_scheduling" | "appearance" | "audit_log" @@ -1731,6 +1735,7 @@ export type FeatureName = | "workspace_batch_actions" | "workspace_proxy"; export const FeatureNames: FeatureName[] = [ + "access_control", "advanced_template_scheduling", "appearance", "audit_log", diff --git a/site/src/pages/CreateTemplatePage/utils.ts b/site/src/pages/CreateTemplatePage/utils.ts index 2e525eae7a29e..fdeb1046d3869 100644 --- a/site/src/pages/CreateTemplatePage/utils.ts +++ b/site/src/pages/CreateTemplatePage/utils.ts @@ -37,6 +37,7 @@ export const newTemplate = (formData: CreateTemplateData) => { autostart_requirement: { days_of_week: autostart_requirement_days_of_week, }, + require_active_version: false, }; }; diff --git a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx index 09f376a83420e..d6afac0fbd504 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx @@ -69,6 +69,7 @@ export const TemplateSettingsForm: FC = ({ template.allow_user_cancel_workspace_jobs, update_workspace_last_used_at: false, update_workspace_dormant_at: false, + require_active_version: false, }, validationSchema, onSubmit, diff --git a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.test.tsx index 8df20b8c3d399..c9f2059672fce 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.test.tsx @@ -43,6 +43,7 @@ const validFormValues: FormValues = { time_til_dormant_autodelete_ms: 0, update_workspace_last_used_at: false, update_workspace_dormant_at: false, + require_active_version: false, }; const renderTemplateSettingsPage = async () => { diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateScheduleForm.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateScheduleForm.tsx index 54d558467753d..88cfcc394bdf7 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateScheduleForm.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateScheduleForm.tsx @@ -113,6 +113,7 @@ export const TemplateScheduleForm: FC = ({ Boolean(template.time_til_dormant_autodelete_ms), update_workspace_last_used_at: false, update_workspace_dormant_at: false, + require_active_version: false, }, validationSchema, onSubmit: () => { @@ -229,6 +230,7 @@ export const TemplateScheduleForm: FC = ({ allow_user_autostop: form.values.allow_user_autostop, update_workspace_last_used_at: form.values.update_workspace_last_used_at, update_workspace_dormant_at: form.values.update_workspace_dormant_at, + require_active_version: false, }); }; diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index c2a520562a53b..f6096ab4e5718 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -26,6 +26,7 @@ const validFormValues: TemplateScheduleFormValues = { failure_cleanup_enabled: false, inactivity_cleanup_enabled: false, dormant_autodeletion_cleanup_enabled: false, + require_active_version: false, autostart_requirement_days_of_week: [ "monday", "tuesday", diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 35882aa8dcba4..7cf0e8c8a6c28 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -467,6 +467,7 @@ export const MockTemplate: TypesGen.Template = { time_til_dormant_autodelete_ms: 0, allow_user_autostart: true, allow_user_autostop: true, + require_active_version: false, }; export const MockTemplateVersionFiles: TemplateVersionFiles = { From c4f590581e53eb5c338d809602d53a2fa24aa79d Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Thu, 19 Oct 2023 10:45:12 +0200 Subject: [PATCH 08/26] feat: expose template insights as Prometheus metrics (#10325) --- cli/server.go | 16 ++ coderd/database/dbauthz/dbauthz.go | 7 + coderd/database/dbfake/dbfake.go | 99 ++++++++++ coderd/database/dbmetrics/dbmetrics.go | 7 + coderd/database/dbmock/dbmock.go | 15 ++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 73 ++++++++ coderd/database/queries/insights.sql | 28 +++ .../insights/metricscollector.go | 174 ++++++++++++++++++ .../insights/metricscollector_test.go | 132 +++++++++++++ .../insights/testdata/insights-metrics.json | 3 + 11 files changed, 555 insertions(+) create mode 100644 coderd/prometheusmetrics/insights/metricscollector.go create mode 100644 coderd/prometheusmetrics/insights/metricscollector_test.go create mode 100644 coderd/prometheusmetrics/insights/testdata/insights-metrics.json diff --git a/cli/server.go b/cli/server.go index a6a08a0b947fd..75caabde07dc1 100644 --- a/cli/server.go +++ b/cli/server.go @@ -80,6 +80,7 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/oauthpki" "github.com/coder/coder/v2/coderd/prometheusmetrics" + "github.com/coder/coder/v2/coderd/prometheusmetrics/insights" "github.com/coder/coder/v2/coderd/schedule" "github.com/coder/coder/v2/coderd/telemetry" "github.com/coder/coder/v2/coderd/tracing" @@ -200,6 +201,21 @@ func enablePrometheus( } afterCtx(ctx, closeWorkspacesFunc) + insightsMetricsCollector, err := insights.NewMetricsCollector(options.Database, options.Logger, 0, 0) + if err != nil { + return nil, xerrors.Errorf("unable to initialize insights metrics collector: %w", err) + } + err = options.PrometheusRegistry.Register(insightsMetricsCollector) + if err != nil { + return nil, xerrors.Errorf("unable to register insights metrics collector: %w", err) + } + + closeInsightsMetricsCollector, err := insightsMetricsCollector.Run(ctx) + if err != nil { + return nil, xerrors.Errorf("unable to run insights metrics collector: %w", err) + } + afterCtx(ctx, closeInsightsMetricsCollector) + if vals.Prometheus.CollectAgentStats { closeAgentStatsFunc, err := prometheusmetrics.AgentStats(ctx, logger, options.PrometheusRegistry, options.Database, time.Now(), 0) if err != nil { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 796c68a5031f8..e9f4acc0a763a 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1327,6 +1327,13 @@ func (q *querier) GetTemplateInsightsByInterval(ctx context.Context, arg databas return q.db.GetTemplateInsightsByInterval(ctx, arg) } +func (q *querier) GetTemplateInsightsByTemplate(ctx context.Context, arg database.GetTemplateInsightsByTemplateParams) ([]database.GetTemplateInsightsByTemplateRow, error) { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { + return nil, err + } + return q.db.GetTemplateInsightsByTemplate(ctx, arg) +} + func (q *querier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { for _, templateID := range arg.TemplateIDs { template, err := q.db.GetTemplateByID(ctx, templateID) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 2d46bbe76ec9a..ea1cd167bf7ca 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2500,6 +2500,10 @@ func (q *FakeQuerier) GetTemplateInsights(_ context.Context, arg database.GetTem templateIDSet := make(map[uuid.UUID]struct{}) appUsageIntervalsByUser := make(map[uuid.UUID]map[time.Time]*database.GetTemplateInsightsRow) + + q.mutex.RLock() + defer q.mutex.RUnlock() + for _, s := range q.workspaceAgentStats { if s.CreatedAt.Before(arg.StartTime) || s.CreatedAt.Equal(arg.EndTime) || s.CreatedAt.After(arg.EndTime) { continue @@ -2648,6 +2652,101 @@ func (q *FakeQuerier) GetTemplateInsightsByInterval(ctx context.Context, arg dat return result, nil } +func (q *FakeQuerier) GetTemplateInsightsByTemplate(_ context.Context, arg database.GetTemplateInsightsByTemplateParams) ([]database.GetTemplateInsightsByTemplateRow, error) { + err := validateDatabaseType(arg) + if err != nil { + return nil, err + } + + q.mutex.RLock() + defer q.mutex.RUnlock() + + // map time.Time x TemplateID x UserID x + appUsageByTemplateAndUser := map[time.Time]map[uuid.UUID]map[uuid.UUID]database.GetTemplateInsightsByTemplateRow{} + + // Review agent stats in terms of usage + templateIDSet := make(map[uuid.UUID]struct{}) + + for _, s := range q.workspaceAgentStats { + if s.CreatedAt.Before(arg.StartTime) || s.CreatedAt.Equal(arg.EndTime) || s.CreatedAt.After(arg.EndTime) { + continue + } + if s.ConnectionCount == 0 { + continue + } + + t := s.CreatedAt.Truncate(time.Minute) + templateIDSet[s.TemplateID] = struct{}{} + + if _, ok := appUsageByTemplateAndUser[t]; !ok { + appUsageByTemplateAndUser[t] = make(map[uuid.UUID]map[uuid.UUID]database.GetTemplateInsightsByTemplateRow) + } + + if _, ok := appUsageByTemplateAndUser[t][s.TemplateID]; !ok { + appUsageByTemplateAndUser[t][s.TemplateID] = make(map[uuid.UUID]database.GetTemplateInsightsByTemplateRow) + } + + if _, ok := appUsageByTemplateAndUser[t][s.TemplateID][s.UserID]; !ok { + appUsageByTemplateAndUser[t][s.TemplateID][s.UserID] = database.GetTemplateInsightsByTemplateRow{} + } + + u := appUsageByTemplateAndUser[t][s.TemplateID][s.UserID] + if s.SessionCountJetBrains > 0 { + u.UsageJetbrainsSeconds = 60 + } + if s.SessionCountVSCode > 0 { + u.UsageVscodeSeconds = 60 + } + if s.SessionCountReconnectingPTY > 0 { + u.UsageReconnectingPtySeconds = 60 + } + if s.SessionCountSSH > 0 { + u.UsageSshSeconds = 60 + } + appUsageByTemplateAndUser[t][s.TemplateID][s.UserID] = u + } + + // Sort used templates + templateIDs := make([]uuid.UUID, 0, len(templateIDSet)) + for templateID := range templateIDSet { + templateIDs = append(templateIDs, templateID) + } + slices.SortFunc(templateIDs, func(a, b uuid.UUID) int { + return slice.Ascending(a.String(), b.String()) + }) + + // Build result + var result []database.GetTemplateInsightsByTemplateRow + for _, templateID := range templateIDs { + r := database.GetTemplateInsightsByTemplateRow{ + TemplateID: templateID, + } + + uniqueUsers := map[uuid.UUID]struct{}{} + + for _, mTemplateUserUsage := range appUsageByTemplateAndUser { + mUserUsage, ok := mTemplateUserUsage[templateID] + if !ok { + continue // template was not used in this time window + } + + for userID, usage := range mUserUsage { + uniqueUsers[userID] = struct{}{} + + r.UsageJetbrainsSeconds += usage.UsageJetbrainsSeconds + r.UsageVscodeSeconds += usage.UsageVscodeSeconds + r.UsageReconnectingPtySeconds += usage.UsageReconnectingPtySeconds + r.UsageSshSeconds += usage.UsageSshSeconds + } + } + + r.ActiveUsers = int64(len(uniqueUsers)) + + result = append(result, r) + } + return result, nil +} + func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { err := validateDatabaseType(arg) if err != nil { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index ed3ff65b18378..3a89ddd379790 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -704,6 +704,13 @@ func (m metricsStore) GetTemplateInsightsByInterval(ctx context.Context, arg dat return r0, r1 } +func (m metricsStore) GetTemplateInsightsByTemplate(ctx context.Context, arg database.GetTemplateInsightsByTemplateParams) ([]database.GetTemplateInsightsByTemplateRow, error) { + start := time.Now() + r0, r1 := m.s.GetTemplateInsightsByTemplate(ctx, arg) + m.queryLatencies.WithLabelValues("GetTemplateInsightsByTemplate").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { start := time.Now() r0, r1 := m.s.GetTemplateParameterInsights(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 491937c6aeb6d..080c9630e7bc4 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1433,6 +1433,21 @@ func (mr *MockStoreMockRecorder) GetTemplateInsightsByInterval(arg0, arg1 interf return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTemplateInsightsByInterval", reflect.TypeOf((*MockStore)(nil).GetTemplateInsightsByInterval), arg0, arg1) } +// GetTemplateInsightsByTemplate mocks base method. +func (m *MockStore) GetTemplateInsightsByTemplate(arg0 context.Context, arg1 database.GetTemplateInsightsByTemplateParams) ([]database.GetTemplateInsightsByTemplateRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetTemplateInsightsByTemplate", arg0, arg1) + ret0, _ := ret[0].([]database.GetTemplateInsightsByTemplateRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetTemplateInsightsByTemplate indicates an expected call of GetTemplateInsightsByTemplate. +func (mr *MockStoreMockRecorder) GetTemplateInsightsByTemplate(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTemplateInsightsByTemplate", reflect.TypeOf((*MockStore)(nil).GetTemplateInsightsByTemplate), arg0, arg1) +} + // GetTemplateParameterInsights mocks base method. func (m *MockStore) GetTemplateParameterInsights(arg0 context.Context, arg1 database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index bb2e95d17c309..2d278ba933e67 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -145,6 +145,7 @@ type sqlcQuerier interface { // that interval will be shorter than a full one. If there is no data for a selected // interval/template, it will be included in the results with 0 active users. GetTemplateInsightsByInterval(ctx context.Context, arg GetTemplateInsightsByIntervalParams) ([]GetTemplateInsightsByIntervalRow, error) + GetTemplateInsightsByTemplate(ctx context.Context, arg GetTemplateInsightsByTemplateParams) ([]GetTemplateInsightsByTemplateRow, error) // GetTemplateParameterInsights does for each template in a given timeframe, // look for the latest workspace build (for every workspace) that has been // created in the timeframe and return the aggregate usage counts of parameter diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d4978e5d141e8..873ec9fdefed3 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1937,6 +1937,79 @@ func (q *sqlQuerier) GetTemplateInsightsByInterval(ctx context.Context, arg GetT return items, nil } +const getTemplateInsightsByTemplate = `-- name: GetTemplateInsightsByTemplate :many +WITH agent_stats_by_interval_and_user AS ( + SELECT + date_trunc('minute', was.created_at) AS created_at_trunc, + was.template_id, + was.user_id, + CASE WHEN SUM(was.session_count_vscode) > 0 THEN 60 ELSE 0 END AS usage_vscode_seconds, + CASE WHEN SUM(was.session_count_jetbrains) > 0 THEN 60 ELSE 0 END AS usage_jetbrains_seconds, + CASE WHEN SUM(was.session_count_reconnecting_pty) > 0 THEN 60 ELSE 0 END AS usage_reconnecting_pty_seconds, + CASE WHEN SUM(was.session_count_ssh) > 0 THEN 60 ELSE 0 END AS usage_ssh_seconds + FROM workspace_agent_stats was + WHERE + was.created_at >= $1::timestamptz + AND was.created_at < $2::timestamptz + AND was.connection_count > 0 + GROUP BY created_at_trunc, was.template_id, was.user_id +) + +SELECT + template_id, + COALESCE(COUNT(DISTINCT user_id))::bigint AS active_users, + COALESCE(SUM(usage_vscode_seconds), 0)::bigint AS usage_vscode_seconds, + COALESCE(SUM(usage_jetbrains_seconds), 0)::bigint AS usage_jetbrains_seconds, + COALESCE(SUM(usage_reconnecting_pty_seconds), 0)::bigint AS usage_reconnecting_pty_seconds, + COALESCE(SUM(usage_ssh_seconds), 0)::bigint AS usage_ssh_seconds +FROM agent_stats_by_interval_and_user +GROUP BY template_id +` + +type GetTemplateInsightsByTemplateParams struct { + StartTime time.Time `db:"start_time" json:"start_time"` + EndTime time.Time `db:"end_time" json:"end_time"` +} + +type GetTemplateInsightsByTemplateRow struct { + TemplateID uuid.UUID `db:"template_id" json:"template_id"` + ActiveUsers int64 `db:"active_users" json:"active_users"` + UsageVscodeSeconds int64 `db:"usage_vscode_seconds" json:"usage_vscode_seconds"` + UsageJetbrainsSeconds int64 `db:"usage_jetbrains_seconds" json:"usage_jetbrains_seconds"` + UsageReconnectingPtySeconds int64 `db:"usage_reconnecting_pty_seconds" json:"usage_reconnecting_pty_seconds"` + UsageSshSeconds int64 `db:"usage_ssh_seconds" json:"usage_ssh_seconds"` +} + +func (q *sqlQuerier) GetTemplateInsightsByTemplate(ctx context.Context, arg GetTemplateInsightsByTemplateParams) ([]GetTemplateInsightsByTemplateRow, error) { + rows, err := q.db.QueryContext(ctx, getTemplateInsightsByTemplate, arg.StartTime, arg.EndTime) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetTemplateInsightsByTemplateRow + for rows.Next() { + var i GetTemplateInsightsByTemplateRow + if err := rows.Scan( + &i.TemplateID, + &i.ActiveUsers, + &i.UsageVscodeSeconds, + &i.UsageJetbrainsSeconds, + &i.UsageReconnectingPtySeconds, + &i.UsageSshSeconds, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getTemplateParameterInsights = `-- name: GetTemplateParameterInsights :many WITH latest_workspace_builds AS ( SELECT diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index 7fb48100d5d8a..01863fede1aed 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -134,6 +134,34 @@ SELECT COALESCE(SUM(usage_ssh_seconds), 0)::bigint AS usage_ssh_seconds FROM agent_stats_by_interval_and_user; +-- name: GetTemplateInsightsByTemplate :many +WITH agent_stats_by_interval_and_user AS ( + SELECT + date_trunc('minute', was.created_at) AS created_at_trunc, + was.template_id, + was.user_id, + CASE WHEN SUM(was.session_count_vscode) > 0 THEN 60 ELSE 0 END AS usage_vscode_seconds, + CASE WHEN SUM(was.session_count_jetbrains) > 0 THEN 60 ELSE 0 END AS usage_jetbrains_seconds, + CASE WHEN SUM(was.session_count_reconnecting_pty) > 0 THEN 60 ELSE 0 END AS usage_reconnecting_pty_seconds, + CASE WHEN SUM(was.session_count_ssh) > 0 THEN 60 ELSE 0 END AS usage_ssh_seconds + FROM workspace_agent_stats was + WHERE + was.created_at >= @start_time::timestamptz + AND was.created_at < @end_time::timestamptz + AND was.connection_count > 0 + GROUP BY created_at_trunc, was.template_id, was.user_id +) + +SELECT + template_id, + COALESCE(COUNT(DISTINCT user_id))::bigint AS active_users, + COALESCE(SUM(usage_vscode_seconds), 0)::bigint AS usage_vscode_seconds, + COALESCE(SUM(usage_jetbrains_seconds), 0)::bigint AS usage_jetbrains_seconds, + COALESCE(SUM(usage_reconnecting_pty_seconds), 0)::bigint AS usage_reconnecting_pty_seconds, + COALESCE(SUM(usage_ssh_seconds), 0)::bigint AS usage_ssh_seconds +FROM agent_stats_by_interval_and_user +GROUP BY template_id; + -- name: GetTemplateAppInsights :many -- GetTemplateAppInsights returns the aggregate usage of each app in a given -- timeframe. The result can be filtered on template_ids, meaning only user data diff --git a/coderd/prometheusmetrics/insights/metricscollector.go b/coderd/prometheusmetrics/insights/metricscollector.go new file mode 100644 index 0000000000000..d19785e8e6131 --- /dev/null +++ b/coderd/prometheusmetrics/insights/metricscollector.go @@ -0,0 +1,174 @@ +package insights + +import ( + "context" + "sync/atomic" + "time" + + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + "golang.org/x/sync/errgroup" + "golang.org/x/xerrors" + + "cdr.dev/slog" + + "github.com/coder/coder/v2/coderd/database" +) + +var templatesActiveUsersDesc = prometheus.NewDesc("coderd_insights_templates_active_users", "The number of active users of the template.", []string{"template_name"}, nil) + +type MetricsCollector struct { + database database.Store + logger slog.Logger + timeWindow time.Duration + tickInterval time.Duration + + data atomic.Pointer[insightsData] +} + +type insightsData struct { + templates []database.GetTemplateInsightsByTemplateRow + + templateNames map[uuid.UUID]string +} + +var _ prometheus.Collector = new(MetricsCollector) + +func NewMetricsCollector(db database.Store, logger slog.Logger, timeWindow time.Duration, tickInterval time.Duration) (*MetricsCollector, error) { + if timeWindow == 0 { + timeWindow = 5 * time.Minute + } + if timeWindow < 5*time.Minute { + return nil, xerrors.Errorf("time window must be at least 5 mins") + } + if tickInterval == 0 { + tickInterval = timeWindow + } + + return &MetricsCollector{ + database: db, + logger: logger.Named("insights_metrics_collector"), + timeWindow: timeWindow, + tickInterval: tickInterval, + }, nil +} + +func (mc *MetricsCollector) Run(ctx context.Context) (func(), error) { + ctx, closeFunc := context.WithCancel(ctx) + done := make(chan struct{}) + + // Use time.Nanosecond to force an initial tick. It will be reset to the + // correct duration after executing once. + ticker := time.NewTicker(time.Nanosecond) + doTick := func() { + defer ticker.Reset(mc.tickInterval) + + now := time.Now() + startTime := now.Add(-mc.timeWindow) + endTime := now + + // Phase 1: Fetch insights from database + // FIXME errorGroup will be used to fetch insights for apps and parameters + eg, egCtx := errgroup.WithContext(ctx) + eg.SetLimit(1) + + var templateInsights []database.GetTemplateInsightsByTemplateRow + + eg.Go(func() error { + var err error + templateInsights, err = mc.database.GetTemplateInsightsByTemplate(egCtx, database.GetTemplateInsightsByTemplateParams{ + StartTime: startTime, + EndTime: endTime, + }) + if err != nil { + mc.logger.Error(ctx, "unable to fetch template insights from database", slog.Error(err)) + } + return err + }) + err := eg.Wait() + if err != nil { + return + } + + // Phase 2: Collect template IDs, and fetch relevant details + templateIDs := uniqueTemplateIDs(templateInsights) + + templateNames := make(map[uuid.UUID]string, len(templateIDs)) + if len(templateIDs) > 0 { + templates, err := mc.database.GetTemplatesWithFilter(ctx, database.GetTemplatesWithFilterParams{ + IDs: templateIDs, + }) + if err != nil { + mc.logger.Error(ctx, "unable to fetch template details from database", slog.Error(err)) + return + } + templateNames = onlyTemplateNames(templates) + } + + // Refresh the collector state + mc.data.Store(&insightsData{ + templates: templateInsights, + templateNames: templateNames, + }) + } + + go func() { + defer close(done) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + ticker.Stop() + doTick() + } + } + }() + return func() { + closeFunc() + <-done + }, nil +} + +func (*MetricsCollector) Describe(descCh chan<- *prometheus.Desc) { + descCh <- templatesActiveUsersDesc +} + +func (mc *MetricsCollector) Collect(metricsCh chan<- prometheus.Metric) { + // Phase 3: Collect metrics + + data := mc.data.Load() + if data == nil { + return // insights data not loaded yet + } + + for _, templateRow := range data.templates { + metricsCh <- prometheus.MustNewConstMetric(templatesActiveUsersDesc, prometheus.GaugeValue, float64(templateRow.ActiveUsers), data.templateNames[templateRow.TemplateID]) + } +} + +// Helper functions below. + +func uniqueTemplateIDs(templateInsights []database.GetTemplateInsightsByTemplateRow) []uuid.UUID { + tids := map[uuid.UUID]bool{} + for _, t := range templateInsights { + tids[t.TemplateID] = true + } + + uniqueUUIDs := make([]uuid.UUID, len(tids)) + var i int + for t := range tids { + uniqueUUIDs[i] = t + i++ + } + return uniqueUUIDs +} + +func onlyTemplateNames(templates []database.Template) map[uuid.UUID]string { + m := map[uuid.UUID]string{} + for _, t := range templates { + m[t.ID] = t.Name + } + return m +} diff --git a/coderd/prometheusmetrics/insights/metricscollector_test.go b/coderd/prometheusmetrics/insights/metricscollector_test.go new file mode 100644 index 0000000000000..0c1726a910e96 --- /dev/null +++ b/coderd/prometheusmetrics/insights/metricscollector_test.go @@ -0,0 +1,132 @@ +package insights_test + +import ( + "context" + "encoding/json" + "io" + "os" + "testing" + "time" + + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/agent/agenttest" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/prometheusmetrics/insights" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/provisioner/echo" + "github.com/coder/coder/v2/testutil" +) + +func TestCollect_TemplateInsights(t *testing.T) { + t.Parallel() + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + db, ps := dbtestutil.NewDB(t) + + options := &coderdtest.Options{ + IncludeProvisionerDaemon: true, + AgentStatsRefreshInterval: time.Millisecond * 100, + Database: db, + Pubsub: ps, + } + client := coderdtest.New(t, options) + + // Given + // Initialize metrics collector + mc, err := insights.NewMetricsCollector(db, logger, 0, time.Second) + require.NoError(t, err) + + registry := prometheus.NewRegistry() + registry.Register(mc) + + // Create two users, one that will appear in the report and another that + // won't (due to not having/using a workspace). + user := coderdtest.CreateFirstUser(t, client) + _, _ = coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + authToken := uuid.NewString() + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.PlanComplete, + ProvisionApply: echo.ProvisionApplyWithAgent(authToken), + }) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + require.Empty(t, template.BuildTimeStats[codersdk.WorkspaceTransitionStart]) + + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + // Start an agent so that we can generate stats. + _ = agenttest.New(t, client.URL, authToken) + resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Run metrics collector + closeFunc, err := mc.Run(ctx) + require.NoError(t, err) + defer closeFunc() + + // Connect to the agent to generate usage/latency stats. + conn, err := client.DialWorkspaceAgent(ctx, resources[0].Agents[0].ID, &codersdk.DialWorkspaceAgentOptions{ + Logger: logger.Named("client"), + }) + require.NoError(t, err) + defer conn.Close() + + sshConn, err := conn.SSHClient(ctx) + require.NoError(t, err) + defer sshConn.Close() + + sess, err := sshConn.NewSession() + require.NoError(t, err) + defer sess.Close() + + r, w := io.Pipe() + defer r.Close() + defer w.Close() + sess.Stdin = r + sess.Stdout = io.Discard + err = sess.Start("cat") + require.NoError(t, err) + + goldenFile, err := os.ReadFile("testdata/insights-metrics.json") + require.NoError(t, err) + golden := map[string]int{} + err = json.Unmarshal(goldenFile, &golden) + require.NoError(t, err) + + collected := map[string]int{} + assert.Eventuallyf(t, func() bool { + // When + metrics, err := registry.Gather() + require.NoError(t, err) + + // Then + for _, metric := range metrics { + switch metric.GetName() { + case "coderd_insights_templates_active_users": + for _, m := range metric.Metric { + collected[metric.GetName()] = int(m.Gauge.GetValue()) + } + default: + require.FailNowf(t, "unexpected metric collected", "metric: %s", metric.GetName()) + } + } + + return assert.ObjectsAreEqualValues(golden, collected) + }, testutil.WaitMedium, testutil.IntervalFast, "template insights are missing") + + // We got our latency metrics, close the connection. + _ = sess.Close() + _ = sshConn.Close() + + require.EqualValues(t, golden, collected) +} diff --git a/coderd/prometheusmetrics/insights/testdata/insights-metrics.json b/coderd/prometheusmetrics/insights/testdata/insights-metrics.json new file mode 100644 index 0000000000000..01c96a78b64a4 --- /dev/null +++ b/coderd/prometheusmetrics/insights/testdata/insights-metrics.json @@ -0,0 +1,3 @@ +{ + "coderd_insights_templates_active_users": 1 +} From b8c7b56fda05872057b36e7ca6bc4550c6eff379 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Thu, 19 Oct 2023 09:12:41 -0300 Subject: [PATCH 09/26] fix(site): fix tabs in the template layout (#10334) --- site/src/components/Tabs/Tabs.tsx | 71 +++++++++++ .../components/UsersLayout/UsersLayout.tsx | 81 +------------ .../src/pages/TemplatePage/TemplateLayout.tsx | 112 +++--------------- 3 files changed, 93 insertions(+), 171 deletions(-) create mode 100644 site/src/components/Tabs/Tabs.tsx diff --git a/site/src/components/Tabs/Tabs.tsx b/site/src/components/Tabs/Tabs.tsx new file mode 100644 index 0000000000000..b7e6c6de61eb7 --- /dev/null +++ b/site/src/components/Tabs/Tabs.tsx @@ -0,0 +1,71 @@ +import { ReactNode } from "react"; +import { NavLink, NavLinkProps } from "react-router-dom"; +import { combineClasses } from "utils/combineClasses"; +import { Margins } from "components/Margins/Margins"; +import { css } from "@emotion/css"; +import { useTheme } from "@mui/material/styles"; + +export const Tabs = ({ children }: { children: ReactNode }) => { + return ( +
({ + borderBottom: `1px solid ${theme.palette.divider}`, + marginBottom: theme.spacing(5), + })} + > + ({ + display: "flex", + alignItems: "center", + gap: theme.spacing(0.25), + })} + > + {children} + +
+ ); +}; + +export const TabLink = (props: NavLinkProps) => { + const theme = useTheme(); + + const baseTabLink = css` + text-decoration: none; + color: ${theme.palette.text.secondary}; + font-size: 14px; + display: block; + padding: ${theme.spacing(0, 2, 2)}; + + &:hover { + color: ${theme.palette.text.primary}; + } + `; + + const activeTabLink = css` + color: ${theme.palette.text.primary}; + position: relative; + + &:before { + content: ""; + left: 0; + bottom: 0; + height: 2px; + width: 100%; + background: ${theme.palette.secondary.dark}; + position: absolute; + } + `; + + return ( + + combineClasses([ + baseTabLink, + isActive ? activeTabLink : undefined, + props.className as string, + ]) + } + {...props} + /> + ); +}; diff --git a/site/src/components/UsersLayout/UsersLayout.tsx b/site/src/components/UsersLayout/UsersLayout.tsx index 7a4d380b3e8cd..397cadc25bb60 100644 --- a/site/src/components/UsersLayout/UsersLayout.tsx +++ b/site/src/components/UsersLayout/UsersLayout.tsx @@ -1,6 +1,5 @@ import Button from "@mui/material/Button"; import Link from "@mui/material/Link"; -import { makeStyles } from "@mui/styles"; import GroupAdd from "@mui/icons-material/GroupAddOutlined"; import PersonAdd from "@mui/icons-material/PersonAddOutlined"; import { USERS_LINK } from "components/Dashboard/Navbar/NavbarView"; @@ -8,18 +7,11 @@ import { PageHeader, PageHeaderTitle } from "components/PageHeader/PageHeader"; import { useFeatureVisibility } from "hooks/useFeatureVisibility"; import { usePermissions } from "hooks/usePermissions"; import { FC } from "react"; -import { - Link as RouterLink, - NavLink, - Outlet, - useNavigate, -} from "react-router-dom"; -import { combineClasses } from "utils/combineClasses"; +import { Link as RouterLink, Outlet, useNavigate } from "react-router-dom"; import { Margins } from "components/Margins/Margins"; -import { Stack } from "components/Stack/Stack"; +import { TabLink, Tabs } from "components/Tabs/Tabs"; export const UsersLayout: FC = () => { - const styles = useStyles(); const { createUser: canCreateUser, createGroup: canCreateGroup } = usePermissions(); const navigate = useNavigate(); @@ -53,35 +45,10 @@ export const UsersLayout: FC = () => { -
- - - - combineClasses([ - styles.tabItem, - isActive ? styles.tabItemActive : undefined, - ]) - } - > - Users - - - combineClasses([ - styles.tabItem, - isActive ? styles.tabItemActive : undefined, - ]) - } - > - Groups - - - -
+ + Users + Groups + @@ -89,39 +56,3 @@ export const UsersLayout: FC = () => { ); }; - -export const useStyles = makeStyles((theme) => { - return { - tabs: { - borderBottom: `1px solid ${theme.palette.divider}`, - marginBottom: theme.spacing(5), - }, - - tabItem: { - textDecoration: "none", - color: theme.palette.text.secondary, - fontSize: 14, - display: "block", - padding: theme.spacing(0, 2, 2), - - "&:hover": { - color: theme.palette.text.primary, - }, - }, - - tabItemActive: { - color: theme.palette.text.primary, - position: "relative", - - "&:before": { - content: `""`, - left: 0, - bottom: 0, - height: 2, - width: "100%", - background: theme.palette.secondary.dark, - position: "absolute", - }, - }, - }; -}); diff --git a/site/src/pages/TemplatePage/TemplateLayout.tsx b/site/src/pages/TemplatePage/TemplateLayout.tsx index 24f4487048320..41a15264c9a7b 100644 --- a/site/src/pages/TemplatePage/TemplateLayout.tsx +++ b/site/src/pages/TemplatePage/TemplateLayout.tsx @@ -1,8 +1,7 @@ -import { css } from "@emotion/css"; import { useTheme } from "@emotion/react"; import { createContext, type FC, Suspense, useContext } from "react"; import { useQuery } from "react-query"; -import { NavLink, Outlet, useNavigate, useParams } from "react-router-dom"; +import { Outlet, useNavigate, useParams } from "react-router-dom"; import type { AuthorizationRequest } from "api/typesGenerated"; import { checkAuthorization, @@ -11,10 +10,10 @@ import { } from "api/api"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Margins } from "components/Margins/Margins"; -import { Stack } from "components/Stack/Stack"; import { Loader } from "components/Loader/Loader"; import { useOrganizationId } from "hooks/useOrganizationId"; import { TemplatePageHeader } from "./TemplatePageHeader"; +import { TabLink, Tabs } from "components/Tabs/Tabs"; const templatePermissions = ( templateId: string, @@ -85,34 +84,6 @@ export const TemplateLayout: FC<{ children?: JSX.Element }> = ({ return ; } - const itemStyles = css` - text-decoration: none; - color: ${theme.palette.text.secondary}; - font-size: 14; - display: block; - padding: ${theme.spacing(0, 2, 2)}; - - &:hover { - color: ${theme.palette.text.primary}; - } - `; - - const activeItemStyles = css` - ${itemStyles} - color: ${theme.palette.text.primary}; - position: relative; - - &:before { - content: ""; - left: 0; - bottom: 0; - height: 2; - width: 100%; - background: ${theme.palette.secondary.dark}; - position: absolute; - } - `; - return ( <> = ({ }} /> -
- - - - isActive ? activeItemStyles : itemStyles - } - > - Summary - - - isActive ? activeItemStyles : itemStyles - } - > - Docs - - {data.permissions.canUpdateTemplate && ( - - isActive ? activeItemStyles : itemStyles - } - > - Source Code - - )} - - isActive ? activeItemStyles : itemStyles - } - > - Versions - - - isActive ? activeItemStyles : itemStyles - } - > - Embed - - {shouldShowInsights && ( - - isActive ? activeItemStyles : itemStyles - } - > - Insights - - )} - - -
+ + + Summary + + Docs + {data.permissions.canUpdateTemplate && ( + Source Code + )} + Versions + Embed + {shouldShowInsights && ( + Insights + )} + From f677c4470ba44fd49d547c3102ee9af1d98a1b27 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Thu, 19 Oct 2023 09:13:21 -0300 Subject: [PATCH 10/26] chore(site): add custom popover component (#10319) --- site/.eslintrc.yaml | 4 + .../Navbar/UserDropdown/BorderedMenu.tsx | 30 -- .../UserDropdown/UserDropdown.stories.tsx | 9 +- .../Navbar/UserDropdown/UserDropdown.tsx | 137 +++++---- .../UserDropdownContent.stories.tsx | 42 --- .../UserDropdown/UserDropdownContent.test.tsx | 17 +- .../UserDropdown/UserDropdownContent.tsx | 9 +- .../components/HelpTooltip/HelpTooltip.tsx | 2 + site/src/components/IconField/IconField.tsx | 66 +++-- site/src/components/Popover/Popover.tsx | 178 ++++++++++++ .../PopoverContainer.stories.tsx | 20 -- .../PopoverContainer/PopoverContainer.tsx | 244 ---------------- .../Resources/PortForwardButton.tsx | 91 +++--- .../Resources/SSHButton/SSHButton.stories.tsx | 2 +- .../Resources/SSHButton/SSHButton.tsx | 51 ++-- .../TemplateInsightsPage/DateRange.tsx | 188 ++++++------ site/src/pages/TerminalPage/TerminalPage.tsx | 148 +++++----- .../UsersTable/EditRolesButton.stories.tsx | 2 +- .../UsersPage/UsersTable/EditRolesButton.tsx | 63 ++-- .../BuildParametersPopover.tsx | 178 ++++++------ .../pages/WorkspacePage/WorkspaceStats.tsx | 269 +++++++++--------- .../pages/WorkspacesPage/WorkspacesButton.tsx | 58 +--- 22 files changed, 791 insertions(+), 1017 deletions(-) delete mode 100644 site/src/components/Dashboard/Navbar/UserDropdown/BorderedMenu.tsx delete mode 100644 site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.stories.tsx create mode 100644 site/src/components/Popover/Popover.tsx delete mode 100644 site/src/components/PopoverContainer/PopoverContainer.stories.tsx delete mode 100644 site/src/components/PopoverContainer/PopoverContainer.tsx diff --git a/site/.eslintrc.yaml b/site/.eslintrc.yaml index 478c1e070415e..018bf9043d72e 100644 --- a/site/.eslintrc.yaml +++ b/site/.eslintrc.yaml @@ -135,6 +135,10 @@ rules: message: "You should use the Alert component provided on components/Alert/Alert" + - name: "@mui/material/Popover" + message: + "You should use the Popover component provided on + components/Popover/Popover" no-unused-vars: "off" "object-curly-spacing": "off" react-hooks/exhaustive-deps: warn diff --git a/site/src/components/Dashboard/Navbar/UserDropdown/BorderedMenu.tsx b/site/src/components/Dashboard/Navbar/UserDropdown/BorderedMenu.tsx deleted file mode 100644 index b57984aa6dab7..0000000000000 --- a/site/src/components/Dashboard/Navbar/UserDropdown/BorderedMenu.tsx +++ /dev/null @@ -1,30 +0,0 @@ -import { css } from "@emotion/css"; -import { useTheme } from "@emotion/react"; -import Popover, { type PopoverProps } from "@mui/material/Popover"; -import type { FC, PropsWithChildren } from "react"; - -type BorderedMenuVariant = "user-dropdown"; - -export type BorderedMenuProps = Omit & { - variant?: BorderedMenuVariant; -}; - -export const BorderedMenu: FC> = ({ - children, - variant, - ...rest -}) => { - const theme = useTheme(); - - const paper = css` - width: 260px; - border-radius: ${theme.shape.borderRadius}px; - box-shadow: ${theme.shadows[6]}; - `; - - return ( - - {children} - - ); -}; diff --git a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.stories.tsx b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.stories.tsx index 7ebffa89a0461..394d4846e2a2f 100644 --- a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.stories.tsx +++ b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.stories.tsx @@ -1,4 +1,4 @@ -import { MockUser } from "testHelpers/entities"; +import { MockBuildInfo, MockUser } from "testHelpers/entities"; import { UserDropdown } from "./UserDropdown"; import type { Meta, StoryObj } from "@storybook/react"; @@ -7,6 +7,13 @@ const meta: Meta = { component: UserDropdown, args: { user: MockUser, + isDefaultOpen: true, + buildInfo: MockBuildInfo, + supportLinks: [ + { icon: "docs", name: "Documentation", target: "" }, + { icon: "bug", name: "Report a bug", target: "" }, + { icon: "chat", name: "Join the Coder Discord", target: "" }, + ], }, }; diff --git a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.tsx b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.tsx index 50143eeecf217..d8194ac8c567e 100644 --- a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.tsx +++ b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdown.tsx @@ -1,21 +1,25 @@ import Badge from "@mui/material/Badge"; -import MenuItem from "@mui/material/MenuItem"; -import { useState, FC, PropsWithChildren, MouseEvent } from "react"; +import { FC, PropsWithChildren } from "react"; import { colors } from "theme/colors"; import * as TypesGen from "api/typesGenerated"; import { navHeight } from "theme/constants"; -import { BorderedMenu } from "./BorderedMenu"; import { DropdownArrow } from "components/DropdownArrow/DropdownArrow"; import { UserAvatar } from "components/UserAvatar/UserAvatar"; import { UserDropdownContent } from "./UserDropdownContent"; import { BUTTON_SM_HEIGHT } from "theme/theme"; import { css } from "@emotion/react"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; export interface UserDropdownProps { user: TypesGen.User; buildInfo?: TypesGen.BuildInfoResponse; supportLinks?: TypesGen.LinkConfig[]; onSignOut: () => void; + isDefaultOpen?: boolean; } export const UserDropdown: FC> = ({ @@ -23,76 +27,69 @@ export const UserDropdown: FC> = ({ user, supportLinks, onSignOut, + isDefaultOpen, }: UserDropdownProps) => { - const [anchorEl, setAnchorEl] = useState(); - - const handleDropdownClick = (ev: MouseEvent): void => { - setAnchorEl(ev.currentTarget); - }; - const onPopoverClose = () => { - setAnchorEl(undefined); - }; - return ( - <> - css` - height: ${navHeight}px; - padding: ${theme.spacing(1.5, 0)}; + + {(popover) => ( + <> + + + - - - - + ({ + ".MuiPaper-root": { + width: 260, + boxShadow: theme.shadows[6], + }, + })} + > + + + + )} + ); }; diff --git a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.stories.tsx b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.stories.tsx deleted file mode 100644 index f9f8c5a8a5a03..0000000000000 --- a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.stories.tsx +++ /dev/null @@ -1,42 +0,0 @@ -import { MockUser } from "testHelpers/entities"; -import { UserDropdownContent } from "./UserDropdownContent"; -import type { Meta, StoryObj } from "@storybook/react"; - -const meta: Meta = { - title: "components/UserDropdownContent", - component: UserDropdownContent, -}; - -export default meta; -type Story = StoryObj; - -export const ExampleNoRoles: Story = { - args: { - user: { - ...MockUser, - roles: [], - }, - }, -}; - -export const ExampleOneRole: Story = { - args: { - user: { - ...MockUser, - roles: [{ name: "member", display_name: "Member" }], - }, - }, -}; - -export const ExampleThreeRoles: Story = { - args: { - user: { - ...MockUser, - roles: [ - { name: "admin", display_name: "Admin" }, - { name: "member", display_name: "Member" }, - { name: "auditor", display_name: "Auditor" }, - ], - }, - }, -}; diff --git a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.test.tsx b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.test.tsx index 4e488f04d0b3d..f9650a60d0443 100644 --- a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.test.tsx +++ b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.test.tsx @@ -2,15 +2,14 @@ import { screen } from "@testing-library/react"; import { MockUser } from "testHelpers/entities"; import { render, waitForLoaderToBeRemoved } from "testHelpers/renderHelpers"; import { Language, UserDropdownContent } from "./UserDropdownContent"; +import { Popover } from "components/Popover/Popover"; describe("UserDropdownContent", () => { it("has the correct link for the account item", async () => { render( - , + + + , ); await waitForLoaderToBeRemoved(); @@ -25,11 +24,9 @@ describe("UserDropdownContent", () => { it("calls the onSignOut function", async () => { const onSignOut = jest.fn(); render( - , + + + , ); await waitForLoaderToBeRemoved(); screen.getByText(Language.signOutLabel).click(); diff --git a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.tsx b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.tsx index ebc103da57887..99d2d65a48f9a 100644 --- a/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.tsx +++ b/site/src/components/Dashboard/Navbar/UserDropdown/UserDropdownContent.tsx @@ -16,6 +16,7 @@ import { type Interpolation, type Theme, } from "@emotion/react"; +import { usePopover } from "components/Popover/Popover"; export const Language = { accountLabel: "Account", @@ -82,7 +83,6 @@ export interface UserDropdownContentProps { user: TypesGen.User; buildInfo?: TypesGen.BuildInfoResponse; supportLinks?: TypesGen.LinkConfig[]; - onPopoverClose: () => void; onSignOut: () => void; } @@ -90,9 +90,14 @@ export const UserDropdownContent: FC = ({ buildInfo, user, supportLinks, - onPopoverClose, onSignOut, }) => { + const popover = usePopover(); + + const onPopoverClose = () => { + popover.setIsOpen(false); + }; + return (
diff --git a/site/src/components/HelpTooltip/HelpTooltip.tsx b/site/src/components/HelpTooltip/HelpTooltip.tsx index 92990c318f9b9..a9c01acb44fa3 100644 --- a/site/src/components/HelpTooltip/HelpTooltip.tsx +++ b/site/src/components/HelpTooltip/HelpTooltip.tsx @@ -1,4 +1,6 @@ import Link from "@mui/material/Link"; +// This is used as base for the main HelpTooltip component +// eslint-disable-next-line no-restricted-imports -- Read above import Popover, { PopoverProps } from "@mui/material/Popover"; import HelpIcon from "@mui/icons-material/HelpOutline"; import OpenInNewIcon from "@mui/icons-material/OpenInNew"; diff --git a/site/src/components/IconField/IconField.tsx b/site/src/components/IconField/IconField.tsx index ab6fd044a888d..6e8bf11506cfa 100644 --- a/site/src/components/IconField/IconField.tsx +++ b/site/src/components/IconField/IconField.tsx @@ -1,15 +1,19 @@ import Button from "@mui/material/Button"; import InputAdornment from "@mui/material/InputAdornment"; -import Popover from "@mui/material/Popover"; import TextField, { TextFieldProps } from "@mui/material/TextField"; import { makeStyles } from "@mui/styles"; import Picker from "@emoji-mart/react"; -import { useRef, FC, useState } from "react"; +import { FC } from "react"; import { DropdownArrow } from "components/DropdownArrow/DropdownArrow"; import { Stack } from "components/Stack/Stack"; import { colors } from "theme/colors"; import data from "@emoji-mart/data/sets/14/twitter.json"; import icons from "theme/icons.json"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; // See: https://github.com/missive/emoji-mart/issues/51#issuecomment-287353222 const urlFromUnifiedCode = (unified: string) => @@ -45,8 +49,6 @@ const IconField: FC = ({ onPickEmoji, ...textFieldProps }) => { } const styles = useStyles(); - const emojiButtonRef = useRef(null); - const [isEmojiPickerOpen, setIsEmojiPickerOpen] = useState(false); const hasIcon = textFieldProps.value && textFieldProps.value !== ""; return ( @@ -71,36 +73,32 @@ const IconField: FC = ({ onPickEmoji, ...textFieldProps }) => { }} /> - - - { - setIsEmojiPickerOpen(false); - }} - > - { - const value = emoji.src ?? urlFromUnifiedCode(emoji.unified); - onPickEmoji(value); - setIsEmojiPickerOpen(false); - }} - /> + + {(popover) => ( + <> + + + + + { + const value = emoji.src ?? urlFromUnifiedCode(emoji.unified); + onPickEmoji(value); + popover.setIsOpen(false); + }} + /> + + + )} ); diff --git a/site/src/components/Popover/Popover.tsx b/site/src/components/Popover/Popover.tsx new file mode 100644 index 0000000000000..aa9b39183e6c2 --- /dev/null +++ b/site/src/components/Popover/Popover.tsx @@ -0,0 +1,178 @@ +import { + ReactElement, + ReactNode, + cloneElement, + createContext, + useContext, + useEffect, + useRef, + useState, +} from "react"; +// This is used as base for the main Popover component +// eslint-disable-next-line no-restricted-imports -- Read above +import MuiPopover, { + type PopoverProps as MuiPopoverProps, +} from "@mui/material/Popover"; + +type TriggerMode = "hover" | "click"; + +type TriggerRef = React.RefObject; + +type TriggerElement = ReactElement<{ + onClick?: () => void; + ref: TriggerRef; +}>; + +type PopoverContextValue = { + isOpen: boolean; + setIsOpen: React.Dispatch>; + triggerRef: TriggerRef; + mode: TriggerMode; +}; + +const PopoverContext = createContext( + undefined, +); + +export const Popover = (props: { + children: ReactNode | ((popover: PopoverContextValue) => ReactNode); // Allows inline usage + mode?: TriggerMode; + isDefaultOpen?: boolean; +}) => { + const [isOpen, setIsOpen] = useState(props.isDefaultOpen ?? false); + const triggerRef = useRef(null); + const value = { isOpen, setIsOpen, triggerRef, mode: props.mode ?? "click" }; + + return ( + + {typeof props.children === "function" + ? props.children(value) + : props.children} + + ); +}; + +export const usePopover = () => { + const context = useContext(PopoverContext); + if (!context) { + throw new Error( + "Popover compound components cannot be rendered outside the Popover component", + ); + } + return context; +}; + +export const PopoverTrigger = (props: { + children: TriggerElement; + hover?: boolean; +}) => { + const popover = usePopover(); + + const clickProps = { + onClick: () => { + popover.setIsOpen((isOpen) => !isOpen); + }, + }; + + const hoverProps = { + onPointerEnter: () => { + popover.setIsOpen(true); + }, + onPointerLeave: () => { + popover.setIsOpen(false); + }, + }; + + return cloneElement(props.children, { + ...(popover.mode === "click" ? clickProps : hoverProps), + ref: popover.triggerRef, + }); +}; + +type Horizontal = "left" | "right"; + +export const PopoverContent = ( + props: Omit & { + horizontal?: Horizontal; + }, +) => { + const popover = usePopover(); + const [isReady, setIsReady] = useState(false); + const horizontal = props.horizontal ?? "left"; + const hoverMode = popover.mode === "hover"; + + // This is a hack to make sure the popover is not rendered until the trigger + // is ready. This is a limitation on MUI that does not support defaultIsOpen + // on Popover but we need it to storybook the component. + useEffect(() => { + if (!isReady && popover.triggerRef.current !== null) { + setIsReady(true); + } + }, [isReady, popover.triggerRef]); + + if (!popover.triggerRef.current) { + return null; + } + + return ( + ({ + // When it is on hover mode, and the moude is moving from the trigger to + // the popover, if there is any space, the popover will be closed. I + // found this is a limitation on how MUI structured the component. It is + // not a big issue for now but we can reavaluate it in the future. + marginTop: hoverMode ? undefined : theme.spacing(1), + pointerEvents: hoverMode ? "none" : undefined, + "& .MuiPaper-root": { + minWidth: theme.spacing(40), + fontSize: 14, + pointerEvents: hoverMode ? "auto" : undefined, + }, + })} + {...horizontalProps(horizontal)} + {...modeProps(popover)} + {...props} + open={popover.isOpen} + onClose={() => popover.setIsOpen(false)} + anchorEl={popover.triggerRef.current} + /> + ); +}; + +const modeProps = (popover: PopoverContextValue) => { + if (popover.mode === "hover") { + return { + onMouseEnter: () => { + popover.setIsOpen(true); + }, + onMouseLeave: () => { + popover.setIsOpen(false); + }, + }; + } + + return {}; +}; + +const horizontalProps = (horizontal: Horizontal) => { + if (horizontal === "right") { + return { + anchorOrigin: { + vertical: "bottom", + horizontal: "right", + }, + transformOrigin: { + vertical: "top", + horizontal: "right", + }, + } as const; + } + + return { + anchorOrigin: { + vertical: "bottom", + horizontal: "left", + }, + } as const; +}; diff --git a/site/src/components/PopoverContainer/PopoverContainer.stories.tsx b/site/src/components/PopoverContainer/PopoverContainer.stories.tsx deleted file mode 100644 index 879faf7d389c9..0000000000000 --- a/site/src/components/PopoverContainer/PopoverContainer.stories.tsx +++ /dev/null @@ -1,20 +0,0 @@ -import { Meta, StoryObj } from "@storybook/react"; -import { PopoverContainer } from "./PopoverContainer"; -import Button from "@mui/material/Button"; - -const meta: Meta = { - title: "components/PopoverContainer", - component: PopoverContainer, - args: { - anchorButton: , - children:

Hiya!

, - originY: "bottom", - }, -}; - -export default meta; - -type Story = StoryObj; -const Example: Story = {}; - -export { Example as PopoverContainer }; diff --git a/site/src/components/PopoverContainer/PopoverContainer.tsx b/site/src/components/PopoverContainer/PopoverContainer.tsx deleted file mode 100644 index 833d8b267d5fe..0000000000000 --- a/site/src/components/PopoverContainer/PopoverContainer.tsx +++ /dev/null @@ -1,244 +0,0 @@ -/** - * @file Abstracts over MUI's Popover component to simplify using it (and hide - * some of the wonkier parts of the API). - * - * Just place a button and some content in the component, and things just work. - * No setup needed with hooks or refs. - */ -import { - type KeyboardEvent, - type MouseEvent, - type PropsWithChildren, - type ReactElement, - createContext, - useCallback, - useContext, - useEffect, - useRef, - useState, -} from "react"; - -import { type Theme, type SystemStyleObject, Box } from "@mui/system"; -import Popover, { type PopoverOrigin } from "@mui/material/Popover"; -import { useNavigate, type LinkProps } from "react-router-dom"; -import { useTheme } from "@emotion/react"; - -function getButton(container: HTMLElement) { - return ( - container.querySelector("button") ?? - container.querySelector('[aria-role="button"]') - ); -} - -const ClosePopoverContext = createContext<(() => void) | null>(null); - -type PopoverLinkProps = LinkProps & { - to: string; - sx?: SystemStyleObject; -}; - -/** - * A custom version of a React Router Link that makes sure to close the popover - * before starting a navigation. - * - * This is necessary because React Router's navigation logic doesn't work well - * with modals (including MUI's base Popover component). - * - * --- - * If the page being navigated to has lazy loading and isn't available yet, the - * previous components are supposed to be hidden during the transition, but - * because most React modals use React.createPortal to put content outside of - * the main DOM tree, React Router has no way of knowing about them. So open - * modals have a high risk of not disappearing until the page transition - * finishes and the previous components fully unmount. - */ -export function PopoverLink({ - children, - to, - sx, - ...linkProps -}: PopoverLinkProps) { - const closePopover = useContext(ClosePopoverContext); - if (closePopover === null) { - throw new Error("PopoverLink is not located inside of a PopoverContainer"); - } - - // Luckily, useNavigate and Link are designed to be imperative/declarative - // mirrors of each other, so their inputs should never get out of sync - const navigate = useNavigate(); - const theme = useTheme(); - - const onClick = (event: MouseEvent) => { - event.preventDefault(); - event.stopPropagation(); - closePopover(); - - // Hacky, but by using a promise to push the navigation to resolve via the - // micro-task queue, there's guaranteed to be a period for the popover to - // close. Tried React DOM's flushSync function, but it was unreliable. - void Promise.resolve().then(() => { - navigate(to, linkProps); - }); - }; - - return ( - - {children} - - ); -} - -type PopoverContainerProps = PropsWithChildren<{ - /** - * Does not require any hooks or refs to work. Also does not override any refs - * or event handlers attached to the button. - */ - anchorButton: ReactElement; - - width?: number; - originX?: PopoverOrigin["horizontal"]; - originY?: PopoverOrigin["vertical"]; - sx?: SystemStyleObject; -}>; - -export function PopoverContainer({ - children, - anchorButton, - originX = 0, - originY = 0, - width = 320, - sx = {}, -}: PopoverContainerProps) { - const parentClosePopover = useContext(ClosePopoverContext); - if (parentClosePopover !== null) { - throw new Error( - "Popover detected inside of Popover - this will always be a bad user experience", - ); - } - - const buttonContainerRef = useRef(null); - - // Ref value is for effects and event listeners; state value is for React - // renders. Have to duplicate state because after the initial render, it's - // never safe to reference ref contents inside a render path, especially with - // React 18 concurrency. Duplication is a necessary evil because of MUI's - // weird, clunky APIs - const anchorButtonRef = useRef(null); - const [loadedButton, setLoadedButton] = useState(); - - // Makes container listen to changes in button. If this approach becomes - // untenable in the future, it can be replaced with React.cloneElement, but - // the trade-off there is that every single anchorButton will need to be - // wrapped inside React.forwardRef, making the abstraction leak a little more - useEffect(() => { - const buttonContainer = buttonContainerRef.current; - if (buttonContainer === null) { - throw new Error("Please attach container ref to button container"); - } - - const initialButton = getButton(buttonContainer); - if (initialButton === null) { - throw new Error("Initial ref query failed"); - } - anchorButtonRef.current = initialButton; - - const onContainerMutation: MutationCallback = () => { - const newButton = getButton(buttonContainer); - if (newButton === null) { - throw new Error("Semantic button removed after DOM update"); - } - - anchorButtonRef.current = newButton; - setLoadedButton((current) => { - return current === undefined ? undefined : newButton; - }); - }; - - const observer = new MutationObserver(onContainerMutation); - observer.observe(buttonContainer, { - childList: true, - subtree: true, - }); - - return () => observer.disconnect(); - }, []); - - // Not using useInteractive because the container element is just meant to - // catch events from the inner button, not act as a button itself - const onInnerButtonInteraction = () => { - if (anchorButtonRef.current === null) { - throw new Error("Usable ref value is unavailable"); - } - - setLoadedButton(anchorButtonRef.current); - }; - - const onInnerButtonKeydown = (event: KeyboardEvent) => { - if (event.key === "Enter" || event.key === " ") { - onInnerButtonInteraction(); - } - }; - - const closePopover = useCallback(() => { - setLoadedButton(undefined); - }, []); - - return ( - <> - {/* Cannot switch with Box component; breaks implementation */} -
- {anchorButton} -
- - - - {children} - - - - ); -} diff --git a/site/src/components/Resources/PortForwardButton.tsx b/site/src/components/Resources/PortForwardButton.tsx index f22e0e2082a80..83f54aeebb73b 100644 --- a/site/src/components/Resources/PortForwardButton.tsx +++ b/site/src/components/Resources/PortForwardButton.tsx @@ -1,11 +1,9 @@ import Box from "@mui/material/Box"; import Link from "@mui/material/Link"; -import Popover from "@mui/material/Popover"; import CircularProgress from "@mui/material/CircularProgress"; import OpenInNewOutlined from "@mui/icons-material/OpenInNewOutlined"; import { css } from "@emotion/css"; import { useTheme } from "@emotion/react"; -import { useRef, useState } from "react"; import { useQuery } from "react-query"; import { colors } from "theme/colors"; import { @@ -22,6 +20,11 @@ import type { WorkspaceAgentListeningPort, } from "api/typesGenerated"; import { portForwardURL } from "utils/portForward"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; export interface PortForwardButtonProps { host: string; @@ -32,9 +35,6 @@ export interface PortForwardButtonProps { export const PortForwardButton: React.FC = (props) => { const theme = useTheme(); - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(false); - const id = isOpen ? "schedule-popover" : undefined; const portsQuery = useQuery({ queryKey: ["portForward", props.agent.id], queryFn: () => getAgentListeningPorts(props.agent.id), @@ -42,43 +42,36 @@ export const PortForwardButton: React.FC = (props) => { refetchInterval: 5_000, }); - const onClose = () => { - setIsOpen(false); - }; - return ( - <> - { - setIsOpen(true); - }} - > - Ports - {portsQuery.data ? ( - theme.spacing(0, 0.5), - borderRadius: "50%", - display: "flex", - alignItems: "center", - justifyContent: "center", - backgroundColor: colors.gray[11], - ml: 1, - }} - > - {portsQuery.data.ports.length} - - ) : ( - - )} - - + + + Ports + {portsQuery.data ? ( + theme.spacing(0, 0.5), + borderRadius: "50%", + display: "flex", + alignItems: "center", + justifyContent: "center", + backgroundColor: colors.gray[11], + ml: 1, + }} + > + {portsQuery.data.ports.length} + + ) : ( + + )} + + + = (props) => { margin-top: ${theme.spacing(0.5)}; `, }} - id={id} - open={isOpen} - anchorEl={anchorRef.current} - onClose={onClose} - anchorOrigin={{ - vertical: "bottom", - horizontal: "right", - }} - transformOrigin={{ - vertical: "top", - horizontal: "right", - }} > - - + + ); }; diff --git a/site/src/components/Resources/SSHButton/SSHButton.stories.tsx b/site/src/components/Resources/SSHButton/SSHButton.stories.tsx index 5829af000d81c..b5e451160fc60 100644 --- a/site/src/components/Resources/SSHButton/SSHButton.stories.tsx +++ b/site/src/components/Resources/SSHButton/SSHButton.stories.tsx @@ -22,7 +22,7 @@ export const Opened: Story = { args: { workspaceName: MockWorkspace.name, agentName: MockWorkspaceAgent.name, - defaultIsOpen: true, + isDefaultOpen: true, sshPrefix: "coder.", }, }; diff --git a/site/src/components/Resources/SSHButton/SSHButton.tsx b/site/src/components/Resources/SSHButton/SSHButton.tsx index 846ba5baaa1ea..d9827d07956c0 100644 --- a/site/src/components/Resources/SSHButton/SSHButton.tsx +++ b/site/src/components/Resources/SSHButton/SSHButton.tsx @@ -1,7 +1,6 @@ -import Popover from "@mui/material/Popover"; import { css } from "@emotion/css"; import { type Interpolation, type Theme, useTheme } from "@emotion/react"; -import { type FC, type PropsWithChildren, useRef, useState } from "react"; +import { type FC, type PropsWithChildren } from "react"; import { HelpTooltipLink, HelpTooltipLinksGroup, @@ -11,41 +10,35 @@ import { docs } from "utils/docs"; import { CodeExample } from "../../CodeExample/CodeExample"; import { Stack } from "../../Stack/Stack"; import { SecondaryAgentButton } from "../AgentButton"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; export interface SSHButtonProps { workspaceName: string; agentName: string; - defaultIsOpen?: boolean; + isDefaultOpen?: boolean; sshPrefix?: string; } export const SSHButton: FC> = ({ workspaceName, agentName, - defaultIsOpen = false, + isDefaultOpen = false, sshPrefix, }) => { const theme = useTheme(); - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(defaultIsOpen); - const id = isOpen ? "schedule-popover" : undefined; - - const onClose = () => { - setIsOpen(false); - }; return ( - <> - { - setIsOpen(true); - }} - > - SSH - + + + SSH + - > = ({ margin-top: ${theme.spacing(0.25)}; `, }} - id={id} - open={isOpen} - anchorEl={anchorRef.current} - onClose={onClose} - anchorOrigin={{ - vertical: "bottom", - horizontal: "left", - }} - transformOrigin={{ - vertical: "top", - horizontal: "left", - }} > Run the following commands to connect with SSH: @@ -107,8 +88,8 @@ export const SSHButton: FC> = ({ SSH configuration - - + + ); }; diff --git a/site/src/pages/TemplatePage/TemplateInsightsPage/DateRange.tsx b/site/src/pages/TemplatePage/TemplateInsightsPage/DateRange.tsx index 9e0194e98d8df..aa6d43bc9a298 100644 --- a/site/src/pages/TemplatePage/TemplateInsightsPage/DateRange.tsx +++ b/site/src/pages/TemplatePage/TemplateInsightsPage/DateRange.tsx @@ -5,7 +5,6 @@ import "react-date-range/dist/styles.css"; import "react-date-range/dist/theme/default.css"; import Button from "@mui/material/Button"; import ArrowRightAltOutlined from "@mui/icons-material/ArrowRightAltOutlined"; -import Popover from "@mui/material/Popover"; import { DateRangePicker, createStaticRanges } from "react-date-range"; import { addDays, @@ -16,6 +15,11 @@ import { startOfHour, subDays, } from "date-fns"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; // The type definition from @types is wrong declare module "react-date-range" { @@ -41,8 +45,6 @@ export const DateRange = ({ onChange: (value: DateRangeValue) => void; }) => { const selectionStatusRef = useRef<"idle" | "selecting">("idle"); - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(false); const [ranges, setRanges] = useState([ { ...value, @@ -53,105 +55,91 @@ export const DateRange = ({ startDate: ranges[0].startDate as Date, endDate: ranges[0].endDate as Date, }; - const handleClose = () => { - const now = new Date(); - onChange({ - startDate: startOfDay(currentRange.startDate), - endDate: isToday(currentRange.endDate) - ? startOfHour(addHours(now, 1)) - : startOfDay(addDays(currentRange.endDate, 1)), - }); - setIsOpen(false); - }; return ( - <> - - - { - const range = item.selection; - setRanges([range]); - - // When it is the first selection, we don't want to close the popover - // We have to do that ourselves because the library doesn't provide a way to do it - if (selectionStatusRef.current === "idle") { - selectionStatusRef.current = "selecting"; - return; - } - - selectionStatusRef.current = "idle"; - const startDate = range.startDate as Date; - const endDate = range.endDate as Date; - onChange({ - startDate, - endDate, - }); - setIsOpen(false); - }} - moveRangeOnFirstSelection={false} - months={2} - ranges={ranges} - maxDate={new Date()} - direction="horizontal" - staticRanges={createStaticRanges([ - { - label: "Today", - range: () => ({ - startDate: new Date(), - endDate: new Date(), - }), - }, - { - label: "Yesterday", - range: () => ({ - startDate: subDays(new Date(), 1), - endDate: subDays(new Date(), 1), - }), - }, - { - label: "Last 7 days", - range: () => ({ - startDate: subDays(new Date(), 6), - endDate: new Date(), - }), - }, - { - label: "Last 14 days", - range: () => ({ - startDate: subDays(new Date(), 13), - endDate: new Date(), - }), - }, - { - label: "Last 30 days", - range: () => ({ - startDate: subDays(new Date(), 29), - endDate: new Date(), - }), - }, - ])} - /> - - + + {(popover) => ( + <> + + + + + { + const range = item.selection; + setRanges([range]); + + // When it is the first selection, we don't want to close the popover + // We have to do that ourselves because the library doesn't provide a way to do it + if (selectionStatusRef.current === "idle") { + selectionStatusRef.current = "selecting"; + return; + } + + selectionStatusRef.current = "idle"; + const startDate = range.startDate as Date; + const endDate = range.endDate as Date; + const now = new Date(); + onChange({ + startDate: startOfDay(startDate), + endDate: isToday(endDate) + ? startOfHour(addHours(now, 1)) + : startOfDay(addDays(endDate, 1)), + }); + popover.setIsOpen(false); + }} + moveRangeOnFirstSelection={false} + months={2} + ranges={ranges} + maxDate={new Date()} + direction="horizontal" + staticRanges={createStaticRanges([ + { + label: "Today", + range: () => ({ + startDate: new Date(), + endDate: new Date(), + }), + }, + { + label: "Yesterday", + range: () => ({ + startDate: subDays(new Date(), 1), + endDate: subDays(new Date(), 1), + }), + }, + { + label: "Last 7 days", + range: () => ({ + startDate: subDays(new Date(), 6), + endDate: new Date(), + }), + }, + { + label: "Last 14 days", + range: () => ({ + startDate: subDays(new Date(), 13), + endDate: new Date(), + }), + }, + { + label: "Last 30 days", + range: () => ({ + startDate: subDays(new Date(), 29), + endDate: new Date(), + }), + }, + ])} + /> + + + )} + ); }; diff --git a/site/src/pages/TerminalPage/TerminalPage.tsx b/site/src/pages/TerminalPage/TerminalPage.tsx index dfc52c8a91e24..1761cd3a987ea 100644 --- a/site/src/pages/TerminalPage/TerminalPage.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.tsx @@ -20,7 +20,6 @@ import Box from "@mui/material/Box"; import { useDashboard } from "components/Dashboard/DashboardProvider"; import { Region } from "api/typesGenerated"; import { getLatencyColor } from "utils/latency"; -import Popover from "@mui/material/Popover"; import { ProxyStatusLatency } from "components/ProxyStatusLatency/ProxyStatusLatency"; import { portForwardURL } from "utils/portForward"; import { @@ -31,6 +30,11 @@ import { } from "./TerminalAlerts"; import { useQuery } from "react-query"; import { deploymentConfig } from "api/queries/deployment"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; export const Language = { workspaceErrorMessagePrefix: "Unable to fetch workspace: ", @@ -332,13 +336,11 @@ const TerminalPage: FC = () => { const BottomBar = ({ proxy, latency }: { proxy: Region; latency?: number }) => { const theme = useTheme(); const color = getLatencyColor(theme, latency); - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(false); return ( theme.spacing(1, 2), + padding: theme.spacing(0, 2), background: (theme) => theme.palette.background.paper, display: "flex", alignItems: "center", @@ -347,82 +349,80 @@ const BottomBar = ({ proxy, latency }: { proxy: Region; latency?: number }) => { borderTop: (theme) => `1px solid ${theme.palette.divider}`, }} > - setIsOpen(true)} - onMouseLeave={() => setIsOpen(false)} - sx={{ - background: "none", - cursor: "pointer", - display: "flex", - alignItems: "center", - gap: 1, - border: 0, - }} - > - + + + + + + + theme.spacing(1, 2), + }, }} - /> - - - setIsOpen(false)} - sx={{ - pointerEvents: "none", - "& .MuiPaper-root": { - padding: (theme) => theme.spacing(1, 2), - marginTop: -1, - }, - }} - anchorOrigin={{ - vertical: "top", - horizontal: "right", - }} - transformOrigin={{ - vertical: "bottom", - horizontal: "right", - }} - > - theme.palette.text.secondary, - fontWeight: 500, + anchorOrigin={{ + vertical: "top", + horizontal: "right", + }} + transformOrigin={{ + vertical: "bottom", + horizontal: "right", }} > - Selected proxy - - - - - + theme.palette.text.secondary, + fontWeight: 500, + }} + > + Selected proxy + + + + + + + {proxy.display_name} - {proxy.display_name} + - - + ); diff --git a/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx b/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx index 422faabb66574..2d53044cfdd4f 100644 --- a/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx +++ b/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx @@ -10,7 +10,7 @@ const meta: Meta = { title: "pages/UsersPage/EditRolesButton", component: EditRolesButton, args: { - defaultIsOpen: true, + isDefaultOpen: true, }, }; diff --git a/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx b/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx index be5e38f9302a4..980122ba5f55d 100644 --- a/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx +++ b/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx @@ -1,8 +1,7 @@ import IconButton from "@mui/material/IconButton"; import { EditSquare } from "components/Icons/EditSquare"; -import { useRef, useState, FC } from "react"; +import { FC } from "react"; import { makeStyles } from "@mui/styles"; -import Popover from "@mui/material/Popover"; import { Stack } from "components/Stack/Stack"; import Checkbox from "@mui/material/Checkbox"; import UserIcon from "@mui/icons-material/PersonOutline"; @@ -12,6 +11,11 @@ import { HelpTooltipText, HelpTooltipTitle, } from "components/HelpTooltip/HelpTooltip"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; const roleDescriptions: Record = { owner: @@ -59,7 +63,7 @@ export interface EditRolesButtonProps { roles: Role[]; selectedRoles: Role[]; onChange: (roles: Role["name"][]) => void; - defaultIsOpen?: boolean; + isDefaultOpen?: boolean; oidcRoleSync: boolean; userLoginType: string; } @@ -69,14 +73,11 @@ export const EditRolesButton: FC = ({ selectedRoles, onChange, isLoading, - defaultIsOpen = false, + isDefaultOpen = false, userLoginType, oidcRoleSync, }) => { const styles = useStyles(); - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(defaultIsOpen); - const id = isOpen ? "edit-roles-popover" : undefined; const selectedRoleNames = selectedRoles.map((role) => role.name); const handleChange = (roleName: string) => { @@ -91,42 +92,30 @@ export const EditRolesButton: FC = ({ const canSetRoles = userLoginType !== "oidc" || (userLoginType === "oidc" && !oidcRoleSync); + if (!canSetRoles) { + return ( + + Externally controlled + + Roles for this user are controlled by the OIDC identity provider. + + + ); + } + return ( - <> - {canSetRoles ? ( + + setIsOpen(true)} > - ) : ( - - Externally controlled - - Roles for this user are controlled by the OIDC identity provider. - - - )} - - setIsOpen(false)} - anchorOrigin={{ - vertical: "bottom", - horizontal: "left", - }} - transformOrigin={{ - vertical: "top", - horizontal: "left", - }} - classes={{ paper: styles.popoverPaper }} - > + + +
= ({
- - + + ); }; diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx index 0b106af0890fc..333d7999e7855 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx @@ -1,7 +1,6 @@ import ExpandMoreOutlined from "@mui/icons-material/ExpandMoreOutlined"; import Box from "@mui/material/Box"; import Button from "@mui/material/Button"; -import Popover from "@mui/material/Popover"; import { useQuery } from "react-query"; import { getWorkspaceParameters } from "api/api"; import { @@ -19,10 +18,15 @@ import { HelpTooltipTitle, } from "components/HelpTooltip/HelpTooltip"; import { useFormik } from "formik"; -import { useRef, useState } from "react"; import { docs } from "utils/docs"; import { getFormHelpers } from "utils/formUtils"; import { getInitialRichParameterValues } from "utils/richParameters"; +import { + Popover, + PopoverContent, + PopoverTrigger, + usePopover, +} from "components/Popover/Popover"; export const BuildParametersPopover = ({ workspace, @@ -33,12 +37,43 @@ export const BuildParametersPopover = ({ disabled?: boolean; onSubmit: (buildParameters: WorkspaceBuildParameter[]) => void; }) => { - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(false); + return ( + + + + + ({ ".MuiPaper-root": { width: theme.spacing(38) } })} + > + + + + ); +}; + +const BuildParametersPopoverContent = ({ + workspace, + onSubmit, +}: { + workspace: Workspace; + onSubmit: (buildParameters: WorkspaceBuildParameter[]) => void; +}) => { + const popover = usePopover(); const { data: parameters } = useQuery({ queryKey: ["workspace", workspace.id, "parameters"], queryFn: () => getWorkspaceParameters(workspace), - enabled: isOpen, + enabled: popover.isOpen, }); const ephemeralParameters = parameters ? parameters.templateVersionRichParameters.filter((p) => p.ephemeral) @@ -46,93 +81,56 @@ export const BuildParametersPopover = ({ return ( <> - - { - setIsOpen(false); - }} - anchorOrigin={{ - vertical: "bottom", - horizontal: "right", - }} - transformOrigin={{ - vertical: "top", - horizontal: "right", - }} - sx={{ - ".MuiPaper-root": { - width: (theme) => theme.spacing(38), - marginTop: 1, - }, - }} - > - - {parameters && parameters.buildParameters && ephemeralParameters ? ( - ephemeralParameters.length > 0 ? ( - <> - theme.palette.text.secondary, - p: 2.5, - borderBottom: (theme) => - `1px solid ${theme.palette.divider}`, - }} - > - Build Options - - These parameters only apply for a single workspace start. - - - -
{ - onSubmit(buildParameters); - setIsOpen(false); - }} - ephemeralParameters={ephemeralParameters} - buildParameters={parameters.buildParameters} - /> - - - ) : ( - theme.palette.text.secondary, - p: 2.5, - borderBottom: (theme) => `1px solid ${theme.palette.divider}`, + {parameters && parameters.buildParameters && ephemeralParameters ? ( + ephemeralParameters.length > 0 ? ( + <> + theme.palette.text.secondary, + p: 2.5, + borderBottom: (theme) => `1px solid ${theme.palette.divider}`, + }} + > + Build Options + + These parameters only apply for a single workspace start. + + + + { + onSubmit(buildParameters); + popover.setIsOpen(false); }} + ephemeralParameters={ephemeralParameters} + buildParameters={parameters.buildParameters} + /> + + + ) : ( + theme.palette.text.secondary, + p: 2.5, + borderBottom: (theme) => `1px solid ${theme.palette.divider}`, + }} + > + Build Options + + This template has no ephemeral build options. + + + - Build Options - - This template has no ephemeral build options. - - - - Read the docs - - - - ) - ) : ( - - )} - - + Read the docs + + + + ) + ) : ( + + )} ); }; diff --git a/site/src/pages/WorkspacePage/WorkspaceStats.tsx b/site/src/pages/WorkspacePage/WorkspaceStats.tsx index c55f09d34f693..773be0baa59f6 100644 --- a/site/src/pages/WorkspacePage/WorkspaceStats.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceStats.tsx @@ -1,6 +1,6 @@ import Link from "@mui/material/Link"; import { WorkspaceOutdatedTooltip } from "components/WorkspaceOutdatedTooltip/WorkspaceOutdatedTooltip"; -import { FC, useRef, useState } from "react"; +import { FC } from "react"; import { Link as RouterLink } from "react-router-dom"; import { createDayString } from "utils/createDayString"; import { @@ -16,11 +16,16 @@ import IconButton from "@mui/material/IconButton"; import RemoveIcon from "@mui/icons-material/RemoveOutlined"; import { makeStyles } from "@mui/styles"; import AddIcon from "@mui/icons-material/AddOutlined"; -import Popover from "@mui/material/Popover"; import TextField from "@mui/material/TextField"; import Button from "@mui/material/Button"; import { WorkspaceStatusText } from "components/WorkspaceStatusBadge/WorkspaceStatusBadge"; import { DormantDeletionStat } from "components/WorkspaceDeletion"; +import { + Popover, + PopoverContent, + PopoverTrigger, + usePopover, +} from "components/Popover/Popover"; const Language = { workspaceDetails: "Workspace Details", @@ -62,10 +67,6 @@ export const WorkspaceStats: FC = ({ const styles = useStyles(); const deadlinePlusEnabled = maxDeadlineIncrease >= 1; const deadlineMinusEnabled = maxDeadlineDecrease >= 1; - const addButtonRef = useRef(null); - const subButtonRef = useRef(null); - const [isAddingTime, setIsAddingTime] = useState(false); - const [isSubTime, setIsSubTime] = useState(false); return ( <> @@ -138,26 +139,50 @@ export const WorkspaceStats: FC = ({ {canUpdateWorkspace && canEditDeadline(workspace) && ( - setIsSubTime(true)} - > - - - setIsAddingTime(true)} - > - - + + + + + + + + + + + + + + + + + + + + )} @@ -174,118 +199,106 @@ export const WorkspaceStats: FC = ({ /> )} + + ); +}; - setIsAddingTime(false)} - anchorOrigin={{ - vertical: "bottom", - horizontal: "right", - }} - transformOrigin={{ - vertical: "top", - horizontal: "right", +const AddTimeContent = (props: { + maxDeadlineIncrease: number; + onDeadlinePlus: (value: number) => void; +}) => { + const styles = useStyles(); + const popover = usePopover(); + + return ( + <> + Add hours to deadline + + Delay the shutdown of this workspace for a few more hours. This is only + applied once. + + { + e.preventDefault(); + const formData = new FormData(e.currentTarget); + const hours = Number(formData.get("hours")); + props.onDeadlinePlus(hours); + popover.setIsOpen(false); }} > - Add hours to deadline - - Delay the shutdown of this workspace for a few more hours. This is - only applied once. - - { - e.preventDefault(); - const formData = new FormData(e.currentTarget); - const hours = Number(formData.get("hours")); - onDeadlinePlus(hours); - setIsAddingTime(false); + - + inputProps={{ + min: 0, + max: props.maxDeadlineIncrease, + step: 1, + defaultValue: 1, + }} + /> - - - + + + + ); +}; - setIsSubTime(false)} - anchorOrigin={{ - vertical: "bottom", - horizontal: "right", - }} - transformOrigin={{ - vertical: "top", - horizontal: "right", +export const DecreaseTimeContent = (props: { + onDeadlineMinus: (hours: number) => void; + maxDeadlineDecrease: number; +}) => { + const styles = useStyles(); + const popover = usePopover(); + + return ( + <> + + Subtract hours to deadline + + + Anticipate the shutdown of this workspace for a few more hours. This is + only applied once. + +
{ + e.preventDefault(); + const formData = new FormData(e.currentTarget); + const hours = Number(formData.get("hours")); + props.onDeadlineMinus(hours); + popover.setIsOpen(false); }} > - - Subtract hours to deadline - - - Anticipate the shutdown of this workspace for a few more hours. This - is only applied once. - - { - e.preventDefault(); - const formData = new FormData(e.currentTarget); - const hours = Number(formData.get("hours")); - onDeadlineMinus(hours); - setIsSubTime(false); + - + inputProps={{ + min: 0, + max: props.maxDeadlineDecrease, + step: 1, + defaultValue: 1, + }} + /> - - -
+ + ); }; diff --git a/site/src/pages/WorkspacesPage/WorkspacesButton.tsx b/site/src/pages/WorkspacesPage/WorkspacesButton.tsx index dfb6e05be35b0..2e3d6563602df 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesButton.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesButton.tsx @@ -1,9 +1,4 @@ -import { - type PropsWithChildren, - type ReactNode, - useState, - useRef, -} from "react"; +import { type PropsWithChildren, type ReactNode, useState } from "react"; import { type Template } from "api/typesGenerated"; import { type UseQueryResult } from "react-query"; import { @@ -20,7 +15,11 @@ import { OverflowY } from "components/OverflowY/OverflowY"; import { EmptyState } from "components/EmptyState/EmptyState"; import { Avatar } from "components/Avatar/Avatar"; import { SearchBox } from "./WorkspacesSearchBox"; -import Popover from "@mui/material/Popover"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "components/Popover/Popover"; const ICON_SIZE = 18; const COLUMN_GAP = 1.5; @@ -42,9 +41,6 @@ export function WorkspacesButton({ const [searchTerm, setSearchTerm] = useState(""); const processed = sortTemplatesByUsersDesc(templates ?? [], searchTerm); - const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(false); - let emptyState: ReactNode = undefined; if (templates?.length === 0) { emptyState = ( @@ -62,37 +58,13 @@ export function WorkspacesButton({ } return ( - <> - - setIsOpen(false)} - anchorEl={anchorRef.current} - anchorOrigin={{ - vertical: "bottom", - horizontal: "right", - }} - transformOrigin={{ - vertical: "top", - horizontal: "right", - }} - css={(theme) => ({ - marginTop: theme.spacing(1), - "& .MuiPaper-root": { - width: theme.spacing(40), - }, - })} - > + + + + + setSearchTerm(newValue)} @@ -142,8 +114,8 @@ export function WorkspacesButton({ See all templates
- - + + ); } From 42c21d400fcc501495789d202ca893535f15bb66 Mon Sep 17 00:00:00 2001 From: Muhammad Atif Ali Date: Thu, 19 Oct 2023 15:30:48 +0300 Subject: [PATCH 11/26] fix(docs): update external-auth docs to use `coder_external_auth` (#10347) --- docs/admin/external-auth.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/admin/external-auth.md b/docs/admin/external-auth.md index 5b005cfe6e28c..73852a052201a 100644 --- a/docs/admin/external-auth.md +++ b/docs/admin/external-auth.md @@ -158,8 +158,8 @@ The following example will require users authenticate via GitHub and auto-clone a repo into the `~/coder` directory. ```hcl -data "coder_git_auth" "github" { - # Matches the ID of the git auth provider in Coder. +data "coder_external_auth" "github" { + # Matches the ID of the external auth provider in Coder. id = "github" } @@ -168,7 +168,7 @@ resource "coder_agent" "dev" { arch = "amd64" dir = "~/coder" env = { - GITHUB_TOKEN : data.coder_git_auth.github.access_token + GITHUB_TOKEN : data.coder_external_auth.github.access_token } startup_script = < Date: Thu, 19 Oct 2023 16:08:56 +0300 Subject: [PATCH 12/26] chore: bump github.com/aws/smithy-go from 1.14.2 to 1.15.0 (#10282) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- go.mod | 2 +- go.sum | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index ab60c12f53789..73e8179ff3b66 100644 --- a/go.mod +++ b/go.mod @@ -201,7 +201,7 @@ require ( ) require ( - github.com/aws/smithy-go v1.14.2 + github.com/aws/smithy-go v1.15.0 github.com/chromedp/cdproto v0.0.0-20230802225258-3cf4e6d46a89 github.com/chromedp/chromedp v0.9.2 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 diff --git a/go.sum b/go.sum index c128700886900..8fe52143c7a00 100644 --- a/go.sum +++ b/go.sum @@ -155,8 +155,9 @@ github.com/aws/aws-sdk-go-v2/service/ssooidc v1.15.1/go.mod h1:XO/VcyoQ8nKyKfFW/ github.com/aws/aws-sdk-go-v2/service/sts v1.21.1 h1:pAOJj+80tC8sPVgSDHzMYD6KLWsaLQ1kZw31PTeORbs= github.com/aws/aws-sdk-go-v2/service/sts v1.21.1/go.mod h1:G8SbvL0rFk4WOJroU8tKBczhsbhj2p/YY7qeJezJ3CI= github.com/aws/smithy-go v1.14.0/go.mod h1:Tg+OJXh4MB2R/uN61Ko2f6hTZwB/ZYGOtib8J3gBHzA= -github.com/aws/smithy-go v1.14.2 h1:MJU9hqBGbvWZdApzpvoF2WAIJDbtjK2NDJSiJP7HblQ= github.com/aws/smithy-go v1.14.2/go.mod h1:Tg+OJXh4MB2R/uN61Ko2f6hTZwB/ZYGOtib8J3gBHzA= +github.com/aws/smithy-go v1.15.0 h1:PS/durmlzvAFpQHDs4wi4sNNP9ExsqZh6IlfdHXgKK8= +github.com/aws/smithy-go v1.15.0/go.mod h1:Tg+OJXh4MB2R/uN61Ko2f6hTZwB/ZYGOtib8J3gBHzA= github.com/aymanbagabas/go-osc52 v1.0.3/go.mod h1:zT8H+Rk4VSabYN90pWyugflM3ZhpTZNC7cASDfUCdT4= github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k= github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8= From 557adab224114b2dce1dd80a774356fd21fb525e Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Thu, 19 Oct 2023 14:59:08 -0300 Subject: [PATCH 13/26] chore(site): remove template ACL XService (#10332) --- site/src/api/api.ts | 2 +- site/src/api/queries/templates.ts | 54 ++- .../TemplatePermissionsPage.tsx | 91 +++-- .../TemplatePermissionsPageView.tsx | 16 +- .../xServices/template/templateACLXService.ts | 366 ------------------ 5 files changed, 128 insertions(+), 401 deletions(-) delete mode 100644 site/src/xServices/template/templateACLXService.ts diff --git a/site/src/api/api.ts b/site/src/api/api.ts index dccdc383ccbf9..c602a727e389e 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -947,7 +947,7 @@ export const getTemplateACL = async ( export const updateTemplateACL = async ( templateId: string, data: TypesGen.UpdateTemplateACL, -): Promise => { +): Promise<{ message: string }> => { const response = await axios.patch( `/api/v2/templates/${templateId}/acl`, data, diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index e5a39a015b661..a295832fca00e 100644 --- a/site/src/api/queries/templates.ts +++ b/site/src/api/queries/templates.ts @@ -6,8 +6,13 @@ import { type TemplateVersion, CreateTemplateRequest, ProvisionerJob, + TemplateRole, } from "api/typesGenerated"; -import { type QueryClient, type QueryOptions } from "react-query"; +import { + MutationOptions, + type QueryClient, + type QueryOptions, +} from "react-query"; import { delay } from "utils/delay"; export const templateByNameKey = (orgId: string, name: string) => [ @@ -36,6 +41,53 @@ export const templates = (orgId: string) => { }; }; +export const templateACL = (templateId: string) => { + return { + queryKey: ["templateAcl", templateId], + queryFn: () => API.getTemplateACL(templateId), + }; +}; + +export const setUserRole = ( + queryClient: QueryClient, +): MutationOptions< + Awaited>, + unknown, + { templateId: string; userId: string; role: TemplateRole } +> => { + return { + mutationFn: ({ templateId, userId, role }) => + API.updateTemplateACL(templateId, { + user_perms: { + [userId]: role, + }, + }), + onSuccess: async (_res, { templateId }) => { + await queryClient.invalidateQueries(["templateAcl", templateId]); + }, + }; +}; + +export const setGroupRole = ( + queryClient: QueryClient, +): MutationOptions< + Awaited>, + unknown, + { templateId: string; groupId: string; role: TemplateRole } +> => { + return { + mutationFn: ({ templateId, groupId, role }) => + API.updateTemplateACL(templateId, { + group_perms: { + [groupId]: role, + }, + }), + onSuccess: async (_res, { templateId }) => { + await queryClient.invalidateQueries(["templateAcl", templateId]); + }, + }; +}; + export const templateExamples = (orgId: string) => { return { queryKey: [...getTemplatesQueryKey(orgId), "examples"], diff --git a/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPage.tsx b/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPage.tsx index 898202bd51b7c..6cf11ec1faf62 100644 --- a/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPage.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPage.tsx @@ -1,7 +1,6 @@ import Button from "@mui/material/Button"; import Link from "@mui/material/Link"; import ArrowRightAltOutlined from "@mui/icons-material/ArrowRightAltOutlined"; -import { useMachine } from "@xstate/react"; import { Paywall } from "components/Paywall/Paywall"; import { Stack } from "components/Stack/Stack"; import { useFeatureVisibility } from "hooks/useFeatureVisibility"; @@ -9,10 +8,12 @@ import { useOrganizationId } from "hooks/useOrganizationId"; import { FC } from "react"; import { Helmet } from "react-helmet-async"; import { pageTitle } from "utils/page"; -import { templateACLMachine } from "xServices/template/templateACLXService"; import { useTemplateSettings } from "../TemplateSettingsLayout"; import { TemplatePermissionsPageView } from "./TemplatePermissionsPageView"; import { docs } from "utils/docs"; +import { useMutation, useQuery, useQueryClient } from "react-query"; +import { setGroupRole, setUserRole, templateACL } from "api/queries/templates"; +import { displaySuccess } from "components/GlobalSnackbar/utils"; export const TemplatePermissionsPage: FC< React.PropsWithChildren @@ -20,10 +21,16 @@ export const TemplatePermissionsPage: FC< const organizationId = useOrganizationId(); const { template, permissions } = useTemplateSettings(); const { template_rbac: isTemplateRBACEnabled } = useFeatureVisibility(); - const [state, send] = useMachine(templateACLMachine, { - context: { templateId: template.id }, - }); - const { templateACL, userToBeUpdated, groupToBeUpdated } = state.context; + const templateACLQuery = useQuery(templateACL(template.id)); + const queryClient = useQueryClient(); + + const addUserMutation = useMutation(setUserRole(queryClient)); + const updateUserMutation = useMutation(setUserRole(queryClient)); + const removeUserMutation = useMutation(setUserRole(queryClient)); + + const addGroupMutation = useMutation(setGroupRole(queryClient)); + const updateGroupMutation = useMutation(setGroupRole(queryClient)); + const removeGroupMutation = useMutation(setGroupRole(queryClient)); return ( <> @@ -58,29 +65,67 @@ export const TemplatePermissionsPage: FC< { - send("ADD_USER", { user, role, onDone: reset }); + onAddUser={async (user, role, reset) => { + await addUserMutation.mutateAsync({ + templateId: template.id, + userId: user.id, + role, + }); + reset(); }} - isAddingUser={state.matches("addingUser")} - onUpdateUser={(user, role) => { - send("UPDATE_USER_ROLE", { user, role }); + isAddingUser={addUserMutation.isLoading} + onUpdateUser={async (user, role) => { + await updateUserMutation.mutateAsync({ + templateId: template.id, + userId: user.id, + role, + }); + displaySuccess("User role updated successfully!"); }} - updatingUser={userToBeUpdated} - onRemoveUser={(user) => { - send("REMOVE_USER", { user }); + updatingUserId={ + updateUserMutation.isLoading + ? updateUserMutation.variables?.userId + : undefined + } + onRemoveUser={async (user) => { + await removeUserMutation.mutateAsync({ + templateId: template.id, + userId: user.id, + role: "", + }); + displaySuccess("User removed successfully!"); }} - onAddGroup={(group, role, reset) => { - send("ADD_GROUP", { group, role, onDone: reset }); + onAddGroup={async (group, role, reset) => { + await addGroupMutation.mutateAsync({ + templateId: template.id, + groupId: group.id, + role, + }); + reset(); }} - isAddingGroup={state.matches("addingGroup")} - onUpdateGroup={(group, role) => { - send("UPDATE_GROUP_ROLE", { group, role }); + isAddingGroup={addGroupMutation.isLoading} + onUpdateGroup={async (group, role) => { + await updateGroupMutation.mutateAsync({ + templateId: template.id, + groupId: group.id, + role, + }); + displaySuccess("Group role updated successfully!"); }} - updatingGroup={groupToBeUpdated} - onRemoveGroup={(group) => { - send("REMOVE_GROUP", { group }); + updatingGroupId={ + updateGroupMutation.isLoading + ? updateGroupMutation.variables?.groupId + : undefined + } + onRemoveGroup={async (group) => { + await removeGroupMutation.mutateAsync({ + groupId: group.id, + templateId: template.id, + role: "", + }); + displaySuccess("Group removed successfully!"); }} /> )} diff --git a/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPageView.tsx b/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPageView.tsx index e106242ccc443..a4cded2abe1b6 100644 --- a/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPageView.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplatePermissionsPage/TemplatePermissionsPageView.tsx @@ -161,7 +161,7 @@ export interface TemplatePermissionsPageViewProps { ) => void; isAddingUser: boolean; onUpdateUser: (user: TemplateUser, role: TemplateRole) => void; - updatingUser: TemplateUser | undefined; + updatingUserId: TemplateUser["id"] | undefined; onRemoveUser: (user: TemplateUser) => void; // Group onAddGroup: ( @@ -171,7 +171,7 @@ export interface TemplatePermissionsPageViewProps { ) => void; isAddingGroup: boolean; onUpdateGroup: (group: TemplateGroup, role: TemplateRole) => void; - updatingGroup: TemplateGroup | undefined; + updatingGroupId?: TemplateGroup["id"] | undefined; onRemoveGroup: (group: Group) => void; } @@ -185,13 +185,13 @@ export const TemplatePermissionsPageView: FC< // User onAddUser, isAddingUser, - updatingUser, + updatingUserId, onUpdateUser, onRemoveUser, // Group onAddGroup, isAddingGroup, - updatingGroup, + updatingGroupId, onUpdateGroup, onRemoveGroup, }) => { @@ -265,9 +265,7 @@ export const TemplatePermissionsPageView: FC< { onUpdateGroup( group, @@ -313,9 +311,7 @@ export const TemplatePermissionsPageView: FC< { onUpdateUser( user, diff --git a/site/src/xServices/template/templateACLXService.ts b/site/src/xServices/template/templateACLXService.ts deleted file mode 100644 index 9b0b1e7462d1d..0000000000000 --- a/site/src/xServices/template/templateACLXService.ts +++ /dev/null @@ -1,366 +0,0 @@ -import { getTemplateACL, updateTemplateACL } from "api/api"; -import { - TemplateACL, - TemplateGroup, - TemplateRole, - TemplateUser, -} from "api/typesGenerated"; -import { displaySuccess } from "components/GlobalSnackbar/utils"; -import { assign, createMachine } from "xstate"; - -export const templateACLMachine = createMachine( - { - schema: { - context: {} as { - templateId: string; - templateACL?: TemplateACL; - // User - userToBeAdded?: TemplateUser; - userToBeUpdated?: TemplateUser; - addUserCallback?: () => void; - // Group - groupToBeAdded?: TemplateGroup; - groupToBeUpdated?: TemplateGroup; - addGroupCallback?: () => void; - }, - services: {} as { - loadTemplateACL: { - data: TemplateACL; - }; - // User - addUser: { - data: unknown; - }; - updateUser: { - data: unknown; - }; - // Group - addGroup: { - data: unknown; - }; - updateGroup: { - data: unknown; - }; - }, - events: {} as // User - | { - type: "ADD_USER"; - user: TemplateUser; - role: TemplateRole; - onDone: () => void; - } - | { - type: "UPDATE_USER_ROLE"; - user: TemplateUser; - role: TemplateRole; - } - | { - type: "REMOVE_USER"; - user: TemplateUser; - } - // Group - | { - type: "ADD_GROUP"; - group: TemplateGroup; - role: TemplateRole; - onDone: () => void; - } - | { - type: "UPDATE_GROUP_ROLE"; - group: TemplateGroup; - role: TemplateRole; - } - | { - type: "REMOVE_GROUP"; - group: TemplateGroup; - }, - }, - tsTypes: {} as import("./templateACLXService.typegen").Typegen0, - id: "templateACL", - initial: "loading", - states: { - loading: { - invoke: { - src: "loadTemplateACL", - onDone: { - actions: ["assignTemplateACL"], - target: "idle", - }, - }, - }, - idle: { - on: { - // User - ADD_USER: { target: "addingUser", actions: ["assignUserToBeAdded"] }, - UPDATE_USER_ROLE: { - target: "updatingUser", - actions: ["assignUserToBeUpdated"], - }, - REMOVE_USER: { - target: "removingUser", - actions: ["removeUserFromTemplateACL"], - }, - // Group - ADD_GROUP: { - target: "addingGroup", - actions: ["assignGroupToBeAdded"], - }, - UPDATE_GROUP_ROLE: { - target: "updatingGroup", - actions: ["assignGroupToBeUpdated"], - }, - REMOVE_GROUP: { - target: "removingGroup", - actions: ["removeGroupFromTemplateACL"], - }, - }, - }, - // User - addingUser: { - invoke: { - src: "addUser", - onDone: { - target: "idle", - actions: ["addUserToTemplateACL", "runAddUserCallback"], - }, - }, - }, - updatingUser: { - invoke: { - src: "updateUser", - onDone: { - target: "idle", - actions: [ - "updateUserOnTemplateACL", - "clearUserToBeUpdated", - "displayUpdateUserSuccessMessage", - ], - }, - }, - }, - removingUser: { - invoke: { - src: "removeUser", - onDone: { - target: "idle", - actions: ["displayRemoveUserSuccessMessage"], - }, - }, - }, - // Group - addingGroup: { - invoke: { - src: "addGroup", - onDone: { - target: "idle", - actions: ["addGroupToTemplateACL", "runAddGroupCallback"], - }, - }, - }, - updatingGroup: { - invoke: { - src: "updateGroup", - onDone: { - target: "idle", - actions: [ - "updateGroupOnTemplateACL", - "clearGroupToBeUpdated", - "displayUpdateGroupSuccessMessage", - ], - }, - }, - }, - removingGroup: { - invoke: { - src: "removeGroup", - onDone: { - target: "idle", - actions: ["displayRemoveGroupSuccessMessage"], - }, - }, - }, - }, - }, - { - services: { - loadTemplateACL: ({ templateId }) => getTemplateACL(templateId), - // User - addUser: ({ templateId }, { user, role }) => - updateTemplateACL(templateId, { - user_perms: { - [user.id]: role, - }, - }), - updateUser: ({ templateId }, { user, role }) => - updateTemplateACL(templateId, { - user_perms: { - [user.id]: role, - }, - }), - removeUser: ({ templateId }, { user }) => - updateTemplateACL(templateId, { - user_perms: { - [user.id]: "", - }, - }), - // Group - addGroup: ({ templateId }, { group, role }) => - updateTemplateACL(templateId, { - group_perms: { - [group.id]: role, - }, - }), - updateGroup: ({ templateId }, { group, role }) => - updateTemplateACL(templateId, { - group_perms: { - [group.id]: role, - }, - }), - removeGroup: ({ templateId }, { group }) => - updateTemplateACL(templateId, { - group_perms: { - [group.id]: "", - }, - }), - }, - actions: { - assignTemplateACL: assign({ - templateACL: (_, { data }) => data, - }), - // User - assignUserToBeAdded: assign({ - userToBeAdded: (_, { user, role }) => ({ ...user, role }), - addUserCallback: (_, { onDone }) => onDone, - }), - addUserToTemplateACL: assign({ - templateACL: ({ templateACL, userToBeAdded }) => { - if (!userToBeAdded) { - throw new Error("No user to be added"); - } - if (!templateACL) { - throw new Error("Template ACL is not loaded yet"); - } - return { - ...templateACL, - users: [...templateACL.users, userToBeAdded], - }; - }, - }), - runAddUserCallback: ({ addUserCallback }) => { - if (addUserCallback) { - addUserCallback(); - } - }, - assignUserToBeUpdated: assign({ - userToBeUpdated: (_, { user, role }) => ({ ...user, role }), - }), - updateUserOnTemplateACL: assign({ - templateACL: ({ templateACL, userToBeUpdated }) => { - if (!userToBeUpdated) { - throw new Error("No user to be added"); - } - if (!templateACL) { - throw new Error("Template ACL is not loaded yet"); - } - return { - ...templateACL, - users: templateACL.users.map((oldTemplateUser) => { - return oldTemplateUser.id === userToBeUpdated.id - ? userToBeUpdated - : oldTemplateUser; - }), - }; - }, - }), - clearUserToBeUpdated: assign({ - userToBeUpdated: (_) => undefined, - }), - displayUpdateUserSuccessMessage: () => { - displaySuccess("User role update successfully!"); - }, - removeUserFromTemplateACL: assign({ - templateACL: ({ templateACL }, { user }) => { - if (!templateACL) { - throw new Error("Template ACL is not loaded yet"); - } - return { - ...templateACL, - users: templateACL.users.filter((oldTemplateUser) => { - return oldTemplateUser.id !== user.id; - }), - }; - }, - }), - displayRemoveUserSuccessMessage: () => { - displaySuccess("User removed successfully!"); - }, - // Group - assignGroupToBeAdded: assign({ - groupToBeAdded: (_, { group, role }) => ({ ...group, role }), - addGroupCallback: (_, { onDone }) => onDone, - }), - addGroupToTemplateACL: assign({ - templateACL: ({ templateACL, groupToBeAdded }) => { - if (!groupToBeAdded) { - throw new Error("No group to be added"); - } - if (!templateACL) { - throw new Error("Template ACL is not loaded yet"); - } - return { - ...templateACL, - group: [...templateACL.group, groupToBeAdded], - }; - }, - }), - runAddGroupCallback: ({ addGroupCallback }) => { - if (addGroupCallback) { - addGroupCallback(); - } - }, - assignGroupToBeUpdated: assign({ - groupToBeUpdated: (_, { group, role }) => ({ ...group, role }), - }), - updateGroupOnTemplateACL: assign({ - templateACL: ({ templateACL, groupToBeUpdated }) => { - if (!groupToBeUpdated) { - throw new Error("No group to be added"); - } - if (!templateACL) { - throw new Error("Template ACL is not loaded yet"); - } - return { - ...templateACL, - group: templateACL.group.map((oldTemplateGroup) => { - return oldTemplateGroup.id === groupToBeUpdated.id - ? groupToBeUpdated - : oldTemplateGroup; - }), - }; - }, - }), - clearGroupToBeUpdated: assign({ - groupToBeUpdated: (_) => undefined, - }), - displayUpdateGroupSuccessMessage: () => { - displaySuccess("Group role update successfully!"); - }, - removeGroupFromTemplateACL: assign({ - templateACL: ({ templateACL }, { group }) => { - if (!templateACL) { - throw new Error("Template ACL is not loaded yet"); - } - return { - ...templateACL, - group: templateACL.group.filter((oldTemplateGroup) => { - return oldTemplateGroup.id !== group.id; - }), - }; - }, - }), - displayRemoveGroupSuccessMessage: () => { - displaySuccess("Group removed successfully!"); - }, - }, - }, -); From ab2904a6765b386d3b77bcf7f94f8891d62c27df Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 19 Oct 2023 14:31:48 -0400 Subject: [PATCH 14/26] feat: add user groups column to users table (#10284) * refactor: extract UserRoleCell into separate component * wip: add placeholder Groups column * fix: remove redundant css styles * refactor: update EditRolesButton to use Sets to detect selections * wip: commit progress for updated roles column * wip: commit current role pill progress * fix: update state sync logic * chore: add groupsByUserId query options factory * fix: update return value of select function * chore: drill groups data down to cell component * wip: commit current cell progress * fix: remove redundant classes * wip: commit current styling progress * fix: update line height for CTA * fix: update spacing * chore: add tooltip for Groups column header * fix: remove tsbuild file * refactor: consolidate tooltip components * fix: update font size defaults inside theme * fix: expand hoverable/clickable area of groups cell * fix: remove possible undefined cases from HelpTooltip * chore: add popover functionality to groups * wip: commit progress on groups tooltip * fix: remove zero-height group name visual bug * feat: get basic version of user group tooltips done * perf: move sort order callback outside loop * fix: update spacing for tooltip * feat: make popovers entirely hover-based * fix: disable scroll locking for popover * docs: add comments explaining some pitfalls with Popover component * refactor: simplify userRoleCell implementation * feat: complete main feature * fix: prevent scroll lock for role tooltips * fix: change import to type import * refactor: simplify how groups are clustered * refactor: update UserRoleCell to use Popover * refactor: remove unnecessary fragment * chore: add id/aria support for Popover * refactor: update UserGroupsCell to use Popover * chore: redo visual design for UserGroupsCell * fix: shrink UserGroupsCell text * fix: update UsersTable test to include groups info --- site/src/api/queries/groups.ts | 39 ++- site/src/components/Avatar/Avatar.tsx | 9 +- .../components/HelpTooltip/HelpTooltip.tsx | 4 +- site/src/components/Popover/Popover.tsx | 32 ++- site/src/pages/UsersPage/UsersPage.tsx | 137 +++++----- site/src/pages/UsersPage/UsersPageView.tsx | 14 +- .../UsersTable/EditRolesButton.stories.tsx | 6 +- .../UsersPage/UsersTable/EditRolesButton.tsx | 12 +- .../UsersTable/TableColumnHelpTooltip.tsx | 58 +++++ .../UsersPage/UsersTable/UserGroupsCell.tsx | 118 +++++++++ .../UsersPage/UsersTable/UserRoleCell.tsx | 171 +++++++++++++ .../UsersTable/UserRoleHelpTooltip.tsx | 31 --- .../UsersTable/UsersTable.stories.tsx | 8 + .../pages/UsersPage/UsersTable/UsersTable.tsx | 43 +++- .../UsersPage/UsersTable/UsersTableBody.tsx | 242 ++++++++---------- site/src/theme/constants.ts | 2 +- site/src/theme/theme.ts | 10 +- 17 files changed, 654 insertions(+), 282 deletions(-) create mode 100644 site/src/pages/UsersPage/UsersTable/TableColumnHelpTooltip.tsx create mode 100644 site/src/pages/UsersPage/UsersTable/UserGroupsCell.tsx create mode 100644 site/src/pages/UsersPage/UsersTable/UserRoleCell.tsx delete mode 100644 site/src/pages/UsersPage/UsersTable/UserRoleHelpTooltip.tsx diff --git a/site/src/api/queries/groups.ts b/site/src/api/queries/groups.ts index d34ad24a6abd0..3dc6759c12484 100644 --- a/site/src/api/queries/groups.ts +++ b/site/src/api/queries/groups.ts @@ -1,4 +1,4 @@ -import { QueryClient } from "react-query"; +import { QueryClient, UseQueryOptions } from "react-query"; import * as API from "api/api"; import { checkAuthorization } from "api/api"; import { @@ -25,6 +25,43 @@ export const group = (groupId: string) => { }; }; +export type GroupsByUserId = Readonly>; + +export function groupsByUserId(organizationId: string) { + return { + ...groups(organizationId), + select: (allGroups) => { + // Sorting here means that nothing has to be sorted for the individual + // user arrays later + const sorted = [...allGroups].sort((g1, g2) => { + const key = + g1.display_name && g2.display_name ? "display_name" : "name"; + + if (g1[key] === g2[key]) { + return 0; + } + + return g1[key] < g2[key] ? -1 : 1; + }); + + const userIdMapper = new Map(); + for (const group of sorted) { + for (const user of group.members) { + let groupsForUser = userIdMapper.get(user.id); + if (groupsForUser === undefined) { + groupsForUser = []; + userIdMapper.set(user.id, groupsForUser); + } + + groupsForUser.push(group); + } + } + + return userIdMapper as GroupsByUserId; + }, + } satisfies UseQueryOptions; +} + export const groupPermissions = (groupId: string) => { return { queryKey: [...getGroupQueryKey(groupId), "permissions"], diff --git a/site/src/components/Avatar/Avatar.tsx b/site/src/components/Avatar/Avatar.tsx index 02dbb2b141480..89de1f174ba4b 100644 --- a/site/src/components/Avatar/Avatar.tsx +++ b/site/src/components/Avatar/Avatar.tsx @@ -5,16 +5,23 @@ import { FC } from "react"; import { css, type Interpolation, type Theme } from "@emotion/react"; export type AvatarProps = MuiAvatarProps & { - size?: "sm" | "md" | "xl"; + size?: "xs" | "sm" | "md" | "xl"; colorScheme?: "light" | "darken"; fitImage?: boolean; }; const sizeStyles = { + xs: (theme) => ({ + width: theme.spacing(2), + height: theme.spacing(2), + fontSize: theme.spacing(1), + fontWeight: 700, + }), sm: (theme) => ({ width: theme.spacing(3), height: theme.spacing(3), fontSize: theme.spacing(1.5), + fontWeight: 600, }), md: {}, xl: (theme) => ({ diff --git a/site/src/components/HelpTooltip/HelpTooltip.tsx b/site/src/components/HelpTooltip/HelpTooltip.tsx index a9c01acb44fa3..b79babde64700 100644 --- a/site/src/components/HelpTooltip/HelpTooltip.tsx +++ b/site/src/components/HelpTooltip/HelpTooltip.tsx @@ -88,7 +88,7 @@ export const HelpPopover: FC< export const HelpTooltip: FC> = ({ children, - open, + open = false, size = "medium", icon: Icon = HelpIcon, iconClassName, @@ -96,7 +96,7 @@ export const HelpTooltip: FC> = ({ }) => { const theme = useTheme(); const anchorRef = useRef(null); - const [isOpen, setIsOpen] = useState(Boolean(open)); + const [isOpen, setIsOpen] = useState(open); const id = isOpen ? "help-popover" : undefined; const onClose = () => { diff --git a/site/src/components/Popover/Popover.tsx b/site/src/components/Popover/Popover.tsx index aa9b39183e6c2..5a936ac21a29e 100644 --- a/site/src/components/Popover/Popover.tsx +++ b/site/src/components/Popover/Popover.tsx @@ -5,6 +5,7 @@ import { createContext, useContext, useEffect, + useId, useRef, useState, } from "react"; @@ -19,11 +20,14 @@ type TriggerMode = "hover" | "click"; type TriggerRef = React.RefObject; type TriggerElement = ReactElement<{ - onClick?: () => void; ref: TriggerRef; + onClick?: () => void; + "aria-haspopup"?: boolean; + "aria-owns"?: string | undefined; }>; type PopoverContextValue = { + id: string; isOpen: boolean; setIsOpen: React.Dispatch>; triggerRef: TriggerRef; @@ -39,9 +43,17 @@ export const Popover = (props: { mode?: TriggerMode; isDefaultOpen?: boolean; }) => { + const hookId = useId(); const [isOpen, setIsOpen] = useState(props.isDefaultOpen ?? false); const triggerRef = useRef(null); - const value = { isOpen, setIsOpen, triggerRef, mode: props.mode ?? "click" }; + + const value: PopoverContextValue = { + isOpen, + setIsOpen, + triggerRef, + id: `${hookId}-popover`, + mode: props.mode ?? "click", + }; return ( @@ -62,10 +74,7 @@ export const usePopover = () => { return context; }; -export const PopoverTrigger = (props: { - children: TriggerElement; - hover?: boolean; -}) => { +export const PopoverTrigger = (props: { children: TriggerElement }) => { const popover = usePopover(); const clickProps = { @@ -85,6 +94,8 @@ export const PopoverTrigger = (props: { return cloneElement(props.children, { ...(popover.mode === "click" ? clickProps : hoverProps), + "aria-haspopup": true, + "aria-owns": popover.isOpen ? popover.id : undefined, ref: popover.triggerRef, }); }; @@ -118,10 +129,10 @@ export const PopoverContent = ( ({ - // When it is on hover mode, and the moude is moving from the trigger to + // When it is on hover mode, and the mode is moving from the trigger to // the popover, if there is any space, the popover will be closed. I // found this is a limitation on how MUI structured the component. It is - // not a big issue for now but we can reavaluate it in the future. + // not a big issue for now but we can re-evaluate it in the future. marginTop: hoverMode ? undefined : theme.spacing(1), pointerEvents: hoverMode ? "none" : undefined, "& .MuiPaper-root": { @@ -133,6 +144,7 @@ export const PopoverContent = ( {...horizontalProps(horizontal)} {...modeProps(popover)} {...props} + id={popover.id} open={popover.isOpen} onClose={() => popover.setIsOpen(false)} anchorEl={popover.triggerRef.current} @@ -143,10 +155,10 @@ export const PopoverContent = ( const modeProps = (popover: PopoverContextValue) => { if (popover.mode === "hover") { return { - onMouseEnter: () => { + onPointerEnter: () => { popover.setIsOpen(true); }, - onMouseLeave: () => { + onPointerLeave: () => { popover.setIsOpen(false); }, }; diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index c6c3e2ba19919..7d09df412b32e 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -1,23 +1,10 @@ -import { User } from "api/typesGenerated"; -import { DeleteDialog } from "components/Dialogs/DeleteDialog/DeleteDialog"; -import { nonInitialPage } from "components/PaginationWidget/utils"; -import { useMe } from "hooks/useMe"; -import { usePermissions } from "hooks/usePermissions"; -import { FC, ReactNode, useState } from "react"; -import { Helmet } from "react-helmet-async"; -import { useSearchParams, useNavigate } from "react-router-dom"; -import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; -import { ResetPasswordDialog } from "./ResetPasswordDialog"; -import { pageTitle } from "utils/page"; -import { UsersPageView } from "./UsersPageView"; -import { useStatusFilterMenu } from "./UsersFilter"; -import { useFilter } from "components/Filter/filter"; -import { useDashboard } from "components/Dashboard/DashboardProvider"; -import { useMutation, useQuery, useQueryClient } from "react-query"; +import { type FC, type ReactNode, useState } from "react"; + +import { type User } from "api/typesGenerated"; import { roles } from "api/queries/roles"; +import { groupsByUserId } from "api/queries/groups"; +import { getErrorMessage } from "api/errors"; import { deploymentConfig } from "api/queries/deployment"; -import { prepareQuery } from "utils/filters"; -import { usePagination } from "hooks"; import { users, suspendUser, @@ -27,38 +14,55 @@ import { updateRoles, authMethods, } from "api/queries/users"; -import { displayError, displaySuccess } from "components/GlobalSnackbar/utils"; -import { getErrorMessage } from "api/errors"; + +import { useMutation, useQuery, useQueryClient } from "react-query"; +import { useSearchParams, useNavigate } from "react-router-dom"; +import { useOrganizationId, usePagination } from "hooks"; +import { useMe } from "hooks/useMe"; +import { usePermissions } from "hooks/usePermissions"; +import { useStatusFilterMenu } from "./UsersFilter"; +import { useFilter } from "components/Filter/filter"; +import { useDashboard } from "components/Dashboard/DashboardProvider"; import { generateRandomString } from "utils/random"; +import { prepareQuery } from "utils/filters"; + +import { Helmet } from "react-helmet-async"; +import { DeleteDialog } from "components/Dialogs/DeleteDialog/DeleteDialog"; +import { nonInitialPage } from "components/PaginationWidget/utils"; +import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; +import { ResetPasswordDialog } from "./ResetPasswordDialog"; +import { pageTitle } from "utils/page"; +import { UsersPageView } from "./UsersPageView"; +import { displayError, displaySuccess } from "components/GlobalSnackbar/utils"; export const UsersPage: FC<{ children?: ReactNode }> = () => { const queryClient = useQueryClient(); const navigate = useNavigate(); + const searchParamsResult = useSearchParams(); const { entitlements } = useDashboard(); const [searchParams] = searchParamsResult; - const filter = searchParams.get("filter") ?? ""; - const pagination = usePagination({ - searchParamsResult, - }); + + const pagination = usePagination({ searchParamsResult }); const usersQuery = useQuery( users({ - q: prepareQuery(filter), + q: prepareQuery(searchParams.get("filter") ?? ""), limit: pagination.limit, offset: pagination.offset, }), ); + + const organizationId = useOrganizationId(); + const groupsByUserIdQuery = useQuery(groupsByUserId(organizationId)); + const authMethodsQuery = useQuery(authMethods()); + const { updateUsers: canEditUsers, viewDeploymentValues } = usePermissions(); const rolesQuery = useQuery(roles()); const { data: deploymentValues } = useQuery({ ...deploymentConfig(), enabled: viewDeploymentValues, }); - // Indicates if oidc roles are synced from the oidc idp. - // Assign 'false' if unknown. - const oidcRoleSyncEnabled = - viewDeploymentValues && - deploymentValues?.config.oidc?.user_role_field !== ""; + const me = useMe(); const useFilterResult = useFilter({ searchParamsResult, @@ -74,36 +78,47 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { status: option?.value, }), }); - const authMethodsQuery = useQuery(authMethods()); - const isLoading = - usersQuery.isLoading || rolesQuery.isLoading || authMethodsQuery.isLoading; - const [confirmSuspendUser, setConfirmSuspendUser] = useState(); + const [userToSuspend, setUserToSuspend] = useState(); const suspendUserMutation = useMutation(suspendUser(queryClient)); - const [confirmActivateUser, setConfirmActivateUser] = useState(); + const [userToActivate, setUserToActivate] = useState(); const activateUserMutation = useMutation(activateUser(queryClient)); - const [confirmDeleteUser, setConfirmDeleteUser] = useState(); + const [userToDelete, setUserToDelete] = useState(); const deleteUserMutation = useMutation(deleteUser(queryClient)); const [confirmResetPassword, setConfirmResetPassword] = useState<{ user: User; newPassword: string; }>(); - const updatePasswordMutation = useMutation(updatePassword()); + const updatePasswordMutation = useMutation(updatePassword()); const updateRolesMutation = useMutation(updateRoles(queryClient)); + // Indicates if oidc roles are synced from the oidc idp. + // Assign 'false' if unknown. + const oidcRoleSyncEnabled = + viewDeploymentValues && + deploymentValues?.config.oidc?.user_role_field !== ""; + + const isLoading = + usersQuery.isLoading || + rolesQuery.isLoading || + authMethodsQuery.isLoading || + groupsByUserIdQuery.isLoading; + return ( <> {pageTitle("Users")} + { navigate( @@ -116,9 +131,9 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { "/audit?filter=" + encodeURIComponent(`username:${user.username}`), ); }} - onDeleteUser={setConfirmDeleteUser} - onSuspendUser={setConfirmSuspendUser} - onActivateUser={setConfirmActivateUser} + onDeleteUser={setUserToDelete} + onSuspendUser={setUserToSuspend} + onActivateUser={setUserToActivate} onResetUserPassword={(user) => { setConfirmResetPassword({ user, @@ -147,9 +162,7 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { filterProps={{ filter: useFilterResult, error: usersQuery.error, - menus: { - status: statusMenu, - }, + menus: { status: statusMenu }, }} count={usersQuery.data?.count} page={pagination.page} @@ -158,48 +171,44 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { /> setUserToDelete(undefined)} onConfirm={async () => { try { - await deleteUserMutation.mutateAsync(confirmDeleteUser!.id); - setConfirmDeleteUser(undefined); + await deleteUserMutation.mutateAsync(userToDelete!.id); + setUserToDelete(undefined); displaySuccess("Successfully deleted the user."); } catch (e) { displayError(getErrorMessage(e, "Error deleting user.")); } }} - onCancel={() => { - setConfirmDeleteUser(undefined); - }} /> setUserToSuspend(undefined)} onConfirm={async () => { try { - await suspendUserMutation.mutateAsync(confirmSuspendUser!.id); - setConfirmSuspendUser(undefined); + await suspendUserMutation.mutateAsync(userToSuspend!.id); + setUserToSuspend(undefined); displaySuccess("Successfully suspended the user."); } catch (e) { displayError(getErrorMessage(e, "Error suspending user.")); } }} - onClose={() => { - setConfirmSuspendUser(undefined); - }} description={ <> Do you want to suspend the user{" "} - {confirmSuspendUser?.username ?? ""}? + {userToSuspend?.username ?? ""}? } /> @@ -207,26 +216,24 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { setUserToActivate(undefined)} onConfirm={async () => { try { - await activateUserMutation.mutateAsync(confirmActivateUser!.id); - setConfirmActivateUser(undefined); + await activateUserMutation.mutateAsync(userToActivate!.id); + setUserToActivate(undefined); displaySuccess("Successfully activated the user."); } catch (e) { displayError(getErrorMessage(e, "Error activating user.")); } }} - onClose={() => { - setConfirmActivateUser(undefined); - }} description={ <> Do you want to activate{" "} - {confirmActivateUser?.username ?? ""}? + {userToActivate?.username ?? ""}? } /> diff --git a/site/src/pages/UsersPage/UsersPageView.tsx b/site/src/pages/UsersPage/UsersPageView.tsx index 56cd39e85d446..5ea743d210781 100644 --- a/site/src/pages/UsersPage/UsersPageView.tsx +++ b/site/src/pages/UsersPage/UsersPageView.tsx @@ -1,5 +1,7 @@ -import { ComponentProps, FC } from "react"; -import * as TypesGen from "api/typesGenerated"; +import { type ComponentProps, type FC } from "react"; +import type * as TypesGen from "api/typesGenerated"; +import { type GroupsByUserId } from "api/queries/groups"; + import { UsersTable } from "./UsersTable/UsersTable"; import { UsersFilter } from "./UsersFilter"; import { @@ -12,10 +14,10 @@ export interface UsersPageViewProps { users?: TypesGen.User[]; roles?: TypesGen.AssignableRoles[]; isUpdatingUserRoles?: boolean; - canEditUsers?: boolean; + canEditUsers: boolean; oidcRoleSyncEnabled: boolean; canViewActivity?: boolean; - isLoading?: boolean; + isLoading: boolean; authMethods?: TypesGen.AuthMethods; onSuspendUser: (user: TypesGen.User) => void; onDeleteUser: (user: TypesGen.User) => void; @@ -30,6 +32,8 @@ export interface UsersPageViewProps { filterProps: ComponentProps; isNonInitialPage: boolean; actorID: string; + groupsByUserId: GroupsByUserId | undefined; + // Pagination count?: number; page: number; @@ -60,6 +64,7 @@ export const UsersPageView: FC> = ({ limit, onPageChange, page, + groupsByUserId, }) => { return ( <> @@ -77,6 +82,7 @@ export const UsersPageView: FC> = ({ = { export default meta; type Story = StoryObj; +const selectedRoleNames = new Set([MockUserAdminRole.name, MockOwnerRole.name]); + export const Open: Story = { args: { + selectedRoleNames, roles: MockSiteRoles, - selectedRoles: [MockUserAdminRole, MockOwnerRole], }, parameters: { chromatic: { delay: 300 }, @@ -30,8 +32,8 @@ export const Open: Story = { export const Loading: Story = { args: { isLoading: true, + selectedRoleNames, roles: MockSiteRoles, - selectedRoles: [MockUserAdminRole, MockOwnerRole], userLoginType: "password", oidcRoleSync: false, }, diff --git a/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx b/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx index 980122ba5f55d..eca9bcf389f9f 100644 --- a/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx +++ b/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx @@ -61,7 +61,7 @@ const Option: React.FC<{ export interface EditRolesButtonProps { isLoading: boolean; roles: Role[]; - selectedRoles: Role[]; + selectedRoleNames: Set; onChange: (roles: Role["name"][]) => void; isDefaultOpen?: boolean; oidcRoleSync: boolean; @@ -70,7 +70,7 @@ export interface EditRolesButtonProps { export const EditRolesButton: FC = ({ roles, - selectedRoles, + selectedRoleNames, onChange, isLoading, isDefaultOpen = false, @@ -78,11 +78,11 @@ export const EditRolesButton: FC = ({ oidcRoleSync, }) => { const styles = useStyles(); - const selectedRoleNames = selectedRoles.map((role) => role.name); const handleChange = (roleName: string) => { - if (selectedRoleNames.includes(roleName)) { - onChange(selectedRoleNames.filter((role) => role !== roleName)); + if (selectedRoleNames.has(roleName)) { + const serialized = [...selectedRoleNames]; + onChange(serialized.filter((role) => role !== roleName)); return; } @@ -126,7 +126,7 @@ export const EditRolesButton: FC = ({