Skip to content

Commit 8910f05

Browse files
EmyrkEmyrk
authored
fix: /workspaces should work even if missing template perms (coder#9152)
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.
1 parent e39402f commit 8910f05

2 files changed

Lines changed: 87 additions & 3 deletions

File tree

‎coderd/workspaces.go‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
8686
return
8787
}
8888

89+
if len(data.templates) == 0 {
90+
httpapi.Forbidden(rw)
91+
return
92+
}
93+
8994
httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace(
9095
workspace,
9196
data.builds[0],
@@ -815,6 +820,11 @@ func (api *API) putWorkspaceLock(rw http.ResponseWriter, r *http.Request) {
815820
return
816821
}
817822

823+
if len(data.templates) == 0 {
824+
httpapi.Forbidden(rw)
825+
return
826+
}
827+
818828
httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace(
819829
workspace,
820830
data.builds[0],
@@ -964,6 +974,16 @@ func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) {
964974
})
965975
return
966976
}
977+
if len(data.templates) == 0 {
978+
_ = sendEvent(ctx, codersdk.ServerSentEvent{
979+
Type: codersdk.ServerSentEventTypeError,
980+
Data: codersdk.Response{
981+
Message: "Forbidden reading template of selected workspace.",
982+
Detail: err.Error(),
983+
},
984+
})
985+
return
986+
}
967987

968988
_ = sendEvent(ctx, codersdk.ServerSentEvent{
969989
Type: codersdk.ServerSentEventTypeData,
@@ -1025,6 +1045,10 @@ type workspaceData struct {
10251045
users []database.User
10261046
}
10271047

1048+
// workspacesData only returns the data the caller can access. If the caller
1049+
// does not have the correct perms to read a given template, the template will
1050+
// not be returned.
1051+
// So the caller must check the templates & users exist before using them.
10281052
func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspace) (workspaceData, error) {
10291053
workspaceIDs := make([]uuid.UUID, 0, len(workspaces))
10301054
templateIDs := make([]uuid.UUID, 0, len(workspaces))
@@ -1090,17 +1114,22 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c
10901114

10911115
apiWorkspaces := make([]codersdk.Workspace, 0, len(workspaces))
10921116
for _, workspace := range workspaces {
1117+
// If any data is missing from the workspace, just skip returning
1118+
// this workspace. This is not ideal, but the user cannot read
1119+
// all the workspace's data, so do not show them.
1120+
// Ideally we could just return some sort of "unknown" for the missing
1121+
// fields?
10931122
build, exists := buildByWorkspaceID[workspace.ID]
10941123
if !exists {
1095-
return nil, xerrors.Errorf("build not found for workspace %q", workspace.Name)
1124+
continue
10961125
}
10971126
template, exists := templateByID[workspace.TemplateID]
10981127
if !exists {
1099-
return nil, xerrors.Errorf("template not found for workspace %q", workspace.Name)
1128+
continue
11001129
}
11011130
owner, exists := userByID[workspace.OwnerID]
11021131
if !exists {
1103-
return nil, xerrors.Errorf("owner not found for workspace: %q", workspace.Name)
1132+
continue
11041133
}
11051134

11061135
apiWorkspaces = append(apiWorkspaces, convertWorkspace(

‎enterprise/coderd/workspaces_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,61 @@ func TestWorkspacesFiltering(t *testing.T) {
688688
})
689689
}
690690

691+
// TestWorkspacesWithoutTemplatePerms creates a workspace for a user, then drops
692+
// the user's perms to the underlying template.
693+
func TestWorkspacesWithoutTemplatePerms(t *testing.T) {
694+
t.Parallel()
695+
696+
client, first := coderdenttest.New(t, &coderdenttest.Options{
697+
Options: &coderdtest.Options{
698+
IncludeProvisionerDaemon: true,
699+
},
700+
LicenseOptions: &coderdenttest.LicenseOptions{
701+
Features: license.Features{
702+
codersdk.FeatureTemplateRBAC: 1,
703+
},
704+
},
705+
})
706+
707+
version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil)
708+
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
709+
template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID)
710+
711+
user, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID)
712+
workspace := coderdtest.CreateWorkspace(t, user, first.OrganizationID, template.ID)
713+
714+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
715+
defer cancel()
716+
717+
// Remove everyone access
718+
err := client.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{
719+
GroupPerms: map[string]codersdk.TemplateRole{
720+
first.OrganizationID.String(): codersdk.TemplateRoleDeleted,
721+
},
722+
})
723+
require.NoError(t, err, "remove everyone access")
724+
725+
// This should fail as the user cannot read the template
726+
_, err = user.Workspace(ctx, workspace.ID)
727+
require.Error(t, err, "fetch workspace")
728+
var sdkError *codersdk.Error
729+
require.ErrorAs(t, err, &sdkError)
730+
require.Equal(t, http.StatusForbidden, sdkError.StatusCode())
731+
732+
_, err = user.Workspaces(ctx, codersdk.WorkspaceFilter{})
733+
require.NoError(t, err, "fetch workspaces should not fail")
734+
735+
// Now create another workspace the user can read.
736+
version2 := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil)
737+
coderdtest.AwaitTemplateVersionJob(t, client, version2.ID)
738+
template2 := coderdtest.CreateTemplate(t, client, first.OrganizationID, version2.ID)
739+
_ = coderdtest.CreateWorkspace(t, user, first.OrganizationID, template2.ID)
740+
741+
workspaces, err := user.Workspaces(ctx, codersdk.WorkspaceFilter{})
742+
require.NoError(t, err, "fetch workspaces should not fail")
743+
require.Len(t, workspaces.Workspaces, 1)
744+
}
745+
691746
func TestWorkspaceLock(t *testing.T) {
692747
t.Parallel()
693748

0 commit comments

Comments
 (0)