Skip to content

TimedScope improvements and login duration clean-up #19243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Umbraco.Core/Configuration/Models/SecuritySettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ public class SecuritySettings
/// 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.
/// </remarks>
[Range(0, int.MaxValue)] // TODO (V17): Change property type to short and update maximum range to short.MaxValue
[DefaultValue(StaticUserDefaultFailedLoginDurationInMilliseconds)]
public long UserDefaultFailedLoginDurationInMilliseconds { get; set; } = StaticUserDefaultFailedLoginDurationInMilliseconds;

Expand All @@ -148,6 +149,7 @@ public class SecuritySettings
/// <value>
/// The minimum duration (in milliseconds) of failed login attempts.
/// </value>
[Range(0, int.MaxValue)] // TODO (V17): Change property type to short and update maximum range to short.MaxValue
[DefaultValue(StaticUserMinimumFailedLoginDurationInMilliseconds)]
public long UserMinimumFailedLoginDurationInMilliseconds { get; set; } = StaticUserMinimumFailedLoginDurationInMilliseconds;
}
4 changes: 4 additions & 0 deletions src/Umbraco.Core/TimedScope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ public void Dispose()
{
Thread.Sleep(remaining);
}

_cancellationTokenSource.Dispose();
}

/// <summary>
Expand All @@ -151,6 +153,8 @@ public async ValueTask DisposeAsync()
{
await Task.Delay(remaining, _timeProvider, _cancellationTokenSource.Token).ConfigureAwait(false);
}

_cancellationTokenSource.Dispose();
}

private bool TryGetRemaining(out TimeSpan remaining)
Expand Down
24 changes: 3 additions & 21 deletions src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public class AuthenticationController : UmbracoApiControllerBase
private readonly IUserService _userService;
private readonly WebRoutingSettings _webRoutingSettings;

private const int FailedLoginDurationRandomOffsetInMilliseconds = 100;
private static long? _loginDurationAverage;

// TODO: We need to review all _userManager.Raise calls since many/most should be on the usermanager or signinmanager, very few should be here
Expand Down Expand Up @@ -419,13 +418,12 @@ public async Task<bool> IsAuthenticated()
public async Task<ActionResult<UserDetail?>> PostLogin(LoginModel loginModel)
{
// Start a timed scope to ensure failed responses return is a consistent time
await using var timedScope = new TimedScope(GetLoginDuration(), CancellationToken.None);
var loginDuration = Math.Max(_loginDurationAverage ?? _securitySettings.UserDefaultFailedLoginDurationInMilliseconds, _securitySettings.UserMinimumFailedLoginDurationInMilliseconds);
await using var timedScope = new TimedScope(loginDuration, HttpContext.RequestAborted);

// Sign the user in with username/password, this also gives a chance for developers to
// custom verify the credentials and auto-link user accounts with a custom IBackOfficePasswordChecker
SignInResult result = await _signInManager.PasswordSignInAsync(
loginModel.Username, loginModel.Password, true, true);

SignInResult result = await _signInManager.PasswordSignInAsync(loginModel.Username, loginModel.Password, true, true);
if (result.Succeeded is false)
{
BackOfficeIdentityUser? user = await _userManager.FindByNameAsync(loginModel.Username.Trim());
Expand Down Expand Up @@ -476,22 +474,6 @@ await _userManager.CheckPasswordAsync(user, loginModel.Password))
return GetUserDetail(_userService.GetByUsername(loginModel.Username));
}

private long GetLoginDuration()
{
var loginDuration = Math.Max(_loginDurationAverage ?? _securitySettings.UserDefaultFailedLoginDurationInMilliseconds, _securitySettings.UserMinimumFailedLoginDurationInMilliseconds);
var random = new Random();
var randomDelay = random.Next(-FailedLoginDurationRandomOffsetInMilliseconds, FailedLoginDurationRandomOffsetInMilliseconds);
loginDuration += randomDelay;

// Just be sure we don't get a negative number - possible if someone has configured a very low UserMinimumFailedLoginDurationInMilliseconds value.
if (loginDuration < 0)
{
loginDuration = 0;
}

return loginDuration;
}

/// <summary>
/// Processes a password reset request. Looks for a match on the provided email address
/// and if found sends an email with a link to reset it
Expand Down
Loading