From 5c8430f2070aec6c1986b997b7d9dad9f1e4193e Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Fri, 31 Jul 2026 09:35:14 +0200 Subject: [PATCH] Treat member order as insignificant when resolving a group "jane+john" and "john+jane" name the same group, but they did not behave that way. Jellyfin only routes a login here when no account matches the typed name, so logging in with the reversed spelling of an existing group found nothing and quietly created a second shared account for the same two people - each with its own watched state. Group identity is now order-independent: - Member names are sorted alphabetically when building an account name, so a given set of members always produces the same name. - Before creating anything, the login path looks for an existing group whose members are exactly the named set, compared as a set rather than a sequence, and logs into that account if it finds one. - Stored member lists are kept in the same canonical order on create and update, so a group's stored order does not depend on the order an admin happened to select members in. Passing no name through to provisioning lets it generate the canonical name, rather than preserving whatever order was typed. Members are also now checked in the order they were typed, stopping at the first match, so whoever puts their own name first is verified first. Verification is a deliberately slow hash comparison, so the ordering is worth having; it is only a preference, and any member's password still unlocks the group. --- .../DynamicGroupTests.cs | 113 +++++++++++++++++- .../StubCryptoProvider.cs | 7 ++ .../Services/DynamicGroupService.cs | 63 +++++++++- .../Services/ProvisioningService.cs | 20 +++- README.md | 19 ++- 5 files changed, 208 insertions(+), 14 deletions(-) diff --git a/Jellyfin.Plugin.WatchedTogether.Tests/DynamicGroupTests.cs b/Jellyfin.Plugin.WatchedTogether.Tests/DynamicGroupTests.cs index 04a25c5..8ac954f 100644 --- a/Jellyfin.Plugin.WatchedTogether.Tests/DynamicGroupTests.cs +++ b/Jellyfin.Plugin.WatchedTogether.Tests/DynamicGroupTests.cs @@ -45,7 +45,8 @@ public class DynamicGroupTests private sealed record Harness( DynamicGroupService Service, - Mock Provisioning); + Mock Provisioning, + StubCryptoProvider Crypto); private static Harness MakeService( IReadOnlyList knownUsers, @@ -67,6 +68,13 @@ public class DynamicGroupTests return null!; }); + // Any known user must also resolve by id, so an existing group can be followed back to its + // shared account. + foreach (var u in knownUsers) + { + userManager.Setup(m => m.GetUserById(u.Id)).Returns(u); + } + var provisioning = new Mock(); var createdShared = MakeUser("created-shared"); @@ -78,13 +86,15 @@ public class DynamicGroupTests userManager.Setup(m => m.GetUserById(createdShared.Id)).Returns(createdShared); + var crypto = new StubCryptoProvider(validPairs); + var service = new DynamicGroupService( userManager.Object, provisioning.Object, - new StubCryptoProvider(validPairs), + crypto, NullLogger.Instance); - return new Harness(service, provisioning); + return new Harness(service, provisioning, crypto); } [Fact] @@ -98,13 +108,108 @@ public class DynamicGroupTests var result = await h.Service.TryCreateFromLoginAsync("alice+bob", "alice-pw"); Assert.NotNull(result); + // No name is passed: provisioning generates the canonical alphabetical one. h.Provisioning.Verify( p => p.CreateGroupAsync( It.Is>(ids => ids.Count == 2), - "alice+bob"), + null), Times.Once); } + [Fact] + public async Task ReversedNameOrder_ReusesTheExistingGroup() + { + // "john+jane" and "jane+john" are the same group. Jellyfin only calls this code when no + // account matches the typed name, so without an order-independent lookup the reversed + // spelling would quietly create a second account for the same two people. + using var ctx = PluginTestContext.Create(); + var alice = MakeUser("alice", AliceHash); + var bob = MakeUser("bob", BobHash); + var shared = MakeUser("alice+bob"); + + ctx.Configuration.Groups.Add(new SharedGroup + { + SharedUserId = shared.Id, + MemberUserIds = [alice.Id, bob.Id] + }); + + var h = MakeService([alice, bob, shared], (BobHash, "bob-pw")); + + var result = await h.Service.TryCreateFromLoginAsync("bob+alice", "bob-pw"); + + Assert.NotNull(result); + Assert.Equal("alice+bob", result!.SharedUsername); + h.Provisioning.VerifyNoOtherCalls(); + } + + [Fact] + public async Task ANewGroup_IsNamedCanonically() + { + // Provisioning is asked for no particular name so it generates the canonical sorted one, + // rather than preserving whatever order happened to be typed. + using var ctx = PluginTestContext.Create(); + var alice = MakeUser("alice", AliceHash); + var bob = MakeUser("bob", BobHash); + var h = MakeService([alice, bob], (BobHash, "bob-pw")); + + await h.Service.TryCreateFromLoginAsync("bob+alice", "bob-pw"); + + h.Provisioning.Verify( + p => p.CreateGroupAsync(It.IsAny>(), null), + Times.Once); + } + + [Fact] + public async Task ReversedNameOrder_WhenTheGroupIsDisabled_IsRejected() + { + using var ctx = PluginTestContext.Create(); + var alice = MakeUser("alice", AliceHash); + var bob = MakeUser("bob", BobHash); + var shared = MakeUser("alice+bob"); + + ctx.Configuration.Groups.Add(new SharedGroup + { + SharedUserId = shared.Id, + MemberUserIds = [alice.Id, bob.Id], + IsDisabled = true + }); + + var h = MakeService([alice, bob, shared], (BobHash, "bob-pw")); + + Assert.Null(await h.Service.TryCreateFromLoginAsync("bob+alice", "bob-pw")); + h.Provisioning.VerifyNoOtherCalls(); + } + + [Fact] + public async Task TheFirstTypedMember_HasTheirPasswordCheckedFirst() + { + // Password verification is a deliberately slow hash comparison, so whoever puts their own + // name first should be checked first. + using var ctx = PluginTestContext.Create(); + var alice = MakeUser("alice", AliceHash); + var bob = MakeUser("bob", BobHash); + var h = MakeService([alice, bob], (BobHash, "bob-pw")); + + await h.Service.TryCreateFromLoginAsync("bob+alice", "bob-pw"); + + // Bob was typed first and his password matched, so alice's hash is never touched. + Assert.Equal(["B2B2B2B2"], h.Crypto.VerifiedSalts); + } + + [Fact] + public async Task ALaterMembersPassword_StillWorks() + { + // The first-typed member is only a preference: the second member's password must still + // unlock the group once the first fails to match. + using var ctx = PluginTestContext.Create(); + var alice = MakeUser("alice", AliceHash); + var bob = MakeUser("bob", BobHash); + var h = MakeService([alice, bob], (BobHash, "bob-pw")); + + Assert.NotNull(await h.Service.TryCreateFromLoginAsync("alice+bob", "bob-pw")); + Assert.Equal(["A1A1A1A1", "B2B2B2B2"], h.Crypto.VerifiedSalts); + } + [Fact] public async Task AnyNamedMembersPassword_Works() { diff --git a/Jellyfin.Plugin.WatchedTogether.Tests/StubCryptoProvider.cs b/Jellyfin.Plugin.WatchedTogether.Tests/StubCryptoProvider.cs index 8250f74..2a7410d 100644 --- a/Jellyfin.Plugin.WatchedTogether.Tests/StubCryptoProvider.cs +++ b/Jellyfin.Plugin.WatchedTogether.Tests/StubCryptoProvider.cs @@ -21,6 +21,12 @@ public sealed class StubCryptoProvider : ICryptoProvider _validPairs = validPairs; } + /// + /// Gets the salt of each credential this provider was asked to verify, in call order. Lets a + /// test assert which member's password was checked first. + /// + public List VerifiedSalts { get; } = new(); + public string DefaultHashMethod => "PBKDF2-SHA512"; public bool Verify(PasswordHash hash, ReadOnlySpan password) @@ -30,6 +36,7 @@ public sealed class StubCryptoProvider : ICryptoProvider // Identify the stored credential by its salt rather than by re-formatting the whole hash, // which need not round-trip through Parse/ToString byte for byte. var salt = Convert.ToHexString(hash.Salt); + VerifiedSalts.Add(salt); foreach (var (validHash, validPassword) in _validPairs) { diff --git a/Jellyfin.Plugin.WatchedTogether/Services/DynamicGroupService.cs b/Jellyfin.Plugin.WatchedTogether/Services/DynamicGroupService.cs index e5f4031..8de6968 100644 --- a/Jellyfin.Plugin.WatchedTogether/Services/DynamicGroupService.cs +++ b/Jellyfin.Plugin.WatchedTogether/Services/DynamicGroupService.cs @@ -116,7 +116,12 @@ public class DynamicGroupService : IDynamicGroupService // The password must belong to one of the named members. Without this any visitor could // conjure a shared account out of two usernames they happened to know. - if (!members.Any(m => VerifyPassword(m, password))) + // + // Members are tried in the order they were typed and the loop stops at the first match, so + // whoever types their own name first has their password checked first. Verification is a + // deliberately slow hash comparison, so the ordering is worth having. + var matched = members.FirstOrDefault(m => VerifyPassword(m, password)); + if (matched is null) { _logger.LogWarning( "Dynamic group login for {Username} rejected: no named member's password matched", @@ -124,11 +129,45 @@ public class DynamicGroupService : IDynamicGroupService return null; } + var memberIds = members.Select(m => m.Id).ToList(); + + // The same people in a different order are the same group: someone typing "john+jane" must + // land on the existing "jane+john" account rather than creating a second one. Jellyfin only + // reaches this code when no account matches the typed name, so without this check every + // ordering would spawn its own account. + var existing = FindGroupWithSameMembers(config, memberIds); + if (existing is not null) + { + if (existing.IsDisabled) + { + _logger.LogWarning( + "Login for {Username} rejected: the matching group is disabled", + enteredUsername); + return null; + } + + var existingUser = _userManager.GetUserById(existing.SharedUserId); + if (existingUser is null) + { + _logger.LogWarning( + "Group for {Username} references a shared account that no longer exists", + enteredUsername); + return null; + } + + _logger.LogInformation( + "Login as {Entered} resolved to the existing shared account {Username}", + enteredUsername, + existingUser.Username); + + return new DynamicGroupResult(existing, existingUser.Username); + } + + // Passing no name lets provisioning generate the canonical alphabetically-sorted one, so + // the account is named the same whichever order the members were typed in. // The account is limited to the libraries all named members share, so creating one at the // login screen cannot grant anybody access they did not already have. - var group = await _provisioningService.CreateGroupAsync( - members.Select(m => m.Id).ToList(), - enteredUsername).ConfigureAwait(false); + var group = await _provisioningService.CreateGroupAsync(memberIds, null).ConfigureAwait(false); var sharedUser = _userManager.GetUserById(group.SharedUserId); if (sharedUser is null) @@ -144,6 +183,22 @@ public class DynamicGroupService : IDynamicGroupService return new DynamicGroupResult(group, sharedUser.Username); } + /// + /// Finds a configured group whose members are exactly the given set, ignoring order. + /// + /// The plugin configuration to search. + /// The member identifiers to match. + /// The matching group, or null if no group has that membership. + private static SharedGroup? FindGroupWithSameMembers( + PluginConfiguration config, + IReadOnlyList memberIds) + { + var wanted = memberIds.ToHashSet(); + + return config.Groups.FirstOrDefault(g => + g.MemberUserIds.Count == wanted.Count && wanted.SetEquals(g.MemberUserIds)); + } + /// /// Verifies a submitted password against a member's live stored hash. /// diff --git a/Jellyfin.Plugin.WatchedTogether/Services/ProvisioningService.cs b/Jellyfin.Plugin.WatchedTogether/Services/ProvisioningService.cs index 296c808..0badb9f 100644 --- a/Jellyfin.Plugin.WatchedTogether/Services/ProvisioningService.cs +++ b/Jellyfin.Plugin.WatchedTogether/Services/ProvisioningService.cs @@ -88,6 +88,12 @@ public class ProvisioningService : IProvisioningService members.Add(member); } + // Store members in the same alphabetical order the generated name uses, so that the stored + // order is canonical however the members were supplied. Password checks then always run in + // a predictable order too. + members = members.OrderBy(m => m.Username, StringComparer.OrdinalIgnoreCase).ToList(); + distinctIds = members.Select(m => m.Id).ToList(); + var accountName = string.IsNullOrWhiteSpace(name) ? BuildDefaultName(members.Select(m => m.Username), config.NameSeparator) : name.Trim(); @@ -165,6 +171,11 @@ public class ProvisioningService : IProvisioningService } } + // Keep the stored order canonical, matching how groups are created. + distinctIds = distinctIds + .OrderBy(id => _userManager.GetUserById(id)?.Username, StringComparer.OrdinalIgnoreCase) + .ToList(); + group.MemberUserIds = distinctIds; group.SyncUnwatched = syncUnwatched; group.SyncPlayCount = syncPlayCount; @@ -211,15 +222,20 @@ public class ProvisioningService : IProvisioningService /// /// Joins member names into a display name, falling back to a generic name if the result would - /// exceed the username column limit. Membership is tracked by GUID, so the name is cosmetic. + /// exceed the username column limit. /// + /// + /// Names are sorted alphabetically so that a given set of members always produces the same + /// account name. Without this, "jane+john" and "john+jane" would be two different names for the + /// same group and would end up as two separate accounts. + /// /// The member usernames. /// The configured separator. /// A name that fits within the username length limit. private static string BuildDefaultName(IEnumerable usernames, string separator) { var sep = string.IsNullOrEmpty(separator) ? "+" : separator; - var joined = string.Join(sep, usernames); + var joined = string.Join(sep, usernames.OrderBy(n => n, StringComparer.OrdinalIgnoreCase)); if (joined.Length <= MaxUsernameLength) { diff --git a/README.md b/README.md index f003b67..973654d 100644 --- a/README.md +++ b/README.md @@ -81,13 +81,23 @@ On an unrecognised name, it: 1. Splits the name on the separator (`+` by default) — `alice+bob+carol` → three parts. 2. Requires **every part** to be an existing, enabled user that is not itself a shared account. -3. Requires the submitted password to match **one of those members'** stored hashes. -4. Only then creates the shared account, and returns its name so Jellyfin completes the login. +3. Requires the submitted password to match **one of those members'** stored hashes. Members are + checked in the order you typed them and the check stops at the first match, so putting your own + name first is marginally quicker. +4. Looks for an existing group with exactly those members. If one exists, you are logged into it. +5. Otherwise creates the shared account, and returns its name so Jellyfin completes the login. Step 3 is what stops this being an open door: knowing two usernames is not enough to bring an account into being. If any check fails, the plugin declines and the login fails exactly as an ordinary typo would. +#### Order does not matter + +`john+jane` and `jane+john` are the same group. Member names are sorted alphabetically to build the +account name, and the lookup in step 4 compares members as a set, so both spellings resolve to one +account rather than creating a second one for the same two people. The account itself is named with +the sorted spelling — `jane+john` — whichever order you happened to type. + #### The name collision, and why it is harmless `+` is a legal Jellyfin username character: @@ -165,7 +175,7 @@ Download the release `.zip`, extract it into a `WatchedTogether` folder inside y On the shared device, at the Jellyfin login screen: -- **Username:** `alice+bob` (the members' usernames, joined with `+`) +- **Username:** `alice+bob` (the members' usernames, joined with `+`, in any order) - **Password:** your own That is the whole setup. The account is created on first use and reused from then on. Add a third @@ -177,7 +187,8 @@ If you would rather provision groups explicitly — or you have turned auto-crea 1. Go to **Dashboard → Plugins → Watched Together**. 2. Under **Create a group**, select **two or more** members. -3. Optionally give the account a name. Left blank, the member names are joined with `+`. +3. Optionally give the account a name. Left blank, the member names are sorted alphabetically and + joined with `+`. 4. Click **Create group**. Either way, a new user appears in your user list and can be renamed like any other.