From d29a1687856f0874297657e0dd23513b062adad1 Mon Sep 17 00:00:00 2001 From: George K Date: Thu, 22 Jan 2026 08:12:15 -0800 Subject: [PATCH] fix(coderd/rbac): reinstate deployment-wide workspace.share permission for owner role (#21620) The removal of that permission from the role broke valid use cases (e.g. a site owner user creating a workspace owned by a system account and then trying to share it with another user). The bulk of the PR is made up of the rollbacks of the previously introduced test updates necessitated by the removal. Related to: https://github.com/coder/internal/issues/1285 --- cli/sharing_test.go | 6 ++-- coderd/database/dbgen/dbgen.go | 12 ++----- coderd/rbac/authz_internal_test.go | 25 +++++++------- coderd/rbac/roles.go | 2 +- coderd/rbac/roles_test.go | 4 +-- enterprise/cli/sharing_test.go | 6 ++-- enterprise/coderd/workspaces_test.go | 40 +++++++++++----------- enterprise/coderd/workspacesharing_test.go | 4 ++- 8 files changed, 46 insertions(+), 53 deletions(-) diff --git a/cli/sharing_test.go b/cli/sharing_test.go index 90df1fa992..19e1853470 100644 --- a/cli/sharing_test.go +++ b/cli/sharing_test.go @@ -197,7 +197,7 @@ func TestSharingStatus(t *testing.T) { ctx = testutil.Context(t, testutil.WaitMedium) ) - err := workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ toShareWithUser.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -248,7 +248,7 @@ func TestSharingRemove(t *testing.T) { ctx := testutil.Context(t, testutil.WaitMedium) // Share the workspace with a user to later remove - err := workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ toShareWithUser.ID.String(): codersdk.WorkspaceRoleUse, toRemoveUser.ID.String(): codersdk.WorkspaceRoleUse, @@ -309,7 +309,7 @@ func TestSharingRemove(t *testing.T) { ctx := testutil.Context(t, testutil.WaitMedium) // Share the workspace with a user to later remove - err := workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ toRemoveUser2.ID.String(): codersdk.WorkspaceRoleUse, toRemoveUser1.ID.String(): codersdk.WorkspaceRoleUse, diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index f8f836b25f..b67e3d9390 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -42,16 +42,8 @@ import ( // genCtx is to give all generator functions permission if the db is a dbauthz db. var genCtx = dbauthz.As(context.Background(), rbac.Subject{ - ID: "owner", - Roles: rbac.Roles(append( - must(rbac.RoleIdentifiers{rbac.RoleOwner()}.Expand()), - rbac.Role{ - Identifier: rbac.RoleIdentifier{Name: "dbgen-workspace-sharer"}, - Site: rbac.Permissions(map[string][]policy.Action{ - rbac.ResourceWorkspace.Type: {policy.ActionShare}, - }), - }, - )), + ID: "owner", + Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.RoleOwner()}.Expand())), Groups: []string{}, Scope: rbac.ExpandableScope(rbac.ScopeAll), }) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 8c48e7acd6..853fed8359 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -496,33 +496,32 @@ func TestAuthorizeDomain(t *testing.T) { }, } - siteAdminWorkspaceActions := slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionShare) testAuthorize(t, "SiteAdmin", user, []authTestCase{ // Similar to an orphaned user, but has site level perms {resource: ResourceTemplate.AnyOrganization(), actions: []policy.Action{policy.ActionCreate}, allow: true}, // Org + me - {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: siteAdminWorkspaceActions, allow: true}, - {resource: ResourceWorkspace.InOrg(defOrg), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: ResourceWorkspace.AvailableActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: ResourceWorkspace.AvailableActions(), allow: true}, - {resource: ResourceWorkspace.WithOwner(user.ID), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.WithOwner(user.ID), actions: ResourceWorkspace.AvailableActions(), allow: true}, - {resource: ResourceWorkspace.All(), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.All(), actions: ResourceWorkspace.AvailableActions(), allow: true}, // Other org + me - {resource: ResourceWorkspace.InOrg(unusedID).WithOwner(user.ID), actions: siteAdminWorkspaceActions, allow: true}, - {resource: ResourceWorkspace.InOrg(unusedID), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner(user.ID), actions: ResourceWorkspace.AvailableActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(unusedID), actions: ResourceWorkspace.AvailableActions(), allow: true}, // Other org + other user - {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: ResourceWorkspace.AvailableActions(), allow: true}, - {resource: ResourceWorkspace.WithOwner("not-me"), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.WithOwner("not-me"), actions: ResourceWorkspace.AvailableActions(), allow: true}, // Other org + other use - {resource: ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), actions: siteAdminWorkspaceActions, allow: true}, - {resource: ResourceWorkspace.InOrg(unusedID), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), actions: ResourceWorkspace.AvailableActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(unusedID), actions: ResourceWorkspace.AvailableActions(), allow: true}, - {resource: ResourceWorkspace.WithOwner("not-me"), actions: siteAdminWorkspaceActions, allow: true}, + {resource: ResourceWorkspace.WithOwner("not-me"), actions: ResourceWorkspace.AvailableActions(), allow: true}, }) user = Subject{ @@ -757,7 +756,7 @@ func TestAuthorizeLevels(t *testing.T) { testAuthorize(t, "AdminAlwaysAllow", user, cases(func(c authTestCase) authTestCase { - c.actions = slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionShare) + c.actions = ResourceWorkspace.AvailableActions() c.allow = true return c }, []authTestCase{ diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 4c3376decc..c0094c7ecd 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -267,7 +267,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { opts = &RoleOptions{} } - ownerWorkspaceActions := slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionShare) + ownerWorkspaceActions := ResourceWorkspace.AvailableActions() if opts.NoOwnerWorkspaceExec { // Remove ssh and application connect from the owner role. This // prevents owners from have exec access to all workspaces. diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index a0a9e541a9..b2402a318d 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -302,9 +302,9 @@ func TestRolePermissions(t *testing.T) { InOrg(orgID). WithOwner(currentUser.String()), AuthorizeMap: map[bool][]hasAuthSubjects{ - true: {orgAdmin, orgAdminBanWorkspace}, + true: {owner, orgAdmin, orgAdminBanWorkspace}, false: { - owner, memberMe, setOtherOrg, + memberMe, setOtherOrg, templateAdmin, userAdmin, orgTemplateAdmin, orgUserAdmin, orgAuditor, }, diff --git a/enterprise/cli/sharing_test.go b/enterprise/cli/sharing_test.go index c096b515e3..9e99b85886 100644 --- a/enterprise/cli/sharing_test.go +++ b/enterprise/cli/sharing_test.go @@ -221,7 +221,7 @@ func TestSharingStatus(t *testing.T) { group, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "new-group", []uuid.UUID{orgMember.ID}) require.NoError(t, err) - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -284,7 +284,7 @@ func TestSharingRemove(t *testing.T) { require.NoError(t, err) // Share the workspace with a user to later remove - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group1.ID.String(): codersdk.WorkspaceRoleUse, group2.ID.String(): codersdk.WorkspaceRoleUse, @@ -357,7 +357,7 @@ func TestSharingRemove(t *testing.T) { require.NoError(t, err) // Share the workspace with a user to later remove - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group1.ID.String(): codersdk.WorkspaceRoleUse, group2.ID.String(): codersdk.WorkspaceRoleUse, diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index a1d26197a9..fd4f1d3934 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -3656,7 +3656,7 @@ func TestWorkspacesFiltering(t *testing.T) { }, }) - workspaceOwnerClient, workspaceOwner := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) + _, workspaceOwner := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) sharedWorkspace := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, @@ -3676,7 +3676,7 @@ func TestWorkspacesFiltering(t *testing.T) { }) require.NoError(t, err, "create group") - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ + err = ownerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -3709,8 +3709,8 @@ func TestWorkspacesFiltering(t *testing.T) { }, }) - workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) - sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + _, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) + sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, OrganizationID: owner.OrganizationID, }).Do().Workspace @@ -3727,7 +3727,7 @@ func TestWorkspacesFiltering(t *testing.T) { }) require.NoError(t, err, "create group") - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ + err = ownerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ toShareWithUser.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -3762,8 +3762,8 @@ func TestWorkspacesFiltering(t *testing.T) { }, }, }) - workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) - sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + _, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) + sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, OrganizationID: owner.OrganizationID, }).Do().Workspace @@ -3779,7 +3779,7 @@ func TestWorkspacesFiltering(t *testing.T) { }) require.NoError(t, err, "create group") - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ + err = ownerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -3811,8 +3811,8 @@ func TestWorkspacesFiltering(t *testing.T) { }, }, }) - workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) - sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + _, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) + sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, OrganizationID: owner.OrganizationID, }).Do().Workspace @@ -3827,7 +3827,7 @@ func TestWorkspacesFiltering(t *testing.T) { Name: "wibble", }) require.NoError(t, err, "create group") - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ + err = ownerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -3859,8 +3859,8 @@ func TestWorkspacesFiltering(t *testing.T) { }, }, }) - workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) - sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + _, workspaceOwner = coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAuditor(owner.OrganizationID)) + sharedWorkspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, OrganizationID: owner.OrganizationID, }).Do().Workspace @@ -3875,7 +3875,7 @@ func TestWorkspacesFiltering(t *testing.T) { Name: "wibble", }) require.NoError(t, err, "create group") - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ + err = ownerClient.UpdateWorkspaceACL(ctx, sharedWorkspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -4463,7 +4463,7 @@ func TestDeleteWorkspaceACL(t *testing.T) { Name: "wibble", }) require.NoError(t, err) - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -4512,7 +4512,7 @@ func TestDeleteWorkspaceACL(t *testing.T) { AddUsers: []string{toShareWithUser.ID.String()}, }) require.NoError(t, err) - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ GroupRoles: map[string]codersdk.WorkspaceRole{ group.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -4548,7 +4548,7 @@ func TestWorkspacesSharedWith(t *testing.T) { }, }) - workspaceOwnerClient, workspaceOwner := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + _, workspaceOwner := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) workspace := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, @@ -4576,7 +4576,7 @@ func TestWorkspacesSharedWith(t *testing.T) { require.NoError(t, err) // Share workspace with user and group - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ sharedWithUser.ID.String(): codersdk.WorkspaceRoleUse, }, @@ -4636,7 +4636,7 @@ func TestWorkspacesSharedWith(t *testing.T) { }, }) - workspaceOwnerClient, workspaceOwner := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + _, workspaceOwner := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) workspace := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: workspaceOwner.ID, @@ -4664,7 +4664,7 @@ func TestWorkspacesSharedWith(t *testing.T) { require.NoError(t, err) // Share workspace with user and group - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ sharedWithUser.ID.String(): codersdk.WorkspaceRoleUse, }, diff --git a/enterprise/coderd/workspacesharing_test.go b/enterprise/coderd/workspacesharing_test.go index 9b02cf66d3..2b196b1b70 100644 --- a/enterprise/coderd/workspacesharing_test.go +++ b/enterprise/coderd/workspacesharing_test.go @@ -194,7 +194,9 @@ func TestWorkspaceSharingDisabled(t *testing.T) { require.Equal(t, "Workspace sharing is disabled for this organization.", apiErr.Message) } - err = workspaceOwnerClient.UpdateWorkspaceACL(ctx, ws.ID, codersdk.UpdateWorkspaceACL{ + // Despite the site-wide workspace.share permission for the owner, + // the endpoint should return an authz error. + err = client.UpdateWorkspaceACL(ctx, ws.ID, codersdk.UpdateWorkspaceACL{ UserRoles: map[string]codersdk.WorkspaceRole{ uuid.NewString(): codersdk.WorkspaceRoleUse, },