Skip to content

Commit 559c6c9

Browse files
ronaldbarendseZeegaan
ronaldbarendse
authored and
Zeegaan
committed
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
1 parent d4f8754 commit 559c6c9

File tree

3 files changed

+272
-57
lines changed

3 files changed

+272
-57
lines changed

‎src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs

Lines changed: 83 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ namespace Umbraco.Cms.Api.Management.Controllers.Security;
3434
[ApiExplorerSettings(IgnoreApi = true)]
3535
public class BackOfficeController : SecurityControllerBase
3636
{
37+
private static long? _loginDurationAverage;
38+
3739
private readonly IHttpContextAccessor _httpContextAccessor;
3840
private readonly IBackOfficeSignInManager _backOfficeSignInManager;
3941
private readonly IBackOfficeUserManager _backOfficeUserManager;
@@ -75,45 +77,65 @@ public BackOfficeController(
7577
[Authorize(Policy = AuthorizationPolicies.DenyLocalLoginIfConfigured)]
7678
public async Task<IActionResult> Login(CancellationToken cancellationToken, LoginRequestModel model)
7779
{
78-
IdentitySignInResult result = await _backOfficeSignInManager.PasswordSignInAsync(
79-
model.Username, model.Password, true, true);
80+
// Start a timed scope to ensure failed responses return is a consistent time
81+
var loginDuration = Math.Max(_loginDurationAverage ?? _securitySettings.Value.UserDefaultFailedLoginDurationInMilliseconds, _securitySettings.Value.UserMinimumFailedLoginDurationInMilliseconds);
82+
await using var timedScope = new TimedScope(loginDuration, cancellationToken);
8083

81-
if (result.IsNotAllowed)
84+
IdentitySignInResult result = await _backOfficeSignInManager.PasswordSignInAsync(model.Username, model.Password, true, true);
85+
if (result.Succeeded is false)
8286
{
83-
return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder()
84-
.WithTitle("User is not allowed")
85-
.WithDetail("The operation is not allowed on the user")
86-
.Build());
87-
}
87+
// TODO: The result should include the user and whether the credentials were valid to avoid these additional checks
88+
BackOfficeIdentityUser? user = await _backOfficeUserManager.FindByNameAsync(model.Username.Trim()); // Align with UmbracoSignInManager and trim username!
89+
if (user is not null &&
90+
await _backOfficeUserManager.CheckPasswordAsync(user, model.Password))
91+
{
92+
// The credentials were correct, so cancel timed scope and provide a more detailed failure response
93+
await timedScope.CancelAsync();
8894

89-
if (result.IsLockedOut)
90-
{
91-
return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder()
92-
.WithTitle("User is locked")
93-
.WithDetail("The user is locked, and need to be unlocked before more login attempts can be executed.")
95+
if (result.IsNotAllowed)
96+
{
97+
return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder()
98+
.WithTitle("User is not allowed")
99+
.WithDetail("The operation is not allowed on the user")
100+
.Build());
101+
}
102+
103+
if (result.IsLockedOut)
104+
{
105+
return StatusCode(StatusCodes.Status403Forbidden, new ProblemDetailsBuilder()
106+
.WithTitle("User is locked")
107+
.WithDetail("The user is locked, and need to be unlocked before more login attempts can be executed.")
108+
.Build());
109+
}
110+
111+
if (result.RequiresTwoFactor)
112+
{
113+
string? twofactorView = _backOfficeTwoFactorOptions.GetTwoFactorView(model.Username);
114+
IEnumerable<string> enabledProviders = (await _userTwoFactorLoginService.GetProviderNamesAsync(user.Key)).Result.Where(x => x.IsEnabledOnUser).Select(x => x.ProviderName);
115+
116+
return StatusCode(StatusCodes.Status402PaymentRequired, new RequiresTwoFactorResponseModel()
117+
{
118+
TwoFactorLoginView = twofactorView,
119+
EnabledTwoFactorProviderNames = enabledProviders
120+
});
121+
}
122+
}
123+
124+
return StatusCode(StatusCodes.Status401Unauthorized, new ProblemDetailsBuilder()
125+
.WithTitle("Invalid credentials")
126+
.WithDetail("The provided credentials are invalid. User has not been signed in.")
94127
.Build());
95128
}
96129

97-
if(result.RequiresTwoFactor)
98-
{
99-
string? twofactorView = _backOfficeTwoFactorOptions.GetTwoFactorView(model.Username);
100-
BackOfficeIdentityUser? attemptingUser = await _backOfficeUserManager.FindByNameAsync(model.Username);
101-
IEnumerable<string> enabledProviders = (await _userTwoFactorLoginService.GetProviderNamesAsync(attemptingUser!.Key)).Result.Where(x=>x.IsEnabledOnUser).Select(x=>x.ProviderName);
102-
return StatusCode(StatusCodes.Status402PaymentRequired, new RequiresTwoFactorResponseModel()
103-
{
104-
TwoFactorLoginView = twofactorView,
105-
EnabledTwoFactorProviderNames = enabledProviders
106-
});
107-
}
130+
// Set initial or update average (successful) login duration
131+
_loginDurationAverage = _loginDurationAverage is long average
132+
? (average + (long)timedScope.Elapsed.TotalMilliseconds) / 2
133+
: (long)timedScope.Elapsed.TotalMilliseconds;
108134

109-
if (result.Succeeded)
110-
{
111-
return Ok();
112-
}
113-
return StatusCode(StatusCodes.Status401Unauthorized, new ProblemDetailsBuilder()
114-
.WithTitle("Invalid credentials")
115-
.WithDetail("The provided credentials are invalid. User has not been signed in.")
116-
.Build());
135+
// Cancel the timed scope (we don't want to unnecessarily wait on a successful response)
136+
await timedScope.CancelAsync();
137+
138+
return Ok();
117139
}
118140

119141
[AllowAnonymous]
@@ -171,7 +193,8 @@ public async Task<IActionResult> Authorize(CancellationToken cancellationToken)
171193
{
172194
return BadRequest(new OpenIddictResponse
173195
{
174-
Error = "No context found", ErrorDescription = "Unable to obtain context from the current request."
196+
Error = "No context found",
197+
ErrorDescription = "Unable to obtain context from the current request."
175198
});
176199
}
177200

@@ -180,7 +203,8 @@ public async Task<IActionResult> Authorize(CancellationToken cancellationToken)
180203
{
181204
return BadRequest(new OpenIddictResponse
182205
{
183-
Error = "Invalid 'client ID'", ErrorDescription = "The specified 'client_id' is not valid."
206+
Error = "Invalid 'client ID'",
207+
ErrorDescription = "The specified 'client_id' is not valid."
184208
});
185209
}
186210

@@ -200,7 +224,8 @@ public async Task<IActionResult> Token()
200224
{
201225
return BadRequest(new OpenIddictResponse
202226
{
203-
Error = "No context found", ErrorDescription = "Unable to obtain context from the current request."
227+
Error = "No context found",
228+
ErrorDescription = "Unable to obtain context from the current request."
204229
});
205230
}
206231

@@ -213,35 +238,36 @@ public async Task<IActionResult> Token()
213238
? new SignInResult(OpenIddictServerAspNetCoreDefaults.AuthenticationScheme, authenticateResult.Principal)
214239
: BadRequest(new OpenIddictResponse
215240
{
216-
Error = "Authorization failed", ErrorDescription = "The supplied authorization could not be verified."
241+
Error = "Authorization failed",
242+
ErrorDescription = "The supplied authorization could not be verified."
217243
});
218244
}
219245

220-
if (request.IsClientCredentialsGrantType())
246+
// ensure the client ID and secret are valid (verified by OpenIddict)
247+
if (!request.IsClientCredentialsGrantType())
221248
{
222-
// if we get here, the client ID and secret are valid (verified by OpenIddict)
223-
224-
// grab the user associated with the client ID
225-
BackOfficeIdentityUser? associatedUser = await _backOfficeUserClientCredentialsManager.FindUserAsync(request.ClientId!);
226-
227-
if (associatedUser is not null)
228-
{
229-
// log current datetime as last login (this also ensures that the user is not flagged as inactive)
230-
associatedUser.LastLoginDateUtc = DateTime.UtcNow;
231-
await _backOfficeUserManager.UpdateAsync(associatedUser);
249+
throw new InvalidOperationException("The requested grant type is not supported.");
250+
}
232251

233-
return await SignInBackOfficeUser(associatedUser, request);
234-
}
252+
// grab the user associated with the client ID
253+
BackOfficeIdentityUser? associatedUser = await _backOfficeUserClientCredentialsManager.FindUserAsync(request.ClientId!);
254+
if (associatedUser is not null)
255+
{
256+
// log current datetime as last login (this also ensures that the user is not flagged as inactive)
257+
associatedUser.LastLoginDateUtc = DateTime.UtcNow;
258+
await _backOfficeUserManager.UpdateAsync(associatedUser);
235259

236-
// if this happens, the OpenIddict applications have somehow gone out of sync with the Umbraco users
237-
_logger.LogError("The user associated with the client ID ({clientId}) could not be found", request.ClientId);
238-
return BadRequest(new OpenIddictResponse
239-
{
240-
Error = "Authorization failed", ErrorDescription = "The user associated with the supplied 'client_id' could not be found."
241-
});
260+
return await SignInBackOfficeUser(associatedUser, request);
242261
}
243262

244-
throw new InvalidOperationException("The requested grant type is not supported.");
263+
// if this happens, the OpenIddict applications have somehow gone out of sync with the Umbraco users
264+
_logger.LogError("The user associated with the client ID ({clientId}) could not be found", request.ClientId);
265+
266+
return BadRequest(new OpenIddictResponse
267+
{
268+
Error = "Authorization failed",
269+
ErrorDescription = "The user associated with the supplied 'client_id' could not be found."
270+
});
245271
}
246272

247273
[AllowAnonymous]
@@ -489,7 +515,7 @@ private async Task<IActionResult> SignInBackOfficeUser(BackOfficeIdentityUser ba
489515

490516
private static IActionResult DefaultChallengeResult() => new ChallengeResult(Constants.Security.BackOfficeAuthenticationType);
491517

492-
private RedirectResult CallbackErrorRedirectWithStatus( string flowType, string status, IEnumerable<IdentityError> identityErrors)
518+
private RedirectResult CallbackErrorRedirectWithStatus(string flowType, string status, IEnumerable<IdentityError> identityErrors)
493519
{
494520
var redirectUrl = _securitySettings.Value.BackOfficeHost + "/" +
495521
_securitySettings.Value.AuthorizeCallbackErrorPathName.TrimStart('/').AppendQueryStringToUrl(

‎src/Umbraco.Core/Configuration/Models/SecuritySettings.cs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// See LICENSE for more details.
33

44
using System.ComponentModel;
5+
using System.ComponentModel.DataAnnotations;
56

67
namespace Umbraco.Cms.Core.Configuration.Models;
78

@@ -25,6 +26,8 @@ public class SecuritySettings
2526

2627
internal const int StaticMemberDefaultLockoutTimeInMinutes = 30 * 24 * 60;
2728
internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60;
29+
private const long StaticUserDefaultFailedLoginDurationInMilliseconds = 1000;
30+
private const long StaticUserMinimumFailedLoginDurationInMilliseconds = 250;
2831
internal const string StaticAuthorizeCallbackPathName = "/umbraco/oauth_complete";
2932
internal const string StaticAuthorizeCallbackLogoutPathName = "/umbraco/logout";
3033
internal const string StaticAuthorizeCallbackErrorPathName = "/umbraco/error";
@@ -101,6 +104,30 @@ public class SecuritySettings
101104
[DefaultValue(StaticAllowConcurrentLogins)]
102105
public bool AllowConcurrentLogins { get; set; } = StaticAllowConcurrentLogins;
103106

107+
/// <summary>
108+
/// Gets or sets the default duration (in milliseconds) of failed login attempts.
109+
/// </summary>
110+
/// <value>
111+
/// The default duration (in milliseconds) of failed login attempts.
112+
/// </value>
113+
/// <remarks>
114+
/// The user login endpoint ensures that failed login attempts take at least as long as the average successful login.
115+
/// However, if no successful logins have occurred, this value is used as the default duration.
116+
/// </remarks>
117+
[Range(0, long.MaxValue)]
118+
[DefaultValue(StaticUserDefaultFailedLoginDurationInMilliseconds)]
119+
public long UserDefaultFailedLoginDurationInMilliseconds { get; set; } = StaticUserDefaultFailedLoginDurationInMilliseconds;
120+
121+
/// <summary>
122+
/// Gets or sets the minimum duration (in milliseconds) of failed login attempts.
123+
/// </summary>
124+
/// <value>
125+
/// The minimum duration (in milliseconds) of failed login attempts.
126+
/// </value>
127+
[Range(0, long.MaxValue)]
128+
[DefaultValue(StaticUserMinimumFailedLoginDurationInMilliseconds)]
129+
public long UserMinimumFailedLoginDurationInMilliseconds { get; set; } = StaticUserMinimumFailedLoginDurationInMilliseconds;
130+
104131
/// <summary>
105132
/// 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.
106133
/// </summary>

0 commit comments

Comments
 (0)