fix: /workspaces should work even if missing template perms (#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.
This commit is contained in:
Steven Masley 2023-08-17 13:22:03 -05:00 committed by GitHub
parent e39402f1c9
commit 8910f05172
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 87 additions and 3 deletions

View File

@ -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(

View File

@ -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()