refactor(coderd): enforce workspace template authz in singleWorkspaceData (#29665)

Part of the lite `codersdk.Workspace` work ([GRU-82](https://linear.app/codercom/issue/GRU-82), [RFC](https://app.notion.com/p/coderhq/Lite-Workspace-API-Requests-3aad579be5928060b5e7cc65c539717f)). `api.workspaceData` omits templates the actor cannot read and leaves each caller to check `len(data.templates) == 0` and fail the request itself. Now that `workspaceData` receives the `wsrelated.Config`, it knows whether the template was requested, so the authorization check can live closer to the data. This adds `singleWorkspaceData`, a thin wrapper for single-workspace fetches that returns `errWorkspaceTemplateUnauthorized` when the template was requested but is absent (i.e. unreadable). The five single-workspace callers route through it and map that error to the response their endpoint requires: - get workspace by ID → `403` - get workspace by owner/name → `404` (preserves existence concealment) - create/put workspace → `403` - watch workspace (SSE) → forbidden error event The batch `workspaceData` keeps its RBAC-filtering behavior so the **list** endpoint can still skip workspaces whose template the actor cannot read, rather than failing the whole request. Field selection (`Config`) and authorization policy are kept separate: the strict check is a property of the single-workspace wrapper, not of `All()`. <details> <summary>Design note</summary> The check tests membership by the workspace's `TemplateID` rather than only `len(templates) == 0`, and only real query failures (not RBAC omissions) propagate as generic errors, so a genuine DB error is never mistaken for an authorization denial. </details> ### Testing - New `TestSingleWorkspaceData` (mocked `database.Store`): template readable, template requested-but-unauthorized (sentinel), template not requested (no error), and a real query error (not the sentinel). - Existing `TestWorkspace`, `TestWorkspaceByOwnerAndName`, `TestWorkspaceWatcher`, and `TestWorkspaceIncludeRelated` pass unchanged. --- *Generated by Coder Agents on behalf of @spikecurtis.*
2026-10-16 14:48:56 +08:00 · 2026-09-21 21:06:06 +02:00
parent af0ef00f91
commit 8475a1defb
2 changed files with 118 additions and 30 deletions
@@ -114,8 +114,13 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
 		}
 	}

-	data, err := api.workspaceData(ctx, []database.Workspace{workspace}, related)
+	data, err := api.singleWorkspaceData(ctx, workspace, related)
 	if err != nil {
+		// The template was requested but the actor cannot read it.
+		if errors.Is(err, errWorkspaceTemplateUnauthorized) {
+			httpapi.Forbidden(rw)
+			return
+		}
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error fetching workspace resources.",
 			Detail:  err.Error(),
@@ -123,14 +128,6 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
 		return
 	}

-	// workspaceData omits templates the requester cannot read. If the template
-	// was requested but is absent, the requester is not authorized to read it.
-	// When the template was not requested there is nothing to authorize here.
-	if related.Template && len(data.templates) == 0 {
-		httpapi.Forbidden(rw)
-		return
-	}
-
 	appStatus := codersdk.WorkspaceAppStatus{}
 	if len(data.appStatuses) > 0 {
 		appStatus = data.appStatuses[0]
@@ -344,8 +341,14 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request)
 		return
 	}

-	data, err := api.workspaceData(ctx, []database.Workspace{workspace}, wsrelated.All())
+	data, err := api.singleWorkspaceData(ctx, workspace, wsrelated.All())
 	if err != nil {
+		// Preserve concealment: a template the actor cannot read is reported as
+		// not found rather than forbidden.
+		if errors.Is(err, errWorkspaceTemplateUnauthorized) {
+			httpapi.ResourceNotFound(rw)
+			return
+		}
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error fetching workspace resources.",
 			Detail:  err.Error(),
@@ -353,7 +356,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request)
 		return
 	}

-	if len(data.builds) == 0 || len(data.templates) == 0 {
+	if len(data.builds) == 0 {
 		httpapi.ResourceNotFound(rw)
 		return
 	}
@@ -1607,8 +1610,15 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) {
 		return
 	}

-	data, err := api.workspaceData(ctx, []database.Workspace{workspace}, wsrelated.All())
+	data, err := api.singleWorkspaceData(ctx, workspace, wsrelated.All())
 	if err != nil {
+		// TODO: This is a strange error since it occurs after the mutation.
+		// An example of why we should join in fields to prevent this forbidden
+		// error from being sent, when the action did succeed.
+		if errors.Is(err, errWorkspaceTemplateUnauthorized) {
+			httpapi.Forbidden(rw)
+			return
+		}
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error fetching workspace resources.",
 			Detail:  err.Error(),
@@ -1616,14 +1626,6 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) {
 		return
 	}

-	// TODO: This is a strange error since it occurs after the mutation.
-	// An example of why we should join in fields to prevent this forbidden error
-	// from being sent, when the action did succeed.
-	if len(data.templates) == 0 {
-		httpapi.Forbidden(rw)
-		return
-	}
-
 	aReq.New = newWorkspace

 	appStatus := codersdk.WorkspaceAppStatus{}
@@ -2179,8 +2181,17 @@ func (api *API) watchWorkspace(
 			return
 		}

-		data, err := api.workspaceData(ctx, []database.Workspace{workspace}, wsrelated.All())
+		data, err := api.singleWorkspaceData(ctx, workspace, wsrelated.All())
 		if err != nil {
+			if errors.Is(err, errWorkspaceTemplateUnauthorized) {
+				_ = sendEvent(codersdk.ServerSentEvent{
+					Type: codersdk.ServerSentEventTypeError,
+					Data: codersdk.Response{
+						Message: "Forbidden reading template of selected workspace.",
+					},
+				})
+				return
+			}
 			_ = sendEvent(codersdk.ServerSentEvent{
 				Type: codersdk.ServerSentEventTypeError,
 				Data: codersdk.Response{
@@ -2190,15 +2201,6 @@ func (api *API) watchWorkspace(
 			})
 			return
 		}
-		if len(data.templates) == 0 {
-			_ = sendEvent(codersdk.ServerSentEvent{
-				Type: codersdk.ServerSentEventTypeError,
-				Data: codersdk.Response{
-					Message: "Forbidden reading template of selected workspace.",
-				},
-			})
-			return
-		}

 		appStatus := codersdk.WorkspaceAppStatus{}
 		if len(data.appStatuses) > 0 {
@@ -2807,6 +2809,30 @@ func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspa
 	}, nil
 }

+// errWorkspaceTemplateUnauthorized indicates the actor requested a workspace's
+// template but is not authorized to read it. workspaceData omits templates the
+// actor cannot read, so for a single-workspace fetch an absent-but-requested
+// template means the read was denied.
+var errWorkspaceTemplateUnauthorized = xerrors.New("not authorized to read workspace template")
+
+// singleWorkspaceData loads related data for one workspace. Unlike the batch
+// workspaceData, which omits templates the actor cannot read so list endpoints
+// can skip them, it returns errWorkspaceTemplateUnauthorized when the template
+// was requested but is absent. Callers map that error to the response their
+// endpoint requires (403, 404, or an SSE error).
+func (api *API) singleWorkspaceData(ctx context.Context, workspace database.Workspace, cfg wsrelated.Config) (workspaceData, error) {
+	data, err := api.workspaceData(ctx, []database.Workspace{workspace}, cfg)
+	if err != nil {
+		return workspaceData{}, err
+	}
+	if cfg.Template && !slices.ContainsFunc(data.templates, func(t database.Template) bool {
+		return t.ID == workspace.TemplateID
+	}) {
+		return workspaceData{}, errWorkspaceTemplateUnauthorized
+	}
+	return data, nil
+}
+
 // attachAgentMetadata maps the agent metadata the workspaces query
 // aggregated per workspace onto the agents in the converted response.
 // Each aggregated datum carries its workspace_agent_id. An unparsable
@@ -6,6 +6,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
+	"golang.org/x/xerrors"

 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbmock"
@@ -270,3 +271,64 @@ func TestWorkspaceDataQueryGating(t *testing.T) {
 		})
 	}
 }
+
+// TestSingleWorkspaceData asserts the strict template-authorization behavior
+// layered on top of workspaceData: a requested-but-absent template is reported
+// as errWorkspaceTemplateUnauthorized, while an omitted template or a real
+// query failure are handled distinctly.
+func TestSingleWorkspaceData(t *testing.T) {
+	t.Parallel()
+
+	workspace := database.Workspace{ID: uuid.New(), TemplateID: uuid.New()}
+
+	t.Run("TemplateReadable", func(t *testing.T) {
+		t.Parallel()
+		ctx := testutil.Context(t, testutil.WaitShort)
+		db := dbmock.NewMockStore(gomock.NewController(t))
+		db.EXPECT().GetTemplatesWithFilter(gomock.Any(), gomock.Any()).
+			Return([]database.Template{{ID: workspace.TemplateID}}, nil)
+
+		api := &API{Options: &Options{Database: db}}
+		data, err := api.singleWorkspaceData(ctx, workspace, wsrelated.Config{Template: true})
+		require.NoError(t, err)
+		require.Len(t, data.templates, 1)
+	})
+
+	t.Run("TemplateUnauthorized", func(t *testing.T) {
+		t.Parallel()
+		ctx := testutil.Context(t, testutil.WaitShort)
+		db := dbmock.NewMockStore(gomock.NewController(t))
+		// The actor cannot read the template, so the query omits it.
+		db.EXPECT().GetTemplatesWithFilter(gomock.Any(), gomock.Any()).
+			Return([]database.Template{}, nil)
+
+		api := &API{Options: &Options{Database: db}}
+		_, err := api.singleWorkspaceData(ctx, workspace, wsrelated.Config{Template: true})
+		require.ErrorIs(t, err, errWorkspaceTemplateUnauthorized)
+	})
+
+	t.Run("TemplateNotRequested", func(t *testing.T) {
+		t.Parallel()
+		ctx := testutil.Context(t, testutil.WaitShort)
+		// No template query is expected, and an absent template is not an error.
+		db := dbmock.NewMockStore(gomock.NewController(t))
+
+		api := &API{Options: &Options{Database: db}}
+		_, err := api.singleWorkspaceData(ctx, workspace, wsrelated.Config{})
+		require.NoError(t, err)
+	})
+
+	t.Run("QueryError", func(t *testing.T) {
+		t.Parallel()
+		ctx := testutil.Context(t, testutil.WaitShort)
+		db := dbmock.NewMockStore(gomock.NewController(t))
+		db.EXPECT().GetTemplatesWithFilter(gomock.Any(), gomock.Any()).
+			Return(nil, xerrors.New("boom"))
+
+		api := &API{Options: &Options{Database: db}}
+		_, err := api.singleWorkspaceData(ctx, workspace, wsrelated.Config{Template: true})
+		require.Error(t, err)
+		// A real query failure must not be mistaken for an authorization denial.
+		require.NotErrorIs(t, err, errWorkspaceTemplateUnauthorized)
+	})
+}