From 04dddd3a7b3911ef9a3e83c85aa5419e8db2fa38 Mon Sep 17 00:00:00 2001 From: Patrick Barron Date: Sat, 6 Jan 2024 15:43:48 -0500 Subject: [PATCH] Remove UserManager Cache (#10781) * Remove redundant user cache * Use DI for IPasswordResetProvider and IAuthenticationProvider --- .../Users/UserManager.cs | 169 +++++++++--------- Jellyfin.Server/CoreAppHost.cs | 4 + 2 files changed, 92 insertions(+), 81 deletions(-) diff --git a/Jellyfin.Server.Implementations/Users/UserManager.cs b/Jellyfin.Server.Implementations/Users/UserManager.cs index 990b9a5bd4..cc464a7a3d 100644 --- a/Jellyfin.Server.Implementations/Users/UserManager.cs +++ b/Jellyfin.Server.Implementations/Users/UserManager.cs @@ -1,7 +1,7 @@ #pragma warning disable CA1307 +#pragma warning disable CA1309 // Use ordinal string comparison - EF can't translate this using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -46,8 +46,6 @@ namespace Jellyfin.Server.Implementations.Users private readonly DefaultPasswordResetProvider _defaultPasswordResetProvider; private readonly IServerConfigurationManager _serverConfigurationManager; - private readonly IDictionary _users; - /// /// Initializes a new instance of the class. /// @@ -58,6 +56,8 @@ namespace Jellyfin.Server.Implementations.Users /// The image processor. /// The logger. /// The system config manager. + /// The password reset providers. + /// The authentication providers. public UserManager( IDbContextFactory dbProvider, IEventManager eventManager, @@ -65,7 +65,9 @@ namespace Jellyfin.Server.Implementations.Users IApplicationHost appHost, IImageProcessor imageProcessor, ILogger logger, - IServerConfigurationManager serverConfigurationManager) + IServerConfigurationManager serverConfigurationManager, + IEnumerable passwordResetProviders, + IEnumerable authenticationProviders) { _dbProvider = dbProvider; _eventManager = eventManager; @@ -75,35 +77,36 @@ namespace Jellyfin.Server.Implementations.Users _logger = logger; _serverConfigurationManager = serverConfigurationManager; - _passwordResetProviders = appHost.GetExports(); - _authenticationProviders = appHost.GetExports(); + _passwordResetProviders = passwordResetProviders.ToList(); + _authenticationProviders = authenticationProviders.ToList(); _invalidAuthProvider = _authenticationProviders.OfType().First(); _defaultAuthenticationProvider = _authenticationProviders.OfType().First(); _defaultPasswordResetProvider = _passwordResetProviders.OfType().First(); - - _users = new ConcurrentDictionary(); - using var dbContext = _dbProvider.CreateDbContext(); - foreach (var user in dbContext.Users - .AsSplitQuery() - .Include(user => user.Permissions) - .Include(user => user.Preferences) - .Include(user => user.AccessSchedules) - .Include(user => user.ProfileImage) - .AsEnumerable()) - { - _users.Add(user.Id, user); - } } /// public event EventHandler>? OnUserUpdated; /// - public IEnumerable Users => _users.Values; + public IEnumerable Users + { + get + { + using var dbContext = _dbProvider.CreateDbContext(); + return GetUsersInternal(dbContext).ToList(); + } + } /// - public IEnumerable UsersIds => _users.Keys; + public IEnumerable UsersIds + { + get + { + using var dbContext = _dbProvider.CreateDbContext(); + return dbContext.Users.Select(u => u.Id).ToList(); + } + } // This is some regex that matches only on unicode "word" characters, as well as -, _ and @ // In theory this will cut out most if not all 'control' characters which should help minimize any weirdness @@ -119,8 +122,8 @@ namespace Jellyfin.Server.Implementations.Users throw new ArgumentException("Guid can't be empty", nameof(id)); } - _users.TryGetValue(id, out var user); - return user; + using var dbContext = _dbProvider.CreateDbContext(); + return GetUsersInternal(dbContext).FirstOrDefault(u => u.Id.Equals(id)); } /// @@ -131,7 +134,9 @@ namespace Jellyfin.Server.Implementations.Users throw new ArgumentException("Invalid username", nameof(name)); } - return _users.Values.FirstOrDefault(u => string.Equals(u.Username, name, StringComparison.OrdinalIgnoreCase)); + using var dbContext = _dbProvider.CreateDbContext(); + return GetUsersInternal(dbContext) + .FirstOrDefault(u => string.Equals(u.Username, name)); } /// @@ -196,8 +201,6 @@ namespace Jellyfin.Server.Implementations.Users user.AddDefaultPermissions(); user.AddDefaultPreferences(); - _users.Add(user.Id, user); - return user; } @@ -232,40 +235,46 @@ namespace Jellyfin.Server.Implementations.Users /// public async Task DeleteUserAsync(Guid userId) { - if (!_users.TryGetValue(userId, out var user)) - { - throw new ResourceNotFoundException(nameof(userId)); - } + var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); - if (_users.Count == 1) + await using (dbContext.ConfigureAwait(false)) { - throw new InvalidOperationException(string.Format( - CultureInfo.InvariantCulture, - "The user '{0}' cannot be deleted because there must be at least one user in the system.", - user.Username)); - } + var user = await dbContext.Users + .AsSingleQuery() + .Include(u => u.Permissions) + .FirstOrDefaultAsync(u => u.Id.Equals(userId)) + .ConfigureAwait(false); + if (user is null) + { + throw new ResourceNotFoundException(nameof(userId)); + } - if (user.HasPermission(PermissionKind.IsAdministrator) - && Users.Count(i => i.HasPermission(PermissionKind.IsAdministrator)) == 1) - { - throw new ArgumentException( - string.Format( + if (await dbContext.Users.CountAsync().ConfigureAwait(false) == 1) + { + throw new InvalidOperationException(string.Format( CultureInfo.InvariantCulture, - "The user '{0}' cannot be deleted because there must be at least one admin user in the system.", - user.Username), - nameof(userId)); - } + "The user '{0}' cannot be deleted because there must be at least one user in the system.", + user.Username)); + } + + if (user.HasPermission(PermissionKind.IsAdministrator) + && await dbContext.Users + .CountAsync(u => u.Permissions.Any(p => p.Kind == PermissionKind.IsAdministrator && p.Value)) + .ConfigureAwait(false) == 1) + { + throw new ArgumentException( + string.Format( + CultureInfo.InvariantCulture, + "The user '{0}' cannot be deleted because there must be at least one admin user in the system.", + user.Username), + nameof(userId)); + } - var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); - await using (dbContext.ConfigureAwait(false)) - { dbContext.Users.Remove(user); await dbContext.SaveChangesAsync().ConfigureAwait(false); - } - _users.Remove(userId); - - await _eventManager.PublishAsync(new UserDeletedEventArgs(user)).ConfigureAwait(false); + await _eventManager.PublishAsync(new UserDeletedEventArgs(user)).ConfigureAwait(false); + } } /// @@ -532,23 +541,23 @@ namespace Jellyfin.Server.Implementations.Users /// public async Task InitializeAsync() { - // TODO: Refactor the startup wizard so that it doesn't require a user to already exist. - if (_users.Any()) + var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); + await using (dbContext.ConfigureAwait(false)) { - return; - } + // TODO: Refactor the startup wizard so that it doesn't require a user to already exist. + if (await dbContext.Users.AnyAsync().ConfigureAwait(false)) + { + return; + } - var defaultName = Environment.UserName; - if (string.IsNullOrWhiteSpace(defaultName) || !ValidUsernameRegex().IsMatch(defaultName)) - { - defaultName = "MyJellyfinUser"; - } + var defaultName = Environment.UserName; + if (string.IsNullOrWhiteSpace(defaultName) || !ValidUsernameRegex().IsMatch(defaultName)) + { + defaultName = "MyJellyfinUser"; + } - _logger.LogWarning("No users, creating one with username {UserName}", defaultName); + _logger.LogWarning("No users, creating one with username {UserName}", defaultName); - var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); - await using (dbContext.ConfigureAwait(false)) - { var newUser = await CreateUserInternalAsync(defaultName, dbContext).ConfigureAwait(false); newUser.SetPermission(PermissionKind.IsAdministrator, true); newUser.SetPermission(PermissionKind.EnableContentDeletion, true); @@ -595,12 +604,9 @@ namespace Jellyfin.Server.Implementations.Users var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); await using (dbContext.ConfigureAwait(false)) { - var user = dbContext.Users - .Include(u => u.Permissions) - .Include(u => u.Preferences) - .Include(u => u.AccessSchedules) - .Include(u => u.ProfileImage) - .FirstOrDefault(u => u.Id.Equals(userId)) + var user = await GetUsersInternal(dbContext) + .FirstOrDefaultAsync(u => u.Id.Equals(userId)) + .ConfigureAwait(false) ?? throw new ArgumentException("No user exists with given Id!"); user.SubtitleMode = config.SubtitleMode; @@ -628,7 +634,6 @@ namespace Jellyfin.Server.Implementations.Users user.SetPreference(PreferenceKind.LatestItemExcludes, config.LatestItemsExcludes); dbContext.Update(user); - _users[user.Id] = user; await dbContext.SaveChangesAsync().ConfigureAwait(false); } } @@ -639,12 +644,9 @@ namespace Jellyfin.Server.Implementations.Users var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); await using (dbContext.ConfigureAwait(false)) { - var user = dbContext.Users - .Include(u => u.Permissions) - .Include(u => u.Preferences) - .Include(u => u.AccessSchedules) - .Include(u => u.ProfileImage) - .FirstOrDefault(u => u.Id.Equals(userId)) + var user = await GetUsersInternal(dbContext) + .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" @@ -704,7 +706,6 @@ namespace Jellyfin.Server.Implementations.Users user.SetPreference(PreferenceKind.EnableContentDeletionFromFolders, policy.EnableContentDeletionFromFolders); dbContext.Update(user); - _users[user.Id] = user; await dbContext.SaveChangesAsync().ConfigureAwait(false); } } @@ -725,7 +726,6 @@ namespace Jellyfin.Server.Implementations.Users } user.ProfileImage = null; - _users[user.Id] = user; } internal static void ThrowIfInvalidUsername(string name) @@ -872,8 +872,15 @@ namespace Jellyfin.Server.Implementations.Users private async Task UpdateUserInternalAsync(JellyfinDbContext dbContext, User user) { dbContext.Users.Update(user); - _users[user.Id] = user; await dbContext.SaveChangesAsync().ConfigureAwait(false); } + + private IQueryable GetUsersInternal(JellyfinDbContext dbContext) + => dbContext.Users + .AsSplitQuery() + .Include(user => user.Permissions) + .Include(user => user.Preferences) + .Include(user => user.AccessSchedules) + .Include(user => user.ProfileImage); } } diff --git a/Jellyfin.Server/CoreAppHost.cs b/Jellyfin.Server/CoreAppHost.cs index c12c90a689..93277a981e 100644 --- a/Jellyfin.Server/CoreAppHost.cs +++ b/Jellyfin.Server/CoreAppHost.cs @@ -14,6 +14,7 @@ using Jellyfin.Server.Implementations.Security; using Jellyfin.Server.Implementations.Trickplay; using Jellyfin.Server.Implementations.Users; using MediaBrowser.Controller; +using MediaBrowser.Controller.Authentication; using MediaBrowser.Controller.BaseItemManager; using MediaBrowser.Controller.Devices; using MediaBrowser.Controller.Drawing; @@ -78,6 +79,9 @@ namespace Jellyfin.Server serviceCollection.AddSingleton(); serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); serviceCollection.AddScoped(); serviceCollection.AddSingleton(); serviceCollection.AddSingleton();