From a4b2e8d97c91118ba961b9fc849360b06c7fbc9d Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Mon, 23 Jun 2025 15:36:14 +0000 Subject: [PATCH 1/5] chore: reorder prebuilt workspace authorization logic --- coderd/database/dbauthz/dbauthz.go | 26 +++++++++++++------------ coderd/database/dbauthz/dbauthz_test.go | 24 +++++++++++++++++++++-- coderd/workspacebuilds.go | 13 ++++++------- coderd/wsbuilder/wsbuilder.go | 12 +++++------- 4 files changed, 47 insertions(+), 28 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index adb2007918f8d..8e2fd99829206 100644 --- 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: %w", errors.Join(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", errors.Join(workspaceErr)) } type authContextKey struct{} diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 0ccd867040116..3ee13d762e2ff 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -5590,7 +5590,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{}) @@ -5619,6 +5629,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/workspacebuilds.go b/coderd/workspacebuilds.go index 15614f84b4f70..b29d097cf5ec7 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -393,17 +393,16 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { ctx, tx, 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 workspaceObj, ok := object.(database.Workspace); 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 index a996d1594a50d..ffcad9e04d0fc 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -963,14 +963,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 { From 471d3e82d3fa917f0677849e0779e76b48e62cab Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Mon, 23 Jun 2025 17:13:23 +0000 Subject: [PATCH 2/5] chore: update error message --- coderd/database/dbauthz/dbauthz.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 8e2fd99829206..9d3e6882a3119 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -169,7 +169,7 @@ func (q *querier) authorizePrebuiltWorkspace(ctx context.Context, action policy. if prebuiltErr = q.authorizeContext(ctx, action, workspace.AsPrebuild()); prebuiltErr == nil { return nil } - return xerrors.Errorf("authorize context: %w", errors.Join(workspaceErr, prebuiltErr)) + return xerrors.Errorf("authorize context as prebuild: %w", errors.Join(workspaceErr, prebuiltErr)) } return xerrors.Errorf("authorize context: %w", errors.Join(workspaceErr)) From 1f0e33a6491cc70d7ff743a60e5cc19d69f16bc6 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 24 Jun 2025 09:12:28 +0000 Subject: [PATCH 3/5] chore: improve error message for failed workspace and prebuilt authorization --- coderd/database/dbauthz/dbauthz.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9d3e6882a3119..dfdaeb6cb2ab1 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -169,10 +169,10 @@ func (q *querier) authorizePrebuiltWorkspace(ctx context.Context, action policy. if prebuiltErr = q.authorizeContext(ctx, action, workspace.AsPrebuild()); prebuiltErr == nil { return nil } - return xerrors.Errorf("authorize context as prebuild: %w", errors.Join(workspaceErr, prebuiltErr)) + return xerrors.Errorf("authorize context failed for workspace (%v) and prebuilt (%w)", workspaceErr, prebuiltErr) } - return xerrors.Errorf("authorize context: %w", errors.Join(workspaceErr)) + return xerrors.Errorf("authorize context: %w", workspaceErr) } type authContextKey struct{} From 2c387ce20ef90be5fda727a368ac1dbea628491c Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 24 Jun 2025 09:23:38 +0000 Subject: [PATCH 4/5] refactor: simplify workspace type assertion --- coderd/workspacebuilds.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index b29d097cf5ec7..799ed5b38b2c4 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -397,8 +397,11 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { 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 && workspaceObj.IsPrebuild() { + if action == policy.ActionDelete { + if workspaceObj, ok := object.(interface { + IsPrebuild() bool + AsPrebuild() rbac.Object + }); ok && workspaceObj.IsPrebuild() { return api.Authorize(r, action, workspaceObj.AsPrebuild()) } } From f3f01830e760829f3cd3d1559dc5cb1fca6fe1fc Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 24 Jun 2025 11:33:31 +0000 Subject: [PATCH 5/5] refactor: extract PrebuiltWorkspaceResource interface for prebuilt RBAC support --- coderd/database/modelmethods.go | 7 +++++++ coderd/workspacebuilds.go | 5 +---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index cb16d8c4995b6..725e45c268d72 100644 --- 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 index d1488b85b1f3f..c3c4aba0c378c 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -396,10 +396,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { } // Special handling for prebuilt workspace deletion if action == policy.ActionDelete { - if workspaceObj, ok := object.(interface { - IsPrebuild() bool - AsPrebuild() rbac.Object - }); ok && workspaceObj.IsPrebuild() { + if workspaceObj, ok := object.(database.PrebuiltWorkspaceResource); ok && workspaceObj.IsPrebuild() { return api.Authorize(r, action, workspaceObj.AsPrebuild()) } }