fix(geoip): call the suspicious-login and impossible-travel detectors when a sign-in completes (#215)

[camarigor]

Summary

Fixes #215: the two GeoIP detectors are now called when a sign-in completes, so the databases an admin configures actually produce the alerts the README promises.

SuspiciousLoginDetector and ImpossibleTravelDetector were complete and registered, and nothing ever called ObserveAsync. This adds a SignInObserver that hands a finished sign-in to both, calls it from every place the plugin already records a successful sign-in, and adds one Diagnostics row so an admin can tell "nothing suspicious happened" from "nothing is calling the detectors". Neither detector changes; both keep guarding their own preconditions.

Type of change

• Bug fix (non-breaking)
• New feature (non-breaking)
• Breaking change (existing behaviour, config, or API changes)
• Security fix
• Documentation only
• CI / build / tests only
• Refactor (no functional change)

A user who configured the databases gets alerts that never arrived before, which is the fix, not a new feature. Anyone who never configured them sees no change: both detectors still return early when no database is open, and the new Diagnostics row says so.

Related issues

Fixes #215. The detectors date back to the first commit of the published history (cc80f91, v2.2.2); git log -S ObserveAsync -- src shows no call site was ever added or removed. The comment on NotificationService.NotifySuspiciousLoginAsync already describes the call this adds: "Fired by SuspiciousLoginDetector before the challenge response is returned, fire-and-forget at the call site".

How was this tested?

• Added or updated unit tests
• Added or updated integration tests
• Tested manually against a running Jellyfin server (state version)
• N/A (explain why below)

Server tested against: Jellyfin 12.0.0, official Docker image, with the Jellyfin 12 package built from this branch by build.sh. Clients: curl, driving the same endpoints the web client and a native client use.

The controller paths. Fresh server, three DB-IP Lite databases loaded (Ok in Diagnostics), impossible travel at the default 900 km/h, webhook receiver listening, one account with TOTP:

sign-in from 8.8.8.8      (password + TOTP)  -> Verify 200
sign-in from 200.160.2.3  (password + TOTP)  -> Verify 200     33 s later, 10386 km

suspicious_login   admin signed in from US via Google LLC (IP 8.8.8.8)
suspicious_login   admin signed in from BR via Núcleo de Inf. e Coord. do Ponto BR - NIC.BR (IP 200.160.2.3)
suspicious_login   admin signed in from BR via Impossible travel: 10386km in 0.0h ≈ 1146296km/h from US (IP 200.160.2.3)
[WRN] ImpossibleTravelDetector: [2FA] Impossible travel for admin: 10386km in 0.00906h = 1146296km/h

The paths that never reach the controller. Same setup, but the second sign-in is an app password (a native client signing in through /Users/AuthenticateByName, which the provider and the session handler serve, never Verify):

sign-in from 8.8.8.8      password + TOTP          -> Verify 200                     (controller path)
sign-in from 200.160.2.3  app password, no 2FA UI  -> AuthenticateByName 200          (session-handler path)

suspicious_login   admin signed in from US via Google LLC (IP 8.8.8.8)
suspicious_login   admin signed in from BR via Núcleo de Inf. e Coord. do Ponto BR - NIC.BR (IP 200.160.2.3)
suspicious_login   admin signed in from BR via Impossible travel: 10386km in 0.0h ≈ 9239241km/h from US (IP 200.160.2.3)

The user record after both, which stayed empty before this change:

"SeenContexts": [{"Asn": 15169, "Country": "US", ...}, {"Asn": 22548, "Country": "BR", ...}],
"LastLocation": {"Country": "BR", "Latitude": -23.5611, "Longitude": -46.6876, "Ip": "200.160.2.3", ...}

And the Diagnostics row, before any sign-in and after:

signin_observation  Ok  no sign-in observed since the server started
signin_observation  Ok  last sign-in observed less than a minute ago (2026-09-15 23:59:15Z)

No [ERR] from the plugin in any run; the only [WRN] is the impossible-travel line itself.

Local. With the .NET SDK 9.0.316 that CI installs: dotnet build JellyfinSecurity.sln -c Release has 0 errors, dotnet test -c Release is 527 passed and 0 failed (511 before this branch), node --test tests/oidc-bridge.test.mjs is 8 passed. With the .NET SDK 10.0.400, dotnet build src/... -p:JellyfinVersion=12.0.0 -c Release has 0 warnings and 0 errors.

The wiring tests bite. I broke the code on purpose and re-ran them: removing any one call site fails the test for that file, and swapping the resolved address for the raw peer (challenge.RemoteIp in the controller, info.RemoteEndPoint in the handler) fails it too. All four mutations reverted afterwards.

What changed

• Services/SignInObserver.cs (new): Observe(userId, username, ip) returns immediately and runs the two detectors in order on a background task, so the two writes to the same user record never race, and a failure in either is logged rather than surfaced to the caller. ShouldObserve holds the "is this worth resolving" decision so it can be tested on its own. LastObservedAt feeds the Diagnostics row.
• PluginServiceRegistrator.cs: registers it as a singleton, next to the detectors.
• Api/TwoFactorAuthController.cs: four calls, at the points that already write an AuditResult.Success row: Verify (TOTP or recovery code), Verify/Passkey/Finish, Enroll/Totp/Confirm (forced enrolment ends in a signed-in session) and Authenticate (trusted-device and password-only sessions).
• Services/AuthenticationEventHandler.cs: five calls, for the sign-ins that never reach the controller: device pre-verified, QuickConnect, app password, paired device and bypass. The reconnect path is deliberately left out, because a websocket reconnect on an already-verified token is not a fresh sign-in, and so is the challenge path, where no sign-in has happened yet.
• Services/BypassEvaluator.cs: ResolveClientIp gains an overload taking the peer and the forwarded header, and the HttpContext version now delegates to it. The handler has no HttpContext on its background task; it already snapshots X-Forwarded-For in OnSessionStarted for exactly this reason, and now that snapshot resolves the address the same way every other caller does.
• Services/DiagnosticsService.cs: one row, signin_observation, saying when a sign-in last reached the detectors. Always Ok, because every state it reports is legitimate: a server that just restarted has none yet, one with no database loaded is not expected to resolve anything. The value is the timestamp. An admin who sees the GeoIP rows Ok and this row stuck on "no sign-in observed" while people are signing in is looking at the bug this row was added for.
• Tests (11 new): the ShouldObserve decision; that a server with no GeoIP database is left untouched and the caller sees no exception; that a skipped sign-in is not counted as observed; the four controller call sites and the five handler call sites, each passing the resolved address, with the reconnect path asserted to have none; the new ResolveClientIp overload's fallback; and the three states of the Diagnostics row.

Every call site passes the proxy-walked address, never the peer. That distinction is not cosmetic. In the app-password run above, the plugin's own audit row for that session recorded 172.18.0.1, the Docker gateway, while the detectors correctly saw 200.160.2.3; feeding them the peer would make every remote user look like one address and impossible travel would never fire behind a reverse proxy.

Checklist

• My code follows the existing style (run dotnet format if unsure)
• I've added comments only where the why isn't obvious from the code
• I've updated the README / SECURITY.md / docs if behaviour or config changed: no doc change, because the README already describes this behaviour; the code now matches it
• I've considered backwards compatibility (config migration, file formats, on-disk state): SeenContexts and LastLocation already exist in UserTwoFactorData and start getting written; nothing is read that was not written before
• I've checked the security implications (auth bypass, secret handling, input validation): the observer runs after the sign-in decision is already made and cannot change it; it never blocks the response; notification content is what the detectors already build
• CI passes (dotnet build + dotnet test green)

Additional notes

A web sign-in with 2FA is observed twice: once by Verify and once by the device-pre-verified branch of the session handler that follows it. That is harmless and deliberate. The second observation carries the same address, so the context is no longer novel and the distance is zero, and no second alert fires. I kept it rather than skipping that branch because an app-password sign-in can also land there (the provider arms both the device pre-verify and the user-scoped one-shot, and whichever matches first wins), and skipping it would silently drop those.

Two things I noticed while testing and deliberately did not change, so this stays a wiring fix. Both are one-line calls for you; say which way you want them and I will send either as its own PR:

• The impossible-travel alert goes out through NotifySuspiciousLoginAsync, so it arrives titled "Sign-in from a new location" with the travel detail packed into the asnOrg field ("BR via Impossible travel: 10386km ..."). It reads oddly next to the genuine new-location alert that fires beside it. A dedicated NotifyImpossibleTravelAsync with its own title and an impossible_travel event name would let webhook consumers tell them apart.
• LastLocation.Asn is 0 because ResolveCity reads it from the City database, which carries no ASN field in DB-IP Lite or in GeoLite2-City. The ASN is available from the ASN database the other detector already has open.