Backport pull request #17938 from jellyfin/release-12.z

Fix SyncPlay authentication error handling and limit group member wait time

Original-merge: 5fcb65bb46b1ef528344e90e4cd0f7f974807e9e

Merged-by: crobibero <cody@robibe.ro>

Backported-by: Cody Robibero <cody@robibe.ro>

3 files changed, 260 insertions, 15 deletions

diff --git a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/SyncPlayManagerTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/SyncPlayManagerTests.cs
index b1221f6f71..ecd8fafe80 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/SyncPlayManagerTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/SyncPlayManagerTests.cs
@@ -1,12 +1,17 @@
 using System;
 using System.Threading;
+using System.Threading.Tasks;
 using Jellyfin.Database.Implementations.Entities;
+using MediaBrowser.Controller.Entities;
 using MediaBrowser.Controller.Library;
 using MediaBrowser.Controller.Session;
+using MediaBrowser.Controller.SyncPlay.PlaybackRequests;
 using MediaBrowser.Controller.SyncPlay.Requests;
+using MediaBrowser.Model.SyncPlay;
 using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
 using Xunit;
+using SyncPlayGroup = Emby.Server.Implementations.SyncPlay.Group;
 using SyncPlayManager = Emby.Server.Implementations.SyncPlay.SyncPlayManager;
 
 namespace Jellyfin.Server.Implementations.Tests.SyncPlay;
@@ -55,11 +60,33 @@ public class SyncPlayManagerTests
         Assert.False(harness.Manager.IsUserActive(harness.User.Id));
     }
 
+    [Fact]
+    public async Task HandleRequest_GroupWaitsForAMemberThatNeverReportsReady_RecoversOnItsOwn()
+    {
+        var harness = new ManagerHarness(groupWaitTimeout: 200);
+        var second = harness.CreateSession("session-2");
+
+        var info = harness.Manager.NewGroup(harness.Session, new NewGroupRequest("group"), CancellationToken.None);
+        harness.Manager.JoinGroup(second, new JoinGroupRequest(info.GroupId), CancellationToken.None);
+
+        // Starting playback puts the group behind the ready barrier.
+        harness.Manager.HandleRequest(
+            harness.Session,
+            new PlayGroupRequest(new[] { Guid.NewGuid() }, 0, 0),
+            CancellationToken.None);
+        Assert.Equal(GroupStateType.Waiting, harness.Manager.GetGroup(harness.Session, info.GroupId).State);
+
+        // Neither session ever reports ready, so the group has to come out of the wait by itself.
+        Assert.Equal(
+            GroupStateType.Playing,
+            await harness.WaitForState(harness.Session, info.GroupId, GroupStateType.Playing));
+    }
+
     private sealed class ManagerHarness
     {
         private readonly Mock<ISessionManager> _sessionManager = new();
 
-        public ManagerHarness()
+        public ManagerHarness(long? groupWaitTimeout = null)
         {
             var userManager = new Mock<IUserManager>();
             var libraryManager = new Mock<ILibraryManager>();
@@ -67,11 +94,26 @@ public class SyncPlayManagerTests
             User = new User("tester", "auth-provider", "pwdreset-provider");
             userManager.Setup(m => m.GetUserById(It.IsAny<Guid>())).Returns(User);
 
+            var item = new Mock<BaseItem>();
+            item.Setup(i => i.IsVisibleStandalone(It.IsAny<User>())).Returns(true);
+            item.Object.RunTimeTicks = TimeSpan.FromHours(2).Ticks;
+            libraryManager.Setup(m => m.GetItemById(It.IsAny<Guid>())).Returns(item.Object);
+
+            _sessionManager
+                .Setup(m => m.SendSyncPlayCommand(It.IsAny<string>(), It.IsAny<SendCommand>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.CompletedTask);
+            _sessionManager
+                .Setup(m => m.SendSyncPlayGroupUpdate(It.IsAny<string>(), It.IsAny<GroupUpdate<GroupStateUpdate>>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.CompletedTask);
+
             Manager = new SyncPlayManager(
                 NullLoggerFactory.Instance,
                 userManager.Object,
                 _sessionManager.Object,
-                libraryManager.Object);
+                libraryManager.Object)
+            {
+                GroupWaitTimeout = groupWaitTimeout ?? SyncPlayGroup.DefaultGroupWaitTimeout
+            };
 
             Session = CreateSession("session-1");
         }
@@ -91,5 +133,17 @@ public class SyncPlayManagerTests
                 UserName = User.Username
             };
         }
+
+        public async Task<GroupStateType> WaitForState(SessionInfo session, Guid groupId, GroupStateType expected)
+        {
+            var deadline = DateTime.UtcNow.AddSeconds(10);
+            GroupStateType state;
+            while ((state = Manager.GetGroup(session, groupId).State) != expected && DateTime.UtcNow < deadline)
+            {
+                await Task.Delay(20, TestContext.Current.CancellationToken);
+            }
+
+            return state;
+        }
     }
 }
diff --git a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
index 81af12ba8c..d3cbc9b8be 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
@@ -204,9 +204,131 @@ public class WaitingGroupStateTests
         Assert.InRange(group.LastActivity - before, TimeSpan.Zero, TimeSpan.FromMinutes(1));
     }
 
+    [Fact]
+    public async Task SessionJoined_JoinerNeverReportsReady_GroupResumesWithoutIt()
+    {
+        var harness = new GroupHarness(groupWaitTimeout: 200);
+        var group = harness.Group;
+
+        group.PositionTicks = TimeSpan.FromMinutes(5).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+        group.SetState(new PlayingGroupState(NullLoggerFactory.Instance));
+
+        // A session joins while the group is playing: the group pauses and waits for it.
+        var joiner = harness.NewSession("joiner");
+        group.SessionJoin(joiner, new JoinGroupRequest(group.GroupId), CancellationToken.None);
+
+        Assert.Equal(GroupStateType.Waiting, group.GetInfo().State);
+
+        // The joiner's player aborts and never reports ready. Without a bounded wait the whole
+        // group stays paused forever.
+        await harness.WaitForState(GroupStateType.Playing);
+
+        // Late buffer reports from the session that missed the deadline must not drag the group
+        // back into waiting.
+        group.HandleRequest(
+            joiner,
+            new BufferGroupRequest(DateTime.UtcNow, 0, false, harness.PlaylistItemId),
+            CancellationToken.None);
+
+        Assert.Equal(GroupStateType.Playing, group.GetInfo().State);
+    }
+
+    [Fact]
+    public async Task SessionJoined_GroupWasPaused_TimeoutLeavesTheGroupPaused()
+    {
+        var harness = new GroupHarness(groupWaitTimeout: 200);
+        var group = harness.Group;
+
+        group.PositionTicks = TimeSpan.FromMinutes(5).Ticks;
+
+        // The group has been sitting paused for a while before anyone joins.
+        group.LastActivity = DateTime.UtcNow.AddMinutes(-2);
+        group.SetState(new PausedGroupState(NullLoggerFactory.Instance));
+
+        var joiner = harness.NewSession("joiner");
+        group.SessionJoin(joiner, new JoinGroupRequest(group.GroupId), CancellationToken.None);
+
+        Assert.Equal(GroupStateType.Waiting, group.GetInfo().State);
+
+        // A group that was paused must not start playing because a member failed to report ready.
+        await harness.WaitForState(GroupStateType.Paused);
+
+        // Giving up on the joiner must not move the playback position of an already paused group.
+        Assert.Equal(TimeSpan.FromMinutes(5).Ticks, group.PositionTicks);
+
+        // Every member has to be told the group is no longer waiting.
+        var recipients = harness.StateUpdates
+            .Where(update => update.Update.State == GroupStateType.Paused)
+            .Select(update => update.SessionId)
+            .ToList();
+        Assert.Contains(harness.First.Id, recipients);
+        Assert.Contains(harness.Second.Id, recipients);
+        Assert.Contains(joiner.Id, recipients);
+    }
+
+    [Fact]
+    public async Task Ready_ReportedBeforeTheDeadline_GroupDoesNotGiveUpOnAnyone()
+    {
+        var harness = new GroupHarness(groupWaitTimeout: 200);
+        var group = harness.Group;
+
+        group.PositionTicks = TimeSpan.FromMinutes(5).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+        group.SetState(new PlayingGroupState(NullLoggerFactory.Instance));
+
+        var joiner = harness.NewSession("joiner");
+        group.SessionJoin(joiner, new JoinGroupRequest(group.GroupId), CancellationToken.None);
+        Assert.Equal(GroupStateType.Waiting, group.GetInfo().State);
+
+        group.HandleRequest(
+            joiner,
+            new ReadyGroupRequest(DateTime.UtcNow, group.PositionTicks, true, harness.PlaylistItemId),
+            CancellationToken.None);
+
+        // Everyone reported ready, so no deadline is left to trip and force a spurious unpause.
+        Assert.Equal(GroupStateType.Playing, group.GetInfo().State);
+        Assert.Null(group.GroupWaitDeadline);
+
+        var until = DateTime.UtcNow.AddMilliseconds(3 * 200);
+        while (DateTime.UtcNow < until)
+        {
+            harness.PumpGroupWaitTimeout();
+            await Task.Delay(20, TestContext.Current.CancellationToken);
+        }
+
+        Assert.Equal(GroupStateType.Playing, group.GetInfo().State);
+    }
+
+    [Fact]
+    public async Task SetPlaylistItem_AfterATimeout_GroupWaitsForEveryoneAgain()
+    {
+        var harness = new GroupHarness(groupWaitTimeout: 200);
+        var group = harness.Group;
+
+        group.LastActivity = DateTime.UtcNow;
+        group.SetState(new PlayingGroupState(NullLoggerFactory.Instance));
+
+        var joiner = harness.NewSession("joiner");
+        group.SessionJoin(joiner, new JoinGroupRequest(group.GroupId), CancellationToken.None);
+        await harness.WaitForState(GroupStateType.Playing);
+
+        // Giving up on a session lasts only until the group changes what it is playing.
+        group.HandleRequest(
+            harness.First,
+            new SetPlaylistItemGroupRequest(harness.PlaylistItemId),
+            CancellationToken.None);
+
+        Assert.Equal(GroupStateType.Waiting, group.GetInfo().State);
+        Assert.NotNull(group.GroupWaitDeadline);
+    }
+
     private sealed class GroupHarness
     {
-        public GroupHarness()
+        private readonly ISessionManager _sessionManager;
+        private readonly Guid _userId;
+
+        public GroupHarness(long? groupWaitTimeout = null)
         {
             var userManager = new Mock<IUserManager>();
             var sessionManager = new Mock<ISessionManager>();
@@ -227,27 +349,24 @@ public class WaitingGroupStateTests
 
             sessionManager
                 .Setup(m => m.SendSyncPlayGroupUpdate(It.IsAny<string>(), It.IsAny<GroupUpdate<GroupStateUpdate>>(), It.IsAny<CancellationToken>()))
+                .Callback((string sessionId, GroupUpdate<GroupStateUpdate> update, CancellationToken _) => StateUpdates.Add((sessionId, update.Data)))
                 .Returns(Task.CompletedTask);
 
             Group = new SyncPlayGroup(
                 NullLoggerFactory.Instance,
                 userManager.Object,
                 sessionManager.Object,
-                libraryManager.Object);
-
-            First = new SessionInfo(sessionManager.Object, NullLogger.Instance)
+                libraryManager.Object)
             {
-                Id = "first",
-                UserId = user.Id,
-                UserName = "first"
-            };
-            Second = new SessionInfo(sessionManager.Object, NullLogger.Instance)
-            {
-                Id = "second",
-                UserId = user.Id,
-                UserName = "second"
+                GroupWaitTimeout = groupWaitTimeout ?? SyncPlayGroup.DefaultGroupWaitTimeout
             };
 
+            _sessionManager = sessionManager.Object;
+            _userId = user.Id;
+
+            First = NewSession("first");
+            Second = NewSession("second");
+
             Group.CreateGroup(First, new NewGroupRequest("group"), CancellationToken.None);
             Group.SessionJoin(Second, new JoinGroupRequest(Group.GroupId), CancellationToken.None);
             Group.SetPlayQueue(new List<Guid> { Guid.NewGuid() }, 0, 0);
@@ -256,6 +375,8 @@ public class WaitingGroupStateTests
 
         public SyncPlayGroup Group { get; }
 
+        public List<(string SessionId, GroupStateUpdate Update)> StateUpdates { get; } = new();
+
         public SessionInfo First { get; }
 
         public SessionInfo Second { get; }
@@ -263,5 +384,39 @@ public class WaitingGroupStateTests
         public Guid PlaylistItemId { get; }
 
         public List<SendCommand> Commands { get; } = new List<SendCommand>();
+
+        // Mirrors the sweep SyncPlayManager runs on a timer.
+        public void PumpGroupWaitTimeout()
+        {
+            var group = Group;
+
+            // Group lock required as Group is not thread-safe.
+            lock (group)
+            {
+                group.HandleGroupWaitTimeout(CancellationToken.None);
+            }
+        }
+
+        public async Task WaitForState(GroupStateType expected)
+        {
+            var deadline = DateTime.UtcNow.AddSeconds(10);
+            while (Group.GetInfo().State != expected && DateTime.UtcNow < deadline)
+            {
+                PumpGroupWaitTimeout();
+                await Task.Delay(20, TestContext.Current.CancellationToken);
+            }
+
+            Assert.Equal(expected, Group.GetInfo().State);
+        }
+
+        public SessionInfo NewSession(string id)
+        {
+            return new SessionInfo(_sessionManager, NullLogger.Instance)
+            {
+                Id = id,
+                UserId = _userId,
+                UserName = id
+            };
+        }
     }
 }
diff --git a/tests/Jellyfin.Server.Integration.Tests/Controllers/SyncPlayControllerTests.cs b/tests/Jellyfin.Server.Integration.Tests/Controllers/SyncPlayControllerTests.cs
new file mode 100644
index 0000000000..f84e28de76
--- /dev/null
+++ b/tests/Jellyfin.Server.Integration.Tests/Controllers/SyncPlayControllerTests.cs
@@ -0,0 +1,36 @@
+using System.Net;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace Jellyfin.Server.Integration.Tests.Controllers;
+
+public sealed class SyncPlayControllerTests : IClassFixture<JellyfinApplicationFactory>
+{
+    private readonly JellyfinApplicationFactory _factory;
+
+    public SyncPlayControllerTests(JellyfinApplicationFactory factory)
+    {
+        _factory = factory;
+    }
+
+    [Fact]
+    public async Task GetGroups_Unauthorized_ReturnsUnauthorized()
+    {
+        var client = _factory.CreateClient();
+
+        var response = await client.GetAsync("/SyncPlay/List", TestContext.Current.CancellationToken);
+
+        Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task GetGroups_InvalidToken_ReturnsUnauthorized()
+    {
+        var client = _factory.CreateClient();
+        client.DefaultRequestHeaders.AddAuthHeader("invalid-token");
+
+        var response = await client.GetAsync("/SyncPlay/List", TestContext.Current.CancellationToken);
+
+        Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
+    }
+}