feat(devices): remember the client a second screen approved (#216) - #220
Merged
Merged
Conversation
A sign-in that someone approves on another screen forgets the device the moment it finishes. The OIDC bridge only marks it pre-verified in memory and the QuickConnect allowance is a one-shot with a two-minute life, so a client that cannot render the challenge page, a TV, starts from the same wall on its next session and the user has no record of what they let in. PairDeviceOnSecondScreenApproval, off by default, writes the same PairedDevice record the Setup page's approval flow writes, at the two points where such an approval completes. The row is visible and revocable in the user's device list and can take indefinite trust where the admin allows it. Recording is not granting: a paired device waives 2FA only while BareDeviceIdBypassEnabled is on, which stays off by default. A browser sign-in reaches the same bridge fast path and is deliberately left out, told apart by a flag stamped on the bridge token at the moment the device poll hands it to a client.
…en-approval # Conflicts: # src/Jellyfin.Plugin.TwoFactorAuth/Services/AuthenticationEventHandler.cs
Contributor
Author
|
@ZL154 thanks for merging #202, #217, #218 and #219 today. This is the last one from that set. It conflicted with #217 in the QuickConnect block of It is the second half of #216: after a sign-in someone approved on another screen (the OIDC device flow behind the QR from #218, or QuickConnect), the device is recorded as a paired device instead of being forgotten. It sits behind a new setting that is off by default, and a paired device still only skips 2FA where the bare device ID bypass is on. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The second half of #216. @Heldenkrieger01 asked for two things there: scan a QR and finish the sign-in on a phone, which is #218, and "then trust the device (indefinitely if configured). This could be a configurable option." Nothing covers the second half yet, and this is it.
Both of the sign-ins a person approves on another screen forget the device the moment they finish. The OIDC bridge only marks it pre-verified in
ChallengeStore, which is in memory. The QuickConnect allowance is a one-shot with a two-minute life. Nothing durable is written either way, so a client that cannot render the challenge page starts from the same wall on its next session, and the user has no record of the TV they just let in.This adds a setting that writes the same
PairedDevicerecord the Setup page's approval flow writes, from those two points.Type of change
Related issues
Refs #216. Independent of #218 and #219 and based on current main: the device-poll flow it hooks into has been in the tree since v2.5.9 (#64). The three together are what the reporter's setup needs: #218 shows the code, #219 puts the right host inside it, this one stops the TV being a stranger next time.
What it does, and what it deliberately does not
Resolution is one new setting,
PairDeviceOnSecondScreenApproval, off by default, in Settings under Tunables. When it is on:oidc;quickconnect;device_paired_oidc/device_paired_quickconnect), and moves itsLastUsedAtforward on later approvals so the user's device list does not read as stale.Three limits I want to be explicit about, because the feature is easy to over-read:
BareDeviceIdBypassEnabledis on, which defaults off because a bare DeviceId is client-supplied and not a secret. With that flag off, this setting changes what the user sees in their device list and nothing about what the server accepts. I did not want a second switch that quietly re-opens the first one.Indefinite trust is unchanged: the per-device toggle already exists and is still gated by
AllowIndefiniteTrust. This just gives the TV a row to toggle.How was this tested?
Server tested against: Jellyfin 12.0.0, official Docker image, with Dex 2.41.1 as the identity provider.
Clients tested with: scripted LG webOS and Jellyfin Web clients (device ids and client headers), plus a separate cookie jar standing in for the phone that gives consent.
Every row below is one run of the real flow end to end, read back from
GET TwoFactorAuth/PairedDevicesas the signing-in user and from the audit log:lg-tv-1oidc, auditdevice_paired_oidclg-tv-2pc-browser-1lg-tv-qcquickconnect, auditdevice_paired_quickconnectThe QuickConnect run is the whole dance and not a shortcut: the admin enrolled TOTP, signed in from a "phone" client, got a real 401 challenge, verified it, and only then authorised the code the "TV" had initiated. The pairing row and the existing
quickconnectaudit row land on the same second.Repeating the QuickConnect sign-in twice more added no second row, and
LastUsedAtmoved from 14:06:31 to 14:08:41 whileCreatedAtstayed put.One cosmetic note: the QuickConnect pairing stores an empty
LastIp, becauseSessionInfo.RemoteEndPointis empty on that path. The plugin's ownquickconnectaudit row has the same gap today, so this is Jellyfin's data rather than something the PR introduces.Local, with the .NET SDK 9.0.316 that CI installs:
dotnet build JellyfinSecurity.sln -c Releasehas 0 errors,dotnet test -c Releaseis 530 passed and 0 failed (511 before this branch),node --test tests/oidc-bridge.test.mjsis 8 passed, andnode --checkon the admin script passes. The two build warnings are the pre-existing CS8602 pair inJellyfin12ShellTests.csthat #202 fixes, untouched here.I also mutation-tested the new tests rather than trusting that green means covered. Five deliberate breakages, five red tests, each the right one: removing the provider call site, removing the QuickConnect call site, dropping the device-flow stamp, ignoring the setting, and removing the duplicate check.
What changed
Services/SecondScreenPairing.cs(new): decides and writes. Refuses an empty DeviceId (nothing to match on later), refuses one over 256 characters (client-supplied, and it lands in a file read on every auth request), trims names to 80, matches withBypassEvaluator.DeviceIdMatchesso the Jellyfin Web ids that carry a per-session timestamp suffix do not add a row per app restart, and does the duplicate check inside the mutation so two sign-ins racing cannot both insert.Services/OidcLoginTokenStore.cs: the bridge entry records whether the token was delivered throughPollDeviceFlow, stamped there and returned byConsume.TryUpdatewith the old value, so a token consumed between the lookup and the stamp stays consumed.Services/TwoFactorAuthProvider.cs: pairs in the bridge fast path, behind that flag.Services/AuthenticationEventHandler.cs: pairs where the QuickConnect one-shot is consumed. That is the first point that knows the TV's own DeviceId, since at/QuickConnect/Authorizethe caller is the phone doing the approving.Configuration/PluginConfiguration.cs,Pages/admin.html,Pages/admin-script.js,Pages/translations/*.json: the setting and its help text, two keys in all eight languages.README.md: one line under Native client support.tests/.../SecondScreenPairingTests.cs(new, 19 cases): what is refused, what a record carries, the store round trip for both sources, idempotency and the freshened timestamp, the device-flow stamp including that it cannot resurrect a redeemed token, and two tests that pin the call sites themselves.Additional notes