Skip to content

Commit 8fc8559

Browse files
johnstcnjohnstcn
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 were not 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 parent ad82a60 commit 8fc8559

7 files changed

+165
-2
lines changed

‎coderd/apikey_test.go

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

198-
func TestAPIKey(t *testing.T) {
198+
func TestAPIKey_OK(t *testing.T) {
199199
t.Parallel()
200200
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
201201
defer cancel()
@@ -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+
func TestAPIKey_Deleted(t *testing.T) {
211+
t.Parallel()
212+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
213+
defer cancel()
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+
var apiErr *codersdk.Error
223+
require.ErrorAs(t, err, &apiErr)
224+
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
225+
}

‎coderd/database/dbfake/databasefake.go

+13
Original file line numberDiff line numberDiff line change
@@ -931,6 +931,13 @@ func (q *fakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.U
931931
if u.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+
for i, k := range q.apiKeys {
936+
if k.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
return nil
935942
}
936943
}
@@ -2768,6 +2775,12 @@ func (q *fakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP
27682775
arg.LifetimeSeconds = 86400
27692776
}
27702777

2778+
for _, u := range q.users {
2779+
if u.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

+32
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000120_trigger_delete_user_apikey.down.sql

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
BEGIN;
2+
3+
DROP TRIGGER IF EXISTS trigger_update_users ON users;
4+
DROP FUNCTION IF EXISTS delete_deleted_user_api_keys;
5+
6+
DROP TRIGGER IF EXISTS trigger_insert_apikeys ON api_keys;
7+
DROP FUNCTION IF EXISTS insert_apikey_fail_if_user_deleted;
8+
9+
COMMIT;

‎coderd/database/migrations/000120_trigger_delete_user_apikey.up.sql

+55
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+
DELETE FROM
5+
api_keys
6+
WHERE
7+
user_id
8+
IN (
9+
SELECT id FROM users WHERE deleted
10+
);
11+
12+
13+
-- When we soft-delete a user, we also want to delete their API key.
14+
CREATE FUNCTION delete_deleted_user_api_keys() RETURNS trigger
15+
LANGUAGE plpgsql
16+
AS $$
17+
DECLARE
18+
BEGIN
19+
IF (NEW.deleted) THEN
20+
DELETE FROM api_keys
21+
WHERE user_id = OLD.id;
22+
END IF;
23+
RETURN NEW;
24+
END;
25+
$$;
26+
27+
28+
CREATE TRIGGER trigger_update_users
29+
AFTER INSERT OR UPDATE ON 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+
CREATE FUNCTION insert_apikey_fail_if_user_deleted() RETURNS trigger
36+
LANGUAGE plpgsql
37+
AS $$
38+
39+
DECLARE
40+
BEGIN
41+
IF (NEW.user_id IS NOT NULL) THEN
42+
IF (SELECT deleted FROM users WHERE id = NEW.user_id LIMIT 1) 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+
CREATE TRIGGER trigger_insert_apikeys
51+
BEFORE INSERT ON api_keys
52+
FOR EACH ROW
53+
EXECUTE PROCEDURE insert_apikey_fail_if_user_deleted();
54+
55+
COMMIT;

‎coderd/userauth_test.go

+30
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,36 @@ import (
2828
"github.com/coder/coder/testutil"
2929
)
3030

31+
func TestUserLogin(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+
var apiErr *codersdk.Error
56+
require.ErrorAs(t, err, &apiErr)
57+
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
58+
})
59+
}
60+
3161
func TestUserAuthMethods(t *testing.T) {
3262
t.Parallel()
3363
t.Run("Password", func(t *testing.T) {

‎coderd/users_test.go

+8-1
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+
var apiErr *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)