feat: Allow admins to access member workspace terminals (#2114)

* allow workspace update permissions to access agents * do not show app links to users without workspace update access * address CR comments * initialize machine context in the hook * revert scoped connected status check
2025-07-30 22:19:53 +00:00 · 2022-06-10 10:46:48 -04:00
parent 0260e39d11
commit 953e8c8fe6
11 changed files with 168 additions and 51 deletions
--- a/coderd/coderd.go
+++ b/coderd/coderd.go
@@ -292,6 +292,7 @@ func New(options *Options) *API {
 				r.Use(
 					apiKeyMiddleware,
 					httpmw.ExtractWorkspaceAgentParam(options.Database),
+					httpmw.ExtractWorkspaceParam(options.Database),
 				)
 				r.Get("/", api.workspaceAgent)
 				r.Get("/dial", api.workspaceAgentDial)
--- a/coderd/coderd_test.go
+++ b/coderd/coderd_test.go
@@ -153,10 +153,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		"GET:/api/v2/workspaceagents/me/listen":                   {NoAuthorize: true},
 		"GET:/api/v2/workspaceagents/me/metadata":                 {NoAuthorize: true},
 		"GET:/api/v2/workspaceagents/me/turn":                     {NoAuthorize: true},
-		"GET:/api/v2/workspaceagents/{workspaceagent}":            {NoAuthorize: true},
-		"GET:/api/v2/workspaceagents/{workspaceagent}/dial":       {NoAuthorize: true},
 		"GET:/api/v2/workspaceagents/{workspaceagent}/iceservers": {NoAuthorize: true},
-		"GET:/api/v2/workspaceagents/{workspaceagent}/pty":        {NoAuthorize: true},
 		"GET:/api/v2/workspaceagents/{workspaceagent}/turn":       {NoAuthorize: true},

 		// These endpoints have more assertions. This is good, add more endpoints to assert if you can!
@@ -210,6 +207,18 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 			AssertAction: rbac.ActionRead,
 			AssertObject: workspaceRBACObj,
 		},
+		"GET:/api/v2/workspaceagents/{workspaceagent}": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: workspaceRBACObj,
+		},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/dial": {
+			AssertAction: rbac.ActionUpdate,
+			AssertObject: workspaceRBACObj,
+		},
+		"GET:/api/v2/workspaceagents/{workspaceagent}/pty": {
+			AssertAction: rbac.ActionUpdate,
+			AssertObject: workspaceRBACObj,
+		},
 		"GET:/api/v2/workspaces/": {
 			StatusCode:   http.StatusOK,
 			AssertAction: rbac.ActionRead,
@@ -378,6 +387,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 			route = strings.ReplaceAll(route, "{workspacebuild}", workspace.LatestBuild.ID.String())
 			route = strings.ReplaceAll(route, "{workspacename}", workspace.Name)
 			route = strings.ReplaceAll(route, "{workspacebuildname}", workspace.LatestBuild.Name)
+			route = strings.ReplaceAll(route, "{workspaceagent}", workspaceResources[0].Agents[0].ID.String())
 			route = strings.ReplaceAll(route, "{template}", template.ID.String())
 			route = strings.ReplaceAll(route, "{hash}", file.Hash)
 			route = strings.ReplaceAll(route, "{workspaceresource}", workspaceResources[0].ID.String())
--- a/coderd/httpmw/apikey.go
+++ b/coderd/httpmw/apikey.go
@@ -37,11 +37,11 @@ type userRolesKey struct{}
 // AuthorizationUserRoles returns the roles used for authorization.
 // Comes from the ExtractAPIKey handler.
 func AuthorizationUserRoles(r *http.Request) database.GetAuthorizationUserRolesRow {
-	apiKey, ok := r.Context().Value(userRolesKey{}).(database.GetAuthorizationUserRolesRow)
+	userRoles, ok := r.Context().Value(userRolesKey{}).(database.GetAuthorizationUserRolesRow)
 	if !ok {
 		panic("developer error: user roles middleware not provided")
 	}
-	return apiKey
+	return userRoles
 }

 // OAuth2Configs is a collection of configurations for OAuth-based authentication.
--- a/coderd/httpmw/workspaceagentparam.go
+++ b/coderd/httpmw/workspaceagentparam.go
@@ -6,6 +6,8 @@ import (
 	"errors"
 	"net/http"

+	"github.com/go-chi/chi/v5"
+
 	"github.com/coder/coder/coderd/database"
 	"github.com/coder/coder/coderd/httpapi"
 )
@@ -74,24 +76,9 @@ func ExtractWorkspaceAgentParam(db database.Store) func(http.Handler) http.Handl
 				})
 				return
 			}
-			workspace, err := db.GetWorkspaceByID(r.Context(), build.WorkspaceID)
-			if err != nil {
-				httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
-					Message: "Internal error fetching workspace.",
-					Detail:  err.Error(),
-				})
-				return
-			}
-
-			apiKey := APIKey(r)
-			if apiKey.UserID != workspace.OwnerID {
-				httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
-					Message: "Getting non-personal agents isn't supported.",
-				})
-				return
-			}

 			ctx := context.WithValue(r.Context(), workspaceAgentParamContextKey{}, agent)
+			chi.RouteContext(ctx).URLParams.Add("workspace", build.WorkspaceID.String())
 			next.ServeHTTP(rw, r.WithContext(ctx))
 		})
 	}
--- a/coderd/workspaceagents.go
+++ b/coderd/workspaceagents.go
@@ -21,6 +21,7 @@ import (
 	"github.com/coder/coder/coderd/database"
 	"github.com/coder/coder/coderd/httpapi"
 	"github.com/coder/coder/coderd/httpmw"
+	"github.com/coder/coder/coderd/rbac"
 	"github.com/coder/coder/coderd/turnconn"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/peer"
@@ -31,6 +32,10 @@ import (

 func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) {
 	workspaceAgent := httpmw.WorkspaceAgentParam(r)
+	workspace := httpmw.WorkspaceParam(r)
+	if !api.Authorize(rw, r, rbac.ActionRead, workspace) {
+		return
+	}
 	dbApps, err := api.Database.GetWorkspaceAppsByAgentID(r.Context(), workspaceAgent.ID)
 	if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
 		httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
@@ -58,6 +63,10 @@ func (api *API) workspaceAgentDial(rw http.ResponseWriter, r *http.Request) {
 	defer api.websocketWaitGroup.Done()

 	workspaceAgent := httpmw.WorkspaceAgentParam(r)
+	workspace := httpmw.WorkspaceParam(r)
+	if !api.Authorize(rw, r, rbac.ActionUpdate, workspace) {
+		return
+	}
 	apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency)
 	if err != nil {
 		httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
@@ -369,6 +378,10 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) {
 	defer api.websocketWaitGroup.Done()

 	workspaceAgent := httpmw.WorkspaceAgentParam(r)
+	workspace := httpmw.WorkspaceParam(r)
+	if !api.Authorize(rw, r, rbac.ActionUpdate, workspace) {
+		return
+	}
 	apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency)
 	if err != nil {
 		httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
--- a/site/src/components/Resources/Resources.tsx
+++ b/site/src/components/Resources/Resources.tsx
@@ -63,9 +63,10 @@ interface ResourcesProps {
  resources?: WorkspaceResource[]
  getResourcesError?: Error
  workspace: Workspace
+  canUpdateWorkspace: boolean
 }

-export const Resources: FC<ResourcesProps> = ({ resources, getResourcesError, workspace }) => {
+export const Resources: FC<ResourcesProps> = ({ resources, getResourcesError, workspace, canUpdateWorkspace }) => {
  const styles = useStyles()
  const theme: Theme = useTheme()

@@ -89,7 +90,7 @@ export const Resources: FC<ResourcesProps> = ({ resources, getResourcesError, wo
                  <AgentHelpTooltip />
                </Stack>
              </TableCell>
-              <TableCell>{Language.accessLabel}</TableCell>
+              {canUpdateWorkspace && <TableCell>{Language.accessLabel}</TableCell>}
              <TableCell>{Language.statusLabel}</TableCell>
            </TableHeaderRow>
          </TableHead>
@@ -130,28 +131,30 @@ export const Resources: FC<ResourcesProps> = ({ resources, getResourcesError, wo
                      {agent.name}
                      <span className={styles.operatingSystem}>{agent.operating_system}</span>
                    </TableCell>
-                    <TableCell>
-                      <Stack>
-                        {agent.status === "connected" && (
-                          <TerminalLink
-                            className={styles.accessLink}
-                            workspaceName={workspace.name}
-                            agentName={agent.name}
-                            userName={workspace.owner_name}
-                          />
-                        )}
-                        {agent.status === "connected" &&
-                          agent.apps.map((app) => (
-                            <AppLink
-                              key={app.name}
-                              appIcon={app.icon}
-                              appName={app.name}
-                              userName={workspace.owner_name}
+                    {canUpdateWorkspace && (
+                      <TableCell>
+                        <Stack>
+                          {agent.status === "connected" && (
+                            <TerminalLink
+                              className={styles.accessLink}
                              workspaceName={workspace.name}
+                              agentName={agent.name}
+                              userName={workspace.owner_name}
                            />
-                          ))}
-                      </Stack>
-                    </TableCell>
+                          )}
+                          {agent.status === "connected" &&
+                            agent.apps.map((app) => (
+                              <AppLink
+                                key={app.name}
+                                appIcon={app.icon}
+                                appName={app.name}
+                                userName={workspace.owner_name}
+                                workspaceName={workspace.name}
+                              />
+                            ))}
+                        </Stack>
+                      </TableCell>
+                    )}
                    <TableCell>
                      <span style={{ color: getDisplayAgentStatus(theme, agent).color }}>
                        {getDisplayAgentStatus(theme, agent).status}
--- a/site/src/components/Workspace/Workspace.stories.tsx
+++ b/site/src/components/Workspace/Workspace.stories.tsx
@@ -22,6 +22,13 @@ Started.args = {
  handleStop: action("stop"),
  resources: [Mocks.MockWorkspaceResource, Mocks.MockWorkspaceResource2],
  builds: [Mocks.MockWorkspaceBuild],
+  canUpdateWorkspace: true,
+}
+
+export const WithoutUpdateAccess = Template.bind({})
+WithoutUpdateAccess.args = {
+  ...Started.args,
+  canUpdateWorkspace: false,
 }

 export const Starting = Template.bind({})
--- a/site/src/components/Workspace/Workspace.tsx
+++ b/site/src/components/Workspace/Workspace.tsx
@@ -28,6 +28,7 @@ export interface WorkspaceProps {
  resources?: TypesGen.WorkspaceResource[]
  getResourcesError?: Error
  builds?: TypesGen.WorkspaceBuild[]
+  canUpdateWorkspace: boolean
 }

 /**
@@ -44,6 +45,7 @@ export const Workspace: FC<WorkspaceProps> = ({
  resources,
  getResourcesError,
  builds,
+  canUpdateWorkspace,
 }) => {
  const styles = useStyles()
  const navigate = useNavigate()
@@ -80,7 +82,12 @@ export const Workspace: FC<WorkspaceProps> = ({
          <WorkspaceStats workspace={workspace} />

          {!!resources && !!resources.length && (
-            <Resources resources={resources} getResourcesError={getResourcesError} workspace={workspace} />
+            <Resources
+              resources={resources}
+              getResourcesError={getResourcesError}
+              workspace={workspace}
+              canUpdateWorkspace={canUpdateWorkspace}
+            />
          )}

          <WorkspaceSection title="Timeline" contentsProps={{ className: styles.timelineContents }}>
--- a/site/src/pages/WorkspacePage/WorkspacePage.tsx
+++ b/site/src/pages/WorkspacePage/WorkspacePage.tsx
@@ -1,5 +1,5 @@
-import { useMachine } from "@xstate/react"
-import React, { useEffect } from "react"
+import { useMachine, useSelector } from "@xstate/react"
+import React, { useContext, useEffect } from "react"
 import { Helmet } from "react-helmet"
 import { useParams } from "react-router-dom"
 import { DeleteWorkspaceDialog } from "../../components/DeleteWorkspaceDialog/DeleteWorkspaceDialog"
@@ -8,6 +8,8 @@ import { FullScreenLoader } from "../../components/Loader/FullScreenLoader"
 import { Workspace } from "../../components/Workspace/Workspace"
 import { firstOrItem } from "../../util/array"
 import { pageTitle } from "../../util/page"
+import { selectUser } from "../../xServices/auth/authSelectors"
+import { XServiceContext } from "../../xServices/StateContext"
 import { workspaceMachine } from "../../xServices/workspace/workspaceXService"
 import { workspaceScheduleBannerMachine } from "../../xServices/workspaceSchedule/workspaceScheduleBannerXService"

@@ -16,8 +18,17 @@ export const WorkspacePage: React.FC = () => {
  const username = firstOrItem(usernameQueryParam, null)
  const workspaceName = firstOrItem(workspaceQueryParam, null)

-  const [workspaceState, workspaceSend] = useMachine(workspaceMachine)
-  const { workspace, resources, getWorkspaceError, getResourcesError, builds } = workspaceState.context
+  const xServices = useContext(XServiceContext)
+  const me = useSelector(xServices.authXService, selectUser)
+
+  const [workspaceState, workspaceSend] = useMachine(workspaceMachine, {
+    context: {
+      userId: me?.id,
+    },
+  })
+  const { workspace, resources, getWorkspaceError, getResourcesError, builds, permissions } = workspaceState.context
+
+  const canUpdateWorkspace = !!permissions?.updateWorkspace

  const [bannerState, bannerSend] = useMachine(workspaceScheduleBannerMachine)

@@ -56,6 +67,7 @@ export const WorkspacePage: React.FC = () => {
          resources={resources}
          getResourcesError={getResourcesError instanceof Error ? getResourcesError : undefined}
          builds={builds}
+          canUpdateWorkspace={canUpdateWorkspace}
        />
        <DeleteWorkspaceDialog
          isOpen={workspaceState.matches({ ready: { build: "askingDelete" } })}
--- a/site/src/xServices/auth/authSelectors.ts
+++ b/site/src/xServices/auth/authSelectors.ts
@@ -10,3 +10,7 @@ export const selectOrgId = (state: AuthState): string | undefined => {
 export const selectPermissions = (state: AuthState): AuthContext["permissions"] => {
  return state.context.permissions
 }
+
+export const selectUser = (state: AuthState): AuthContext["me"] => {
+  return state.context.me
+}
--- a/site/src/xServices/workspace/workspaceXService.ts
+++ b/site/src/xServices/workspace/workspaceXService.ts
@@ -17,6 +17,8 @@ const Language = {
  buildError: "Workspace action failed.",
 }

+type Permissions = Record<keyof ReturnType<typeof permissionsToCheck>, boolean>
+
 export interface WorkspaceContext {
  workspace?: TypesGen.Workspace
  template?: TypesGen.Template
@@ -34,6 +36,10 @@ export interface WorkspaceContext {
  getBuildsError?: Error | unknown
  loadMoreBuildsError?: Error | unknown
  cancellationMessage: string
+  // permissions
+  permissions?: Permissions
+  checkPermissionsError?: Error | unknown
+  userId?: string
 }

 export type WorkspaceEvent =
@@ -48,6 +54,30 @@ export type WorkspaceEvent =
  | { type: "LOAD_MORE_BUILDS" }
  | { type: "REFRESH_TIMELINE" }

+export const checks = {
+  readWorkspace: "readWorkspace",
+  updateWorkspace: "updateWorkspace",
+} as const
+
+const permissionsToCheck = (workspace: TypesGen.Workspace) => ({
+  [checks.readWorkspace]: {
+    object: {
+      resource_type: "workspace",
+      resource_id: workspace.id,
+      owner_id: workspace.owner_id,
+    },
+    action: "read",
+  },
+  [checks.updateWorkspace]: {
+    object: {
+      resource_type: "workspace",
+      resource_id: workspace.id,
+      owner_id: workspace.owner_id,
+    },
+    action: "update",
+  },
+})
+
 export const workspaceMachine = createMachine(
  {
    tsTypes: {} as import("./workspaceXService.typegen").Typegen0,
@@ -82,6 +112,9 @@ export const workspaceMachine = createMachine(
        loadMoreBuilds: {
          data: TypesGen.WorkspaceBuild[]
        }
+        checkPermissions: {
+          data: TypesGen.UserAuthorizationResponse
+        }
      },
    },
    id: "workspaceState",
@@ -99,7 +132,7 @@ export const workspaceMachine = createMachine(
          src: "getWorkspace",
          id: "getWorkspace",
          onDone: {
-            target: "ready",
+            target: "gettingPermissions",
            actions: ["assignWorkspace"],
          },
          onError: {
@@ -109,6 +142,25 @@ export const workspaceMachine = createMachine(
        },
        tags: "loading",
      },
+      gettingPermissions: {
+        entry: "clearGetPermissionsError",
+        invoke: {
+          src: "checkPermissions",
+          id: "checkPermissions",
+          onDone: [
+            {
+              actions: ["assignPermissions"],
+              target: "ready",
+            },
+          ],
+          onError: [
+            {
+              actions: "assignGetPermissionsError",
+              target: "error",
+            },
+          ],
+        },
+      },
      ready: {
        type: "parallel",
        states: {
@@ -312,6 +364,7 @@ export const workspaceMachine = createMachine(
          workspace: undefined,
          template: undefined,
          build: undefined,
+          permissions: undefined,
        }),
      assignWorkspace: assign({
        workspace: (_, event) => event.data,
@@ -323,6 +376,17 @@ export const workspaceMachine = createMachine(
      assignTemplate: assign({
        template: (_, event) => event.data,
      }),
+      assignPermissions: assign({
+        // Setting event.data as Permissions to be more stricted. So we know
+        // what permissions we asked for.
+        permissions: (_, event) => event.data as Permissions,
+      }),
+      assignGetPermissionsError: assign({
+        checkPermissionsError: (_, event) => event.data,
+      }),
+      clearGetPermissionsError: assign({
+        checkPermissionsError: (_) => undefined,
+      }),
      assignBuild: (_, event) =>
        assign({
          build: event.data,
@@ -489,14 +553,23 @@ export const workspaceMachine = createMachine(
        if (context.workspace) {
          return await API.getWorkspaceBuilds(context.workspace.id)
        } else {
-          throw Error("Cannot refresh workspace without id")
+          throw Error("Cannot get builds without id")
        }
      },
      loadMoreBuilds: async (context) => {
        if (context.workspace) {
          return await API.getWorkspaceBuilds(context.workspace.id)
        } else {
-          throw Error("Cannot refresh workspace without id")
+          throw Error("Cannot load more builds without id")
+        }
+      },
+      checkPermissions: async (context) => {
+        if (context.workspace && context.userId) {
+          return await API.checkUserPermissions(context.userId, {
+            checks: permissionsToCheck(context.workspace),
+          })
+        } else {
+          throw Error("Cannot check permissions without both workspace and user id")
        }
      },
    },