Merge pull request #18026 from fmarcac/fix/device-access-hosted-service-12z

Fix device access revocation not logging out existing sessions

Jellyfin.Server.Implementations/Users/DeviceAccessHost.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Data;
@@ -9,6 +10,7 @@ using MediaBrowser.Controller.Devices;
 using MediaBrowser.Controller.Library;
 using MediaBrowser.Controller.Session;
 using Microsoft.Extensions.Hosting;
+using Microsoft.Extensions.Logging;
 
 namespace Jellyfin.Server.Implementations.Users;
 
@@ -20,6 +22,7 @@ public sealed class DeviceAccessHost : IHostedService
     private readonly IUserManager _userManager;
     private readonly IDeviceManager _deviceManager;
     private readonly ISessionManager _sessionManager;
+    private readonly ILogger<DeviceAccessHost> _logger;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="DeviceAccessHost"/> class.
@@ -27,11 +30,17 @@ public sealed class DeviceAccessHost : IHostedService
     /// <param name="userManager">The <see cref="IUserManager"/>.</param>
     /// <param name="deviceManager">The <see cref="IDeviceManager"/>.</param>
     /// <param name="sessionManager">The <see cref="ISessionManager"/>.</param>
-    public DeviceAccessHost(IUserManager userManager, IDeviceManager deviceManager, ISessionManager sessionManager)
+    /// <param name="logger">The <see cref="ILogger{TCategoryName}"/>.</param>
+    public DeviceAccessHost(
+        IUserManager userManager,
+        IDeviceManager deviceManager,
+        ISessionManager sessionManager,
+        ILogger<DeviceAccessHost> logger)
     {
         _userManager = userManager;
         _deviceManager = deviceManager;
         _sessionManager = sessionManager;
+        _logger = logger;
     }
 
     /// <inheritdoc />
@@ -53,9 +62,18 @@ public sealed class DeviceAccessHost : IHostedService
     private async void OnUserUpdated(object? sender, GenericEventArgs<User> e)
     {
         var user = e.Argument;
-        if (!user.HasPermission(PermissionKind.EnableAllDevices))
+
+        // This handler is async void, so an escaping exception would terminate the process.
+        try
         {
-            await UpdateDeviceAccess(user).ConfigureAwait(false);
+            if (!user.HasPermission(PermissionKind.EnableAllDevices))
+            {
+                await UpdateDeviceAccess(user).ConfigureAwait(false);
+            }
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Error updating device access for user {UserId}", user.Id);
         }
     }
 

Jellyfin.Server.Implementations/Users/UserManager.cs

@@ -847,14 +847,16 @@ namespace Jellyfin.Server.Implementations.Users
         /// <inheritdoc/>
         public async Task UpdatePolicyAsync(Guid userId, UserPolicy policy)
         {
+            User user;
             using (await _userLock.LockAsync(userId).ConfigureAwait(false))
             {
                 var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false);
                 await using (dbContext.ConfigureAwait(false))
                 {
-                    var user = UserQuery(dbContext)
+                    user = await UserQuery(dbContext)
                         .AsTracking()
-                        .FirstOrDefault(u => u.Id.Equals(userId))
+                        .FirstOrDefaultAsync(u => u.Id.Equals(userId))
+                        .ConfigureAwait(false)
                         ?? throw new ArgumentException("No user exists with given Id!");
 
                     // The default number of login attempts is 3, but for some god forsaken reason it's sent to the server as "0"
@@ -919,6 +921,10 @@ namespace Jellyfin.Server.Implementations.Users
                     await dbContext.SaveChangesAsync().ConfigureAwait(false);
                 }
             }
+
+            var eventArgs = new UserUpdatedEventArgs(user);
+            await _eventManager.PublishAsync(eventArgs).ConfigureAwait(false);
+            OnUserUpdated?.Invoke(this, eventArgs);
         }
 
         /// <inheritdoc/>

Jellyfin.Server/Startup.cs

@@ -18,6 +18,7 @@ using Jellyfin.Networking.HappyEyeballs;
 using Jellyfin.Server.Extensions;
 using Jellyfin.Server.HealthChecks;
 using Jellyfin.Server.Implementations.Extensions;
+using Jellyfin.Server.Implementations.Users;
 using MediaBrowser.Common.Net;
 using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Extensions;
@@ -155,6 +156,7 @@ namespace Jellyfin.Server
             services.AddHostedService<LibraryChangedNotifier>();
             services.AddHostedService<UserDataChangeNotifier>();
             services.AddHostedService<RecordingNotifier>();
+            services.AddHostedService<DeviceAccessHost>();
         }
 
         /// <summary>

tests/Jellyfin.Server.Implementations.Tests/Users/DeviceAccessHostTests.cs

@@ -0,0 +1,111 @@
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Jellyfin.Data.Events;
+using Jellyfin.Data.Queries;
+using Jellyfin.Database.Implementations.Entities;
+using Jellyfin.Database.Implementations.Entities.Security;
+using Jellyfin.Server.Implementations.Users;
+using MediaBrowser.Controller.Devices;
+using MediaBrowser.Controller.Library;
+using MediaBrowser.Controller.Session;
+using MediaBrowser.Model.Querying;
+using Microsoft.Extensions.Logging.Abstractions;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Users;
+
+public class DeviceAccessHostTests
+{
+    [Fact]
+    public async Task OnUserUpdated_LogoutThrows_DoesNotEscapeToThreadPool()
+    {
+        var user = new User("test", "default", "default");
+        var device = new Device(user.Id, "app", "1.0", "device", "device-id");
+
+        var deviceManager = new Mock<IDeviceManager>();
+        deviceManager.Setup(d => d.GetDevices(It.IsAny<DeviceQuery>()))
+            .Returns(new QueryResult<Device>(new[] { device }));
+        deviceManager.Setup(d => d.CanAccessDevice(user, device.DeviceId)).Returns(false);
+
+        var sessionManager = new Mock<ISessionManager>();
+        sessionManager.Setup(s => s.Logout(It.IsAny<Device>()))
+            .ThrowsAsync(new ObjectDisposedException(nameof(ISessionManager)));
+
+        var userManager = new Mock<IUserManager>();
+        var host = new DeviceAccessHost(
+            userManager.Object,
+            deviceManager.Object,
+            sessionManager.Object,
+            NullLogger<DeviceAccessHost>.Instance);
+        await host.StartAsync(TestContext.Current.CancellationToken);
+
+        var context = new CapturingSynchronizationContext();
+        var previous = SynchronizationContext.Current;
+        SynchronizationContext.SetSynchronizationContext(context);
+        try
+        {
+            userManager.Raise(m => m.OnUserUpdated += null, userManager.Object, new GenericEventArgs<User>(user));
+        }
+        finally
+        {
+            SynchronizationContext.SetSynchronizationContext(previous);
+        }
+
+        Assert.Empty(context.Exceptions);
+    }
+
+    [Fact]
+    public async Task OnUserUpdated_DeviceNoLongerAllowed_LogsOutDevice()
+    {
+        var user = new User("test", "default", "default");
+        var device = new Device(user.Id, "app", "1.0", "device", "device-id");
+
+        var deviceManager = new Mock<IDeviceManager>();
+        deviceManager.Setup(d => d.GetDevices(It.IsAny<DeviceQuery>()))
+            .Returns(new QueryResult<Device>(new[] { device }));
+        deviceManager.Setup(d => d.CanAccessDevice(user, device.DeviceId)).Returns(false);
+
+        var loggedOut = new TaskCompletionSource();
+        var sessionManager = new Mock<ISessionManager>();
+        sessionManager.Setup(s => s.Logout(It.IsAny<Device>()))
+            .Callback(() => loggedOut.TrySetResult())
+            .Returns(Task.CompletedTask);
+
+        var userManager = new Mock<IUserManager>();
+        var host = new DeviceAccessHost(
+            userManager.Object,
+            deviceManager.Object,
+            sessionManager.Object,
+            NullLogger<DeviceAccessHost>.Instance);
+        await host.StartAsync(TestContext.Current.CancellationToken);
+
+        userManager.Raise(m => m.OnUserUpdated += null, userManager.Object, new GenericEventArgs<User>(user));
+
+        await loggedOut.Task.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken);
+        sessionManager.Verify(s => s.Logout(device), Times.Once);
+    }
+
+    private sealed class CapturingSynchronizationContext : SynchronizationContext
+    {
+        public List<Exception> Exceptions { get; } = new List<Exception>();
+
+        public override void Post(SendOrPostCallback d, object? state) => Run(d, state);
+
+        public override void Send(SendOrPostCallback d, object? state) => Run(d, state);
+
+        private void Run(SendOrPostCallback d, object? state)
+        {
+            try
+            {
+                d(state);
+            }
+            catch (Exception ex)
+            {
+                Exceptions.Add(ex);
+            }
+        }
+    }
+}

tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerUpdateUserTests.cs

@@ -6,6 +6,7 @@ using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Data;
 using Jellyfin.Database.Implementations;
+using Jellyfin.Database.Implementations.Entities;
 using Jellyfin.Database.Implementations.Enums;
 using Jellyfin.Database.Implementations.Locking;
 using Jellyfin.Database.Providers.Sqlite;
@@ -17,6 +18,7 @@ using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Drawing;
 using MediaBrowser.Controller.Events;
 using MediaBrowser.Model.Cryptography;
+using MediaBrowser.Model.Users;
 using Microsoft.Data.Sqlite;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging.Abstractions;
@@ -119,6 +121,27 @@ public sealed class UserManagerUpdateUserTests : IDisposable
         Assert.Equal(before, await ReadChildRowsAsync());
     }
 
+    [Fact]
+    public async Task UpdatePolicyAsync_RaisesOnUserUpdated()
+    {
+        var user = await _userManager.CreateUserAsync("policyeventuser");
+
+        User? updated = null;
+        _userManager.OnUserUpdated += (_, e) => updated = e.Argument;
+
+        await _userManager.UpdatePolicyAsync(
+            user.Id,
+            new UserPolicy
+            {
+                EnableAllDevices = false,
+                AuthenticationProviderId = user.AuthenticationProviderId,
+                PasswordResetProviderId = user.PasswordResetProviderId
+            });
+
+        Assert.NotNull(updated);
+        Assert.Equal(user.Id, updated.Id);
+    }
+
     [Fact]
     public async Task UpdateUserAsync_AppliesPermissionAndPreferenceChanges()
     {

tests/Jellyfin.Server.Integration.Tests/HostedServiceRegistrationTests.cs

@@ -0,0 +1,26 @@
+using Jellyfin.Server.Implementations.Users;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
+using Xunit;
+
+namespace Jellyfin.Server.Integration.Tests;
+
+public sealed class HostedServiceRegistrationTests : IClassFixture<JellyfinApplicationFactory>
+{
+    private readonly JellyfinApplicationFactory _factory;
+
+    public HostedServiceRegistrationTests(JellyfinApplicationFactory factory)
+    {
+        _factory = factory;
+    }
+
+    [Fact]
+    public void DeviceAccessHost_IsRegisteredAsHostedService()
+    {
+        _ = _factory.CreateClient();
+
+        var hostedServices = _factory.Services.GetServices<IHostedService>();
+
+        Assert.Contains(hostedServices, service => service is DeviceAccessHost);
+    }
+}