Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit2dac342

Browse files
authored
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
1 parentb342bd7 commit2dac342

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
returndeleteQ(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-
returnq.db.UpdateUserDeletedByID(ctx, database.UpdateUserDeletedByIDParams{
613-
ID:id,
614-
Deleted:true,
615-
})
616-
}
617-
returndeleteQ(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
returndeleteQ(q.log,q.auth,q.db.GetWorkspaceByID,func(ctx context.Context,id uuid.UUID)error {
622612
returnq.db.UpdateWorkspaceDeletedByID(ctx, database.UpdateWorkspaceDeletedByIDParams{
@@ -2881,16 +2871,8 @@ func (q *querier) UpdateUserAppearanceSettings(ctx context.Context, arg database
28812871
returnq.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-
returnq.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-
returndeleteQ(q.log,q.auth,fetch,q.db.UpdateUserDeletedByID)(ctx,arg)
2874+
func (q*querier)UpdateUserDeletedByID(ctx context.Context,id uuid.UUID)error {
2875+
returndeleteQ(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
iforig.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
returnuser

‎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
returnreflect.ValueOf(v).FieldByName("Valid").Bool()
734734
}
735735

736+
// Took the error from the real database.
737+
vardeletedUserLinkError=&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
returnxerrors.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
deferq.mutex.Unlock()
55625574

5575+
ifu,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-
iferr:=validateDatabaseType(params);err!=nil {
6729-
returnerr
6730-
}
6731-
6743+
func (q*FakeQuerier)UpdateUserDeletedByID(_ context.Context,id uuid.UUID)error {
67326744
q.mutex.Lock()
67336745
deferq.mutex.Unlock()
67346746

67356747
fori,u:=rangeq.users {
6736-
ifu.ID==params.ID {
6737-
u.Deleted=params.Deleted
6748+
ifu.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-
ifi>=len(q.apiKeys) {
6743-
break
6744-
}
6745-
k:=q.apiKeys[i]
6746-
ifk.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+
returnid==u.UserID
6754+
})
6755+
6756+
q.userLinks=slices.DeleteFunc(q.userLinks,func(u database.UserLink)bool {
6757+
returnid==u.UserID
6758+
})
67546759
returnnil
67556760
}
67566761
}
@@ -6804,6 +6809,10 @@ func (q *FakeQuerier) UpdateUserLink(_ context.Context, params database.UpdateUs
68046809
q.mutex.Lock()
68056810
deferq.mutex.Unlock()
68066811

6812+
ifu,err:=q.getUserByIDNoLock(params.UserID);err==nil&&u.Deleted {
6813+
return database.UserLink{},deletedUserLinkError
6814+
}
6815+
68076816
fori,link:=rangeq.userLinks {
68086817
iflink.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 aboutcustomizing 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 aboutcustomizing 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 aboutcustomizing how changed files appear on GitHub.
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
DROPTRIGGER IF EXISTS trigger_update_usersON users;
2+
DROPFUNCTION IF EXISTS delete_deleted_user_resources;
3+
4+
DROPTRIGGER IF EXISTS trigger_upsert_user_linksON user_links;
5+
DROPFUNCTION IF EXISTS insert_user_links_fail_if_user_deleted;
6+
7+
-- Restore the previous trigger
8+
CREATEFUNCTIONdelete_deleted_user_api_keys() RETURNS trigger
9+
LANGUAGE plpgsql
10+
AS $$
11+
DECLARE
12+
BEGIN
13+
IF (NEW.deleted) THEN
14+
DELETEFROM api_keys
15+
WHERE user_id=OLD.id;
16+
END IF;
17+
RETURN NEW;
18+
END;
19+
$$;
20+
21+
22+
CREATETRIGGERtrigger_update_users
23+
AFTER INSERTORUPDATEON users
24+
FOR EACH ROW
25+
WHEN (NEW.deleted= true)
26+
EXECUTE PROCEDURE delete_deleted_user_api_keys();
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+
DELETEFROM
3+
user_links
4+
WHERE
5+
user_id
6+
IN (
7+
SELECT idFROM usersWHERE deleted
8+
);
9+
10+
-- Drop the old trigger
11+
DROPTRIGGER trigger_update_usersON users;
12+
-- Drop the old function
13+
DROPFUNCTION 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+
CREATEFUNCTIONdelete_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+
DELETEFROM 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+
DELETEFROM 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+
CREATETRIGGERtrigger_update_users
41+
AFTER INSERTORUPDATEON 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+
CREATEFUNCTIONinsert_user_links_fail_if_user_deleted() RETURNS trigger
49+
LANGUAGE plpgsql
50+
AS $$
51+
52+
DECLARE
53+
BEGIN
54+
IF (NEW.user_idIS NOT NULL) THEN
55+
IF (SELECT deletedFROM usersWHERE id=NEW.user_idLIMIT1) 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+
CREATETRIGGERtrigger_upsert_user_links
64+
BEFORE INSERTORUPDATEON user_links
65+
FOR EACH ROW
66+
EXECUTE PROCEDURE insert_user_links_fail_if_user_deleted();

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp