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.
This commit is contained in:
@@ -45,7 +45,8 @@ public class DynamicGroupTests
|
||||
|
||||
private sealed record Harness(
|
||||
DynamicGroupService Service,
|
||||
Mock<IProvisioningService> Provisioning);
|
||||
Mock<IProvisioningService> Provisioning,
|
||||
StubCryptoProvider Crypto);
|
||||
|
||||
private static Harness MakeService(
|
||||
IReadOnlyList<User> 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<IProvisioningService>();
|
||||
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<DynamicGroupService>.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<IReadOnlyList<Guid>>(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<IReadOnlyList<Guid>>(), 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()
|
||||
{
|
||||
|
||||
@@ -21,6 +21,12 @@ public sealed class StubCryptoProvider : ICryptoProvider
|
||||
_validPairs = validPairs;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public List<string> VerifiedSalts { get; } = new();
|
||||
|
||||
public string DefaultHashMethod => "PBKDF2-SHA512";
|
||||
|
||||
public bool Verify(PasswordHash hash, ReadOnlySpan<char> 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)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user