Fix admin_update_user_password/3 writing the hash for any caller

[timujinne]

What

admin_update_user_password/3 now answers to the same rank rule as its sibling
update_user_status/3: when context[:admin_user] is present, the write is
refused with {:error, :insufficient_permissions} unless
can_manage_user_credentials?/2 allows that actor over that target.

Why here and not in the caller

#686 introduced the rank rule and cited its siblings as the model; the follow-up
review then moved update_user_status/3 into the context because a rule living
in one LiveView does not live anywhere — the guard had been added to the edit
form while the user-list and user-detail pages reached the context ungated.

Credential management was left behind in that move. The edit form asks
can_manage_user_credentials?/2 to hide the UI and to refuse the event, but the
context function itself asked nothing and wrote the hash for whoever reached it.
Credentials are the more final half of the pair: setting a password also deletes
every session token of the target.

The actor was already in hand — :admin_user has always been threaded through
context for the audit row, so only the question was missing. It now authorizes
as well as audits.

Reachability

Not exploitable through the shipped UI today. The one live caller
(user_form.ex) gates on socket.assigns.can_manage_credentials in all four of
its handlers. This closes the second caller before it exists, rather than a hole
in front of one.

Compatibility

Omitting :admin_user stays the system path — seeds, migrations and mix tasks
act with no actor and are not rank-checked, matching update_user_status/3.
The only behaviour change for an existing caller is that an out-of-rank actor
now gets {:error, :insufficient_permissions} instead of a successful write.

Tests

Six cases in security_authority_test.exs, mirroring the status block. They
assert on the stored hash, not the return tuple, so a guard that refused
only after writing would still fail them.

Verified red first: the three refusal cases returned {:ok, %User{}} before the
change, while the three permissive cases passed unchanged — so the new block is
not vacuous.

Run manually, since CI here is workflow_dispatch only:

• test/integration/users/security_authority_test.exs — 30 tests, 0 failures
• test/integration/users — 367 tests, 0 failures, no excluded line
  (a database-less run reports 0 tests, 0 failures (367 excluded) and still
  exits 0, so the excluded count is the thing worth reading)
• mix format --check-formatted, mix compile --warnings-as-errors,
  mix credo --strict (10104 mods/funs) — clean

@version and CHANGELOG.md deliberately untouched.

[timujinne]

Combined review — PR #690

Three independent reviewers, same brief, none saw the others: Kimi K3
(ask-kimi, max effort), GLM-5.2 (ask-glm, ASK_EFFORT=max), and Opus
(project reviewer agent, the only one with a shell).

Coverage caveat, stated up front. GLM returned only its closing summary —
the body of its review was lost in transport, so its findings below are recorded
at the level of detail it gave, not as a full pass. Opus produced no deliverable
within the window. Everything marked verified below was checked by me directly
against the tree, and the CRITICAL was reproduced in a sandboxed test run.

Verdict: NEEDS-WORK. Three blocking items. The first one is not about this
PR's code — it is about the claim this PR inherits.

---

BLOCKING 1 — BUG - CRITICAL: the takeover this line of work claims to close is still wide open

Found by Kimi; reached from a different direction by GLM; reproduced by me.

user_form.ex:559 takes custom_fields straight from client params:

custom_fields_params=Map.get(user_params,"custom_fields",%{})

and hands it to Auth.update_user_fields/2 (user_form.ex:716), inside the
with chain at user_form.ex:576 — with no authority check anywhere on that
path. update_user_fields/2 (auth.ex:1808) then resolves each key with
String.to_existing_atom/1 and, if it lands in

updatable_profile_fields=[:first_name,:last_name,:email,:username,:user_timezone]

writes it into the schema via profile_changeset, which casts :email and
:username directly — no confirmation-token flow.

The credential defence added by #686 is Map.drop(profile_params, ["password", "email", "username"]) at user_form.ex:643-650. It guards profile_params.
It has nothing to do with custom_fields_params.

Do X: as an actor holding only the users permission — which
user_form.ex:95-96 admits onto the page — open the Owner's edit form and submit
user[custom_fields][email]=attacker@example.com.

Observe Y (sandbox run, this tree):

email before: probe_target@example.com after: attacker@example.com
username after: "attacker"
custom_fields after: %{}

custom_fields coming back empty is the proof: the keys were not stored as
custom fields, they were routed into the columns.

From there: public forgot-password page → reset mail lands in the attacker's
inbox → reset_user_password/2 (token-gated by design, correctly no rank check)
→ session as Owner. The same payload with "username" removes a sign-in route
from an account the actor may not manage.

This is pre-existing, not introduced here. It is blocking anyway, because it
is the direct answer to what this PR and #686 claim to have done: the credential
rank rule is bypassable end-to-end, and the guard shipped in #686 reads as
complete when it is not.

Fix: filter custom_fields_params against the schema-field list in
update_user/2 before it reaches update_user_fields/2, using the same
can_manage_credentials answer the profile path already uses. The caller knows
the authority; update_user_fields/2 does not and should not have to.

---

BLOCKING 2 — BUG - MEDIUM: the new return shape crashes its only caller, after a partial write

Found independently by both Kimi and GLM. Verified by reading.

This one is introduced by this PR. update_profile_and_password/3
(user_form.ex:936) matches:

{:error,password_changeset}->profile_changeset=Auth.change_user_registration(user,user_params)combined_changeset=merge_password_errors(profile_changeset,password_changeset)

The new {:error, :insufficient_permissions} matches that clause, binding the
atom as password_changeset; merge_password_errors/2 then reads
password_changeset.errors → BadMapError, LiveView crashes. Worse, the profile
write at user_form.ex:940 has already committed, so the refusal leaves a
partial write.

Reachability — narrower than the reviewers stated, and I want that on record.
The password branch is only entered when password_provided is true, which
requires the password param to have survived the Map.drop at
user_form.ex:643 — which requires can_manage_credentials to be true. So this
needs the mount-time assign (user_form.ex:105-112) to have gone stale: the
Owner opens U's form, U is promoted to Owner (or the actor demoted), then save.
A race, not a walk-up. It is still a regression this PR introduces, and the
partial write makes it worse than a clean error.

Fix: match {:error, :insufficient_permissions} explicitly, refuse through
the existing deny_credential_action/1, and decide authority before writing
the profile rather than after.

---

BLOCKING 3 — BUG - MEDIUM: the system path is fail-open on shape

Found by Kimi.

caseMap.get(context,:admin_user)do%User{}=actor->..._system->do_admin_update_user_password(user,attrs,context)end

Any value that is present but not a %User{} — a map decoded from JSON by a host
application's API controller, a bare uuid string, false — silently takes the
unchecked path. Auth.admin_update_user_password(owner, %{password: "X"}, %{admin_user: %{"uuid" => "…"}}) writes the hash.

This matters more than it looks because PhoenixKit is a library: the unchecked
2-arg form is the default and the moduledoc's first example, so nothing
distinguishes "a mix task with no actor" from "a host caller that forgot".

Fix: only nil/absent is the system path; any other value is
{:error, :insufficient_permissions}. toggle_user_confirmation/2
(auth.ex:1331) carries the identical fallback and should get the same
treatment — noting that this makes the pair stricter than
update_user_status/3, which is the right direction to diverge.

---

Test quality — the honest accounting

Asked of every new test: would it still pass with the production change
reverted? Kimi answered it per test; Opus was to verify by actually reverting
and did not report.

Test	Red on revert?
Admin may not set an Owner's password	yes — real
Admin may not set another Admin's password	yes — real
non-staff actor refused	yes — real
an in-rank actor succeeds	no — pins pre-existing permissive behaviour
changing your own password is allowed	no — same
omitting the actor is the system path	no — pins the fail-open default (see #3)

Three of six actually test the change. The other three are anti-over-refusal
guards, which are worth keeping — but they should be described as that, not
counted as coverage of the new rule.

All six assert on the stored hash re-read through Repo.get!
(security_authority_test.exs:47), so a guard that refused only after writing
would fail them. That part is right.

Missing: no test that a non-%User{}:admin_user is refused (today it
writes), and none exercising the caller's handling of the new error tuple — the
crash path in #2.

---

The mix.exs commit (40cd3d11)

• The env prefix fix is correct on the platforms this project supports.
  mix cmd shells out through System.cmd/3, which execs the first word, so the
  bare MIX_ENV=prod assignment did raise :enoent. On Windows there is no
  env binary and mix cmd uses no shell, so the step would fail there — CI is
  ubuntu-only and nothing claims Windows support, so this is a NITPICK.
• Moving hex.audit / hex.build into subprocesses does not weaken the
  gate: same tasks, same arguments, same MIX_ENV, same tree and lockfile.
  Failure propagation is intact — the observed :enoent abort proves a failing
  cmd step kills the alias. Cost is recompilation time, not coverage.

---

The same shape left open elsewhere in auth.ex

This PR closes one function whose authority rule lived only in its LiveView
caller. Kimi enumerated the rest; the ones that carry real weight:

1. update_user_profile/2 (auth.ex:1597) — rewrites email/username of any
   user; the rule is the Map.drop in the caller. GLM independently named this
   one as the half-applied principle.
2. update_user_fields/2 (auth.ex:1808) — same fields, reachable unfiltered.
   This is BLOCKING 1.
3. admin_confirm_user/1 / admin_unconfirm_user/1 (auth.ex:1269, 1293) —
   public and ungated; unconfirming locks the target out of all eleven
   require_email_confirmation gates.
4. set_organization/2, remove_from_organization/1, change_account_type/2
   (auth.ex:968, 981, 990) — reachable from user_form.ex:336,735 with no
   rank rule anywhere.
5. deliver_user_reset_password_instructions/3 (auth.ex:1374) — gated only in
   the event handler; impact limited (mails the target's own address).

---

Nitpick on the commit message

It says the live caller "gates on can_manage_credentials in all four of its
handlers". Three handlers refuse (user_form.ex:201,214,222); the fourth path —
save — does not refuse, it silently drops fields (user_form.ex:643-649).
The distinction is not cosmetic: silent-drop is exactly the shape that made
BLOCKING 1 invisible for as long as it has been.

---

Where the reviewers agreed and disagreed

Both Kimi and GLM independently found BLOCKING 2 — two models, two routes, same
defect, which is the strongest signal in this round. Both also converged on the
"principle applied half-way" theme, though only Kimi followed it to the
reachable custom_fields exploit. Neither disputed the core guard itself: the
argument order matches the definition, has_system_role?/2 correctly falls
through to a query when roles is %Ecto.Association.NotLoaded{}, and there is
no unsatisfiable-gate trap of the kind that bit the OAuth work.

[timujinne]

Addendum — the third reviewer landed, and the record above needs correcting

The combined review I posted earlier said Opus "produced no deliverable within
the window". That was true when I wrote it and is no longer true: the review
arrived afterwards. It is the strongest of the three, so the correction matters
more than the tidiness of the thread.

It independently confirmed both findings with live probes rather than by
reading — an Admin mounting the Owner's edit form, asserting the page really did
refuse (refute html =~ "toggle_password_field" passed, so the rank rule was
in force), and then rewriting the Owner's email through save_user anyway. It
also reproduced the per-test revert experiment and got exactly the split I
reported: tests 1–3 fail on revert, tests 4–6 pass.

Already fixed in 2b4a5f0c, before this review arrived

Its blocking items 1 and 2, and the first half of 3 — the custom_fields
bypass, the unhandled {:error, :insufficient_permissions}, and the fail-open
_system branch. Two notes where its recommendation is broader than what I
shipped:

• It suggests filtering custom_fields_paramsagainst the declared field
  definitions. I dropped the five schema identity names unconditionally
  instead. Its version is stricter and worth considering, but it makes the
  filter depend on operator-configured state; mine cannot be widened by adding
  a field definition named email.
• It also wants update_user_profile/2 itself to carry a context-level rank
  check, not just the caller. That is the right end state and is not in this
  PR.

Not fixed — carried forward

1. A refused takeover leaves no trace. No Logger, no Activity, no
   AuditLog on the refusal branch, while do_deactivate_user/2 logs a far
   milder refusal (auth.ex:2227). If the second caller this guard exists for
   ever appears, the guard blocks it invisibly. Fixed in the follow-up
   commit.
2. Three test names promise a permission they never grant.plain_user/0
   registers and confirms, nothing more — so "a non-staff actor holding only a
   permission" exercises a bare default-role actor. The threat model this whole
   series exists for is never constructed. Fixed in the follow-up commit.
3. cmd mix hex.audit changes what the gate verifies. From the Hex 2.4.1
   beam: hex.audit sets a deferred exit code via System.at_exit, not
   Mix.raise. In-VM, a retired dependency let docs, hex.build and
   release_check still run and failed at the end; as a subprocess it aborts
   the alias immediately — reintroducing the "gate stops early" mode the sibling
   :enoent fix removed. hex.build is unaffected (it does Mix.raise).
   Fail-fast may be intended, but the comment frames the change as pure VM
   isolation.
4. The env comment states a version-specific behaviour as unconditional.
   mix cmd execing the first word directly is Elixir ≥ 1.19; mix.exs allows
   ~> 1.18, where it routed through the shell and the bare assignment worked.
   The env form is correct on both.
5. CHANGELOG.md — it asks for a ### Security entry under 1.7.236.
   Deliberately not done: CHANGELOG.md and @version are maintainer-owned
   in this repo. Flagging rather than editing.
6. /admin/users/sessions — HIGH, pre-existing, needs no new caller. Any
   holder of users can revoke every session of any account, Owner included,
   with no rank check in the LiveView or the context, no AuditLog and no
   Activity row. This is the one item it found that is exploitable today
   without waiting for a second caller, and it is outside this PR.
7. Self-edit is a current-password-free password change. Rule 1 of
   validate_admin_authority_over/2 admits your own account, so
   /admin/users/edit/<self> sets a password without validate_current_password/2,
   unlike update_user_password/3. Pre-existing since Fix four account-takeover paths found by a security review #686; a stolen session
   becomes a permanent takeover with no re-authentication.
8. Roles.assign_role/3 takes assigned_by as audit metadata only and
   refuses "Owner" and nothing else — it will grant "Admin" to anyone. The
   LiveViews go through sync_user_roles/3, which does authorize; this is the
   unchecked shortcut.

The generalisation worth keeping

Its closing observation is the most useful thing in this round, because it turns
a class of bug into a grep:

An actor threaded in for the audit row is the strongest available signal that
the function has an authority question it is not asking.

Three instances so far — admin_update_user_password/3 before this PR,
delete_admin_note/2, and Roles.assign_role/3. Every function taking
admin_user / assigned_by / actor / current_user and never passing it to
a predicate is a candidate. That would have found this defect without a review.

Correction to my own commit message

The message for 913315e7 says the live caller "gates in all four of its
handlers". Three handlers refuse; the fourth — save — silently drops fields, and
a fifth handler in the same module (save_user → custom_fields) did not gate
at all. Silent-drop is exactly the shape that kept blocking item 1 invisible
through two review rounds.