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.