Merge pull request #1926 from Bond-009/auth

Add clearer exceptions, warnings and docs

10 files changed, 63 insertions, 57 deletions

diff --git a/Emby.Server.Implementations/Cryptography/CryptographyProvider.cs b/Emby.Server.Implementations/Cryptography/CryptographyProvider.cs
index 23b77e268..fec7d161e 100644
--- a/Emby.Server.Implementations/Cryptography/CryptographyProvider.cs
+++ b/Emby.Server.Implementations/Cryptography/CryptographyProvider.cs
@@ -30,6 +30,9 @@ namespace Emby.Server.Implementations.Cryptography
 
         private bool _disposed = false;
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="CryptographyProvider"/> class.
+        /// </summary>
         public CryptographyProvider()
         {
             // FIXME: When we get DotNet Standard 2.1 we need to revisit how we do the crypto
@@ -59,12 +62,6 @@ namespace Emby.Server.Implementations.Cryptography
             throw new CryptographicException($"Cannot currently use PBKDF2 with requested hash method: {method}");
         }
 
-        public byte[] ComputeHash(string hashMethod, byte[] bytes)
-            => ComputeHash(hashMethod, bytes, Array.Empty<byte>());
-
-        public byte[] ComputeHashWithDefaultMethod(byte[] bytes)
-            => ComputeHash(DefaultHashMethod, bytes);
-
         public byte[] ComputeHash(string hashMethod, byte[] bytes, byte[] salt)
         {
             if (hashMethod == DefaultHashMethod)
@@ -90,7 +87,6 @@ namespace Emby.Server.Implementations.Cryptography
             }
 
             throw new CryptographicException($"Requested hash method is not supported: {hashMethod}");
-
         }
 
         public byte[] ComputeHashWithDefaultMethod(byte[] bytes, byte[] salt)
diff --git a/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs b/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs
index 85110c21c..c043568d5 100644
--- a/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs
+++ b/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs
@@ -59,7 +59,10 @@ namespace Emby.Server.Implementations.Library
             if (_cryptographyProvider.GetSupportedHashMethods().Contains(readyHash.Id)
                 || _cryptographyProvider.DefaultHashMethod == readyHash.Id)
             {
-                byte[] calculatedHash = _cryptographyProvider.ComputeHash(readyHash.Id, passwordbytes, readyHash.Salt);
+                byte[] calculatedHash = _cryptographyProvider.ComputeHash(
+                    readyHash.Id,
+                    passwordbytes,
+                    readyHash.Salt);
 
                 if (calculatedHash.SequenceEqual(readyHash.Hash))
                 {
diff --git a/Emby.Server.Implementations/Library/UserManager.cs b/Emby.Server.Implementations/Library/UserManager.cs
index b4e082b06..2b22129f3 100644
--- a/Emby.Server.Implementations/Library/UserManager.cs
+++ b/Emby.Server.Implementations/Library/UserManager.cs
@@ -182,12 +182,7 @@ namespace Emby.Server.Implementations.Library
             _defaultPasswordResetProvider = passwordResetProviders.OfType<DefaultPasswordResetProvider>().First();
         }
 
-        /// <summary>
-        /// Gets a User by Id.
-        /// </summary>
-        /// <param name="id">The id.</param>
-        /// <returns>User.</returns>
-        /// <exception cref="ArgumentException"></exception>
+        /// <inheritdoc />
         public User GetUserById(Guid id)
         {
             if (id == Guid.Empty)
@@ -199,11 +194,7 @@ namespace Emby.Server.Implementations.Library
             return user;
         }
 
-        /// <summary>
-        /// Gets the user by identifier.
-        /// </summary>
-        /// <param name="id">The identifier.</param>
-        /// <returns>User.</returns>
+        /// <inheritdoc />
         public User GetUserById(string id)
             => GetUserById(new Guid(id));
 
@@ -431,7 +422,6 @@ namespace Emby.Server.Implementations.Library
         {
             try
             {
-
                 var authenticationResult = provider is IRequiresResolvedUser requiresResolvedUser
                     ? await requiresResolvedUser.Authenticate(username, password, resolvedUser).ConfigureAwait(false)
                     : await provider.Authenticate(username, password).ConfigureAwait(false);
@@ -538,6 +528,8 @@ namespace Emby.Server.Implementations.Library
                 defaultName = "MyJellyfinUser";
             }
 
+            _logger.LogWarning("No users, creating one with username {UserName}", defaultName);
+
             var name = MakeValidUsername(defaultName);
 
             var user = InstantiateNewUser(name);
@@ -601,7 +593,7 @@ namespace Emby.Server.Implementations.Library
                 catch (Exception ex)
                 {
                     // Have to use a catch-all unfortunately because some .net image methods throw plain Exceptions
-                    _logger.LogError(ex, "Error generating PrimaryImageAspectRatio for {user}", user.Name);
+                    _logger.LogError(ex, "Error generating PrimaryImageAspectRatio for {User}", user.Name);
                 }
             }
 
@@ -625,7 +617,7 @@ namespace Emby.Server.Implementations.Library
             }
             catch (Exception ex)
             {
-                _logger.LogError(ex, "Error getting {imageType} image info for {imagePath}", image.Type, image.Path);
+                _logger.LogError(ex, "Error getting {ImageType} image info for {ImagePath}", image.Type, image.Path);
                 return null;
             }
         }
diff --git a/Emby.Server.Implementations/Library/UserViewManager.cs b/Emby.Server.Implementations/Library/UserViewManager.cs
index 88e2a8fa6..9c9977642 100644
--- a/Emby.Server.Implementations/Library/UserViewManager.cs
+++ b/Emby.Server.Implementations/Library/UserViewManager.cs
@@ -42,6 +42,11 @@ namespace Emby.Server.Implementations.Library
         {
             var user = _userManager.GetUserById(query.UserId);
 
+            if (user == null)
+            {
+                throw new ArgumentException("User Id specified in the query does not exist.", nameof(query));
+            }
+
             var folders = _libraryManager.GetUserRootFolder()
                 .GetChildren(user, true)
                 .OfType<Folder>()
@@ -54,7 +59,7 @@ namespace Emby.Server.Implementations.Library
             foreach (var folder in folders)
             {
                 var collectionFolder = folder as ICollectionFolder;
-                var folderViewType = collectionFolder == null ? null : collectionFolder.CollectionType;
+                var folderViewType = collectionFolder?.CollectionType;
 
                 if (UserView.IsUserSpecific(folder))
                 {
@@ -130,16 +135,11 @@ namespace Emby.Server.Implementations.Library
                 {
                     var index = orders.IndexOf(i.Id.ToString("N", CultureInfo.InvariantCulture));
 
-                    if (index == -1)
+                    if (index == -1
+                        && i is UserView view
+                        && view.DisplayParentId != Guid.Empty)
                     {
-                        var view = i as UserView;
-                        if (view != null)
-                        {
-                            if (!view.DisplayParentId.Equals(Guid.Empty))
-                            {
-                                index = orders.IndexOf(view.DisplayParentId.ToString("N", CultureInfo.InvariantCulture));
-                            }
-                        }
+                        index = orders.IndexOf(view.DisplayParentId.ToString("N", CultureInfo.InvariantCulture));
                     }
 
                     return index == -1 ? int.MaxValue : index;
diff --git a/Emby.Server.Implementations/Session/SessionManager.cs b/Emby.Server.Implementations/Session/SessionManager.cs
index 61329160a..d1392e162 100644
--- a/Emby.Server.Implementations/Session/SessionManager.cs
+++ b/Emby.Server.Implementations/Session/SessionManager.cs
@@ -1388,27 +1388,28 @@ namespace Emby.Server.Implementations.Session
             if (user != null)
             {
                 // TODO: Move this to userManager?
-                if (!string.IsNullOrEmpty(request.DeviceId))
+                if (!string.IsNullOrEmpty(request.DeviceId)
+                    && !_deviceManager.CanAccessDevice(user, request.DeviceId))
                 {
-                    if (!_deviceManager.CanAccessDevice(user, request.DeviceId))
-                    {
-                        throw new SecurityException("User is not allowed access from this device.");
-                    }
+                    throw new SecurityException("User is not allowed access from this device.");
                 }
             }
 
             if (enforcePassword)
             {
-                var result = await _userManager.AuthenticateUser(request.Username, request.Password, request.PasswordSha1, request.RemoteEndPoint, true).ConfigureAwait(false);
-
-                if (result == null)
-                {
-                    AuthenticationFailed?.Invoke(this, new GenericEventArgs<AuthenticationRequest>(request));
+                user = await _userManager.AuthenticateUser(
+                    request.Username,
+                    request.Password,
+                    request.PasswordSha1,
+                    request.RemoteEndPoint,
+                    true).ConfigureAwait(false);
+            }
 
-                    throw new SecurityException("Invalid user or password entered.");
-                }
+            if (user == null)
+            {
+                AuthenticationFailed?.Invoke(this, new GenericEventArgs<AuthenticationRequest>(request));
 
-                user = result;
+                throw new SecurityException("Invalid user or password entered.");
             }
 
             var token = GetAuthorizationToken(user, request.DeviceId, request.App, request.AppVersion, request.DeviceName);
diff --git a/MediaBrowser.Api/UserLibrary/UserViewsService.cs b/MediaBrowser.Api/UserLibrary/UserViewsService.cs
index 2fa5d8933..d62049ce9 100644
--- a/MediaBrowser.Api/UserLibrary/UserViewsService.cs
+++ b/MediaBrowser.Api/UserLibrary/UserViewsService.cs
@@ -10,6 +10,7 @@ using MediaBrowser.Model.Entities;
 using MediaBrowser.Model.Library;
 using MediaBrowser.Model.Querying;
 using MediaBrowser.Model.Services;
+using Microsoft.Extensions.Logging;
 
 namespace MediaBrowser.Api.UserLibrary
 {
@@ -49,7 +50,12 @@ namespace MediaBrowser.Api.UserLibrary
         private readonly IAuthorizationContext _authContext;
         private readonly ILibraryManager _libraryManager;
 
-        public UserViewsService(IUserManager userManager, IUserViewManager userViewManager, IDtoService dtoService, IAuthorizationContext authContext, ILibraryManager libraryManager)
+        public UserViewsService(
+            IUserManager userManager,
+            IUserViewManager userViewManager,
+            IDtoService dtoService,
+            IAuthorizationContext authContext,
+            ILibraryManager libraryManager)
         {
             _userManager = userManager;
             _userViewManager = userViewManager;
diff --git a/MediaBrowser.Common/Cryptography/PasswordHash.cs b/MediaBrowser.Common/Cryptography/PasswordHash.cs
index 4c6804097..19b8be47a 100644
--- a/MediaBrowser.Common/Cryptography/PasswordHash.cs
+++ b/MediaBrowser.Common/Cryptography/PasswordHash.cs
@@ -60,13 +60,13 @@ namespace MediaBrowser.Common.Cryptography
         /// <value>Return the hashed password.</value>
         public byte[] Hash { get; }
 
-        public static PasswordHash Parse(string storageString)
+        public static PasswordHash Parse(string hashString)
         {
-            string[] splitted = storageString.Split('$');
+            string[] splitted = hashString.Split('$');
             // The string should at least contain the hash function and the hash itself
             if (splitted.Length < 3)
             {
-                throw new ArgumentException("String doesn't contain enough segments", nameof(storageString));
+                throw new ArgumentException("String doesn't contain enough segments", nameof(hashString));
             }
 
             // Start at 1, the first index shouldn't contain any data
diff --git a/MediaBrowser.Controller/Entities/Folder.cs b/MediaBrowser.Controller/Entities/Folder.cs
index 1cf7bb9b2..07fbe6035 100644
--- a/MediaBrowser.Controller/Entities/Folder.cs
+++ b/MediaBrowser.Controller/Entities/Folder.cs
@@ -1152,6 +1152,11 @@ namespace MediaBrowser.Controller.Entities
 
         public List<BaseItem> GetChildren(User user, bool includeLinkedChildren)
         {
+            if (user == null)
+            {
+                throw new ArgumentNullException(nameof(user));
+            }
+
             return GetChildren(user, includeLinkedChildren, null);
         }
 
@@ -1163,7 +1168,10 @@ namespace MediaBrowser.Controller.Entities
             }
 
             //the true root should return our users root folder children
-            if (IsPhysicalRoot) return LibraryManager.GetUserRootFolder().GetChildren(user, includeLinkedChildren);
+            if (IsPhysicalRoot)
+            {
+                return LibraryManager.GetUserRootFolder().GetChildren(user, includeLinkedChildren);
+            }
 
             var result = new Dictionary<Guid, BaseItem>();
 
diff --git a/MediaBrowser.Controller/Library/IUserManager.cs b/MediaBrowser.Controller/Library/IUserManager.cs
index bbedc0442..eea2e3a71 100644
--- a/MediaBrowser.Controller/Library/IUserManager.cs
+++ b/MediaBrowser.Controller/Library/IUserManager.cs
@@ -39,17 +39,21 @@ namespace MediaBrowser.Controller.Library
         event EventHandler<GenericEventArgs<User>> UserDeleted;
 
         event EventHandler<GenericEventArgs<User>> UserCreated;
+
         event EventHandler<GenericEventArgs<User>> UserPolicyUpdated;
+
         event EventHandler<GenericEventArgs<User>> UserConfigurationUpdated;
+
         event EventHandler<GenericEventArgs<User>> UserPasswordChanged;
+
         event EventHandler<GenericEventArgs<User>> UserLockedOut;
 
         /// <summary>
-        /// Gets a User by Id
+        /// Gets a User by Id.
         /// </summary>
         /// <param name="id">The id.</param>
-        /// <returns>User.</returns>
-        /// <exception cref="ArgumentNullException"></exception>
+        /// <returns>The user with the specified Id, or <c>null</c> if the user doesn't exist.</returns>
+        /// <exception cref="ArgumentException"><c>id</c> is an empty Guid.</exception>
         User GetUserById(Guid id);
 
         /// <summary>
diff --git a/MediaBrowser.Model/Cryptography/ICryptoProvider.cs b/MediaBrowser.Model/Cryptography/ICryptoProvider.cs
index ce6493232..2d75c9b3e 100644
--- a/MediaBrowser.Model/Cryptography/ICryptoProvider.cs
+++ b/MediaBrowser.Model/Cryptography/ICryptoProvider.cs
@@ -8,10 +8,6 @@ namespace MediaBrowser.Model.Cryptography
 
         IEnumerable<string> GetSupportedHashMethods();
 
-        byte[] ComputeHash(string HashMethod, byte[] bytes);
-
-        byte[] ComputeHashWithDefaultMethod(byte[] bytes);
-
         byte[] ComputeHash(string HashMethod, byte[] bytes, byte[] salt);
 
         byte[] ComputeHashWithDefaultMethod(byte[] bytes, byte[] salt);