fix: add postgres triggers to remove deleted users from user_links (#12117)

* chore: add database test fixture to insert non-unique linked_ids
* chore: create unit test to exercise failed email change bug
* fix: add postgres triggers to keep user_links clear of deleted users
* Add migrations to prevent deleted users with links
* Force soft delete of users, do not allow un-delete

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")

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 {

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{

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

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

coderd/database/dbmetrics/dbmetrics.go

@@ -1838,11 +1838,11 @@ func (m metricsStore) UpdateUserAppearanceSettings(ctx context.Context, arg data
 	return r0, r1
 }
 
-func (m metricsStore) UpdateUserDeletedByID(ctx context.Context, arg database.UpdateUserDeletedByIDParams) error {
+func (m metricsStore) UpdateUserDeletedByID(ctx context.Context, id uuid.UUID) error {
 	start := time.Now()
-	err := m.s.UpdateUserDeletedByID(ctx, arg)
+	r0 := m.s.UpdateUserDeletedByID(ctx, id)
 	m.queryLatencies.WithLabelValues("UpdateUserDeletedByID").Observe(time.Since(start).Seconds())
-	return err
+	return r0
 }
 
 func (m metricsStore) UpdateUserHashedPassword(ctx context.Context, arg database.UpdateUserHashedPasswordParams) error {

coderd/database/dbmock/dbmock.go

@@ -3873,7 +3873,7 @@ func (mr *MockStoreMockRecorder) UpdateUserAppearanceSettings(arg0, arg1 any) *g
 }
 
 // UpdateUserDeletedByID mocks base method.
-func (m *MockStore) UpdateUserDeletedByID(arg0 context.Context, arg1 database.UpdateUserDeletedByIDParams) error {
+func (m *MockStore) UpdateUserDeletedByID(arg0 context.Context, arg1 uuid.UUID) error {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "UpdateUserDeletedByID", arg0, arg1)
 	ret0, _ := ret[0].(error)

coderd/database/dump.sql

@@ -187,14 +187,22 @@ CREATE TYPE workspace_transition AS ENUM (
     'delete'
 );
 
-CREATE FUNCTION delete_deleted_user_api_keys() RETURNS trigger
+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;
@@ -215,6 +223,21 @@ BEGIN
 END;
 $$;
 
+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 FUNCTION tailnet_notify_agent_change() RETURNS trigger
     LANGUAGE plpgsql
     AS $$
@@ -1551,7 +1574,9 @@ CREATE TRIGGER tailnet_notify_tunnel_change AFTER INSERT OR DELETE OR UPDATE ON
 
 CREATE TRIGGER trigger_insert_apikeys BEFORE INSERT ON api_keys FOR EACH ROW EXECUTE FUNCTION insert_apikey_fail_if_user_deleted();
 
-CREATE TRIGGER trigger_update_users AFTER INSERT OR UPDATE ON users FOR EACH ROW WHEN ((new.deleted = true)) EXECUTE FUNCTION delete_deleted_user_api_keys();
+CREATE TRIGGER trigger_update_users AFTER INSERT OR UPDATE ON users FOR EACH ROW WHEN ((new.deleted = true)) EXECUTE FUNCTION delete_deleted_user_resources();
+
+CREATE TRIGGER trigger_upsert_user_links BEFORE INSERT OR UPDATE ON user_links FOR EACH ROW EXECUTE FUNCTION insert_user_links_fail_if_user_deleted();
 
 ALTER TABLE ONLY api_keys
     ADD CONSTRAINT api_keys_user_id_uuid_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;

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();

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();

coderd/database/querier.go

@@ -355,7 +355,7 @@ type sqlcQuerier interface {
 	UpdateTemplateVersionExternalAuthProvidersByJobID(ctx context.Context, arg UpdateTemplateVersionExternalAuthProvidersByJobIDParams) error
 	UpdateTemplateWorkspacesLastUsedAt(ctx context.Context, arg UpdateTemplateWorkspacesLastUsedAtParams) error
 	UpdateUserAppearanceSettings(ctx context.Context, arg UpdateUserAppearanceSettingsParams) (User, error)
-	UpdateUserDeletedByID(ctx context.Context, arg UpdateUserDeletedByIDParams) error
+	UpdateUserDeletedByID(ctx context.Context, id uuid.UUID) error
 	UpdateUserHashedPassword(ctx context.Context, arg UpdateUserHashedPasswordParams) error
 	UpdateUserLastSeenAt(ctx context.Context, arg UpdateUserLastSeenAtParams) (User, error)
 	UpdateUserLink(ctx context.Context, arg UpdateUserLinkParams) (UserLink, error)

coderd/database/queries.sql.go

@@ -7894,18 +7894,13 @@ const updateUserDeletedByID = `-- name: UpdateUserDeletedByID :exec
 UPDATE
 	users
 SET
-	deleted = $2
+	deleted = true
 WHERE
 	id = $1
 `
 
-type UpdateUserDeletedByIDParams struct {
-	ID      uuid.UUID `db:"id" json:"id"`
-	Deleted bool      `db:"deleted" json:"deleted"`
-}
-
-func (q *sqlQuerier) UpdateUserDeletedByID(ctx context.Context, arg UpdateUserDeletedByIDParams) error {
-	_, err := q.db.ExecContext(ctx, updateUserDeletedByID, arg.ID, arg.Deleted)
+func (q *sqlQuerier) UpdateUserDeletedByID(ctx context.Context, id uuid.UUID) error {
+	_, err := q.db.ExecContext(ctx, updateUserDeletedByID, id)
 	return err
 }
 

coderd/database/queries/users.sql

@@ -116,7 +116,7 @@ WHERE
 UPDATE
 	users
 SET
-	deleted = $2
+	deleted = true
 WHERE
 	id = $1;
 

coderd/userauth_test.go

@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/golang-jwt/jwt/v4"
@@ -24,6 +25,7 @@ import (
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
 	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/promoauth"
@@ -632,7 +634,7 @@ func TestUserOAuth2Github(t *testing.T) {
 			coderEmail,
 		}
 
-		owner := coderdtest.New(t, &coderdtest.Options{
+		owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
 			Auditor: auditor,
 			GithubOAuth2Config: &coderd.GithubOAuth2Config{
 				AllowSignups:  true,
@@ -655,9 +657,12 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 			},
 		})
-		coderdtest.CreateFirstUser(t, owner)
+		first := coderdtest.CreateFirstUser(t, owner)
 
 		ctx := testutil.Context(t, testutil.WaitLong)
+		ownerUser, err := owner.User(context.Background(), "me")
+		require.NoError(t, err)
+
 		// Create the user, then delete the user, then create again.
 		// This causes the email change to fail.
 		client := codersdk.New(owner.URL)
@@ -668,6 +673,23 @@ func TestUserOAuth2Github(t *testing.T) {
 
 		err = owner.DeleteUser(ctx, deleted.ID)
 		require.NoError(t, err)
+		// Check no user links for the user
+		links, err := db.GetUserLinksByUserID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(ownerUser, first.OrganizationID)), deleted.ID)
+		require.NoError(t, err)
+		require.Empty(t, links)
+
+		// Make sure a user_link cannot be created with a deleted user.
+		// nolint:gocritic // Unit test
+		_, err = db.InsertUserLink(dbauthz.AsSystemRestricted(ctx), database.InsertUserLinkParams{
+			UserID:            deleted.ID,
+			LoginType:         "github",
+			LinkedID:          "100",
+			OAuthAccessToken:  "random",
+			OAuthRefreshToken: "random",
+			OAuthExpiry:       time.Now(),
+			DebugContext:      []byte(`{}`),
+		})
+		require.ErrorContains(t, err, "Cannot create user_link for deleted user")
 
 		// Create the user again.
 		client, _ = fake.Login(t, client, jwt.MapClaims{})

coderd/users.go

@@ -524,10 +524,7 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err = api.Database.UpdateUserDeletedByID(ctx, database.UpdateUserDeletedByIDParams{
-		ID:      user.ID,
-		Deleted: true,
-	})
+	err = api.Database.UpdateUserDeletedByID(ctx, user.ID)
 	if dbauthz.IsNotAuthorizedError(err) {
 		httpapi.Forbidden(rw)
 		return

enterprise/cli/server_dbcrypt_test.go

@@ -236,15 +236,18 @@ func genData(t *testing.T, db database.Store) []database.User {
 					OAuthAccessToken:  "access-" + usr.ID.String(),
 					OAuthRefreshToken: "refresh-" + usr.ID.String(),
 				})
-				// Fun fact: our schema allows _all_ login types to have
-				// a user_link. Even though I'm not sure how it could occur
-				// in practice, making sure to test all combinations here.
-				_ = dbgen.UserLink(t, db, database.UserLink{
-					UserID:            usr.ID,
-					LoginType:         usr.LoginType,
-					OAuthAccessToken:  "access-" + usr.ID.String(),
-					OAuthRefreshToken: "refresh-" + usr.ID.String(),
-				})
+				// Deleted users cannot have user_links
+				if !deleted {
+					// Fun fact: our schema allows _all_ login types to have
+					// a user_link. Even though I'm not sure how it could occur
+					// in practice, making sure to test all combinations here.
+					_ = dbgen.UserLink(t, db, database.UserLink{
+						UserID:            usr.ID,
+						LoginType:         usr.LoginType,
+						OAuthAccessToken:  "access-" + usr.ID.String(),
+						OAuthRefreshToken: "refresh-" + usr.ID.String(),
+					})
+				}
 				users = append(users, usr)
 			}
 		}