diff --git a/coderd/apikey_test.go b/coderd/apikey_test.go index ed9bfae86c731..412f7bebae660 100644 --- a/coderd/apikey_test.go +++ b/coderd/apikey_test.go @@ -195,7 +195,7 @@ func TestSessionExpiry(t *testing.T) { } } -func TestAPIKey(t *testing.T) { +func TestAPIKey_OK(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -206,3 +206,20 @@ func TestAPIKey(t *testing.T) { require.NoError(t, err) require.Greater(t, len(res.Key), 2) } + +func TestAPIKey_Deleted(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + _, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + require.NoError(t, client.DeleteUser(context.Background(), anotherUser.ID)) + + // Attempt to create an API key for the deleted user. This should fail. + _, err := client.CreateAPIKey(ctx, anotherUser.Username) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) +} diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index 98b03514a2f24..7f80385af2dfb 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -931,6 +931,13 @@ func (q *fakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.U if u.ID == params.ID { u.Deleted = params.Deleted q.users[i] = u + // NOTE: In the real world, this is done by a trigger. + for i, k := range q.apiKeys { + if k.UserID == u.ID { + q.apiKeys[i] = q.apiKeys[len(q.apiKeys)-1] + q.apiKeys = q.apiKeys[:len(q.apiKeys)-1] + } + } return nil } } @@ -2768,6 +2775,12 @@ func (q *fakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP arg.LifetimeSeconds = 86400 } + for _, u := range q.users { + if u.ID == arg.UserID && u.Deleted { + return database.APIKey{}, xerrors.Errorf("refusing to create APIKey for deleted user") + } + } + //nolint:gosimple key := database.APIKey{ ID: arg.ID, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 50a27bc0ede48..bb8c12d2bc654 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -129,6 +129,34 @@ CREATE TYPE workspace_transition AS ENUM ( 'delete' ); +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 FUNCTION insert_apikey_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 API key for deleted user'; + END IF; + END IF; + RETURN NEW; +END; +$$; + CREATE TABLE api_keys ( id text NOT NULL, hashed_secret bytea NOT NULL, @@ -895,6 +923,10 @@ CREATE INDEX workspace_resources_job_id_idx ON workspace_resources USING btree ( CREATE UNIQUE INDEX workspaces_owner_id_lower_idx ON workspaces USING btree (owner_id, lower((name)::text)) WHERE (deleted = false); +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(); + ALTER TABLE ONLY api_keys ADD CONSTRAINT api_keys_user_id_uuid_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000120_trigger_delete_user_apikey.down.sql b/coderd/database/migrations/000120_trigger_delete_user_apikey.down.sql new file mode 100644 index 0000000000000..f5c8592c44948 --- /dev/null +++ b/coderd/database/migrations/000120_trigger_delete_user_apikey.down.sql @@ -0,0 +1,9 @@ +BEGIN; + +DROP TRIGGER IF EXISTS trigger_update_users ON users; +DROP FUNCTION IF EXISTS delete_deleted_user_api_keys; + +DROP TRIGGER IF EXISTS trigger_insert_apikeys ON api_keys; +DROP FUNCTION IF EXISTS insert_apikey_fail_if_user_deleted; + +COMMIT; diff --git a/coderd/database/migrations/000120_trigger_delete_user_apikey.up.sql b/coderd/database/migrations/000120_trigger_delete_user_apikey.up.sql new file mode 100644 index 0000000000000..9ea208bef4b51 --- /dev/null +++ b/coderd/database/migrations/000120_trigger_delete_user_apikey.up.sql @@ -0,0 +1,55 @@ +BEGIN; + +-- We need to delete all existing API keys for soft-deleted users. +DELETE FROM + api_keys +WHERE + user_id +IN ( + SELECT id FROM users WHERE deleted +); + + +-- When we soft-delete a user, we also want to delete their API key. +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(); + +-- When we insert a new api key, we want to fail if the user is soft-deleted. +CREATE FUNCTION insert_apikey_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 API key for deleted user'; + END IF; + END IF; + RETURN NEW; +END; +$$; + +CREATE TRIGGER trigger_insert_apikeys +BEFORE INSERT ON api_keys +FOR EACH ROW +EXECUTE PROCEDURE insert_apikey_fail_if_user_deleted(); + +COMMIT; diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index ca6c5b49ce4c2..1723edb51ef32 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -28,6 +28,36 @@ import ( "github.com/coder/coder/testutil" ) +func TestUserLogin(t *testing.T) { + t.Parallel() + t.Run("OK", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + _, err := anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: "SomeSecurePassword!", + }) + require.NoError(t, err) + }) + t.Run("UserDeleted", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + client.DeleteUser(context.Background(), anotherUser.ID) + _, err := anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: "SomeSecurePassword!", + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + }) +} + func TestUserAuthMethods(t *testing.T) { t.Parallel() t.Run("Password", func(t *testing.T) { diff --git a/coderd/users_test.go b/coderd/users_test.go index eef4cb2312ee4..8dd5ad11d615c 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -285,7 +285,7 @@ func TestDeleteUser(t *testing.T) { user := coderdtest.CreateFirstUser(t, client) authz := coderdtest.AssertRBAC(t, api, client) - _, another := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + anotherClient, another := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) err := client.DeleteUser(context.Background(), another.ID) require.NoError(t, err) // Attempt to create a user with the same email and username, and delete them again. @@ -299,6 +299,13 @@ func TestDeleteUser(t *testing.T) { err = client.DeleteUser(context.Background(), another.ID) require.NoError(t, err) + // IMPORTANT: assert that the deleted user's session is no longer valid. + _, err = anotherClient.User(context.Background(), codersdk.Me) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + // RBAC checks authz.AssertChecked(t, rbac.ActionCreate, rbac.ResourceUser) authz.AssertChecked(t, rbac.ActionDelete, another)