diff --git a/bitwarden-server.slnx b/bitwarden-server.slnx index 2c9fac1170b0..0a4a2a4e316e 100644 --- a/bitwarden-server.slnx +++ b/bitwarden-server.slnx @@ -28,6 +28,7 @@ + @@ -77,6 +78,7 @@ + diff --git a/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs b/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs deleted file mode 100644 index f3695ca6a8fd..000000000000 --- a/src/Api/AdminConsole/Controllers/ValidationErrorTypedResultsExtensions.cs +++ /dev/null @@ -1,30 +0,0 @@ -using Bit.Core.AdminConsole.Utilities.v2.Validation; -using Bit.HttpExtensions; - -namespace Microsoft.AspNetCore.Http.HttpResults; - -public static class ValidationErrorTypedResultsExtensions -{ - extension(TypedResults) - { - /// - /// Produces a 400 Bad Request RFC 7807 problem response keyed by - /// , with the error's - /// as the i18n code and - /// as the human-readable detail. - /// - public static BitwardenValidationProblemResult BitwardenValidationProblem(IValidationError validationError) - { - ArgumentNullException.ThrowIfNull(validationError); - - return TypedResults.BitwardenValidationProblem( - errors: new Dictionary - { - { - validationError.PropertyName, - [new BitwardenTypedResultsExtensions.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/AdminConsole/Utilities/v2/Validation/IValidationError.cs b/src/Core/AdminConsole/Utilities/v2/Validation/IValidationError.cs index bfe0b36eb870..6da9524079de 100644 --- a/src/Core/AdminConsole/Utilities/v2/Validation/IValidationError.cs +++ b/src/Core/AdminConsole/Utilities/v2/Validation/IValidationError.cs @@ -1,4 +1,6 @@ -namespace Bit.Core.AdminConsole.Utilities.v2.Validation; +using System.Text.Json.Nodes; + +namespace Bit.Core.AdminConsole.Utilities.v2.Validation; /// /// An error tied to a specific request property. Implementing this on an allows @@ -10,4 +12,15 @@ public interface IValidationError string PropertyName { get; } string Message { get; } string Type { get; } + + /// + /// The substitutions a client needs to render its own localized message for — the limit that + /// was exceeded, the bound that was missed. Null when the code needs none, which is most of them. Carries the + /// limit that was breached, never anything derived from the value that breached it. + /// + /// + /// A default member so the errors that already implement this interface compile untouched and take parameters + /// on one at a time, rather than every one of them changing to say it has none. + /// + JsonObject? Parameters => null; } diff --git a/src/Core/AdminConsole/Utilities/v2/Validation/ValidationErrorTypedResultsExtensions.cs b/src/Core/AdminConsole/Utilities/v2/Validation/ValidationErrorTypedResultsExtensions.cs new file mode 100644 index 000000000000..7e136cb2dc78 --- /dev/null +++ b/src/Core/AdminConsole/Utilities/v2/Validation/ValidationErrorTypedResultsExtensions.cs @@ -0,0 +1,61 @@ +using Bit.Core.AdminConsole.Utilities.v2.Validation; +using Bit.HttpExtensions; + +namespace Microsoft.AspNetCore.Http.HttpResults; + +/// +/// Renders s as the Bitwarden problem response. Lives in Core rather than beside +/// BitwardenValidationProblem in HttpExtensions because it is the one layer that can see both sides: +/// HttpExtensions does not reference Core, so it cannot name . +/// +public static class ValidationErrorTypedResultsExtensions +{ + extension(TypedResults) + { + /// + /// Produces an RFC 7807 problem response keyed by , with the + /// error's as the i18n code and + /// as the human-readable detail. + /// + /// + /// 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. + /// + public static BitwardenValidationProblemResult BitwardenValidationProblem( + IValidationError validationError, + string title = "One or more validation errors occurred.", + string type = "validation_error", + int statusCode = StatusCodes.Status400BadRequest) + { + ArgumentNullException.ThrowIfNull(validationError); + + return TypedResults.BitwardenValidationProblem( + validationErrors: [validationError], + title: title, + type: type, + statusCode: statusCode); + } + + /// + /// + /// Errors naming the same property are collected under it rather than overwriting one another, so a model + /// that fails several ways at once reports every failure. + /// + public static BitwardenValidationProblemResult BitwardenValidationProblem( + IEnumerable validationErrors, + string title = "One or more validation errors occurred.", + string type = "validation_error", + int statusCode = StatusCodes.Status400BadRequest) + { + ArgumentNullException.ThrowIfNull(validationErrors); + + return TypedResults.BitwardenValidationProblem( + errors: validationErrors.Select(error => ( + error.PropertyName, + new ErrorCode(error.Type, error.Message, error.Parameters))), + title: title, + type: type, + statusCode: statusCode); + } + } +} 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.Generator/AttributeTranslation.cs b/src/HttpExtensions.Generator/AttributeTranslation.cs new file mode 100644 index 000000000000..388e89e037f1 --- /dev/null +++ b/src/HttpExtensions.Generator/AttributeTranslation.cs @@ -0,0 +1,211 @@ +using System.Globalization; +using Microsoft.CodeAnalysis; + +namespace Bit.HttpExtensions.Generator; + +/// One validation attribute, read at compile time and turned into the code it should report. +/// Substitution name to the C# expression producing its value. +/// +/// A C# expression reconstructing the attribute, used to ask it how it words its message. Null when the +/// attribute's constructor is itself unsafe under trimming, in which case this candidate can only be identified +/// by elimination. +/// +internal sealed record AttributeTranslation( + string Code, + IReadOnlyList> Parameters, + string? Construction); + +internal static class AttributeTranslator +{ + private const string Ns = "System.ComponentModel.DataAnnotations"; + + public static bool IsValidationAttribute(AttributeData attribute) + { + for (var type = attribute.AttributeClass; type is not null; type = type.BaseType) + { + if (type.ToDisplayString() == $"{Ns}.ValidationAttribute") + { + return true; + } + } + + return false; + } + + /// Translates one attribute, or returns null when it is not one we have a name for. + public static AttributeTranslation? Translate(AttributeData attribute) + { + var name = attribute.AttributeClass?.ToDisplayString(); + if (name is null) + { + return null; + } + + var parameters = new List>(); + string code; + string? construction; + + switch (name) + { + case $"{Ns}.RequiredAttribute": + code = "required"; + construction = $"new global::{Ns}.RequiredAttribute()"; + break; + + case $"{Ns}.StringLengthAttribute": + { + var max = Ctor(attribute, 0)?.Value; + var min = Named(attribute, "MinimumLength")?.Value; + var bounded = min is int lower && lower > 0; + + if (bounded) + { + // One message covers both directions, so the two cannot be told apart. Report the constraint + // rather than guessing a direction; the client composes from both bounds. + code = "invalid_length"; + parameters.Add(new("min", Literal(min))); + parameters.Add(new("max", Literal(max))); + construction = + $"new global::{Ns}.StringLengthAttribute({Literal(max)}) {{ MinimumLength = {Literal(min)} }}"; + } + else + { + code = "too_long"; + parameters.Add(new("max", Literal(max))); + construction = $"new global::{Ns}.StringLengthAttribute({Literal(max)})"; + } + + break; + } + + // MaxLength, MinLength and Compare have [RequiresUnreferencedCode] constructors, so generated code + // cannot build them to ask for their wording. They are identified by elimination instead. + case $"{Ns}.MaxLengthAttribute": + code = "too_long"; + parameters.Add(new("max", Literal(Ctor(attribute, 0)?.Value))); + construction = null; + break; + + case $"{Ns}.MinLengthAttribute": + code = "too_short"; + parameters.Add(new("min", Literal(Ctor(attribute, 0)?.Value))); + construction = null; + break; + + case $"{Ns}.CompareAttribute": + code = "must_match"; + parameters.Add(new("other", Literal(Ctor(attribute, 0)?.Value))); + construction = null; + break; + + case $"{Ns}.RangeAttribute": + { + // The (Type, string, string) overload states its bounds as strings; the numeric ones do not. + var first = Ctor(attribute, 0)?.Value; + var second = Ctor(attribute, 1)?.Value; + var third = Ctor(attribute, 2)?.Value; + var (min, max) = third is null ? (first, second) : (second, third); + + code = "out_of_range"; + parameters.Add(new("min", Literal(min))); + parameters.Add(new("max", Literal(max))); + construction = third is null + ? $"new global::{Ns}.RangeAttribute({Literal(min)}, {Literal(max)})" + : null; + break; + } + + case $"{Ns}.EmailAddressAttribute": + code = "invalid_email"; + construction = $"new global::{Ns}.EmailAddressAttribute()"; + break; + + case $"{Ns}.RegularExpressionAttribute": + { + var pattern = Ctor(attribute, 0)?.Value; + code = "invalid_format"; + parameters.Add(new("pattern", Literal(pattern))); + construction = $"new global::{Ns}.RegularExpressionAttribute({Literal(pattern)})"; + break; + } + + case $"{Ns}.UrlAttribute": + code = "invalid_format"; + construction = $"new global::{Ns}.UrlAttribute()"; + break; + + case $"{Ns}.PhoneAttribute": + code = "invalid_format"; + construction = $"new global::{Ns}.PhoneAttribute()"; + break; + + case $"{Ns}.CreditCardAttribute": + code = "invalid_format"; + construction = $"new global::{Ns}.CreditCardAttribute()"; + break; + + default: + return null; + } + + // The wording depends on ErrorMessage when it is set, so the reconstruction has to carry it too. + if (construction is not null && Named(attribute, "ErrorMessage")?.Value is string explicitMessage) + { + construction = WithInitializer(construction, $"ErrorMessage = {Quote(explicitMessage)}"); + } + else if (construction is not null && + (Named(attribute, "ErrorMessageResourceName") is not null || + Named(attribute, "ErrorMessageResourceType") is not null)) + { + // Resource-backed wording cannot be reproduced from metadata alone. + construction = null; + } + + return new AttributeTranslation(code, parameters, construction); + } + + /// Merges another assignment into an object initializer, adding one if it has none. + private static string WithInitializer(string construction, string assignment) + { + var brace = construction.LastIndexOf('{'); + return brace < 0 + ? construction + $" {{ {assignment} }}" + : construction.Insert(brace + 1, $" {assignment},"); + } + + private static TypedConstant? Ctor(AttributeData attribute, int index) => + attribute.ConstructorArguments.Length > index ? attribute.ConstructorArguments[index] : null; + + private static TypedConstant? Named(AttributeData attribute, string name) + { + foreach (var pair in attribute.NamedArguments) + { + if (pair.Key == name) + { + return pair.Value; + } + } + + return null; + } + + /// The value as a C# expression. + public static string Literal(object? value) => value switch + { + null => "null", + string s => Quote(s), + bool b => b ? "true" : "false", + double d => d.ToString("R", CultureInfo.InvariantCulture) + "d", + float f => f.ToString("R", CultureInfo.InvariantCulture) + "f", + decimal m => m.ToString(CultureInfo.InvariantCulture) + "m", + long l => l.ToString(CultureInfo.InvariantCulture) + "L", + _ => Convert.ToString(value, CultureInfo.InvariantCulture) ?? "null", + }; + + public static string Quote(string value) => + "\"" + value + .Replace("\\", "\\\\") + .Replace("\"", "\\\"") + .Replace("\r", "\\r") + .Replace("\n", "\\n") + "\""; +} diff --git a/src/HttpExtensions.Generator/HttpExtensions.Generator.csproj b/src/HttpExtensions.Generator/HttpExtensions.Generator.csproj new file mode 100644 index 000000000000..93e2f93fb8ff --- /dev/null +++ b/src/HttpExtensions.Generator/HttpExtensions.Generator.csproj @@ -0,0 +1,19 @@ + + + + + netstandard2.0 + latest + enable + false + false + true + + $(NoWarn);RS2008 + + + + + + + diff --git a/src/HttpExtensions.Generator/IsExternalInit.cs b/src/HttpExtensions.Generator/IsExternalInit.cs new file mode 100644 index 000000000000..8a37c6270c0b --- /dev/null +++ b/src/HttpExtensions.Generator/IsExternalInit.cs @@ -0,0 +1,7 @@ +namespace System.Runtime.CompilerServices; + +/// +/// Polyfill. Records and init accessors need this type to exist; .NET Standard 2.0, which analyzers must +/// target, predates it. +/// +internal static class IsExternalInit; diff --git a/src/HttpExtensions.Generator/ValidationCodeMapGenerator.cs b/src/HttpExtensions.Generator/ValidationCodeMapGenerator.cs new file mode 100644 index 000000000000..f9a6a5a98b09 --- /dev/null +++ b/src/HttpExtensions.Generator/ValidationCodeMapGenerator.cs @@ -0,0 +1,328 @@ +using System.Collections.Immutable; +using System.Text; +using Microsoft.CodeAnalysis; + +namespace Bit.HttpExtensions.Generator; + +/// +/// Emits the map from validation paths to the codes they should be reported under, for models that must resolve +/// without reflecting over themselves. +/// +/// +/// Emits no validation logic. The framework decides whether a model is valid; this only recovers what its +/// attributes said, because a validation result records a message and discards the constraint that produced it. +/// +[Generator] +public sealed class ValidationCodeMapGenerator : IIncrementalGenerator +{ + private const int MaxDepth = 6; + + private static readonly DiagnosticDescriptor Ambiguous = new( + id: "BWVAL001", + title: "Validation codes cannot be told apart", + messageFormat: + "Property '{0}' carries more than one constraint whose wording cannot be reconstructed, so a failure " + + "on it cannot be attributed to one of them. Give all but one an ErrorMessage, or reduce it to a " + + "single constraint.", + category: "Validation", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: + "Failures on this property will be reported without a code rather than with a guessed one."); + + public void Initialize(IncrementalGeneratorInitializationContext context) + { + var marked = context.SyntaxProvider + .ForAttributeWithMetadataName( + "Bit.HttpExtensions.GenerateValidationCodesAttribute", + predicate: static (_, _) => true, + transform: static (ctx, _) => (INamedTypeSymbol)ctx.TargetSymbol) + .Collect(); + + var validatable = context.SyntaxProvider + .ForAttributeWithMetadataName( + "Microsoft.Extensions.Validation.ValidatableTypeAttribute", + predicate: static (_, _) => true, + transform: static (ctx, _) => (INamedTypeSymbol)ctx.TargetSymbol) + .Collect(); + + context.RegisterSourceOutput(marked.Combine(validatable), static (spc, pair) => Emit(spc, pair.Left, pair.Right)); + } + + private static void Emit( + SourceProductionContext context, + ImmutableArray marked, + ImmutableArray validatable) + { + var roots = marked + .Concat(validatable) + .Distinct(SymbolEqualityComparer.Default) + .OfType() + .OrderBy(static type => type.ToDisplayString(), StringComparer.Ordinal) + .ToList(); + + var source = new StringBuilder(); + source.AppendLine("// "); + source.AppendLine("#nullable enable"); + source.AppendLine(); + source.AppendLine("namespace Bit.HttpExtensions.Generated;"); + source.AppendLine(); + source.AppendLine("internal static class ValidationCodeRegistrations"); + source.AppendLine("{"); + source.AppendLine(" [global::System.Runtime.CompilerServices.ModuleInitializer]"); + source.AppendLine(" internal static void Register()"); + source.AppendLine(" {"); + + var wrote = false; + foreach (var root in roots) + { + var entries = new List(); + Walk(root, string.Empty, string.Empty, new List { root }, entries, 0, context); + + if (entries.Count == 0) + { + continue; + } + + wrote = true; + AppendRegistration(source, root, entries); + } + + if (!wrote) + { + source.AppendLine(" // No marked request models in this assembly."); + } + + source.AppendLine(" }"); + source.AppendLine("}"); + + context.AddSource("ValidationCodeRegistrations.g.cs", source.ToString()); + } + + private static void AppendRegistration(StringBuilder source, INamedTypeSymbol root, List entries) + { + source.AppendLine(" global::Bit.HttpExtensions.ValidationCodeMap.Register("); + source.AppendLine($" typeof({root.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)}),"); + source.AppendLine(" new global::System.Collections.Generic.KeyValuePair[]"); + source.AppendLine(" {"); + + foreach (var entry in entries) + { + source.AppendLine($" new({AttributeTranslator.Quote(entry.Path)}, new global::Bit.HttpExtensions.ValidationCodeEntry("); + source.AppendLine($" {AttributeTranslator.Quote(entry.WirePath)},"); + source.AppendLine($" {AttributeTranslator.Quote(entry.DisplayName)},"); + source.AppendLine(" new global::Bit.HttpExtensions.ValidationCodeCandidate[]"); + source.AppendLine(" {"); + + // A property carrying one constraint cannot be ambiguous, so it needs no wording to be told apart by. + var ambiguous = entry.Translations.Count > 1; + foreach (var translation in entry.Translations) + { + var message = ambiguous && translation.Construction is not null + ? $"static name => {translation.Construction}.FormatErrorMessage(name)" + : "null"; + source.Append($" new({AttributeTranslator.Quote(translation.Code)}, {message}"); + source.Append(Parameters(translation)); + source.AppendLine("),"); + } + + source.AppendLine(" })),"); + } + + source.AppendLine(" });"); + } + + private static string Parameters(AttributeTranslation translation) + { + if (translation.Parameters.Count == 0) + { + return string.Empty; + } + + var parts = translation.Parameters.Select(p => + $"new({AttributeTranslator.Quote(p.Key)}, {p.Value})"); + return $", new global::System.Collections.Generic.KeyValuePair[] {{ {string.Join(", ", parts)} }}"; + } + + private sealed record Entry( + string Path, string WirePath, string DisplayName, List Translations); + + /// + /// Walks the model graph, recording every property that carries a constraint. + /// + /// + /// Paths mirror what validation produces: dotted for nesting, [] for a collection element. The wire + /// path is built alongside so the document can name what the client actually sent. + /// + private static void Walk( + INamedTypeSymbol type, + string prefix, + string wirePrefix, + List visiting, + List entries, + int depth, + SourceProductionContext context) + { + if (depth > MaxDepth) + { + return; + } + + foreach (var property in type.GetMembers().OfType()) + { + if (property.DeclaredAccessibility != Accessibility.Public || property.IsStatic || + property.GetMethod is null || property.IsIndexer) + { + continue; + } + + var path = prefix + property.Name; + var wirePath = wirePrefix + WireName(property); + + var translations = new List(); + foreach (var attribute in property.GetAttributes()) + { + if (AttributeTranslator.IsValidationAttribute(attribute) && + AttributeTranslator.Translate(attribute) is { } translation) + { + translations.Add(translation); + } + } + + if (translations.Count > 0) + { + // Exactly one candidate may be identified by elimination; two would make the choice a guess. + if (translations.Count > 1 && translations.Count(t => t.Construction is null) > 1) + { + context.ReportDiagnostic(Diagnostic.Create( + Ambiguous, property.Locations.FirstOrDefault(), path)); + } + else + { + entries.Add(new Entry(path, wirePath, DisplayName(property), translations)); + } + } + + var (nested, isCollection) = Unwrap(property.Type); + if (nested is null || AlreadyVisiting(visiting, nested)) + { + continue; + } + + var separator = isCollection ? "[]." : "."; + visiting.Add(nested); + Walk(nested, path + separator, wirePath + separator, visiting, entries, depth + 1, context); + visiting.RemoveAt(visiting.Count - 1); + } + } + + private static bool AlreadyVisiting(List visiting, INamedTypeSymbol candidate) + { + foreach (var seen in visiting) + { + if (SymbolEqualityComparer.Default.Equals(seen, candidate)) + { + return true; + } + } + + return false; + } + + /// + /// The model behind a property type: the element for a collection, the underlying type for a nullable, and + /// null for anything that is a value rather than a model. + /// + private static (INamedTypeSymbol? Type, bool IsCollection) Unwrap(ITypeSymbol type) + { + if (type is IArrayTypeSymbol array) + { + return (Walkable(array.ElementType), true); + } + + if (type is not INamedTypeSymbol named) + { + return (null, false); + } + + if (named.IsGenericType && named.ConstructedFrom.SpecialType == SpecialType.System_Nullable_T) + { + return (Walkable(named.TypeArguments[0]), false); + } + + if (named.SpecialType != SpecialType.System_String) + { + foreach (var iface in named.AllInterfaces.Concat(new[] { named })) + { + if (iface.IsGenericType && + iface.ConstructedFrom.SpecialType == SpecialType.System_Collections_Generic_IEnumerable_T) + { + return (Walkable(iface.TypeArguments[0]), true); + } + } + } + + return (Walkable(named), false); + } + + /// A type worth descending into: one of ours, with properties that could carry constraints. + private static INamedTypeSymbol? Walkable(ITypeSymbol type) + { + if (type is not INamedTypeSymbol named || + named.TypeKind is not (TypeKind.Class or TypeKind.Struct) || + named.SpecialType != SpecialType.None || + named.IsGenericType) + { + return null; + } + + var containing = named.ContainingNamespace?.ToDisplayString() ?? string.Empty; + if (containing.Length == 0 || + containing == "System" || containing.StartsWith("System.", StringComparison.Ordinal) || + containing == "Microsoft" || containing.StartsWith("Microsoft.", StringComparison.Ordinal)) + { + return null; + } + + return named; + } + + /// The name the client sent: the JSON name when renamed, otherwise the camel-cased property. + private static string WireName(IPropertySymbol property) + { + foreach (var attribute in property.GetAttributes()) + { + if (attribute.AttributeClass?.ToDisplayString() == "System.Text.Json.Serialization.JsonPropertyNameAttribute" && + attribute.ConstructorArguments.Length > 0 && + attribute.ConstructorArguments[0].Value is string name) + { + return name; + } + } + + var clr = property.Name; + return clr.Length > 0 && char.IsUpper(clr[0]) ? char.ToLowerInvariant(clr[0]) + clr.Substring(1) : clr; + } + + /// The name the framework puts in its messages. + private static string DisplayName(IPropertySymbol property) + { + foreach (var attribute in property.GetAttributes()) + { + if (attribute.AttributeClass?.ToDisplayString() != "System.ComponentModel.DataAnnotations.DisplayAttribute") + { + continue; + } + + foreach (var named in attribute.NamedArguments) + { + if (named.Key == "Name" && named.Value.Value is string display) + { + return display; + } + } + } + + return property.Name; + } +} diff --git a/src/HttpExtensions.Generator/packages.lock.json b/src/HttpExtensions.Generator/packages.lock.json new file mode 100644 index 000000000000..10fcc592c469 --- /dev/null +++ b/src/HttpExtensions.Generator/packages.lock.json @@ -0,0 +1,109 @@ +{ + "version": 1, + "dependencies": { + ".NETStandard,Version=v2.0": { + "Microsoft.CodeAnalysis.CSharp": { + "type": "Direct", + "requested": "[4.8.0, 4.8.0]", + "resolved": "4.8.0", + "contentHash": "+3+qfdb/aaGD8PZRCrsdobbzGs1m9u119SkkJt8e/mk3xLJz/udLtS2T6nY27OTXxBBw10HzAbC8Z9w08VyP/g==", + "dependencies": { + "Microsoft.CodeAnalysis.Common": "[4.8.0]" + } + }, + "NETStandard.Library": { + "type": "Direct", + "requested": "[2.0.3, )", + "resolved": "2.0.3", + "contentHash": "st47PosZSHrjECdjeIzZQbzivYBJFv6P2nv4cj2ypdI204DO+vZ7l5raGMiX4eXMJ53RfOIg+/s4DHVZ54Nu2A==", + "dependencies": { + "Microsoft.NETCore.Platforms": "1.1.0" + } + }, + "Microsoft.CodeAnalysis.Analyzers": { + "type": "Transitive", + "resolved": "3.3.4", + "contentHash": "AxkxcPR+rheX0SmvpLVIGLhOUXAKG56a64kV9VQZ4y9gR9ZmPXnqZvHJnmwLSwzrEP6junUF11vuc+aqo5r68g==" + }, + "Microsoft.CodeAnalysis.Common": { + "type": "Transitive", + "resolved": "4.8.0", + "contentHash": "/jR+e/9aT+BApoQJABlVCKnnggGQbvGh7BKq2/wI1LamxC+LbzhcLj4Vj7gXCofl1n4E521YfF9w0WcASGg/KA==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "3.3.4", + "System.Collections.Immutable": "7.0.0", + "System.Memory": "4.5.5", + "System.Reflection.Metadata": "7.0.0", + "System.Runtime.CompilerServices.Unsafe": "6.0.0", + "System.Text.Encoding.CodePages": "7.0.0", + "System.Threading.Tasks.Extensions": "4.5.4" + } + }, + "Microsoft.NETCore.Platforms": { + "type": "Transitive", + "resolved": "1.1.0", + "contentHash": "kz0PEW2lhqygehI/d6XsPCQzD7ff7gUJaVGPVETX611eadGsA3A877GdSlU0LRVMCTH/+P3o2iDTak+S08V2+A==" + }, + "System.Buffers": { + "type": "Transitive", + "resolved": "4.5.1", + "contentHash": "Rw7ijyl1qqRS0YQD/WycNst8hUUMgrMH4FCn1nNm27M4VxchZ1js3fVjQaANHO5f3sN4isvP4a+Met9Y4YomAg==" + }, + "System.Collections.Immutable": { + "type": "Transitive", + "resolved": "7.0.0", + "contentHash": "dQPcs0U1IKnBdRDBkrCTi1FoajSTBzLcVTpjO4MBCMC7f4pDOIPzgBoX8JjG7X6uZRJ8EBxsi8+DR1JuwjnzOQ==", + "dependencies": { + "System.Memory": "4.5.5", + "System.Runtime.CompilerServices.Unsafe": "6.0.0" + } + }, + "System.Memory": { + "type": "Transitive", + "resolved": "4.5.5", + "contentHash": "XIWiDvKPXaTveaB7HVganDlOCRoj03l+jrwNvcge/t8vhGYKvqV+dMv6G4SAX2NoNmN0wZfVPTAlFwZcZvVOUw==", + "dependencies": { + "System.Buffers": "4.5.1", + "System.Numerics.Vectors": "4.4.0", + "System.Runtime.CompilerServices.Unsafe": "4.5.3" + } + }, + "System.Numerics.Vectors": { + "type": "Transitive", + "resolved": "4.4.0", + "contentHash": "UiLzLW+Lw6HLed1Hcg+8jSRttrbuXv7DANVj0DkL9g6EnnzbL75EB7EWsw5uRbhxd/4YdG8li5XizGWepmG3PQ==" + }, + "System.Reflection.Metadata": { + "type": "Transitive", + "resolved": "7.0.0", + "contentHash": "MclTG61lsD9sYdpNz9xsKBzjsmsfCtcMZYXz/IUr2zlhaTaABonlr1ESeompTgM+Xk+IwtGYU7/voh3YWB/fWw==", + "dependencies": { + "System.Collections.Immutable": "7.0.0", + "System.Memory": "4.5.5" + } + }, + "System.Runtime.CompilerServices.Unsafe": { + "type": "Transitive", + "resolved": "6.0.0", + "contentHash": "/iUeP3tq1S0XdNNoMz5C9twLSrM/TH+qElHkXWaPvuNOt+99G75NrV0OS2EqHx5wMN7popYjpc8oTjC1y16DLg==" + }, + "System.Text.Encoding.CodePages": { + "type": "Transitive", + "resolved": "7.0.0", + "contentHash": "LSyCblMpvOe0N3E+8e0skHcrIhgV2huaNcjUUEa8hRtgEAm36aGkRoC8Jxlb6Ra6GSfF29ftduPNywin8XolzQ==", + "dependencies": { + "System.Memory": "4.5.5", + "System.Runtime.CompilerServices.Unsafe": "6.0.0" + } + }, + "System.Threading.Tasks.Extensions": { + "type": "Transitive", + "resolved": "4.5.4", + "contentHash": "zteT+G8xuGu6mS+mzDzYXbzS7rd3K6Fjb9RiZlYlJPam2/hU7JCBZBVEcywNuR+oZ1ncTvc/cq0faRr3P01OVg==", + "dependencies": { + "System.Runtime.CompilerServices.Unsafe": "4.5.3" + } + } + } + } +} \ No newline at end of file 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/GenerateValidationCodesAttribute.cs b/src/HttpExtensions/GenerateValidationCodesAttribute.cs new file mode 100644 index 000000000000..3644edc48031 --- /dev/null +++ b/src/HttpExtensions/GenerateValidationCodesAttribute.cs @@ -0,0 +1,19 @@ +namespace Bit.HttpExtensions; + +/// +/// Marks a request model whose validation codes should be worked out at build time rather than by reflecting over +/// it at runtime. +/// +/// +/// +/// Needed only where the app must survive trimming or ahead-of-time publishing — a minimal API. MVC declares +/// itself unsupported under both, so a controller's models gain nothing from this and can be left unmarked; they +/// resolve by reflection instead. +/// +/// +/// Marking a type also covers everything reachable from it, so a model whose properties are themselves models +/// needs the attribute only at the root. +/// +/// +[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = false)] +public sealed class GenerateValidationCodesAttribute : Attribute; 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..e097325c4453 --- /dev/null +++ b/src/HttpExtensions/README.md @@ -0,0 +1,196 @@ +# HttpExtensions — validation problems + +Every validation failure, wherever it is detected, 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": "Reason is required." }], + "name": [{ + "type": "too_long", + "detail": "Name must be 200 characters or shorter.", + "parameters": { "max": 200 } + }], + "members[1].email": [{ "type": "invalid_email", "detail": "Email is not an address." }] + } +} +``` + +`errors` is keyed by the property **as the client sent it** — camel-cased, `[JsonPropertyName]` +honoured, nested and indexed. `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 whole 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.")), +]); +``` + +Domain errors implementing `IValidationError` render through the same document — see +`ValidationErrorTypedResultsExtensions` in Core, which is the one layer that can see both +`IValidationError` and this project. + +## Attribute failures + +DataAnnotations records a message and throws away the constraint that produced it: the `200` in +`[StringLength(200)]` is gone by the time the failure is reported. +[`ValidationCodeMap`](ValidationCodeMap.cs) recovers it, and +[`ValidationProblemFactory`](ValidationProblemFactory.cs) turns the result into the document above. + +Nothing here validates anything. The framework decides whether a value is valid; this only names +what it found. A path the map does not recognise is still reported, as +`ValidationCodes.Invalid` with its original message — a 400 with an empty `errors` map would tell a +client less than nothing. + +### Two ways a code is resolved + +| | Detection | Lookup | Trimming / AOT | +| --- | --- | --- | --- | +| **MVC controllers** | model binding fills `ModelState` | reflection over the request model | not supported — see below | +| **Minimal APIs** | `AddValidation()` | map generated at build time | supported | + +`TryResolve` prefers a generated map and falls back to reflection. `TryResolveRegistered` reads +only the generated map and never reflects; that is the entry point for anything that must survive +publishing. + +### Opting a model into the generated map + +Mark the root. Everything reachable from it is covered, so nested models need nothing. + +```csharp +[GenerateValidationCodes] +public sealed class CreateThingRequestModel +{ + [Required] + public string? Reason { get; init; } + + [StringLength(200)] + public string? Name { get; init; } +} +``` + +The framework's `[ValidatableType]` works as a trigger too, since a minimal API being validated +already carries it. + +## How the generator identifies a failure + +For a property with **one** constraint, the path alone is enough — an error on `name` can only have +come from the one attribute there. No message is involved, so framework wording is irrelevant. This +covers the large majority of properties. + +For a property with **several**, the generator reconstructs each attribute and asks it how it words +itself: + +```csharp +new("required", static name => new RequiredAttribute().FormatErrorMessage(name)), +new("too_long", null, [new("max", 50)]), +``` + +Asking beats storing a copy: a framework release that rewords a message moves both sides together. + +`MaxLengthAttribute`, `MinLengthAttribute` and `CompareAttribute` have +`[RequiresUnreferencedCode]` constructors, so generated code cannot build them to ask. Only *n − 1* +candidates need identifying though, so one is left as the fallback — above, `required` identifies +itself and anything else on that property must be the length failure. `BWVAL001` warns when two +constraints on one property are both unaskable, because then the choice would be a guess. + +## Why not the built-in validation generator + +ASP.NET Core ships its own validation source generator behind `AddValidation()`, and this project +uses it: it does the detection, the graph walking, the collections, the cycle detection. What it +does not do is name the failure. Its 400 looks like this: + +```json +{ + "title": "One or more validation errors occurred.", + "errors": { + "Reason": ["The Reason field is required."], + "Seats": ["The field Seats must be between 1 and 100."] + } +} +``` + +A `Dictionary` of English prose, keyed by the CLR name. There is no code to +switch on and no `200` to render a localized message from — the constraint that produced each +sentence was read and then discarded. That is the gap, and it is not a gap a configuration setting +closes: + +- **A generator cannot consume another generator's output.** Roslyn does not feed generated source + back in as input, so ours cannot build on theirs. Vendoring their generator wholesale is the only + way to reuse its walking logic, which means owning it. +- **There is nothing to hook.** Their validation error type carries `Name`, `Path`, `ErrorMessage` + and `Container` — no code, no parameters. The `OnValidationError` callback that could have + enriched errors as they were recorded was `[Experimental]` in .NET 10 and is removed in .NET 11. +- **Their emitted types are not a public contract.** `ValidatableTypeInfo` and friends are being + removed from the public API and emitted as `file` classes, so reading their metadata at runtime + is not a supported seam either. + +So this generator is complementary rather than a replacement. It emits **no validation logic at +all** — just a lookup table saying what each path's constraints were called and what values they +carried, which the framework had at compile time and threw away at runtime. + +### Why a generator rather than reading the attributes at runtime + +Because of trimming. Recovering a constraint by reflection means `Type.GetProperty` and +`GetCustomAttributes` over an open-ended object graph, which the trim analyzer reports as +`IL2070`/`IL2075` — the property or the attribute may not survive publish. There is no annotation +that fixes it, because the graph is not knowable from the signature. Baking the table at build time +is the only way the lookup still works after an AOT publish. + +On the controller surface none of that applies, which is why MVC reflects instead. + +## Why MVC does not use the generator + +`AddControllers()` is annotated: + +> `[RequiresUnreferencedCode("MVC does not currently support trimming or native AOT.")]` + +A generated map for the controller surface would make an un-publishable path look publishable. +The reflective resolver is used there instead and declares itself with +`[RequiresUnreferencedCode]`, so a caller that does care is told at compile time rather than +discovering it after publish. + +This project builds with `IsAotCompatible`, so anything new that reflects without saying so fails +the build. + +## 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. +- Minimal APIs are not migrated. The generated path is tested but no endpoint uses it yet; PAM's + filter still calls `Validator.TryValidateObject`, which is what blocks AOT there. +- `.Produces(400)` is not declared on endpoints, so OpenAPI does + not yet describe the coded shape. diff --git a/src/HttpExtensions/ValidationCodeCandidate.cs b/src/HttpExtensions/ValidationCodeCandidate.cs new file mode 100644 index 000000000000..a814c9902bc5 --- /dev/null +++ b/src/HttpExtensions/ValidationCodeCandidate.cs @@ -0,0 +1,32 @@ +namespace Bit.HttpExtensions; + +/// +/// One code a validated property can report. +/// +/// +/// Formats the message the framework records when this constraint fails, given the property's display name. Used +/// only to tell candidates apart when a property carries more than one constraint. +/// +/// +/// A function rather than a string so the wording is asked of the constraint itself at the moment it is needed, +/// rather than copied at build time and left to drift. Exactly one candidate on a property may leave this null: +/// it is the fallback, taken when no other candidate claims the message. That is what lets a property whose other +/// constraint cannot be constructed under trimming still resolve — the one we can ask about identifies itself, +/// and the remaining failure must be the other. +/// +public sealed record ValidationCodeCandidate( + string Code, + Func? Message = null, + IReadOnlyList>? Parameters = null); + +/// +/// What is known about one validated property: the name it goes by on the wire, the name the framework puts in +/// its messages, and the codes it can report. +/// +/// +/// The path as the client sent it, with [] where an index belongs — members[].email. +/// +public sealed record ValidationCodeEntry( + string WirePath, + string DisplayName, + IReadOnlyList Candidates); diff --git a/src/HttpExtensions/ValidationCodeMap.cs b/src/HttpExtensions/ValidationCodeMap.cs new file mode 100644 index 000000000000..97889ebb86c1 --- /dev/null +++ b/src/HttpExtensions/ValidationCodeMap.cs @@ -0,0 +1,401 @@ +using System.Collections; +using System.Collections.Concurrent; +using System.ComponentModel.DataAnnotations; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; +using System.Reflection; +using System.Text; +using System.Text.Json.Nodes; +using System.Text.Json.Serialization; + +namespace Bit.HttpExtensions; + +/// +/// Works out which code and parameters a validation failure should be reported under. +/// +/// +/// +/// The framework decides whether a value is valid and records a message; the constraint that produced it +/// is discarded on the way. This recovers the constraint, so the 200 in [StringLength(200)] reaches +/// the client instead of being buried in prose. +/// +/// +/// There are two ways in. reads a map a generator supplied at build time and +/// touches no reflection, which is what a trimmed or ahead-of-time published app needs. +/// falls back to walking the model when nothing was registered for it, and says so with +/// . Controllers take the second path — MVC declares itself +/// unsupported under trimming, so there is nothing there for a generator to save. +/// +/// +public static class ValidationCodeMap +{ + private static readonly ConcurrentDictionary> _registered = new(); + private static readonly ConcurrentDictionary<(Type Root, string Path), ValidationCodeEntry?> _reflected = new(); + + /// + /// Adds the paths reachable from . Called from generated module initializers, so + /// it runs before any request is served and never while one is being resolved. + /// + /// + /// Keyed by the type the paths are rooted at, not by the path alone. Two request models both carrying a + /// Name would otherwise share one entry and the second would silently take the first's code. + /// + public static void Register(Type rootType, IEnumerable> entries) + { + ArgumentNullException.ThrowIfNull(rootType); + ArgumentNullException.ThrowIfNull(entries); + + _registered[rootType] = entries.ToDictionary(entry => entry.Key, entry => entry.Value, StringComparer.Ordinal); + } + + /// + /// Resolves against the generated map only, never reflecting. + /// + /// + /// The entry point for anything that has to survive trimming. Returns false for a model no generator covered, + /// rather than quietly reaching for reflection that would not be there after publish. + /// + public static bool TryResolveRegistered( + Type rootType, string validationPath, string message, out string wirePath, out ErrorCode error) + { + ArgumentNullException.ThrowIfNull(rootType); + ArgumentNullException.ThrowIfNull(validationPath); + + var (normalized, indices) = Normalize(validationPath); + var entry = _registered.TryGetValue(rootType, out var entries) && entries.TryGetValue(normalized, out var found) + ? found + : null; + + return Complete(entry, message, indices, out wirePath, out error); + } + + /// + /// Resolves against the generated map, falling back to walking when nothing was + /// registered for it. + /// + /// False when the path names nothing constrained, leaving the caller to report it uncoded. + [RequiresUnreferencedCode( + "Falls back to walking the request model's properties when no generated map covers it. Reachable from MVC " + + "model binding, which does not support trimming or native AOT either.")] + public static bool TryResolve( + Type rootType, string validationPath, string message, out string wirePath, out ErrorCode error) + { + ArgumentNullException.ThrowIfNull(rootType); + ArgumentNullException.ThrowIfNull(validationPath); + + if (_registered.ContainsKey(rootType)) + { + return TryResolveRegistered(rootType, validationPath, message, out wirePath, out error); + } + + var (normalized, indices) = Normalize(validationPath); + var entry = _reflected.GetOrAdd((rootType, normalized), static key => Describe(key.Root, key.Path)); + + return Complete(entry, message, indices, out wirePath, out error); + } + + /// Forgets what reflection worked out. For tests that redefine a model between cases. + internal static void Clear() + { + _registered.Clear(); + _reflected.Clear(); + } + + private static bool Complete( + ValidationCodeEntry? entry, string message, List indices, out string wirePath, out ErrorCode error) + { + if (entry is null || Select(entry, message) is not { } candidate) + { + wirePath = string.Empty; + error = null!; + return false; + } + + wirePath = Reindex(entry.WirePath, indices); + error = new ErrorCode(candidate.Code, message, Parameters(candidate)); + return true; + } + + /// + /// Picks the candidate whose constraint would have produced this message, or the one left as the fallback. + /// + /// + /// A property carrying one constraint cannot be ambiguous, so its single candidate is the fallback and answers + /// whatever the framework said — which keeps the common case independent of framework wording entirely. + /// + private static ValidationCodeCandidate? Select(ValidationCodeEntry entry, string message) + { + ValidationCodeCandidate? fallback = null; + + foreach (var candidate in entry.Candidates) + { + if (candidate.Message is null) + { + // At most one candidate is left unformattable; a second would make the choice a guess. + if (fallback is not null) + { + return null; + } + + fallback = candidate; + continue; + } + + if (string.Equals(Safe(candidate.Message, entry.DisplayName), message, StringComparison.Ordinal)) + { + return candidate; + } + } + + return fallback; + } + + /// + /// Asks the constraint how it words itself, or null if it will not say. + /// + /// + /// A custom attribute is free to throw from when handed a + /// name out of context. Losing the tie-break costs the failure its code; letting the exception out would cost + /// the caller its response. + /// + private static string? Safe(Func message, string displayName) + { + try + { + return message(displayName); + } + catch (Exception) + { + return null; + } + } + + private static JsonObject? Parameters(ValidationCodeCandidate candidate) + { + if (candidate.Parameters is not { Count: > 0 } source) + { + return null; + } + + var parameters = new JsonObject(); + foreach (var (name, value) in source) + { + parameters[name] = value switch + { + null => null, + string text => JsonValue.Create(text), + int number => JsonValue.Create(number), + long number => JsonValue.Create(number), + double number => JsonValue.Create(number), + decimal number => JsonValue.Create(number), + bool flag => JsonValue.Create(flag), + _ => JsonValue.Create(Convert.ToString(value, CultureInfo.InvariantCulture)), + }; + } + + return parameters; + } + + /// + /// Follows from to the property it names, and + /// describes what that property constrains. Null when the path leads nowhere, or nowhere constrained. + /// + [RequiresUnreferencedCode("Walks the request model's property graph.")] + private static ValidationCodeEntry? Describe(Type rootType, string normalizedPath) + { + var current = rootType; + var wirePath = new StringBuilder(); + PropertyInfo? property = null; + + foreach (var rawSegment in normalizedPath.Split('.')) + { + var isCollection = rawSegment.EndsWith("[]", StringComparison.Ordinal); + var name = isCollection ? rawSegment[..^2] : rawSegment; + + property = current?.GetProperty( + name, BindingFlags.Public | BindingFlags.Instance | BindingFlags.FlattenHierarchy); + if (property is null) + { + return null; + } + + if (wirePath.Length > 0) + { + wirePath.Append('.'); + } + + wirePath.Append(WireName(property)); + if (isCollection) + { + wirePath.Append("[]"); + } + + var propertyType = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType; + current = isCollection ? ElementType(propertyType) : propertyType; + } + + if (property is null) + { + return null; + } + + var attributes = property.GetCustomAttributes(inherit: true).ToArray(); + if (attributes.Length == 0) + { + return null; + } + + var displayName = property.GetCustomAttribute()?.GetName() ?? property.Name; + var ambiguous = attributes.Length > 1; + + var candidates = new List(attributes.Length); + foreach (var attribute in attributes) + { + if (Translate(attribute) is not { } translated) + { + continue; + } + + // Only worth asking when there is something to tell apart; the constraint builds the string. + candidates.Add(ambiguous + ? translated with { Message = attribute.FormatErrorMessage } + : translated); + } + + return candidates.Count == 0 + ? null + : new ValidationCodeEntry(wirePath.ToString(), displayName, candidates); + } + + /// The code and constraint values for one attribute, or null if we have no name for it. + private static ValidationCodeCandidate? Translate(ValidationAttribute attribute) => attribute switch + { + RequiredAttribute => new(ValidationCodes.Required), + + // One message covers both directions of a two-ended constraint, leaving nothing to tell them apart. The + // client gets both bounds and composes the sentence itself. + StringLengthAttribute { MinimumLength: > 0 } length => new(ValidationCodes.InvalidLength, null, + [new(ValidationParameters.Min, length.MinimumLength), new(ValidationParameters.Max, length.MaximumLength)]), + StringLengthAttribute length => new(ValidationCodes.TooLong, null, + [new(ValidationParameters.Max, length.MaximumLength)]), + + MaxLengthAttribute max => new(ValidationCodes.TooLong, null, [new(ValidationParameters.Max, max.Length)]), + MinLengthAttribute min => new(ValidationCodes.TooShort, null, [new(ValidationParameters.Min, min.Length)]), + + RangeAttribute range => new(ValidationCodes.OutOfRange, null, + [new(ValidationParameters.Min, range.Minimum), new(ValidationParameters.Max, range.Maximum)]), + + EmailAddressAttribute => new(ValidationCodes.InvalidEmail), + CompareAttribute compare => new(ValidationCodes.MustMatch, null, + [new(ValidationParameters.Other, compare.OtherProperty)]), + RegularExpressionAttribute expression => new(ValidationCodes.InvalidFormat, null, + [new(ValidationParameters.Pattern, expression.Pattern)]), + + UrlAttribute or PhoneAttribute or CreditCardAttribute => new(ValidationCodes.InvalidFormat), + + _ => null, + }; + + /// The name the client sent: the JSON name when renamed, otherwise the camel-cased property. + [RequiresUnreferencedCode("Reads the property's JSON naming attribute.")] + private static string WireName(PropertyInfo property) + { + if (property.GetCustomAttribute()?.Name is { } renamed) + { + return renamed; + } + + return CamelCase(property.Name); + } + + internal static string CamelCase(string name) => + name.Length > 0 && char.IsUpper(name[0]) ? char.ToLowerInvariant(name[0]) + name[1..] : name; + + [RequiresUnreferencedCode("Inspects the collection's interfaces to find its element type.")] + private static Type? ElementType(Type type) + { + if (type.IsArray) + { + return type.GetElementType(); + } + + foreach (var candidate in type.GetInterfaces().Append(type)) + { + if (candidate.IsGenericType && candidate.GetGenericTypeDefinition() == typeof(IEnumerable<>)) + { + return candidate.GetGenericArguments()[0]; + } + } + + return typeof(IEnumerable).IsAssignableFrom(type) ? typeof(object) : null; + } + + /// + /// Flattens Members[0].Email to Members[].Email, keeping the indices so the reported path can + /// name the element that actually failed. + /// + private static (string Normalized, List Indices) Normalize(string path) + { + var indices = new List(); + if (!path.Contains('[', StringComparison.Ordinal)) + { + return (path, indices); + } + + var normalized = new StringBuilder(path.Length); + for (var i = 0; i < path.Length; i++) + { + if (path[i] != '[') + { + normalized.Append(path[i]); + continue; + } + + var close = path.IndexOf(']', i); + if (close < 0) + { + normalized.Append(path, i, path.Length - i); + break; + } + + if (int.TryParse(path.AsSpan(i + 1, close - i - 1), out var index)) + { + indices.Add(index); + normalized.Append("[]"); + } + else + { + normalized.Append(path, i, close - i + 1); + } + + i = close; + } + + return (normalized.ToString(), indices); + } + + /// Puts the indices taken out by back into the wire path, in order. + private static string Reindex(string wirePath, List indices) + { + if (indices.Count == 0) + { + return wirePath; + } + + var result = new StringBuilder(wirePath.Length + (indices.Count * 2)); + var next = 0; + for (var i = 0; i < wirePath.Length; i++) + { + if (wirePath[i] == '[' && i + 1 < wirePath.Length && wirePath[i + 1] == ']' && next < indices.Count) + { + result.Append('[').Append(indices[next++]).Append(']'); + i++; + continue; + } + + result.Append(wirePath[i]); + } + + return result.ToString(); + } +} 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/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..53dd47afe4c1 --- /dev/null +++ b/src/HttpExtensions/ValidationProblemFactory.cs @@ -0,0 +1,141 @@ +using System.Diagnostics.CodeAnalysis; +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. +/// +/// +/// The framework detects; this only names what it found. Every failure in the model state reaches the document, +/// coded when knows the path 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. + /// + /// + /// The types model state was bound from — the action's parameters. Paths are looked up under each in turn, + /// because a model state key carries no indication of which parameter it came from. + /// + [RequiresUnreferencedCode( + "Recovers validation codes by walking the request model's properties. Takes a ModelStateDictionary, so it " + + "is only reachable from MVC, which does not support trimming or native AOT.")] + public static BitwardenValidationProblemDetails FromModelState( + ModelStateDictionary modelState, + IReadOnlyList rootTypes, + string title = "One or more validation errors occurred.", + string type = "validation_error", + int statusCode = StatusCodes.Status400BadRequest) + { + ArgumentNullException.ThrowIfNull(modelState); + ArgumentNullException.ThrowIfNull(rootTypes); + + var errors = new Dictionary>(StringComparer.Ordinal); + + foreach (var (key, entry) in modelState) + { + if (entry.ValidationState != ModelValidationState.Invalid || entry.Errors.Count == 0) + { + continue; + } + + foreach (var modelError in entry.Errors) + { + var message = Describe(modelError); + + if (!TryResolveAny(rootTypes, key, message, out var wirePath, out var error)) + { + wirePath = ToWireName(key); + error = new ErrorCode(ValidationCodes.Invalid, message); + } + + if (!errors.TryGetValue(wirePath, out var codes)) + { + codes = []; + errors[wirePath] = codes; + } + + codes.Add(error); + } + } + + return new BitwardenValidationProblemDetails + { + Status = statusCode, + Title = title, + Type = type, + Errors = errors.ToDictionary(pair => pair.Key, pair => pair.Value.ToArray(), StringComparer.Ordinal), + }; + } + + /// + /// Looks the path up under each candidate root, taking the first that claims it. + /// + /// + /// A miss is ordinary rather than exceptional: an action taking a route id alongside a body has one root + /// that knows the path and one that does not. + /// + [RequiresUnreferencedCode("Resolves through ValidationCodeMap, which walks the request model.")] + private static bool TryResolveAny( + IReadOnlyList rootTypes, string key, string message, out string wirePath, out ErrorCode error) + { + for (var i = 0; i < rootTypes.Count; i++) + { + if (ValidationCodeMap.TryResolve(rootTypes[i], key, message, out wirePath, out error)) + { + return true; + } + } + + wirePath = string.Empty; + error = null!; + return false; + } + + /// + /// 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."; + + /// + /// Best-effort wire name for a path the map does not know, matching the camel casing the serializer applies + /// to everything else. Only reached for an unmapped path, where the alternative is reporting the CLR name. + /// + 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) + { + if (segment.Length == 0 || !char.IsUpper(segment[0])) + { + return segment; + } + + return char.ToLowerInvariant(segment[0]) + segment[1..]; + } +} 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..293844437610 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,37 @@ 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. + /// + /// + /// Roots are the action's own parameter types. Model state names the property a failure sits on but not the + /// argument it was bound from, so the lookup tries each until one claims the path. + /// + /// + protected static IActionResult? TryCodedProblem(ActionExecutingContext context) + { + var featureService = context.HttpContext.RequestServices.GetService(); + if (featureService?.IsEnabled(FeatureFlagKeys.CodedValidationProblems) != true) + { + return null; + } + + var rootTypes = context.ActionDescriptor.Parameters + .Select(parameter => parameter.ParameterType) + .ToArray(); + + return new ObjectResult(ValidationProblemFactory.FromModelState(context.ModelState, rootTypes)) + { + 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 deleted file mode 100644 index 17b229e723ed..000000000000 --- a/test/Api.Test/AdminConsole/Controllers/ValidationErrorTypedResultsExtensionsTests.cs +++ /dev/null @@ -1,35 +0,0 @@ -using Bit.Core.AdminConsole.Utilities.v2.Validation; -using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.HttpResults; -using Xunit; -using static Microsoft.AspNetCore.Http.HttpResults.BitwardenTypedResultsExtensions; - -namespace Bit.Api.Test.AdminConsole.Controllers; - -public class ValidationErrorTypedResultsExtensionsTests -{ - [Fact] - public void BitwardenValidationProblem_WithValidationError_KeysByPropertyName() - { - var validationError = new TestValidationError("email", "Member not claimed", "memberNotClaimed"); - - var result = TypedResults.BitwardenValidationProblem(validationError); - - Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); - var errors = Assert.IsType>(result.ProblemDetails.Extensions["errors"]); - var entry = Assert.Single(errors); - Assert.Equal("email", entry.Key); - var error = Assert.Single(entry.Value); - Assert.Equal("memberNotClaimed", error.Type); - Assert.Equal("Member not claimed", error.Detail); - } - - [Fact] - public void BitwardenValidationProblem_WithNullValidationError_Throws() - { - Assert.Throws(() => - TypedResults.BitwardenValidationProblem(null!)); - } - - private sealed record TestValidationError(string PropertyName, string Message, string Type) : IValidationError; -} 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/Core.Test/AdminConsole/Utilities/v2/Validation/ValidationErrorTypedResultsExtensionsTests.cs b/test/Core.Test/AdminConsole/Utilities/v2/Validation/ValidationErrorTypedResultsExtensionsTests.cs new file mode 100644 index 000000000000..3dab08d45bc9 --- /dev/null +++ b/test/Core.Test/AdminConsole/Utilities/v2/Validation/ValidationErrorTypedResultsExtensionsTests.cs @@ -0,0 +1,92 @@ +using Bit.Core.AdminConsole.Utilities.v2.Validation; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.HttpResults; +using Xunit; + +namespace Bit.Core.Test.AdminConsole.Utilities.v2.Validation; + +public class ValidationErrorTypedResultsExtensionsTests +{ + [Fact] + public void BitwardenValidationProblem_WithValidationError_KeysByPropertyName() + { + var validationError = new TestValidationError("email", "Member not claimed", "memberNotClaimed"); + + var result = TypedResults.BitwardenValidationProblem(validationError); + + Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); + var errors = result.ProblemDetails.Errors; + var entry = Assert.Single(errors); + Assert.Equal("email", entry.Key); + var error = Assert.Single(entry.Value); + Assert.Equal("memberNotClaimed", error.Type); + Assert.Equal("Member not claimed", error.Detail); + } + + [Fact] + public void BitwardenValidationProblem_WithNullValidationError_Throws() + { + Assert.Throws(() => + TypedResults.BitwardenValidationProblem((IValidationError)null!)); + } + + [Fact] + public void BitwardenValidationProblem_WithStatusCode_CarriesTheProblemOnThatStatus() + { + var validationError = new TestValidationError("code", "Access is already active.", "accessAlreadyActive"); + + var result = TypedResults.BitwardenValidationProblem( + validationError, + title: "The request conflicts with the current state.", + type: "conflict_error", + statusCode: StatusCodes.Status409Conflict); + + Assert.Equal(StatusCodes.Status409Conflict, result.StatusCode); + Assert.Equal("The request conflicts with the current state.", result.ProblemDetails.Title); + Assert.Equal("conflict_error", result.ProblemDetails.Type); + } + + [Fact] + public void BitwardenValidationProblem_WithErrorsOnDifferentProperties_KeysEachSeparately() + { + IValidationError[] validationErrors = + [ + new TestValidationError("email", "Email is required.", "required"), + new TestValidationError("name", "Name is required.", "required"), + ]; + + var result = TypedResults.BitwardenValidationProblem(validationErrors); + + var errors = result.ProblemDetails.Errors; + Assert.Equal(2, errors.Count); + Assert.Equal("required", Assert.Single(errors["email"]).Type); + Assert.Equal("required", Assert.Single(errors["name"]).Type); + } + + [Fact] + public void BitwardenValidationProblem_WithErrorsOnOneProperty_CollectsThemUnderIt() + { + // A model that fails several ways at once reports every failure, rather than the last one winning. + IValidationError[] validationErrors = + [ + new TestValidationError("password", "Password is too short.", "tooShort"), + new TestValidationError("password", "Password needs a digit.", "missingDigit"), + ]; + + var result = TypedResults.BitwardenValidationProblem(validationErrors); + + var errors = result.ProblemDetails.Errors; + var entry = Assert.Single(errors); + Assert.Equal("password", entry.Key); + Assert.Equal(["tooShort", "missingDigit"], entry.Value.Select(error => error.Type)); + } + + [Fact] + public void BitwardenValidationProblem_WithNullValidationErrors_Throws() + { + Assert.Throws(() => + TypedResults.BitwardenValidationProblem((IEnumerable)null!)); + } + + private sealed record TestValidationError(string PropertyName, string Message, string Type) : IValidationError; +} diff --git a/test/HttpExtensions.RoundTrip.Test/GeneratedCodeMapTests.cs b/test/HttpExtensions.RoundTrip.Test/GeneratedCodeMapTests.cs new file mode 100644 index 000000000000..37d8c1afbedb --- /dev/null +++ b/test/HttpExtensions.RoundTrip.Test/GeneratedCodeMapTests.cs @@ -0,0 +1,153 @@ +using System.ComponentModel.DataAnnotations; +using System.Text.Json.Serialization; +using Microsoft.Extensions.Validation; +using Xunit; + +namespace Bit.HttpExtensions.RoundTrip.Test; + +/// +/// The generated map, which is what a trimmed or ahead-of-time published minimal API resolves against. +/// +/// +/// These go through rather than +/// , so a regression that made the generated path fall back to +/// reflection fails here instead of at publish time. +/// +public class GeneratedCodeMapTests +{ + [Fact] + public void AConstrainedProperty_ResolvesFromTheGeneratedMap() + { + Assert.True(ValidationCodeMap.TryResolveRegistered( + typeof(GeneratedModel), "Name", "whatever the framework said", out var wirePath, out var error)); + + Assert.Equal("name", wirePath); + Assert.Equal(ValidationCodes.TooLong, error.Type); + Assert.Equal(200, (int)error.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void ARenamedProperty_ReportsTheNameTheClientSent() + { + ValidationCodeMap.TryResolveRegistered(typeof(GeneratedModel), "RenamedOnTheWire", "m", out var wirePath, out _); + + Assert.Equal("tag", wirePath); + } + + [Fact] + public void ANestedCollection_KeepsTheIndexItFailedAt() + { + Assert.True(ValidationCodeMap.TryResolveRegistered( + typeof(GeneratedModel), "Members[2].Email", "m", out var wirePath, out var error)); + + Assert.Equal("members[2].email", wirePath); + Assert.Equal(ValidationCodes.Required, error.Type); + } + + [Fact] + public void ATwoEndedLengthConstraint_ReportsOneCodeCarryingBothBounds() + { + ValidationCodeMap.TryResolveRegistered(typeof(GeneratedModel), "Bounded", "m", out _, out var error); + + 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 APropertyThatCanFailTwoWays_ResolvesByAskingTheConstraintHowItWordsItself() + { + // The generated map reconstructs the attribute and asks it, rather than holding a copy of its wording. + var required = new RequiredAttribute().FormatErrorMessage("AccessCode"); + var tooLong = new StringLengthAttribute(25).FormatErrorMessage("AccessCode"); + + ValidationCodeMap.TryResolveRegistered(typeof(GeneratedModel), "AccessCode", required, out _, out var missing); + ValidationCodeMap.TryResolveRegistered(typeof(GeneratedModel), "AccessCode", tooLong, out _, out var overlong); + + Assert.Equal(ValidationCodes.Required, missing.Type); + Assert.Equal(ValidationCodes.TooLong, overlong.Type); + Assert.Equal(25, (int)overlong.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void AConstraintThatCannotBeReconstructed_IsIdentifiedByElimination() + { + // MaxLength's constructor is [RequiresUnreferencedCode], so generated code cannot ask it for its wording. + // Required identifies itself, and the remaining failure must be the other one. + var required = new RequiredAttribute().FormatErrorMessage("Capped"); + + ValidationCodeMap.TryResolveRegistered(typeof(GeneratedModel), "Capped", required, out _, out var missing); + ValidationCodeMap.TryResolveRegistered(typeof(GeneratedModel), "Capped", "anything else", out _, out var other); + + Assert.Equal(ValidationCodes.Required, missing.Type); + Assert.Equal(ValidationCodes.TooLong, other.Type); + Assert.Equal(50, (int)other.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void AnExplicitErrorMessage_IsWhatTheConstraintIsAskedToProduce() + { + ValidationCodeMap.TryResolveRegistered( + typeof(GeneratedModel), "Custom", "Custom is required, please.", out _, out var error); + + Assert.Equal(ValidationCodes.Required, error.Type); + } + + [Fact] + public void TheFrameworksOwnMarker_AlsoTriggersGeneration() + { + // A minimal API being validated by AddValidation() already carries [ValidatableType], so it should not + // have to carry ours as well. This is also what pins the attribute's metadata name, which moved + // namespaces in .NET 10 GA. + Assert.True(ValidationCodeMap.TryResolveRegistered( + typeof(ValidatableTypeModel), "Reason", "m", out var wirePath, out var error)); + + Assert.Equal("reason", wirePath); + Assert.Equal(ValidationCodes.Required, error.Type); + } + + [Fact] + public void AnUnmarkedModel_IsNotInTheGeneratedMap() => + // Only marked roots are generated; an unmarked one resolves by reflection instead. + Assert.False(ValidationCodeMap.TryResolveRegistered(typeof(RoundTripModel), "Reason", "m", out _, out _)); +} + +[ValidatableType] +public sealed class ValidatableTypeModel +{ + [Required] + public string? Reason { get; set; } +} + +public sealed class GeneratedMember +{ + [Required] + public string? Email { get; set; } +} + +[GenerateValidationCodes] +public sealed class GeneratedModel +{ + [StringLength(200)] + public string? Name { get; set; } + + [StringLength(200, MinimumLength = 5)] + public string? Bounded { get; set; } + + [Required] + [StringLength(25)] + public string? AccessCode { get; set; } + + [Required] + [MaxLength(50)] + public string? Capped { get; set; } + + [Required(ErrorMessage = "Custom is required, please.")] + public string? Custom { get; set; } + + [JsonPropertyName("tag")] + [Required] + public string? RenamedOnTheWire { get; set; } + + public List? Members { get; set; } +} diff --git a/test/HttpExtensions.RoundTrip.Test/HttpExtensions.RoundTrip.Test.csproj b/test/HttpExtensions.RoundTrip.Test/HttpExtensions.RoundTrip.Test.csproj new file mode 100644 index 000000000000..d3ce94c06f9a --- /dev/null +++ b/test/HttpExtensions.RoundTrip.Test/HttpExtensions.RoundTrip.Test.csproj @@ -0,0 +1,33 @@ + + + + false + + $(NoWarn);ASP0029 + + + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + diff --git a/test/HttpExtensions.RoundTrip.Test/ValidationRoundTripTests.cs b/test/HttpExtensions.RoundTrip.Test/ValidationRoundTripTests.cs new file mode 100644 index 000000000000..448da906bb49 --- /dev/null +++ b/test/HttpExtensions.RoundTrip.Test/ValidationRoundTripTests.cs @@ -0,0 +1,229 @@ +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.RoundTrip.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_IsKeyedByTheNameTheClientSent() + { + var errors = await PostAsync(new { reason = "r" }); + + Assert.True(errors.TryGetProperty("tag", out _)); + Assert.False(errors.TryGetProperty("renamedOnTheWire", 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; + } + + var rootTypes = context.ActionDescriptor.Parameters + .Select(parameter => parameter.ParameterType) + .ToArray(); + + context.Result = new ObjectResult( + ValidationProblemFactory.FromModelState(context.ModelState, rootTypes)) + { + 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.RoundTrip.Test/packages.lock.json b/test/HttpExtensions.RoundTrip.Test/packages.lock.json new file mode 100644 index 000000000000..a6112256b58c --- /dev/null +++ b/test/HttpExtensions.RoundTrip.Test/packages.lock.json @@ -0,0 +1,113 @@ +{ + "version": 1, + "dependencies": { + "net10.0": { + "coverlet.collector": { + "type": "Direct", + "requested": "[6.0.0, )", + "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, )", + "resolved": "18.0.1", + "contentHash": "WNpu6vI2rA0pXY4r7NKxCN16XRWl5uHu6qjuyVLoDo6oYEggIQefrMjkRuibQHm/NslIUNCcKftvoWAN80MSAg==", + "dependencies": { + "Microsoft.CodeCoverage": "18.0.1", + "Microsoft.TestPlatform.TestHost": "18.0.1" + } + }, + "xunit": { + "type": "Direct", + "requested": "[2.6.6, )", + "resolved": "2.6.6", + "contentHash": "MAbOOMtZIKyn2lrAmMlvhX0BhDOX/smyrTB+8WTXnSKkrmTGBS2fm8g1PZtHBPj91Dc5DJA7fY+/81TJ/yUFZw==", + "dependencies": { + "xunit.analyzers": "1.10.0", + "xunit.assert": "2.6.6", + "xunit.core": "[2.6.6]" + } + }, + "xunit.runner.visualstudio": { + "type": "Direct", + "requested": "[2.5.6, )", + "resolved": "2.5.6", + "contentHash": "CW6uhMXNaQQNMSG1IWhHkBT+V5eqHqn7MP0zfNMhU9wS/sgKX7FGL3rzoaUgt26wkY3bpf7pDVw3IjXhwfiP4w==" + }, + "Microsoft.CodeCoverage": { + "type": "Transitive", + "resolved": "18.0.1", + "contentHash": "O+utSr97NAJowIQT/OVp3Lh9QgW/wALVTP4RG1m2AfFP4IyJmJz0ZBmFJUsRQiAPgq6IRC0t8AAzsiPIsaUDEA==" + }, + "Microsoft.TestPlatform.ObjectModel": { + "type": "Transitive", + "resolved": "18.0.1", + "contentHash": "qT/mwMcLF9BieRkzOBPL2qCopl8hQu6A1P7JWAoj/FMu5i9vds/7cjbJ/LLtaiwWevWLAeD5v5wjQJ/l6jvhWQ==" + }, + "Microsoft.TestPlatform.TestHost": { + "type": "Transitive", + "resolved": "18.0.1", + "contentHash": "uDJKAEjFTaa2wHdWlfo6ektyoh+WD4/Eesrwb4FpBFKsLGehhACVnwwTI4qD3FrIlIEPlxdXg3SyrYRIcO+RRQ==", + "dependencies": { + "Microsoft.TestPlatform.ObjectModel": "18.0.1", + "Newtonsoft.Json": "13.0.3" + } + }, + "Newtonsoft.Json": { + "type": "Transitive", + "resolved": "13.0.3", + "contentHash": "HrC5BXdl00IP9zeV+0Z848QWPAoCr9P3bDEZguI+gkLcBKAOxix/tLEAAHC+UvDNPv4a2d18lOReHMOagPa+zQ==" + }, + "xunit.abstractions": { + "type": "Transitive", + "resolved": "2.0.3", + "contentHash": "pot1I4YOxlWjIb5jmwvvQNbTrZ3lJQ+jUGkGjWE3hEFM0l5gOnBWS+H3qsex68s5cO52g+44vpGzhAt+42vwKg==" + }, + "xunit.analyzers": { + "type": "Transitive", + "resolved": "1.10.0", + "contentHash": "Lw8CiDy5NaAWcO6keqD7iZHYUTIuCOcoFrUHw5Sv84ITZ9gFeDybdkVdH0Y2maSlP9fUjtENyiykT44zwFQIHA==" + }, + "xunit.assert": { + "type": "Transitive", + "resolved": "2.6.6", + "contentHash": "74Cm9lAZOk5TKCz2MvCBCByKsS23yryOKDIMxH3XRDHXmfGM02jKZWzRA7g4mGB41GnBnv/pcWP3vUYkrCtEcg==" + }, + "xunit.core": { + "type": "Transitive", + "resolved": "2.6.6", + "contentHash": "tqi7RfaNBqM7t8zx6QHryuBPzmotsZXKGaWnopQG2Ez5UV7JoWuyoNdT6gLpDIcKdGYey6YTXJdSr9IXDMKwjg==", + "dependencies": { + "xunit.extensibility.core": "[2.6.6]", + "xunit.extensibility.execution": "[2.6.6]" + } + }, + "xunit.extensibility.core": { + "type": "Transitive", + "resolved": "2.6.6", + "contentHash": "ty6VKByzbx4Toj4/VGJLEnlmOawqZiMv0in/tLju+ftA+lbWuAWDERM+E52Jfhj4ZYHrAYVa14KHK5T+dq0XxA==", + "dependencies": { + "xunit.abstractions": "2.0.3" + } + }, + "xunit.extensibility.execution": { + "type": "Transitive", + "resolved": "2.6.6", + "contentHash": "UDjIVGj2TepVKN3n32/qXIdb3U6STwTb9L6YEwoQO2A8OxiJS5QAVv2l1aT6tDwwv/9WBmm8Khh/LyHALipcng==", + "dependencies": { + "xunit.extensibility.core": "[2.6.6]" + } + }, + "httpextensions": { + "type": "Project" + } + } + } +} \ No newline at end of file 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/ValidationCodeMapTests.cs b/test/HttpExtensions.Test/ValidationCodeMapTests.cs new file mode 100644 index 000000000000..192fd2888070 --- /dev/null +++ b/test/HttpExtensions.Test/ValidationCodeMapTests.cs @@ -0,0 +1,181 @@ +using System.ComponentModel.DataAnnotations; +using System.Text.Json.Serialization; +using Xunit; + +namespace Bit.HttpExtensions.Test; + +public class ValidationCodeMapTests +{ + private sealed class Member + { + [Required] + public string? Email { get; set; } + } + + private sealed class Address + { + [Required] + public string? Line { get; set; } + } + + private sealed class Owner + { + public List
? Addresses { get; set; } + } + + private sealed class Root + { + [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; } + + [JsonPropertyName("tag")] + [Required] + public string? RenamedOnTheWire { get; set; } + + public string? Unconstrained { get; set; } + + public List? Members { get; set; } + + public Owner? Owner { get; set; } + } + + private sealed class OtherRoot + { + [Required] + public string? Name { get; set; } + } + + [Fact] + public void AConstrainedProperty_ResolvesToItsCodeAndWireName() + { + Assert.True(ValidationCodeMap.TryResolve(typeof(Root), "Name", "too long", out var wirePath, out var error)); + + Assert.Equal("name", wirePath); + Assert.Equal(ValidationCodes.TooLong, error.Type); + Assert.Equal("too long", error.Detail); + Assert.Equal(200, (int)error.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void ATwoEndedConstraint_ReportsOneCodeCarryingBothBounds() + { + ValidationCodeMap.TryResolve(typeof(Root), "Bounded", "m", out _, out var error); + + 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 ARange_CarriesBothBounds() + { + ValidationCodeMap.TryResolve(typeof(Root), "Seats", "m", out _, out var error); + + 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 ARenamedProperty_ReportsTheNameTheClientSent() + { + ValidationCodeMap.TryResolve(typeof(Root), "RenamedOnTheWire", "m", out var wirePath, out _); + + Assert.Equal("tag", wirePath); + } + + [Fact] + public void TheSamePathUnderTwoRoots_ResolvesSeparately() + { + // Keying on the path alone would let one request model silently take the other's code. + ValidationCodeMap.TryResolve(typeof(Root), "Name", "m", out _, out var fromRoot); + ValidationCodeMap.TryResolve(typeof(OtherRoot), "Name", "m", out _, out var fromOther); + + Assert.Equal(ValidationCodes.TooLong, fromRoot.Type); + Assert.Equal(ValidationCodes.Required, fromOther.Type); + } + + [Fact] + public void ACollectionElement_KeepsTheIndexItFailedAt() + { + Assert.True(ValidationCodeMap.TryResolve( + typeof(Root), "Members[3].Email", "required", out var wirePath, out var error)); + + Assert.Equal("members[3].email", wirePath); + Assert.Equal(ValidationCodes.Required, error.Type); + } + + [Fact] + public void ACollectionNestedUnderAProperty_RestoresItsIndex() + { + Assert.True(ValidationCodeMap.TryResolve( + typeof(Root), "Owner.Addresses[2].Line", "required", out var wirePath, out _)); + + Assert.Equal("owner.addresses[2].line", wirePath); + } + + [Fact] + public void APropertyThatCanFailTwoWays_ResolvesByTheMessageTheFrameworkRecorded() + { + // Asking the framework for the message rather than holding a copy of it is what makes this survive a + // reword: both sides move together. + var required = new RequiredAttribute().FormatErrorMessage("AccessCode"); + var tooLong = new StringLengthAttribute(25).FormatErrorMessage("AccessCode"); + + ValidationCodeMap.TryResolve(typeof(Root), "AccessCode", required, out _, out var missing); + ValidationCodeMap.TryResolve(typeof(Root), "AccessCode", tooLong, out _, out var overlong); + + Assert.Equal(ValidationCodes.Required, missing.Type); + Assert.Equal(ValidationCodes.TooLong, overlong.Type); + Assert.Equal(25, (int)overlong.Parameters![ValidationParameters.Max]!); + } + + [Fact] + public void AnAmbiguousPathWithAMessageNoCandidateClaims_ResolvesToNothing() => + // Reporting one of the two at random would be worse than reporting it uncoded. + Assert.False(ValidationCodeMap.TryResolve(typeof(Root), "AccessCode", "reworded upstream", out _, out _)); + + [Fact] + public void AnUnambiguousPath_IgnoresTheMessageEntirely() + { + // The single-constraint case is what keeps most of this independent of framework wording. + Assert.True(ValidationCodeMap.TryResolve(typeof(Root), "Name", "anything at all", out _, out var error)); + + Assert.Equal(ValidationCodes.TooLong, error.Type); + } + + [Fact] + public void AnUnconstrainedProperty_ResolvesToNothing() => + Assert.False(ValidationCodeMap.TryResolve(typeof(Root), "Unconstrained", "m", out _, out _)); + + [Fact] + public void APathNamingNoProperty_ResolvesToNothing() => + Assert.False(ValidationCodeMap.TryResolve(typeof(Root), "NotAProperty", "m", out _, out _)); + + [Fact] + public void APathThroughNothing_ResolvesToNothing() => + Assert.False(ValidationCodeMap.TryResolve(typeof(Root), "Name.Deeper", "m", out _, out _)); + + [Fact] + public void ParametersAreNotSharedBetweenResolutions() + { + ValidationCodeMap.TryResolve(typeof(Root), "Name", "m", out _, out var first); + ValidationCodeMap.TryResolve(typeof(Root), "Name", "m", out _, out var second); + + Assert.NotSame(first.Parameters, second.Parameters); + } + + [Fact] + public void NullRootType_Throws() => + Assert.Throws(() => ValidationCodeMap.TryResolve(null!, "Name", "m", out _, out _)); +} 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..1f4c65b9ec5f --- /dev/null +++ b/test/HttpExtensions.Test/ValidationProblemFactoryTests.cs @@ -0,0 +1,109 @@ +using System.ComponentModel.DataAnnotations; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Xunit; + +namespace Bit.HttpExtensions.Test; + +public class ValidationProblemFactoryTests +{ + private sealed class Known + { + [StringLength(200)] + public string? Name { get; set; } + } + + 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 AMappedPath_IsReportedWithItsCode() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Name", "Name is too long.")), [typeof(Known)]); + + var error = Assert.Single(problem.Errors["name"]); + Assert.Equal(ValidationCodes.TooLong, error.Type); + Assert.Equal("Name is too long.", error.Detail); + } + + [Fact] + public void AnUnmappedPath_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.")), [typeof(Known)]); + + var error = Assert.Single(problem.Errors["mystery"]); + Assert.Equal(ValidationCodes.Invalid, error.Type); + Assert.Equal("Something was wrong.", error.Detail); + } + + [Fact] + public void AnUnmappedNestedPath_IsCamelCasedSegmentBySegment() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Owner.PostCode", "Bad.")), [typeof(Known)]); + + Assert.True(problem.Errors.ContainsKey("owner.postCode")); + } + + [Fact] + public void AModelLevelFailure_KeepsTheEmptyKey() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState((string.Empty, "Body is empty.")), [typeof(Known)]); + + var error = Assert.Single(problem.Errors[string.Empty]); + Assert.Equal(ValidationCodes.Invalid, error.Type); + } + + [Fact] + public void SeveralFailuresOnOnePath_AreCollectedUnderIt() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Name", "Name is too long."), ("Name", "Name is also wrong.")), [typeof(Known)]); + + Assert.Equal(2, problem.Errors["name"].Length); + } + + [Fact] + public void NoRootTypes_ReportsEverythingUncodedRatherThanThrowing() + { + var problem = ValidationProblemFactory.FromModelState(ModelState(("Name", "Name is too long.")), []); + + Assert.Equal(ValidationCodes.Invalid, Assert.Single(problem.Errors["name"]).Type); + } + + [Fact] + public void ValidModelState_ProducesAnEmptyErrorsMap() => + Assert.Empty(ValidationProblemFactory.FromModelState(new ModelStateDictionary(), [typeof(Known)]).Errors); + + [Fact] + public void TheDocumentCarriesTheProblemMembers() + { + var problem = ValidationProblemFactory.FromModelState( + ModelState(("Name", "Name is too long.")), [typeof(Known)]); + + 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!, [typeof(Known)])); + + [Fact] + public void NullRootTypes_Throws() => + Assert.Throws(() => + ValidationProblemFactory.FromModelState(new ModelStateDictionary(), null!)); +} 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, )",