Repository navigation
chore: address remaining PR feedback for types, ESM resolution, and tech debt #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
152cbae
2ed8e49
568ff94
0ecf9b3
726d346
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,11 +18,12 @@ This document tracks known technical debt items that should be addressed in futu | |||||||||||
| - The `session` table currently exposes raw `ipAddress` and `userAgent` fields indefinitely. | ||||||||||||
| - There is no automated cleanup or anonymization of this Personally Identifiable Information (PII). | ||||||||||||
|
|
||||||||||||
| **Recommended Solution**: | ||||||||||||
| **Recommended Solution (Time-bounded retention model chosen)**: | ||||||||||||
|
|
||||||||||||
| - Update the schema with `anonymizeIp` and `anonymizeUserAgent` helpers to hash/truncate data before insert. | ||||||||||||
| - Create a `background` (or `jobs`) module in `apps/api` using `@nestjs/schedule`. | ||||||||||||
| - Implement `runPIICleanup` to run daily, finding sessions older than 30 days and anonymizing their PII, emitting audit logs. | ||||||||||||
| - Keep raw IP/user-agent on insert for security auditing. | ||||||||||||
| - Remove or rename the legacy `anonymizeIp` and `anonymizeUserAgent` pre-insert helpers if they exist. | ||||||||||||
| - Implement `runPIICleanup` inside the `background`/`jobs` module using `@nestjs/schedule` to run daily. | ||||||||||||
| - This job will find sessions older than 30 days and anonymize their PII (nullify or hash), emitting audit logs. | ||||||||||||
|
Comment on lines
+25
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid documenting this as an in-process cron without a singleton guarantee. Putting Suggested doc change-- Implement `runPIICleanup` inside the `background`/`jobs` module using `@nestjs/schedule` to run daily.
+- Implement `runPIICleanup` as a singleton scheduled task.
+- If `@nestjs/schedule` is used, guard execution with leader election or a distributed lock; otherwise run it from a dedicated worker/queue so only one instance performs the daily cleanup.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| - Add a Drizzle migration to backfill and anonymize existing old sessions. | ||||||||||||
|
|
||||||||||||
| ### 2. Permission Caching Architecture | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| export abstract class EncryptionService { | ||
| abstract decrypt(val: string): Promise<string>; | ||
| abstract encrypt(val: string): Promise<string>; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,18 +6,22 @@ | |||||||
| "types": "dist/index.d.ts", | ||||||||
| "exports": { | ||||||||
| ".": { | ||||||||
| "types": "./dist/index.d.ts", | ||||||||
| "import": "./dist/index.js", | ||||||||
| "default": "./dist/index.js" | ||||||||
| }, | ||||||||
| "./schema/identity": { | ||||||||
| "types": "./dist/schema/identity.d.ts", | ||||||||
| "import": "./dist/schema/identity.js", | ||||||||
| "default": "./dist/schema/identity.js" | ||||||||
| }, | ||||||||
| "./schema/tenant": { | ||||||||
| "types": "./dist/schema/tenant.d.ts", | ||||||||
| "import": "./dist/schema/tenant.js", | ||||||||
| "default": "./dist/schema/tenant.js" | ||||||||
| }, | ||||||||
| "./dist/schema/identity": { | ||||||||
| "types": "./dist/schema/identity.d.ts", | ||||||||
| "import": "./dist/schema/identity.js", | ||||||||
| "default": "./dist/schema/identity.js" | ||||||||
| } | ||||||||
|
|
@@ -38,4 +42,4 @@ | |||||||
| "drizzle-kit": "^0.31.8", | ||||||||
| "typescript": "^5.7.3" | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Minor: Missing trailing newline. The file ends without a trailing newline. POSIX convention and most linters expect files to end with a newline character. 🔧 Add trailing newline }
-}
+}
+📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -128,7 +128,7 @@ | |||||||||||||||||||||||||||||||||||||
| it("create delegates to auth provider", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const user = await adapter.create({ | ||||||||||||||||||||||||||||||||||||||
| email: "a@b.com", | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -140,7 +140,7 @@ | |||||||||||||||||||||||||||||||||||||
| it("update handles password via auth provider; updates fields; throws if missing user after update", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // password path | ||||||||||||||||||||||||||||||||||||||
| db.query.user.findFirst.mockResolvedValueOnce(mkUser({ id: "u1" })); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -158,7 +158,7 @@ | |||||||||||||||||||||||||||||||||||||
| expect(res.name).toBe("A"); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // missing setPassword support | ||||||||||||||||||||||||||||||||||||||
| const adapter2 = new DrizzleUserAdapter(db, mkOptions(), { | ||||||||||||||||||||||||||||||||||||||
| createUser: (input: CreateUserInput) => auth.createUser(input), | ||||||||||||||||||||||||||||||||||||||
| } as unknown as IAuthProvider); | ||||||||||||||||||||||||||||||||||||||
| await expect( | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -169,35 +169,44 @@ | |||||||||||||||||||||||||||||||||||||
| db.update.mockClear(); | ||||||||||||||||||||||||||||||||||||||
| db.query.user.findFirst.mockResolvedValueOnce(mkUser({ id: "u1" })); | ||||||||||||||||||||||||||||||||||||||
| await adapter.update("u1", { password: "pw" } as UpdateUserInput); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.update).not.toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| it("delete cascades and related tables in a transaction", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Access the transaction mock to verify cascade behavior | ||||||||||||||||||||||||||||||||||||||
| const txCalls: string[] = []; | ||||||||||||||||||||||||||||||||||||||
| const txCalls: any[] = []; | ||||||||||||||||||||||||||||||||||||||
| db.transaction.mockImplementation((fn: (tx: MockTx) => unknown) => { | ||||||||||||||||||||||||||||||||||||||
| const tx = { | ||||||||||||||||||||||||||||||||||||||
| delete: vi.fn().mockImplementation(() => { | ||||||||||||||||||||||||||||||||||||||
| txCalls.push("delete"); | ||||||||||||||||||||||||||||||||||||||
| delete: vi.fn().mockImplementation((table: any) => { | ||||||||||||||||||||||||||||||||||||||
| txCalls.push(table); | ||||||||||||||||||||||||||||||||||||||
| return { where: vi.fn().mockReturnThis() }; | ||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||
| } as unknown as MockTx; | ||||||||||||||||||||||||||||||||||||||
| return fn(tx); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| await adapter.delete("u1"); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.transaction).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| expect(txCalls.length).toBeGreaterThan(0); | ||||||||||||||||||||||||||||||||||||||
| expect(txCalls).toHaveLength(5); | ||||||||||||||||||||||||||||||||||||||
| expect(txCalls).toEqual([ | ||||||||||||||||||||||||||||||||||||||
| schema.member, | ||||||||||||||||||||||||||||||||||||||
| schema.invitation, | ||||||||||||||||||||||||||||||||||||||
| schema.session, | ||||||||||||||||||||||||||||||||||||||
| schema.account, | ||||||||||||||||||||||||||||||||||||||
| schema.user, | ||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| it("findById and findByEmail return mapped or null", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| auth.findById = vi.fn().mockResolvedValue(mkUser({ id: "u1" })); | ||||||||||||||||||||||||||||||||||||||
| const byId = await adapter.findById("u1"); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -210,7 +219,7 @@ | |||||||||||||||||||||||||||||||||||||
| const authNoFind = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||||||||||||||||||||||||||||||||||||||
| delete (authNoFind as any).findById; | ||||||||||||||||||||||||||||||||||||||
| const adapterFallback = new DrizzleUserAdapter(db, mkOptions(), authNoFind); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| db.query.user.findFirst.mockResolvedValueOnce(mkUser({ id: "u2" })); | ||||||||||||||||||||||||||||||||||||||
| const byIdFallback = await adapterFallback.findById("u2"); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -240,7 +249,7 @@ | |||||||||||||||||||||||||||||||||||||
| it("deleteIfNotLastAdmin handles various scenarios", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const mockSelect = vi.fn(); | ||||||||||||||||||||||||||||||||||||||
| const mockTx = { | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -367,21 +376,23 @@ | |||||||||||||||||||||||||||||||||||||
| it("forceVerifyEmail updates user", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| await adapter.forceVerifyEmail("u1"); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.update).toHaveBeenCalledWith(schema.user); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.set).toHaveBeenCalledWith( | ||||||||||||||||||||||||||||||||||||||
| expect.objectContaining({ emailVerified: true }), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.where).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| it("count returns total users with optional filtering", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Mock count result | ||||||||||||||||||||||||||||||||||||||
| const mockCountResult = [{ count: 5 }]; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -393,19 +404,21 @@ | |||||||||||||||||||||||||||||||||||||
| // 1. Global count | ||||||||||||||||||||||||||||||||||||||
| const total = await adapter.count(); | ||||||||||||||||||||||||||||||||||||||
| expect(total).toBe(5); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.innerJoin).not.toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // 2. Tenant count | ||||||||||||||||||||||||||||||||||||||
| db.innerJoin.mockClear(); | ||||||||||||||||||||||||||||||||||||||
| const totalTenant = await adapter.count({ tenantId: "t1" }); | ||||||||||||||||||||||||||||||||||||||
| expect(totalTenant).toBe(5); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(db.innerJoin).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| it("findAll builds search filters correctly", async () => { | ||||||||||||||||||||||||||||||||||||||
| const db = mkDb(); | ||||||||||||||||||||||||||||||||||||||
| const auth = mkAuth(); | ||||||||||||||||||||||||||||||||||||||
| const adapter = new DrizzleUserAdapter(db, mkOptions(), auth); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const dataChain = { | ||||||||||||||||||||||||||||||||||||||
| from: vi.fn().mockReturnThis(), | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -430,6 +443,7 @@ | |||||||||||||||||||||||||||||||||||||
| await adapter.findAll({ search: "test", limit: 10 }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(dataChain.where).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(countChain.where).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
@@ -466,9 +480,9 @@ | |||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| await adapter.findAll({ tenantId: "t1" }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| expect(dataChain.innerJoin).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| expect(countChain.innerJoin).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| expect(dataChain.where).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| expect(countChain.where).toHaveBeenCalled(); | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
482
to
487
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert tenant filtering on the count query too. This spec verifies Suggested fix expect(dataChain.innerJoin).toHaveBeenCalled();
expect(countChain.innerJoin).toHaveBeenCalled();
expect(dataChain.where).toHaveBeenCalled();
+ expect(countChain.where).toHaveBeenCalled();📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify one irreversible cleanup strategy.
nullify or hashis too ambiguous for a compliance-sensitive path. Those options have different privacy properties, and a hash can still leave the data linkable. Please document a single approved transformation and explicitly forbid raw IP/user-agent from being copied into the audit logs produced by this job.Suggested doc change