chore: reorder prebuilt workspace authorization logic (#18506)

ssncferreira · web-flow · commit f44969b6898b · 2025-06-24T16:33:21.000+01:00
## Description Follow-up from PR #18333 Related with: #18333 (comment) This changes the authorization logic to first try the normal workspace authorization check, and only if the resource is a prebuilt workspace, fall back to the prebuilt workspace authorization check. Since prebuilt workspaces are a subset of workspaces, the normal workspace check is more likely to succeed. This is a small optimization to reduce unnecessary prebuilt authorization calls.
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -151,26 +151,28 @@ func (q *querier) authorizeContext(ctx context.Context, action policy.Action, ob
 
 // authorizePrebuiltWorkspace handles authorization for workspace resource types.
 // prebuilt_workspaces are a subset of workspaces, currently limited to
-// supporting delete operations. Therefore, if the action is delete or
-// update and the workspace is a prebuild, a prebuilt-specific authorization
-// is attempted first. If that fails, it falls back to normal workspace
-// authorization.
+// supporting delete operations. This function first attempts normal workspace
+// authorization. If that fails, the action is delete or update and the workspace
+// is a prebuild, a prebuilt-specific authorization is attempted.
 // Note: Delete operations of workspaces requires both update and delete
 // permissions.
 func (q *querier) authorizePrebuiltWorkspace(ctx context.Context, action policy.Action, workspace database.Workspace) error {
-	var prebuiltErr error
-	// Special handling for prebuilt_workspace deletion authorization check
+	// Try default workspace authorization first
+	var workspaceErr error
+	if workspaceErr = q.authorizeContext(ctx, action, workspace); workspaceErr == nil {
+		return nil
+	}
+
+	// Special handling for prebuilt workspace deletion
 	if (action == policy.ActionUpdate || action == policy.ActionDelete) && workspace.IsPrebuild() {
-		// Try prebuilt-specific authorization first
+		var prebuiltErr error
 		if prebuiltErr = q.authorizeContext(ctx, action, workspace.AsPrebuild()); prebuiltErr == nil {
 			return nil
 		}
+		return xerrors.Errorf("authorize context failed for workspace (%v) and prebuilt (%w)", workspaceErr, prebuiltErr)
 	}
-	// Fallback to normal workspace authorization check
-	if err := q.authorizeContext(ctx, action, workspace); err != nil {
-		return xerrors.Errorf("authorize context: %w", errors.Join(prebuiltErr, err))
-	}
-	return nil
+
+	return xerrors.Errorf("authorize context: %w", workspaceErr)
 }
 
 type authContextKey struct{}
diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -5650,7 +5650,17 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() {
 			Reason:            database.BuildReasonInitiator,
 			TemplateVersionID: tv.ID,
 			JobID:             pj.ID,
-		}).Asserts(w.AsPrebuild(), policy.ActionDelete)
+		}).
+			// Simulate a fallback authorization flow:
+			// - First, the default workspace authorization fails (simulated by returning an error).
+			// - Then, authorization is retried using the prebuilt workspace object, which succeeds.
+			// The test asserts that both authorization attempts occur in the correct order.
+			WithSuccessAuthorizer(func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error {
+				if obj.Type == rbac.ResourceWorkspace.Type {
+					return xerrors.Errorf("not authorized for workspace type")
+				}
+				return nil
+			}).Asserts(w, policy.ActionDelete, w.AsPrebuild(), policy.ActionDelete)
 	}))
 	s.Run("PrebuildUpdate/InsertWorkspaceBuildParameters", s.Subtest(func(db database.Store, check *expects) {
 		u := dbgen.User(s.T(), db, database.User{})
@@ -5679,6 +5689,16 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() {
 		})
 		check.Args(database.InsertWorkspaceBuildParametersParams{
 			WorkspaceBuildID: wb.ID,
-		}).Asserts(w.AsPrebuild(), policy.ActionUpdate)
+		}).
+			// Simulate a fallback authorization flow:
+			// - First, the default workspace authorization fails (simulated by returning an error).
+			// - Then, authorization is retried using the prebuilt workspace object, which succeeds.
+			// The test asserts that both authorization attempts occur in the correct order.
+			WithSuccessAuthorizer(func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error {
+				if obj.Type == rbac.ResourceWorkspace.Type {
+					return xerrors.Errorf("not authorized for workspace type")
+				}
+				return nil
+			}).Asserts(w, policy.ActionUpdate, w.AsPrebuild(), policy.ActionUpdate)
 	}))
 }
diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go
@@ -199,6 +199,13 @@ func (gm GroupMember) RBACObject() rbac.Object {
 	return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String())
 }
 
+// PrebuiltWorkspaceResource defines the interface for types that can be identified as prebuilt workspaces
+// and converted to their corresponding prebuilt workspace RBAC object.
+type PrebuiltWorkspaceResource interface {
+	IsPrebuild() bool
+	AsPrebuild() rbac.Object
+}
+
 // WorkspaceTable converts a Workspace to it's reduced version.
 // A more generalized solution is to use json marshaling to
 // consistently keep these two structs in sync.
diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go
@@ -391,17 +391,16 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
 			tx,
 			api.FileCache,
 			func(action policy.Action, object rbac.Objecter) bool {
+				if auth := api.Authorize(r, action, object); auth {
+					return true
+				}
 				// Special handling for prebuilt workspace deletion
-				if object.RBACObject().Type == rbac.ResourceWorkspace.Type && action == policy.ActionDelete {
-					if workspaceObj, ok := object.(database.Workspace); ok {
-						// Try prebuilt-specific authorization first
-						if auth := api.Authorize(r, action, workspaceObj.AsPrebuild()); auth {
-							return auth
-						}
+				if action == policy.ActionDelete {
+					if workspaceObj, ok := object.(database.PrebuiltWorkspaceResource); ok && workspaceObj.IsPrebuild() {
+						return api.Authorize(r, action, workspaceObj.AsPrebuild())
 					}
 				}
-				// Fallback to default authorization
-				return api.Authorize(r, action, object)
+				return false
 			},
 			audit.WorkspaceBuildBaggageFromRequest(r),
 		)
diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go
@@ -1049,14 +1049,12 @@ func (b *Builder) authorize(authFunc func(action policy.Action, object rbac.Obje
 		return BuildError{http.StatusBadRequest, msg, xerrors.New(msg)}
 	}
 
+	// Try default workspace authorization first
+	authorized := authFunc(action, b.workspace)
+
 	// Special handling for prebuilt workspace deletion
-	authorized := false
-	if action == policy.ActionDelete && b.workspace.IsPrebuild() && authFunc(action, b.workspace.AsPrebuild()) {
-		authorized = true
-	}
-	// Fallback to default authorization
-	if !authorized && authFunc(action, b.workspace) {
-		authorized = true
+	if !authorized && action == policy.ActionDelete && b.workspace.IsPrebuild() {
+		authorized = authFunc(action, b.workspace.AsPrebuild())
 	}
 
 	if !authorized {
