From c6b31655f6fba6f2e756b27b8a37e3986d5dbb06 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Mon, 8 Sep 2025 13:34:50 +0000 Subject: [PATCH 1/8] chore: move user and group fetching into function --- cli/sharing.go | 195 ++++++++++++++++++++++++++++++------------------- 1 file changed, 120 insertions(+), 75 deletions(-) diff --git a/cli/sharing.go b/cli/sharing.go index aa1678e7a9e81..66171602b29db 100644 --- a/cli/sharing.go +++ b/cli/sharing.go @@ -72,11 +72,12 @@ func (r *RootCmd) statusWorkspaceSharing() *serpent.Command { func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Command { var ( + client = new(codersdk.Client) + users []string + groups []string + // Username regex taken from codersdk/name.go nameRoleRegex = regexp.MustCompile(`(^[a-zA-Z0-9]+(?:-[a-zA-Z0-9]+)*)+(?::([A-Za-z0-9-]+))?`) - client = new(codersdk.Client) - users []string - groups []string ) cmd := &serpent.Command{ @@ -115,84 +116,35 @@ func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Comma return err } - userRoles := make(map[string]codersdk.WorkspaceRole, len(users)) - if len(users) > 0 { - orgMembers, err := client.OrganizationMembers(inv.Context(), org.ID) - if err != nil { - return err + userRoleStrings := make([][2]string, len(users)) + for index, user := range users { + userAndRole := nameRoleRegex.FindStringSubmatch(user) + if userAndRole == nil { + return xerrors.Errorf("invalid user format %q: must match pattern 'username:role'", user) } - for _, user := range users { - userAndRole := nameRoleRegex.FindStringSubmatch(user) - if userAndRole == nil { - return xerrors.Errorf("invalid user format %q: must match pattern 'username:role'", user) - } - - username := userAndRole[1] - role := userAndRole[2] - if role == "" { - role = string(codersdk.WorkspaceRoleUse) - } - - userID := "" - for _, member := range orgMembers { - if member.Username == username { - userID = member.UserID.String() - break - } - } - if userID == "" { - return xerrors.Errorf("could not find user %s in the organization %s", username, org.Name) - } - - workspaceRole, err := stringToWorkspaceRole(role) - if err != nil { - return err - } - - userRoles[userID] = workspaceRole - } + userRoleStrings[index] = [2]string{userAndRole[1], userAndRole[2]} } - groupRoles := make(map[string]codersdk.WorkspaceRole) - if len(groups) > 0 { - orgGroups, err := client.Groups(inv.Context(), codersdk.GroupArguments{ - Organization: org.ID.String(), - }) - if err != nil { - return err + groupRoleStrings := make([][2]string, len(groups)) + for index, group := range groups { + groupAndRole := nameRoleRegex.FindStringSubmatch(group) + if groupAndRole == nil { + return xerrors.Errorf("invalid group format %q: must match pattern 'group:role'", group) } - for _, group := range groups { - groupAndRole := nameRoleRegex.FindStringSubmatch(group) - if groupAndRole == nil { - return xerrors.Errorf("invalid group format %q: must match pattern 'group:role'", group) - } - groupName := groupAndRole[1] - role := groupAndRole[2] - if role == "" { - role = string(codersdk.WorkspaceRoleUse) - } - - var orgGroup *codersdk.Group - for _, group := range orgGroups { - if group.Name == groupName { - orgGroup = &group - break - } - } - - if orgGroup == nil { - return xerrors.Errorf("could not find group named %s belonging to the organization %s", groupName, org.Name) - } - - workspaceRole, err := stringToWorkspaceRole(role) - if err != nil { - return err - } - - groupRoles[orgGroup.ID.String()] = workspaceRole - } + groupRoleStrings[index] = [2]string{groupAndRole[1], groupAndRole[2]} + } + + userRoles, groupRoles, err := fetchUsersAndGroups(inv.Context(), fetchUsersAndGroupsParams{ + Client: client, + Org: &org, + Users: userRoleStrings, + Groups: groupRoleStrings, + DefaultRole: codersdk.WorkspaceRoleUse, + }) + if err != nil { + return err } err = client.UpdateWorkspaceACL(inv.Context(), workspace.ID, codersdk.UpdateWorkspaceACL{ @@ -227,6 +179,8 @@ func stringToWorkspaceRole(role string) (codersdk.WorkspaceRole, error) { return codersdk.WorkspaceRoleUse, nil case string(codersdk.WorkspaceRoleAdmin): return codersdk.WorkspaceRoleAdmin, nil + case string(codersdk.WorkspaceRoleDeleted): + return codersdk.WorkspaceRoleDeleted, nil default: return "", xerrors.Errorf("invalid role %q: expected %q or %q", role, codersdk.WorkspaceRoleAdmin, codersdk.WorkspaceRoleUse) @@ -277,3 +231,94 @@ func workspaceACLToTable(ctx context.Context, acl *codersdk.WorkspaceACL) (strin return out, nil } + +type fetchUsersAndGroupsParams struct { + Client *codersdk.Client + Org *codersdk.Organization + Users [][2]string + Groups [][2]string + DefaultRole codersdk.WorkspaceRole +} + +func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) (map[string]codersdk.WorkspaceRole, map[string]codersdk.WorkspaceRole, error) { + var ( + client = params.Client + org = params.Org + users = params.Users + groups = params.Groups + defaultRole = params.DefaultRole + ) + + userRoles := make(map[string]codersdk.WorkspaceRole, len(users)) + if len(users) > 0 { + orgMembers, err := client.OrganizationMembers(ctx, org.ID) + if err != nil { + return nil, nil, err + } + + for _, user := range users { + username := user[0] + role := user[1] + if role == "" { + role = string(defaultRole) + } + + userID := "" + for _, member := range orgMembers { + if member.Username == username { + userID = member.UserID.String() + break + } + } + if userID == "" { + return nil, nil, xerrors.Errorf("could not find user %s in the organization %s", username, org.Name) + } + + workspaceRole, err := stringToWorkspaceRole(role) + if err != nil { + return nil, nil, err + } + + userRoles[userID] = workspaceRole + } + } + + groupRoles := make(map[string]codersdk.WorkspaceRole) + if len(groups) > 0 { + orgGroups, err := client.Groups(ctx, codersdk.GroupArguments{ + Organization: org.ID.String(), + }) + if err != nil { + return nil, nil, err + } + + for _, group := range groups { + groupName := group[0] + role := group[1] + if role == "" { + role = string(defaultRole) + } + + var orgGroup *codersdk.Group + for _, group := range orgGroups { + if group.Name == groupName { + orgGroup = &group + break + } + } + + if orgGroup == nil { + return nil, nil, xerrors.Errorf("could not find group named %s belonging to the organization %s", groupName, org.Name) + } + + workspaceRole, err := stringToWorkspaceRole(role) + if err != nil { + return nil, nil, err + } + + groupRoles[orgGroup.ID.String()] = workspaceRole + } + } + + return userRoles, groupRoles, nil +} From 5e621dcb2447422d28cff5184af1fb51338112d6 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Mon, 8 Sep 2025 16:07:06 +0000 Subject: [PATCH 2/8] feat(cli): add `sharing remove` command --- cli/sharing.go | 115 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 107 insertions(+), 8 deletions(-) diff --git a/cli/sharing.go b/cli/sharing.go index 66171602b29db..d79741a7e4cf5 100644 --- a/cli/sharing.go +++ b/cli/sharing.go @@ -26,6 +26,7 @@ func (r *RootCmd) sharing() *serpent.Command { }, Children: []*serpent.Command{ r.shareWorkspace(orgContext), + r.unshareWorkspace(orgContext), r.statusWorkspaceSharing(), }, Hidden: true, @@ -173,6 +174,104 @@ func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Comma return cmd } +func (r *RootCmd) unshareWorkspace(orgContext *OrganizationContext) *serpent.Command { + var ( + client = new(codersdk.Client) + users []string + groups []string + ) + + cmd := &serpent.Command{ + Use: "remove --user --group ", + Aliases: []string{"unshare"}, + Short: "Remove shared access for users or groups from a workspace.", + Options: serpent.OptionSet{ + { + Name: "user", + Description: "A comma separated list of users to share the workspace with.", + Flag: "user", + Value: serpent.StringArrayOf(&users), + }, { + Name: "group", + Description: "A comma separated list of groups to share the workspace with.", + Flag: "group", + Value: serpent.StringArrayOf(&groups), + }, + }, + Middleware: serpent.Chain( + r.InitClient(client), + serpent.RequireNArgs(1), + ), + Handler: func(inv *serpent.Invocation) error { + if len(users) == 0 && len(groups) == 0 { + return xerrors.New("at least one user or group must be provided") + } + + workspace, err := namedWorkspace(inv.Context(), client, inv.Args[0]) + if err != nil { + return xerrors.Errorf("could not fetch the workspace %s: %w", inv.Args[0], err) + } + + org, err := orgContext.Selected(inv, client) + if err != nil { + return err + } + + userRoleStrings := make([][2]string, len(users)) + for index, user := range users { + if !codersdk.UsernameValidRegex.MatchString(user) { + return xerrors.Errorf("invalid username") + } + + userRoleStrings[index] = [2]string{user, ""} + } + + groupRoleStrings := make([][2]string, len(groups)) + for index, group := range groups { + if !codersdk.UsernameValidRegex.MatchString(group) { + return xerrors.Errorf("invalid group name") + } + + groupRoleStrings[index] = [2]string{group, ""} + } + + userRoles, groupRoles, err := fetchUsersAndGroups(inv.Context(), fetchUsersAndGroupsParams{ + Client: client, + Org: &org, + Users: userRoleStrings, + Groups: groupRoleStrings, + DefaultRole: codersdk.WorkspaceRoleDeleted, + }) + if err != nil { + return err + } + + err = client.UpdateWorkspaceACL(inv.Context(), workspace.ID, codersdk.UpdateWorkspaceACL{ + UserRoles: userRoles, + GroupRoles: groupRoles, + }) + if err != nil { + return err + } + + acl, err := client.WorkspaceACL(inv.Context(), workspace.ID) + if err != nil { + return xerrors.Errorf("could not fetch current workspace ACL after sharing %w", err) + } + + out, err := workspaceACLToTable(inv.Context(), &acl) + if err != nil { + return err + } + + _, err = fmt.Fprintln(inv.Stdout, out) + return err + }, + } + + return cmd +} + func stringToWorkspaceRole(role string) (codersdk.WorkspaceRole, error) { switch role { case string(codersdk.WorkspaceRoleUse): @@ -182,8 +281,8 @@ func stringToWorkspaceRole(role string) (codersdk.WorkspaceRole, error) { case string(codersdk.WorkspaceRoleDeleted): return codersdk.WorkspaceRoleDeleted, nil default: - return "", xerrors.Errorf("invalid role %q: expected %q or %q", - role, codersdk.WorkspaceRoleAdmin, codersdk.WorkspaceRoleUse) + return "", xerrors.Errorf("invalid role %q: expected %q, %q, or \"%q\"", + role, codersdk.WorkspaceRoleAdmin, codersdk.WorkspaceRoleUse, codersdk.WorkspaceRoleDeleted) } } @@ -240,7 +339,7 @@ type fetchUsersAndGroupsParams struct { DefaultRole codersdk.WorkspaceRole } -func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) (map[string]codersdk.WorkspaceRole, map[string]codersdk.WorkspaceRole, error) { +func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) (userRoles map[string]codersdk.WorkspaceRole, groupRoles map[string]codersdk.WorkspaceRole, err error) { var ( client = params.Client org = params.Org @@ -249,7 +348,7 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) defaultRole = params.DefaultRole ) - userRoles := make(map[string]codersdk.WorkspaceRole, len(users)) + userRoles = make(map[string]codersdk.WorkspaceRole, len(users)) if len(users) > 0 { orgMembers, err := client.OrganizationMembers(ctx, org.ID) if err != nil { @@ -283,7 +382,7 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) } } - groupRoles := make(map[string]codersdk.WorkspaceRole) + groupRoles = make(map[string]codersdk.WorkspaceRole) if len(groups) > 0 { orgGroups, err := client.Groups(ctx, codersdk.GroupArguments{ Organization: org.ID.String(), @@ -300,9 +399,9 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) } var orgGroup *codersdk.Group - for _, group := range orgGroups { - if group.Name == groupName { - orgGroup = &group + for _, og := range orgGroups { + if og.Name == groupName { + orgGroup = &og break } } From 94d851a097d15ca29e383b61b16bd80a84551d63 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Mon, 8 Sep 2025 21:00:02 +0000 Subject: [PATCH 3/8] fix: only return non-deleted groups --- coderd/workspaces.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 3b8e35c003682..5b230f70f8af6 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -2215,12 +2215,18 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) { } groupIDs = append(groupIDs, id) } - // For context see https://github.com/coder/coder/pull/19375 - // nolint:gocritic - dbGroups, err := api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs}) - if err != nil && !xerrors.Is(err, sql.ErrNoRows) { - httpapi.InternalServerError(rw, err) - return + + // `GetGroups` returns all groups in the org if `GroupIds` is empty so we check + // the length before making the DB call. + dbGroups := make([]database.GetGroupsRow, 0) + if len(groupIDs) > 0 { + // For context see https://github.com/coder/coder/pull/19375 + // nolint:gocritic + dbGroups, err = api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs}) + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + httpapi.InternalServerError(rw, err) + return + } } groups := make([]codersdk.WorkspaceGroup, 0, len(dbGroups)) From a4cd678b1c84e6566279d91f9e9bed00ca3ef5d4 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Mon, 8 Sep 2025 21:00:02 +0000 Subject: [PATCH 4/8] chore: add tests for `sharing remove` command --- cli/sharing_test.go | 114 ++++++++++++++++++++++++ enterprise/cli/sharing_test.go | 153 ++++++++++++++++++++++++++++++++- 2 files changed, 266 insertions(+), 1 deletion(-) diff --git a/cli/sharing_test.go b/cli/sharing_test.go index 01bfc0a83873a..d5cda77b7b252 100644 --- a/cli/sharing_test.go +++ b/cli/sharing_test.go @@ -217,3 +217,117 @@ func TestSharingStatus(t *testing.T) { assert.True(t, found, "expected to find username %s with role %s in the output: %s", toShareWithUser.Username, codersdk.WorkspaceRoleUse, out.String()) }) } + +func TestSharingRemove(t *testing.T) { + t.Parallel() + + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentWorkspaceSharing)} + + t.Run("RemoveSharedUser_Simple", func(t *testing.T) { + t.Parallel() + + var ( + client, db = coderdtest.NewWithDatabase(t, &coderdtest.Options{ + DeploymentValues: dv, + }) + orgOwner = coderdtest.CreateFirstUser(t, client) + workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID, rbac.ScopedRoleOrgAuditor(orgOwner.OrganizationID)) + workspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OwnerID: workspaceOwner.ID, + OrganizationID: orgOwner.OrganizationID, + }).Do().Workspace + _, toRemoveUser = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + _, toShareWithUser = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + ) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // Share the workspace with a user to later remove + err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + UserRoles: map[string]codersdk.WorkspaceRole{ + toShareWithUser.ID.String(): codersdk.WorkspaceRoleUse, + toRemoveUser.ID.String(): codersdk.WorkspaceRoleUse, + }, + }) + require.NoError(t, err) + + inv, root := clitest.New(t, + "sharing", + "remove", + workspace.Name, + "--org", orgOwner.OrganizationID.String(), + "--user", toRemoveUser.Username, + ) + clitest.SetupConfig(t, workspaceOwnerClient, root) + + out := bytes.NewBuffer(nil) + inv.Stdout = out + err = inv.WithContext(ctx).Run() + require.NoError(t, err) + + acl, err := workspaceOwnerClient.WorkspaceACL(inv.Context(), workspace.ID) + require.NoError(t, err) + + removedCorrectUser := true + keptOtherUser := false + for _, user := range acl.Users { + if user.ID == toRemoveUser.ID { + removedCorrectUser = false + } + + if user.ID == toShareWithUser.ID { + keptOtherUser = true + } + } + assert.True(t, removedCorrectUser) + assert.True(t, keptOtherUser) + }) + + t.Run("RemoveSharedUser_Multiple", func(t *testing.T) { + t.Parallel() + + var ( + client, db = coderdtest.NewWithDatabase(t, &coderdtest.Options{ + DeploymentValues: dv, + }) + orgOwner = coderdtest.CreateFirstUser(t, client) + workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID, rbac.ScopedRoleOrgAuditor(orgOwner.OrganizationID)) + workspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OwnerID: workspaceOwner.ID, + OrganizationID: orgOwner.OrganizationID, + }).Do().Workspace + _, toRemoveUser1 = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + _, toRemoveUser2 = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + ) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // Share the workspace with a user to later remove + err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + UserRoles: map[string]codersdk.WorkspaceRole{ + toRemoveUser2.ID.String(): codersdk.WorkspaceRoleUse, + toRemoveUser1.ID.String(): codersdk.WorkspaceRoleUse, + }, + }) + require.NoError(t, err) + + inv, root := clitest.New(t, + "sharing", + "remove", + workspace.Name, + "--org", orgOwner.OrganizationID.String(), + fmt.Sprintf("--user=%s,%s", toRemoveUser1.Username, toRemoveUser2.Username), + ) + clitest.SetupConfig(t, workspaceOwnerClient, root) + + out := bytes.NewBuffer(nil) + inv.Stdout = out + err = inv.WithContext(ctx).Run() + require.NoError(t, err) + + acl, err := workspaceOwnerClient.WorkspaceACL(inv.Context(), workspace.ID) + require.NoError(t, err) + assert.Empty(t, acl.Users) + }) +} diff --git a/enterprise/cli/sharing_test.go b/enterprise/cli/sharing_test.go index 906c02a148d4b..4dac0501f9fa9 100644 --- a/enterprise/cli/sharing_test.go +++ b/enterprise/cli/sharing_test.go @@ -23,7 +23,7 @@ import ( "github.com/coder/coder/v2/testutil" ) -func TestSharingShareEnterprise(t *testing.T) { +func TestSharingShare(t *testing.T) { t.Parallel() dv := coderdtest.DeploymentValues(t) @@ -245,6 +245,157 @@ func TestSharingStatus(t *testing.T) { }) } +func TestSharingRemove(t *testing.T) { + t.Parallel() + + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentWorkspaceSharing)} + + t.Run("RemoveSharedGroup_Single", func(t *testing.T) { + t.Parallel() + + var ( + client, db, orgOwner = coderdenttest.NewWithDatabase(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID, rbac.ScopedRoleOrgAuditor(orgOwner.OrganizationID)) + workspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OwnerID: workspaceOwner.ID, + OrganizationID: orgOwner.OrganizationID, + }).Do().Workspace + _, groupUser1 = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + _, groupUser2 = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + ) + + ctx := testutil.Context(t, testutil.WaitMedium) + + group1, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "group-1", []uuid.UUID{groupUser1.ID, groupUser2.ID}) + require.NoError(t, err) + + group2, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "group-2", []uuid.UUID{groupUser1.ID, groupUser2.ID}) + require.NoError(t, err) + + // Share the workspace with a user to later remove + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + GroupRoles: map[string]codersdk.WorkspaceRole{ + group1.ID.String(): codersdk.WorkspaceRoleUse, + group2.ID.String(): codersdk.WorkspaceRoleUse, + }, + }) + require.NoError(t, err) + + inv, root := clitest.New(t, + "sharing", + "remove", + workspace.Name, + "--org", orgOwner.OrganizationID.String(), + "--group", group1.Name, + ) + clitest.SetupConfig(t, workspaceOwnerClient, root) + + err = inv.WithContext(ctx).Run() + require.NoError(t, err) + + acl, err := workspaceOwnerClient.WorkspaceACL(inv.Context(), workspace.ID) + require.NoError(t, err) + + removedGroup1 := true + removedGroup2 := true + for _, group := range acl.Groups { + if group.ID == group1.ID { + removedGroup1 = false + continue + } + + if group.ID == group2.ID { + removedGroup2 = false + continue + } + } + assert.True(t, removedGroup1) + assert.False(t, removedGroup2) + }) + + t.Run("RemoveSharedGroup_Multiple", func(t *testing.T) { + t.Parallel() + + var ( + client, db, orgOwner = coderdenttest.NewWithDatabase(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID, rbac.ScopedRoleOrgAuditor(orgOwner.OrganizationID)) + workspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OwnerID: workspaceOwner.ID, + OrganizationID: orgOwner.OrganizationID, + }).Do().Workspace + _, groupUser1 = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + _, groupUser2 = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID) + ) + + ctx := testutil.Context(t, testutil.WaitMedium) + + group1, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "group-1", []uuid.UUID{groupUser1.ID, groupUser2.ID}) + require.NoError(t, err) + + group2, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "group-2", []uuid.UUID{groupUser1.ID, groupUser2.ID}) + require.NoError(t, err) + + // Share the workspace with a user to later remove + err = client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + GroupRoles: map[string]codersdk.WorkspaceRole{ + group1.ID.String(): codersdk.WorkspaceRoleUse, + group2.ID.String(): codersdk.WorkspaceRoleUse, + }, + }) + require.NoError(t, err) + + inv, root := clitest.New(t, + "sharing", + "remove", + workspace.Name, + "--org", orgOwner.OrganizationID.String(), + fmt.Sprintf("--group=%s,%s", group1.Name, group2.Name), + ) + clitest.SetupConfig(t, workspaceOwnerClient, root) + + err = inv.WithContext(ctx).Run() + require.NoError(t, err) + + acl, err := workspaceOwnerClient.WorkspaceACL(inv.Context(), workspace.ID) + require.NoError(t, err) + + removedGroup1 := true + removedGroup2 := true + for _, group := range acl.Groups { + if group.ID == group1.ID { + removedGroup1 = false + continue + } + + if group.ID == group2.ID { + removedGroup2 = false + continue + } + } + assert.True(t, removedGroup1) + assert.True(t, removedGroup2) + }) +} + func createGroupWithMembers(ctx context.Context, client *codersdk.Client, orgID uuid.UUID, name string, memberIDs []uuid.UUID) (codersdk.Group, error) { group, err := client.CreateGroup(ctx, orgID, codersdk.CreateGroupRequest{ Name: name, From d7edc04781b7efbeac4a14be8d26cdd21a0bd567 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Wed, 10 Sep 2025 23:41:31 +0000 Subject: [PATCH 5/8] chore: use new(bytes.Buffer) vs bytes.NewBuffer --- cli/sharing_test.go | 12 ++++++------ enterprise/cli/sharing_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/cli/sharing_test.go b/cli/sharing_test.go index d5cda77b7b252..8c1d078bf6e63 100644 --- a/cli/sharing_test.go +++ b/cli/sharing_test.go @@ -44,7 +44,7 @@ func TestSharingShare(t *testing.T) { inv, root := clitest.New(t, "sharing", "add", workspace.Name, "--org", orgOwner.OrganizationID.String(), "--user", toShareWithUser.Username) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err := inv.WithContext(ctx).Run() require.NoError(t, err) @@ -91,7 +91,7 @@ func TestSharingShare(t *testing.T) { ) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err := inv.WithContext(ctx).Run() require.NoError(t, err) @@ -142,7 +142,7 @@ func TestSharingShare(t *testing.T) { ) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err := inv.WithContext(ctx).Run() require.NoError(t, err) @@ -202,7 +202,7 @@ func TestSharingStatus(t *testing.T) { inv, root := clitest.New(t, "sharing", "status", workspace.Name, "--org", orgOwner.OrganizationID.String()) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) @@ -261,7 +261,7 @@ func TestSharingRemove(t *testing.T) { ) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) @@ -321,7 +321,7 @@ func TestSharingRemove(t *testing.T) { ) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) diff --git a/enterprise/cli/sharing_test.go b/enterprise/cli/sharing_test.go index 4dac0501f9fa9..1b75e9571ea58 100644 --- a/enterprise/cli/sharing_test.go +++ b/enterprise/cli/sharing_test.go @@ -59,7 +59,7 @@ func TestSharingShare(t *testing.T) { inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--org", orgOwner.OrganizationID.String(), "--group", group.Name) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) @@ -117,7 +117,7 @@ func TestSharingShare(t *testing.T) { fmt.Sprintf("--group=%s,%s", wibbleGroup.Name, wobbleGroup.Name)) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) @@ -164,7 +164,7 @@ func TestSharingShare(t *testing.T) { inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--org", orgOwner.OrganizationID.String(), "--group", fmt.Sprintf("%s:admin", group.Name)) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) @@ -229,7 +229,7 @@ func TestSharingStatus(t *testing.T) { inv, root := clitest.New(t, "sharing", "status", workspace.Name, "--org", orgOwner.OrganizationID.String()) clitest.SetupConfig(t, workspaceOwnerClient, root) - out := bytes.NewBuffer(nil) + out := new(bytes.Buffer) inv.Stdout = out err = inv.WithContext(ctx).Run() require.NoError(t, err) From 7845efcc68048beaaa56b75207d80869bdcd44e1 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Wed, 10 Sep 2025 23:43:12 +0000 Subject: [PATCH 6/8] chore: change comment to be accurate --- coderd/workspaces.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 5b230f70f8af6..64ef5c9f8171f 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -2216,8 +2216,8 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) { groupIDs = append(groupIDs, id) } - // `GetGroups` returns all groups in the org if `GroupIds` is empty so we check - // the length before making the DB call. + // `GetGroups` returns all groups if `GroupIds` is empty so we check the length + // before making the DB call. dbGroups := make([]database.GetGroupsRow, 0) if len(groupIDs) > 0 { // For context see https://github.com/coder/coder/pull/19375 From 455c08824811af0faf96007a8e7f60f155c489cd Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Wed, 10 Sep 2025 23:44:01 +0000 Subject: [PATCH 7/8] chore: refactor commands to not use orgContext --- cli/sharing.go | 42 ++++++++++++++-------------------- cli/sharing_test.go | 9 +++----- enterprise/cli/sharing_test.go | 10 ++++---- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/cli/sharing.go b/cli/sharing.go index d79741a7e4cf5..26f60ef81253c 100644 --- a/cli/sharing.go +++ b/cli/sharing.go @@ -10,13 +10,12 @@ import ( "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" "github.com/coder/serpent" + "github.com/google/uuid" ) const defaultGroupDisplay = "-" func (r *RootCmd) sharing() *serpent.Command { - orgContext := NewOrganizationContext() - cmd := &serpent.Command{ Use: "sharing [subcommand]", Short: "Commands for managing shared workspaces", @@ -25,14 +24,13 @@ func (r *RootCmd) sharing() *serpent.Command { return inv.Command.HelpHandler(inv) }, Children: []*serpent.Command{ - r.shareWorkspace(orgContext), - r.unshareWorkspace(orgContext), + r.shareWorkspace(), + r.unshareWorkspace(), r.statusWorkspaceSharing(), }, Hidden: true, } - orgContext.AttachOptions(cmd) return cmd } @@ -71,7 +69,7 @@ func (r *RootCmd) statusWorkspaceSharing() *serpent.Command { return cmd } -func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Command { +func (r *RootCmd) shareWorkspace() *serpent.Command { var ( client = new(codersdk.Client) users []string @@ -112,11 +110,6 @@ func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Comma return xerrors.Errorf("could not fetch the workspace %s: %w", inv.Args[0], err) } - org, err := orgContext.Selected(inv, client) - if err != nil { - return err - } - userRoleStrings := make([][2]string, len(users)) for index, user := range users { userAndRole := nameRoleRegex.FindStringSubmatch(user) @@ -139,7 +132,8 @@ func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Comma userRoles, groupRoles, err := fetchUsersAndGroups(inv.Context(), fetchUsersAndGroupsParams{ Client: client, - Org: &org, + OrgID: workspace.OrganizationID, + OrgName: workspace.OrganizationName, Users: userRoleStrings, Groups: groupRoleStrings, DefaultRole: codersdk.WorkspaceRoleUse, @@ -174,7 +168,7 @@ func (r *RootCmd) shareWorkspace(orgContext *OrganizationContext) *serpent.Comma return cmd } -func (r *RootCmd) unshareWorkspace(orgContext *OrganizationContext) *serpent.Command { +func (r *RootCmd) unshareWorkspace() *serpent.Command { var ( client = new(codersdk.Client) users []string @@ -212,11 +206,6 @@ func (r *RootCmd) unshareWorkspace(orgContext *OrganizationContext) *serpent.Com return xerrors.Errorf("could not fetch the workspace %s: %w", inv.Args[0], err) } - org, err := orgContext.Selected(inv, client) - if err != nil { - return err - } - userRoleStrings := make([][2]string, len(users)) for index, user := range users { if !codersdk.UsernameValidRegex.MatchString(user) { @@ -237,7 +226,8 @@ func (r *RootCmd) unshareWorkspace(orgContext *OrganizationContext) *serpent.Com userRoles, groupRoles, err := fetchUsersAndGroups(inv.Context(), fetchUsersAndGroupsParams{ Client: client, - Org: &org, + OrgID: workspace.OrganizationID, + OrgName: workspace.OrganizationName, Users: userRoleStrings, Groups: groupRoleStrings, DefaultRole: codersdk.WorkspaceRoleDeleted, @@ -333,7 +323,8 @@ func workspaceACLToTable(ctx context.Context, acl *codersdk.WorkspaceACL) (strin type fetchUsersAndGroupsParams struct { Client *codersdk.Client - Org *codersdk.Organization + OrgID uuid.UUID + OrgName string Users [][2]string Groups [][2]string DefaultRole codersdk.WorkspaceRole @@ -342,7 +333,8 @@ type fetchUsersAndGroupsParams struct { func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) (userRoles map[string]codersdk.WorkspaceRole, groupRoles map[string]codersdk.WorkspaceRole, err error) { var ( client = params.Client - org = params.Org + orgID = params.OrgID + orgName = params.OrgName users = params.Users groups = params.Groups defaultRole = params.DefaultRole @@ -350,7 +342,7 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) userRoles = make(map[string]codersdk.WorkspaceRole, len(users)) if len(users) > 0 { - orgMembers, err := client.OrganizationMembers(ctx, org.ID) + orgMembers, err := client.OrganizationMembers(ctx, orgID) if err != nil { return nil, nil, err } @@ -370,7 +362,7 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) } } if userID == "" { - return nil, nil, xerrors.Errorf("could not find user %s in the organization %s", username, org.Name) + return nil, nil, xerrors.Errorf("could not find user %s in the organization %s", username, orgName) } workspaceRole, err := stringToWorkspaceRole(role) @@ -385,7 +377,7 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) groupRoles = make(map[string]codersdk.WorkspaceRole) if len(groups) > 0 { orgGroups, err := client.Groups(ctx, codersdk.GroupArguments{ - Organization: org.ID.String(), + Organization: orgID.String(), }) if err != nil { return nil, nil, err @@ -407,7 +399,7 @@ func fetchUsersAndGroups(ctx context.Context, params fetchUsersAndGroupsParams) } if orgGroup == nil { - return nil, nil, xerrors.Errorf("could not find group named %s belonging to the organization %s", groupName, org.Name) + return nil, nil, xerrors.Errorf("could not find group named %s belonging to the organization %s", groupName, orgName) } workspaceRole, err := stringToWorkspaceRole(role) diff --git a/cli/sharing_test.go b/cli/sharing_test.go index 8c1d078bf6e63..9044ed4968170 100644 --- a/cli/sharing_test.go +++ b/cli/sharing_test.go @@ -41,7 +41,7 @@ func TestSharingShare(t *testing.T) { ) ctx := testutil.Context(t, testutil.WaitMedium) - inv, root := clitest.New(t, "sharing", "add", workspace.Name, "--org", orgOwner.OrganizationID.String(), "--user", toShareWithUser.Username) + inv, root := clitest.New(t, "sharing", "add", workspace.Name, "--user", toShareWithUser.Username) clitest.SetupConfig(t, workspaceOwnerClient, root) out := new(bytes.Buffer) @@ -86,7 +86,7 @@ func TestSharingShare(t *testing.T) { ctx := testutil.Context(t, testutil.WaitMedium) inv, root := clitest.New(t, "sharing", - "add", workspace.Name, "--org", orgOwner.OrganizationID.String(), + "add", workspace.Name, fmt.Sprintf("--user=%s,%s", toShareWithUser1.Username, toShareWithUser2.Username), ) clitest.SetupConfig(t, workspaceOwnerClient, root) @@ -137,7 +137,6 @@ func TestSharingShare(t *testing.T) { ctx := testutil.Context(t, testutil.WaitMedium) inv, root := clitest.New(t, "sharing", "add", workspace.Name, - "--org", orgOwner.OrganizationID.String(), "--user", fmt.Sprintf("%s:admin", toShareWithUser.Username), ) clitest.SetupConfig(t, workspaceOwnerClient, root) @@ -199,7 +198,7 @@ func TestSharingStatus(t *testing.T) { }) require.NoError(t, err) - inv, root := clitest.New(t, "sharing", "status", workspace.Name, "--org", orgOwner.OrganizationID.String()) + inv, root := clitest.New(t, "sharing", "status", workspace.Name) clitest.SetupConfig(t, workspaceOwnerClient, root) out := new(bytes.Buffer) @@ -256,7 +255,6 @@ func TestSharingRemove(t *testing.T) { "sharing", "remove", workspace.Name, - "--org", orgOwner.OrganizationID.String(), "--user", toRemoveUser.Username, ) clitest.SetupConfig(t, workspaceOwnerClient, root) @@ -316,7 +314,6 @@ func TestSharingRemove(t *testing.T) { "sharing", "remove", workspace.Name, - "--org", orgOwner.OrganizationID.String(), fmt.Sprintf("--user=%s,%s", toRemoveUser1.Username, toRemoveUser2.Username), ) clitest.SetupConfig(t, workspaceOwnerClient, root) diff --git a/enterprise/cli/sharing_test.go b/enterprise/cli/sharing_test.go index 1b75e9571ea58..65b8ce53a2bf1 100644 --- a/enterprise/cli/sharing_test.go +++ b/enterprise/cli/sharing_test.go @@ -56,7 +56,7 @@ func TestSharingShare(t *testing.T) { group, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "new-group", []uuid.UUID{orgMember.ID}) require.NoError(t, err) - inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--org", orgOwner.OrganizationID.String(), "--group", group.Name) + inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--group", group.Name) clitest.SetupConfig(t, workspaceOwnerClient, root) out := new(bytes.Buffer) @@ -113,7 +113,7 @@ func TestSharingShare(t *testing.T) { wobbleGroup, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "wobble", []uuid.UUID{wobbleMember.ID}) require.NoError(t, err) - inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--org", orgOwner.OrganizationID.String(), + inv, root := clitest.New(t, "sharing", "share", workspace.Name, fmt.Sprintf("--group=%s,%s", wibbleGroup.Name, wobbleGroup.Name)) clitest.SetupConfig(t, workspaceOwnerClient, root) @@ -161,7 +161,7 @@ func TestSharingShare(t *testing.T) { group, err := createGroupWithMembers(ctx, client, orgOwner.OrganizationID, "new-group", []uuid.UUID{orgMember.ID}) require.NoError(t, err) - inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--org", orgOwner.OrganizationID.String(), "--group", fmt.Sprintf("%s:admin", group.Name)) + inv, root := clitest.New(t, "sharing", "share", workspace.Name, "--group", fmt.Sprintf("%s:admin", group.Name)) clitest.SetupConfig(t, workspaceOwnerClient, root) out := new(bytes.Buffer) @@ -226,7 +226,7 @@ func TestSharingStatus(t *testing.T) { }) require.NoError(t, err) - inv, root := clitest.New(t, "sharing", "status", workspace.Name, "--org", orgOwner.OrganizationID.String()) + inv, root := clitest.New(t, "sharing", "status", workspace.Name) clitest.SetupConfig(t, workspaceOwnerClient, root) out := new(bytes.Buffer) @@ -295,7 +295,6 @@ func TestSharingRemove(t *testing.T) { "sharing", "remove", workspace.Name, - "--org", orgOwner.OrganizationID.String(), "--group", group1.Name, ) clitest.SetupConfig(t, workspaceOwnerClient, root) @@ -367,7 +366,6 @@ func TestSharingRemove(t *testing.T) { "sharing", "remove", workspace.Name, - "--org", orgOwner.OrganizationID.String(), fmt.Sprintf("--group=%s,%s", group1.Name, group2.Name), ) clitest.SetupConfig(t, workspaceOwnerClient, root) From b0c55f4c49ede11235f4b5f5b55438183b8ed2b5 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Thu, 11 Sep 2025 00:03:38 +0000 Subject: [PATCH 8/8] chore: lint --- cli/sharing.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/sharing.go b/cli/sharing.go index 26f60ef81253c..accc930ea4b60 100644 --- a/cli/sharing.go +++ b/cli/sharing.go @@ -7,10 +7,11 @@ import ( "golang.org/x/xerrors" + "github.com/google/uuid" + "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" "github.com/coder/serpent" - "github.com/google/uuid" ) const defaultGroupDisplay = "-"