fix: /workspaces should work even if missing template perms (#9152)

Emyrk · web-flow · commit 8910f05172f9 · 2023-08-17T13:22:03.000-05:00
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.
diff --git 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
@@ -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()
 
