Repository navigation
chore(auth): drop password-auth enum values via text-column conversion (preserves hash chains) - #1064
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR removes two PostgreSQL enum types ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d2ac1791b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/lib/src/auth/__tests__/session-abuse-vectors.test.ts (1)
389-413: LGTM — reason-label swap aligned with sibling test.Cosmetic change;
reasonis free-form. Minor nit while you're in the file: the section header at line 169 still readsTOKEN VERSION MISMATCH (PASSWORD CHANGE, FORCED LOGOUT)and the inline comment at line 188 references "password change, etc." — since the product is passwordless-only now, you may want to retire those references (e.g.,FORCED LOGOUT / TOKEN ROTATION) to keep the file consistent with the PR's intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/auth/__tests__/session-abuse-vectors.test.ts` around lines 389 - 413, Update the obsolete header and inline comment text to remove password-specific wording: replace the section header string "TOKEN VERSION MISMATCH (PASSWORD CHANGE, FORCED LOGOUT)" with a neutral label such as "TOKEN VERSION MISMATCH (FORCED LOGOUT / TOKEN ROTATION)" and update the nearby inline comment that currently references "password change, etc." to reference "forced logout or token rotation" (locate these exact strings in the tests to change the text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lib/src/auth/__tests__/session-abuse-vectors.test.ts`:
- Around line 389-413: Update the obsolete header and inline comment text to
remove password-specific wording: replace the section header string "TOKEN
VERSION MISMATCH (PASSWORD CHANGE, FORCED LOGOUT)" with a neutral label such as
"TOKEN VERSION MISMATCH (FORCED LOGOUT / TOKEN ROTATION)" and update the nearby
inline comment that currently references "password change, etc." to reference
"forced logout or token rotation" (locate these exact strings in the tests to
change the text).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64ee6dca-8e5e-4c5b-90bd-1b581eb1364e
📒 Files selected for processing (16)
apps/web/src/app/admin/audit-logs/page.tsxapps/web/src/services/api/rollback-service.tspackages/db/drizzle/0105_drop_password_auth_enums.sqlpackages/db/drizzle/meta/0105_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/monitoring.tspackages/db/src/schema/security-audit.tspackages/lib/src/audit/security-audit.tspackages/lib/src/auth/__tests__/rate-limit-utils.test.tspackages/lib/src/auth/__tests__/session-abuse-vectors.test.tspackages/lib/src/auth/__tests__/session-service-unit.test.tspackages/lib/src/auth/rate-limit-utils.tspackages/lib/src/monitoring/activity-logger.tspackages/lib/src/permissions/__tests__/rollback-permissions.test.tspackages/lib/src/security/__tests__/distributed-rate-limit.test.tspackages/lib/src/security/distributed-rate-limit.ts
💤 Files with no reviewable changes (10)
- packages/lib/src/auth/tests/rate-limit-utils.test.ts
- packages/lib/src/permissions/tests/rollback-permissions.test.ts
- packages/lib/src/audit/security-audit.ts
- packages/db/src/schema/monitoring.ts
- packages/db/src/schema/security-audit.ts
- packages/lib/src/auth/rate-limit-utils.ts
- packages/lib/src/security/tests/distributed-rate-limit.test.ts
- packages/lib/src/monitoring/activity-logger.ts
- apps/web/src/app/admin/audit-logs/page.tsx
- packages/lib/src/security/distributed-rate-limit.ts
1d2ac17 to
6750da7
Compare
… + drop legacy password values Password authentication was removed from PageSpace (passwordless-only: passkey + magic link). Vestigial enum values on activity_operation and security_event_type had no live consumers but blocked a clean schema. Schema + code changes replicated from commit a3013a9 (originally on pu/docs-audit, extracted here so the migration ships atomically with the schema edit): - activity_operation: drop 'password_change' - security_event_type: drop auth.password.{changed,reset.requested,reset.completed} - Remove SecurityAuditService.logPasswordChanged() - Remove RATE_LIMIT_CONFIGS.PASSWORD_RESET and DISTRIBUTED_RATE_LIMITS.PASSWORD_RESET - Drop 'password_change' from ActivityOperation type union and UI filter arrays Also updates two pre-existing session-test fixtures that passed 'password_change' / 'password_changed' as a free-form 'reason' string to revokeAllUserSessions() — switched to 'admin_action' (a neighboring test already uses that value) so grepping for password refs comes back clean. Migration 0105_drop_password_auth_enums.sql converts the three consuming columns to text and drops the enum types: activity_logs.operation (was activity_operation, now text) activity_logs.rollbackSourceOperation (was activity_operation, now text) security_audit_log.event_type (was security_event_type, now text) Why text columns instead of DELETE + rename-and-swap: both tables are tamper-evident hash chains. computeLogHash (activity-logger.ts) and computeSecurityEventHash (security-audit.ts) include the column value in the hashed payload, and the chain verifiers (hash-chain-verifier.ts, security-audit-chain-verifier.ts) require each row's previousHash to match the immediately prior row's stored hash. Either DELETE-ing or UPDATE-ing affected rows would make the verifiers report chain breaks on unmodified data. Converting the column to text preserves every stored value verbatim so the chains remain valid without verifier changes. Writer-side type safety is preserved: ActivityOperation (already a string-literal union) and SecurityEventType (now a hand-written union replacing the Drizzle enumValues-derived type) continue to constrain callers at the TS layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6750da7 to
c72ef7a
Compare
Summary
Extracts the code-only portion of commit
a3013a93(originally onpu/docs-audit, PR #1063) onto its own branch and adds the Postgres migration required to actually deploy the enum drops.pu/docs-auditkeeps its 6 marketing-docs files; on merge of this PR to master,pu/docs-auditrebases cleanly.Migration approach — why text columns, not DELETE + rename-and-swap
Both tables are tamper-evident hash chains.
activity_logs.operationis part ofcomputeLogHash(packages/lib/src/monitoring/activity-logger.ts) andsecurity_audit_log.event_typeis part ofcomputeSecurityEventHash(packages/lib/src/audit/security-audit.ts). Both chain verifiers (hash-chain-verifier.ts,security-audit-chain-verifier.ts) require each row'spreviousHashto match the immediately prior row's stored hash.DELETEon orphan rows → next surviving row'spreviousHashpoints to a removed predecessor → verifier reports a chain break on every deleted segment, on unmodified data.UPDATE event_typeto another value → storedeventHashno longer matches recomputed hash → verifier reports hash mismatches on every rewritten row.Converting the column to text preserves every stored value verbatim, so the chains stay valid and the
/api/cron/verify-audit-chainjob keeps passing without any verifier changes. Writer-side type safety is preserved viaActivityOperation(already a string-literal union) andSecurityEventType(now a hand-written union inpackages/db/src/schema/security-audit.tsreplacing theenumValues-derived type).Tradeoff: the DB no longer enforces enum membership at
INSERTtime. TS is the source of truth —ActivityOperation/SecurityEventTypetypes constrain all in-repo writers.What changed
Schema (2 files) — drop the
pgEnumdeclarations, change the consuming columns totext:packages/db/src/schema/monitoring.ts— dropactivityOperationEnum;activity_logs.operationandactivity_logs.rollbackSourceOperationbecometext.packages/db/src/schema/security-audit.ts— dropsecurityEventTypeEnum;security_audit_log.event_typebecomestext.SecurityEventTypenow a hand-written string-literal union.DB package exports —
packages/db/src/index.tsdrops both enum re-exports.SecurityEventTypeexport preserved.Schema test —
packages/db/src/schema/__tests__/schema-definitions.test.tsdrops the two enum-existence assertions.Code (9 files, from
a3013a93) — remove dead hooks:SecurityAuditService.logPasswordChanged()RATE_LIMIT_CONFIGS.PASSWORD_RESET,DISTRIBUTED_RATE_LIMITS.PASSWORD_RESETActivityOperationtype union entryTest-label cleanup (2 files, not in
a3013a93) — two pre-existing session tests passed'password_change'/'password_changed'as a free-formreasonstring torevokeAllUserSessions(); switched to'admin_action'(a neighboring test already uses that value) so grepping for password refs comes back clean. Cosmetic only;reasonis a free-formtextcolumn, not an enum.Migration (new file) —
packages/db/drizzle/0105_drop_password_auth_enums.sql:No DELETEs, no UPDATEs on hashed rows.
drizzle-kit generatedetects the column-type changes and produces a compatible snapshot (meta/0105_snapshot.json); the hand-written SQL adds explicitUSINGcasts and theDROP TYPEstatements that Drizzle doesn't emit.Verification checklist (reviewer can run locally)
Then a dry-run of
0105_drop_password_auth_enums.sqlagainst staging before production apply. The migration is non-destructive — no rows are deleted or modified — so rollback if needed is a matter of re-creating the enums and casting columns back (new migration), not restoring data.Rollback note
Re-creating the dropped enum types and casting the columns back requires a follow-up migration; but since no data was deleted or rewritten, existing values cast back cleanly (as long as no new non-enum values were inserted in the meantime).
Summary by CodeRabbit
Release Notes