fix: move MFA email OTP trigger out of auth schema#1800
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DB as auth.mfa_factors
participant Trigger as trg_enforce_email_otp_for_mfa
participant Func as public.enforce_email_otp_for_mfa()
participant Helper as public.get_mfa_email_otp_enforced_at()
participant UserSec as public.user_security
Client->>DB: INSERT / UPDATE auth.mfa_factors
DB->>Trigger: BEFORE INSERT OR UPDATE
Trigger->>Func: invoke enforcement function
Func->>Helper: compute enforced_at
Func->>UserSec: check latest email_otp_verified_at for user
alt recent verified OTP exists and meets enforced_at
Func-->>Trigger: allow operation
Trigger-->>DB: proceed
DB-->>Client: success
else missing or stale verification
Func-->>Trigger: RAISE EXCEPTION
Trigger-->>Client: error (insert/update aborted)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bce881d61
ℹ️ 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".
| DROP TRIGGER IF EXISTS trg_enforce_email_otp_for_mfa ON auth.mfa_factors; | ||
|
|
||
| CREATE TRIGGER trg_enforce_email_otp_for_mfa | ||
| BEFORE INSERT OR UPDATE ON auth.mfa_factors |
There was a problem hiding this comment.
This migration now executes DROP TRIGGER/CREATE TRIGGER on auth.mfa_factors without any insufficient_privilege handling, so environments where migrations cannot manage auth-owned objects (the same case previously handled in 20260312000000_remove_rbac_security_settings_singletons.sql) will hard-fail the entire migration instead of degrading gracefully. That turns a schema cleanup into a deploy blocker for restricted Supabase setups; wrapping these statements in a guarded DO ... EXCEPTION WHEN insufficient_privilege block would preserve the prior behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/mfa-email-otp-trigger.test.ts (1)
5-53: Add companion E2E coverage for the user-facing MFA enrollment flow.This file adds strong Postgres-level checks, but the affected flow should also have an end-to-end test to catch integration regressions above SQL/catalog level.
As per coding guidelines "Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql`:
- Around line 59-67: Remove the inner EXCEPTION handler and make the DROP
deterministic by checking schema privileges first; replace the DO block that
wraps EXECUTE 'DROP FUNCTION IF EXISTS auth.enforce_email_otp_for_mfa()' so it
uses has_schema_privilege('auth', '<appropriate_privilege>') to gate the DROP
(and otherwise RAISE NOTICE or RAISE EXCEPTION), thereby eliminating the silent
insufficient_privilege rescue and ensuring auth.enforce_email_otp_for_mfa() is
either dropped or the migration fails/notes lack of permission
deterministically.
- Around line 5-58: Add explicit ACL hardening for the SECURITY DEFINER function
public.enforce_email_otp_for_mfa(): after creating/altering the function, revoke
all privileges from PUBLIC and then grant only the minimal execute privilege to
the owning role (postgres) so the function cannot be invoked by unprivileged
roles; reference the function name public.enforce_email_otp_for_mfa() and the
trigger trg_enforce_email_otp_for_mfa when making the ACL changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c621c378-19a0-4a2c-bd43-50f2b4f43898
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sqltests/mfa-email-otp-trigger.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql`:
- Around line 57-103: Update the privilege and dependency guards: extend
v_can_manage_auth_trigger to also require has_function_privilege(current_user,
'public.enforce_email_otp_for_mfa()', 'EXECUTE') so creating
trg_enforce_email_otp_for_mfa that calls public.enforce_email_otp_for_mfa() is
gated; and before executing DROP FUNCTION auth.enforce_email_otp_for_mfa() in
the v_can_drop_legacy_auth_function block, add a check that no triggers or other
objects reference that auth.enforce_email_otp_for_mfa() (e.g., query
pg_trigger/pg_depend for references to the function oid) and skip/drop only if
there are zero dependents, keeping the existing privilege checks on
v_can_drop_legacy_auth_function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6381f1f5-f911-4fc0-bd77-c68c50af9197
📒 Files selected for processing (2)
supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sqltests/mfa-email-otp-trigger.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/mfa-email-otp-trigger.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql (1)
59-60:⚠️ Potential issue | 🟠 MajorAdd missing trigger-function EXECUTE guard and legacy dependency guard.
Line 60 currently checks schema/table privileges but not EXECUTE on
public.enforce_email_otp_for_mfa(), so Line 68 can still fail.
Also, Line 102 dropsauth.enforce_email_otp_for_mfa()without checking active dependents, which can abort the migration if a trigger/object still references it.Suggested patch
DO $$ DECLARE v_can_manage_auth_trigger boolean := has_schema_privilege(current_user, 'auth', 'USAGE') - AND has_table_privilege(current_user, 'auth.mfa_factors', 'TRIGGER'); + AND has_table_privilege(current_user, 'auth.mfa_factors', 'TRIGGER') + AND has_function_privilege(current_user, 'public.enforce_email_otp_for_mfa()', 'EXECUTE'); BEGIN @@ DO $$ DECLARE @@ v_can_drop_legacy_auth_function boolean := has_schema_privilege(current_user, 'auth', 'USAGE') @@ AND pg_get_userbyid(proc.proowner) = current_user ); + v_legacy_function_still_used boolean := EXISTS ( + SELECT 1 + FROM pg_trigger trg + JOIN pg_proc proc ON proc.oid = trg.tgfoid + JOIN pg_namespace ns ON ns.oid = proc.pronamespace + WHERE ns.nspname = 'auth' + AND proc.proname = 'enforce_email_otp_for_mfa' + AND COALESCE(pg_get_function_identity_arguments(proc.oid), '') = '' + AND NOT trg.tgisinternal + ); BEGIN @@ IF NOT v_can_drop_legacy_auth_function THEN RAISE NOTICE 'Skipping cleanup of auth.enforce_email_otp_for_mfa() (insufficient privileges)'; RETURN; END IF; + + IF v_legacy_function_still_used THEN + RAISE NOTICE 'Skipping cleanup of auth.enforce_email_otp_for_mfa() (still referenced by trigger)'; + RETURN; + END IF; EXECUTE 'DROP FUNCTION auth.enforce_email_otp_for_mfa()'; END; $$;#!/bin/bash set -euo pipefail file="supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql" echo "== Trigger rewrite guard ==" rg -n "v_can_manage_auth_trigger|has_schema_privilege|has_table_privilege|has_function_privilege|CREATE TRIGGER trg_enforce_email_otp_for_mfa" "$file" -C2 echo echo "== Legacy drop guard ==" rg -n "v_can_drop_legacy_auth_function|DROP FUNCTION auth\\.enforce_email_otp_for_mfa\\(\\)|pg_trigger|pg_depend" "$file" -C3 echo echo "Expected:" echo "1) v_can_manage_auth_trigger includes has_function_privilege(..., 'EXECUTE')." echo "2) A dependency-use guard exists before DROP FUNCTION."Also applies to: 67-69, 82-83, 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql` around lines 59 - 60, Extend the trigger-management privilege guard by adding an EXECUTE check for the public function (update v_can_manage_auth_trigger to include has_function_privilege(current_user, 'public.enforce_email_otp_for_mfa()', 'EXECUTE') or equivalent has_function_privilege call) so attempts to CREATE TRIGGER trg_enforce_email_otp_for_mfa or ALTER/CREATE that reference public.enforce_email_otp_for_mfa() will be skipped when EXECUTE is missing; also add a legacy-drop dependency guard before DROP FUNCTION auth.enforce_email_otp_for_mfa() that inspects pg_trigger/pg_depend (or queries pg_catalog to detect any active dependents/triggers referencing auth.enforce_email_otp_for_mfa()) and only run the DROP when no dependents exist to avoid aborting the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql`:
- Around line 59-60: Extend the trigger-management privilege guard by adding an
EXECUTE check for the public function (update v_can_manage_auth_trigger to
include has_function_privilege(current_user,
'public.enforce_email_otp_for_mfa()', 'EXECUTE') or equivalent
has_function_privilege call) so attempts to CREATE TRIGGER
trg_enforce_email_otp_for_mfa or ALTER/CREATE that reference
public.enforce_email_otp_for_mfa() will be skipped when EXECUTE is missing; also
add a legacy-drop dependency guard before DROP FUNCTION
auth.enforce_email_otp_for_mfa() that inspects pg_trigger/pg_depend (or queries
pg_catalog to detect any active dependents/triggers referencing
auth.enforce_email_otp_for_mfa()) and only run the DROP when no dependents exist
to avoid aborting the migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c65d4051-591b-4ff1-b48a-46eb1c5d8de4
📒 Files selected for processing (1)
supabase/migrations/20260316132841_move_mfa_email_otp_trigger_to_public.sql
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/tests/00-supabase_test_helpers.sql (1)
453-461: Consider adding a test for the newmark_email_otp_verifiedhelper.The test plan verifies 7 existing helper functions but doesn't include the newly added
tests.mark_email_otp_verified. Adding a test would ensure the function is properly defined and catch signature changes.📝 Suggested addition
-SELECT plan(7); +SELECT plan(8); SELECT function_returns('tests', 'create_supabase_user', ARRAY['text', 'text', 'text', 'jsonb'], 'uuid'); SELECT function_returns('tests', 'get_supabase_uid', ARRAY['text'], 'uuid'); SELECT function_returns('tests', 'get_supabase_user', ARRAY['text'], 'json'); +SELECT function_returns('tests', 'mark_email_otp_verified', ARRAY['text', 'timestamptz'], 'void'); SELECT function_returns('tests', 'authenticate_as', ARRAY['text'], 'void'); SELECT function_returns('tests', 'clear_authentication', ARRAY[null], 'void'); SELECT function_returns('tests', 'rls_enabled', ARRAY['text', 'text'], 'text'); SELECT function_returns('tests', 'rls_enabled', ARRAY['text'], 'text');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/00-supabase_test_helpers.sql` around lines 453 - 461, The test plan currently lists 7 helpers but omits the new tests.mark_email_otp_verified; update the plan count from 7 to 8 and add a SELECT function_returns entry for the tests.mark_email_otp_verified helper (matching the function's actual parameter types and return type used in your implementation) so the SQL test suite verifies the helper exists and its signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/tests/00-supabase_test_helpers.sql`:
- Around line 453-461: The test plan currently lists 7 helpers but omits the new
tests.mark_email_otp_verified; update the plan count from 7 to 8 and add a
SELECT function_returns entry for the tests.mark_email_otp_verified helper
(matching the function's actual parameter types and return type used in your
implementation) so the SQL test suite verifies the helper exists and its
signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e7baa60-2428-452b-8561-e9cfb634d57d
📒 Files selected for processing (9)
supabase/tests/00-supabase_test_helpers.sqlsupabase/tests/35_test_has_2fa_enabled.sqlsupabase/tests/36_test_check_org_members_2fa_enabled.sqlsupabase/tests/37_test_check_min_rights_2fa_enforcement.sqlsupabase/tests/38_test_get_orgs_v7_2fa_enforcement.sqlsupabase/tests/39_test_reject_access_due_to_2fa.sqlsupabase/tests/41_test_reject_access_due_to_2fa_for_app.sqlsupabase/tests/42_test_reject_access_due_to_2fa_for_org.sqlsupabase/tests/43_test_rbac_permission_2fa.sql
|
CI status on March 16, 2026: the GitHub Actions suite is green, including |
|
1 participant
Summary (AI generated)
authschema intopublicand repointauth.mfa_factorsto it.auth.enforce_email_otp_for_mfa()helper is absent.bun.lockrefresh already present in the workspace diff.Motivation (AI generated)
Supabase does not allow app-managed functions to live in the
authschema in this setup, so the trigger helper must be owned inpublicwhile keeping enforcement onauth.mfa_factors.Business Impact (AI generated)
This keeps MFA enrollment enforcement aligned with Supabase constraints, reducing migration risk around authentication objects and preserving the intended enrollment guard.
Test Plan (AI generated)
bun run supabase:db:resetSUPABASE_DB_URL=postgresql://postgres:postgres@127.0.0.1:63662/postgres bunx vitest run tests/mfa-email-otp-trigger.test.tsbunx eslint tests/mfa-email-otp-trigger.test.tsScreenshots
Checklist
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
Tests
Chores