Skip to content

Commit 95bb09d

Browse files
VCST-1628: Make login time the same for registered and unregistered users (#2839)
1 parent d9408cc commit 95bb09d

File tree

4 files changed

+94
-29
lines changed

4 files changed

+94
-29
lines changed
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
using System;
2+
using System.Collections.Concurrent;
3+
using System.Diagnostics;
4+
using System.Threading.Tasks;
5+
6+
namespace VirtoCommerce.Platform.Core.Security;
7+
8+
// This class allows to measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks.
9+
public class DelayedResponse
10+
{
11+
private const int _minDelay = 150; // milliseconds
12+
private static readonly ConcurrentDictionary<string, int> _delaysByName = new();
13+
14+
private readonly string _name;
15+
private readonly Stopwatch _stopwatch;
16+
private readonly Task _failedDelayTask;
17+
private readonly Task _succeededDelayTask;
18+
19+
public static DelayedResponse Create(params string[] nameParts)
20+
{
21+
return new DelayedResponse(string.Join(".", nameParts));
22+
}
23+
24+
public DelayedResponse(string name)
25+
{
26+
_name = name;
27+
_stopwatch = Stopwatch.StartNew();
28+
_delaysByName.TryAdd(name, 0);
29+
var failedDelay = Math.Max(_minDelay, _delaysByName[name]);
30+
_failedDelayTask = Task.Delay(failedDelay);
31+
_succeededDelayTask = Task.Delay(_minDelay);
32+
}
33+
34+
public Task FailAsync()
35+
{
36+
return _failedDelayTask;
37+
}
38+
39+
public Task SucceedAsync()
40+
{
41+
if (_stopwatch.IsRunning)
42+
{
43+
_stopwatch.Stop();
44+
_delaysByName[_name] = (int)_stopwatch.ElapsedMilliseconds;
45+
}
46+
47+
return _succeededDelayTask;
48+
}
49+
}

src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,9 @@ public async Task<ActionResult> Exchange()
117117

118118
if (openIdConnectRequest.IsPasswordGrantType())
119119
{
120+
// Measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks
121+
var delayedResponse = DelayedResponse.Create(nameof(AuthorizationController), nameof(Exchange), "Password");
122+
120123
var user = await _userManager.FindByNameAsync(openIdConnectRequest.Username);
121124

122125
// Allows signin to back office by either username (login) or email if IdentityOptions.User.RequireUniqueEmail is True.
@@ -127,11 +130,13 @@ public async Task<ActionResult> Exchange()
127130

128131
if (user is null)
129132
{
133+
await delayedResponse.FailAsync();
130134
return BadRequest(SecurityErrorDescriber.LoginFailed());
131135
}
132136

133137
if (!_passwordLoginOptions.Enabled && !user.IsAdministrator)
134138
{
139+
await delayedResponse.FailAsync();
135140
return BadRequest(SecurityErrorDescriber.PasswordLoginDisabled());
136141
}
137142

@@ -144,6 +149,7 @@ public async Task<ActionResult> Exchange()
144149
var errors = await requestValidator.ValidateAsync(context);
145150
if (errors.Count > 0)
146151
{
152+
await delayedResponse.FailAsync();
147153
return BadRequest(errors.First());
148154
}
149155
}
@@ -156,6 +162,8 @@ public async Task<ActionResult> Exchange()
156162
await SetLastLoginDate(user);
157163
await _eventPublisher.Publish(new UserLoginEvent(user));
158164

165+
await delayedResponse.SucceedAsync();
166+
159167
return SignIn(ticket.Principal, ticket.Properties, ticket.AuthenticationScheme);
160168
}
161169

src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -97,41 +97,44 @@ public SecurityController(
9797
[AllowAnonymous]
9898
public async Task<ActionResult<SignInResult>> Login([FromBody] LoginRequest request)
9999
{
100-
var userName = request.UserName;
100+
// Measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks
101+
var delayedResponse = DelayedResponse.Create(nameof(SecurityController), nameof(Login));
102+
103+
var user = await UserManager.FindByNameAsync(request.UserName);
101104

102105
// Allows signin to back office by either username (login) or email if IdentityOptions.User.RequireUniqueEmail is True.
103-
if (_identityOptions.User.RequireUniqueEmail)
106+
if (user == null && _identityOptions.User.RequireUniqueEmail)
104107
{
105-
var userByName = await UserManager.FindByNameAsync(userName);
106-
107-
if (userByName == null)
108-
{
109-
var userByEmail = await UserManager.FindByEmailAsync(userName);
110-
if (userByEmail != null)
111-
{
112-
userName = userByEmail.UserName;
113-
}
114-
}
108+
user = await UserManager.FindByEmailAsync(request.UserName);
115109
}
116110

117-
var user = await UserManager.FindByNameAsync(userName);
111+
if (user == null)
112+
{
113+
await delayedResponse.FailAsync();
114+
return Ok(SignInResult.Failed);
115+
}
118116

119117
await _eventPublisher.Publish(new BeforeUserLoginEvent(user));
120118

121-
var loginResult = await _signInManager.PasswordSignInAsync(userName, request.Password, request.RememberMe, true);
119+
var loginResult = await _signInManager.PasswordSignInAsync(user, request.Password, request.RememberMe, lockoutOnFailure: true);
122120

123-
if (loginResult.Succeeded)
121+
if (!loginResult.Succeeded)
124122
{
125-
await SetLastLoginDate(user);
126-
await _eventPublisher.Publish(new UserLoginEvent(user));
123+
await delayedResponse.FailAsync();
124+
return Ok(loginResult);
125+
}
127126

128-
//Do not allow login to admin customers and rejected users
129-
if (await UserManager.IsInRoleAsync(user, PlatformConstants.Security.SystemRoles.Customer))
130-
{
131-
return Ok(SignInResult.NotAllowed);
132-
}
127+
await SetLastLoginDate(user);
128+
await _eventPublisher.Publish(new UserLoginEvent(user));
129+
130+
//Do not allow login to admin customers and rejected users
131+
if (await UserManager.IsInRoleAsync(user, PlatformConstants.Security.SystemRoles.Customer))
132+
{
133+
loginResult = SignInResult.NotAllowed;
133134
}
134135

136+
await delayedResponse.SucceedAsync();
137+
135138
return Ok(loginResult);
136139
}
137140

@@ -611,25 +614,28 @@ public async Task<ActionResult<bool>> ValidatePasswordResetToken(string userId,
611614
[AllowAnonymous]
612615
public async Task<ActionResult> RequestPasswordReset(string loginOrEmail)
613616
{
617+
// Measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks
618+
var delayedResponse = DelayedResponse.Create(nameof(SecurityController), nameof(RequestPasswordReset));
619+
614620
var user = await UserManager.FindByNameAsync(loginOrEmail);
615-
if (user == null)
621+
622+
if (user == null && _identityOptions.User.RequireUniqueEmail)
616623
{
617624
user = await UserManager.FindByEmailAsync(loginOrEmail);
618625
}
619626

620627
// Return 200 to prevent potential user name/email harvesting
621628
if (user == null)
622629
{
630+
await delayedResponse.FailAsync();
623631
return Ok();
624632
}
625633

626634
var nextRequestDate = user.LastPasswordChangeRequestDate + _passwordOptions.RepeatedResetPasswordTimeLimit;
627635
if (nextRequestDate != null && nextRequestDate > DateTime.UtcNow)
628636
{
629-
return Ok(new
630-
{
631-
NextRequestAt = nextRequestDate,
632-
});
637+
await delayedResponse.FailAsync();
638+
return Ok(new { NextRequestAt = nextRequestDate });
633639
}
634640

635641
//Do not permit rejected users and customers
@@ -652,6 +658,8 @@ public async Task<ActionResult> RequestPasswordReset(string loginOrEmail)
652658
await UserManager.UpdateAsync(user);
653659
}
654660

661+
await delayedResponse.SucceedAsync();
662+
655663
return Ok();
656664
}
657665

tests/VirtoCommerce.Platform.Web.Tests/Controllers/Api/SecurityControllerTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ public async Task Login_SignInSuccess()
141141
var user = _fixture.Create<ApplicationUser>();
142142
var request = _fixture.Create<LoginRequest>();
143143
_signInManagerMock
144-
.Setup(x => x.PasswordSignInAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<bool>()))
144+
.Setup(x => x.PasswordSignInAsync(It.IsAny<ApplicationUser>(), It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<bool>()))
145145
.ReturnsAsync(SignInResult.Success);
146146
_userManagerMock
147147
.Setup(x => x.FindByNameAsync(It.IsAny<string>()))
@@ -168,7 +168,7 @@ public async Task Login_UserRoleIsCustomer()
168168
var user = _fixture.Create<ApplicationUser>();
169169
var request = _fixture.Create<LoginRequest>();
170170
_signInManagerMock
171-
.Setup(x => x.PasswordSignInAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<bool>()))
171+
.Setup(x => x.PasswordSignInAsync(It.IsAny<ApplicationUser>(), It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<bool>()))
172172
.ReturnsAsync(SignInResult.Success);
173173
_userManagerMock
174174
.Setup(x => x.FindByNameAsync(It.IsAny<string>()))

0 commit comments

Comments
 (0)