Apply suggestions from code review

Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs

@@ -3,17 +3,16 @@ using System.Collections.Concurrent;
 using System.Globalization;
 using System.Linq;
 using System.Security.Cryptography;
+using MediaBrowser.Common;
+using MediaBrowser.Common.Extensions;
 using MediaBrowser.Controller;
+using MediaBrowser.Controller.Authentication;
 using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Net;
 using MediaBrowser.Controller.QuickConnect;
 using MediaBrowser.Controller.Security;
 using MediaBrowser.Model.QuickConnect;
-using Microsoft.AspNetCore.Http;
-using MediaBrowser.Common;
 using Microsoft.Extensions.Logging;
-using MediaBrowser.Common.Extensions;
-using MediaBrowser.Controller.Authentication;
 
 namespace Emby.Server.Implementations.QuickConnect
 {
@@ -60,7 +59,7 @@ namespace Emby.Server.Implementations.QuickConnect
         public int CodeLength { get; set; } = 6;
 
         /// <inheritdoc/>
-        public string TokenNamePrefix { get; set; } = "QuickConnect-";
+        public string TokenName { get; set; } = "QuickConnect";
 
         /// <inheritdoc/>
         public QuickConnectState State { get; private set; } = QuickConnectState.Unavailable;
@@ -82,7 +81,7 @@ namespace Emby.Server.Implementations.QuickConnect
         /// <inheritdoc/>
         public void Activate()
         {
-            DateActivated = DateTime.Now;
+            DateActivated = DateTime.UtcNow;
             SetState(QuickConnectState.Active);
         }
 
@@ -101,7 +100,7 @@ namespace Emby.Server.Implementations.QuickConnect
         }
 
         /// <inheritdoc/>
-        public QuickConnectResult TryConnect(string friendlyName)
+        public QuickConnectResult TryConnect()
         {
             ExpireRequests();
 
@@ -111,14 +110,11 @@ namespace Emby.Server.Implementations.QuickConnect
                 throw new AuthenticationException("Quick connect is not active on this server");
             }
 
-            _logger.LogDebug("Got new quick connect request from {friendlyName}", friendlyName);
-
             var code = GenerateCode();
             var result = new QuickConnectResult()
             {
                 Secret = GenerateSecureRandom(),
-                FriendlyName = friendlyName,
+                DateAdded = DateTime.UtcNow,
-                DateAdded = DateTime.Now,
                 Code = code
             };
 
@@ -162,13 +158,11 @@ namespace Emby.Server.Implementations.QuickConnect
         }
 
         /// <inheritdoc/>
-        public bool AuthorizeRequest(HttpRequest request, string code)
+        public bool AuthorizeRequest(Guid userId, string code)
         {
             ExpireRequests();
             AssertActive();
 
-            var auth = _authContext.GetAuthorizationInfo(request);
-
             if (!_currentRequests.TryGetValue(code, out QuickConnectResult result))
             {
                 throw new ResourceNotFoundException("Unable to find request");
@@ -182,21 +176,21 @@ namespace Emby.Server.Implementations.QuickConnect
             result.Authentication = Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture);
 
             // Change the time on the request so it expires one minute into the future. It can't expire immediately as otherwise some clients wouldn't ever see that they have been authenticated.
-            var added = result.DateAdded ?? DateTime.Now.Subtract(new TimeSpan(0, Timeout, 0));
+            var added = result.DateAdded ?? DateTime.UtcNow.Subtract(TimeSpan.FromMinutes(Timeout));
-            result.DateAdded = added.Subtract(new TimeSpan(0, Timeout - 1, 0));
+            result.DateAdded = added.Subtract(TimeSpan.FromMinutes(Timeout - 1));
 
             _authenticationRepository.Create(new AuthenticationInfo
             {
-                AppName = TokenNamePrefix + result.FriendlyName,
+                AppName = TokenName,
                 AccessToken = result.Authentication,
                 DateCreated = DateTime.UtcNow,
                 DeviceId = _appHost.SystemId,
                 DeviceName = _appHost.FriendlyName,
                 AppVersion = _appHost.ApplicationVersionString,
-                UserId = auth.UserId
+                UserId = userId
             });
 
-            _logger.LogInformation("Allowing device {FriendlyName} to login as user {Username} with quick connect code {Code}", result.FriendlyName, auth.User.Username, result.Code);
+            _logger.LogDebug("Authorizing device with code {Code} to login as user {userId}", code, userId);
 
             return true;
         }
@@ -210,7 +204,7 @@ namespace Emby.Server.Implementations.QuickConnect
                 UserId = user
             });
 
-            var tokens = raw.Items.Where(x => x.AppName.StartsWith(TokenNamePrefix, StringComparison.CurrentCulture));
+            var tokens = raw.Items.Where(x => x.AppName.StartsWith(TokenName, StringComparison.CurrentCulture));
 
             var removed = 0;
             foreach (var token in tokens)
@@ -256,7 +250,7 @@ namespace Emby.Server.Implementations.QuickConnect
         public void ExpireRequests(bool expireAll = false)
         {
             // Check if quick connect should be deactivated
-            if (State == QuickConnectState.Active && DateTime.Now > DateActivated.AddMinutes(Timeout) && !expireAll)
+            if (State == QuickConnectState.Active && DateTime.UtcNow > DateActivated.AddMinutes(Timeout) && !expireAll)
             {
                 _logger.LogDebug("Quick connect time expired, deactivating");
                 SetState(QuickConnectState.Available);
@@ -270,7 +264,7 @@ namespace Emby.Server.Implementations.QuickConnect
             for (int i = 0; i < values.Count; i++)
             {
                 var added = values[i].DateAdded ?? DateTime.UnixEpoch;
-                if (DateTime.Now > added.AddMinutes(Timeout) || expireAll)
+                if (DateTime.UtcNow > added.AddMinutes(Timeout) || expireAll)
                 {
                     code = values[i].Code;
                     _logger.LogDebug("Removing expired request {code}", code);

Emby.Server.Implementations/Session/SessionManager.cs

@@ -1433,7 +1433,7 @@ namespace Emby.Server.Implementations.Session
                 Limit = 1
             });
 
-            if (result.TotalRecordCount < 1)
+            if (result.TotalRecordCount == 0)
             {
                 throw new SecurityException("Unknown quick connect token");
             }

Jellyfin.Api/Controllers/QuickConnectController.cs

@@ -1,8 +1,8 @@
+using System;
 using System.ComponentModel.DataAnnotations;
 using Jellyfin.Api.Constants;
 using Jellyfin.Api.Helpers;
 using MediaBrowser.Common.Extensions;
-using MediaBrowser.Controller.Library;
 using MediaBrowser.Controller.Net;
 using MediaBrowser.Controller.QuickConnect;
 using MediaBrowser.Model.QuickConnect;
@@ -18,22 +18,18 @@ namespace Jellyfin.Api.Controllers
     public class QuickConnectController : BaseJellyfinApiController
     {
         private readonly IQuickConnect _quickConnect;
-        private readonly IUserManager _userManager;
         private readonly IAuthorizationContext _authContext;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="QuickConnectController"/> class.
         /// </summary>
         /// <param name="quickConnect">Instance of the <see cref="IQuickConnect"/> interface.</param>
-        /// <param name="userManager">Instance of the <see cref="IUserManager"/> interface.</param>
         /// <param name="authContext">Instance of the <see cref="IAuthorizationContext"/> interface.</param>
         public QuickConnectController(
             IQuickConnect quickConnect,
-            IUserManager userManager,
             IAuthorizationContext authContext)
         {
             _quickConnect = quickConnect;
-            _userManager = userManager;
             _authContext = authContext;
         }
 
@@ -53,15 +49,14 @@ namespace Jellyfin.Api.Controllers
         /// <summary>
         /// Initiate a new quick connect request.
         /// </summary>
-        /// <param name="friendlyName">Device friendly name.</param>
         /// <response code="200">Quick connect request successfully created.</response>
         /// <response code="401">Quick connect is not active on this server.</response>
         /// <returns>A <see cref="QuickConnectResult"/> with a secret and code for future use or an error message.</returns>
         [HttpGet("Initiate")]
         [ProducesResponseType(StatusCodes.Status200OK)]
-        public ActionResult<QuickConnectResult> Initiate([FromQuery] string? friendlyName)
+        public ActionResult<QuickConnectResult> Initiate()
         {
-            return _quickConnect.TryConnect(friendlyName);
+            return _quickConnect.TryConnect();
         }
 
         /// <summary>
@@ -74,12 +69,11 @@ namespace Jellyfin.Api.Controllers
         [HttpGet("Connect")]
         [ProducesResponseType(StatusCodes.Status200OK)]
         [ProducesResponseType(StatusCodes.Status404NotFound)]
-        public ActionResult<QuickConnectResult> Connect([FromQuery] string? secret)
+        public ActionResult<QuickConnectResult> Connect([FromQuery, Required] string secret)
         {
             try
             {
-                var result = _quickConnect.CheckRequestStatus(secret);
+                return _quickConnect.CheckRequestStatus(secret);
-                return result;
             }
             catch (ResourceNotFoundException)
             {
@@ -117,9 +111,9 @@ namespace Jellyfin.Api.Controllers
         [HttpPost("Available")]
         [Authorize(Policy = Policies.RequiresElevation)]
         [ProducesResponseType(StatusCodes.Status204NoContent)]
-        public ActionResult Available([FromQuery] QuickConnectState? status)
+        public ActionResult Available([FromQuery] QuickConnectState status = QuickConnectState.Available)
         {
-            _quickConnect.SetState(status ?? QuickConnectState.Available);
+            _quickConnect.SetState(status);
             return NoContent();
         }
 
@@ -127,16 +121,22 @@ namespace Jellyfin.Api.Controllers
         /// Authorizes a pending quick connect request.
         /// </summary>
         /// <param name="code">Quick connect code to authorize.</param>
+        /// <param name="userId">User id.</param>
         /// <response code="200">Quick connect result authorized successfully.</response>
-        /// <response code="400">Missing quick connect code.</response>
+        /// <response code="403">User is not allowed to authorize quick connect requests.</response>
         /// <returns>Boolean indicating if the authorization was successful.</returns>
         [HttpPost("Authorize")]
         [Authorize(Policy = Policies.DefaultAuthorization)]
         [ProducesResponseType(StatusCodes.Status200OK)]
-        [ProducesResponseType(StatusCodes.Status400BadRequest)]
+        [ProducesResponseType(StatusCodes.Status403Forbidden)]
-        public ActionResult<bool> Authorize([FromQuery, Required] string? code)
+        public ActionResult<bool> Authorize([FromQuery, Required] string code, [FromQuery, Required] Guid userId)
         {
-            return _quickConnect.AuthorizeRequest(Request, code);
+            if (!RequestHelpers.AssertCanUpdateUser(_authContext, HttpContext.Request, userId, true))
+            {
+                return Forbid("User is not allowed to authorize quick connect requests.");
+            }
+
+            return _quickConnect.AuthorizeRequest(userId, code);
         }
 
         /// <summary>

Jellyfin.Api/Controllers/UserController.cs

@@ -239,11 +239,9 @@ namespace Jellyfin.Api.Controllers
                     DeviceName = auth.Device,
                 };
 
-                var result = await _sessionManager.AuthenticateQuickConnect(
+                return await _sessionManager.AuthenticateQuickConnect(
                     authRequest,
                     request.Token).ConfigureAwait(false);
-
-                return result;
             }
             catch (SecurityException e)
             {

MediaBrowser.Controller/QuickConnect/IQuickConnect.cs

@@ -1,6 +1,5 @@
 using System;
 using MediaBrowser.Model.QuickConnect;
-using Microsoft.AspNetCore.Http;
 
 namespace MediaBrowser.Controller.QuickConnect
 {
@@ -15,9 +14,9 @@ namespace MediaBrowser.Controller.QuickConnect
         int CodeLength { get; set; }
 
         /// <summary>
-        /// Gets or sets the string to prefix internal access tokens with.
+        /// Gets or sets the name of internal access tokens.
         /// </summary>
-        string TokenNamePrefix { get; set; }
+        string TokenName { get; set; }
 
         /// <summary>
         /// Gets the current state of quick connect.
@@ -48,9 +47,8 @@ namespace MediaBrowser.Controller.QuickConnect
         /// <summary>
         /// Initiates a new quick connect request.
         /// </summary>
-        /// <param name="friendlyName">Friendly device name to display in the request UI.</param>
         /// <returns>A quick connect result with tokens to proceed or throws an exception if not active.</returns>
-        QuickConnectResult TryConnect(string friendlyName);
+        QuickConnectResult TryConnect();
 
         /// <summary>
         /// Checks the status of an individual request.
@@ -62,10 +60,10 @@ namespace MediaBrowser.Controller.QuickConnect
         /// <summary>
         /// Authorizes a quick connect request to connect as the calling user.
         /// </summary>
-        /// <param name="request">HTTP request object.</param>
+        /// <param name="userId">User id.</param>
         /// <param name="code">Identifying code for the request.</param>
         /// <returns>A boolean indicating if the authorization completed successfully.</returns>
-        bool AuthorizeRequest(HttpRequest request, string code);
+        bool AuthorizeRequest(Guid userId, string code);
 
         /// <summary>
         /// Expire quick connect requests that are over the time limit. If <paramref name="expireAll"/> is true, all requests are unconditionally expired.

MediaBrowser.Model/QuickConnect/QuickConnectResult.cs

@@ -22,11 +22,6 @@ namespace MediaBrowser.Model.QuickConnect
         /// </summary>
         public string? Code { get; set; }
 
-        /// <summary>
-        /// Gets or sets the device friendly name.
-        /// </summary>
-        public string? FriendlyName { get; set; }
-
         /// <summary>
         /// Gets or sets the private access token.
         /// </summary>