Skip to content

feat(devices): remember the client a second screen approved (#216) - #220

Merged
ZL154 merged 2 commits into
ZL154:mainfrom
camarigor:feat/pair-second-screen-approval
Sep 23, 2026
Merged

ZL154 merged 2 commits into
ZL154:mainfrom
camarigor:feat/pair-second-screen-approval

Conversation

@camarigor

Copy link
Copy Markdown
Contributor

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 PairedDevice record the Setup page's approval flow writes, from those two points.

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)

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:

  • a sign-in completed through the OIDC device flow pairs the polling client, source oidc;
  • a sign-in completed through QuickConnect pairs the client that consumed the approval, source quickconnect;
  • either way the record is idempotent per device, carries an audit row (device_paired_oidc / device_paired_quickconnect), and moves its LastUsedAt forward 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:

  1. It does not authenticate anything. An OIDC-only user still signs in through the IdP when the session token is gone. A pairing skips the plugin's 2FA step, not the sign-in itself.
  2. Recording is not granting. A paired device only waives 2FA while BareDeviceIdBypassEnabled is 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.
  3. A browser sign-in is not paired, even though it reaches the same bridge fast path. The desktop case already has the signed trusted-device cookie, and pairing it would put a browser on the bare-DeviceId list. The two are told apart by a flag stamped on the bridge token at the moment the poll hands it to a client, which is the only point that knows how the token travelled.

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?

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

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/PairedDevices as the signing-in user and from the audit log:

Case Setting Sign-in reached the server Paired
OIDC device flow, device lg-tv-1 on yes, session created yes, source oidc, audit device_paired_oidc
OIDC device flow, device lg-tv-2 off yes, session created no row, no audit
Browser OIDC callback, device pc-browser-1 on yes, session created no row, no audit
QuickConnect, device lg-tv-qc on yes, session created yes, source quickconnect, audit device_paired_quickconnect

The 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 quickconnect audit row land on the same second.

Repeating the QuickConnect sign-in twice more added no second row, and LastUsedAt moved from 14:06:31 to 14:08:41 while CreatedAt stayed put.

One cosmetic note: the QuickConnect pairing stores an empty LastIp, because SessionInfo.RemoteEndPoint is empty on that path. The plugin's own quickconnect audit 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 Release has 0 errors, dotnet test -c Release is 530 passed and 0 failed (511 before this branch), node --test tests/oidc-bridge.test.mjs is 8 passed, and node --check on the admin script passes. The two build warnings are the pre-existing CS8602 pair in Jellyfin12ShellTests.cs that #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 with BypassEvaluator.DeviceIdMatches so 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 through PollDeviceFlow, stamped there and returned by Consume. TryUpdate with 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/Authorize the 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

  • Those last two tests read the source files. It is a blunt instrument, and I am using it because GeoIP suspicious-login and impossible-travel detectors are never called #215 was exactly this failure: two detectors sat in the tree for four releases with no caller and every test still passed. A pairing that is never written fails the same silent way.
  • The translations for the seven non-English strings are mine; corrections from native speakers are welcome.
  • Still open on the reporter's side, and not blocking this: whether webOS reports error -27 on the popup itself or only when handing the link to the TV browser.

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
@camarigor

Copy link
Copy Markdown
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 AuthenticationEventHandler, where both PRs add a call at the same spot. I merged main in and kept both, the sign-in observer first and the pairing after it, so it is clean again: CI green and 569 tests passing.

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.

@ZL154
ZL154 merged commit 8240b1f into ZL154:main Sep 23, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants