From 14b2d35834c261e7f3d73c5660051a579c957147 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 12 Jun 2023 19:12:15 -0500 Subject: [PATCH 1/4] test: add unit test that deletes abandoned workspace This is to ensure we do not break this functionality in future. This is important incase this edge case happens, an admin can clean up the abandoned resources. --- cli/delete_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/cli/delete_test.go b/cli/delete_test.go index c79d07b075425..8827b51fcd41d 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -2,6 +2,7 @@ package cli_test import ( "context" + "fmt" "io" "testing" @@ -10,8 +11,11 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/dbauthz" "github.com/coder/coder/codersdk" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func TestDelete(t *testing.T) { @@ -68,6 +72,54 @@ func TestDelete(t *testing.T) { <-doneChan }) + // Super orphaned, as the workspace doesn't even have a user. + // This is not a scenario we should ever get into, as we do not allow users + // to be deleted if they have workspaces. However issue #7872 shows that + // it is possible to get into this state. An admin should be able to still + // force a delete action on the workspace. + t.Run("OrphanDeletedUser", func(t *testing.T) { + t.Parallel() + client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + deleteMeClient, deleteMeUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + workspace := coderdtest.CreateWorkspace(t, deleteMeClient, user.OrganizationID, template.ID) + coderdtest.AwaitWorkspaceBuildJob(t, deleteMeClient, workspace.LatestBuild.ID) + + // The API checks if the user has any workspaces, so we cannot delete a user + // this way. + ctx := testutil.Context(t, testutil.WaitShort) + var _ = ctx + var _ = api + var _ = deleteMeUser + // nolint:gocritic // Unit test + err := api.Database.UpdateUserDeletedByID(dbauthz.AsSystemRestricted(ctx), database.UpdateUserDeletedByIDParams{ + ID: deleteMeUser.ID, + Deleted: true, + }) + require.NoError(t, err) + + inv, root := clitest.New(t, "delete", fmt.Sprintf("%s/%s", deleteMeUser.ID, workspace.ID), "-y", "--orphan") + + clitest.SetupConfig(t, client, root) + doneChan := make(chan struct{}) + pty := ptytest.New(t).Attach(inv) + inv.Stderr = pty.Output() + go func() { + defer close(doneChan) + err := inv.Run() + // When running with the race detector on, we sometimes get an EOF. + if err != nil { + assert.ErrorIs(t, err, io.EOF) + } + }() + pty.ExpectMatch("workspace has been deleted") + <-doneChan + }) + t.Run("DifferentUser", func(t *testing.T) { t.Parallel() adminClient := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) From 16a5d7c73bf2dc9e77ad12c4d6a69522cda9fc77 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 12 Jun 2023 19:26:38 -0500 Subject: [PATCH 2/4] fixed deleting from a slice inside a range loop --- coderd/database/dbfake/dbfake.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index e3a3aecebb0ff..dad08f081a4a9 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -4646,11 +4646,19 @@ func (q *fakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.U u.Deleted = params.Deleted q.users[i] = u // NOTE: In the real world, this is done by a trigger. - for i, k := range q.apiKeys { + i := 0 + for { + if i >= len(q.apiKeys) { + break + } + k := q.apiKeys[i] if k.UserID == u.ID { q.apiKeys[i] = q.apiKeys[len(q.apiKeys)-1] q.apiKeys = q.apiKeys[:len(q.apiKeys)-1] + // We removed an element, so decrement + i-- } + i++ } return nil } From ba01002d69c2eafdb0679710cc827a58fbfcd333 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 12 Jun 2023 19:30:15 -0500 Subject: [PATCH 3/4] Workspace name over id --- cli/delete_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/delete_test.go b/cli/delete_test.go index 8827b51fcd41d..ff900e13a6f08 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -102,7 +102,7 @@ func TestDelete(t *testing.T) { }) require.NoError(t, err) - inv, root := clitest.New(t, "delete", fmt.Sprintf("%s/%s", deleteMeUser.ID, workspace.ID), "-y", "--orphan") + inv, root := clitest.New(t, "delete", fmt.Sprintf("%s/%s", deleteMeUser.ID, workspace.Name), "-y", "--orphan") clitest.SetupConfig(t, client, root) doneChan := make(chan struct{}) From 7dd86944886666b373815efcf8caedcb1a408fe0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 13 Jun 2023 09:49:34 -0500 Subject: [PATCH 4/4] linting --- cli/delete_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cli/delete_test.go b/cli/delete_test.go index ff900e13a6f08..40f5b9ac22168 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -92,9 +92,6 @@ func TestDelete(t *testing.T) { // The API checks if the user has any workspaces, so we cannot delete a user // this way. ctx := testutil.Context(t, testutil.WaitShort) - var _ = ctx - var _ = api - var _ = deleteMeUser // nolint:gocritic // Unit test err := api.Database.UpdateUserDeletedByID(dbauthz.AsSystemRestricted(ctx), database.UpdateUserDeletedByIDParams{ ID: deleteMeUser.ID,