From 839b6816f2ae3e5f54459a0f09dad6b17e2d1e07 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Mon, 20 Jan 2025 14:54:14 +0100 Subject: [PATCH] Merge commit from fork * Add TimedScope * Use TimedScope in login endpoint * Use seperate default duration and only calculate average of actual successful responses * Only return detailed error responses if credentials are valid * Cancel timed scope when credentials are valid * Add UserDefaultFailedLoginDuration and UserMinimumFailedLoginDuration settings --- .../Security/BackOfficeController.cs | 148 +++++++++------- .../Configuration/Models/SecuritySettings.cs | 27 +++ src/Umbraco.Core/TimedScope.cs | 162 ++++++++++++++++++ 3 files changed, 276 insertions(+), 61 deletions(-) create mode 100644 src/Umbraco.Core/TimedScope.cs diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs index fd6f67927c..3e71b4cc31 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs @@ -34,6 +34,8 @@ namespace Umbraco.Cms.Api.Management.Controllers.Security; [ApiExplorerSettings(IgnoreApi = true)] public class BackOfficeController : SecurityControllerBase { + private static long? _loginDurationAverage; + private readonly IHttpContextAccessor _httpContextAccessor; private readonly IBackOfficeSignInManager _backOfficeSignInManager; private readonly IBackOfficeUserManager _backOfficeUserManager; @@ -75,45 +77,65 @@ public class BackOfficeController : SecurityControllerBase [Authorize(Policy = AuthorizationPolicies.DenyLocalLoginIfConfigured)] public async Task Login(CancellationToken cancellationToken, LoginRequestModel model) { - IdentitySignInResult result = await _backOfficeSignInManager.PasswordSignInAsync( - model.Username, model.Password, true, true); + // Start a timed scope to ensure failed responses return is a consistent time + var loginDuration = Math.Max(_loginDurationAverage ?? _securitySettings.Value.UserDefaultFailedLoginDurationInMilliseconds, _securitySettings.Value.UserMinimumFailedLoginDurationInMilliseconds); + await using var timedScope = new TimedScope(loginDuration, cancellationToken); - if (result.IsNotAllowed) + IdentitySignInResult result = await _backOfficeSignInManager.PasswordSignInAsync(model.Username, model.Password, true, true); + if (result.Succeeded is false) { - return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder() - .WithTitle("User is not allowed") - .WithDetail("The operation is not allowed on the user") - .Build()); - } - - if (result.IsLockedOut) - { - return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder() - .WithTitle("User is locked") - .WithDetail("The user is locked, and need to be unlocked before more login attempts can be executed.") - .Build()); - } - - if(result.RequiresTwoFactor) - { - string? twofactorView = _backOfficeTwoFactorOptions.GetTwoFactorView(model.Username); - BackOfficeIdentityUser? attemptingUser = await _backOfficeUserManager.FindByNameAsync(model.Username); - IEnumerable enabledProviders = (await _userTwoFactorLoginService.GetProviderNamesAsync(attemptingUser!.Key)).Result.Where(x=>x.IsEnabledOnUser).Select(x=>x.ProviderName); - return StatusCode(StatusCodes.Status402PaymentRequired, new RequiresTwoFactorResponseModel() + // TODO: The result should include the user and whether the credentials were valid to avoid these additional checks + BackOfficeIdentityUser? user = await _backOfficeUserManager.FindByNameAsync(model.Username.Trim()); // Align with UmbracoSignInManager and trim username! + if (user is not null && + await _backOfficeUserManager.CheckPasswordAsync(user, model.Password)) { - TwoFactorLoginView = twofactorView, - EnabledTwoFactorProviderNames = enabledProviders - }); + // The credentials were correct, so cancel timed scope and provide a more detailed failure response + await timedScope.CancelAsync(); + + if (result.IsNotAllowed) + { + return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder() + .WithTitle("User is not allowed") + .WithDetail("The operation is not allowed on the user") + .Build()); + } + + if (result.IsLockedOut) + { + return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder() + .WithTitle("User is locked") + .WithDetail("The user is locked, and need to be unlocked before more login attempts can be executed.") + .Build()); + } + + if (result.RequiresTwoFactor) + { + string? twofactorView = _backOfficeTwoFactorOptions.GetTwoFactorView(model.Username); + IEnumerable enabledProviders = (await _userTwoFactorLoginService.GetProviderNamesAsync(user.Key)).Result.Where(x => x.IsEnabledOnUser).Select(x => x.ProviderName); + + return StatusCode(StatusCodes.Status402PaymentRequired, new RequiresTwoFactorResponseModel() + { + TwoFactorLoginView = twofactorView, + EnabledTwoFactorProviderNames = enabledProviders + }); + } + } + + return StatusCode(StatusCodes.Status401Unauthorized, new ProblemDetailsBuilder() + .WithTitle("Invalid credentials") + .WithDetail("The provided credentials are invalid. User has not been signed in.") + .Build()); } - if (result.Succeeded) - { - return Ok(); - } - return StatusCode(StatusCodes.Status401Unauthorized, new ProblemDetailsBuilder() - .WithTitle("Invalid credentials") - .WithDetail("The provided credentials are invalid. User has not been signed in.") - .Build()); + // Set initial or update average (successful) login duration + _loginDurationAverage = _loginDurationAverage is long average + ? (average + (long)timedScope.Elapsed.TotalMilliseconds) / 2 + : (long)timedScope.Elapsed.TotalMilliseconds; + + // Cancel the timed scope (we don't want to unnecessarily wait on a successful response) + await timedScope.CancelAsync(); + + return Ok(); } [AllowAnonymous] @@ -171,7 +193,8 @@ public class BackOfficeController : SecurityControllerBase { return BadRequest(new OpenIddictResponse { - Error = "No context found", ErrorDescription = "Unable to obtain context from the current request." + Error = "No context found", + ErrorDescription = "Unable to obtain context from the current request." }); } @@ -180,7 +203,8 @@ public class BackOfficeController : SecurityControllerBase { return BadRequest(new OpenIddictResponse { - Error = "Invalid 'client ID'", ErrorDescription = "The specified 'client_id' is not valid." + Error = "Invalid 'client ID'", + ErrorDescription = "The specified 'client_id' is not valid." }); } @@ -200,7 +224,8 @@ public class BackOfficeController : SecurityControllerBase { return BadRequest(new OpenIddictResponse { - Error = "No context found", ErrorDescription = "Unable to obtain context from the current request." + Error = "No context found", + ErrorDescription = "Unable to obtain context from the current request." }); } @@ -213,35 +238,36 @@ public class BackOfficeController : SecurityControllerBase ? new SignInResult(OpenIddictServerAspNetCoreDefaults.AuthenticationScheme, authenticateResult.Principal) : BadRequest(new OpenIddictResponse { - Error = "Authorization failed", ErrorDescription = "The supplied authorization could not be verified." + Error = "Authorization failed", + ErrorDescription = "The supplied authorization could not be verified." }); } - if (request.IsClientCredentialsGrantType()) + // ensure the client ID and secret are valid (verified by OpenIddict) + if (!request.IsClientCredentialsGrantType()) { - // if we get here, the client ID and secret are valid (verified by OpenIddict) - - // grab the user associated with the client ID - BackOfficeIdentityUser? associatedUser = await _backOfficeUserClientCredentialsManager.FindUserAsync(request.ClientId!); - - if (associatedUser is not null) - { - // log current datetime as last login (this also ensures that the user is not flagged as inactive) - associatedUser.LastLoginDateUtc = DateTime.UtcNow; - await _backOfficeUserManager.UpdateAsync(associatedUser); - - return await SignInBackOfficeUser(associatedUser, request); - } - - // if this happens, the OpenIddict applications have somehow gone out of sync with the Umbraco users - _logger.LogError("The user associated with the client ID ({clientId}) could not be found", request.ClientId); - return BadRequest(new OpenIddictResponse - { - Error = "Authorization failed", ErrorDescription = "The user associated with the supplied 'client_id' could not be found." - }); + throw new InvalidOperationException("The requested grant type is not supported."); } - throw new InvalidOperationException("The requested grant type is not supported."); + // grab the user associated with the client ID + BackOfficeIdentityUser? associatedUser = await _backOfficeUserClientCredentialsManager.FindUserAsync(request.ClientId!); + if (associatedUser is not null) + { + // log current datetime as last login (this also ensures that the user is not flagged as inactive) + associatedUser.LastLoginDateUtc = DateTime.UtcNow; + await _backOfficeUserManager.UpdateAsync(associatedUser); + + return await SignInBackOfficeUser(associatedUser, request); + } + + // if this happens, the OpenIddict applications have somehow gone out of sync with the Umbraco users + _logger.LogError("The user associated with the client ID ({clientId}) could not be found", request.ClientId); + + return BadRequest(new OpenIddictResponse + { + Error = "Authorization failed", + ErrorDescription = "The user associated with the supplied 'client_id' could not be found." + }); } [AllowAnonymous] @@ -489,7 +515,7 @@ public class BackOfficeController : SecurityControllerBase private static IActionResult DefaultChallengeResult() => new ChallengeResult(Constants.Security.BackOfficeAuthenticationType); - private RedirectResult CallbackErrorRedirectWithStatus( string flowType, string status, IEnumerable identityErrors) + private RedirectResult CallbackErrorRedirectWithStatus(string flowType, string status, IEnumerable identityErrors) { var redirectUrl = _securitySettings.Value.BackOfficeHost + "/" + _securitySettings.Value.AuthorizeCallbackErrorPathName.TrimStart('/').AppendQueryStringToUrl( diff --git a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs index b5d81c1ab3..2db42cc3e6 100644 --- a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs +++ b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs @@ -2,6 +2,7 @@ // See LICENSE for more details. using System.ComponentModel; +using System.ComponentModel.DataAnnotations; namespace Umbraco.Cms.Core.Configuration.Models; @@ -25,6 +26,8 @@ public class SecuritySettings internal const int StaticMemberDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60; + private const long StaticUserDefaultFailedLoginDurationInMilliseconds = 1000; + private const long StaticUserMinimumFailedLoginDurationInMilliseconds = 250; internal const string StaticAuthorizeCallbackPathName = "/umbraco/oauth_complete"; internal const string StaticAuthorizeCallbackLogoutPathName = "/umbraco/logout"; internal const string StaticAuthorizeCallbackErrorPathName = "/umbraco/error"; @@ -101,6 +104,30 @@ public class SecuritySettings [DefaultValue(StaticAllowConcurrentLogins)] public bool AllowConcurrentLogins { get; set; } = StaticAllowConcurrentLogins; + /// + /// Gets or sets the default duration (in milliseconds) of failed login attempts. + /// + /// + /// The default duration (in milliseconds) of failed login attempts. + /// + /// + /// The user login endpoint ensures that failed login attempts take at least as long as the average successful login. + /// However, if no successful logins have occurred, this value is used as the default duration. + /// + [Range(0, long.MaxValue)] + [DefaultValue(StaticUserDefaultFailedLoginDurationInMilliseconds)] + public long UserDefaultFailedLoginDurationInMilliseconds { get; set; } = StaticUserDefaultFailedLoginDurationInMilliseconds; + + /// + /// Gets or sets the minimum duration (in milliseconds) of failed login attempts. + /// + /// + /// The minimum duration (in milliseconds) of failed login attempts. + /// + [Range(0, long.MaxValue)] + [DefaultValue(StaticUserMinimumFailedLoginDurationInMilliseconds)] + public long UserMinimumFailedLoginDurationInMilliseconds { get; set; } = StaticUserMinimumFailedLoginDurationInMilliseconds; + /// /// Gets or sets a value of the back-office host URI. Use this when running the back-office client and the Management API on different hosts. Leave empty when running both on the same host. /// diff --git a/src/Umbraco.Core/TimedScope.cs b/src/Umbraco.Core/TimedScope.cs new file mode 100644 index 0000000000..f12f0e90ed --- /dev/null +++ b/src/Umbraco.Core/TimedScope.cs @@ -0,0 +1,162 @@ +namespace Umbraco.Cms.Core; + +/// +/// Makes a code block timed (take at least a certain amount of time). This class cannot be inherited. +/// +public sealed class TimedScope : IDisposable, IAsyncDisposable +{ + private readonly TimeSpan _duration; + private readonly TimeProvider _timeProvider; + private readonly CancellationTokenSource _cancellationTokenSource; + private readonly long _startingTimestamp; + + /// + /// Gets the elapsed time. + /// + /// + /// The elapsed time. + /// + public TimeSpan Elapsed + => _timeProvider.GetElapsedTime(_startingTimestamp); + + /// + /// Gets the remaining time. + /// + /// + /// The remaining time. + /// + public TimeSpan Remaining + => TryGetRemaining(out TimeSpan remaining) ? remaining : TimeSpan.Zero; + + /// + /// Initializes a new instance of the class. + /// + /// The number of milliseconds the scope should at least take. + public TimedScope(long millisecondsDuration) + : this(TimeSpan.FromMilliseconds(millisecondsDuration)) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The number of milliseconds the scope should at least take. + /// The cancellation token. + public TimedScope(long millisecondsDuration, CancellationToken cancellationToken) + : this(TimeSpan.FromMilliseconds(millisecondsDuration), cancellationToken) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The number of milliseconds the scope should at least take. + /// The time provider. + public TimedScope(long millisecondsDuration, TimeProvider timeProvider) + : this(TimeSpan.FromMilliseconds(millisecondsDuration), timeProvider) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The number of milliseconds the scope should at least take. + /// The time provider. + /// The cancellation token. + public TimedScope(long millisecondsDuration, TimeProvider timeProvider, CancellationToken cancellationToken) + : this(TimeSpan.FromMilliseconds(millisecondsDuration), timeProvider, cancellationToken) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The duration the scope should at least take. + public TimedScope(TimeSpan duration) + : this(duration, TimeProvider.System) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The duration the scope should at least take. + /// The time provider. + public TimedScope(TimeSpan duration, TimeProvider timeProvider) + : this(duration, timeProvider, new CancellationTokenSource()) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The duration the scope should at least take. + /// The cancellation token. + public TimedScope(TimeSpan duration, CancellationToken cancellationToken) + : this(duration, TimeProvider.System, cancellationToken) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The duration the scope should at least take. + /// The time provider. + /// The cancellation token. + public TimedScope(TimeSpan duration, TimeProvider timeProvider, CancellationToken cancellationToken) + : this(duration, timeProvider, CancellationTokenSource.CreateLinkedTokenSource(cancellationToken)) + { } + + private TimedScope(TimeSpan duration, TimeProvider timeProvider, CancellationTokenSource cancellationTokenSource) + { + _duration = duration; + _timeProvider = timeProvider; + _cancellationTokenSource = cancellationTokenSource; + _startingTimestamp = timeProvider.GetTimestamp(); + } + + /// + /// Cancels the timed scope. + /// + public void Cancel() + => _cancellationTokenSource.Cancel(); + + /// + /// Cancels the timed scope asynchronously. + /// + public async Task CancelAsync() + => await _cancellationTokenSource.CancelAsync().ConfigureAwait(false); + + /// + /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources. + /// + /// + /// This will block using until the remaining time has elapsed, if not cancelled. + /// + public void Dispose() + { + if (_cancellationTokenSource.IsCancellationRequested is false && + TryGetRemaining(out TimeSpan remaining)) + { + Thread.Sleep(remaining); + } + } + + /// + /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources asynchronously. + /// + /// + /// A task that represents the asynchronous dispose operation. + /// + /// + /// This will delay using until the remaining time has elapsed, if not cancelled. + /// + public async ValueTask DisposeAsync() + { + if (_cancellationTokenSource.IsCancellationRequested is false && + TryGetRemaining(out TimeSpan remaining)) + { + await Task.Delay(remaining, _timeProvider, _cancellationTokenSource.Token).ConfigureAwait(false); + } + } + + private bool TryGetRemaining(out TimeSpan remaining) + { + remaining = _duration.Subtract(Elapsed); + + return remaining > TimeSpan.Zero; + } +}