From 5d760b7ee806d3fb00ac5aa7d0981362526f1d11 Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Sun, 1 Mar 2020 21:38:34 +0100 Subject: [PATCH 1/7] Fix emby/user/public API leaking private data This commit fixes the emby/user/public API that was returning more data than necessary. Now only the following information are returned: - the account name - the primary image tag - the field hasPassword - the field hasConfiguredPassword, useful for the first wizard only (see https://github.com/jellyfin/jellyfin/issues/880#issuecomment-465370051) - the primary image aspect ratio A new DTO class, PrivateUserDTO has been created, and the route has been modified in order to return that data object. --- .../Library/UserManager.cs | 25 ++++++++++ MediaBrowser.Api/UserService.cs | 36 +++++++++----- .../Library/IUserManager.cs | 8 ++++ MediaBrowser.Model/Dto/PublicUserDto.cs | 48 +++++++++++++++++++ 4 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 MediaBrowser.Model/Dto/PublicUserDto.cs diff --git a/Emby.Server.Implementations/Library/UserManager.cs b/Emby.Server.Implementations/Library/UserManager.cs index 6e203f894f..8941767b41 100644 --- a/Emby.Server.Implementations/Library/UserManager.cs +++ b/Emby.Server.Implementations/Library/UserManager.cs @@ -613,6 +613,31 @@ namespace Emby.Server.Implementations.Library return dto; } + public PublicUserDto GetPublicUserDto(User user, string remoteEndPoint = null) + { + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } + + bool hasConfiguredPassword = GetAuthenticationProvider(user).HasPassword(user); + bool hasConfiguredEasyPassword = !string.IsNullOrEmpty(GetAuthenticationProvider(user).GetEasyPasswordHash(user)); + + bool hasPassword = user.Configuration.EnableLocalPassword && + !string.IsNullOrEmpty(remoteEndPoint) && + _networkManager.IsInLocalNetwork(remoteEndPoint) ? hasConfiguredEasyPassword : hasConfiguredPassword; + + + PublicUserDto dto = new PublicUserDto + { + Name = user.Name, + HasPassword = hasPassword, + HasConfiguredPassword = hasConfiguredPassword, + }; + + return dto; + } + public UserDto GetOfflineUserDto(User user) { var dto = GetUserDto(user); diff --git a/MediaBrowser.Api/UserService.cs b/MediaBrowser.Api/UserService.cs index 4015143497..b4ab8c9740 100644 --- a/MediaBrowser.Api/UserService.cs +++ b/MediaBrowser.Api/UserService.cs @@ -35,7 +35,7 @@ namespace MediaBrowser.Api } [Route("/Users/Public", "GET", Summary = "Gets a list of publicly visible users for display on a login screen.")] - public class GetPublicUsers : IReturn + public class GetPublicUsers : IReturn { } @@ -266,22 +266,36 @@ namespace MediaBrowser.Api _authContext = authContext; } + /// + /// Gets the public available Users information + /// + /// The request. + /// System.Object. public object Get(GetPublicUsers request) { - // If the startup wizard hasn't been completed then just return all users - if (!ServerConfigurationManager.Configuration.IsStartupWizardCompleted) + var users = _userManager + .Users + .Where(item => item.Policy.IsDisabled == false) + .Where(item => item.Policy.IsHidden == false); + + var deviceId = _authContext.GetAuthorizationInfo(Request).DeviceId; + + if (!string.IsNullOrWhiteSpace(deviceId)) { - return Get(new GetUsers - { - IsDisabled = false - }); + users = users.Where(i => _deviceManager.CanAccessDevice(i, deviceId)); } - return Get(new GetUsers + if (!_networkManager.IsInLocalNetwork(Request.RemoteIp)) { - IsHidden = false, - IsDisabled = false - }, true, true); + users = users.Where(i => i.Policy.EnableRemoteAccess); + } + + var result = users + .OrderBy(u => u.Name) + .Select(i => _userManager.GetPublicUserDto(i, Request.RemoteIp)) + .ToArray(); + + return ToOptimizedResult(result); } /// diff --git a/MediaBrowser.Controller/Library/IUserManager.cs b/MediaBrowser.Controller/Library/IUserManager.cs index be7b4ce59d..ec6cb35eb9 100644 --- a/MediaBrowser.Controller/Library/IUserManager.cs +++ b/MediaBrowser.Controller/Library/IUserManager.cs @@ -143,6 +143,14 @@ namespace MediaBrowser.Controller.Library /// UserDto. UserDto GetUserDto(User user, string remoteEndPoint = null); + /// + /// Gets the user public dto. + /// + /// Ther user.\ + /// The remote end point. + /// A public UserDto, aka a UserDto stripped of personal data. + PublicUserDto GetPublicUserDto(User user, string remoteEndPoint = null); + /// /// Authenticates the user. /// diff --git a/MediaBrowser.Model/Dto/PublicUserDto.cs b/MediaBrowser.Model/Dto/PublicUserDto.cs new file mode 100644 index 0000000000..bf529a2d0b --- /dev/null +++ b/MediaBrowser.Model/Dto/PublicUserDto.cs @@ -0,0 +1,48 @@ +using System; +using MediaBrowser.Model.Configuration; +using MediaBrowser.Model.Users; + +namespace MediaBrowser.Model.Dto +{ + /// + /// Class PublicUserDto. Its goal is to show only public information about a user + /// + public class PublicUserDto : IItemDto + { + /// + /// Gets or sets the name. + /// + /// The name. + public string Name { get; set; } + + /// + /// Gets or sets the primary image tag. + /// + /// The primary image tag. + public string PrimaryImageTag { get; set; } + + /// + /// Gets or sets a value indicating whether this instance has password. + /// + /// true if this instance has password; otherwise, false. + public bool HasPassword { get; set; } + + /// + /// Gets or sets a value indicating whether this instance has configured password. + /// + /// true if this instance has configured password; otherwise, false. + public bool HasConfiguredPassword { get; set; } + + /// + /// Gets or sets the primary image aspect ratio. + /// + /// The primary image aspect ratio. + public double? PrimaryImageAspectRatio { get; set; } + + /// + public override string ToString() + { + return Name ?? base.ToString(); + } + } +} \ No newline at end of file From 737d4d2b3f200e2dc140151d11dc9f29d70bd5bf Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Tue, 3 Mar 2020 19:51:03 +0100 Subject: [PATCH 2/7] Fix conditional with a less verbose one Co-Authored-By: Vasily --- MediaBrowser.Api/UserService.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MediaBrowser.Api/UserService.cs b/MediaBrowser.Api/UserService.cs index b4ab8c9740..5ad4fce64d 100644 --- a/MediaBrowser.Api/UserService.cs +++ b/MediaBrowser.Api/UserService.cs @@ -275,8 +275,8 @@ namespace MediaBrowser.Api { var users = _userManager .Users - .Where(item => item.Policy.IsDisabled == false) - .Where(item => item.Policy.IsHidden == false); + .Where(item => !item.Policy.IsDisabled) + .Where(item => !item.Policy.IsHidden); var deviceId = _authContext.GetAuthorizationInfo(Request).DeviceId; From cd471ed4df9260d9de3cfa1e58abf537aa460727 Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Tue, 3 Mar 2020 20:20:35 +0100 Subject: [PATCH 3/7] Fix emby/users/public not taking into account first run The previous implementation was not taking in account the first seup phase. Now the check has been added. A little method refactor has been done in order to make the code more elegant. --- MediaBrowser.Api/UserService.cs | 38 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/MediaBrowser.Api/UserService.cs b/MediaBrowser.Api/UserService.cs index 5ad4fce64d..bb630c0b37 100644 --- a/MediaBrowser.Api/UserService.cs +++ b/MediaBrowser.Api/UserService.cs @@ -273,29 +273,31 @@ namespace MediaBrowser.Api /// System.Object. public object Get(GetPublicUsers request) { - var users = _userManager + var result = _userManager .Users - .Where(item => !item.Policy.IsDisabled) - .Where(item => !item.Policy.IsHidden); + .Where(item => !item.Policy.IsDisabled); - var deviceId = _authContext.GetAuthorizationInfo(Request).DeviceId; - - if (!string.IsNullOrWhiteSpace(deviceId)) + if (ServerConfigurationManager.Configuration.IsStartupWizardCompleted) { - users = users.Where(i => _deviceManager.CanAccessDevice(i, deviceId)); + var deviceId = _authContext.GetAuthorizationInfo(Request).DeviceId; + result = result.Where(item => !item.Policy.IsHidden); + + if (!string.IsNullOrWhiteSpace(deviceId)) + { + result = result.Where(i => _deviceManager.CanAccessDevice(i, deviceId)); + } + + if (!_networkManager.IsInLocalNetwork(Request.RemoteIp)) + { + result = result.Where(i => i.Policy.EnableRemoteAccess); + } } - if (!_networkManager.IsInLocalNetwork(Request.RemoteIp)) - { - users = users.Where(i => i.Policy.EnableRemoteAccess); - } - - var result = users - .OrderBy(u => u.Name) - .Select(i => _userManager.GetPublicUserDto(i, Request.RemoteIp)) - .ToArray(); - - return ToOptimizedResult(result); + return ToOptimizedResult(result + .OrderBy(u => u.Name) + .Select(i => _userManager.GetPublicUserDto(i, Request.RemoteIp)) + .ToArray() + ); } /// From 5099f6e4a24128e47edbad674f551c5ebe29cb7e Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Thu, 5 Mar 2020 08:01:47 +0100 Subject: [PATCH 4/7] Add FIXME in HasConfiguredPassword public user DTO method --- MediaBrowser.Model/Dto/PublicUserDto.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/MediaBrowser.Model/Dto/PublicUserDto.cs b/MediaBrowser.Model/Dto/PublicUserDto.cs index bf529a2d0b..d5fd431eb6 100644 --- a/MediaBrowser.Model/Dto/PublicUserDto.cs +++ b/MediaBrowser.Model/Dto/PublicUserDto.cs @@ -31,6 +31,7 @@ namespace MediaBrowser.Model.Dto /// Gets or sets a value indicating whether this instance has configured password. /// /// true if this instance has configured password; otherwise, false. + // FIXME this shouldn't be here, but it's necessary when changing password at the first login public bool HasConfiguredPassword { get; set; } /// From 472efeeec4ddf5dbea1550aeea2173590b24953e Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Sat, 2 May 2020 13:09:57 +0200 Subject: [PATCH 5/7] Remove extra line in UserManager Co-authored-by: Bond-009 --- Emby.Server.Implementations/Library/UserManager.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Emby.Server.Implementations/Library/UserManager.cs b/Emby.Server.Implementations/Library/UserManager.cs index 8941767b41..903d43faa6 100644 --- a/Emby.Server.Implementations/Library/UserManager.cs +++ b/Emby.Server.Implementations/Library/UserManager.cs @@ -627,7 +627,6 @@ namespace Emby.Server.Implementations.Library !string.IsNullOrEmpty(remoteEndPoint) && _networkManager.IsInLocalNetwork(remoteEndPoint) ? hasConfiguredEasyPassword : hasConfiguredPassword; - PublicUserDto dto = new PublicUserDto { Name = user.Name, From 57cf19f058a12810b0d52dc43d84c1796697ce84 Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Wed, 6 May 2020 17:21:21 +0200 Subject: [PATCH 6/7] Fix variable declaration and follow sonarcloud suggestions --- Emby.Server.Implementations/Library/UserManager.cs | 5 +++-- MediaBrowser.Model/Dto/PublicUserDto.cs | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Emby.Server.Implementations/Library/UserManager.cs b/Emby.Server.Implementations/Library/UserManager.cs index 903d43faa6..6537a6a86a 100644 --- a/Emby.Server.Implementations/Library/UserManager.cs +++ b/Emby.Server.Implementations/Library/UserManager.cs @@ -620,8 +620,9 @@ namespace Emby.Server.Implementations.Library throw new ArgumentNullException(nameof(user)); } - bool hasConfiguredPassword = GetAuthenticationProvider(user).HasPassword(user); - bool hasConfiguredEasyPassword = !string.IsNullOrEmpty(GetAuthenticationProvider(user).GetEasyPasswordHash(user)); + IAuthenticationProvider authenticationProvider = GetAuthenticationProvider(user); + bool hasConfiguredPassword = authenticationProvider.HasPassword(user); + bool hasConfiguredEasyPassword = !string.IsNullOrEmpty(authenticationProvider.GetEasyPasswordHash(user)); bool hasPassword = user.Configuration.EnableLocalPassword && !string.IsNullOrEmpty(remoteEndPoint) && diff --git a/MediaBrowser.Model/Dto/PublicUserDto.cs b/MediaBrowser.Model/Dto/PublicUserDto.cs index d5fd431eb6..d4eec8b9df 100644 --- a/MediaBrowser.Model/Dto/PublicUserDto.cs +++ b/MediaBrowser.Model/Dto/PublicUserDto.cs @@ -1,6 +1,4 @@ using System; -using MediaBrowser.Model.Configuration; -using MediaBrowser.Model.Users; namespace MediaBrowser.Model.Dto { @@ -29,9 +27,10 @@ namespace MediaBrowser.Model.Dto /// /// Gets or sets a value indicating whether this instance has configured password. + /// Note that in this case this method should not be here, but it is necessary when changeing password at the + /// first login. /// /// true if this instance has configured password; otherwise, false. - // FIXME this shouldn't be here, but it's necessary when changing password at the first login public bool HasConfiguredPassword { get; set; } /// From 5c6339d8fd4b12237c6cb8eb9d115d59c9c27ddf Mon Sep 17 00:00:00 2001 From: Davide Polonio Date: Thu, 7 May 2020 09:14:00 +0200 Subject: [PATCH 7/7] Fix typo in PublicUserDto Co-authored-by: Vasily --- MediaBrowser.Model/Dto/PublicUserDto.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MediaBrowser.Model/Dto/PublicUserDto.cs b/MediaBrowser.Model/Dto/PublicUserDto.cs index d4eec8b9df..b6bfaf2e9b 100644 --- a/MediaBrowser.Model/Dto/PublicUserDto.cs +++ b/MediaBrowser.Model/Dto/PublicUserDto.cs @@ -27,7 +27,7 @@ namespace MediaBrowser.Model.Dto /// /// Gets or sets a value indicating whether this instance has configured password. - /// Note that in this case this method should not be here, but it is necessary when changeing password at the + /// Note that in this case this method should not be here, but it is necessary when changing password at the /// first login. /// /// true if this instance has configured password; otherwise, false. @@ -45,4 +45,4 @@ namespace MediaBrowser.Model.Dto return Name ?? base.ToString(); } } -} \ No newline at end of file +}