Skip to content

Commit 2dac342

Browse files
EmyrkEmyrk
authored
fix: add postgres triggers to remove deleted users from user_links (coder#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
1 parent b342bd7 commit 2dac342

File tree

16 files changed

+200
-89
lines changed

16 files changed

+200
-89
lines changed

‎cli/delete_test.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111

1212
"github.com/coder/coder/v2/cli/clitest"
1313
"github.com/coder/coder/v2/coderd/coderdtest"
14-
"github.com/coder/coder/v2/coderd/database"
1514
"github.com/coder/coder/v2/coderd/database/dbauthz"
1615
"github.com/coder/coder/v2/codersdk"
1716
"github.com/coder/coder/v2/pty/ptytest"
@@ -95,10 +94,7 @@ func TestDelete(t *testing.T) {
9594
// this way.
9695
ctx := testutil.Context(t, testutil.WaitShort)
9796
// nolint:gocritic // Unit test
98-
err := api.Database.UpdateUserDeletedByID(dbauthz.AsSystemRestricted(ctx), database.UpdateUserDeletedByIDParams{
99-
ID: deleteMeUser.ID,
100-
Deleted: true,
101-
})
97+
err := api.Database.UpdateUserDeletedByID(dbauthz.AsSystemRestricted(ctx), deleteMeUser.ID)
10298
require.NoError(t, err)
10399

104100
inv, root := clitest.New(t, "delete", fmt.Sprintf("%s/%s", deleteMeUser.ID, workspace.Name), "-y", "--orphan")

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -607,16 +607,6 @@ func (q *querier) SoftDeleteTemplateByID(ctx context.Context, id uuid.UUID) erro
607607
return deleteQ(q.log, q.auth, q.db.GetTemplateByID, deleteF)(ctx, id)
608608
}
609609

610-
func (q *querier) SoftDeleteUserByID(ctx context.Context, id uuid.UUID) error {
611-
deleteF := func(ctx context.Context, id uuid.UUID) error {
612-
return q.db.UpdateUserDeletedByID(ctx, database.UpdateUserDeletedByIDParams{
613-
ID: id,
614-
Deleted: true,
615-
})
616-
}
617-
return deleteQ(q.log, q.auth, q.db.GetUserByID, deleteF)(ctx, id)
618-
}
619-
620610
func (q *querier) SoftDeleteWorkspaceByID(ctx context.Context, id uuid.UUID) error {
621611
return deleteQ(q.log, q.auth, q.db.GetWorkspaceByID, func(ctx context.Context, id uuid.UUID) error {
622612
return q.db.UpdateWorkspaceDeletedByID(ctx, database.UpdateWorkspaceDeletedByIDParams{
@@ -2881,16 +2871,8 @@ func (q *querier) UpdateUserAppearanceSettings(ctx context.Context, arg database
28812871
return q.db.UpdateUserAppearanceSettings(ctx, arg)
28822872
}
28832873

2884-
// UpdateUserDeletedByID
2885-
// Deprecated: Delete this function in favor of 'SoftDeleteUserByID'. Deletes are
2886-
// irreversible.
2887-
func (q *querier) UpdateUserDeletedByID(ctx context.Context, arg database.UpdateUserDeletedByIDParams) error {
2888-
fetch := func(ctx context.Context, arg database.UpdateUserDeletedByIDParams) (database.User, error) {
2889-
return q.db.GetUserByID(ctx, arg.ID)
2890-
}
2891-
// This uses the rbac.ActionDelete action always as this function should always delete.
2892-
// We should delete this function in favor of 'SoftDeleteUserByID'.
2893-
return deleteQ(q.log, q.auth, fetch, q.db.UpdateUserDeletedByID)(ctx, arg)
2874+
func (q *querier) UpdateUserDeletedByID(ctx context.Context, id uuid.UUID) error {
2875+
return deleteQ(q.log, q.auth, q.db.GetUserByID, q.db.UpdateUserDeletedByID)(ctx, id)
28942876
}
28952877

28962878
func (q *querier) UpdateUserHashedPassword(ctx context.Context, arg database.UpdateUserHashedPasswordParams) error {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,17 +1015,10 @@ func (s *MethodTestSuite) TestUser() {
10151015
LoginType: database.LoginTypeOIDC,
10161016
}).Asserts(u, rbac.ActionUpdate)
10171017
}))
1018-
s.Run("SoftDeleteUserByID", s.Subtest(func(db database.Store, check *expects) {
1018+
s.Run("UpdateUserDeletedByID", s.Subtest(func(db database.Store, check *expects) {
10191019
u := dbgen.User(s.T(), db, database.User{})
10201020
check.Args(u.ID).Asserts(u, rbac.ActionDelete).Returns()
10211021
}))
1022-
s.Run("UpdateUserDeletedByID", s.Subtest(func(db database.Store, check *expects) {
1023-
u := dbgen.User(s.T(), db, database.User{Deleted: true})
1024-
check.Args(database.UpdateUserDeletedByIDParams{
1025-
ID: u.ID,
1026-
Deleted: true,
1027-
}).Asserts(u, rbac.ActionDelete).Returns()
1028-
}))
10291022
s.Run("UpdateUserHashedPassword", s.Subtest(func(db database.Store, check *expects) {
10301023
u := dbgen.User(s.T(), db, database.User{})
10311024
check.Args(database.UpdateUserHashedPasswordParams{

‎coderd/database/dbgen/dbgen.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -311,10 +311,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {
311311
}
312312

313313
if orig.Deleted {
314-
err = db.UpdateUserDeletedByID(genCtx, database.UpdateUserDeletedByIDParams{
315-
ID: user.ID,
316-
Deleted: orig.Deleted,
317-
})
314+
err = db.UpdateUserDeletedByID(genCtx, user.ID)
318315
require.NoError(t, err, "set user as deleted")
319316
}
320317
return user

‎coderd/database/dbmem/dbmem.go

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -733,6 +733,18 @@ func isNotNull(v interface{}) bool {
733733
return reflect.ValueOf(v).FieldByName("Valid").Bool()
734734
}
735735

736+
// Took the error from the real database.
737+
var deletedUserLinkError = &pq.Error{
738+
Severity: "ERROR",
739+
// "raise_exception" error
740+
Code: "P0001",
741+
Message: "Cannot create user_link for deleted user",
742+
Where: "PL/pgSQL function insert_user_links_fail_if_user_deleted() line 7 at RAISE",
743+
File: "pl_exec.c",
744+
Line: "3864",
745+
Routine: "exec_stmt_raise",
746+
}
747+
736748
func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error {
737749
return xerrors.New("AcquireLock must only be called within a transaction")
738750
}
@@ -5560,6 +5572,10 @@ func (q *FakeQuerier) InsertUserLink(_ context.Context, args database.InsertUser
55605572
q.mutex.Lock()
55615573
defer q.mutex.Unlock()
55625574

5575+
if u, err := q.getUserByIDNoLock(args.UserID); err == nil && u.Deleted {
5576+
return database.UserLink{}, deletedUserLinkError
5577+
}
5578+
55635579
//nolint:gosimple
55645580
link := database.UserLink{
55655581
UserID: args.UserID,
@@ -6724,33 +6740,22 @@ func (q *FakeQuerier) UpdateUserAppearanceSettings(_ context.Context, arg databa
67246740
return database.User{}, sql.ErrNoRows
67256741
}
67266742

6727-
func (q *FakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.UpdateUserDeletedByIDParams) error {
6728-
if err := validateDatabaseType(params); err != nil {
6729-
return err
6730-
}
6731-
6743+
func (q *FakeQuerier) UpdateUserDeletedByID(_ context.Context, id uuid.UUID) error {
67326744
q.mutex.Lock()
67336745
defer q.mutex.Unlock()
67346746

67356747
for i, u := range q.users {
6736-
if u.ID == params.ID {
6737-
u.Deleted = params.Deleted
6748+
if u.ID == id {
6749+
u.Deleted = true
67386750
q.users[i] = u
67396751
// NOTE: In the real world, this is done by a trigger.
6740-
i := 0
6741-
for {
6742-
if i >= len(q.apiKeys) {
6743-
break
6744-
}
6745-
k := q.apiKeys[i]
6746-
if k.UserID == u.ID {
6747-
q.apiKeys[i] = q.apiKeys[len(q.apiKeys)-1]
6748-
q.apiKeys = q.apiKeys[:len(q.apiKeys)-1]
6749-
// We removed an element, so decrement
6750-
i--
6751-
}
6752-
i++
6753-
}
6752+
q.apiKeys = slices.DeleteFunc(q.apiKeys, func(u database.APIKey) bool {
6753+
return id == u.UserID
6754+
})
6755+
6756+
q.userLinks = slices.DeleteFunc(q.userLinks, func(u database.UserLink) bool {
6757+
return id == u.UserID
6758+
})
67546759
return nil
67556760
}
67566761
}
@@ -6804,6 +6809,10 @@ func (q *FakeQuerier) UpdateUserLink(_ context.Context, params database.UpdateUs
68046809
q.mutex.Lock()
68056810
defer q.mutex.Unlock()
68066811

6812+
if u, err := q.getUserByIDNoLock(params.UserID); err == nil && u.Deleted {
6813+
return database.UserLink{}, deletedUserLinkError
6814+
}
6815+
68076816
for i, link := range q.userLinks {
68086817
if link.UserID == params.UserID && link.LoginType == params.LoginType {
68096818
link.OAuthAccessToken = params.OAuthAccessToken

‎coderd/database/dbmetrics/dbmetrics.go

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dump.sql

Lines changed: 27 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000194_trigger_delete_user_user_link.down.sql

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
DROP TRIGGER IF EXISTS trigger_update_users ON users;
2+
DROP FUNCTION IF EXISTS delete_deleted_user_resources;
3+
4+
DROP TRIGGER IF EXISTS trigger_upsert_user_links ON user_links;
5+
DROP FUNCTION IF EXISTS insert_user_links_fail_if_user_deleted;
6+
7+
-- Restore the previous trigger
8+
CREATE FUNCTION delete_deleted_user_api_keys() RETURNS trigger
9+
LANGUAGE plpgsql
10+
AS $$
11+
DECLARE
12+
BEGIN
13+
IF (NEW.deleted) THEN
14+
DELETE FROM api_keys
15+
WHERE user_id = OLD.id;
16+
END IF;
17+
RETURN NEW;
18+
END;
19+
$$;
20+
21+
22+
CREATE TRIGGER trigger_update_users
23+
AFTER INSERT OR UPDATE ON users
24+
FOR EACH ROW
25+
WHEN (NEW.deleted = true)
26+
EXECUTE PROCEDURE delete_deleted_user_api_keys();

‎coderd/database/migrations/000194_trigger_delete_user_user_link.up.sql

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
-- We need to delete all existing user_links for soft-deleted users
2+
DELETE FROM
3+
user_links
4+
WHERE
5+
user_id
6+
IN (
7+
SELECT id FROM users WHERE deleted
8+
);
9+
10+
-- Drop the old trigger
11+
DROP TRIGGER trigger_update_users ON users;
12+
-- Drop the old function
13+
DROP FUNCTION delete_deleted_user_api_keys;
14+
15+
-- When we soft-delete a user, we also want to delete their API key.
16+
-- The previous function deleted all api keys. This extends that with user_links.
17+
CREATE FUNCTION delete_deleted_user_resources() RETURNS trigger
18+
LANGUAGE plpgsql
19+
AS $$
20+
DECLARE
21+
BEGIN
22+
IF (NEW.deleted) THEN
23+
-- Remove their api_keys
24+
DELETE FROM api_keys
25+
WHERE user_id = OLD.id;
26+
27+
-- Remove their user_links
28+
-- Their login_type is preserved in the users table.
29+
-- Matching this user back to the link can still be done by their
30+
-- email if the account is undeleted. Although that is not a guarantee.
31+
DELETE FROM user_links
32+
WHERE user_id = OLD.id;
33+
END IF;
34+
RETURN NEW;
35+
END;
36+
$$;
37+
38+
39+
-- Update it to the new trigger
40+
CREATE TRIGGER trigger_update_users
41+
AFTER INSERT OR UPDATE ON users
42+
FOR EACH ROW
43+
WHEN (NEW.deleted = true)
44+
EXECUTE PROCEDURE delete_deleted_user_resources();
45+
46+
47+
-- Prevent adding new user_links for soft-deleted users
48+
CREATE FUNCTION insert_user_links_fail_if_user_deleted() RETURNS trigger
49+
LANGUAGE plpgsql
50+
AS $$
51+
52+
DECLARE
53+
BEGIN
54+
IF (NEW.user_id IS NOT NULL) THEN
55+
IF (SELECT deleted FROM users WHERE id = NEW.user_id LIMIT 1) THEN
56+
RAISE EXCEPTION 'Cannot create user_link for deleted user';
57+
END IF;
58+
END IF;
59+
RETURN NEW;
60+
END;
61+
$$;
62+
63+
CREATE TRIGGER trigger_upsert_user_links
64+
BEFORE INSERT OR UPDATE ON user_links
65+
FOR EACH ROW
66+
EXECUTE PROCEDURE insert_user_links_fail_if_user_deleted();

0 commit comments

Comments
 (0)