Correctness, robustness & hot-path performance #532

New issue

Open

opened 2026-06-15 22:11:29 +02:00 by moritz · 0 comments

moritz commented

2026-06-15 22:11:29 +02:00

Owner

Motivation

A set of genuine correctness/robustness fixes plus two safe hot-path performance wins. Grouped because each
needs focused tests rather than riding along with mechanical cleanup.

Scope

R-008 — last-admin TOCTOU (the substantive item). Close the race where two concurrent admin demotions can
each read admin-count == 2 and both commit, leaving zero admins (lib/accounts/user.ex:477-529). This is NOT a
simple declarative constraint — "admin" is indirect (role_id → roles.permission_set_name == "admin", multiple
admin roles, the system user excluded). Pick a mechanism in /solutions:
- (a) pg_advisory_xact_lock in the admin-mutating action(s) — pragmatic minimum;
- (b) a BEFORE trigger with SELECT … FOR UPDATE + RAISE EXCEPTION 'ash_error: …' — path-independent, fits the
  existing trigger pattern, but needs a sibling trigger on roles;
- (c) SERIALIZABLE transaction + retry.
  Also map the OIDC email-collision constraint error to a friendly message. Note: worst-case zero-admins is
  recoverable via the seed_admin release task.
R-006 — unbounded atom. Remove the rescue in to_sort_id/1 (member_live/index.ex:783) that builds
:"sort_#{field}" for an unknown field. determine_order/1 is a verified false positive (bounded to
"asc"/"desc") — no change there.
F-042 — KeyError. current_actor/1 (live_helpers.ex:116) is @spec'd to return nil but raises KeyError
when the current_user assign is absent. Fix the fallback.
F-057 — silent no-groups. groups_from_user_info (oidc_role_sync.ex:58) rescues String.to_existing_atom
by re-reading only the string key, so an atom-keyed user_info with an unloaded atom silently yields no groups
(possible silent missing admin assignment).
F-041 — sync symmetry. Add the missing @impl has_change?/0 to SyncMemberEmailToUser so the two
bidirectional email-sync changes run consistently (Ash 3.12+); correct the overstated-atomicity comment in
email_sync/.../helpers.ex.
R-030 (F-043) — error matching. Replace the brittle English-substring error classification in
auth_controller.ex:239-278 with structured Ash-error-type matching (breaks on i18n/rewording).
F-028 — locale leak. Restore the prior Gettext locale after the PDF export sets it (members_pdf.ex:146).
R-015 — Vereinfacht sync offload. The contact sync (vereinfacht/changes/sync_contact.ex:36) already runs
in after_transaction (does not block the tx or fail the save) but is synchronous in the request process, so
the member save waits on the third-party HTTP call. Dispatch via the existing Mv.TaskSupervisor; surface the
result via the existing SyncFlash.
F-030 — config caching. Cache oidc_client_secret/0 via the existing SettingsCache like its sibling
settings reads (currently an uncached Ash.read_one per request through a plug). Safe: config, not authz state.

Acceptance criteria

Tests: two concurrent admin demotions cannot reach zero admins (Ecto sandbox shared-mode); adversarial sort
input does not create atoms; missing-current_user path returns nil; atom-keyed user_info resolves groups;
PDF export locale does not leak; member save returns without waiting on the Vereinfacht API (result via SyncFlash).
Migration (if R-008 chooses a DB mechanism) applies cleanly.
Promote Warning.UnsafeToAtom into the gate .credo.exs (R-006 clears it).

## Motivation A set of genuine correctness/robustness fixes plus two safe hot-path performance wins. Grouped because each needs focused tests rather than riding along with mechanical cleanup. ## Scope - **R-008 — last-admin TOCTOU (the substantive item).** Close the race where two concurrent admin demotions can each read admin-count == 2 and both commit, leaving zero admins (`lib/accounts/user.ex:477-529`). This is NOT a simple declarative constraint — "admin" is indirect (`role_id → roles.permission_set_name == "admin"`, multiple admin roles, the system user excluded). Pick a mechanism in `/solutions`: - (a) `pg_advisory_xact_lock` in the admin-mutating action(s) — pragmatic minimum; - (b) a BEFORE trigger with `SELECT … FOR UPDATE` + `RAISE EXCEPTION 'ash_error: …'` — path-independent, fits the existing trigger pattern, but needs a sibling trigger on `roles`; - (c) SERIALIZABLE transaction + retry. Also map the OIDC email-collision constraint error to a friendly message. Note: worst-case zero-admins is recoverable via the `seed_admin` release task. - **R-006 — unbounded atom.** Remove the rescue in `to_sort_id/1` (`member_live/index.ex:783`) that builds `:"sort_#{field}"` for an unknown field. `determine_order/1` is a verified false positive (bounded to "asc"/"desc") — no change there. - **F-042 — KeyError.** `current_actor/1` (`live_helpers.ex:116`) is @spec'd to return nil but raises `KeyError` when the `current_user` assign is absent. Fix the fallback. - **F-057 — silent no-groups.** `groups_from_user_info` (`oidc_role_sync.ex:58`) rescues `String.to_existing_atom` by re-reading only the string key, so an atom-keyed `user_info` with an unloaded atom silently yields no groups (possible silent missing admin assignment). - **F-041 — sync symmetry.** Add the missing `@impl has_change?/0` to `SyncMemberEmailToUser` so the two bidirectional email-sync changes run consistently (Ash 3.12+); correct the overstated-atomicity comment in `email_sync/.../helpers.ex`. - **R-030 (F-043) — error matching.** Replace the brittle English-substring error classification in `auth_controller.ex:239-278` with structured Ash-error-type matching (breaks on i18n/rewording). - **F-028 — locale leak.** Restore the prior Gettext locale after the PDF export sets it (`members_pdf.ex:146`). - **R-015 — Vereinfacht sync offload.** The contact sync (`vereinfacht/changes/sync_contact.ex:36`) already runs in `after_transaction` (does not block the tx or fail the save) but is synchronous in the request process, so the member save waits on the third-party HTTP call. Dispatch via the existing `Mv.TaskSupervisor`; surface the result via the existing `SyncFlash`. - **F-030 — config caching.** Cache `oidc_client_secret/0` via the existing `SettingsCache` like its sibling settings reads (currently an uncached `Ash.read_one` per request through a plug). Safe: config, not authz state. ## Acceptance criteria - Tests: two concurrent admin demotions cannot reach zero admins (Ecto sandbox shared-mode); adversarial sort input does not create atoms; missing-`current_user` path returns nil; atom-keyed `user_info` resolves groups; PDF export locale does not leak; member save returns without waiting on the Vereinfacht API (result via SyncFlash). - Migration (if R-008 chooses a DB mechanism) applies cleanly. - Promote `Warning.UnsafeToAtom` into the gate `.credo.exs` (R-006 clears it).