From 7a373697a7cb8e830266d49c557d464158a851e8 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 17 Aug 2023 09:55:52 -0500 Subject: [PATCH] fix: /workspaces should work even if missing template perms If a user is missing template perms to a workspace, just block reading that workspace. This is to keep the api consistent, it is not a rbac enforcement. This should ublock users reporting this bug that /workspaces returns nothing when 1 workspace cannot be fully read. We might want to be able to return missing or unknown fields in our api to account for this. --- coderd/workspaces.go | 35 ++++++++++++++++-- enterprise/coderd/workspaces_test.go | 55 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 3 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 8ad02faff33fb..b71e630be6a42 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -86,6 +86,11 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { return } + if len(data.templates) == 0 { + httpapi.Forbidden(rw) + return + } + httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace( workspace, data.builds[0], @@ -815,6 +820,11 @@ func (api *API) putWorkspaceLock(rw http.ResponseWriter, r *http.Request) { return } + if len(data.templates) == 0 { + httpapi.Forbidden(rw) + return + } + httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace( workspace, data.builds[0], @@ -964,6 +974,16 @@ func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) { }) return } + if len(data.templates) == 0 { + _ = sendEvent(ctx, codersdk.ServerSentEvent{ + Type: codersdk.ServerSentEventTypeError, + Data: codersdk.Response{ + Message: "Forbidden reading template of selected workspace.", + Detail: err.Error(), + }, + }) + return + } _ = sendEvent(ctx, codersdk.ServerSentEvent{ Type: codersdk.ServerSentEventTypeData, @@ -1025,6 +1045,10 @@ type workspaceData struct { users []database.User } +// workspacesData only returns the data the caller can access. If the caller +// does not have the correct perms to read a given template, the template will +// not be returned. +// So the caller must check the templates & users exist before using them. func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspace) (workspaceData, error) { workspaceIDs := make([]uuid.UUID, 0, len(workspaces)) templateIDs := make([]uuid.UUID, 0, len(workspaces)) @@ -1090,17 +1114,22 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c apiWorkspaces := make([]codersdk.Workspace, 0, len(workspaces)) for _, workspace := range workspaces { + // If any data is missing from the workspace, just skip returning + // this workspace. This is not ideal, but the user cannot read + // all the workspace's data, so do not show them. + // Ideally we could just return some sort of "unknown" for the missing + // fields? build, exists := buildByWorkspaceID[workspace.ID] if !exists { - return nil, xerrors.Errorf("build not found for workspace %q", workspace.Name) + continue } template, exists := templateByID[workspace.TemplateID] if !exists { - return nil, xerrors.Errorf("template not found for workspace %q", workspace.Name) + continue } owner, exists := userByID[workspace.OwnerID] if !exists { - return nil, xerrors.Errorf("owner not found for workspace: %q", workspace.Name) + continue } apiWorkspaces = append(apiWorkspaces, convertWorkspace( diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 10140bd747957..a5acba7618b64 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -688,6 +688,61 @@ func TestWorkspacesFiltering(t *testing.T) { }) } +// TestWorkspacesWithoutTemplatePerms creates a workspace for a user, then drops +// the user's perms to the underlying template. +func TestWorkspacesWithoutTemplatePerms(t *testing.T) { + t.Parallel() + + client, first := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + + user, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + workspace := coderdtest.CreateWorkspace(t, user, first.OrganizationID, template.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Remove everyone access + err := client.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{ + GroupPerms: map[string]codersdk.TemplateRole{ + first.OrganizationID.String(): codersdk.TemplateRoleDeleted, + }, + }) + require.NoError(t, err, "remove everyone access") + + // This should fail as the user cannot read the template + _, err = user.Workspace(ctx, workspace.ID) + require.Error(t, err, "fetch workspace") + var sdkError *codersdk.Error + require.ErrorAs(t, err, &sdkError) + require.Equal(t, http.StatusForbidden, sdkError.StatusCode()) + + _, err = user.Workspaces(ctx, codersdk.WorkspaceFilter{}) + require.NoError(t, err, "fetch workspaces should not fail") + + // Now create another workspace the user can read. + version2 := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version2.ID) + template2 := coderdtest.CreateTemplate(t, client, first.OrganizationID, version2.ID) + _ = coderdtest.CreateWorkspace(t, user, first.OrganizationID, template2.ID) + + workspaces, err := user.Workspaces(ctx, codersdk.WorkspaceFilter{}) + require.NoError(t, err, "fetch workspaces should not fail") + require.Len(t, workspaces.Workspaces, 1) +} + func TestWorkspaceLock(t *testing.T) { t.Parallel()