From fb34464199415c86048250694524b2ce3b9b1dc6 Mon Sep 17 00:00:00 2001 From: sam Date: Sun, 14 Jul 2024 16:44:41 +0200 Subject: [PATCH] feat(backend): improve bad request errors --- .../Authentication/DiscordAuthController.cs | 2 +- .../Controllers/MembersController.cs | 2 +- .../Controllers/UsersController.cs | 17 +++-- Foxnouns.Backend/ExpectedError.cs | 61 +++++++++++++++- Foxnouns.Backend/Services/AuthService.cs | 4 +- Foxnouns.Backend/Utils/ValidationUtils.cs | 73 +++++++++++-------- 6 files changed, 114 insertions(+), 45 deletions(-) diff --git a/Foxnouns.Backend/Controllers/Authentication/DiscordAuthController.cs b/Foxnouns.Backend/Controllers/Authentication/DiscordAuthController.cs index 35049ae..577231c 100644 --- a/Foxnouns.Backend/Controllers/Authentication/DiscordAuthController.cs +++ b/Foxnouns.Backend/Controllers/Authentication/DiscordAuthController.cs @@ -48,7 +48,7 @@ public class DiscordAuthController( public async Task RegisterAsync([FromBody] AuthController.OauthRegisterRequest req) { var remoteUser = await keyCacheSvc.GetKeyAsync($"discord:{req.Ticket}"); - if (remoteUser == null) throw new ApiError.BadRequest("Invalid ticket", "ticket"); + if (remoteUser == null) throw new ApiError.BadRequest("Invalid ticket", "ticket", req.Ticket); if (await db.AuthMethods.AnyAsync(a => a.AuthType == AuthType.Discord && a.RemoteId == remoteUser.Id)) { logger.Error("Discord user {Id} has valid ticket but is already linked to an existing account", diff --git a/Foxnouns.Backend/Controllers/MembersController.cs b/Foxnouns.Backend/Controllers/MembersController.cs index 1e30549..c189937 100644 --- a/Foxnouns.Backend/Controllers/MembersController.cs +++ b/Foxnouns.Backend/Controllers/MembersController.cs @@ -45,7 +45,7 @@ public class MembersController( if (await db.Members.AnyAsync(m => m.UserId == CurrentUser!.Id && m.Name.ToLower() == req.Name.ToLower())) #pragma warning restore CA1862 { - throw new ApiError.BadRequest("A member with that name already exists", "name"); + throw new ApiError.BadRequest("A member with that name already exists", "name", req.Name); } var member = new Member diff --git a/Foxnouns.Backend/Controllers/UsersController.cs b/Foxnouns.Backend/Controllers/UsersController.cs index e1b5702..ab9ad0e 100644 --- a/Foxnouns.Backend/Controllers/UsersController.cs +++ b/Foxnouns.Backend/Controllers/UsersController.cs @@ -38,30 +38,35 @@ public class UsersController(DatabaseContext db, UserRendererService userRendere { await using var tx = await db.Database.BeginTransactionAsync(); var user = await db.Users.FirstAsync(u => u.Id == CurrentUser!.Id); + var errors = new List<(string, ValidationError?)>(); if (req.Username != null && req.Username != user.Username) { - ValidationUtils.ValidateUsername(req.Username); + errors.Add(("username", ValidationUtils.ValidateUsername(req.Username))); user.Username = req.Username; } if (req.HasProperty(nameof(req.DisplayName))) { - ValidationUtils.ValidateDisplayName(req.DisplayName); + errors.Add(("display_name", ValidationUtils.ValidateDisplayName(req.DisplayName))); user.DisplayName = req.DisplayName; } if (req.HasProperty(nameof(req.Bio))) { - ValidationUtils.ValidateBio(req.Bio); + errors.Add(("bio", ValidationUtils.ValidateBio(req.Bio))); user.Bio = req.Bio; } if (req.HasProperty(nameof(req.Avatar))) - { - ValidationUtils.ValidateAvatar(req.Avatar); + errors.Add(("avatar", ValidationUtils.ValidateAvatar(req.Avatar))); + + ValidationUtils.Validate(errors); + // This is fired off regardless of whether the transaction is committed + // (atomic operations are hard when combined with background jobs) + // so it's in a separate block to the validation above. + if (req.HasProperty(nameof(req.Avatar))) AvatarUpdateJob.QueueUpdateUserAvatar(CurrentUser!.Id, req.Avatar); - } await db.SaveChangesAsync(); await tx.CommitAsync(); diff --git a/Foxnouns.Backend/ExpectedError.cs b/Foxnouns.Backend/ExpectedError.cs index 7dda1ad..6e39af7 100644 --- a/Foxnouns.Backend/ExpectedError.cs +++ b/Foxnouns.Backend/ExpectedError.cs @@ -2,6 +2,7 @@ using System.Collections.ObjectModel; using System.Net; using Foxnouns.Backend.Middleware; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Newtonsoft.Json; using Newtonsoft.Json.Linq; namespace Foxnouns.Backend; @@ -31,11 +32,12 @@ public class ApiError(string message, HttpStatusCode? statusCode = null, ErrorCo public readonly string[] Scopes = scopes?.ToArray() ?? []; } - public class BadRequest(string message, IReadOnlyDictionary? errors = null) + public class BadRequest(string message, IReadOnlyDictionary>? errors = null) : ApiError(message, statusCode: HttpStatusCode.BadRequest) { - public BadRequest(string message, string field) : this(message, - new Dictionary { { field, message } }) + public BadRequest(string message, string field, object actualValue) : this("Error validating input", + new Dictionary> + { { field, [ValidationError.GenericValidationError(message, actualValue)] } }) { } @@ -55,7 +57,7 @@ public class ApiError(string message, HttpStatusCode? statusCode = null, ErrorCo var errorObj = new JObject { { "key", error.Key }, - { "errors", new JArray(new JObject { { "message", error.Value } }) } + { "errors", JArray.FromObject(error.Value) } }; a.Add(errorObj); } @@ -116,4 +118,55 @@ public enum ErrorCode GenericApiError, UserNotFound, MemberNotFound, +} + +public class ValidationError +{ + public required string Message { get; init; } + + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] + public int? MinLength { get; init; } + + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] + public int? MaxLength { get; init; } + + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] + public int? ActualLength { get; init; } + + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] + public IEnumerable? AllowedValues { get; init; } + + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] + public object? ActualValue { get; init; } + + public static ValidationError LengthError(string message, int minLength, int maxLength, int actualLength) + { + return new ValidationError + { + Message = message, + MinLength = minLength, + MaxLength = maxLength, + ActualLength = actualLength + }; + } + + public static ValidationError DisallowedValueError(string message, IEnumerable allowedValues, + object actualValue) + { + return new ValidationError + { + Message = message, + AllowedValues = allowedValues, + ActualValue = actualValue + }; + } + + public static ValidationError GenericValidationError(string message, object? actualValue) + { + return new ValidationError + { + Message = message, + ActualValue = actualValue + }; + } } \ No newline at end of file diff --git a/Foxnouns.Backend/Services/AuthService.cs b/Foxnouns.Backend/Services/AuthService.cs index 5365480..c75f926 100644 --- a/Foxnouns.Backend/Services/AuthService.cs +++ b/Foxnouns.Backend/Services/AuthService.cs @@ -47,7 +47,7 @@ public class AuthService(IClock clock, DatabaseContext db, ISnowflakeGenerator s AssertValidAuthType(authType, instance); if (await db.Users.AnyAsync(u => u.Username == username)) - throw new ApiError.BadRequest("Username is already taken", "username"); + throw new ApiError.BadRequest("Username is already taken", "username", username); var user = new User { @@ -124,7 +124,7 @@ public class AuthService(IClock clock, DatabaseContext db, ISnowflakeGenerator s public (string, Token) GenerateToken(User user, Application application, string[] scopes, Instant expires) { if (!AuthUtils.ValidateScopes(application, scopes)) - throw new ApiError.BadRequest("Invalid scopes requested for this token", "scopes"); + throw new ApiError.BadRequest("Invalid scopes requested for this token", "scopes", scopes); var (token, hash) = GenerateToken(); return (token, new Token diff --git a/Foxnouns.Backend/Utils/ValidationUtils.cs b/Foxnouns.Backend/Utils/ValidationUtils.cs index 182f1bc..a00d4d1 100644 --- a/Foxnouns.Backend/Utils/ValidationUtils.cs +++ b/Foxnouns.Backend/Utils/ValidationUtils.cs @@ -23,54 +23,65 @@ public static class ValidationUtils "pronouns.cc", "pronounscc" ]; - - /// - /// Validates whether a username is valid. If it is not valid, throws . - /// This does not check if the username is already taken. - /// - public static void ValidateUsername(string username) + + public static ValidationError? ValidateUsername(string username) { if (!UsernameRegex.IsMatch(username)) - throw username.Length switch + return username.Length switch { - < 2 => new ApiError.BadRequest("Username is too short", "username"), - > 40 => new ApiError.BadRequest("Username is too long", "username"), - _ => new ApiError.BadRequest( - "Username is invalid, can only contain alphanumeric characters, dashes, underscores, and periods", - "username") + < 2 => ValidationError.LengthError("Username is too short", 2, 40, username.Length), + > 40 => ValidationError.LengthError("Username is too long", 2, 40, username.Length), + _ => ValidationError.GenericValidationError( + "Username is invalid, can only contain alphanumeric characters, dashes, underscores, and periods", username) }; if (InvalidUsernames.Any(u => string.Equals(u, username, StringComparison.InvariantCultureIgnoreCase))) - throw new ApiError.BadRequest("Username is not allowed", "username"); + return ValidationError.GenericValidationError("Username is not allowed", username); + return null; } - public static void ValidateDisplayName(string? displayName) + public static void Validate(IEnumerable<(string, ValidationError?)> errors) { - if (displayName == null) return; - switch (displayName.Length) + errors = errors.Where(e => e.Item2 != null).ToList(); + if (!errors.Any()) return; + + var errorDict = new Dictionary>(); + foreach (var error in errors) { - case 0: - throw new ApiError.BadRequest("Display name is too short", "display_name"); - case > 100: - throw new ApiError.BadRequest("Display name is too long", "display_name"); + if (errorDict.TryGetValue(error.Item1, out var value)) errorDict[error.Item1] = value.Append(error.Item2!); + errorDict.Add(error.Item1, [error.Item2!]); } + + throw new ApiError.BadRequest("Error validating input", errorDict); } - public static void ValidateBio(string? bio) + public static ValidationError? ValidateDisplayName(string? displayName) { - if (bio == null) return; - switch (bio.Length) + return displayName?.Length switch { - case 0: - throw new ApiError.BadRequest("Bio is too short", "bio"); - case > 1024: - throw new ApiError.BadRequest("Bio is too long", "bio"); - } + 0 => ValidationError.LengthError("Display name is too short", 1, 100, displayName.Length), + > 100 => ValidationError.LengthError("Display name is too long", 1, 100, displayName.Length), + _ => null + }; } - public static void ValidateAvatar(string? avatar) + public static ValidationError? ValidateBio(string? bio) { - if (avatar == null) return; - if (avatar.Length > 1_500_000) throw new ApiError.BadRequest("Avatar is too big", "avatar"); + return bio?.Length switch + { + 0 => ValidationError.LengthError("Bio is too short", 1, 1024, bio.Length), + > 1024 => ValidationError.LengthError("Bio is too long", 1, 1024, bio.Length), + _ => null + }; + } + + public static ValidationError? ValidateAvatar(string? avatar) + { + return avatar?.Length switch + { + 0 => ValidationError.GenericValidationError("Avatar cannot be empty", null), + > 1_500_00 => ValidationError.GenericValidationError("Avatar is too large", null), + _ => null + }; } } \ No newline at end of file