Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 43 additions & 18 deletions docs/architecture/adrs/0165-login-never-answers-404.es.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,30 @@
| Código | Significado | Estado anterior |
| --- | --- | --- |
| `AUTH_002` | El código de tenant no existe | 404 |
| `AUTH_004` | El usuario no existe en ese tenant | 404 |
| `AUTH_004` | El IdP autenticó a alguien sin cuenta UMS en ese tenant | 404 |
| `AUTH_006` | Las credenciales no autentican | 401 |

Los mensajes de `AUTH_004` y `AUTH_006` son **idénticos palabra por palabra**: «No pudimos iniciar
sesión. Verifique sus credenciales.» Alguien los escribió así deliberadamente, para que quien
intenta entrar no pueda distinguir «ese usuario no existe» de «existe y te equivocaste». El estado
HTTP deshacía ese cuidado: bastaba mirar si la respuesta era 404 o 401 para saber cuál de las dos
cosas había pasado, sin leer el cuerpo.

Eso es un **oráculo de enumeración**: un atacante sin credenciales puede recorrer una lista de
correos contra `POST /api/v1/auth/login` y separar los que existen de los que no, a razón de una
petición por candidato. Lo que se obtiene no es acceso, pero sí la mitad del trabajo: una lista
depurada de cuentas reales sobre la que concentrar el resto del esfuerzo. Y la mitad cara, porque
es la que no se puede adivinar.
intenta entrar no pueda distinguir un desenlace del otro. El estado HTTP deshacía ese cuidado:
bastaba mirar si la respuesta era 404 o 401 para saber cuál de las dos cosas había pasado, sin leer
el cuerpo. Eso es un **oráculo**: el estado contesta lo que el mensaje se negaba a contestar.

Conviene decir con precisión a quién sirve cada oráculo, porque no son el mismo y la primera
redacción de este ADR los confundió:

- **`AUTH_004` no es «el usuario no existe».** En la rama local, un usuario inexistente devuelve
`AUTH_006` —el mismo código que una contraseña equivocada—, y ahí nunca hubo nada que filtrar
(`AuthenticateUserCommandHandler.AuthenticateLocalAsync`). `AUTH_004` sólo se emite en la rama
federada y **después** de que la cadena de IdP haya autenticado con éxito (`AuthenticateIdpAsync`):
para llegar a ese 404 hay que traer credenciales válidas del IdP. No es, por tanto, el barrido
anónimo de una lista de correos, sino un oráculo para quien ya está dentro — alguien con cuenta en
el IdP corporativo puede recorrer inquilinos y averiguar en cuáles tiene cuenta UMS una identidad
federada. Sigue siendo lo que el mensaje calla a propósito y el estado regalaba.
- **El oráculo anónimo y barato era `AUTH_002`.** La búsqueda del inquilino ocurre antes de tocar
credencial alguna, así que `{ tenantCode: «CANDIDATO», username: …, password: cualquiera }` separa
códigos de inquilino reales de inventados a razón de una petición por candidato y sin credencial
de ningún tipo. Ese sí es el barrido a escala, y §2.3 explica por qué aquí no se cierra del todo.

El desajuste salió a la luz por otro camino: la verificación de contrato Pact esperaba `400` para
un tenant inexistente y la API respondía `404`. El contrato tenía razón por accidente.
Expand All @@ -41,10 +51,10 @@ responde una pregunta que quien pregunta no ha demostrado tener derecho a hacer.

### §2.2 — Todo lo que huele a credencial responde 401

`AUTH_004` (usuario inexistente) pasa a `401`, el mismo estado que `AUTH_006` (credenciales
inválidas) y que `AUTH_005` (cuenta inactiva). Las tres respuestas son ahora indistinguibles desde
fuera salvo por el `code` del cuerpo, que es información que ya se entrega a quien la pide de
frente y que las pantallas necesitan para elegir el mensaje.
`AUTH_004` (el IdP autenticó a alguien que aquí no tiene cuenta) pasa a `401`, el mismo estado que
`AUTH_006` (credenciales inválidas) y que `AUTH_005` (cuenta inactiva). Las tres respuestas son
ahora indistinguibles desde fuera salvo por el `code` del cuerpo, que es información que ya se
entrega a quien la pide de frente y que las pantallas necesitan para elegir el mensaje.

> El `code` sigue diferenciándolas. Eso es deliberado y no reabre el oráculo en la práctica: quien
> enumera lo hace a escala y automatizado, y el cuerpo ya viaja con el mensaje que lo delataría de
Expand Down Expand Up @@ -72,6 +82,20 @@ precisión lo que pasó —la petición trae un código que no existe— y es lo
> `/auth/login`— y merece su propio ADR y su propia conversación con producto. Queda señalado
> aquí para que la asimetría no se lea como olvido.

### §2.4 — `/client/authenticate` tampoco responde ya 404

Al escribir §2.3 se comprobó que el endpoint que se citaba como ejemplo de enumeración cerrada
tenía el mismo agujero: `ClientAuthEndpoints.GetStatusCode` mapeaba `AUTH_004` a `404` mientras
colapsaba a 401 todo lo demás de nivel usuario o inquilino. Ahí el defecto era más nítido que en el
portal, porque ese endpoint persigue la indistinguibilidad como diseño explícito —`SpanishMessage`
devuelve la MISMA cadena exacta para `AUTH_002`, `AUTH_003`, `AUTH_004`, `AUTH_006` y `AUTH_017`, y
no hay ningún humano al otro lado a quien darle la pista—, de modo que el estado era la única señal
que quedaba en pie. El comentario de G-053 en ese método afirmaba que «AUTH_004/005 conservan su
semántica»: `AUTH_005` sí había colapsado a 401, `AUTH_004` se quedó en 404 y nadie lo notó.

`AUTH_004` responde ahora `401` también allí. G-053 no se amplía ni se reinterpreta: se termina de
aplicar donde ya regía.

## Consecuencias

- El front no cambia: `auth.service.ts` decide el mensaje por el `code` del cuerpo, no por el
Expand All @@ -83,13 +107,14 @@ precisión lo que pasó —la petición trae un código que no existe— y es lo
dejó de responder hace tiempo.
- Quien lea `MapAuthError` en el futuro verá 401 donde el instinto REST pediría 404. El comentario
del método explica por qué, y este ADR es la razón larga: **no es un descuido, es la decisión**.
- Ningún test afirmaba el `404` de `AUTH_004` en `/client/authenticate` (§2.4). Que se pudiera
cambiar sin tocar una sola aserción es, en sí, parte del hallazgo: la rama no estaba cubierta.

## Alternativas descartadas

- **Dejar `AUTH_002` en 404 y mover sólo `AUTH_004`.** Cierra el oráculo de cuentas, que es el que
importa, pero deja el contrato Pact roto y obliga a explicar por qué un endpoint de
autenticación devuelve 404 para una cosa y 401 para otra sin que la diferencia sea la
sensibilidad del dato.
- **Dejar `AUTH_002` en 404 y mover sólo `AUTH_004`.** Cierra la filtración de nivel usuario, pero
deja el contrato Pact roto y obliga a explicar por qué un endpoint de autenticación devuelve 404
para una cosa y 401 para otra sin que la diferencia sea la sensibilidad del dato.
- **Colapsar `AUTH_002`, `AUTH_004` y `AUTH_006` en un único 401 sin `code`.** Es lo más hermético
y lo peor de usar: quien teclea mal el código de su organización merece que se lo digan. Se
descarta por coste de usabilidad frente a una ganancia que §2.2 ya obtiene en su mayor parte.
61 changes: 44 additions & 17 deletions docs/architecture/adrs/0165-login-never-answers-404.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,30 @@
| Code | Meaning | Previous status |
| --- | --- | --- |
| `AUTH_002` | The tenant code does not exist | 404 |
| `AUTH_004` | The user does not exist in that tenant | 404 |
| `AUTH_004` | The IdP authenticated someone with no UMS account in that tenant | 404 |
| `AUTH_006` | The credentials do not authenticate | 401 |

The messages for `AUTH_004` and `AUTH_006` are **word-for-word identical**: "No pudimos iniciar
sesión. Verifique sus credenciales." Someone wrote them that way on purpose, so that whoever is
trying to sign in cannot tell "that user does not exist" from "it exists and you got it wrong."
The HTTP status undid that care: looking at whether the response was 404 or 401 was enough to know
which of the two had happened, without reading the body at all.

That is an **enumeration oracle**: an attacker with no credentials can run a list of email
addresses against `POST /api/v1/auth/login` and separate the ones that exist from the ones that do
not, one request per candidate. What that yields is not access, but it is half the work — and the
expensive half, because it is the half you cannot guess.
trying to sign in cannot tell one outcome from the other. The HTTP status undid that care: looking
at whether the response was 404 or 401 was enough to know which of the two had happened, without
reading the body at all. That is an **oracle**: the status answers what the message refused to.

It is worth saying precisely who each oracle serves, because they are not the same one and this
ADR's first draft conflated them:

- **`AUTH_004` is not "the user does not exist."** On the local branch, a user who does not exist
yields `AUTH_006` — the same code as a wrong password — and there was never anything to leak
there (`AuthenticateUserCommandHandler.AuthenticateLocalAsync`). `AUTH_004` is emitted only on
the federated branch, and only **after** the IdP chain has authenticated successfully
(`AuthenticateIdpAsync`): reaching that 404 requires valid IdP credentials. So it is not the
anonymous sweep of an email list; it is an oracle for someone already inside — a holder of a
corporate IdP account can walk tenants and learn which of them a federated identity has a UMS
account in. It is still what the message withholds on purpose and the status handed over free.
- **The anonymous, cheap oracle was `AUTH_002`.** The tenant lookup happens before any credential
is touched, so `{ tenantCode: "CANDIDATE", username: …, password: anything }` separates real
tenant codes from invented ones at one request per candidate, with no credentials of any kind.
That is the sweep that scales — and §2.3 explains why it is not fully closed here.

The mismatch surfaced from another direction: the Pact provider verification expected `400` for an
unknown tenant and the API answered `404`. The contract happened to be right.
Expand All @@ -40,10 +51,10 @@ question the asker has not shown any right to ask.

### §2.2 — Anything credential-shaped answers 401

`AUTH_004` (user does not exist) becomes `401`, the same status as `AUTH_006` (invalid credentials)
and `AUTH_005` (inactive account). The three responses are now indistinguishable from outside
except by the body's `code`, which is information already handed to anyone who asks directly and
which the screens need in order to choose their message.
`AUTH_004` (the IdP authenticated someone who has no account here) becomes `401`, the same status
as `AUTH_006` (invalid credentials) and `AUTH_005` (inactive account). The three responses are now
indistinguishable from outside except by the body's `code`, which is information already handed to
anyone who asks directly and which the screens need in order to choose their message.

> The `code` still tells them apart. That is deliberate and does not reopen the oracle in practice:
> enumeration is done at scale and automated, and the body already carries the message that would
Expand Down Expand Up @@ -72,6 +83,20 @@ contract expects.
> G-053 to `/auth/login` — and deserves its own ADR and its own conversation with product. It is
> flagged here so the asymmetry does not read as an oversight.

### §2.4 — `/client/authenticate` stops answering 404 as well

Writing §2.3 turned up the same hole in the very endpoint being cited as the closed case:
`ClientAuthEndpoints.GetStatusCode` mapped `AUTH_004` to `404` while collapsing everything else at
user or tenant level to 401. The defect is sharper there than on the portal, because that endpoint
pursues indistinguishability as explicit design — `SpanishMessage` returns the SAME exact string for
`AUTH_002`, `AUTH_003`, `AUTH_004`, `AUTH_006` and `AUTH_017`, and there is no human on the other
end who needs the hint — so the status was the only signal left standing. G-053's comment on that
method claimed "AUTH_004/005 keep their semantics": `AUTH_005` had in fact collapsed to 401,
`AUTH_004` stayed at 404 and nobody noticed.

`AUTH_004` now answers `401` there too. G-053 is neither widened nor reinterpreted: it is finished
where it already applied.

## Consequences

- The front end does not change: `auth.service.ts` picks its message from the body's `code`, not
Expand All @@ -83,13 +108,15 @@ contract expects.
- Anyone reading `MapAuthError` in the future will see 401 where REST instinct would ask for 404.
The method's comment says why, and this ADR is the long form: **it is not an oversight, it is the
decision**.
- No test asserted the `404` for `AUTH_004` on `/client/authenticate` (§2.4). That it could be
changed without touching a single assertion is itself part of the finding: the branch was
uncovered.

## Alternatives rejected

- **Leave `AUTH_002` at 404 and move only `AUTH_004`.** Closes the account oracle, which is the one
that matters, but leaves the Pact contract broken and forces an explanation of why an
authentication endpoint returns 404 for one thing and 401 for another when the difference is not
the sensitivity of the data.
- **Leave `AUTH_002` at 404 and move only `AUTH_004`.** Closes the user-level leak, but leaves the
Pact contract broken and forces an explanation of why an authentication endpoint returns 404 for
one thing and 401 for another when the difference is not the sensitivity of the data.
- **Collapse `AUTH_002`, `AUTH_004` and `AUTH_006` into a single 401 with no `code`.** The most
airtight and the worst to use: someone who mistypes their organisation's code deserves to be told
so. Rejected on usability cost against a gain that §2.2 already captures for the most part.
5 changes: 3 additions & 2 deletions docs/architecture/technical-debt.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ What the gate had been hiding — each of these is a live defect, not a type-ann
- **Description**: `Ums.ContractTest` failed 37/38 from the moment of the resync and stayed red on `main` (`c011e42a`). Two failures, one interaction each, both on `POST /api/v1/auth/login`:
1. The pact expected `{ status, title }` — ASP.NET's default `ProblemDetails` — and the API returns `LoginErrorResponse`: `{ code, message, supportReferenceId }`.
2. The pact expected `400` for an unknown tenant and the API answered `404`.
- **What the second one actually was**: not a contract nicety. `AUTH_002` (unknown tenant) and `AUTH_004` (unknown user) both returned `404`, while `AUTH_006` (bad credentials) returned `401`. The messages for `AUTH_004` and `AUTH_006` are word-for-word identical — deliberately, so that nobody can tell "that account does not exist" from "wrong password". The status code gave away exactly what the message was written to hide, which makes the login endpoint an **enumeration oracle**: one request per candidate email separates real accounts from invented ones.
- **What the second one actually was**: not a contract nicety. `AUTH_002` (unknown tenant) and `AUTH_004` both returned `404`, while `AUTH_006` (bad credentials) returned `401`. The messages for `AUTH_004` and `AUTH_006` are word-for-word identical — deliberately, so that nobody can tell one outcome from the other. The status code gave away exactly what the message was written to hide. Which oracle each one is, precisely: `AUTH_002` is the anonymous one — the tenant lookup precedes any credential check, so one request per candidate separates real tenant codes from invented ones with no credentials at all. `AUTH_004` is **not** "unknown user" (the local branch answers `AUTH_006` for that, and never leaked); it fires only after a successful IdP authentication, so it is an oracle for someone already inside: which tenants a federated identity has a UMS account in. See [ADR-0165](./adrs/0165-login-never-answers-404.md) — its first draft conflated the two and was corrected.
- **Why it went unnoticed**: the contract suite had been red since the resync, so a genuine finding was sitting inside a check everyone had learned to expect red. The pact was right and nobody read it.
- **A second, quieter defect in the same file**: the `Content-Type` assertion used `Match.Type("application/problem+json")`. A type matcher on a string matches **any** string, so that line reported OK against a content type the API no longer sends. The check existed and checked nothing — the same shape as the GraphQL-key finding in TD-004.

Expand All @@ -190,7 +190,8 @@ What the gate had been hiding — each of these is a live defect, not a type-ann
- `Content-Type` is now asserted by exact value.
- `AUTH_002` → `400`; `AUTH_004` → `401`, indistinguishable from `AUTH_006`. **No login branch returns 404.** Recorded as [ADR-0165](./adrs/0165-login-never-answers-404.md), because 401-where-REST-would-say-404 is the kind of thing a future reader corrects back unless the reason is written down.
- The front end needed no change: `auth.service.ts` selects its message from the body's `code`, never from the status.
- `/client/authenticate` had the same 404 for `AUTH_004` — inside the endpoint G-053 had hardened, where every message is already the same string and the status was the last signal left. Now 401 as well (ADR-0165 §2.4). No test asserted the old 404.

- **Verified**: `Ums.ContractTest` 38/38.
- **Still owed**: report upstream — `unimar-ums` has the same `MapAuthError`.
- **Reported upstream**: `unimar-ums` has the same `MapAuthError`, the same `ClientAuthEndpoints` 404 and the same pact defects — [unimar-peru/unimar-ums#214](https://github.com/unimar-peru/unimar-ums/issues/214).

Original file line number Diff line number Diff line change
Expand Up @@ -469,13 +469,19 @@ private static async Task<IResult> HandleClientAuthenticateAsync(
// (AUTH_002) y «tenant inactivo» (AUTH_003) NO deben distinguirse de «credenciales
// inválidas» (AUTH_006). Antes AUTH_002→404 y AUTH_003→400 permitían enumerar códigos
// de inquilino comparando el status contra el 401 de credenciales. Ahora todos colapsan
// a 401 con un mensaje genérico e indistinguible. AUTH_004/005 (nivel usuario, tras auth
// del IDP) y AUTH_011/012 (infraestructura) conservan su semántica.
// a 401 con un mensaje genérico e indistinguible. AUTH_011/012 (infraestructura)
// conservan su semántica: no ponen ninguna credencial en duda.
//
// ADR-0165 §2.4: AUTH_004 se sumó al colapso, tarde. `SpanishMessage` ya devolvía para él
// la MISMA cadena exacta que para AUTH_002/003/006/017 —aquí no hay humano a quien darle
// la pista—, así que el 404 era la única señal que quedaba en pie, y distinguía «el IdP te
// autenticó pero no tienes cuenta UMS en este inquilino» de «no te autenticaste». Con el
// cuerpo ya uniformado, el status era el último resquicio del oráculo que G-053 cerró.
private static int GetStatusCode(string error) => error switch
{
var e when e.StartsWith("AUTH_002") => StatusCodes.Status401Unauthorized,
var e when e.StartsWith("AUTH_003") => StatusCodes.Status401Unauthorized,
var e when e.StartsWith("AUTH_004") => StatusCodes.Status404NotFound,
var e when e.StartsWith("AUTH_004") => StatusCodes.Status401Unauthorized,
var e when e.StartsWith("AUTH_005") => StatusCodes.Status401Unauthorized,
var e when e.StartsWith("AUTH_006") => StatusCodes.Status401Unauthorized,
// ADR-UMS-095: bloqueo temporal de cuenta (AUTH_017). Anti-enumeración G-053: en este endpoint
Expand Down
Loading