Skip to content

fix: ensure /workspaces functions correctly even if missing template permissions #9152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 17, 2023
Merged

fix: ensure /workspaces functions correctly even if missing template permissions #9152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions coderd/workspaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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(
Expand Down
55 changes: 55 additions & 0 deletions enterprise/coderd/workspaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down