rbrooks/Quest-Board

Watch

Fork 0

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

Merged

claude-bot merged 1 commit from fix/436-set-admin-duplicate-email into main

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

claude-bot commented

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

Contributor

Closes #436.

The defect

_set_admin_by_email looked users up by email alone and called scalar_one_or_none(). But users is unique on (oidc_sub, oidc_issuer), not on email, so one person can legitimately own several rows — and two matches raise MultipleResultsFound, handing the operator a SQLAlchemy traceback instead of an answer.

Verified against the current code: the issue's claims hold exactly, and _set_admin_by_oidc is already correct and untouched.

The circumstance is the one that makes you run the command

Restoring production onto dev brings production's user rows across. The operator's dev identity authenticates against a different OIDC application, so it is a separate row that was never granted admin. So "I restored prod onto dev and I am not an admin any more" is precisely the case --email could not handle.

It also presents as lost admin when the flag is intact on a different row — and make set-admin only accepts EMAIL, so the documented convenience path is the broken one.

The fix

Ambiguity becomes an outcome rather than an exception. The candidates are listed with their oidc_sub, issuer and current admin status, the message names the flags needed to pick one, it exits non-zero so a script cannot read it as success, and nothing is modified:

ERROR: 2 users share the email 'you@example.com'. Email does not identify a
user — the unique key is (oidc_sub, oidc_issuer).

  Your Name — admin
    --oidc-sub    abc123
    --oidc-issuer https://auth.example.com/application/o/quest-board/
  Your Name — not admin
    --oidc-sub    abc123
    --oidc-issuer https://auth.example.com/application/o/quest-board-dev/

Re-run with --oidc-sub and --oidc-issuer to pick one. No user was modified.

Current admin status is in there because it is usually the answer: the operator often already holds admin on one row and needs the other one. Results are ordered by issuer so the listing is stable between runs.

Guessing would be 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 adds an OPERATIONS.md troubleshooting section — the issue suggested it, and it is worth having because the symptom reads as lost access rather than as two accounts.

Verification

7 mutations, all caught. The one that matters most 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. The others cover granting to the first match instead of refusing, exiting zero on ambiguity, dropping the flags or the admin status from the listing, and both of the paths that had to keep working.

test_ambiguity_does_not_grant_admin_to_anyone is deliberately separate from the main case: in that one both rows end [False, True], which is indistinguishable from "granted to the row that already had it". Seeding two non-admin rows makes any grant at all visible.

The tests use real commits with cleanup rather than the transactional db fixture, because the CLI opens its own AsyncSessionLocal() — a different connection, which cannot see uncommitted fixture rows. Without that, every "no user found" assertion would pass for the wrong reason.

Backend 1613 passed; ruff check/format clean. No frontend or migration changes.

🤖 Generated with Claude Code

Closes #436. ## The defect `_set_admin_by_email` looked users up by email alone and called `scalar_one_or_none()`. But `users` is unique on **`(oidc_sub, oidc_issuer)`**, not on email, so one person can legitimately own several rows — and two matches raise `MultipleResultsFound`, handing the operator a SQLAlchemy traceback instead of an answer. Verified against the current code: the issue's claims hold exactly, and `_set_admin_by_oidc` is already correct and untouched. ## The circumstance is the one that makes you run the command Restoring production onto dev brings production's user rows across. The operator's dev identity authenticates against a different OIDC application, so it is a separate row that was never granted admin. So "I restored prod onto dev and I am not an admin any more" is precisely the case `--email` could not handle. It also presents as *lost* admin when the flag is intact on a different row — and `make set-admin` only accepts `EMAIL`, so the documented convenience path is the broken one. ## The fix Ambiguity becomes an outcome rather than an exception. The candidates are listed with their `oidc_sub`, issuer and **current admin status**, the message names the flags needed to pick one, it exits non-zero so a script cannot read it as success, and nothing is modified: ``` ERROR: 2 users share the email 'you@example.com'. Email does not identify a user — the unique key is (oidc_sub, oidc_issuer). Your Name — admin --oidc-sub abc123 --oidc-issuer https://auth.example.com/application/o/quest-board/ Your Name — not admin --oidc-sub abc123 --oidc-issuer https://auth.example.com/application/o/quest-board-dev/ Re-run with --oidc-sub and --oidc-issuer to pick one. No user was modified. ``` Current admin status is in there because it is usually the answer: the operator often already holds admin on one row and needs the *other* one. Results are ordered by issuer so the listing is stable between runs. Guessing would be 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 adds an `OPERATIONS.md` troubleshooting section — the issue suggested it, and it is worth having because the symptom reads as lost access rather than as two accounts. ## Verification **7 mutations, all caught.** The one that matters most 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. The others cover granting to the first match instead of refusing, exiting zero on ambiguity, dropping the flags or the admin status from the listing, and both of the paths that had to keep working. `test_ambiguity_does_not_grant_admin_to_anyone` is deliberately separate from the main case: in that one both rows end `[False, True]`, which is indistinguishable from "granted to the row that already had it". Seeding two non-admin rows makes any grant at all visible. The tests use real commits with cleanup rather than the transactional `db` fixture, because the CLI opens its own `AsyncSessionLocal()` — a different connection, which cannot see uncommitted fixture rows. Without that, every "no user found" assertion would pass for the wrong reason. Backend **1613 passed**; `ruff check`/`format` clean. No frontend or migration changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit

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

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

CI / Bot/backend version sync (pull_request) Successful in 20s

Details

CI / Backend lint (ruff) (pull_request) Successful in 25s

Details

CI / Summarisation accuracy eval harness (stub provider) (pull_request) Successful in 45s

Details

CI / Docker image build (pull_request) Successful in 22s

Details

CI / Bot tests and audit (pull_request) Successful in 1m20s

Details

CI / Frontend tests, audit, and build (pull_request) Successful in 2m15s

Details

CI / Backend migration, tests, and audit (pull_request) Successful in 9m6s

Details

5479a17c0b

`_set_admin_by_email` looked users up by email alone and called
`scalar_one_or_none()`. But `users` is unique on (oidc_sub, oidc_issuer), not
on email, so one person can legitimately own several rows — and two matches
raise `MultipleResultsFound`, giving the operator a SQLAlchemy traceback
instead of an answer.

The circumstance that produces duplicate emails is the same one that makes
someone reach for this command. Restoring prod onto dev brings prod's user rows
across; the operator's dev identity authenticates against a different OIDC
application, so it is a separate row that was never granted admin. It presents
as *lost* admin when the flag is intact on another row — and `make set-admin`
only accepts EMAIL, so the documented convenience path is the broken one.

Ambiguity is now an outcome rather than an exception: the candidates are listed
with their oidc_sub, issuer and current admin status, the message names the
flags needed to pick one, it exits non-zero so a script cannot read it as
success, and nothing is modified. Guessing would be worse than failing — the
rows differ precisely in which identity they are, so granting to the wrong one
leaves the operator locked out with no sign of why.

`_set_admin_by_oidc` was already correct and is unchanged.

Also adds an OPERATIONS.md troubleshooting section, because the symptom reads
as lost access rather than as two accounts.

7 mutations, all caught — including reverting to `scalar_one_or_none`, which
raises rather than exiting, so the test pins the behaviour change and not just
the message. Tests use real commits with cleanup rather than the transactional
`db` fixture: the CLI opens its own `AsyncSessionLocal()`, a different
connection, so without that every "no user found" assertion would pass for the
wrong reason.

Backend 1613 passed; ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot merged commit fd487694fc into main

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

claude-bot referenced this pull request from a commit

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

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

claude-bot referenced this pull request

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

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