chore: prevent removing members from the default organization (coder#14094)

Emyrk · web-flow · commit 0ad5f6067dd7 · 2024-08-05T13:48:10.000-05:00
* chore: prevent removing members from the default organization

Until multi-organizations is released outside an experiment, the
experiment should be backwards compatible.
diff --git a/cli/organizationmembers_test.go b/cli/organizationmembers_test.go
@@ -34,49 +34,3 @@ func TestListOrganizationMembers(t *testing.T) {
 		require.Contains(t, buf.String(), owner.UserID.String())
 	})
 }
-
-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/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
diff --git a/coderd/members_test.go b/coderd/members_test.go
@@ -30,54 +30,41 @@ func TestAddMember(t *testing.T) {
 	})
 }
 
-func TestListMembers(t *testing.T) {
+func TestDeleteMember(t *testing.T) {
 	t.Parallel()
 
-	t.Run("OK", func(t *testing.T) {
+	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)
 
-		client, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID))
-
-		ctx := testutil.Context(t, testutil.WaitShort)
-		members, err := client.OrganizationMembers(ctx, first.OrganizationID)
-		require.NoError(t, err)
-		require.Len(t, members, 2)
-		require.ElementsMatch(t,
-			[]uuid.UUID{first.UserID, user.ID},
-			db2sdk.List(members, onlyIDs))
+		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 TestRemoveMember(t *testing.T) {
+func TestListMembers(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)
+		client, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID))
 
-		members, err = orgAdminClient.OrganizationMembers(ctx, first.OrganizationID)
+		ctx := testutil.Context(t, testutil.WaitShort)
+		members, err := client.OrganizationMembers(ctx, first.OrganizationID)
 		require.NoError(t, err)
 		require.Len(t, members, 2)
 		require.ElementsMatch(t,
-			[]uuid.UUID{first.UserID, orgAdmin.ID},
+			[]uuid.UUID{first.UserID, user.ID},
 			db2sdk.List(members, onlyIDs))
 	})
 }
diff --git 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()
 
diff --git 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()
 
