Skip to content

Commit 09c122e

Browse files
mregnimregni
authored
fix(auth): stop on failed Identity results and send error codes instead of English texts (#400)
2 parents c58de4e + 0394b5c commit 09c122e

29 files changed

Lines changed: 555 additions & 97 deletions

‎BoardGameTracker.Api/Infrastructure/GlobalExceptionHandler.cs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,20 +43,22 @@ public async ValueTask<bool> TryHandleAsync(HttpContext httpContext, Exception e
4343
}
4444

4545
httpContext.Response.StatusCode = statusCode;
46+
var problemDetails = new ProblemDetails { Status = statusCode, Title = message };
47+
if (exception is ValidationException { Parameters.Count: > 0 } validation)
48+
{
49+
problemDetails.Extensions["params"] = validation.Parameters;
50+
}
51+
4652
var written = await _problemDetailsService.TryWriteAsync(new ProblemDetailsContext
4753
{
4854
HttpContext = httpContext,
4955
Exception = exception,
50-
ProblemDetails = new ProblemDetails
51-
{
52-
Status = statusCode,
53-
Title = message
54-
}
56+
ProblemDetails = problemDetails
5557
});
5658

5759
if (!written)
5860
{
59-
await httpContext.Response.WriteAsJsonAsync(new ProblemDetails { Status = statusCode, Title = message }, cancellationToken);
61+
await httpContext.Response.WriteAsJsonAsync(problemDetails, cancellationToken);
6062
}
6163

6264
return true;

‎BoardGameTracker.Common/Constants.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,12 @@ public static class Errors
5353
{
5454
public const string UsernameAlreadyExists = "error.auth.username-already-exists";
5555
public const string UsernameRequired = "error.auth.username-required";
56+
public const string InvalidUsername = "error.auth.invalid-username";
57+
public const string EmailAlreadyExists = "error.auth.email-already-exists";
58+
public const string InvalidEmail = "error.auth.invalid-email";
59+
public const string PasswordTooShort = "error.auth.password-too-short";
60+
public const string IncorrectPassword = "error.auth.incorrect-password";
61+
public const string AccountUpdateFailed = "error.auth.account-update-failed";
5662
public const string LastSignInMethod = "error.auth.last-sign-in-method";
5763
public const string InvalidRole = "error.auth.invalid-role";
5864
public const string CannotChangeOidcPassword = "error.auth.cannot-change-oidc-password";
@@ -106,6 +112,10 @@ public static class Errors
106112
public const string ImageTooLarge = "error.image.too-large";
107113
public const string ImageUnsupportedFormat = "error.image.unsupported-format";
108114
public const string ProfileImageUpdateFailed = "error.image.profile-update-failed";
115+
public const string ManualMissing = "error.manual.missing";
116+
public const string ManualEmpty = "error.manual.empty";
117+
public const string ManualNotPdf = "error.manual.not-pdf";
118+
public const string ManualTooLarge = "error.manual.too-large";
109119
public const string InvalidResetToken = "error.auth.invalid-reset-token";
110120
public const string GameAlreadyOnLoan = "error.loan.game-already-on-loan";
111121
public const string LoanDueBeforeStart = "error.loan.due-before-start";

‎BoardGameTracker.Common/Exceptions/ValidationException.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ public class ValidationException : Exception
44
{
55
public Dictionary<string, string[]> Errors { get; }
66

7+
public IReadOnlyDictionary<string, object> Parameters { get; } = new Dictionary<string, object>();
8+
79
public ValidationException(string message)
810
: base(message)
911
{
@@ -13,6 +15,12 @@ public ValidationException(string message)
1315
};
1416
}
1517

18+
public ValidationException(string message, IReadOnlyDictionary<string, object> parameters)
19+
: this(message)
20+
{
21+
Parameters = parameters;
22+
}
23+
1624
public ValidationException(Dictionary<string, string[]> errors)
1725
: base("One or more validation errors occurred.")
1826
{

‎BoardGameTracker.Core/Auth/AuthService.cs‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -241,14 +241,14 @@ public async Task<UserDto> RegisterAsync(RegisterRequest request)
241241

242242
var user = new ApplicationUser(request.Username, request.Email, request.Username);
243243
user.ConfirmEmail();
244-
var result = await _userManager.CreateAsync(user, request.Password);
245-
if (!result.Succeeded)
244+
(await _userManager.CreateAsync(user, request.Password)).ThrowIfFailed();
245+
246+
if (!(await _userManager.AddToRoleAsync(user, role)).Succeeded)
246247
{
247-
throw new ValidationException(string.Join(", ", result.Errors.Select(e => e.Description)));
248+
await _userManager.DeleteAsync(user);
249+
throw new DomainException(Constants.Errors.AccountUpdateFailed);
248250
}
249251

250-
await _userManager.AddToRoleAsync(user, role);
251-
252252
if (request.CreatePlayer)
253253
{
254254
var player = await _playerService.Create(new CreatePlayerCommand
@@ -257,13 +257,13 @@ public async Task<UserDto> RegisterAsync(RegisterRequest request)
257257
Email = request.Email
258258
});
259259
user.LinkPlayer(player.Id);
260-
await _userManager.UpdateAsync(user);
260+
await SaveRegisteredLinkAsync(user);
261261
await _playerAccountImageService.ApplyAccountImageAsync(user);
262262
}
263263
else if (request.PlayerId.HasValue)
264264
{
265265
user.LinkPlayer(request.PlayerId.Value);
266-
await _userManager.UpdateAsync(user);
266+
await SaveRegisteredLinkAsync(user);
267267
await _playerAccountImageService.ApplyAccountImageAsync(user);
268268
}
269269

@@ -273,6 +273,14 @@ public async Task<UserDto> RegisterAsync(RegisterRequest request)
273273
return user.ToDto(roles);
274274
}
275275

276+
private async Task SaveRegisteredLinkAsync(ApplicationUser user)
277+
{
278+
if (!(await _userManager.UpdateAsync(user)).Succeeded)
279+
{
280+
throw new DomainException(Constants.Errors.PlayerLinkUpdateFailed);
281+
}
282+
}
283+
276284
public async Task<ProfileResponse> GetProfileAsync(string userId)
277285
{
278286
var user = await _userManager.FindByIdAsync(userId)
@@ -312,7 +320,7 @@ public async Task<ProfileResponse> UpdateProfileAsync(string userId, UpdateProfi
312320
}
313321
}
314322

315-
await _userManager.UpdateAsync(user);
323+
(await _userManager.UpdateAsync(user)).ThrowIfFailed();
316324
await _playerAccountImageService.RelinkAsync(previousPlayerId, user);
317325

318326
_logger.LogInformation("User {UserId} updated their profile", userId);
@@ -408,7 +416,7 @@ public async Task ChangePasswordAsync(string userId, ChangePasswordRequest reque
408416
if (!result.Succeeded)
409417
{
410418
_logger.LogWarning("Failed password change attempt for user {UserId}", userId);
411-
throw new ValidationException(string.Join(", ", result.Errors.Select(e => e.Description)));
419+
result.ThrowIfFailed();
412420
}
413421

414422
await _tokenService.RevokeAllUserTokensAsync(userId, "Password changed");
@@ -422,11 +430,7 @@ public async Task<ResetPasswordResponse> ResetPasswordAsync(string userId)
422430

423431
var tempPassword = GenerateTempPassword();
424432
var token = await _userManager.GeneratePasswordResetTokenAsync(user);
425-
var result = await _userManager.ResetPasswordAsync(user, token, tempPassword);
426-
if (!result.Succeeded)
427-
{
428-
throw new ValidationException(string.Join(", ", result.Errors.Select(e => e.Description)));
429-
}
433+
(await _userManager.ResetPasswordAsync(user, token, tempPassword)).ThrowIfFailed();
430434

431435
await _tokenService.RevokeAllUserTokensAsync(userId, "Password reset by admin");
432436
_logger.LogInformation("Admin reset password for user {Username}", user.UserName);

‎BoardGameTracker.Core/Auth/IdentityResultExtensions.cs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
using BoardGameTracker.Common;
2+
using BoardGameTracker.Common.Exceptions;
3+
using Microsoft.AspNetCore.Identity;
4+
5+
namespace BoardGameTracker.Core.Auth;
6+
7+
public static class IdentityResultExtensions
8+
{
9+
public static void ThrowIfFailed(this IdentityResult result)
10+
{
11+
if (!result.Succeeded)
12+
{
13+
throw new ValidationException(ErrorCode(result));
14+
}
15+
}
16+
17+
private static string ErrorCode(IdentityResult result) => result.Errors.FirstOrDefault()?.Code switch
18+
{
19+
nameof(IdentityErrorDescriber.PasswordTooShort) => Constants.Errors.PasswordTooShort,
20+
nameof(IdentityErrorDescriber.PasswordMismatch) => Constants.Errors.IncorrectPassword,
21+
nameof(IdentityErrorDescriber.DuplicateUserName) => Constants.Errors.UsernameAlreadyExists,
22+
nameof(IdentityErrorDescriber.InvalidUserName) => Constants.Errors.InvalidUsername,
23+
nameof(IdentityErrorDescriber.DuplicateEmail) => Constants.Errors.EmailAlreadyExists,
24+
nameof(IdentityErrorDescriber.InvalidEmail) => Constants.Errors.InvalidEmail,
25+
_ => Constants.Errors.AccountUpdateFailed
26+
};
27+
}

‎BoardGameTracker.Core/Auth/OidcRoleReconciler.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,10 @@ public async Task ReconcileAsync(ApplicationUser user, OidcProvider provider, Oi
5151
return;
5252
}
5353

54-
if (!current.Contains(target))
54+
if (!current.Contains(target) && !(await _userManager.AddToRoleAsync(user, target)).Succeeded)
5555
{
56-
await _userManager.AddToRoleAsync(user, target);
56+
_logger.LogWarning("Could not give OIDC user {UserId} the role {Role}; keeping {Previous}", user.Id, target, string.Join(",", current));
57+
return;
5758
}
5859

5960
var obsolete = current.Where(r => r != target).ToList();

‎BoardGameTracker.Core/Auth/OidcService.cs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,13 @@ public async Task<OidcLoginResult> CompleteLoginAsync(string code, string? state
197197
}
198198

199199
user.UpdateLastLogin();
200-
await _userManager.UpdateAsync(user);
200+
var saved = await _userManager.UpdateAsync(user);
201+
if (!saved.Succeeded)
202+
{
203+
_logger.LogError("Failed to save user {UserId} at sign-in via {Provider}: {Errors}", user.Id, provider.DisplayName, string.Join(", ", saved.Errors.Select(e => e.Code)));
204+
throw new DomainException(Constants.Errors.OidcFailed);
205+
}
206+
201207
var sessionKey = StartSession(user, provider, identity);
202208
await _context.SaveChangesAsync();
203209
QueuePictureImport(user.Id, provider, identity.Claims);

‎BoardGameTracker.Core/Auth/UserAdminService.cs‎

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -150,14 +150,23 @@ public async Task DeleteAsync(string userId, string currentUserId)
150150
var user = await _userManager.FindByIdAsync(userId)
151151
?? throw new EntityNotFoundException(nameof(ApplicationUser), userId);
152152

153-
var admins = await _userManager.GetUsersInRoleAsync(Constants.AuthRoles.Admin);
154-
if (admins.Count == 1 && admins[0].Id == userId)
153+
await AdminRoleGate.Lock.WaitAsync();
154+
try
155155
{
156-
throw new DomainException(Constants.Errors.CannotDeleteLastAdmin);
156+
var admins = await _userManager.GetUsersInRoleAsync(Constants.AuthRoles.Admin);
157+
if (admins.Any(a => a.Id == userId) && !admins.Any(a => a.Id != userId && !a.IsDisabled))
158+
{
159+
throw new DomainException(Constants.Errors.CannotDeleteLastAdmin);
160+
}
161+
162+
(await _userManager.DeleteAsync(user)).ThrowIfFailed();
163+
}
164+
finally
165+
{
166+
AdminRoleGate.Lock.Release();
157167
}
158168

159169
var linkedPlayerId = user.PlayerId;
160-
await _userManager.DeleteAsync(user);
161170
_accessCheck.Forget(userId);
162171
_imageService.DeleteImage(user.ProfileImage);
163172
await _playerAccountImageService.ReleaseAsync(linkedPlayerId);
@@ -190,8 +199,7 @@ public async Task<UserDto> UpdateRoleAsync(string userId, string role, string cu
190199
_logger.LogInformation("Admin {AdminId} updated role for user {UserId} ({Username}) to {Role}",
191200
currentUserId, userId, user.UserName, role);
192201

193-
var updatedRoles = await _userManager.GetRolesAsync(user);
194-
return user.ToDto(updatedRoles);
202+
return await ToDtoAsync(user);
195203
}
196204

197205
public async Task<UserDto> UpdateUserAsync(string userId, string username, string? email, string role, int? playerId, string currentUserId)
@@ -214,19 +222,12 @@ public async Task<UserDto> UpdateUserAsync(string userId, string username, strin
214222
{
215223
await EnsureNotLastAdminAsync(userId, role, currentUserId);
216224

217-
if (!string.Equals(user.UserName, username, StringComparison.OrdinalIgnoreCase))
225+
var renamed = !string.Equals(user.UserName, username, StringComparison.OrdinalIgnoreCase);
226+
if (renamed && await _userManager.FindByNameAsync(username) != null)
218227
{
219-
var existingUser = await _userManager.FindByNameAsync(username);
220-
if (existingUser != null)
221-
{
222-
throw new ValidationException(Constants.Errors.UsernameAlreadyExists);
223-
}
224-
225-
await _userManager.SetUserNameAsync(user, username);
228+
throw new ValidationException(Constants.Errors.UsernameAlreadyExists);
226229
}
227230

228-
user.UpdateEmail(email, user.EmailConfirmed || !string.Equals(user.Email, email, StringComparison.OrdinalIgnoreCase));
229-
230231
var previousPlayerId = user.PlayerId;
231232
if (playerId != user.PlayerId)
232233
{
@@ -241,7 +242,13 @@ public async Task<UserDto> UpdateUserAsync(string userId, string username, strin
241242
}
242243
}
243244

244-
await _userManager.UpdateAsync(user);
245+
if (renamed)
246+
{
247+
user.UserName = username;
248+
}
249+
250+
user.UpdateEmail(email, user.EmailConfirmed || !string.Equals(user.Email, email, StringComparison.OrdinalIgnoreCase));
251+
(await _userManager.UpdateAsync(user)).ThrowIfFailed();
245252
await _playerAccountImageService.RelinkAsync(previousPlayerId, user);
246253

247254
await ReplaceRoleAsync(user, role);
@@ -254,8 +261,7 @@ public async Task<UserDto> UpdateUserAsync(string userId, string username, strin
254261
_logger.LogInformation("Admin {AdminId} updated user {UserId} (username: {Username}, role: {Role})",
255262
currentUserId, userId, username, role);
256263

257-
var updatedRoles = await _userManager.GetRolesAsync(user);
258-
return user.ToDto(updatedRoles);
264+
return await ToDtoAsync(user);
259265
}
260266

261267
public async Task<UserDto> DisableAsync(string userId, string currentUserId)
@@ -280,8 +286,8 @@ public async Task<UserDto> DisableAsync(string userId, string currentUserId)
280286
}
281287
}
282288

283-
await _userManager.SetLockoutEnabledAsync(user, true);
284-
await _userManager.SetLockoutEndDateAsync(user, DateTimeOffset.MaxValue);
289+
(await _userManager.SetLockoutEnabledAsync(user, true)).ThrowIfFailed();
290+
(await _userManager.SetLockoutEndDateAsync(user, DateTimeOffset.MaxValue)).ThrowIfFailed();
285291
_accessCheck.Forget(userId);
286292
}
287293
finally
@@ -306,7 +312,7 @@ public async Task<UserDto> EnableAsync(string userId, string currentUserId)
306312
var user = await _userManager.FindByIdAsync(userId)
307313
?? throw new EntityNotFoundException(nameof(ApplicationUser), userId);
308314

309-
await _userManager.SetLockoutEndDateAsync(user, null);
315+
(await _userManager.SetLockoutEndDateAsync(user, null)).ThrowIfFailed();
310316
_accessCheck.Forget(userId);
311317

312318
_logger.LogInformation("Admin {AdminId} enabled user {UserId} ({Username})", currentUserId, userId, user.UserName);
@@ -357,7 +363,7 @@ public async Task<UserDto> ManageRoleByIdentityProviderAsync(string userId, stri
357363
}
358364

359365
user.ManageRoleByIdentityProvider();
360-
await _userManager.UpdateAsync(user);
366+
(await _userManager.UpdateAsync(user)).ThrowIfFailed();
361367

362368
_logger.LogInformation("Admin {AdminId} handed the role of user {UserId} back to the identity provider", currentUserId, userId);
363369
return await ToDtoAsync(user);
@@ -406,13 +412,13 @@ private async Task ReplaceRoleAsync(ApplicationUser user, string role)
406412

407413
if (!currentRoles.Contains(role))
408414
{
409-
await _userManager.AddToRoleAsync(user, role);
415+
(await _userManager.AddToRoleAsync(user, role)).ThrowIfFailed();
410416
}
411417

412418
var obsoleteRoles = currentRoles.Where(r => r != role).ToList();
413419
if (obsoleteRoles.Count > 0)
414420
{
415-
await _userManager.RemoveFromRolesAsync(user, obsoleteRoles);
421+
(await _userManager.RemoveFromRolesAsync(user, obsoleteRoles)).ThrowIfFailed();
416422
}
417423
}
418424
}

‎BoardGameTracker.Core/Manuals/ManualService.cs‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using BoardGameTracker.Common;
12
using BoardGameTracker.Common.DTOs;
23
using BoardGameTracker.Common.Entities;
34
using BoardGameTracker.Common.Exceptions;
@@ -79,7 +80,7 @@ public async Task<List<Manual>> UploadManuals(int gameId, IReadOnlyList<IFormFil
7980

8081
if (files.Count == 0)
8182
{
82-
throw new ValidationException("Files", "No files were provided.");
83+
throw new ValidationException(Constants.Errors.ManualMissing);
8384
}
8485

8586
foreach (var file in files)
@@ -256,27 +257,33 @@ private static void ValidateFile(IFormFile file)
256257
{
257258
if (file.Length == 0)
258259
{
259-
throw new ValidationException("Files", $"'{file.FileName}' is empty.");
260+
throw new ValidationException(Constants.Errors.ManualEmpty, FileParameters(file));
260261
}
261262

262263
var isPdf = string.Equals(file.ContentType, PdfContentType, StringComparison.OrdinalIgnoreCase)
263264
&& string.Equals(Path.GetExtension(file.FileName), PdfExtension, StringComparison.OrdinalIgnoreCase);
264265
if (!isPdf)
265266
{
266-
throw new ValidationException("Files", $"'{file.FileName}' is not a PDF file.");
267+
throw new ValidationException(Constants.Errors.ManualNotPdf, FileParameters(file));
267268
}
268269

269270
if (file.Length > MaxManualBytes)
270271
{
271-
throw new ValidationException("Files", $"'{file.FileName}' exceeds the maximum size of {MaxManualBytes / (1024 * 1024)} MB.");
272+
throw new ValidationException(Constants.Errors.ManualTooLarge, new Dictionary<string, object>
273+
{
274+
["fileName"] = file.FileName,
275+
["maxSize"] = MaxManualBytes / (1024 * 1024)
276+
});
272277
}
273278

274279
if (!HasPdfSignature(file))
275280
{
276-
throw new ValidationException("Files", $"'{file.FileName}' is not a PDF file.");
281+
throw new ValidationException(Constants.Errors.ManualNotPdf, FileParameters(file));
277282
}
278283
}
279284

285+
private static Dictionary<string, object> FileParameters(IFormFile file) => new() { ["fileName"] = file.FileName };
286+
280287
private static bool HasPdfSignature(IFormFile file)
281288
{
282289
using var stream = file.OpenReadStream();

0 commit comments

Comments
 (0)