Fix the credential rank rule being bypassable through custom_fields

[timujinne]

Follow-up to #690, which merged before this landed. The takeover #686 and #690
set out to close is still open on main today — this closes it.

The bypass

user_form.ex took params["custom_fields"] verbatim and handed it to
Auth.update_user_fields/2, which resolves each key with
String.to_existing_atom/1 and, when the name matches
[:first_name, :last_name, :email, :username, :user_timezone], writes it
into the schema through profile_changeset — no confirmation-token flow.

The credential defence from #686 is a Map.drop over profile_params. It never
saw custom_fields.

Do X: as an Admin (or any holder of the users permission), open an Owner's
edit form — the page correctly refuses you, the password toggle is not even
rendered — then push the form's own event:

render_submit(view,"save_user",%{"user"=>%{"custom_fields"=>%{"email"=>"attacker@example.com"}}})

Observe Y: the Owner's email column is rewritten. Public password-reset page,
and the account is gone. "username" goes the same way, removing the victim's
second sign-in route.

Found by a three-reviewer round on #690 (Kimi K3, GLM-5.2, and an Opus agent
that reproduced it end to end against a live database). Reproduced here as a
LiveView test before the fix.

What changed

• The five schema identity names are dropped from custom_fields
  unconditionally, not only when the actor is out of rank. The form renders a
  real input for each, so such a key is never a legitimate submission from this
  page — and an unconditional filter cannot go stale the way an authority assign
  computed at mount can.
• The write path re-asks the rank rule against the target reloaded from the
  database, instead of reading @can_manage_credentials from mount/3. A
  target promoted between mount and submit made the form and the context
  disagree: the form wrote the credential fields on the old answer, then took
  {:error, :insufficient_permissions} from the context, which the clause below
  expects to be a changeset — merge_password_errors/2 called .errors on an
  atom and the LiveView died, after a partial write. The reload is load-bearing:
  the mounted struct carries a preloaded :roles list, so re-asking with it
  would only agree with itself.
• Only an absent actor is the system path. A :admin_user that is present
  but not a %User{} — a map decoded from JSON by a host controller, a bare uuid
  string — took the unchecked branch, and then crashed in the audit write.
• Refusals are logged. A guard nobody can tell fired is the failure mode this
  whole series is about; do_deactivate_user/2 already set the precedent.
• users_permission_holder/0 builds the actor three existing tests were
  named for and never created: a role holding users and no staff rank. They
  passed for a reason that could not distinguish it from a bare default-role
  user.

Tests

The bypass is pinned at the form, where the params arrive, by pushing the event
straight at the LiveView — form/3 refuses params with no matching rendered
input, which is exactly the gap the attack uses.

The stale-assign test carries email in its payload on purpose: that is what
makes it discriminating. Reverting only the reload turns it red (verified —
observed attacker@example.com).

• test/integration/users — 372 tests, 0 failures, no excluded line
• mix format --check-formatted, mix compile --warnings-as-errors,
  mix credo --strict (10114 mods/funs) — clean

@version and CHANGELOG.md untouched. The reviewers also asked for a
### Security entry; that is maintainer-owned here, so it is flagged rather
than written.

Not in this PR

Recorded in the #690 thread and still open: /admin/users/sessions revokes any
account's sessions with no rank check and no audit row (pre-existing, needs no
second caller); update_user_profile/2 should carry the rank rule in the
context rather than only at its caller; cmd mix hex.audit turns a deferred
exit code into an immediate alias abort.

[timujinne]

Post-merge review — credential bypass via custom_fields (Track A, 3-pass)

Three-pass iterative review (broad → deepen → verify/narrow) on merge commit 2eadb6e9. Verdict: the bypass this PR targets is correctly closed, at both sinks. Below: what's verified closed, what #692 already fixed, and the same-series siblings left open — those are the actionable follow-ups.

Verified CLOSED by #691 (confirmed against merged tree)

• custom_fields[email] / [username] sink — drop_schema_identity_fields/1 (user_form.ex:560, applied at :760) drops the five identity names before they reach update_user_fields/2. Deny-list is exactly the set update_user_fields/2 routes into profile_changeset (auth.ex:1836).
• profile_params sink — Map.drop(["password","email","username"]) for out-of-rank actors, gated on credential_authority_now/2 target reload (user_form.ex:990).
• Negative results (worth re-checking but currently clean): confirmed_at, is_active, hashed_password are NOT bypassable via custom_fields — gated by confirm_changeset/status_changeset/password_changeset, none in profile_changeset's cast list. Self-service user_settings.ex path is safe (actor == target).

Already fixed by #692

• F-1 (refusal path crash on malformed target, both actor branches) — refuse_credential_write/2 at current HEAD takes the target unguarded and uses a safe label. ✅
• F-4 partially (deny-list duplication) — user_form.ex now reads Auth.updatable_profile_fields/0. Parametric tests for all five names still outstanding.

🔴 Same-series siblings still fail-open (not a #691 regression — introduced together in c9413b86, #691 hardened only the password path)

Both update_user_status/3 (auth.ex:2221) and toggle_user_confirmation/3 (auth.ex:1343) keep the _system -> shape that performs the unchecked write for ANY non-%User{} actor:

caseKeyword.get(opts,:actor)do%User{}=actor->ifcan_manage_...(user,actor),do: ...,else: {:error,:insufficient_permissions}_system->do_...(user)# ← fires for %{"uuid" => ...}, a uuid string, false, 42end

• toggle_user_confirmation/3 is the more serious one: unchecked path → admin_unconfirm_user/1 → confirmed_at = nil → target locked out of all confirmation-gated pages (DoS / mass-lockout). grep toggle_user_confirmation in security_authority_test.exs → 0 matches — no test for the confirmation rank rule at all.
• update_user_status/3 — same shape, less severe (reversible status change).
• Neither is reachable from core (all LV callers pass a real %User{} actor; referrals.ex intentionally omits :actor for cron) — host-only risk, but same authority-bypass class this PR set out to close.
• No malformed-actor test exists for either. The word "malformed" appears in security_authority_test.exs only at line 285 (the password test). A regression reintroducing _system -> in either sibling would pass green.

Suggested follow-up PR: mirror the password 3-branch restructure in both siblings + add malformed-actor tests. Same pattern as #691, ~30 lines + tests.

Other findings (lower severity, for the backlog)

• F-3 · MEDIUM — update_user_profile/2, update_user_fields/2, bulk_update_user_fields/2 (auth.ex:1623/1834/2003) are public sinks with no actor param. Rank rule lives in callers, not the sink. Latent host surface — any host building attrs from request params can write identity fields with no confirmation flow. Pushing :actor into the sinks is the bigger fix.
• F-6 · LOW — refusal is Logger.warning-only; success path writes AuditLog + Activity.log. A takeover attempt leaves no DB trace. Optional Activity.log(action: "user.credential_write_refused").
• F-9 · LOW · pre-existing — update_profile_and_password/2 (user_form.ex:1000) does two independent writes; if the password step fails (incl. the new :insufficient_permissions clause this PR added), the email/username change from step 1 is already persisted. No Ecto.Multi. Predates #691.
• F-10 · LOW — credential_authority_now/2 reloads the target but uses the stale mounted actor. A demoted Admin keeps their preloaded role for the rest of the session. Inherent to session auth; at minimum document the target-side-only freshness.
• F-11 · LOW — stale-authority test asserts hashed_password/email unchanged but not first_name == "Renamed"; a regression dropping the whole profile write would pass.

Note on method

This was a three-pass single-track review (broad → deepen → verify/narrow). An independent second track would add the most value precisely on F-2/F-7 (the sibling fail-open functions) and on re-verifying the negative results. Full artifact trace in our internal review directory.

[timujinne]

проверка публикации (test, будет удалён)

[timujinne]

Post-merge review — credential bypass via custom_fields (Track A, 3-pass)

Three-pass iterative review (broad → deepen → verify/narrow) on merge commit 2eadb6e9. Verdict: the bypass this PR targets is correctly closed, at both sinks. Below: what's verified closed, what #692 already fixed, and the same-series siblings left open — those are the actionable follow-ups.

Verified CLOSED by #691 (confirmed against merged tree)

• custom_fields[email] / [username] sink — drop_schema_identity_fields/1 (user_form.ex:560, applied at :760) drops the five identity names before they reach update_user_fields/2. Deny-list is exactly the set update_user_fields/2 routes into profile_changeset (auth.ex:1836).
• profile_params sink — Map.drop(["password","email","username"]) for out-of-rank actors, gated on credential_authority_now/2 target reload (user_form.ex:990).
• Negative results (worth re-checking but currently clean): confirmed_at, is_active, hashed_password are NOT bypassable via custom_fields — gated by confirm_changeset/status_changeset/password_changeset, none in profile_changeset's cast list. Self-service user_settings.ex path is safe (actor == target).

Already fixed by #692

• F-1 (refusal path crash on malformed target, both actor branches) — refuse_credential_write/2 at current HEAD takes the target unguarded and uses a safe label. ✅
• F-4 partially (deny-list duplication) — user_form.ex now reads Auth.updatable_profile_fields/0. Parametric tests for all five names still outstanding.

🔴 Same-series siblings still fail-open (not a #691 regression — introduced together in c9413b86, #691 hardened only the password path)

Both update_user_status/3 (auth.ex:2221) and toggle_user_confirmation/3 (auth.ex:1343) keep the _system -> shape that performs the unchecked write for ANY non-%User{} actor:

caseKeyword.get(opts,:actor)do%User{}=actor->ifcan_manage_...(user,actor),do: ...,else: {:error,:insufficient_permissions}_system->do_...(user)# ← fires for %{"uuid" => ...}, a uuid string, false, 42end

• toggle_user_confirmation/3 is the more serious one: unchecked path → admin_unconfirm_user/1 → confirmed_at = nil → target locked out of all confirmation-gated pages (DoS / mass-lockout). grep toggle_user_confirmation in security_authority_test.exs → 0 matches — no test for the confirmation rank rule at all.
• update_user_status/3 — same shape, less severe (reversible status change).
• Neither is reachable from core (all LV callers pass a real %User{} actor; referrals.ex intentionally omits :actor for cron) — host-only risk, but same authority-bypass class this PR set out to close.
• No malformed-actor test exists for either. The word "malformed" appears in security_authority_test.exs only at line 285 (the password test). A regression reintroducing _system -> in either sibling would pass green.

Suggested follow-up PR: mirror the password 3-branch restructure in both siblings + add malformed-actor tests. Same pattern as #691, ~30 lines + tests.

Other findings (lower severity, for the backlog)

• F-3 · MEDIUM — update_user_profile/2, update_user_fields/2, bulk_update_user_fields/2 (auth.ex:1623/1834/2003) are public sinks with no actor param. Rank rule lives in callers, not the sink. Latent host surface — any host building attrs from request params can write identity fields with no confirmation flow. Pushing :actor into the sinks is the bigger fix.
• F-6 · LOW — refusal is Logger.warning-only; success path writes AuditLog + Activity.log. A takeover attempt leaves no DB trace. Optional Activity.log(action: "user.credential_write_refused").
• F-9 · LOW · pre-existing — update_profile_and_password/2 (user_form.ex:1000) does two independent writes; if the password step fails (incl. the new :insufficient_permissions clause this PR added), the email/username change from step 1 is already persisted. No Ecto.Multi. Predates Fix the credential rank rule being bypassable through custom_fields #691.
• F-10 · LOW — credential_authority_now/2 reloads the target but uses the stale mounted actor. A demoted Admin keeps their preloaded role for the rest of the session. Inherent to session auth; at minimum document the target-side-only freshness.
• F-11 · LOW — stale-authority test asserts hashed_password/email unchanged but not first_name == "Renamed"; a regression dropping the whole profile write would pass.

Note on method

This was a three-pass single-track review (broad → deepen → verify/narrow). An independent second track would add the most value precisely on F-2/F-7 (the sibling fail-open functions) and on re-verifying the negative results. Full artifact trace in our internal review directory.