diff --git a/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs b/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs index f3695ca6a8fd..fcce5a78782e 100644 --- a/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs +++ b/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs @@ -18,11 +18,11 @@ public static BitwardenValidationProblemResult BitwardenValidationProblem(IValid ArgumentNullException.ThrowIfNull(validationError); return TypedResults.BitwardenValidationProblem( - errors: new Dictionary + errors: new Dictionary { { validationError.PropertyName, - [new BitwardenTypedResultsExtensions.ErrorCode(validationError.Type, validationError.Message)] + [new ErrorCode(validationError.Type, validationError.Message)] } }); } diff --git a/src/Api/Utilities/ModelStateValidationFilterAttribute.cs b/src/Api/Utilities/ModelStateValidationFilterAttribute.cs index 3fe4f748fb0b..ff6ea12f05ea 100644 --- a/src/Api/Utilities/ModelStateValidationFilterAttribute.cs +++ b/src/Api/Utilities/ModelStateValidationFilterAttribute.cs @@ -14,15 +14,19 @@ public ModelStateValidationFilterAttribute(bool publicApi) _publicApi = publicApi; } + /// + /// Only the internal API takes the coded document. The public API's error shape is a published contract with + /// its own versioning, so it keeps answering as it always has until that contract is revised deliberately. + /// protected override void OnModelStateInvalid(ActionExecutingContext context) { if (_publicApi) { context.Result = new BadRequestObjectResult(new ErrorResponseModel(context.ModelState)); + return; } - else - { - context.Result = new BadRequestObjectResult(new InternalApi.ErrorResponseModel(context.ModelState)); - } + + context.Result = TryCodedProblem(context) + ?? new BadRequestObjectResult(new InternalApi.ErrorResponseModel(context.ModelState)); } } diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index 3cfd1d7a2adb..46b6b21b651f 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -243,6 +243,7 @@ public static partial class FeatureFlagKeys public const string OrgCipherPushFanout = "pm-35168-org-cipher-push-fanout"; public const string FedRampGovRegion = "fedramp-gov-region"; public const string ManagedDeviceFramework = "pm-27719-managed-device-framework"; + public const string CodedValidationProblems = "coded-validation-problems"; /* Tools Team */ public const string UseSdkPasswordGenerators = "pm-19976-use-sdk-password-generators"; diff --git a/src/HttpExtensions/BitwardenTypedResultsExtensions.cs b/src/HttpExtensions/BitwardenTypedResultsExtensions.cs index 41bb12378582..f12a38ef431c 100644 --- a/src/HttpExtensions/BitwardenTypedResultsExtensions.cs +++ b/src/HttpExtensions/BitwardenTypedResultsExtensions.cs @@ -7,38 +7,88 @@ public static class BitwardenTypedResultsExtensions extension(TypedResults) { /// - /// Produces a 400 Bad Request RFC 7807 problem response, mirroring - /// TypedResults.ValidationProblem but typing each error entry as an array of - /// rather than string[]. + /// Produces an RFC 7807 problem response, mirroring TypedResults.ValidationProblem but typing + /// each error entry as an array of rather than string[]. /// /// WARNING: This is currently experimental and may change in the future. /// /// + /// + /// The response status. Defaults to 400 Bad Request. Pass another 4xx when the failure is carried by this + /// same body but is not bad input — 409 Conflict for a state conflict, say — so a caller that only reads + /// the status still learns something before it reads the codes. + /// + /// + /// Further problem members. A member named errors is dropped: that name belongs to + /// , and a document carrying it twice is not parseable. + /// public static BitwardenValidationProblemResult BitwardenValidationProblem( IDictionary errors, string? detail = null, string? instance = null, string title = "One or more validation errors occurred.", string type = "validation_error", - IDictionary? extensions = null) + IDictionary? extensions = null, + int statusCode = StatusCodes.Status400BadRequest) { ArgumentNullException.ThrowIfNull(errors); - var problemExtensions = extensions is null - ? new Dictionary() - : new Dictionary(extensions); + var problemDetails = new BitwardenValidationProblemDetails + { + Detail = detail, + Instance = instance, + Status = statusCode, + Title = title, + Type = type, + Errors = new Dictionary(errors), + }; - problemExtensions["errors"] = errors; + if (extensions is not null) + { + foreach (var (key, value) in extensions + .Where(extension => extension.Key != BitwardenValidationProblemDetails.ErrorsMember)) + { + problemDetails.Extensions[key] = value; + } + } - return new BitwardenValidationProblemResult(TypedResults.Problem( + return new BitwardenValidationProblemResult(problemDetails); + } + + /// + /// Produces an RFC 7807 problem response from a flat sequence of property/code pairs, grouping them by + /// property so a caller that discovers failures one at a time does not have to build the dictionary itself. + /// + /// WARNING: This is currently experimental and may change in the future. + /// + /// + /// + /// The failures, in the order they should appear under each property. More than one pair may name the same + /// property; they are collected into that property's array rather than overwriting one another. + /// + public static BitwardenValidationProblemResult BitwardenValidationProblem( + IEnumerable<(string PropertyName, ErrorCode Code)> errors, + string? detail = null, + string? instance = null, + string title = "One or more validation errors occurred.", + string type = "validation_error", + IDictionary? extensions = null, + int statusCode = StatusCodes.Status400BadRequest) + { + ArgumentNullException.ThrowIfNull(errors); + + var grouped = errors + .GroupBy(error => error.PropertyName) + .ToDictionary(group => group.Key, group => group.Select(error => error.Code).ToArray()); + + return TypedResults.BitwardenValidationProblem( + errors: grouped, detail: detail, instance: instance, - statusCode: StatusCodes.Status400BadRequest, title: title, type: type, - extensions: problemExtensions)); + extensions: extensions, + statusCode: statusCode); } } - - public record ErrorCode(string Type, string Detail); } diff --git a/src/HttpExtensions/BitwardenValidationProblemDetails.cs b/src/HttpExtensions/BitwardenValidationProblemDetails.cs new file mode 100644 index 000000000000..153bd2147966 --- /dev/null +++ b/src/HttpExtensions/BitwardenValidationProblemDetails.cs @@ -0,0 +1,31 @@ +using System.Text.Json.Serialization; +using Microsoft.AspNetCore.Mvc; + +namespace Bit.HttpExtensions; + +/// +/// The body a writes: the RFC 7807 members plus an +/// errors map keyed by the property that failed, each entry carrying a code the client switches on. +/// +/// +/// Named as a type rather than assembled into so that the document a +/// client parses and the document OpenAPI describes are the same declaration, and neither can drift from the +/// other. +/// +public sealed class BitwardenValidationProblemDetails : ProblemDetails +{ + /// The document member the errors map is written under. + internal const string ErrorsMember = "errors"; + + /// + /// The failures, keyed by the property name as it appeared on the wire. A property that failed several ways + /// carries an entry per failure. + /// + /// + /// Ordered after the inherited members so the code that reads the document meets type and + /// status before the detail of what went wrong, as it does in every other problem response. + /// + [JsonPropertyOrder(100)] + [JsonPropertyName(ErrorsMember)] + public IDictionary Errors { get; init; } = new Dictionary(); +} diff --git a/src/HttpExtensions/BitwardenValidationProblemResult.cs b/src/HttpExtensions/BitwardenValidationProblemResult.cs index 37f84a428cef..040951a1545c 100644 --- a/src/HttpExtensions/BitwardenValidationProblemResult.cs +++ b/src/HttpExtensions/BitwardenValidationProblemResult.cs @@ -1,17 +1,20 @@ -using Microsoft.AspNetCore.Http; +using System.Reflection; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc; namespace Bit.HttpExtensions; /// /// A Bitwarden-flavored RFC 7807 validation problem result. Wraps an inner -/// so we have room to grow — for example, implementing -/// IEndpointMetadataProvider for OpenAPI — without changing the public signature of +/// so we have room to grow without changing the public signature of /// TypedResults.BitwardenValidationProblem. /// public sealed class BitwardenValidationProblemResult : IResult, + IEndpointMetadataProvider, IStatusCodeHttpResult, IContentTypeHttpResult, IValueHttpResult, @@ -19,21 +22,41 @@ public sealed class BitwardenValidationProblemResult : { private readonly ProblemHttpResult _inner; - internal BitwardenValidationProblemResult(ProblemHttpResult inner) + internal BitwardenValidationProblemResult(BitwardenValidationProblemDetails problemDetails) { - ArgumentNullException.ThrowIfNull(inner); - _inner = inner; + ArgumentNullException.ThrowIfNull(problemDetails); + _inner = TypedResults.Problem(problemDetails); + ProblemDetails = problemDetails; } - public ProblemDetails ProblemDetails => _inner.ProblemDetails; + public BitwardenValidationProblemDetails ProblemDetails { get; } public int? StatusCode => _inner.StatusCode; public string? ContentType => _inner.ContentType; - object? IValueHttpResult.Value => _inner.ProblemDetails; + object? IValueHttpResult.Value => ProblemDetails; - ProblemDetails? IValueHttpResult.Value => _inner.ProblemDetails; + ProblemDetails? IValueHttpResult.Value => ProblemDetails; public Task ExecuteAsync(HttpContext httpContext) => _inner.ExecuteAsync(httpContext); + + /// + /// Declares the problem document on every endpoint whose handler can return this result, so a client reads + /// the coded shape out of OpenAPI rather than out of our source. + /// + /// + /// Declares 400, the status the result defaults to. An endpoint that carries this same body on another + /// status — 409 for a state conflict — declares that one itself with Produces. + /// + public static void PopulateMetadata(MethodInfo method, EndpointBuilder builder) + { + ArgumentNullException.ThrowIfNull(method); + ArgumentNullException.ThrowIfNull(builder); + + builder.Metadata.Add(new ProducesResponseTypeMetadata( + StatusCodes.Status400BadRequest, + typeof(BitwardenValidationProblemDetails), + ["application/problem+json"])); + } } diff --git a/src/HttpExtensions/ErrorCode.cs b/src/HttpExtensions/ErrorCode.cs new file mode 100644 index 000000000000..d1e25f2f69c5 --- /dev/null +++ b/src/HttpExtensions/ErrorCode.cs @@ -0,0 +1,33 @@ +using System.Text.Json.Nodes; +using System.Text.Json.Serialization; + +namespace Bit.HttpExtensions; + +/// +/// One failure under a property: a stable the client switches on, a human-readable +/// , and the substitutions it needs to render that detail in another language. +/// +/// +/// The machine-readable code. Names what went wrong and never the property it is keyed under — required, +/// not name_required. Draw it from rather than spelling one locally. +/// +/// +/// The English message. A client that localizes renders from and its +/// instead, so this is a fallback rather than the contract. +/// +/// +/// The substitutions a client needs to render its own message for — a length limit, a +/// range bound. Omitted from the body when null, so a code needing no substitution costs nothing. Carries the +/// limit that was breached and never anything derived from the value that breached it: a length ceiling, never +/// the string that overran it. Key it from . +/// +/// +/// rather than a dictionary of object so the document stays serializable without +/// reflection. An object value has to have its runtime type resolved when it is written, which is the one +/// thing a source-generated — and so trim- and AOT-safe — serializer cannot do. +/// +public sealed record ErrorCode( + string Type, + string Detail, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + JsonObject? Parameters = null); diff --git a/src/HttpExtensions/HttpExtensions.csproj b/src/HttpExtensions/HttpExtensions.csproj index 2fadb712c8c1..3cf4bc7c6b6e 100644 --- a/src/HttpExtensions/HttpExtensions.csproj +++ b/src/HttpExtensions/HttpExtensions.csproj @@ -1,5 +1,15 @@ + + + true + false + + diff --git a/src/HttpExtensions/README.md b/src/HttpExtensions/README.md new file mode 100644 index 000000000000..b3ae6ec253a6 --- /dev/null +++ b/src/HttpExtensions/README.md @@ -0,0 +1,101 @@ +# HttpExtensions — validation problems + +Every validation failure answers with one RFC 7807 document carrying a machine-readable code the +client can switch on and localize. + +Before, a request that failed its DataAnnotations answered with the `ErrorResponseModel` envelope +while a request its handler rejected answered with a problem document. Same endpoint, same field, +two bodies and two key conventions — a client needed both parsers, and only one of them carried a +code. + +```json +{ + "type": "validation_error", + "title": "One or more validation errors occurred.", + "status": 400, + "errors": { + "reason": [{ "type": "required", "detail": "The Reason field is required." }], + "name": [{ + "type": "too_long", + "detail": "The field Name must be a string with a maximum length of 200.", + "parameters": { "max": 200 } + }] + } +} +``` + +`type` names what went wrong and never the field it is keyed under (`required`, not +`name_required`), so a client handles it generically wherever it appears. `parameters` carries what +a client needs to write its own sentence in its own language. + +## Vocabulary + +Draw codes from [`ValidationCodes`](ValidationCodes.cs) and parameter keys from +[`ValidationParameters`](ValidationParameters.cs). Two validators catching the same condition must +answer with the same code — that is the point of a shared list, and it applies to parameter keys +just as much: a client looking up `max` finds nothing under `maximum`. + +`parameters` carries the limit that was breached and never anything derived from the value that +breached it — a length ceiling, never the string that overran it. Error bodies travel further than +the request did. + +## Returning one from a handler + +```csharp +return TypedResults.BitwardenValidationProblem( +[ + ("reason", new ErrorCode(ValidationCodes.Required, "Reason is required.")), +]); +``` + +## Attribute failures + +DataAnnotations records a message and discards the constraint that produced it: the `200` in +`[StringLength(200)]` is gone by the time the failure is reported. The sentence it leaves behind +does still contain that `200`, so [`ValidationMessageCodes`](ValidationMessageCodes.cs) recognises +the wording and lifts the value back out, and +[`ValidationProblemFactory`](ValidationProblemFactory.cs) assembles the document. + +Nothing here validates anything and nothing here reflects. The framework decides whether a value is +valid; this reads what it said afterwards. Being pure string work it needs no generator, no +registration and no `[RequiresUnreferencedCode]` — the assembly builds with `IsAotCompatible` and is +clean, so it can be published from a trimmed or ahead-of-time minimal API as-is. + +### What this costs + +The wording is the contract, and that has three consequences worth knowing before you rely on it. + +**A reworded message loses its code.** The failure is still reported, with its message intact, as +`invalid`. `ValidationMessageCodesTests` asserts every pattern against the message its attribute +actually produces, so a framework reword fails the build on the next SDK bump rather than in +production. + +**An explicit `ErrorMessage` never matches.** `[Required(ErrorMessage = "'key' must be provided")]` +is recognised by nothing and reports as `invalid`. Roughly 5% of the repo's validation attributes +set one today. + +**Renamed and non-numeric values are approximate.** Model state keys the CLR name, so a property +renamed by `[JsonPropertyName]` is reported under its camel-cased CLR name rather than the name the +client sent — the same thing the previous envelope did. And a bound lifted out of prose is the +framework's rendering of it, so `[Range(typeof(DateTime), "2020-01-01", …)]` yields +`"2020-01-01 00:00:00"`, not the declared literal. + +All three are fixable by reading the model's attributes instead of its messages — with reflection, +which is not trim-safe, or with a source generator, which is more machinery. This takes the cheap +option deliberately. + +## Turning it on + +The coded document is behind `FeatureFlagKeys.CodedValidationProblems`. With the flag off, every +surface answers exactly as it did before. + +It replaces a body clients are already parsing, so each surface opts in separately — +`ModelStateValidationFilterAttribute.TryCodedProblem` offers it, and only the internal Api takes it +today. The public API keeps its published shape, which is versioned on its own terms. + +## Known gaps + +- `ExceptionHandlerFilterAttribute` still answers with `ErrorResponseModel`, so + `throw new BadRequestException(modelState)` bypasses all of this. +- `.Produces(400)` is not declared on endpoints, so OpenAPI does + not yet describe the coded shape. diff --git a/src/HttpExtensions/ValidationCodes.cs b/src/HttpExtensions/ValidationCodes.cs new file mode 100644 index 000000000000..73faeb6ade15 --- /dev/null +++ b/src/HttpExtensions/ValidationCodes.cs @@ -0,0 +1,62 @@ +namespace Bit.HttpExtensions; + +/// +/// The repo-wide vocabulary of validation codes. A code names what went wrong; the property it is keyed +/// under names where, so a code never repeats the field it describes — required, not +/// name_required. +/// +/// +/// +/// Codes are scoped to their property, so the same code under two properties means the same thing in both, and a +/// client can handle required generically wherever it appears rather than learning one spelling per field. +/// +/// +/// Two validators catching the same condition must answer with the same code — that is the point of a shared +/// list. Add a constant here before inventing a spelling locally. +/// +/// +public static class ValidationCodes +{ + /// A value is absent that must be present. + public const string Required = "required"; + + /// A value is longer than its limit. Carries max. + public const string TooLong = "too_long"; + + /// A value is shorter than its limit. Carries min. + public const string TooShort = "too_short"; + + /// + /// A value breaches a length constraint that bounds it at both ends. Carries min and max. + /// + /// + /// One code rather than and because a two-ended constraint + /// reports the same message whichever end was breached, leaving nothing to tell them apart. The client has + /// both bounds and composes the sentence itself. + /// + public const string InvalidLength = "invalid_length"; + + /// A number falls outside its permitted range. Carries min and max. + public const string OutOfRange = "out_of_range"; + + /// A number is zero or negative where only a positive value makes sense. + public const string MustBePositive = "must_be_positive"; + + /// A value exceeds a ceiling. Carries max. + public const string ExceedsMax = "exceeds_max"; + + /// A value is not a well-formed email address. + public const string InvalidEmail = "invalid_email"; + + /// A value does not match the pattern this property requires. Carries pattern. + public const string InvalidFormat = "invalid_format"; + + /// A value differs from the one it was required to equal. Carries other. + public const string MustMatch = "must_match"; + + /// A value is not one this property accepts, for a reason no more specific code covers. + public const string Invalid = "invalid"; + + /// A value is well-formed but already taken by something else. + public const string Taken = "taken"; +} diff --git a/src/HttpExtensions/ValidationMessageCodes.cs b/src/HttpExtensions/ValidationMessageCodes.cs new file mode 100644 index 000000000000..a1982796f6fa --- /dev/null +++ b/src/HttpExtensions/ValidationMessageCodes.cs @@ -0,0 +1,175 @@ +using System.Globalization; +using System.Text.Json.Nodes; +using System.Text.RegularExpressions; + +namespace Bit.HttpExtensions; + +/// +/// Names a validation failure by recognising the message the framework recorded for it. +/// +/// +/// +/// DataAnnotations records a message and discards the constraint that produced it, so the only thing left to work +/// from is the sentence — which does at least still contain the limit that was breached. Each pattern below +/// recognises one attribute's wording and lifts its values back out. +/// +/// +/// Nothing here validates anything, and nothing here reflects. The framework decides whether a value is valid; +/// this only reads what it said afterwards. Being pure string work, it survives trimming and ahead-of-time +/// publishing with no annotations and no generated lookup. +/// +/// +/// The cost is that the wording is the contract. A framework release that rewrites a message stops it being +/// recognised, and the failure is reported as with its message intact +/// rather than under a wrong code. ValidationMessageCodesTests asserts every pattern against the message +/// its attribute actually produces, so a reword fails the build on the next SDK bump instead of in production. +/// +/// +public static partial class ValidationMessageCodes +{ + /// + /// The code and parameters for , or when no + /// pattern claims it. + /// + /// + /// Always answers. An unrecognised message keeps its detail and loses only its code, because a failure that + /// reached the client uncoded is still a failure it can read. + /// + public static ErrorCode Resolve(string message) + { + ArgumentNullException.ThrowIfNull(message); + + if (Required().IsMatch(message)) + { + return new ErrorCode(ValidationCodes.Required, message); + } + + if (BoundedLength().Match(message) is { Success: true } bounded) + { + // One message covers both directions of a two-ended constraint, so there is nothing to tell them + // apart. The client gets both bounds and composes the sentence itself. + return Coded(ValidationCodes.InvalidLength, message, + (ValidationParameters.Min, bounded.Groups[1].Value), + (ValidationParameters.Max, bounded.Groups[2].Value)); + } + + if (MaximumLength().Match(message) is { Success: true } tooLong) + { + return Coded(ValidationCodes.TooLong, message, (ValidationParameters.Max, tooLong.Groups[1].Value)); + } + + if (MinimumLength().Match(message) is { Success: true } tooShort) + { + return Coded(ValidationCodes.TooShort, message, (ValidationParameters.Min, tooShort.Groups[1].Value)); + } + + if (Range().Match(message) is { Success: true } range) + { + return Coded(ValidationCodes.OutOfRange, message, + (ValidationParameters.Min, range.Groups[1].Value), + (ValidationParameters.Max, range.Groups[2].Value)); + } + + if (EmailAddress().IsMatch(message)) + { + return new ErrorCode(ValidationCodes.InvalidEmail, message); + } + + if (Pattern().Match(message) is { Success: true } pattern) + { + return Coded(ValidationCodes.InvalidFormat, message, + (ValidationParameters.Pattern, pattern.Groups[1].Value)); + } + + if (Compare().Match(message) is { Success: true } compare) + { + return Coded(ValidationCodes.MustMatch, message, (ValidationParameters.Other, compare.Groups[1].Value)); + } + + if (Url().IsMatch(message) || Phone().IsMatch(message) || CreditCard().IsMatch(message)) + { + return new ErrorCode(ValidationCodes.InvalidFormat, message); + } + + return new ErrorCode(ValidationCodes.Invalid, message); + } + + private static ErrorCode Coded(string code, string message, params (string Name, string Value)[] parameters) + { + var bag = new JsonObject(); + foreach (var (name, value) in parameters) + { + bag[name] = Number(value); + } + + return new ErrorCode(code, message, bag); + } + + /// + /// The bound as the closest thing it reads as. + /// + /// + /// Lifted out of prose, so a bound that is not a number is carried as the text it was written as — a date + /// range states its bounds as dates, and inventing a number for them would be worse than passing them along. + /// + private static JsonNode? Number(string value) + { + if (int.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var whole)) + { + return JsonValue.Create(whole); + } + + if (long.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var wide)) + { + return JsonValue.Create(wide); + } + + if (double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out var real)) + { + return JsonValue.Create(real); + } + + return JsonValue.Create(value); + } + + [GeneratedRegex(@"^The (?:.+) field is required\.$", RegexOptions.CultureInvariant)] + private static partial Regex Required(); + + [GeneratedRegex( + @"^The field (?:.+) must be a string with a minimum length of (.+) and a maximum length of (.+)\.$", + RegexOptions.CultureInvariant)] + private static partial Regex BoundedLength(); + + [GeneratedRegex( + @"^The field (?:.+) must be a string (?:or array type )?with a maximum length of '?(.+?)'?\.$", + RegexOptions.CultureInvariant)] + private static partial Regex MaximumLength(); + + [GeneratedRegex( + @"^The field (?:.+) must be a string (?:or array type )?with a minimum length of '?(.+?)'?\.$", + RegexOptions.CultureInvariant)] + private static partial Regex MinimumLength(); + + [GeneratedRegex(@"^The field (?:.+) must be between (.+) and (.+)\.$", RegexOptions.CultureInvariant)] + private static partial Regex Range(); + + [GeneratedRegex(@"^The (?:.+) field is not a valid e-mail address\.$", RegexOptions.CultureInvariant)] + private static partial Regex EmailAddress(); + + [GeneratedRegex(@"^The field (?:.+) must match the regular expression '(.+)'\.$", RegexOptions.CultureInvariant)] + private static partial Regex Pattern(); + + [GeneratedRegex(@"^'(?:.+)' and '(.+)' do not match\.$", RegexOptions.CultureInvariant)] + private static partial Regex Compare(); + + [GeneratedRegex( + @"^The (?:.+) field is not a valid fully-qualified http, https, or ftp URL\.$", + RegexOptions.CultureInvariant)] + private static partial Regex Url(); + + [GeneratedRegex(@"^The (?:.+) field is not a valid phone number\.$", RegexOptions.CultureInvariant)] + private static partial Regex Phone(); + + [GeneratedRegex(@"^The (?:.+) field is not a valid credit card number\.$", RegexOptions.CultureInvariant)] + private static partial Regex CreditCard(); +} diff --git a/src/HttpExtensions/ValidationParameters.cs b/src/HttpExtensions/ValidationParameters.cs new file mode 100644 index 000000000000..b97384e1f23b --- /dev/null +++ b/src/HttpExtensions/ValidationParameters.cs @@ -0,0 +1,24 @@ +namespace Bit.HttpExtensions; + +/// +/// The repo-wide spelling of the keys an bag carries. +/// +/// +/// A shared list for the same reason is one: a client renders its own message by +/// looking a substitution up by name, so two validators reporting the same limit as max and maximum +/// break it exactly as surely as two spellings of the code would. +/// +public static class ValidationParameters +{ + /// The lower bound a value missed. + public const string Min = "min"; + + /// The upper bound a value exceeded. + public const string Max = "max"; + + /// The pattern a value did not match. + public const string Pattern = "pattern"; + + /// The property this one was required to match. + public const string Other = "other"; +} diff --git a/src/HttpExtensions/ValidationProblemFactory.cs b/src/HttpExtensions/ValidationProblemFactory.cs new file mode 100644 index 000000000000..099622680041 --- /dev/null +++ b/src/HttpExtensions/ValidationProblemFactory.cs @@ -0,0 +1,100 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.ModelBinding; + +namespace Bit.HttpExtensions; + +/// +/// Turns the model state the framework filled in into the coded problem document clients read. +/// +/// +/// Every failure in the model state reaches the document, coded when +/// recognises its message and as when +/// it does not — a failure is never dropped for want of a code, because a client that gets a 400 with an empty +/// errors map has been told less than nothing. +/// +public static class ValidationProblemFactory +{ + /// + /// Builds the document for , keyed by the property names the client sent. + /// + public static BitwardenValidationProblemDetails FromModelState( + ModelStateDictionary modelState, + string title = "One or more validation errors occurred.", + string type = "validation_error", + int statusCode = StatusCodes.Status400BadRequest) + { + ArgumentNullException.ThrowIfNull(modelState); + + var errors = new Dictionary>(StringComparer.Ordinal); + + foreach (var (key, entry) in modelState) + { + if (entry.ValidationState != ModelValidationState.Invalid || entry.Errors.Count == 0) + { + continue; + } + + var wirePath = ToWireName(key); + if (!errors.TryGetValue(wirePath, out var codes)) + { + codes = []; + errors[wirePath] = codes; + } + + foreach (var modelError in entry.Errors) + { + codes.Add(ValidationMessageCodes.Resolve(Describe(modelError))); + } + } + + return new BitwardenValidationProblemDetails + { + Status = statusCode, + Title = title, + Type = type, + Errors = errors.ToDictionary(pair => pair.Key, pair => pair.Value.ToArray(), StringComparer.Ordinal), + }; + } + + /// + /// The message to report, falling back when model binding recorded an exception instead of one. + /// + /// + /// The exception's own message is not used: a binding failure carries framework or serializer detail that + /// describes our internals rather than the caller's mistake, and it is not ours to put on the wire. + /// + private static string Describe(ModelError modelError) => + !string.IsNullOrEmpty(modelError.ErrorMessage) + ? modelError.ErrorMessage + : "The value provided is not valid."; + + /// + /// The property path as the client wrote it, matching the camel casing the serializer applies to everything + /// else. + /// + /// + /// Model state keys the CLR name, and without reading the model there is nothing to consult for a property + /// renamed by [JsonPropertyName]. Such a property is reported under its camel-cased CLR name, which is + /// what the previous envelope did too. + /// + private static string ToWireName(string modelStateKey) + { + if (string.IsNullOrEmpty(modelStateKey)) + { + return string.Empty; + } + + var segments = modelStateKey.Split('.'); + for (var i = 0; i < segments.Length; i++) + { + segments[i] = CamelCase(segments[i]); + } + + return string.Join('.', segments); + } + + private static string CamelCase(string segment) => + segment.Length > 0 && char.IsUpper(segment[0]) + ? char.ToLowerInvariant(segment[0]) + segment[1..] + : segment; +} diff --git a/src/HttpExtensions/packages.lock.json b/src/HttpExtensions/packages.lock.json index 4a91a8cd78fb..c105b3057f6b 100644 --- a/src/HttpExtensions/packages.lock.json +++ b/src/HttpExtensions/packages.lock.json @@ -1,6 +1,13 @@ { "version": 1, "dependencies": { - "net10.0": {} + "net10.0": { + "Microsoft.NET.ILLink.Tasks": { + "type": "Direct", + "requested": "[10.0.8, )", + "resolved": "10.0.8", + "contentHash": "dVbSXGIFNR5nZcv2tOLoWI+a9T4jtFd77IYjuND+QVe360qWgAF7H0WtoopYhRw/+SgpGUTyrkrh+65+ClNnfw==" + } + } } } \ No newline at end of file diff --git a/src/SharedWeb/Utilities/ModelStateValidationFilterAttribute.cs b/src/SharedWeb/Utilities/ModelStateValidationFilterAttribute.cs index c4dfbfb89e12..39c754c0682a 100644 --- a/src/SharedWeb/Utilities/ModelStateValidationFilterAttribute.cs +++ b/src/SharedWeb/Utilities/ModelStateValidationFilterAttribute.cs @@ -1,6 +1,11 @@ -using Bit.Core.Models.Api; +using Bit.Core; +using Bit.Core.Models.Api; +using Bit.HttpExtensions; +using Bitwarden.Server.Sdk.Features; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.Extensions.DependencyInjection; namespace Bit.SharedWeb.Utilities; @@ -28,4 +33,29 @@ protected virtual void OnModelStateInvalid(ActionExecutingContext context) { context.Result = new BadRequestObjectResult(new ErrorResponseModel(context.ModelState)); } + + /// + /// The coded problem document for this failure, or null when the surface has not been switched over to it. + /// + /// + /// + /// Offered rather than applied, because the shape is a breaking change for whoever is reading it: a caller + /// parsing validationErrors finds nothing it recognises in an RFC 7807 body. Each surface opts in on + /// its own schedule, and none is switched by inheriting from this. + /// + /// + protected static IActionResult? TryCodedProblem(ActionExecutingContext context) + { + var featureService = context.HttpContext.RequestServices.GetService(); + if (featureService?.IsEnabled(FeatureFlagKeys.CodedValidationProblems) != true) + { + return null; + } + + return new ObjectResult(ValidationProblemFactory.FromModelState(context.ModelState)) + { + StatusCode = StatusCodes.Status400BadRequest, + ContentTypes = { "application/problem+json" }, + }; + } } diff --git a/test/Api.Test/AdminConsole/Controllers/ValidationErrorTypedResultsExtensionsTests.cs b/test/Api.Test/AdminConsole/Controllers/ValidationErrorTypedResultsExtensionsTests.cs index 17b229e723ed..7789e20f144c 100644 --- a/test/Api.Test/AdminConsole/Controllers/ValidationErrorTypedResultsExtensionsTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/ValidationErrorTypedResultsExtensionsTests.cs @@ -2,7 +2,6 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.HttpResults; using Xunit; -using static Microsoft.AspNetCore.Http.HttpResults.BitwardenTypedResultsExtensions; namespace Bit.Api.Test.AdminConsole.Controllers; @@ -16,7 +15,7 @@ public void BitwardenValidationProblem_WithValidationError_KeysByPropertyName() var result = TypedResults.BitwardenValidationProblem(validationError); Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); - var errors = Assert.IsType>(result.ProblemDetails.Extensions["errors"]); + var errors = result.ProblemDetails.Errors; var entry = Assert.Single(errors); Assert.Equal("email", entry.Key); var error = Assert.Single(entry.Value); diff --git a/test/Api.Test/Utilities/ModelStateValidationFilterAttributeTests.cs b/test/Api.Test/Utilities/ModelStateValidationFilterAttributeTests.cs new file mode 100644 index 000000000000..d1a708919a57 --- /dev/null +++ b/test/Api.Test/Utilities/ModelStateValidationFilterAttributeTests.cs @@ -0,0 +1,109 @@ +using System.ComponentModel.DataAnnotations; +using Bit.Api.Utilities; +using Bit.Core; +using Bit.HttpExtensions; +using Bitwarden.Server.Sdk.Features; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Controllers; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +using NSubstitute; +using Xunit; +using InternalApi = Bit.Core.Models.Api; +using PublicApi = Bit.Api.Models.Public.Response; + +namespace Bit.Api.Test.Utilities; + +/// +/// Which body each surface answers a failed model with, and when. +/// +/// +/// The coded document replaces one a client is already parsing, so what matters here is not that it is produced +/// but that nothing produces it until it is switched on, and that the public API never does. +/// +public class ModelStateValidationFilterAttributeTests +{ + private sealed class Model + { + [Required] + public string? Name { get; set; } + } + + [Fact] + public void InternalApi_WithTheFlagOff_AnswersWithTheLegacyEnvelope() + { + var context = Context(); + + new ModelStateValidationFilterAttribute(publicApi: false).OnActionExecuting(context); + + var result = Assert.IsType(context.Result); + Assert.IsType(result.Value); + } + + [Fact] + public void InternalApi_WithTheFlagOn_AnswersWithTheCodedProblemDocument() + { + var context = Context(flagEnabled: true); + + new ModelStateValidationFilterAttribute(publicApi: false).OnActionExecuting(context); + + var result = Assert.IsType(context.Result); + Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); + Assert.Contains("application/problem+json", result.ContentTypes); + var problem = Assert.IsType(result.Value); + Assert.Equal("validation_error", problem.Type); + Assert.Single(problem.Errors); + } + + [Fact] + public void PublicApi_WithTheFlagOn_StillAnswersWithItsPublishedShape() + { + // The public API's error shape is versioned separately and is not carried along by this flag. + var context = Context(flagEnabled: true); + + new ModelStateValidationFilterAttribute(publicApi: true).OnActionExecuting(context); + + var result = Assert.IsType(context.Result); + Assert.IsType(result.Value); + } + + [Fact] + public void AValidModel_IsLeftAlone() + { + var context = Context(flagEnabled: true, invalid: false); + + new ModelStateValidationFilterAttribute(publicApi: false).OnActionExecuting(context); + + Assert.Null(context.Result); + } + + private static ActionExecutingContext Context(bool flagEnabled = false, bool invalid = true) + { + var featureService = Substitute.For(); + featureService.IsEnabled(FeatureFlagKeys.CodedValidationProblems).Returns(flagEnabled); + + var services = new ServiceCollection(); + services.AddSingleton(featureService); + + var httpContext = new DefaultHttpContext { RequestServices = services.BuildServiceProvider() }; + var descriptor = new ControllerActionDescriptor + { + Parameters = [new ParameterDescriptor { Name = "model", ParameterType = typeof(Model) }], + }; + + var actionContext = new ActionContext(httpContext, new RouteData(), descriptor); + if (invalid) + { + actionContext.ModelState.AddModelError("Name", "The Name field is required."); + } + + return new ActionExecutingContext( + actionContext, + [], + new Dictionary { ["model"] = new Model() }, + controller: null!); + } +} diff --git a/test/HttpExtensions.Test/BitwardenTypedResultsExtensionsTests.cs b/test/HttpExtensions.Test/BitwardenTypedResultsExtensionsTests.cs index 1e04b38f8726..aead3b3e0288 100644 --- a/test/HttpExtensions.Test/BitwardenTypedResultsExtensionsTests.cs +++ b/test/HttpExtensions.Test/BitwardenTypedResultsExtensionsTests.cs @@ -1,14 +1,13 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.HttpResults; using Xunit; -using static Microsoft.AspNetCore.Http.HttpResults.BitwardenTypedResultsExtensions; namespace Bit.HttpExtensions.Test; public class BitwardenTypedResultsExtensionsTests { [Fact] - public void BitwardenValidationProblem_WithErrorsDictionary_Returns400WithErrorsExtension() + public void BitwardenValidationProblem_WithErrorsDictionary_Returns400CarryingThem() { var errors = new Dictionary { @@ -20,8 +19,7 @@ public void BitwardenValidationProblem_WithErrorsDictionary_Returns400WithErrors Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); Assert.Equal("One or more validation errors occurred.", result.ProblemDetails.Title); Assert.Equal("validation_error", result.ProblemDetails.Type); - Assert.True(result.ProblemDetails.Extensions.ContainsKey("errors")); - Assert.Same(errors, result.ProblemDetails.Extensions["errors"]); + Assert.Equal("invalid", Assert.Single(result.ProblemDetails.Errors["email"]).Type); } [Fact] @@ -39,11 +37,11 @@ public void BitwardenValidationProblem_WithExtensions_PreservesCallerExtensionsA var result = TypedResults.BitwardenValidationProblem(errors, extensions: callerExtensions); Assert.Equal("abc-123", result.ProblemDetails.Extensions["traceId"]); - Assert.Same(errors, result.ProblemDetails.Extensions["errors"]); + Assert.Equal("invalid", Assert.Single(result.ProblemDetails.Errors["email"]).Type); } [Fact] - public void BitwardenValidationProblem_WithExtensionsContainingErrorsKey_OverwritesWithProvidedErrors() + public void BitwardenValidationProblem_WithExtensionsContainingErrorsKey_DropsItForTheErrorsMap() { var errors = new Dictionary { @@ -51,12 +49,31 @@ public void BitwardenValidationProblem_WithExtensionsContainingErrorsKey_Overwri }; var callerExtensions = new Dictionary { - { "errors", "should be overwritten" } + { "errors", "should be dropped" } }; var result = TypedResults.BitwardenValidationProblem(errors, extensions: callerExtensions); - Assert.Same(errors, result.ProblemDetails.Extensions["errors"]); + Assert.DoesNotContain("errors", result.ProblemDetails.Extensions.Keys); + Assert.Equal("invalid", Assert.Single(result.ProblemDetails.Errors["email"]).Type); + } + + [Fact] + public void BitwardenValidationProblem_WithAStatusCode_UsesItInsteadOf400() + { + var errors = new Dictionary + { + { "code", [new ErrorCode("already_active", "You already have this.")] } + }; + + var result = TypedResults.BitwardenValidationProblem( + errors, title: "The request conflicts with the current state.", type: "conflict_error", + statusCode: StatusCodes.Status409Conflict); + + Assert.Equal(StatusCodes.Status409Conflict, result.StatusCode); + Assert.Equal(StatusCodes.Status409Conflict, result.ProblemDetails.Status); + Assert.Equal("conflict_error", result.ProblemDetails.Type); + Assert.Equal("already_active", Assert.Single(result.ProblemDetails.Errors["code"]).Type); } [Fact] @@ -64,4 +81,43 @@ public void BitwardenValidationProblem_WithNullErrors_Throws() { Assert.Throws(() => TypedResults.BitwardenValidationProblem(((IDictionary)null!)!)); } + + [Fact] + public void BitwardenValidationProblem_WithPairs_GroupsThemByProperty() + { + (string, ErrorCode)[] errors = + [ + ("password", new ErrorCode("too_short", "Password is too short.")), + ("email", new ErrorCode("invalid", "Email is invalid.")), + ("password", new ErrorCode("missing_digit", "Password needs a digit.")), + ]; + + var result = TypedResults.BitwardenValidationProblem(errors); + + Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); + var grouped = result.ProblemDetails.Errors; + Assert.Equal(2, grouped.Count); + Assert.Equal("invalid", Assert.Single(grouped["email"]).Type); + Assert.Equal(["too_short", "missing_digit"], grouped["password"].Select(error => error.Type)); + } + + [Fact] + public void BitwardenValidationProblem_WithPairsAndAStatusCode_UsesItInsteadOf400() + { + (string, ErrorCode)[] errors = [("code", new ErrorCode("already_active", "You already have this."))]; + + var result = TypedResults.BitwardenValidationProblem( + errors, title: "The request conflicts with the current state.", type: "conflict_error", + statusCode: StatusCodes.Status409Conflict); + + Assert.Equal(StatusCodes.Status409Conflict, result.StatusCode); + Assert.Equal("conflict_error", result.ProblemDetails.Type); + } + + [Fact] + public void BitwardenValidationProblem_WithNullPairs_Throws() + { + Assert.Throws(() => + TypedResults.BitwardenValidationProblem((IEnumerable<(string, ErrorCode)>)null!)); + } } diff --git a/test/HttpExtensions.Test/BitwardenValidationProblemResultTests.cs b/test/HttpExtensions.Test/BitwardenValidationProblemResultTests.cs index 6514e5dc3a29..307eb3ad1452 100644 --- a/test/HttpExtensions.Test/BitwardenValidationProblemResultTests.cs +++ b/test/HttpExtensions.Test/BitwardenValidationProblemResultTests.cs @@ -1,11 +1,11 @@ using System.Text.Json; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; using Xunit; -using static Microsoft.AspNetCore.Http.HttpResults.BitwardenTypedResultsExtensions; namespace Bit.HttpExtensions.Test; @@ -61,4 +61,16 @@ public async Task ExecuteAsync_WritesProblemDetailsToResponseBody() Assert.Equal("memberNotClaimed", firstError.GetProperty("type").GetString()); Assert.Equal("Member not claimed", firstError.GetProperty("detail").GetString()); } + + [Fact] + public void ReturningIt_DocumentsTheProblemDocumentOnTheEndpoint() + { + var handler = RequestDelegateFactory.Create(() => + TypedResults.BitwardenValidationProblem(new Dictionary())); + + var response = Assert.Single(handler.EndpointMetadata.OfType()); + Assert.Equal(StatusCodes.Status400BadRequest, response.StatusCode); + Assert.Equal(typeof(BitwardenValidationProblemDetails), response.Type); + Assert.Equal(["application/problem+json"], response.ContentTypes); + } } diff --git a/test/HttpExtensions.Test/HttpExtensions.Test.csproj b/test/HttpExtensions.Test/HttpExtensions.Test.csproj index c961546b6944..ac981f361228 100644 --- a/test/HttpExtensions.Test/HttpExtensions.Test.csproj +++ b/test/HttpExtensions.Test/HttpExtensions.Test.csproj @@ -9,6 +9,7 @@ runtime; build; native; contentfiles; analyzers; buildtransitive all + diff --git a/test/HttpExtensions.Test/ValidationMessageCodesTests.cs b/test/HttpExtensions.Test/ValidationMessageCodesTests.cs new file mode 100644 index 000000000000..2379f03214a5 --- /dev/null +++ b/test/HttpExtensions.Test/ValidationMessageCodesTests.cs @@ -0,0 +1,144 @@ +using System.ComponentModel.DataAnnotations; +using Xunit; + +namespace Bit.HttpExtensions.Test; + +/// +/// Every pattern, checked against the message its attribute actually produces. +/// +/// +/// Recognising a message is the whole mechanism, so the wording is the contract. These ask the real attribute to +/// format itself and assert the code that comes back, which turns a framework reword into a failure here on the +/// next SDK bump rather than a silently uncoded error in production. +/// +public class ValidationMessageCodesTests +{ + private static ErrorCode Resolve(ValidationAttribute attribute, string name = "Name") => + ValidationMessageCodes.Resolve(attribute.FormatErrorMessage(name)); + + [Fact] + public void Required() + { + Assert.Equal(ValidationCodes.Required, Resolve(new RequiredAttribute()).Type); + } + + [Fact] + public void StringLengthWithOnlyAMaximum_IsTooLongAndCarriesIt() + { + var error = Resolve(new StringLengthAttribute(200)); + + Assert.Equal(ValidationCodes.TooLong, error.Type); + Assert.Equal(200, (int)error.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void StringLengthWithBothBounds_IsOneCodeCarryingBoth() + { + // The attribute words both directions identically, so there is nothing to tell them apart. + var error = Resolve(new StringLengthAttribute(200) { MinimumLength = 5 }); + + Assert.Equal(ValidationCodes.InvalidLength, error.Type); + Assert.Equal(5, (int)error.Parameters![ValidationParameters.Min]!); + Assert.Equal(200, (int)error.Parameters[ValidationParameters.Max]!); + } + + [Fact] + public void MaxLength_IsTooLongAndCarriesIt() + { + var error = Resolve(new MaxLengthAttribute(100)); + + Assert.Equal(ValidationCodes.TooLong, error.Type); + Assert.Equal(100, (int)error.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void MinLength_IsTooShortAndCarriesIt() + { + var error = Resolve(new MinLengthAttribute(1)); + + Assert.Equal(ValidationCodes.TooShort, error.Type); + Assert.Equal(1, (int)error.Parameters![ValidationParameters.Min]!); + } + + [Fact] + public void Range_CarriesBothBounds() + { + var error = Resolve(new RangeAttribute(1, 100)); + + Assert.Equal(ValidationCodes.OutOfRange, error.Type); + Assert.Equal(1, (int)error.Parameters![ValidationParameters.Min]!); + Assert.Equal(100, (int)error.Parameters[ValidationParameters.Max]!); + } + + [Fact] + public void ARangeStatedInNonNumbers_CarriesTheBoundsAsTheMessageRendered_Them() + { + // The limitation of reading bounds out of prose, pinned rather than papered over: what comes back is the + // framework's rendering of the bound, not the string the attribute was declared with. A client gets + // something it can display, but not the original literal. + var error = Resolve(new RangeAttribute(typeof(DateTime), "2020-01-01", "2030-01-01"), "Starts"); + + Assert.Equal(ValidationCodes.OutOfRange, error.Type); + Assert.Equal("2020-01-01 00:00:00", (string)error.Parameters![ValidationParameters.Min]!); + Assert.Equal("2030-01-01 00:00:00", (string)error.Parameters[ValidationParameters.Max]!); + } + + [Fact] + public void EmailAddress() + { + Assert.Equal(ValidationCodes.InvalidEmail, Resolve(new EmailAddressAttribute()).Type); + } + + [Fact] + public void RegularExpression_CarriesThePattern() + { + var error = Resolve(new RegularExpressionAttribute("^a+$")); + + Assert.Equal(ValidationCodes.InvalidFormat, error.Type); + Assert.Equal("^a+$", (string)error.Parameters![ValidationParameters.Pattern]!); + } + + [Fact] + public void Compare_CarriesTheOtherProperty() + { + var error = Resolve(new CompareAttribute("Password"), "ConfirmPassword"); + + Assert.Equal(ValidationCodes.MustMatch, error.Type); + Assert.Equal("Password", (string)error.Parameters![ValidationParameters.Other]!); + } + + [Fact] + public void Url_IsInvalidFormat() => + Assert.Equal(ValidationCodes.InvalidFormat, Resolve(new UrlAttribute()).Type); + + [Fact] + public void Phone_IsInvalidFormat() => + Assert.Equal(ValidationCodes.InvalidFormat, Resolve(new PhoneAttribute()).Type); + + [Fact] + public void CreditCard_IsInvalidFormat() => + Assert.Equal(ValidationCodes.InvalidFormat, Resolve(new CreditCardAttribute()).Type); + + [Fact] + public void AMessageNoPatternClaims_KeepsItsDetailAndLosesOnlyItsCode() + { + // What an explicit ErrorMessage looks like: recognised by nothing, still reported. + var error = ValidationMessageCodes.Resolve("'key' must be provided"); + + Assert.Equal(ValidationCodes.Invalid, error.Type); + Assert.Equal("'key' must be provided", error.Detail); + Assert.Null(error.Parameters); + } + + [Fact] + public void EveryMessageCarriesItsOriginalDetail() + { + var message = new StringLengthAttribute(200).FormatErrorMessage("Name"); + + Assert.Equal(message, ValidationMessageCodes.Resolve(message).Detail); + } + + [Fact] + public void NullMessage_Throws() => + Assert.Throws(() => ValidationMessageCodes.Resolve(null!)); +} diff --git a/test/HttpExtensions.Test/ValidationProblemDocumentTests.cs b/test/HttpExtensions.Test/ValidationProblemDocumentTests.cs new file mode 100644 index 000000000000..445f36cd9526 --- /dev/null +++ b/test/HttpExtensions.Test/ValidationProblemDocumentTests.cs @@ -0,0 +1,181 @@ +using System.Text.Json; +using System.Text.Json.Nodes; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace Bit.HttpExtensions.Test; + +/// +/// The whole document, not one field at a time. Everything else asserts the piece it cares about, which cannot +/// catch a member appearing, disappearing, or moving — and the document is the contract every client parses, so a +/// change to it is a breaking change however the C# side is spelled. Read these as the wire format's spec. +/// +public class ValidationProblemDocumentTests +{ + [Fact] + public async Task SeveralPropertiesFailingAtOnce() + { + var result = TypedResults.BitwardenValidationProblem(errors: new[] + { + ("reason", new ErrorCode("required", "Reason is required.")), + ("name", new ErrorCode("too_long", "Name must be 200 characters or shorter.", + new JsonObject { ["max"] = 200 })), + ("duration", new ErrorCode("must_be_positive", "Duration must be greater than zero.")), + }); + + Assert.Equal( + """ + { + "type": "validation_error", + "title": "One or more validation errors occurred.", + "status": 400, + "errors": { + "reason": [ + { + "type": "required", + "detail": "Reason is required." + } + ], + "name": [ + { + "type": "too_long", + "detail": "Name must be 200 characters or shorter.", + "parameters": { + "max": 200 + } + } + ], + "duration": [ + { + "type": "must_be_positive", + "detail": "Duration must be greater than zero." + } + ] + } + } + """, + await ExecuteAsync(result)); + } + + [Fact] + public async Task ConflictCarriedInTheSameBodyOnADifferentStatus() + { + var result = TypedResults.BitwardenValidationProblem( + errors: new[] { ("code", new ErrorCode("already_active", "You already have active access to this item.")) }, + title: "The request conflicts with the current state.", + type: "conflict_error", + statusCode: StatusCodes.Status409Conflict); + + Assert.Equal( + """ + { + "type": "conflict_error", + "title": "The request conflicts with the current state.", + "status": 409, + "errors": { + "code": [ + { + "type": "already_active", + "detail": "You already have active access to this item." + } + ] + } + } + """, + await ExecuteAsync(result)); + } + + [Fact] + public async Task SeveralFailuresOnOneProperty() + { + var result = TypedResults.BitwardenValidationProblem(errors: new[] + { + ("password", new ErrorCode("too_short", "Password must be at least 12 characters.", + new JsonObject { ["min"] = 12 })), + ("password", new ErrorCode("invalid", "Password is not valid.")), + }); + + Assert.Equal( + """ + { + "type": "validation_error", + "title": "One or more validation errors occurred.", + "status": 400, + "errors": { + "password": [ + { + "type": "too_short", + "detail": "Password must be at least 12 characters.", + "parameters": { + "min": 12 + } + }, + { + "type": "invalid", + "detail": "Password is not valid." + } + ] + } + } + """, + await ExecuteAsync(result)); + } + + [Fact] + public async Task ANestedModelAndACollectionElement() + { + var result = TypedResults.BitwardenValidationProblem(errors: new[] + { + ("owner.email", new ErrorCode("invalid_email", "Email is not an address.")), + ("members[1].name", new ErrorCode("too_long", "Name must be 200 characters or shorter.", + new JsonObject { ["max"] = 200 })), + }); + + Assert.Equal( + """ + { + "type": "validation_error", + "title": "One or more validation errors occurred.", + "status": 400, + "errors": { + "owner.email": [ + { + "type": "invalid_email", + "detail": "Email is not an address." + } + ], + "members[1].name": [ + { + "type": "too_long", + "detail": "Name must be 200 characters or shorter.", + "parameters": { + "max": 200 + } + } + ] + } + } + """, + await ExecuteAsync(result)); + } + + private static async Task ExecuteAsync(IResult result) + { + var context = new DefaultHttpContext + { + RequestServices = new ServiceCollection().AddLogging().AddOptions().BuildServiceProvider(), + }; + using var stream = new MemoryStream(); + context.Response.Body = stream; + + await result.ExecuteAsync(context); + + stream.Position = 0; + return Indent(await new StreamReader(stream).ReadToEndAsync()); + } + + private static string Indent(string json) => JsonSerializer.Serialize( + JsonSerializer.Deserialize(json), new JsonSerializerOptions { WriteIndented = true }); +} diff --git a/test/HttpExtensions.Test/ValidationProblemFactoryTests.cs b/test/HttpExtensions.Test/ValidationProblemFactoryTests.cs new file mode 100644 index 000000000000..7e823556078c --- /dev/null +++ b/test/HttpExtensions.Test/ValidationProblemFactoryTests.cs @@ -0,0 +1,93 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Xunit; + +namespace Bit.HttpExtensions.Test; + +public class ValidationProblemFactoryTests +{ + private static ModelStateDictionary ModelState(params (string Key, string Message)[] errors) + { + var modelState = new ModelStateDictionary(); + foreach (var (key, message) in errors) + { + modelState.AddModelError(key, message); + } + + return modelState; + } + + [Fact] + public void ARecognisedMessage_IsReportedWithItsCode() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Name", "The Name field is required."))); + + var error = Assert.Single(problem.Errors["name"]); + Assert.Equal(ValidationCodes.Required, error.Type); + Assert.Equal("The Name field is required.", error.Detail); + } + + [Fact] + public void AnUnrecognisedMessage_IsStillReported() + { + // Dropping it would answer 400 with an empty errors map, which tells the client less than nothing. + var problem = ValidationProblemFactory.FromModelState(ModelState(("Mystery", "Something was wrong."))); + + var error = Assert.Single(problem.Errors["mystery"]); + Assert.Equal(ValidationCodes.Invalid, error.Type); + Assert.Equal("Something was wrong.", error.Detail); + } + + [Fact] + public void ANestedPath_IsCamelCasedSegmentBySegment() + { + var problem = ValidationProblemFactory.FromModelState(ModelState(("Owner.PostCode", "Bad."))); + + Assert.True(problem.Errors.ContainsKey("owner.postCode")); + } + + [Fact] + public void ACollectionElement_KeepsItsIndex() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Members[1].Email", "The Email field is required."))); + + Assert.Equal(ValidationCodes.Required, Assert.Single(problem.Errors["members[1].email"]).Type); + } + + [Fact] + public void AModelLevelFailure_KeepsTheEmptyKey() + { + var problem = ValidationProblemFactory.FromModelState(ModelState((string.Empty, "Body is empty."))); + + Assert.Equal(ValidationCodes.Invalid, Assert.Single(problem.Errors[string.Empty]).Type); + } + + [Fact] + public void SeveralFailuresOnOnePath_AreCollectedUnderIt() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Name", "The Name field is required."), ("Name", "Name is also wrong."))); + + Assert.Equal(2, problem.Errors["name"].Length); + } + + [Fact] + public void ValidModelState_ProducesAnEmptyErrorsMap() => + Assert.Empty(ValidationProblemFactory.FromModelState(new ModelStateDictionary()).Errors); + + [Fact] + public void TheDocumentCarriesTheProblemMembers() + { + var problem = ValidationProblemFactory.FromModelState(ModelState(("Name", "The Name field is required."))); + + Assert.Equal(StatusCodes.Status400BadRequest, problem.Status); + Assert.Equal("validation_error", problem.Type); + Assert.Equal("One or more validation errors occurred.", problem.Title); + } + + [Fact] + public void NullModelState_Throws() => + Assert.Throws(() => ValidationProblemFactory.FromModelState(null!)); +} diff --git a/test/HttpExtensions.Test/ValidationRoundTripTests.cs b/test/HttpExtensions.Test/ValidationRoundTripTests.cs new file mode 100644 index 000000000000..165e7095f64e --- /dev/null +++ b/test/HttpExtensions.Test/ValidationRoundTripTests.cs @@ -0,0 +1,227 @@ +using System.ComponentModel.DataAnnotations; +using System.Net; +using System.Net.Http.Json; +using System.Text.Json; +using System.Text.Json.Serialization; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ApplicationModels; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace Bit.HttpExtensions.Test; + +/// +/// The whole chain, end to end: a DataAnnotations attribute, the message the framework records for it, the +/// generated map, and the coded document that reaches the client. +/// +/// +/// This is the test that earns the design. The generator recovers a code by recognising what the framework said, +/// so a framework release that rewords a message breaks the mapping silently — everywhere except here, where the +/// assertion is on the code rather than the message and turns the drift into a failure on the next SDK bump. +/// +public class ValidationRoundTripTests +{ + [Fact] + public async Task ARequiredValue_ReportsRequired() + { + var errors = await PostAsync(new { }); + + Assert.Equal(ValidationCodes.Required, Code(errors, "reason")); + } + + [Fact] + public async Task AValueOverItsMaximum_ReportsTooLongAndCarriesTheLimit() + { + var errors = await PostAsync(new { reason = "r", name = new string('n', 201) }); + + Assert.Equal(ValidationCodes.TooLong, Code(errors, "name")); + Assert.Equal(200, errors.GetProperty("name")[0].GetProperty("parameters").GetProperty("max").GetInt32()); + } + + [Fact] + public async Task ANumberOutsideItsRange_ReportsOutOfRangeAndCarriesBothBounds() + { + var errors = await PostAsync(new { reason = "r", seats = 0 }); + + var parameters = errors.GetProperty("seats")[0].GetProperty("parameters"); + Assert.Equal(ValidationCodes.OutOfRange, Code(errors, "seats")); + Assert.Equal(1, parameters.GetProperty("min").GetInt32()); + Assert.Equal(100, parameters.GetProperty("max").GetInt32()); + } + + [Fact] + public async Task AMalformedAddress_ReportsInvalidEmail() + { + var errors = await PostAsync(new { reason = "r", email = "not-an-address" }); + + Assert.Equal(ValidationCodes.InvalidEmail, Code(errors, "email")); + } + + [Fact] + public async Task ATwoEndedLengthConstraint_ReportsOneCodeCarryingBothBounds() + { + // The constraint reports the same message whichever end was breached, so both breaches answer alike. + var tooShort = await PostAsync(new { reason = "r", bounded = "ab" }); + var tooLong = await PostAsync(new { reason = "r", bounded = new string('b', 201) }); + + Assert.Equal(ValidationCodes.InvalidLength, Code(tooShort, "bounded")); + Assert.Equal(ValidationCodes.InvalidLength, Code(tooLong, "bounded")); + + var parameters = tooShort.GetProperty("bounded")[0].GetProperty("parameters"); + Assert.Equal(5, parameters.GetProperty("min").GetInt32()); + Assert.Equal(200, parameters.GetProperty("max").GetInt32()); + } + + [Fact] + public async Task APropertyThatCanFailTwoWays_ReportsTheOneThatFired() + { + // Two attributes on one property, told apart by the message the framework recorded. + var missing = await PostAsync(new { reason = "r" }); + var overlong = await PostAsync(new { reason = "r", accessCode = new string('a', 26) }); + + Assert.Equal(ValidationCodes.Required, Code(missing, "accessCode")); + Assert.Equal(ValidationCodes.TooLong, Code(overlong, "accessCode")); + Assert.Equal(25, overlong.GetProperty("accessCode")[0].GetProperty("parameters").GetProperty("max").GetInt32()); + } + + [Fact] + public async Task ARenamedProperty_IsKeyedByItsCamelCasedClrName() + { + // The limit of not reading the model: model state keys the CLR name, so a property renamed by + // [JsonPropertyName] cannot be reported under the name the client actually sent. This matches what the + // previous envelope did, so it is not a regression — but it is not the ideal either. + var errors = await PostAsync(new { reason = "r" }); + + Assert.True(errors.TryGetProperty("renamedOnTheWire", out _)); + Assert.False(errors.TryGetProperty("tag", out _)); + } + + [Fact] + public async Task ANestedModel_IsKeyedByItsPath() + { + var errors = await PostAsync(new { reason = "r", owner = new { } }); + + Assert.Equal(ValidationCodes.Required, Code(errors, "owner.postcode")); + } + + [Fact] + public async Task ACollectionElement_IsKeyedByItsIndex() + { + var errors = await PostAsync(new { reason = "r", members = new[] { new { }, new { } } }); + + Assert.Equal(ValidationCodes.Required, Code(errors, "members[0].postcode")); + Assert.Equal(ValidationCodes.Required, Code(errors, "members[1].postcode")); + } + + [Fact] + public async Task TheDocumentIsAProblemDocument() + { + var (response, body) = await PostRawAsync(new { }); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.StartsWith("application/problem+json", response.Content.Headers.ContentType?.ToString()); + Assert.Equal("validation_error", body.GetProperty("type").GetString()); + Assert.Equal(400, body.GetProperty("status").GetInt32()); + Assert.Equal("One or more validation errors occurred.", body.GetProperty("title").GetString()); + } + + private static string? Code(JsonElement errors, string property) => + errors.GetProperty(property)[0].GetProperty("type").GetString(); + + private static async Task PostAsync(object payload) => + (await PostRawAsync(payload)).Body.GetProperty("errors"); + + private static async Task<(HttpResponseMessage Response, JsonElement Body)> PostRawAsync(object payload) + { + var builder = WebApplication.CreateEmptyBuilder(new WebApplicationOptions()); + builder.WebHost.UseTestServer(); + builder.Services.AddLogging(); + builder.Services + .AddControllers(options => options.Conventions.Add(new CodedValidationConvention())) + .AddApplicationPart(typeof(ValidationRoundTripTests).Assembly); + + var app = builder.Build(); + app.MapControllers(); + + await app.StartAsync(); + using var client = app.GetTestClient(); + + var response = await client.PostAsJsonAsync("/round-trip", payload); + var raw = await response.Content.ReadAsStringAsync(); + Assert.False(string.IsNullOrWhiteSpace(raw), $"Empty body, status {(int)response.StatusCode}."); + var body = JsonSerializer.Deserialize(raw); + + await app.StopAsync(); + return (response, body); + } + + /// Applies the coded filter the way the Api's convention applies it to every controller. + private sealed class CodedValidationConvention : IControllerModelConvention + { + public void Apply(ControllerModel controller) => controller.Filters.Add(new CodedValidationFilter()); + } + + private sealed class CodedValidationFilter : ActionFilterAttribute + { + public override void OnActionExecuting(ActionExecutingContext context) + { + if (context.ModelState.IsValid) + { + return; + } + + context.Result = new ObjectResult(ValidationProblemFactory.FromModelState(context.ModelState)) + { + StatusCode = StatusCodes.Status400BadRequest, + ContentTypes = { "application/problem+json" }, + }; + } + } +} + +public sealed class RoundTripInner +{ + [Required] + public string? Postcode { get; set; } +} + +public sealed class RoundTripModel +{ + [Required] + public string? Reason { get; set; } + + [StringLength(200)] + public string? Name { get; set; } + + [Required] + [StringLength(25)] + public string? AccessCode { get; set; } + + [StringLength(200, MinimumLength = 5)] + public string? Bounded { get; set; } + + [Range(1, 100)] + public int Seats { get; set; } = 1; + + [EmailAddress] + public string? Email { get; set; } + + [JsonPropertyName("tag")] + [Required] + public string? RenamedOnTheWire { get; set; } + + public RoundTripInner? Owner { get; set; } + + public List? Members { get; set; } +} + +[Route("round-trip")] +public sealed class RoundTripController : ControllerBase +{ + [HttpPost] + public IActionResult Post([FromBody] RoundTripModel model) => Ok(); +} diff --git a/test/HttpExtensions.Test/packages.lock.json b/test/HttpExtensions.Test/packages.lock.json index 24037b8f688c..c093ba2b77db 100644 --- a/test/HttpExtensions.Test/packages.lock.json +++ b/test/HttpExtensions.Test/packages.lock.json @@ -18,6 +18,12 @@ "resolved": "6.0.0", "contentHash": "tW3lsNS+dAEII6YGUX/VMoJjBS1QvsxqJeqLaJXub08y1FSjasFPtQ4UBUsudE9PNrzLjooClMsPtY2cZLdXpQ==" }, + "Microsoft.AspNetCore.TestHost": { + "type": "Direct", + "requested": "[10.0.10, 10.0.10]", + "resolved": "10.0.10", + "contentHash": "Kks+OpQlP/eWQhnTjkiv0H9kc9Uqa7ieAzNV62yJpZ8Ips/WwpfpwrwZUKzV83CBJaBl5OI7J2XxVVTC8vah/Q==" + }, "Microsoft.NET.Test.Sdk": { "type": "Direct", "requested": "[18.0.1, )",