diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 4235ca99d2..f1309b3e25 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -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 diff --git a/coderd/wsrelateddata_internal_test.go b/coderd/wsrelateddata_internal_test.go index 1ef8c555d3..4e7087e160 100644 --- a/coderd/wsrelateddata_internal_test.go +++ b/coderd/wsrelateddata_internal_test.go @@ -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) + }) +}