A token whose account row is gone is admitted, where upstream refuses it #144

New issue

Closed

opened 2026-08-16 08:42:21 +02:00 by Claude · 1 comment

Claude commented

2026-08-16 08:42:21 +02:00

Collaborator

crates/jellyfin-http/src/auth.rs reads the account's permissions as a request
becomes a caller and never asks whether the account is there. Upstream's
DefaultAuthorizationHandler does:

var user = _userManager.GetUserById(userId);
if (user is null)
{
    throw new ResourceNotFoundException();
}

— which its exception middleware maps to 404. Ours reads an empty permission
list, then an empty schedule list, and admits.

Reachable only through a token whose account has since been deleted.
session/users.rs::delete_user takes the tokens with the row, so the dashboard
path closes it; what is left is a restore, a direct write, or a row removed by
anything that does not revoke.

Found in the review of #142/#134 alongside #143, which is the same shape one
layer up — worth doing in the same pass, because both are a row the caller path
does not read.

Where to start

Authenticated::of in crates/jellyfin-http/src/auth.rs. The cost is the point:
this is a third read per authenticated request unless the answer can be had off
one already made. users.permissions() returns an empty vector for an account
that is not there and for one carrying no permission rows, so it cannot stand
in — measure whether by_id is worth it, or whether the repository should
answer both in one statement.

Whatever it costs, say so with a number: #134 argued its two reads and this one
should be argued the same way.

How to know it is done

A token for a deleted account is answered 404 with the body upstream gives, on
a route under each policy type. A route test — deleting an account on the shared
reference is a write.

House rules

Tests run under cargo nextest; upstream's source is the checkout in upstream/
at the pin; comments say what the code does, not how it got there.

`crates/jellyfin-http/src/auth.rs` reads the account's permissions as a request becomes a caller and never asks whether the account is there. Upstream's `DefaultAuthorizationHandler` does: ```csharp var user = _userManager.GetUserById(userId); if (user is null) { throw new ResourceNotFoundException(); } ``` — which its exception middleware maps to `404`. Ours reads an empty permission list, then an empty schedule list, and admits. Reachable only through a token whose account has since been deleted. `session/users.rs::delete_user` takes the tokens with the row, so the dashboard path closes it; what is left is a restore, a direct write, or a row removed by anything that does not revoke. Found in the review of #142/#134 alongside #143, which is the same shape one layer up — worth doing in the same pass, because both are a row the caller path does not read. ## Where to start `Authenticated::of` in `crates/jellyfin-http/src/auth.rs`. The cost is the point: this is a third read per authenticated request unless the answer can be had off one already made. `users.permissions()` returns an empty vector for an account that is not there *and* for one carrying no permission rows, so it cannot stand in — measure whether `by_id` is worth it, or whether the repository should answer both in one statement. Whatever it costs, say so with a number: #134 argued its two reads and this one should be argued the same way. ## How to know it is done A token for a deleted account is answered `404` with the body upstream gives, on a route under each policy type. A route test — deleting an account on the shared reference is a write. ### House rules Tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin**; comments say what the code does, not how it got there.

Claude added the

correctness

http

labels

2026-08-16 08:58:02 +02:00

alex referenced this issue from a commit

2026-08-16 09:28:23 +02:00

auth: a check made at login has to survive the token

Claude referenced this issue

2026-08-16 09:29:46 +02:00

Per-request authorization stops at the token: neither remote access nor the access schedule is checked #134

Claude commented

2026-08-16 09:30:03 +02:00

Author

Collaborator

Closed by 9775776.

This issue's premise was wrong: the 404 is unreachable.
AuthorizationInfo.UserId => User?.Id ?? Guid.Empty, so a token whose account
row has gone carries the empty id — and DefaultAuthorizationHandler returns
at its first arm, !isApiKey && userId.IsEmpty(), before ever reaching the
GetUserById null check that throws ResourceNotFoundException.

And the answer is not one status. That first arm returns without deciding, and
ASP.NET satisfies a requirement when any handler succeeds it — so what a caller
with no account is told depends on whether the policy has another handler that
can. Measured route by route, on a private instance with the row taken out from
under a live token:

policy	upstream
`[Authorize]`, `IgnoreParentalControl`	403, empty — nothing else can succeed the requirement
`FirstTimeSetupOrDefault`, `FirstTimeSetupOrIgnoreParentalControl`	200 — its own handler succeeds on `UserRoles.User`, which every authenticated caller carries
`FirstTimeSetupOrElevated`, `RequiresElevation`, `UserPermissionRequirement`	403 — the role is `User`, not administrator
no attribute	200

So admitted() returns Admitted::{Yes, Undecided} rather than a bare result,
and each policy says what undecided costs it. It also caught a second bug the
issue did not mention: administers() was still reading the dead account's
permission rows, so an orphaned token administered the server. What an account
holds belongs to the account now.

The state needs a write that ignores the constraint. Devices.UserId
cascades, so the route — and any DELETE with foreign keys on — takes the
tokens with the row and the token simply stops resolving, which is 401. What
is left is a restore, a migration, or a hand-edited database. Both are asserted.

The cost. One primary-key read per authenticated request, which is the read
this issue said would have to be argued: 0.464 ms median over a thousand
sequential /Users/Me requests against 0.470 ms without it — below the floor of
that measurement, and recorded in D32 as wanting a retake on a larger library.

Replayed as a pair afterwards: 7 of 8 routes agree exactly. The eighth is
upstream answering 500 on the first request after a restart, where it trips
over the foreign-key violation while stamping the device row it just used — an
artefact of the state I had to manufacture, not a decision; every request after
it is the 403 we give.

Closed by `9775776`. **This issue's premise was wrong: the `404` is unreachable.** `AuthorizationInfo.UserId => User?.Id ?? Guid.Empty`, so a token whose account row has gone carries the *empty* id — and `DefaultAuthorizationHandler` returns at its first arm, `!isApiKey && userId.IsEmpty()`, before ever reaching the `GetUserById` null check that throws `ResourceNotFoundException`. And the answer is not one status. That first arm returns *without deciding*, and ASP.NET satisfies a requirement when any handler succeeds it — so what a caller with no account is told depends on whether the policy has another handler that can. Measured route by route, on a private instance with the row taken out from under a live token: | policy | upstream | |---|---| | `[Authorize]`, `IgnoreParentalControl` | **403**, empty — nothing else can succeed the requirement | | `FirstTimeSetupOrDefault`, `FirstTimeSetupOrIgnoreParentalControl` | **200** — its own handler succeeds on `UserRoles.User`, which every authenticated caller carries | | `FirstTimeSetupOrElevated`, `RequiresElevation`, `UserPermissionRequirement` | **403** — the role is `User`, not administrator | | no attribute | 200 | So `admitted()` returns `Admitted::{Yes, Undecided}` rather than a bare result, and each policy says what undecided costs it. It also caught a second bug the issue did not mention: `administers()` was still reading the dead account's permission rows, so an orphaned token administered the server. What an account holds belongs to the account now. **The state needs a write that ignores the constraint.** `Devices.UserId` cascades, so the route — and any `DELETE` with foreign keys on — takes the tokens with the row and the token simply stops resolving, which is `401`. What is left is a restore, a migration, or a hand-edited database. Both are asserted. **The cost.** One primary-key read per authenticated request, which is the read this issue said would have to be argued: 0.464 ms median over a thousand sequential `/Users/Me` requests against 0.470 ms without it — below the floor of that measurement, and recorded in D32 as wanting a retake on a larger library. Replayed as a pair afterwards: 7 of 8 routes agree exactly. The eighth is upstream answering `500` on the *first* request after a restart, where it trips over the foreign-key violation while stamping the device row it just used — an artefact of the state I had to manufacture, not a decision; every request after it is the `403` we give.

Claude closed this issue

2026-08-16 09:30:04 +02:00

Claude changed title from ~~A token whose account was deleted is admitted, where upstream answers 404~~ to A token whose account row is gone is admitted, where upstream refuses it

2026-08-16 09:30:24 +02:00