From fb50b4df8b5e6dec9567772aa8baecfbccb34124 Mon Sep 17 00:00:00 2001 From: Tim Eisele Date: Thu, 20 Aug 2026 18:35:26 +0200 Subject: Stop user updates from orphaning permission and preference rows (#17645) * Stop user updates from orphaning permission and preference rows * Make UserId non-nullable * Remove unnecessary ToList * Update Jellyfin.Server.Implementations/Users/UserManager.cs Co-authored-by: Claus Vium --------- Co-authored-by: Claus Vium --- .../Users/UserManagerProfileImageTests.cs | 23 --- .../Users/UserManagerUpdateUserTests.cs | 181 +++++++++++++++++++++ 2 files changed, 181 insertions(+), 23 deletions(-) create mode 100644 tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerUpdateUserTests.cs (limited to 'tests/Jellyfin.Server.Implementations.Tests') diff --git a/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerProfileImageTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerProfileImageTests.cs index 778b888735..cb714a4014 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerProfileImageTests.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerProfileImageTests.cs @@ -1,6 +1,5 @@ using System; using System.IO; -using System.Linq; using System.Threading; using System.Threading.Tasks; using Jellyfin.Database.Implementations; @@ -92,28 +91,6 @@ namespace Jellyfin.Server.Implementations.Tests.Users new NoLockBehavior(NullLogger.Instance)); } - [Fact] - public async Task UpdateUserAsync_DoesNotLeaveOrphanedPermissionsOrPreferences() - { - var user = await _userManager.CreateUserAsync("updateduser"); - var permissionCount = user.Permissions.Count; - var preferenceCount = user.Preferences.Count; - - user.LastActivityDate = DateTime.UtcNow; - await _userManager.UpdateUserAsync(user); - await _userManager.UpdateUserAsync(user); - - await using var context = CreateDbContext(); - Assert.Empty(await context.Permissions - .Where(permission => !permission.UserId.HasValue) - .ToListAsync(TestContext.Current.CancellationToken)); - Assert.Empty(await context.Preferences - .Where(preference => !preference.UserId.HasValue) - .ToListAsync(TestContext.Current.CancellationToken)); - Assert.Equal(permissionCount, await context.Permissions.CountAsync(TestContext.Current.CancellationToken)); - Assert.Equal(preferenceCount, await context.Preferences.CountAsync(TestContext.Current.CancellationToken)); - } - [Fact] public async Task ClearProfileImageAsync_WhenInMemoryImageHasTemporaryKey_RemovesPersistedImage() { diff --git a/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerUpdateUserTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerUpdateUserTests.cs new file mode 100644 index 0000000000..c940f92109 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerUpdateUserTests.cs @@ -0,0 +1,181 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Jellyfin.Data; +using Jellyfin.Database.Implementations; +using Jellyfin.Database.Implementations.Enums; +using Jellyfin.Database.Implementations.Locking; +using Jellyfin.Database.Providers.Sqlite; +using Jellyfin.Server.Implementations.Users; +using MediaBrowser.Common; +using MediaBrowser.Common.Net; +using MediaBrowser.Controller; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.Drawing; +using MediaBrowser.Controller.Events; +using MediaBrowser.Model.Cryptography; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.Users; + +public sealed class UserManagerUpdateUserTests : IDisposable +{ + private readonly SqliteConnection _connection; + private readonly DbContextOptions _dbOptions; + private readonly UserManager _userManager; + + public UserManagerUpdateUserTests() + { + _connection = new SqliteConnection("Data Source=:memory:"); + _connection.Open(); + + _dbOptions = new DbContextOptionsBuilder() + .UseSqlite(_connection) + .Options; + + // Create the schema + using var ctx = CreateDbContext(); + ctx.Database.EnsureCreated(); + + var factory = new Mock>(); + factory.Setup(f => f.CreateDbContext()).Returns(CreateDbContext); + factory.Setup(f => f.CreateDbContextAsync(It.IsAny())) + .ReturnsAsync(CreateDbContext); + + var cryptoProvider = new Mock(); + var configManager = new Mock(); + var appPaths = new Mock(); + appPaths.Setup(x => x.ProgramDataPath).Returns(Path.GetTempPath()); + configManager.Setup(x => x.ApplicationPaths).Returns(appPaths.Object); + + var appHost = new Mock(); + + var defaultAuthProvider = new DefaultAuthenticationProvider( + NullLogger.Instance, + cryptoProvider.Object); + var invalidAuthProvider = new InvalidAuthProvider(); + var defaultPasswordResetProvider = new DefaultPasswordResetProvider( + configManager.Object, + appHost.Object); + + _userManager = new UserManager( + factory.Object, + new NoopEventManager(), + new Mock().Object, + appHost.Object, + new Mock().Object, + NullLogger.Instance, + configManager.Object, + [defaultPasswordResetProvider], + [defaultAuthProvider, invalidAuthProvider]); + } + + public void Dispose() + { + _userManager.Dispose(); + _connection.Dispose(); + } + + [Fact] + public async Task UpdateUserAsync_DoesNotDetachPermissionsOrPreferences() + { + var user = await _userManager.CreateUserAsync("orphanuser"); + var permissionCount = user.Permissions.Count; + var preferenceCount = user.Preferences.Count; + + user.LastActivityDate = DateTime.UtcNow; + await _userManager.UpdateUserAsync(user); + await _userManager.UpdateUserAsync(user); + + await using var context = CreateDbContext(); + Assert.Equal(permissionCount, await context.Permissions.CountAsync(TestContext.Current.CancellationToken)); + Assert.Equal(preferenceCount, await context.Preferences.CountAsync(TestContext.Current.CancellationToken)); + Assert.All( + await context.Permissions.ToListAsync(TestContext.Current.CancellationToken), + permission => Assert.Equal(user.Id, permission.UserId)); + Assert.All( + await context.Preferences.ToListAsync(TestContext.Current.CancellationToken), + preference => Assert.Equal(user.Id, preference.UserId)); + } + + [Fact] + public async Task UpdateUserAsync_WhenOnlyTheUserRowChanged_LeavesChildRowsUntouched() + { + var user = await _userManager.CreateUserAsync("churnuser"); + var before = await ReadChildRowsAsync(); + + // A session activity stamp goes through the same path. It must not rewrite all 37 child + // rows, which is what tearing the collections down and rebuilding them used to do. + user.LastActivityDate = DateTime.UtcNow; + await _userManager.UpdateUserAsync(user); + + Assert.Equal(before, await ReadChildRowsAsync()); + } + + [Fact] + public async Task UpdateUserAsync_AppliesPermissionAndPreferenceChanges() + { + var user = await _userManager.CreateUserAsync("policyuser"); + Assert.False(user.HasPermission(PermissionKind.IsAdministrator)); + + user.SetPermission(PermissionKind.IsAdministrator, true); + user.SetPreference(PreferenceKind.BlockedTags, ["spoilers"]); + user.Permissions.Remove(user.Permissions.First(permission => permission.Kind == PermissionKind.EnableAllChannels)); + + await _userManager.UpdateUserAsync(user); + + var reloaded = _userManager.GetUserById(user.Id)!; + Assert.True(reloaded.HasPermission(PermissionKind.IsAdministrator)); + Assert.Equal(new[] { "spoilers" }, reloaded.GetPreference(PreferenceKind.BlockedTags)); + Assert.DoesNotContain(reloaded.Permissions, permission => permission.Kind == PermissionKind.EnableAllChannels); + + await using var context = CreateDbContext(); + Assert.Equal(reloaded.Permissions.Count, await context.Permissions.CountAsync(TestContext.Current.CancellationToken)); + } + + private JellyfinDbContext CreateDbContext() + { + return new JellyfinDbContext( + _dbOptions, + NullLogger.Instance, + new SqliteDatabaseProvider(null!, NullLogger.Instance), + new NoLockBehavior(NullLogger.Instance)); + } + + /// + /// Reads the identity and concurrency token of every permission and preference row. + /// + private async Task> ReadChildRowsAsync() + { + await using var context = CreateDbContext(); + var permissions = await context.Permissions + .OrderBy(permission => permission.Id) + .Select(permission => new ValueTuple("Permission", permission.Id, (int)permission.Kind, permission.RowVersion)) + .ToListAsync(TestContext.Current.CancellationToken); + var preferences = await context.Preferences + .OrderBy(preference => preference.Id) + .Select(preference => new ValueTuple("Preference", preference.Id, (int)preference.Kind, preference.RowVersion)) + .ToListAsync(TestContext.Current.CancellationToken); + + return permissions.Concat(preferences).ToList(); + } + + private sealed class NoopEventManager : IEventManager + { + public void Publish(T eventArgs) + where T : EventArgs + { + } + + public Task PublishAsync(T eventArgs) + where T : EventArgs + => Task.CompletedTask; + } +} -- cgit v1.2.3