diff --git a/Emby.Server.Implementations/Session/SessionManager.cs b/Emby.Server.Implementations/Session/SessionManager.cs index 2eeeecfec0..60b91dd3f5 100644 --- a/Emby.Server.Implementations/Session/SessionManager.cs +++ b/Emby.Server.Implementations/Session/SessionManager.cs @@ -2043,7 +2043,7 @@ namespace Emby.Server.Implementations.Session { CheckDisposed(); - var adminUserIds = _userManager.Users + var adminUserIds = _userManager.GetUsers() .Where(i => i.HasPermission(PermissionKind.IsAdministrator)) .Select(i => i.Id) .ToList(); diff --git a/Jellyfin.Api/Controllers/StartupController.cs b/Jellyfin.Api/Controllers/StartupController.cs index 09f20558fe..f8817888e6 100644 --- a/Jellyfin.Api/Controllers/StartupController.cs +++ b/Jellyfin.Api/Controllers/StartupController.cs @@ -1,5 +1,5 @@ +using System; using System.ComponentModel.DataAnnotations; -using System.Linq; using System.Threading.Tasks; using Jellyfin.Api.Constants; using Jellyfin.Api.Models.StartupDtos; @@ -111,7 +111,7 @@ public class StartupController : BaseJellyfinApiController { // TODO: Remove this method when startup wizard no longer requires an existing user. await _userManager.InitializeAsync().ConfigureAwait(false); - var user = _userManager.Users.First(); + var user = _userManager.GetFirstUser() ?? throw new InvalidOperationException("No user exists after initialization."); return new StartupUserDto { Name = user.Username @@ -131,7 +131,12 @@ public class StartupController : BaseJellyfinApiController [ProducesResponseType(StatusCodes.Status204NoContent)] public async Task UpdateStartupUser([FromBody] StartupUserDto startupUserDto) { - var user = _userManager.Users.First(); + var user = _userManager.GetFirstUser(); + if (user is null) + { + return NotFound(); + } + if (string.IsNullOrWhiteSpace(startupUserDto.Password)) { return BadRequest("Password must not be empty"); diff --git a/Jellyfin.Api/Controllers/UserController.cs b/Jellyfin.Api/Controllers/UserController.cs index d0ced277a0..5eadb7a831 100644 --- a/Jellyfin.Api/Controllers/UserController.cs +++ b/Jellyfin.Api/Controllers/UserController.cs @@ -392,7 +392,7 @@ public class UserController : BaseJellyfinApiController if (!string.Equals(user.Username, updateUser.Name, StringComparison.Ordinal)) { - await _userManager.RenameUser(user, updateUser.Name).ConfigureAwait(false); + await _userManager.RenameUser(user.Id, user.Username, updateUser.Name).ConfigureAwait(false); } await _userManager.UpdateConfigurationAsync(requestUserId, updateUser.Configuration).ConfigureAwait(false); @@ -448,7 +448,7 @@ public class UserController : BaseJellyfinApiController // If removing admin access if (!newPolicy.IsAdministrator && user.HasPermission(PermissionKind.IsAdministrator)) { - if (_userManager.Users.Count(i => i.HasPermission(PermissionKind.IsAdministrator)) == 1) + if (_userManager.GetUsers().Count(i => i.HasPermission(PermissionKind.IsAdministrator)) == 1) { return StatusCode(StatusCodes.Status403Forbidden, "There must be at least one user in the system with administrative access."); } @@ -463,7 +463,7 @@ public class UserController : BaseJellyfinApiController // If disabling if (newPolicy.IsDisabled && !user.HasPermission(PermissionKind.IsDisabled)) { - if (_userManager.Users.Count(i => !i.HasPermission(PermissionKind.IsDisabled)) == 1) + if (_userManager.GetUsers().Count(i => !i.HasPermission(PermissionKind.IsDisabled)) == 1) { return StatusCode(StatusCodes.Status403Forbidden, "There must be at least one enabled user in the system."); } @@ -620,7 +620,7 @@ public class UserController : BaseJellyfinApiController private IEnumerable Get(bool? isHidden, bool? isDisabled, bool filterByDevice, bool filterByNetwork) { - var users = _userManager.Users; + var users = _userManager.GetUsers(); if (isDisabled.HasValue) { diff --git a/Jellyfin.Server.Implementations/Users/UserManager.cs b/Jellyfin.Server.Implementations/Users/UserManager.cs index c13fc00ebc..b0c377cd96 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 RS0030 // Do not use banned APIs using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -52,7 +52,6 @@ namespace Jellyfin.Server.Implementations.Users private readonly DefaultPasswordResetProvider _defaultPasswordResetProvider; private readonly IServerConfigurationManager _serverConfigurationManager; - private readonly IDictionary _users; private readonly AsyncKeyedLocker _userLock = new(); /// @@ -92,30 +91,28 @@ namespace Jellyfin.Server.Implementations.Users _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) - .AsNoTracking() - .AsEnumerable()) - { - _users.Add(user.Id, user); - } } /// public event EventHandler>? OnUserUpdated; /// - public IEnumerable Users => _users.Values; + public IEnumerable GetUsers() + { + using var dbContext = _dbProvider.CreateDbContext(); + return UserQuery(dbContext) + .ToArray(); + } /// - public IEnumerable UsersIds => _users.Keys; + public IEnumerable GetUsersIds() + { + using var dbContext = _dbProvider.CreateDbContext(); + return dbContext.Users + .AsNoTracking() + .Select(user => user.Id) + .ToArray(); + } // 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 @@ -131,11 +128,28 @@ namespace Jellyfin.Server.Implementations.Users throw new ArgumentException("Guid can't be empty", nameof(id)); } - using (_userLock.Lock(id)) - { - _users.TryGetValue(id, out var user); - return user; - } + using var dbContext = _dbProvider.CreateDbContext(); + return UserQuery(dbContext) + .FirstOrDefault(user => user.Id == id); + } + + private static IQueryable UserQuery(JellyfinDbContext dbContext) + { + return dbContext.Users + .AsSingleQuery() + .Include(user => user.Permissions) + .Include(user => user.Preferences) + .Include(user => user.AccessSchedules) + .Include(user => user.ProfileImage) + .AsNoTracking(); + } + + /// + public User? GetFirstUser() + { + using var dbContext = _dbProvider.CreateDbContext(); + return UserQuery(dbContext) + .FirstOrDefault(); } /// @@ -146,22 +160,29 @@ 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(); +#pragma warning disable CA1862 // Use the 'StringComparison' method overloads to perform case-insensitive string comparisons +#pragma warning disable CA1311 // Specify a culture or use an invariant version to avoid implicit dependency on current culture +#pragma warning disable CA1304 // The behavior of 'string.ToUpper()' could vary based on the current user's locale settings + return UserQuery(dbContext) + .FirstOrDefault(u => u.Username.ToUpper() == name.ToUpper()); +#pragma warning restore CA1304 // The behavior of 'string.ToUpper()' could vary based on the current user's locale settings +#pragma warning restore CA1311 // Specify a culture or use an invariant version to avoid implicit dependency on current culture +#pragma warning restore CA1862 // Use the 'StringComparison' method overloads to perform case-insensitive string comparisons } /// - public async Task RenameUser(User user, string newName) + public async Task RenameUser(Guid userId, string oldName, string newName) { - ArgumentNullException.ThrowIfNull(user); - ThrowIfInvalidUsername(newName); - if (user.Username.Equals(newName, StringComparison.Ordinal)) + if (oldName.Equals(newName, StringComparison.Ordinal)) { throw new ArgumentException("The new and old names must be different."); } - using (await _userLock.LockAsync(user.Id).ConfigureAwait(false)) + User user = null!; // user is never actually null where its used afterwards so we can just ignore. + using (await _userLock.LockAsync(userId).ConfigureAwait(false)) { var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); await using (dbContext.ConfigureAwait(false)) @@ -170,7 +191,7 @@ namespace Jellyfin.Server.Implementations.Users #pragma warning disable CA1311 // Specify a culture or use an invariant version to avoid implicit dependency on current culture #pragma warning disable CA1304 // The behavior of 'string.ToUpper()' could vary based on the current user's locale settings if (await dbContext.Users - .AnyAsync(u => u.Username.ToUpper() == newName.ToUpper() && !u.Id.Equals(user.Id)) + .AnyAsync(u => u.Username.ToUpper() == newName.ToUpper() && u.Id != userId) .ConfigureAwait(false)) { throw new ArgumentException(string.Format( @@ -182,6 +203,16 @@ namespace Jellyfin.Server.Implementations.Users #pragma warning restore CA1311 // Specify a culture or use an invariant version to avoid implicit dependency on current culture #pragma warning restore CA1862 // Use the 'StringComparison' method overloads to perform case-insensitive string comparisons + // Load a fresh tracked entity inside the lock so the RowVersion is current. + user = await dbContext.Users + .Include(u => u.Permissions) + .Include(u => u.Preferences) + .Include(u => u.AccessSchedules) + .Include(u => u.ProfileImage) + .FirstOrDefaultAsync(u => u.Id == userId) + .ConfigureAwait(false) + ?? throw new ResourceNotFoundException(nameof(userId)); + user.Username = newName; await UpdateUserInternalAsync(dbContext, user).ConfigureAwait(false); } @@ -197,11 +228,7 @@ namespace Jellyfin.Server.Implementations.Users { using (await _userLock.LockAsync(user.Id).ConfigureAwait(false)) { - var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); - await using (dbContext.ConfigureAwait(false)) - { - await UpdateUserInternalAsync(dbContext, user).ConfigureAwait(false); - } + await UpdateUserInternalAsync(user).ConfigureAwait(false); } } @@ -231,23 +258,30 @@ namespace Jellyfin.Server.Implementations.Users { ThrowIfInvalidUsername(name); - if (Users.Any(u => u.Username.Equals(name, StringComparison.OrdinalIgnoreCase))) - { - throw new ArgumentException(string.Format( - CultureInfo.InvariantCulture, - "A user with the name '{0}' already exists.", - name)); - } - User newUser; var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); await using (dbContext.ConfigureAwait(false)) { +#pragma warning disable CA1862 // Use the 'StringComparison' method overloads to perform case-insensitive string comparisons +#pragma warning disable CA1311 // Specify a culture or use an invariant version to avoid implicit dependency on current culture +#pragma warning disable CA1304 // The behavior of 'string.ToUpper()' could vary based on the current user's locale settings + if (await dbContext.Users + .AnyAsync(u => u.Username.ToUpper() == name.ToUpper()) + .ConfigureAwait(false)) + { + throw new ArgumentException(string.Format( + CultureInfo.InvariantCulture, + "A user with the name '{0}' already exists.", + name)); + } +#pragma warning restore CA1304 // The behavior of 'string.ToUpper()' could vary based on the current user's locale settings +#pragma warning restore CA1311 // Specify a culture or use an invariant version to avoid implicit dependency on current culture +#pragma warning restore CA1862 // Use the 'StringComparison' method overloads to perform case-insensitive string comparisons + newUser = await CreateUserInternalAsync(name, dbContext).ConfigureAwait(false); dbContext.Users.Add(newUser); await dbContext.SaveChangesAsync().ConfigureAwait(false); - _users.Add(newUser.Id, newUser); } await _eventManager.PublishAsync(new UserCreatedEventArgs(newUser)).ConfigureAwait(false); @@ -261,39 +295,44 @@ namespace Jellyfin.Server.Implementations.Users User? user; using (await _userLock.LockAsync(userId).ConfigureAwait(false)) { - if (!_users.TryGetValue(userId, out user)) - { - throw new ResourceNotFoundException(nameof(userId)); - } - - if (_users.Count == 1) - { - 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)); - } - - if (user.HasPermission(PermissionKind.IsAdministrator) - && Users.Count(i => i.HasPermission(PermissionKind.IsAdministrator)) == 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.Attach(user); + user = await dbContext.Users + .Include(u => u.Permissions) + .FirstOrDefaultAsync(u => u.Id.Equals(userId)) + .ConfigureAwait(false); + + if (user is null) + { + throw new ResourceNotFoundException(nameof(userId)); + } + + var userCount = await dbContext.Users.CountAsync().ConfigureAwait(false); + if (userCount == 1) + { + 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)); + } + + if (user.HasPermission(PermissionKind.IsAdministrator) + && await dbContext.Users + .CountAsync(i => i.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)); + } + dbContext.Users.Remove(user); await dbContext.SaveChangesAsync().ConfigureAwait(false); } - - _users.Remove(userId); } await _eventManager.PublishAsync(new UserDeletedEventArgs(user)).ConfigureAwait(false); @@ -316,8 +355,27 @@ namespace Jellyfin.Server.Implementations.Users using (await _userLock.LockAsync(user.Id).ConfigureAwait(false)) { - await GetAuthenticationProvider(user).ChangePassword(user, newPassword).ConfigureAwait(false); - await UpdateUserAsync(user).ConfigureAwait(false); + var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); + await using (dbContext.ConfigureAwait(false)) + { + // Reload the user inside the lock to get the current RowVersion. + // The incoming user may have been loaded with AsNoTracking() before this + // lock was acquired; a concurrent write (e.g. a login updating LastLoginDate) + // could have incremented RowVersion in the database in the meantime. + // Since we now hold the per-user lock, no other write can race with us, so + // the freshly loaded RowVersion is guaranteed to be current. + var dbUser = await dbContext.Users + .Include(u => u.Permissions) + .Include(u => u.Preferences) + .Include(u => u.AccessSchedules) + .Include(u => u.ProfileImage) + .FirstOrDefaultAsync(u => u.Id == user.Id) + .ConfigureAwait(false) + ?? throw new ResourceNotFoundException(nameof(user.Id)); + + await GetAuthenticationProvider(dbUser).ChangePassword(dbUser, newPassword).ConfigureAwait(false); + await dbContext.SaveChangesAsync().ConfigureAwait(false); + } } await _eventManager.PublishAsync(new UserPasswordChangedEventArgs(user)).ConfigureAwait(false); @@ -424,9 +482,17 @@ namespace Jellyfin.Server.Implementations.Users } bool success; - var user = Users.FirstOrDefault(i => string.Equals(username, i.Username, StringComparison.OrdinalIgnoreCase)); + var user = GetUserByName(username); using (await _userLock.LockAsync(user?.Id ?? Guid.Empty).ConfigureAwait(false)) { + // Reload the user now that we hold the lock so the RowVersion is current. + // GetUserByName uses AsNoTracking and the snapshot may be stale if another + // write (e.g. a concurrent login) incremented RowVersion after our initial load. + if (user is not null) + { + user = GetUserById(user.Id) ?? user; + } + var authResult = await AuthenticateLocalUser(username, password, user) .ConfigureAwait(false); var authenticationProvider = authResult.AuthenticationProvider; @@ -445,7 +511,7 @@ namespace Jellyfin.Server.Implementations.Users // Search the database for the user again // the authentication provider might have created it - user = Users.FirstOrDefault(i => string.Equals(username, i.Username, StringComparison.OrdinalIgnoreCase)); + user = GetUserByName(username); if (authenticationProvider is IHasNewUserPolicy hasNewUserPolicy && user is not null) { @@ -461,7 +527,7 @@ namespace Jellyfin.Server.Implementations.Users if (providerId is not null && !string.Equals(providerId, user.AuthenticationProviderId, StringComparison.OrdinalIgnoreCase)) { user.AuthenticationProviderId = providerId; - await UpdateUserAsync(user).ConfigureAwait(false); + await UpdateUserInternalAsync(user).ConfigureAwait(false); } } @@ -512,7 +578,7 @@ namespace Jellyfin.Server.Implementations.Users } user.InvalidLoginAttemptCount = 0; - await UpdateUserAsync(user).ConfigureAwait(false); + await UpdateUserInternalAsync(user).ConfigureAwait(false); _logger.LogInformation("Authentication request for {UserName} has succeeded.", user.Username); } else @@ -566,22 +632,22 @@ 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()) - { - return; - } - - var defaultName = Environment.UserName; - if (string.IsNullOrWhiteSpace(defaultName) || !ValidUsernameRegex().IsMatch(defaultName)) - { - defaultName = "MyJellyfinUser"; - } - - _logger.LogWarning("No users, creating one with username {UserName}", defaultName); - var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); await using (dbContext.ConfigureAwait(false)) { + if (await dbContext.Users.AnyAsync().ConfigureAwait(false)) + { + return; + } + + var defaultName = Environment.UserName; + if (string.IsNullOrWhiteSpace(defaultName) || !ValidUsernameRegex().IsMatch(defaultName)) + { + defaultName = "MyJellyfinUser"; + } + + _logger.LogWarning("No users, creating one with username {UserName}", defaultName); + var newUser = await CreateUserInternalAsync(defaultName, dbContext).ConfigureAwait(false); newUser.SetPermission(PermissionKind.IsAdministrator, true); newUser.SetPermission(PermissionKind.EnableContentDeletion, true); @@ -589,7 +655,6 @@ namespace Jellyfin.Server.Implementations.Users dbContext.Users.Add(newUser); await dbContext.SaveChangesAsync().ConfigureAwait(false); - _users.Add(newUser.Id, newUser); } } @@ -664,7 +729,6 @@ namespace Jellyfin.Server.Implementations.Users user.SetPreference(PreferenceKind.LatestItemExcludes, config.LatestItemsExcludes); dbContext.Update(user); - _users[user.Id] = user; await dbContext.SaveChangesAsync().ConfigureAwait(false); } } @@ -745,7 +809,6 @@ namespace Jellyfin.Server.Implementations.Users user.SetPreference(PreferenceKind.EnableContentDeletionFromFolders, policy.EnableContentDeletionFromFolders); dbContext.Update(user); - _users[user.Id] = user; await dbContext.SaveChangesAsync().ConfigureAwait(false); } } @@ -769,7 +832,6 @@ namespace Jellyfin.Server.Implementations.Users } user.ProfileImage = null; - _users[user.Id] = user; } } @@ -916,14 +978,22 @@ namespace Jellyfin.Server.Implementations.Users user.InvalidLoginAttemptCount); } - await UpdateUserAsync(user).ConfigureAwait(false); + await UpdateUserInternalAsync(user).ConfigureAwait(false); + } + + private async Task UpdateUserInternalAsync(User user) + { + var dbContext = await _dbProvider.CreateDbContextAsync().ConfigureAwait(false); + await using (dbContext.ConfigureAwait(false)) + { + await UpdateUserInternalAsync(dbContext, user).ConfigureAwait(false); + } } private async Task UpdateUserInternalAsync(JellyfinDbContext dbContext, User user) { dbContext.Users.Attach(user); dbContext.Entry(user).State = EntityState.Modified; - _users[user.Id] = user; await dbContext.SaveChangesAsync().ConfigureAwait(false); } diff --git a/MediaBrowser.Controller/Library/IUserManager.cs b/MediaBrowser.Controller/Library/IUserManager.cs index 0109cf4b7d..ad0954db89 100644 --- a/MediaBrowser.Controller/Library/IUserManager.cs +++ b/MediaBrowser.Controller/Library/IUserManager.cs @@ -24,14 +24,14 @@ namespace MediaBrowser.Controller.Library /// /// Gets the users. /// - /// The users. - IEnumerable Users { get; } + /// The users. + IEnumerable GetUsers(); /// /// Gets the user ids. /// - /// The users ids. - IEnumerable UsersIds { get; } + /// The users ids. + IEnumerable GetUsersIds(); /// /// Initializes the user manager and ensures that a user exists. @@ -47,6 +47,12 @@ namespace MediaBrowser.Controller.Library /// id is an empty Guid. User? GetUserById(Guid id); + /// + /// Gets the first available user. + /// + /// The first user, or null if no users exist. + User? GetFirstUser(); + /// /// Gets the name of the user by. /// @@ -57,12 +63,13 @@ namespace MediaBrowser.Controller.Library /// /// Renames the user. /// - /// The user. + /// The UserId to change. + /// The old Username. /// The new name. /// Task. /// If user is null. /// If the provided user doesn't exist. - Task RenameUser(User user, string newName); + Task RenameUser(Guid userId, string oldName, string newName); /// /// Updates the user. diff --git a/src/Jellyfin.LiveTv/LiveTvManager.cs b/src/Jellyfin.LiveTv/LiveTvManager.cs index 53bc6751fc..ee3dc3187b 100644 --- a/src/Jellyfin.LiveTv/LiveTvManager.cs +++ b/src/Jellyfin.LiveTv/LiveTvManager.cs @@ -1204,7 +1204,7 @@ namespace Jellyfin.LiveTv { Services = services, IsEnabled = services.Length > 0, - EnabledUsers = _userManager.Users + EnabledUsers = _userManager.GetUsers() .Where(IsLiveTvEnabled) .Select(i => i.Id.ToString("N", CultureInfo.InvariantCulture)) .ToArray() @@ -1220,7 +1220,7 @@ namespace Jellyfin.LiveTv public IEnumerable GetEnabledUsers() { - return _userManager.Users + return _userManager.GetUsers() .Where(IsLiveTvEnabled); } diff --git a/src/Jellyfin.LiveTv/Recordings/RecordingNotifier.cs b/src/Jellyfin.LiveTv/Recordings/RecordingNotifier.cs index a5d186ce18..4b0f63b041 100644 --- a/src/Jellyfin.LiveTv/Recordings/RecordingNotifier.cs +++ b/src/Jellyfin.LiveTv/Recordings/RecordingNotifier.cs @@ -79,7 +79,7 @@ namespace Jellyfin.LiveTv.Recordings private async Task SendMessage(SessionMessageType name, TimerEventInfo info) { - var users = _userManager.Users + var users = _userManager.GetUsers() .Where(i => i.HasPermission(PermissionKind.EnableLiveTvAccess)) .Select(i => i.Id) .ToList();