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.

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())
+}

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,

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;
 

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;

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;

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) {

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)