coder · Emyrk · Feb 20, 2024 · Feb 12, 2024 · Feb 12, 2024 · Feb 12, 2024
diff --git a/cli/delete_test.go b/cli/delete_test.go
@@ -11,7 +11,6 @@ import (
 
 	"github.com/coder/coder/v2/cli/clitest"
 	"github.com/coder/coder/v2/coderd/coderdtest"
-	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/pty/ptytest"
@@ -95,10 +94,7 @@ func TestDelete(t *testing.T) {
 		// this way.
 		ctx := testutil.Context(t, testutil.WaitShort)
 		// nolint:gocritic // Unit test
-		err := api.Database.UpdateUserDeletedByID(dbauthz.AsSystemRestricted(ctx), database.UpdateUserDeletedByIDParams{
-			ID:      deleteMeUser.ID,
-			Deleted: true,
-		})
+		err := api.Database.UpdateUserDeletedByID(dbauthz.AsSystemRestricted(ctx), deleteMeUser.ID)
 		require.NoError(t, err)
 
 		inv, root := clitest.New(t, "delete", fmt.Sprintf("%s/%s", deleteMeUser.ID, workspace.Name), "-y", "--orphan")

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -607,16 +607,6 @@ func (q *querier) SoftDeleteTemplateByID(ctx context.Context, id uuid.UUID) erro
 	return deleteQ(q.log, q.auth, q.db.GetTemplateByID, deleteF)(ctx, id)
 }
 
-func (q *querier) SoftDeleteUserByID(ctx context.Context, id uuid.UUID) error {
-	deleteF := func(ctx context.Context, id uuid.UUID) error {
-		return q.db.UpdateUserDeletedByID(ctx, database.UpdateUserDeletedByIDParams{
-			ID:      id,
-			Deleted: true,
-		})
-	}
-	return deleteQ(q.log, q.auth, q.db.GetUserByID, deleteF)(ctx, id)
-}
-
 func (q *querier) SoftDeleteWorkspaceByID(ctx context.Context, id uuid.UUID) error {
 	return deleteQ(q.log, q.auth, q.db.GetWorkspaceByID, func(ctx context.Context, id uuid.UUID) error {
 		return q.db.UpdateWorkspaceDeletedByID(ctx, database.UpdateWorkspaceDeletedByIDParams{
@@ -2881,16 +2871,8 @@ func (q *querier) UpdateUserAppearanceSettings(ctx context.Context, arg database
 	return q.db.UpdateUserAppearanceSettings(ctx, arg)
 }
 
-// UpdateUserDeletedByID
-// Deprecated: Delete this function in favor of 'SoftDeleteUserByID'. Deletes are
-// irreversible.
-func (q *querier) UpdateUserDeletedByID(ctx context.Context, arg database.UpdateUserDeletedByIDParams) error {
-	fetch := func(ctx context.Context, arg database.UpdateUserDeletedByIDParams) (database.User, error) {
-		return q.db.GetUserByID(ctx, arg.ID)
-	}
-	// This uses the rbac.ActionDelete action always as this function should always delete.
-	// We should delete this function in favor of 'SoftDeleteUserByID'.
-	return deleteQ(q.log, q.auth, fetch, q.db.UpdateUserDeletedByID)(ctx, arg)
+func (q *querier) UpdateUserDeletedByID(ctx context.Context, id uuid.UUID) error {
+	return deleteQ(q.log, q.auth, q.db.GetUserByID, q.db.UpdateUserDeletedByID)(ctx, id)
 }
 
 func (q *querier) UpdateUserHashedPassword(ctx context.Context, arg database.UpdateUserHashedPasswordParams) error {

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -1015,17 +1015,10 @@ func (s *MethodTestSuite) TestUser() {
 			LoginType: database.LoginTypeOIDC,
 		}).Asserts(u, rbac.ActionUpdate)
 	}))
-	s.Run("SoftDeleteUserByID", s.Subtest(func(db database.Store, check *expects) {
+	s.Run("UpdateUserDeletedByID", s.Subtest(func(db database.Store, check *expects) {
 		u := dbgen.User(s.T(), db, database.User{})
 		check.Args(u.ID).Asserts(u, rbac.ActionDelete).Returns()
 	}))
-	s.Run("UpdateUserDeletedByID", s.Subtest(func(db database.Store, check *expects) {
-		u := dbgen.User(s.T(), db, database.User{Deleted: true})
-		check.Args(database.UpdateUserDeletedByIDParams{
-			ID:      u.ID,
-			Deleted: true,
-		}).Asserts(u, rbac.ActionDelete).Returns()
-	}))
 	s.Run("UpdateUserHashedPassword", s.Subtest(func(db database.Store, check *expects) {
 		u := dbgen.User(s.T(), db, database.User{})
 		check.Args(database.UpdateUserHashedPasswordParams{

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -311,10 +311,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {
 	}
 
 	if orig.Deleted {
-		err = db.UpdateUserDeletedByID(genCtx, database.UpdateUserDeletedByIDParams{
-			ID:      user.ID,
-			Deleted: orig.Deleted,
-		})
+		err = db.UpdateUserDeletedByID(genCtx, user.ID)
 		require.NoError(t, err, "set user as deleted")
 	}
 	return user

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -733,6 +733,18 @@ func isNotNull(v interface{}) bool {
 	return reflect.ValueOf(v).FieldByName("Valid").Bool()
 }
 
+// Took the error from the real database.
+var deletedUserLinkError = &pq.Error{
+	Severity: "ERROR",
+	// "raise_exception" error
+	Code:    "P0001",
+	Message: "Cannot create user_link for deleted user",
+	Where:   "PL/pgSQL function insert_user_links_fail_if_user_deleted() line 7 at RAISE",
+	File:    "pl_exec.c",
+	Line:    "3864",
+	Routine: "exec_stmt_raise",
+}
+
 func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error {
 	return xerrors.New("AcquireLock must only be called within a transaction")
 }
@@ -5560,6 +5572,10 @@ func (q *FakeQuerier) InsertUserLink(_ context.Context, args database.InsertUser
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
+	if u, err := q.getUserByIDNoLock(args.UserID); err == nil && u.Deleted {
+		return database.UserLink{}, deletedUserLinkError
+	}
+
 	//nolint:gosimple
 	link := database.UserLink{
 		UserID:                 args.UserID,
@@ -6724,33 +6740,22 @@ func (q *FakeQuerier) UpdateUserAppearanceSettings(_ context.Context, arg databa
 	return database.User{}, sql.ErrNoRows
 }
 
-func (q *FakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.UpdateUserDeletedByIDParams) error {
-	if err := validateDatabaseType(params); err != nil {
-		return err
-	}
-
+func (q *FakeQuerier) UpdateUserDeletedByID(_ context.Context, id uuid.UUID) error {
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
 	for i, u := range q.users {
-		if u.ID == params.ID {
-			u.Deleted = params.Deleted
+		if u.ID == id {
+			u.Deleted = true
 			q.users[i] = u
 			// NOTE: In the real world, this is done by a trigger.
-			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++
-			}
+			q.apiKeys = slices.DeleteFunc(q.apiKeys, func(u database.APIKey) bool {
+				return id == u.UserID
+			})
+
+			q.userLinks = slices.DeleteFunc(q.userLinks, func(u database.UserLink) bool {
+				return id == u.UserID
+			})
 			return nil
 		}
 	}
@@ -6804,6 +6809,10 @@ func (q *FakeQuerier) UpdateUserLink(_ context.Context, params database.UpdateUs
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
+	if u, err := q.getUserByIDNoLock(params.UserID); err == nil && u.Deleted {
+		return database.UserLink{}, deletedUserLinkError
+	}
+
 	for i, link := range q.userLinks {
 		if link.UserID == params.UserID && link.LoginType == params.LoginType {
 			link.OAuthAccessToken = params.OAuthAccessToken

diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000194_trigger_delete_user_user_link.down.sql b/coderd/database/migrations/000194_trigger_delete_user_user_link.down.sql
@@ -0,0 +1,26 @@
+DROP TRIGGER IF EXISTS trigger_update_users ON users;
+DROP FUNCTION IF EXISTS delete_deleted_user_resources;
+
+DROP TRIGGER IF EXISTS trigger_upsert_user_links ON user_links;
+DROP FUNCTION IF EXISTS insert_user_links_fail_if_user_deleted;
+
+-- Restore the previous trigger
+CREATE FUNCTION delete_deleted_user_api_keys() RETURNS trigger
+	LANGUAGE plpgsql
+AS $$
+DECLARE
+BEGIN
+	IF (NEW.deleted) THEN
+		DELETE FROM api_keys
+		WHERE user_id = OLD.id;
+	END IF;
+	RETURN NEW;
+END;
+$$;
+
+
+CREATE TRIGGER trigger_update_users
+	AFTER INSERT OR UPDATE ON users
+	FOR EACH ROW
+	WHEN (NEW.deleted = true)
+EXECUTE PROCEDURE delete_deleted_user_api_keys();
diff --git a/coderd/database/migrations/000194_trigger_delete_user_user_link.up.sql b/coderd/database/migrations/000194_trigger_delete_user_user_link.up.sql
@@ -0,0 +1,66 @@
+-- We need to delete all existing user_links for soft-deleted users
+DELETE FROM
+	user_links
+WHERE
+	user_id
+		IN (
+		SELECT id FROM users WHERE deleted
+	);
+
+-- Drop the old trigger
+DROP TRIGGER trigger_update_users ON users;
+-- Drop the old function
+DROP FUNCTION delete_deleted_user_api_keys;
+
+-- When we soft-delete a user, we also want to delete their API key.
+-- The previous function deleted all api keys. This extends that with user_links.
+CREATE FUNCTION delete_deleted_user_resources() RETURNS trigger
+	LANGUAGE plpgsql
+AS $$
+DECLARE
+BEGIN
+	IF (NEW.deleted) THEN
+		-- Remove their api_keys
+		DELETE FROM api_keys
+		WHERE user_id = OLD.id;
+
+		-- Remove their user_links
+		-- Their login_type is preserved in the users table.
+		-- Matching this user back to the link can still be done by their
+		-- email if the account is undeleted. Although that is not a guarantee.
+		DELETE FROM user_links
+		WHERE user_id = OLD.id;
+	END IF;
+	RETURN NEW;
+END;
+$$;
+
+
+-- Update it to the new trigger
+CREATE TRIGGER trigger_update_users
+	AFTER INSERT OR UPDATE ON users
+	FOR EACH ROW
+	WHEN (NEW.deleted = true)
+EXECUTE PROCEDURE delete_deleted_user_resources();
+
+
+-- Prevent adding new user_links for soft-deleted users
+CREATE FUNCTION insert_user_links_fail_if_user_deleted() RETURNS trigger
+	LANGUAGE plpgsql
+AS $$
+
+DECLARE
+BEGIN
+	IF (NEW.user_id IS NOT NULL) THEN
+		IF (SELECT deleted FROM users WHERE id = NEW.user_id LIMIT 1) THEN
+			RAISE EXCEPTION 'Cannot create user_link for deleted user';
+		END IF;
+	END IF;
+	RETURN NEW;
+END;
+$$;
+
+CREATE TRIGGER trigger_upsert_user_links
+	BEFORE INSERT OR UPDATE ON user_links
+	FOR EACH ROW
+EXECUTE PROCEDURE insert_user_links_fail_if_user_deleted();
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql
@@ -116,7 +116,7 @@ WHERE
 UPDATE
 	users
 SET
-	deleted = $2
+	deleted = true
 WHERE
 	id = $1;
-Original file line number
+Diff line change
@@ Expand Up / @@ -116,7 +116,7 @@ WHERE @@
     UPDATE
     	users
     SET
-    	deleted = $2
+    	deleted = true
     WHERE
     	id = $1;
@@ Expand Down @@