Merge pull request #6200 from nielsvanvelzen/fix-quickconnect

Refactor Quick Connect

Emby.Server.Implementations/QuickConnect/QuickConnectManager.cs

@@ -1,5 +1,3 @@
-#nullable disable
-
 using System;
 using System.Collections.Concurrent;
 using System.Globalization;
@@ -9,7 +7,6 @@ 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;
@@ -22,14 +19,28 @@ namespace Emby.Server.Implementations.QuickConnect
     /// </summary>
     public class QuickConnectManager : IQuickConnect, IDisposable
     {
-        private readonly RNGCryptoServiceProvider _rng = new RNGCryptoServiceProvider();
+        /// <summary>
-        private readonly ConcurrentDictionary<string, QuickConnectResult> _currentRequests = new ConcurrentDictionary<string, QuickConnectResult>();
+        /// The name of internal access tokens.
+        /// </summary>
+        private const string TokenName = "QuickConnect";
+
+        /// <summary>
+        /// The length of user facing codes.
+        /// </summary>
+        private const int CodeLength = 6;
+
+        /// <summary>
+        /// The time (in minutes) that the quick connect token is valid.
+        /// </summary>
+        private const int Timeout = 10;
+
+        private readonly RNGCryptoServiceProvider _rng = new();
+        private readonly ConcurrentDictionary<string, QuickConnectResult> _currentRequests = new();
 
         private readonly IServerConfigurationManager _config;
         private readonly ILogger<QuickConnectManager> _logger;
-        private readonly IAuthenticationRepository _authenticationRepository;
-        private readonly IAuthorizationContext _authContext;
         private readonly IServerApplicationHost _appHost;
+        private readonly IAuthenticationRepository _authenticationRepository;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="QuickConnectManager"/> class.
@@ -38,86 +49,42 @@ namespace Emby.Server.Implementations.QuickConnect
         /// <param name="config">Configuration.</param>
         /// <param name="logger">Logger.</param>
         /// <param name="appHost">Application host.</param>
-        /// <param name="authContext">Authentication context.</param>
         /// <param name="authenticationRepository">Authentication repository.</param>
         public QuickConnectManager(
             IServerConfigurationManager config,
             ILogger<QuickConnectManager> logger,
             IServerApplicationHost appHost,
-            IAuthorizationContext authContext,
             IAuthenticationRepository authenticationRepository)
         {
             _config = config;
             _logger = logger;
             _appHost = appHost;
-            _authContext = authContext;
             _authenticationRepository = authenticationRepository;
-
-            ReloadConfiguration();
         }
 
-        /// <inheritdoc/>
+        /// <inheritdoc />
-        public int CodeLength { get; set; } = 6;
+        public bool IsEnabled => _config.Configuration.QuickConnectAvailable;
 
-        /// <inheritdoc/>
+        /// <summary>
-        public string TokenName { get; set; } = "QuickConnect";
+        /// Assert that quick connect is currently active and throws an exception if it is not.
-
+        /// </summary>
-        /// <inheritdoc/>
+        private void AssertActive()
-        public QuickConnectState State { get; private set; } = QuickConnectState.Unavailable;
-
-        /// <inheritdoc/>
-        public int Timeout { get; set; } = 5;
-
-        private DateTime DateActivated { get; set; }
-
-        /// <inheritdoc/>
-        public void AssertActive()
         {
-            if (State != QuickConnectState.Active)
+            if (!IsEnabled)
             {
-                throw new ArgumentException("Quick connect is not active on this server");
+                throw new AuthenticationException("Quick connect is not active on this server");
             }
         }
 
-        /// <inheritdoc/>
-        public void Activate()
-        {
-            DateActivated = DateTime.UtcNow;
-            SetState(QuickConnectState.Active);
-        }
-
-        /// <inheritdoc/>
-        public void SetState(QuickConnectState newState)
-        {
-            _logger.LogDebug("Changed quick connect state from {State} to {newState}", State, newState);
-
-            ExpireRequests(true);
-
-            State = newState;
-            _config.Configuration.QuickConnectAvailable = newState == QuickConnectState.Available || newState == QuickConnectState.Active;
-            _config.SaveConfiguration();
-
-            _logger.LogDebug("Configuration saved");
-        }
-
         /// <inheritdoc/>
         public QuickConnectResult TryConnect()
         {
+            AssertActive();
             ExpireRequests();
 
-            if (State != QuickConnectState.Active)
+            var secret = GenerateSecureRandom();
-            {
-                _logger.LogDebug("Refusing quick connect initiation request, current state is {State}", State);
-                throw new AuthenticationException("Quick connect is not active on this server");
-            }
-
             var code = GenerateCode();
-            var result = new QuickConnectResult()
+            var result = new QuickConnectResult(secret, code, DateTime.UtcNow);
-            {
-                Secret = GenerateSecureRandom(),
-                DateAdded = DateTime.UtcNow,
-                Code = code
-            };
 
             _currentRequests[code] = result;
             return result;
@@ -126,12 +93,12 @@ namespace Emby.Server.Implementations.QuickConnect
         /// <inheritdoc/>
         public QuickConnectResult CheckRequestStatus(string secret)
         {
-            ExpireRequests();
             AssertActive();
+            ExpireRequests();
 
             string code = _currentRequests.Where(x => x.Value.Secret == secret).Select(x => x.Value.Code).DefaultIfEmpty(string.Empty).First();
 
-            if (!_currentRequests.TryGetValue(code, out QuickConnectResult result))
+            if (!_currentRequests.TryGetValue(code, out QuickConnectResult? result))
             {
                 throw new ResourceNotFoundException("Unable to find request with provided secret");
             }
@@ -139,8 +106,11 @@ namespace Emby.Server.Implementations.QuickConnect
             return result;
         }
 
-        /// <inheritdoc/>
+        /// <summary>
-        public string GenerateCode()
+        /// Generates a short code to display to the user to uniquely identify this request.
+        /// </summary>
+        /// <returns>A short, unique alphanumeric string.</returns>
+        private string GenerateCode()
         {
             Span<byte> raw = stackalloc byte[4];
 
@@ -161,10 +131,10 @@ namespace Emby.Server.Implementations.QuickConnect
         /// <inheritdoc/>
         public bool AuthorizeRequest(Guid userId, string code)
         {
-            ExpireRequests();
             AssertActive();
+            ExpireRequests();
 
-            if (!_currentRequests.TryGetValue(code, out QuickConnectResult result))
+            if (!_currentRequests.TryGetValue(code, out QuickConnectResult? result))
             {
                 throw new ResourceNotFoundException("Unable to find request");
             }
@@ -174,16 +144,16 @@ namespace Emby.Server.Implementations.QuickConnect
                 throw new InvalidOperationException("Request is already authorized");
             }
 
-            result.Authentication = Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture);
+            var token = Guid.NewGuid();
+            result.Authentication = token;
 
             // 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.UtcNow.Subtract(TimeSpan.FromMinutes(Timeout));
+            result.DateAdded = DateTime.Now.Add(TimeSpan.FromMinutes(1));
-            result.DateAdded = added.Subtract(TimeSpan.FromMinutes(Timeout - 1));
 
             _authenticationRepository.Create(new AuthenticationInfo
             {
                 AppName = TokenName,
-                AccessToken = result.Authentication,
+                AccessToken = token.ToString("N", CultureInfo.InvariantCulture),
                 DateCreated = DateTime.UtcNow,
                 DeviceId = _appHost.SystemId,
                 DeviceName = _appHost.FriendlyName,
@@ -196,28 +166,6 @@ namespace Emby.Server.Implementations.QuickConnect
             return true;
         }
 
-        /// <inheritdoc/>
-        public int DeleteAllDevices(Guid user)
-        {
-            var raw = _authenticationRepository.Get(new AuthenticationInfoQuery()
-            {
-                DeviceId = _appHost.SystemId,
-                UserId = user
-            });
-
-            var tokens = raw.Items.Where(x => x.AppName.StartsWith(TokenName, StringComparison.Ordinal));
-
-            var removed = 0;
-            foreach (var token in tokens)
-            {
-                _authenticationRepository.Delete(token);
-                _logger.LogDebug("Deleted token {AccessToken}", token.AccessToken);
-                removed++;
-            }
-
-            return removed;
-        }
-
         /// <summary>
         /// Dispose.
         /// </summary>
@@ -235,7 +183,7 @@ namespace Emby.Server.Implementations.QuickConnect
         {
             if (disposing)
             {
-                _rng?.Dispose();
+                _rng.Dispose();
             }
         }
 
@@ -247,22 +195,19 @@ namespace Emby.Server.Implementations.QuickConnect
             return Convert.ToHexString(bytes);
         }
 
-        /// <inheritdoc/>
+        /// <summary>
-        public void ExpireRequests(bool expireAll = false)
+        /// Expire quick connect requests that are over the time limit. If <paramref name="expireAll"/> is true, all requests are unconditionally expired.
+        /// </summary>
+        /// <param name="expireAll">If true, all requests will be expired.</param>
+        private void ExpireRequests(bool expireAll = false)
         {
-            // Check if quick connect should be deactivated
+            // All requests before this timestamp have expired
-            if (State == QuickConnectState.Active && DateTime.UtcNow > DateActivated.AddMinutes(Timeout) && !expireAll)
+            var minTime = DateTime.UtcNow.AddMinutes(-Timeout);
-            {
-                _logger.LogDebug("Quick connect time expired, deactivating");
-                SetState(QuickConnectState.Available);
-                expireAll = true;
-            }
 
             // Expire stale connection requests
             foreach (var (_, currentRequest) in _currentRequests)
             {
-                var added = currentRequest.DateAdded ?? DateTime.UnixEpoch;
+                if (expireAll || currentRequest.DateAdded > minTime)
-                if (expireAll || DateTime.UtcNow > added.AddMinutes(Timeout))
                 {
                     var code = currentRequest.Code;
                     _logger.LogDebug("Removing expired request {Code}", code);
@@ -274,10 +219,5 @@ namespace Emby.Server.Implementations.QuickConnect
                 }
             }
         }
-
-        private void ReloadConfiguration()
-        {
-            State = _config.Configuration.QuickConnectAvailable ? QuickConnectState.Available : QuickConnectState.Unavailable;
-        }
     }
 }

Jellyfin.Api/Controllers/QuickConnectController.cs

@@ -2,6 +2,7 @@ using System.ComponentModel.DataAnnotations;
 using Jellyfin.Api.Constants;
 using Jellyfin.Api.Helpers;
 using MediaBrowser.Common.Extensions;
+using MediaBrowser.Controller.Authentication;
 using MediaBrowser.Controller.QuickConnect;
 using MediaBrowser.Model.QuickConnect;
 using Microsoft.AspNetCore.Authorization;
@@ -30,13 +31,12 @@ namespace Jellyfin.Api.Controllers
         /// Gets the current quick connect state.
         /// </summary>
         /// <response code="200">Quick connect state returned.</response>
-        /// <returns>The current <see cref="QuickConnectState"/>.</returns>
+        /// <returns>Whether Quick Connect is enabled on the server or not.</returns>
-        [HttpGet("Status")]
+        [HttpGet("Enabled")]
         [ProducesResponseType(StatusCodes.Status200OK)]
-        public ActionResult<QuickConnectState> GetStatus()
+        public ActionResult<bool> GetEnabled()
         {
-            _quickConnect.ExpireRequests();
+            return _quickConnect.IsEnabled;
-            return _quickConnect.State;
         }
 
         /// <summary>
@@ -49,7 +49,14 @@ namespace Jellyfin.Api.Controllers
         [ProducesResponseType(StatusCodes.Status200OK)]
         public ActionResult<QuickConnectResult> Initiate()
         {
-            return _quickConnect.TryConnect();
+            try
+            {
+                return _quickConnect.TryConnect();
+            }
+            catch (AuthenticationException)
+            {
+                return Unauthorized("Quick connect is disabled");
+            }
         }
 
         /// <summary>
@@ -72,42 +79,10 @@ namespace Jellyfin.Api.Controllers
             {
                 return NotFound("Unknown secret");
             }
-        }
+            catch (AuthenticationException)
-
-        /// <summary>
-        /// Temporarily activates quick connect for five minutes.
-        /// </summary>
-        /// <response code="204">Quick connect has been temporarily activated.</response>
-        /// <response code="403">Quick connect is unavailable on this server.</response>
-        /// <returns>An <see cref="NoContentResult"/> on success.</returns>
-        [HttpPost("Activate")]
-        [Authorize(Policy = Policies.DefaultAuthorization)]
-        [ProducesResponseType(StatusCodes.Status204NoContent)]
-        [ProducesResponseType(StatusCodes.Status403Forbidden)]
-        public ActionResult Activate()
-        {
-            if (_quickConnect.State == QuickConnectState.Unavailable)
             {
-                return StatusCode(StatusCodes.Status403Forbidden, "Quick connect is unavailable");
+                return Unauthorized("Quick connect is disabled");
             }
-
-            _quickConnect.Activate();
-            return NoContent();
-        }
-
-        /// <summary>
-        /// Enables or disables quick connect.
-        /// </summary>
-        /// <param name="status">New <see cref="QuickConnectState"/>.</param>
-        /// <response code="204">Quick connect state set successfully.</response>
-        /// <returns>An <see cref="NoContentResult"/> on success.</returns>
-        [HttpPost("Available")]
-        [Authorize(Policy = Policies.RequiresElevation)]
-        [ProducesResponseType(StatusCodes.Status204NoContent)]
-        public ActionResult Available([FromQuery] QuickConnectState status = QuickConnectState.Available)
-        {
-            _quickConnect.SetState(status);
-            return NoContent();
         }
 
         /// <summary>
@@ -129,26 +104,14 @@ namespace Jellyfin.Api.Controllers
                 return StatusCode(StatusCodes.Status403Forbidden, "Unknown user id");
             }
 
-            return _quickConnect.AuthorizeRequest(userId.Value, code);
+            try
-        }
-
-        /// <summary>
-        /// Deauthorize all quick connect devices for the current user.
-        /// </summary>
-        /// <response code="200">All quick connect devices were deleted.</response>
-        /// <returns>The number of devices that were deleted.</returns>
-        [HttpPost("Deauthorize")]
-        [Authorize(Policy = Policies.DefaultAuthorization)]
-        [ProducesResponseType(StatusCodes.Status200OK)]
-        public ActionResult<int> Deauthorize()
-        {
-            var userId = ClaimHelpers.GetUserId(Request.HttpContext.User);
-            if (!userId.HasValue)
             {
-                return 0;
+                return _quickConnect.AuthorizeRequest(userId.Value, code);
+            }
+            catch (AuthenticationException)
+            {
+                return Unauthorized("Quick connect is disabled");
             }
-
-            return _quickConnect.DeleteAllDevices(userId.Value);
         }
     }
 }

MediaBrowser.Controller/QuickConnect/IQuickConnect.cs

@@ -1,5 +1,3 @@
-#nullable disable
-
 using System;
 using MediaBrowser.Model.QuickConnect;
 
@@ -11,40 +9,9 @@ namespace MediaBrowser.Controller.QuickConnect
     public interface IQuickConnect
     {
         /// <summary>
-        /// Gets or sets the length of user facing codes.
+        /// Gets a value indicating whether quick connect is enabled or not.
         /// </summary>
-        int CodeLength { get; set; }
+        bool IsEnabled { get; }
-
-        /// <summary>
-        /// Gets or sets the name of internal access tokens.
-        /// </summary>
-        string TokenName { get; set; }
-
-        /// <summary>
-        /// Gets the current state of quick connect.
-        /// </summary>
-        QuickConnectState State { get; }
-
-        /// <summary>
-        /// Gets or sets the time (in minutes) before quick connect will automatically deactivate.
-        /// </summary>
-        int Timeout { get; set; }
-
-        /// <summary>
-        /// Assert that quick connect is currently active and throws an exception if it is not.
-        /// </summary>
-        void AssertActive();
-
-        /// <summary>
-        /// Temporarily activates quick connect for a short amount of time.
-        /// </summary>
-        void Activate();
-
-        /// <summary>
-        /// Changes the state of quick connect.
-        /// </summary>
-        /// <param name="newState">New state to change to.</param>
-        void SetState(QuickConnectState newState);
 
         /// <summary>
         /// Initiates a new quick connect request.
@@ -66,24 +33,5 @@ namespace MediaBrowser.Controller.QuickConnect
         /// <param name="code">Identifying code for the request.</param>
         /// <returns>A boolean indicating if the authorization completed successfully.</returns>
         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.
-        /// </summary>
-        /// <param name="expireAll">If true, all requests will be expired.</param>
-        void ExpireRequests(bool expireAll = false);
-
-        /// <summary>
-        /// Deletes all quick connect access tokens for the provided user.
-        /// </summary>
-        /// <param name="user">Guid of the user to delete tokens for.</param>
-        /// <returns>A count of the deleted tokens.</returns>
-        int DeleteAllDevices(Guid user);
-
-        /// <summary>
-        /// Generates a short code to display to the user to uniquely identify this request.
-        /// </summary>
-        /// <returns>A short, unique alphanumeric string.</returns>
-        string GenerateCode();
     }
 }

MediaBrowser.Model/QuickConnect/QuickConnectResult.cs

@@ -3,38 +3,46 @@ using System;
 namespace MediaBrowser.Model.QuickConnect
 {
     /// <summary>
-    /// Stores the result of an incoming quick connect request.
+    /// Stores the state of an quick connect request.
     /// </summary>
     public class QuickConnectResult
     {
+        /// <summary>
+        /// Initializes a new instance of the <see cref="QuickConnectResult"/> class.
+        /// </summary>
+        /// <param name="secret">The secret used to query the request state.</param>
+        /// <param name="code">The code used to allow the request.</param>
+        /// <param name="dateAdded">The time when the request was created.</param>
+        public QuickConnectResult(string secret, string code, DateTime dateAdded)
+        {
+            Secret = secret;
+            Code = code;
+            DateAdded = dateAdded;
+        }
+
         /// <summary>
         /// Gets a value indicating whether this request is authorized.
         /// </summary>
-        public bool Authenticated => !string.IsNullOrEmpty(Authentication);
+        public bool Authenticated => Authentication != null;
 
         /// <summary>
-        /// Gets or sets the secret value used to uniquely identify this request. Can be used to retrieve authentication information.
+        /// Gets the secret value used to uniquely identify this request. Can be used to retrieve authentication information.
         /// </summary>
-        public string? Secret { get; set; }
+        public string Secret { get; }
 
         /// <summary>
-        /// Gets or sets the user facing code used so the user can quickly differentiate this request from others.
+        /// Gets the user facing code used so the user can quickly differentiate this request from others.
         /// </summary>
-        public string? Code { get; set; }
+        public string Code { get; }
 
         /// <summary>
         /// Gets or sets the private access token.
         /// </summary>
-        public string? Authentication { get; set; }
+        public Guid? Authentication { get; set; }
-
-        /// <summary>
-        /// Gets or sets an error message.
-        /// </summary>
-        public string? Error { get; set; }
 
         /// <summary>
         /// Gets or sets the DateTime that this request was created.
         /// </summary>
-        public DateTime? DateAdded { get; set; }
+        public DateTime DateAdded { get; set; }
     }
 }

MediaBrowser.Model/QuickConnect/QuickConnectState.cs

@@ -1,23 +0,0 @@
-namespace MediaBrowser.Model.QuickConnect
-{
-    /// <summary>
-    /// Quick connect state.
-    /// </summary>
-    public enum QuickConnectState
-    {
-        /// <summary>
-        /// This feature has not been opted into and is unavailable until the server administrator chooses to opt-in.
-        /// </summary>
-        Unavailable = 0,
-
-        /// <summary>
-        /// The feature is enabled for use on the server but is not currently accepting connection requests.
-        /// </summary>
-        Available = 1,
-
-        /// <summary>
-        /// The feature is actively accepting connection requests.
-        /// </summary>
-        Active = 2
-    }
-}