From d3fb4630d1745ede755cd8a9fc113895111e2ed0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Aug 2024 13:10:42 -0500 Subject: [PATCH 1/6] chore: prevent removing members from the default organization Until multi-organizations is released outside an experiment, the experiment should be backwards compatible. --- coderd/members.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/coderd/members.go b/coderd/members.go index 4c28d4b6434f6..805bdafbd0447 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -106,6 +106,19 @@ func (api *API) deleteOrganizationMember(rw http.ResponseWriter, r *http.Request aReq.Old = member.OrganizationMember.Auditable(member.Username) defer commitAudit() + if organization.IsDefault { + // Multi-organizations is currently an experiment, which means it is feasible + // for a deployment to enable, then disable this. To maintain backwards + // compatibility, this safety is necessary. + // TODO: Remove this check when multi-organizations is fully supported. + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Removing members from the default organization is not supported.", + Detail: "Multi-organizations is currently an experiment, and until it is fully supported, the default org should be protected.", + Validations: nil, + }) + return + } + if member.UserID == apiKey.UserID { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{Message: "cannot remove self from an organization"}) return From 15e7ca24825fb58a9be02e4fad9deb4995a380b1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Aug 2024 14:59:25 -0500 Subject: [PATCH 2/6] chore: fixup test that it now requires enterprise --- coderd/members_test.go | 31 ---------------------------- enterprise/members_test.go | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/coderd/members_test.go b/coderd/members_test.go index 8ca655590c956..8d963221ffd77 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -50,37 +50,6 @@ func TestListMembers(t *testing.T) { }) } -func TestRemoveMember(t *testing.T) { - t.Parallel() - - t.Run("OK", func(t *testing.T) { - t.Parallel() - owner := coderdtest.New(t, nil) - first := coderdtest.CreateFirstUser(t, owner) - orgAdminClient, orgAdmin := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID)) - _, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) - - ctx := testutil.Context(t, testutil.WaitMedium) - // Verify the org of 3 members - members, err := orgAdminClient.OrganizationMembers(ctx, first.OrganizationID) - require.NoError(t, err) - require.Len(t, members, 3) - require.ElementsMatch(t, - []uuid.UUID{first.UserID, user.ID, orgAdmin.ID}, - db2sdk.List(members, onlyIDs)) - - // Delete a member - err = orgAdminClient.DeleteOrganizationMember(ctx, first.OrganizationID, user.Username) - require.NoError(t, err) - - members, err = orgAdminClient.OrganizationMembers(ctx, first.OrganizationID) - require.NoError(t, err) - require.Len(t, members, 2) - require.ElementsMatch(t, - []uuid.UUID{first.UserID, orgAdmin.ID}, - db2sdk.List(members, onlyIDs)) - }) -} func onlyIDs(u codersdk.OrganizationMemberWithUserData) uuid.UUID { return u.UserID diff --git a/enterprise/members_test.go b/enterprise/members_test.go index c328ce71d05fd..e29912be1bda2 100644 --- a/enterprise/members_test.go +++ b/enterprise/members_test.go @@ -18,6 +18,47 @@ import ( func TestEnterpriseMembers(t *testing.T) { t.Parallel() + t.Run("Remove", func(t *testing.T) { + t.Parallel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentMultiOrganization)} + owner, first := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + secondOrg := coderdenttest.CreateOrganization(t, owner, coderdenttest.CreateOrganizationOptions{}) + + orgAdminClient, orgAdmin := coderdtest.CreateAnotherUser(t, owner, secondOrg.ID, rbac.ScopedRoleOrgAdmin(secondOrg.ID)) + _, user := coderdtest.CreateAnotherUser(t, owner, secondOrg.ID) + + ctx := testutil.Context(t, testutil.WaitMedium) + // Verify the org of 3 members + members, err := orgAdminClient.OrganizationMembers(ctx, secondOrg.ID) + require.NoError(t, err) + require.Len(t, members, 3) + require.ElementsMatch(t, + []uuid.UUID{first.UserID, user.ID, orgAdmin.ID}, + db2sdk.List(members, onlyIDs)) + + // Delete a member + err = orgAdminClient.DeleteOrganizationMember(ctx, secondOrg.ID, user.Username) + require.NoError(t, err) + + members, err = orgAdminClient.OrganizationMembers(ctx, secondOrg.ID) + require.NoError(t, err) + require.Len(t, members, 2) + require.ElementsMatch(t, + []uuid.UUID{first.UserID, orgAdmin.ID}, + db2sdk.List(members, onlyIDs)) + }) + t.Run("PostUser", func(t *testing.T) { t.Parallel() From b3f6c784f25f0f7a5946a5cb2d851cb528627fcf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Aug 2024 15:02:44 -0500 Subject: [PATCH 3/6] fmt --- coderd/members_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/members_test.go b/coderd/members_test.go index 8d963221ffd77..f8dd211678a73 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -50,7 +50,6 @@ func TestListMembers(t *testing.T) { }) } - func onlyIDs(u codersdk.OrganizationMemberWithUserData) uuid.UUID { return u.UserID } From fb5a6cda2039c065a0ff427dde5badf93535fafd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 2 Aug 2024 14:14:27 -0500 Subject: [PATCH 4/6] move test to enterprise --- cli/organizationmembers_test.go | 45 ----------------- enterprise/cli/organizationmembers_test.go | 58 ++++++++++++++++++++++ 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/cli/organizationmembers_test.go b/cli/organizationmembers_test.go index e17b268ea798a..a1ee3b2da8cb7 100644 --- a/cli/organizationmembers_test.go +++ b/cli/organizationmembers_test.go @@ -35,48 +35,3 @@ func TestListOrganizationMembers(t *testing.T) { }) } -func TestRemoveOrganizationMembers(t *testing.T) { - t.Parallel() - - t.Run("OK", func(t *testing.T) { - t.Parallel() - - ownerClient := coderdtest.New(t, &coderdtest.Options{}) - owner := coderdtest.CreateFirstUser(t, ownerClient) - orgAdminClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAdmin(owner.OrganizationID)) - _, user := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) - - ctx := testutil.Context(t, testutil.WaitMedium) - - inv, root := clitest.New(t, "organization", "members", "remove", "-O", owner.OrganizationID.String(), user.Username) - clitest.SetupConfig(t, orgAdminClient, root) - - buf := new(bytes.Buffer) - inv.Stdout = buf - err := inv.WithContext(ctx).Run() - require.NoError(t, err) - - members, err := orgAdminClient.OrganizationMembers(ctx, owner.OrganizationID) - require.NoError(t, err) - - require.Len(t, members, 2) - }) - - t.Run("UserNotExists", func(t *testing.T) { - t.Parallel() - - ownerClient := coderdtest.New(t, &coderdtest.Options{}) - owner := coderdtest.CreateFirstUser(t, ownerClient) - orgAdminClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAdmin(owner.OrganizationID)) - - ctx := testutil.Context(t, testutil.WaitMedium) - - inv, root := clitest.New(t, "organization", "members", "remove", "-O", owner.OrganizationID.String(), "random_name") - clitest.SetupConfig(t, orgAdminClient, root) - - buf := new(bytes.Buffer) - inv.Stdout = buf - err := inv.WithContext(ctx).Run() - require.ErrorContains(t, err, "must be an existing uuid or username") - }) -} diff --git a/enterprise/cli/organizationmembers_test.go b/enterprise/cli/organizationmembers_test.go index fb6a7cb286058..b33d597faacae 100644 --- a/enterprise/cli/organizationmembers_test.go +++ b/enterprise/cli/organizationmembers_test.go @@ -15,6 +15,64 @@ import ( "github.com/coder/coder/v2/testutil" ) +func TestRemoveOrganizationMembers(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentMultiOrganization)} + + ownerClient, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + secondOrganization := coderdenttest.CreateOrganization(t, ownerClient, coderdenttest.CreateOrganizationOptions{}) + orgAdminClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, secondOrganization.ID, rbac.ScopedRoleOrgAdmin(secondOrganization.ID)) + _, user := coderdtest.CreateAnotherUser(t, ownerClient, secondOrganization.ID) + + ctx := testutil.Context(t, testutil.WaitMedium) + + inv, root := clitest.New(t, "organization", "members", "remove", "-O", secondOrganization.ID.String(), user.Username) + clitest.SetupConfig(t, orgAdminClient, root) + + buf := new(bytes.Buffer) + inv.Stdout = buf + err := inv.WithContext(ctx).Run() + require.NoError(t, err) + + members, err := orgAdminClient.OrganizationMembers(ctx, secondOrganization.ID) + require.NoError(t, err) + + require.Len(t, members, 2) + }) + + t.Run("UserNotExists", func(t *testing.T) { + t.Parallel() + + ownerClient := coderdtest.New(t, &coderdtest.Options{}) + owner := coderdtest.CreateFirstUser(t, ownerClient) + orgAdminClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAdmin(owner.OrganizationID)) + + ctx := testutil.Context(t, testutil.WaitMedium) + + inv, root := clitest.New(t, "organization", "members", "remove", "-O", owner.OrganizationID.String(), "random_name") + clitest.SetupConfig(t, orgAdminClient, root) + + buf := new(bytes.Buffer) + inv.Stdout = buf + err := inv.WithContext(ctx).Run() + require.ErrorContains(t, err, "must be an existing uuid or username") + }) +} + func TestEnterpriseListOrganizationMembers(t *testing.T) { t.Parallel() From 419cb449a954bc7066868c97af516813bf9837f7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 2 Aug 2024 14:57:27 -0500 Subject: [PATCH 5/6] make fmt --- cli/organizationmembers_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/organizationmembers_test.go b/cli/organizationmembers_test.go index a1ee3b2da8cb7..6cd8b9d3ccd4a 100644 --- a/cli/organizationmembers_test.go +++ b/cli/organizationmembers_test.go @@ -34,4 +34,3 @@ func TestListOrganizationMembers(t *testing.T) { require.Contains(t, buf.String(), owner.UserID.String()) }) } - From 476ed7dda70f7342e504a9f86a9cdd0262379612 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 5 Aug 2024 13:38:59 -0500 Subject: [PATCH 6/6] add unit test to verify not deleting org member from default org --- coderd/members_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/coderd/members_test.go b/coderd/members_test.go index f8dd211678a73..13b6779c30663 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -30,6 +30,25 @@ func TestAddMember(t *testing.T) { }) } +func TestDeleteMember(t *testing.T) { + t.Parallel() + + t.Run("NotAllowed", func(t *testing.T) { + t.Parallel() + owner := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + _, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitMedium) + // Deleting members from the default org is not allowed. + // If this behavior changes, and we allow deleting members from the default org, + // this test should be updated to check there is no error. + // nolint:gocritic // must be an owner to see the user + err := owner.DeleteOrganizationMember(ctx, first.OrganizationID, user.Username) + require.ErrorContains(t, err, "Multi-organizations is currently an experiment") + }) +} + func TestListMembers(t *testing.T) { t.Parallel()