rbrooks/Quest-Board

Watch

Fork 0

[Backend] `set_admin --email` crashes instead of disambiguating when two identities share an email #436

New issue

Closed

opened 2026-08-29 06:35:31 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-08-29 06:35:31 +00:00

Contributor

The defect

_set_admin_by_email (webapp/backend/app/cli.py) looks a user up by email alone:

result = await db.execute(select(User).where(User.email == email))
user = result.scalar_one_or_none()

users is unique on (oidc_sub, oidc_issuer), not on email. So one person can legitimately have several rows — and when they do, scalar_one_or_none() raises MultipleResultsFound. The operator gets a SQLAlchemy traceback rather than "two users share that address, disambiguate with --oidc-sub".

make set-admin EMAIL=… wraps this path, so the documented convenience command is the one that breaks.

Not hypothetical — it is what a prod→dev restore produces

Hit on the dev host, 2026-08-29:

display_name | email               | is_admin | oidc_issuer
-------------+---------------------+----------+--------------------------------------------------
Ryan Brooks  | ryan@ryanbrooks.net | t        | https://auth.rhoving.com/application/o/quest-board/
Ryan Brooks  | ryan@ryanbrooks.net | f        | https://auth.rhoving.com/application/o/quest-board-dev/

Same oidc_sub, different issuer, because dev authenticates against its own OIDC application. Restoring production onto dev brings prod's user rows across, and the operator's dev identity is a separate row that was never granted admin.

So the exact circumstance that makes someone reach for set_admin — "I restored prod onto dev and I am not an admin any more" — is the circumstance in which --email cannot work. Had to use --oidc-sub + --oidc-issuer instead.

Worth noting the framing trap this creates: it presents as lost admin, when in fact the admin flag is intact on a different row. An operator without database access could reasonably conclude the restore corrupted something.

Proposed fix

Make the ambiguity a first-class outcome rather than an exception:

Use .scalars().all() and branch on the count.
Zero → the existing clear "no user found" error.
One → grant, as now.
Several → list the candidates (display name, issuer, current is_admin) and tell the operator to re-run with --oidc-sub/--oidc-issuer. Do not guess, and do not grant to all of them.

_set_admin_by_oidc is already correct and needs no change.

Acceptance criteria

Two users sharing an email produce a readable message naming both issuers and their current admin status, not MultipleResultsFound
The message names the flags needed to disambiguate
A single match still grants admin exactly as before, and a zero match still reports clearly
Ambiguity exits non-zero, so a script cannot mistake it for success
A test covers the two-row case and asserts neither row was modified

The same class of problem as #407 — an identity lookup that cannot safely disambiguate. There it can scrub the wrong person; here it merely fails, which is the better failure, but the underlying assumption ("email identifies a user") is shared and wrong in both.
docs/OPERATIONS.md may be worth a line about multiple identities after a cross-environment restore, since the symptom reads as lost access.

## The defect `_set_admin_by_email` (`webapp/backend/app/cli.py`) looks a user up by email alone: ```python result = await db.execute(select(User).where(User.email == email)) user = result.scalar_one_or_none() ``` `users` is unique on **`(oidc_sub, oidc_issuer)`**, not on email. So one person can legitimately have several rows — and when they do, `scalar_one_or_none()` raises `MultipleResultsFound`. The operator gets a SQLAlchemy traceback rather than "two users share that address, disambiguate with `--oidc-sub`". `make set-admin EMAIL=…` wraps this path, so the documented convenience command is the one that breaks. ## Not hypothetical — it is what a prod→dev restore produces Hit on the dev host, 2026-08-29: ``` display_name | email | is_admin | oidc_issuer -------------+---------------------+----------+-------------------------------------------------- Ryan Brooks | ryan@ryanbrooks.net | t | https://auth.rhoving.com/application/o/quest-board/ Ryan Brooks | ryan@ryanbrooks.net | f | https://auth.rhoving.com/application/o/quest-board-dev/ ``` Same `oidc_sub`, different issuer, because dev authenticates against its own OIDC application. Restoring production onto dev brings prod's user rows across, and the operator's *dev* identity is a separate row that was never granted admin. So the exact circumstance that makes someone reach for `set_admin` — "I restored prod onto dev and I am not an admin any more" — is the circumstance in which `--email` cannot work. Had to use `--oidc-sub` + `--oidc-issuer` instead. Worth noting the framing trap this creates: it presents as *lost* admin, when in fact the admin flag is intact on a different row. An operator without database access could reasonably conclude the restore corrupted something. ## Proposed fix Make the ambiguity a first-class outcome rather than an exception: - Use `.scalars().all()` and branch on the count. - Zero → the existing clear "no user found" error. - One → grant, as now. - **Several → list the candidates** (display name, issuer, current `is_admin`) and tell the operator to re-run with `--oidc-sub`/`--oidc-issuer`. Do not guess, and do not grant to all of them. `_set_admin_by_oidc` is already correct and needs no change. ## Acceptance criteria - [ ] Two users sharing an email produce a readable message naming both issuers and their current admin status, not `MultipleResultsFound` - [ ] The message names the flags needed to disambiguate - [ ] A single match still grants admin exactly as before, and a zero match still reports clearly - [ ] Ambiguity exits non-zero, so a script cannot mistake it for success - [ ] A test covers the two-row case and asserts neither row was modified ## Related - The same class of problem as #407 — an identity lookup that cannot safely disambiguate. There it can scrub the wrong person; here it merely fails, which is the better failure, but the underlying assumption ("email identifies a user") is shared and wrong in both. - `docs/OPERATIONS.md` may be worth a line about multiple identities after a cross-environment restore, since the symptom reads as lost access.

claude-bot referenced this issue

2026-08-29 06:39:20 +00:00

[Frontend] Admins should be able to see every campaign from the admin panel #437

claude-bot added this to the v4.1.0 - Data Durability & Recovery milestone

2026-08-29 07:35:55 +00:00

claude-bot referenced this issue

2026-08-29 18:03:43 +00:00

[Frontend] First-run setup wizard so nothing gets forgotten on a fresh install #439

rbrooks referenced this issue from a commit

2026-09-01 16:25:12 +00:00

fix(cli): set_admin --email disambiguates instead of crashing (#436)

claude-bot referenced this issue from a pull request that will close it,

2026-09-01 16:25:26 +00:00

fix(cli): set_admin --email disambiguates instead of crashing (#436) #480

claude-bot closed this issue

2026-09-01 16:35:26 +00:00

claude-bot referenced this issue from a commit

2026-09-01 16:35:26 +00:00

Merge PR #480: set_admin --email disambiguates instead of crashing (#436)

claude-bot commented

2026-09-01 16:35:36 +00:00

Author

Contributor

Done in PR #480. The issue's evidence held up exactly against the current code — scalar_one_or_none() on an email-only lookup, with _set_admin_by_oidc already correct and untouched.

Ambiguity is now an outcome rather than an exception. The candidates are listed with oidc_sub, issuer and current admin status, the message names the flags to disambiguate, it exits non-zero, and nothing is modified. Admin status is in there because it is usually the whole answer: the operator typically already holds admin on one row and needs the other one.

Guessing would have been worse than failing. The rows differ precisely in which identity they are, so granting to the wrong one leaves the operator still locked out with no sign of why.

Also took the optional suggestion and added an OPERATIONS.md troubleshooting section, since this presents as lost access when the flag is intact on another row.

7 mutations, all caught. The one worth naming is reverting to scalar_one_or_none() — that raises rather than exiting, so pytest.raises(SystemExit) pins the actual behaviour change and not merely the wording.

Two things about the tests worth recording, both cases of a test that would otherwise have passed for the wrong reason:

The CLI opens its own AsyncSessionLocal(), a different connection from the transactional db fixture, so it cannot see uncommitted fixture rows. Written the obvious way, every "no user found" assertion would have passed against an effectively empty table. These use real commits with cleanup instead — the same trap #457 documented for the Celery purge tasks.
The main ambiguity test seeds one admin and one non-admin row, so "nothing changed" and "granted to the row that already had it" both end [False, True] — indistinguishable. A second test seeds two non-admin rows, where any grant at all shows up.

Backend 1613 passed; ruff clean. All five acceptance criteria met.

Done in PR #480. The issue's evidence held up exactly against the current code — `scalar_one_or_none()` on an email-only lookup, with `_set_admin_by_oidc` already correct and untouched. Ambiguity is now an outcome rather than an exception. The candidates are listed with `oidc_sub`, issuer and **current admin status**, the message names the flags to disambiguate, it exits non-zero, and nothing is modified. Admin status is in there because it is usually the whole answer: the operator typically already holds admin on one row and needs the other one. Guessing would have been worse than failing. The rows differ precisely in *which identity they are*, so granting to the wrong one leaves the operator still locked out with no sign of why. Also took the optional suggestion and added an `OPERATIONS.md` troubleshooting section, since this presents as lost access when the flag is intact on another row. **7 mutations, all caught.** The one worth naming is reverting to `scalar_one_or_none()` — that *raises* rather than exiting, so `pytest.raises(SystemExit)` pins the actual behaviour change and not merely the wording. Two things about the tests worth recording, both cases of a test that would otherwise have passed for the wrong reason: - The CLI opens its own `AsyncSessionLocal()`, a different connection from the transactional `db` fixture, so it cannot see uncommitted fixture rows. Written the obvious way, every "no user found" assertion would have passed against an effectively empty table. These use real commits with cleanup instead — the same trap #457 documented for the Celery purge tasks. - The main ambiguity test seeds one admin and one non-admin row, so "nothing changed" and "granted to the row that already had it" both end `[False, True]` — indistinguishable. A second test seeds two non-admin rows, where any grant at all shows up. Backend 1613 passed; ruff clean. All five acceptance criteria met.