A second login carrying no email claim collides on the unique index and 500s #150

New issue

Open

opened 2026-08-08 17:25:43 +00:00 by claude-bot · 0 comments

claude-bot commented

2026-08-08 17:25:43 +00:00

Severity: LOW — not reachable against the current provider, but it is a 500 where there used to be a clean refusal

Found while implementing #149, and deliberately not fixed there: every available fix is a product decision that issue had not taken.

The bug

User.email is unique=True (backend/app/models/models.py:294), and the callback stores (userinfo.get("email") or "").lower() — so an identity with no email claim is written with the empty string.

Before #149, is_email_allowed("") returned False and such a login was refused at the gate, before any row was written. That gate is gone. Now the first email-less identity is created with email = "", and the second collides on the unique index → IntegrityError → 500.

Not a privilege problem. Accounts are keyed on provider_sub, so no takeover is possible and no session is issued — the write simply fails. It is a crash where a refusal belongs.

Not currently reachable

Authentik binds the email scope to the Circa provider and every account on that instance is admin-created with an address. So this needs either a provider that omits the claim, a scope binding removed, or a second provider added later.

Recording it rather than fixing it blind, because it becomes reachable the moment any of those changes and the symptom — a 500 on the second such person's first login — points nowhere near the cause.

The options, none of which is obviously right

Refuse a login with no email claim. Restores the old behaviour with a clear message. But it reintroduces an email-shaped precondition immediately after #149 removed email from the authorization path, and the reason would now be database schema rather than security — a bad reason to refuse someone the IdP has authenticated.
Make email nullable and drop it from the unique index, or make the index partial on email IS NOT NULL. Most honest about the data: an address the provider did not supply is genuinely unknown, and NULL says so where "" does not. Costs a migration, and every consumer of user.email needs checking for the None case — the display path in particular (#107 renders display names, and the fallback chain ends at the address).
Synthesise a placeholder from provider_sub. Keeps the column non-null and unique with no migration, and is the cheapest. But it invents a fact — the UI would show something that looks like an address and is not one, which is the #78 shape this project keeps refusing elsewhere.

My inclination is (2): it is the only one that records what is actually known. But it is a migration and a sweep of every user.email reader, so it wants doing deliberately rather than folded into an unrelated change.

Worth doing at the same time

UserRepository.get_by_email has zero call sites in app/ or tests/ — verified while implementing #149. It is the one piece of code that could quietly make the address load-bearing again, so it should go, or gain a comment saying why it exists.

References

backend/app/models/models.py:294 — the unique constraint
backend/app/api/routes/auth.py — the callback's upsert
#149 (removed the gate that made this unreachable), #107 (display names)

## Severity: LOW — not reachable against the current provider, but it is a 500 where there used to be a clean refusal Found while implementing #149, and deliberately not fixed there: every available fix is a product decision that issue had not taken. ## The bug `User.email` is `unique=True` (`backend/app/models/models.py:294`), and the callback stores `(userinfo.get("email") or "").lower()` — so an identity with no `email` claim is written with the empty string. Before #149, `is_email_allowed("")` returned `False` and such a login was refused at the gate, before any row was written. That gate is gone. Now the first email-less identity is created with `email = ""`, and the **second** collides on the unique index → `IntegrityError` → 500. Not a privilege problem. Accounts are keyed on `provider_sub`, so no takeover is possible and no session is issued — the write simply fails. It is a crash where a refusal belongs. ## Not currently reachable Authentik binds the `email` scope to the Circa provider and every account on that instance is admin-created with an address. So this needs either a provider that omits the claim, a scope binding removed, or a second provider added later. Recording it rather than fixing it blind, because it becomes reachable the moment any of those changes and the symptom — a 500 on the second such person's first login — points nowhere near the cause. ## The options, none of which is obviously right 1. **Refuse a login with no email claim.** Restores the old behaviour with a clear message. But it reintroduces an email-shaped precondition immediately after #149 removed email from the authorization path, and the reason would now be database schema rather than security — a bad reason to refuse someone the IdP has authenticated. 2. **Make `email` nullable and drop it from the unique index**, or make the index partial on `email IS NOT NULL`. Most honest about the data: an address the provider did not supply is genuinely unknown, and `NULL` says so where `""` does not. Costs a migration, and every consumer of `user.email` needs checking for the None case — the display path in particular (#107 renders display names, and the fallback chain ends at the address). 3. **Synthesise a placeholder** from `provider_sub`. Keeps the column non-null and unique with no migration, and is the cheapest. But it invents a fact — the UI would show something that looks like an address and is not one, which is the #78 shape this project keeps refusing elsewhere. My inclination is (2): it is the only one that records what is actually known. But it is a migration and a sweep of every `user.email` reader, so it wants doing deliberately rather than folded into an unrelated change. ## Worth doing at the same time `UserRepository.get_by_email` has **zero call sites** in `app/` or `tests/` — verified while implementing #149. It is the one piece of code that could quietly make the address load-bearing again, so it should go, or gain a comment saying why it exists. ## References - `backend/app/models/models.py:294` — the unique constraint - `backend/app/api/routes/auth.py` — the callback's upsert - #149 (removed the gate that made this unreachable), #107 (display names)

claude-bot referenced this issue

2026-08-08 17:26:08 +00:00

Authorization should come from Authentik: drop the email_verified gate, take roles from groups #149