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

Commit8fc8559

Browse files
authored
fix(coderd): ensure that user API keys are deleted when a user is (#7270)
Fixes an issue where API tokens belonging to a deleted user werenot invalidated:- Adds a trigger to delete rows from the api_key stable when the column deleted is set to true in the users table.- Adds a trigger to the api_keys table to ensure that new rows may not be added where user_id corresponds to a deleted user.- Adds a migration to delete all API keys from deleted users.- Adds tests + dbfake implementation for the above.
1 parentad82a60 commit8fc8559

File tree

7 files changed

+165
-2
lines changed

7 files changed

+165
-2
lines changed

‎coderd/apikey_test.go‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func TestSessionExpiry(t *testing.T) {
195195
}
196196
}
197197

198-
funcTestAPIKey(t*testing.T) {
198+
funcTestAPIKey_OK(t*testing.T) {
199199
t.Parallel()
200200
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong)
201201
defercancel()
@@ -206,3 +206,20 @@ func TestAPIKey(t *testing.T) {
206206
require.NoError(t,err)
207207
require.Greater(t,len(res.Key),2)
208208
}
209+
210+
funcTestAPIKey_Deleted(t*testing.T) {
211+
t.Parallel()
212+
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong)
213+
defercancel()
214+
client:=coderdtest.New(t,&coderdtest.Options{IncludeProvisionerDaemon:true})
215+
user:=coderdtest.CreateFirstUser(t,client)
216+
_,anotherUser:=coderdtest.CreateAnotherUser(t,client,user.OrganizationID)
217+
require.NoError(t,client.DeleteUser(context.Background(),anotherUser.ID))
218+
219+
// Attempt to create an API key for the deleted user. This should fail.
220+
_,err:=client.CreateAPIKey(ctx,anotherUser.Username)
221+
require.Error(t,err)
222+
varapiErr*codersdk.Error
223+
require.ErrorAs(t,err,&apiErr)
224+
require.Equal(t,http.StatusBadRequest,apiErr.StatusCode())
225+
}

‎coderd/database/dbfake/databasefake.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -931,6 +931,13 @@ func (q *fakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.U
931931
ifu.ID==params.ID {
932932
u.Deleted=params.Deleted
933933
q.users[i]=u
934+
// NOTE: In the real world, this is done by a trigger.
935+
fori,k:=rangeq.apiKeys {
936+
ifk.UserID==u.ID {
937+
q.apiKeys[i]=q.apiKeys[len(q.apiKeys)-1]
938+
q.apiKeys=q.apiKeys[:len(q.apiKeys)-1]
939+
}
940+
}
934941
returnnil
935942
}
936943
}
@@ -2768,6 +2775,12 @@ func (q *fakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP
27682775
arg.LifetimeSeconds=86400
27692776
}
27702777

2778+
for_,u:=rangeq.users {
2779+
ifu.ID==arg.UserID&&u.Deleted {
2780+
return database.APIKey{},xerrors.Errorf("refusing to create APIKey for deleted user")
2781+
}
2782+
}
2783+
27712784
//nolint:gosimple
27722785
key:= database.APIKey{
27732786
ID:arg.ID,

‎coderd/database/dump.sql‎

Lines changed: 32 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
BEGIN;
2+
3+
DROPTRIGGER IF EXISTS trigger_update_usersON users;
4+
DROPFUNCTION IF EXISTS delete_deleted_user_api_keys;
5+
6+
DROPTRIGGER IF EXISTS trigger_insert_apikeysON api_keys;
7+
DROPFUNCTION IF EXISTS insert_apikey_fail_if_user_deleted;
8+
9+
COMMIT;
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
BEGIN;
2+
3+
-- We need to delete all existing API keys for soft-deleted users.
4+
DELETEFROM
5+
api_keys
6+
WHERE
7+
user_id
8+
IN (
9+
SELECT idFROM usersWHERE deleted
10+
);
11+
12+
13+
-- When we soft-delete a user, we also want to delete their API key.
14+
CREATEFUNCTIONdelete_deleted_user_api_keys() RETURNS trigger
15+
LANGUAGE plpgsql
16+
AS $$
17+
DECLARE
18+
BEGIN
19+
IF (NEW.deleted) THEN
20+
DELETEFROM api_keys
21+
WHERE user_id=OLD.id;
22+
END IF;
23+
RETURN NEW;
24+
END;
25+
$$;
26+
27+
28+
CREATETRIGGERtrigger_update_users
29+
AFTER INSERTORUPDATEON users
30+
FOR EACH ROW
31+
WHEN (NEW.deleted= true)
32+
EXECUTE PROCEDURE delete_deleted_user_api_keys();
33+
34+
-- When we insert a new api key, we want to fail if the user is soft-deleted.
35+
CREATEFUNCTIONinsert_apikey_fail_if_user_deleted() RETURNS trigger
36+
LANGUAGE plpgsql
37+
AS $$
38+
39+
DECLARE
40+
BEGIN
41+
IF (NEW.user_idIS NOT NULL) THEN
42+
IF (SELECT deletedFROM usersWHERE id=NEW.user_idLIMIT1) THEN
43+
RAISE EXCEPTION'Cannot create API key for deleted user';
44+
END IF;
45+
END IF;
46+
RETURN NEW;
47+
END;
48+
$$;
49+
50+
CREATETRIGGERtrigger_insert_apikeys
51+
BEFORE INSERTON api_keys
52+
FOR EACH ROW
53+
EXECUTE PROCEDURE insert_apikey_fail_if_user_deleted();
54+
55+
COMMIT;

‎coderd/userauth_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,36 @@ import (
2828
"github.com/coder/coder/testutil"
2929
)
3030

31+
funcTestUserLogin(t*testing.T) {
32+
t.Parallel()
33+
t.Run("OK",func(t*testing.T) {
34+
t.Parallel()
35+
client:=coderdtest.New(t,nil)
36+
user:=coderdtest.CreateFirstUser(t,client)
37+
anotherClient,anotherUser:=coderdtest.CreateAnotherUser(t,client,user.OrganizationID)
38+
_,err:=anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{
39+
Email:anotherUser.Email,
40+
Password:"SomeSecurePassword!",
41+
})
42+
require.NoError(t,err)
43+
})
44+
t.Run("UserDeleted",func(t*testing.T) {
45+
t.Parallel()
46+
client:=coderdtest.New(t,nil)
47+
user:=coderdtest.CreateFirstUser(t,client)
48+
anotherClient,anotherUser:=coderdtest.CreateAnotherUser(t,client,user.OrganizationID)
49+
client.DeleteUser(context.Background(),anotherUser.ID)
50+
_,err:=anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{
51+
Email:anotherUser.Email,
52+
Password:"SomeSecurePassword!",
53+
})
54+
require.Error(t,err)
55+
varapiErr*codersdk.Error
56+
require.ErrorAs(t,err,&apiErr)
57+
require.Equal(t,http.StatusUnauthorized,apiErr.StatusCode())
58+
})
59+
}
60+
3161
funcTestUserAuthMethods(t*testing.T) {
3262
t.Parallel()
3363
t.Run("Password",func(t*testing.T) {

‎coderd/users_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ func TestDeleteUser(t *testing.T) {
285285
user:=coderdtest.CreateFirstUser(t,client)
286286
authz:=coderdtest.AssertRBAC(t,api,client)
287287

288-
_,another:=coderdtest.CreateAnotherUser(t,client,user.OrganizationID)
288+
anotherClient,another:=coderdtest.CreateAnotherUser(t,client,user.OrganizationID)
289289
err:=client.DeleteUser(context.Background(),another.ID)
290290
require.NoError(t,err)
291291
// Attempt to create a user with the same email and username, and delete them again.
@@ -299,6 +299,13 @@ func TestDeleteUser(t *testing.T) {
299299
err=client.DeleteUser(context.Background(),another.ID)
300300
require.NoError(t,err)
301301

302+
// IMPORTANT: assert that the deleted user's session is no longer valid.
303+
_,err=anotherClient.User(context.Background(),codersdk.Me)
304+
require.Error(t,err)
305+
varapiErr*codersdk.Error
306+
require.ErrorAs(t,err,&apiErr)
307+
require.Equal(t,http.StatusUnauthorized,apiErr.StatusCode())
308+
302309
// RBAC checks
303310
authz.AssertChecked(t,rbac.ActionCreate,rbac.ResourceUser)
304311
authz.AssertChecked(t,rbac.ActionDelete,another)

0 commit comments

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