chore(seed): stop relying on user team provision triggers#2413
chore(seed): stop relying on user team provision triggers#2413ben-fornefeld wants to merge 6 commits intomainfrom
Conversation
ben-fornefeld
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit ed6c0b6. Bugbot is set up for automated code reviews on this repo. Configure here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| func (s *APIStore) PostAdminUsersBootstrap(c *gin.Context) { | ||
| func (s *APIStore) PostAdminUsersUserIdBootstrap(c *gin.Context, userId api.UserId) { | ||
| ctx := c.Request.Context() |
There was a problem hiding this comment.
The userId from the path parameter is passed directly to bootstrapUser without verifying the user exists in public.users. Previously, auth.MustGetUserID(c) implicitly proved user existence via JWT validation. If a caller supplies a UUID with no corresponding public.users row (fabricated, or because UpsertPublicUser has not run yet), bootstrapUser will hit a DB-level error. Depending on how handleProvisioningError maps that, the response may be a generic 500 instead of a 404/422. Since the sync trigger is now removed, the bootstrap endpoint depends on the caller sequencing UpsertPublicUser before this call. Consider either returning a clear 404 when the user is not found, or calling UpsertPublicUser idempotently inside bootstrapUser so the endpoint is self-contained.
There was a problem hiding this comment.
This finding is stale for #2413. The admin bootstrap route change was split into #2414, so this path is no longer part of the trigger-migration PR.
![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: 0add9142aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| DROP TRIGGER IF EXISTS sync_inserts_to_public_users ON auth.users; | ||
| DROP TRIGGER IF EXISTS sync_updates_to_public_users ON auth.users; | ||
| DROP TRIGGER IF EXISTS sync_deletes_to_public_users ON auth.users; |
There was a problem hiding this comment.
This migration removes the auth.users→public.users sync triggers, but the only replacement path is an optional background worker (packages/dashboard-api/internal/cfg/model.go:24 defaults ENABLE_AUTH_USER_SYNC_BACKGROUND_WORKER to false, and packages/dashboard-api/main.go:230 only starts it when enabled). In environments using defaults, inserts/updates/deletes in auth.users will no longer propagate, leaving stale/missing public.users rows and causing downstream FK-dependent writes (e.g. access tokens or memberships) to fail for new users.
Useful? React with 👍 / 👎.
| team, err := s.bootstrapUser(ctx, userId) | ||
| if err != nil { | ||
| s.handleProvisioningError(ctx, c, "bootstrap user", err) |
There was a problem hiding this comment.
Now that userId comes from the URL path, callers can pass IDs that do not exist in auth.users; in that case bootstrapUser returns a not-found error, but this path forwards it to handleProvisioningError, which maps non-ProvisionError failures to HTTP 500. That turns invalid/stale user IDs into server errors instead of 4xx responses, causing avoidable retries and misleading operational alerts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This finding is stale for #2413. The POST /admin/users/{userId}/bootstrap change was split into #2414, so this route behavior is no longer part of the trigger-migration PR.
| func (s *APIStore) PostAdminUsersUserIdBootstrap(c *gin.Context, userId api.UserId) { | ||
| ctx := c.Request.Context() | ||
| telemetry.ReportEvent(ctx, "bootstrap user") | ||
|
|
||
| userID := auth.MustGetUserID(c) | ||
| team, err := s.bootstrapUser(ctx, userID) | ||
| team, err := s.bootstrapUser(ctx, userId) | ||
| if err != nil { | ||
| s.handleProvisioningError(ctx, c, "bootstrap user", err) | ||
|
|
There was a problem hiding this comment.
🔴 When an admin calls POST /admin/users/{userId}/bootstrap with a UUID that has no corresponding row in auth.users, the endpoint returns HTTP 500 instead of 404. Before this PR, auth.MustGetUserID(c) required a valid JWT which implicitly proved the user existed; now that userId comes from the URL path with only admin-token auth, any arbitrary UUID can be submitted and the missing not-found check causes the generic 500 fallback to fire.
Extended reasoning...
What the bug is and how it manifests
PostAdminUsersUserIdBootstrap passes the URL path parameter userId directly to bootstrapUser(). The first thing bootstrapUser does is call s.supabaseDB.Write.GetAuthUserByID(ctx, userID). If the UUID has no matching row in auth.users, this returns pgx.ErrNoRows (a sqlc :one query). The function wraps it with fmt.Errorf("get auth user: %w", err) and returns it to the caller.
The specific code path that triggers it
PostAdminUsersUserIdBootstrap (team_provisioning.go:43-50) calls s.handleProvisioningError on the returned error. handleProvisioningError checks errors.As(err, &provisionErr) for a *ProvisionError — the wrapped pgx.ErrNoRows is not a *ProvisionError, so it falls through to the generic branch and returns HTTP 500 with message "Failed to bootstrap user".
Why existing code does not prevent it
Before this PR the endpoint was POST /admin/users/bootstrap and its OpenAPI security required both AdminTokenAuth AND Supabase1TokenAuth. A Supabase JWT is only issued for real auth users, so auth.MustGetUserID(c) could only produce a UUID that already existed in auth.users. The new endpoint drops the Supabase requirement and takes userId purely from the URL path with only admin-token auth, so any UUID — including fabricated ones — can reach bootstrapUser.
What the impact would be
An admin automation or CLI that bootstraps a user whose sign-up did not complete, or supplies a typo UUID, receives a confusing 500 instead of a meaningful 404. Monitoring systems alert on 5xx traffic that is actually a 4xx input error.
How to fix it
In bootstrapUser, after GetAuthUserByID fails, check dberrors.IsNotFoundError(err) (the same helper used correctly in auth_user_sync.go line 102) and return a *ProvisionError with StatusCode: http.StatusNotFound. handleProvisioningError will then map it to a 404 response.
Step-by-step proof
- Admin sends POST /admin/users/00000000-0000-0000-0000-000000000099/bootstrap with a valid X-Admin-Token header.
- PostAdminUsersUserIdBootstrap extracts userId = 00000000-...-0099 and calls bootstrapUser(ctx, userId).
- bootstrapUser calls GetAuthUserByID(ctx, userId); pgx executes SELECT from auth.users WHERE id = , finds no row, and returns pgx.ErrNoRows.
- bootstrapUser wraps it and returns: fmt.Errorf("get auth user: %w", pgx.ErrNoRows).
- PostAdminUsersUserIdBootstrap calls handleProvisioningError with that error.
- errors.As(err, &provisionErr) returns false because it is not a *ProvisionError.
- The generic branch fires: response is HTTP 500 {"code":500,"message":"Failed to bootstrap user"} instead of 404.
🔬 also observed by claude
There was a problem hiding this comment.
This finding is stale for #2413. The admin-bootstrap endpoint change was split into #2414, so the 404/500 behavior for path-supplied user IDs is tracked there rather than in this migration PR.
ben-fornefeld
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed6c0b6. Configure here.
| err = authDb.Write.UpsertPublicUser(ctx, authqueries.UpsertPublicUserParams{ | ||
| ID: userID, | ||
| Email: email, | ||
| }) |
There was a problem hiding this comment.
Removed trigger-created team cleanup causes duplicate teams
High Severity
The INSERT INTO auth.users at line 138 still fires the sync_inserts_to_public_users trigger, which inserts into public.users, which fires the post_user_signup trigger (moved to public.users by migration 20260316130000). That trigger creates a default team and users_teams row. The old code explicitly deleted this trigger-created team before creating the seed team, but that cleanup step was removed. The UpsertPublicUser call at line 145 is effectively a no-op since the row already exists from the sync trigger, and it doesn't suppress post_user_signup. The seed now produces two teams and two users_teams entries (both with is_default = true), breaking the deterministic single-team setup the seed intends to create.
Reviewed by Cursor Bugbot for commit ed6c0b6. Configure here.
There was a problem hiding this comment.
seeding runs either manually on cluster creation or in integration tests. we can life with this until merging #2421 right after
2 participants
Summary
public.usersrows explicitly instead of relying on database-side auth user sync triggersValidation
go test ./packages/dashboard-api/internal/handlers ./packages/db/scripts/seed/postgres ./packages/local-dev ./tests/integration/internal/utils ./tests/integration