Give every validation failure one coded, localizable problem shape

bitwarden-server.slnx

@@ -28,6 +28,7 @@
     <Project Path="src/Events/Events.csproj" />
     <Project Path="src/EventsProcessor/EventsProcessor.csproj" />
     <Project Path="src/HttpExtensions/HttpExtensions.csproj" />
+    <Project Path="src/HttpExtensions.Generator/HttpExtensions.Generator.csproj" />
     <Project Path="src/Icons/Icons.csproj" />
     <Project Path="src/Identity/Identity.csproj" />
     <Project Path="src/Infrastructure.Dapper/Infrastructure.Dapper.csproj" />
@@ -77,6 +78,7 @@
     <Project Path="test/Events.Test/Events.Test.csproj" />
     <Project Path="test/EventsProcessor.Test/EventsProcessor.Test.csproj" />
     <Project Path="test/HttpExtensions.Test/HttpExtensions.Test.csproj" />
+    <Project Path="test/HttpExtensions.RoundTrip.Test/HttpExtensions.RoundTrip.Test.csproj" />
     <Project Path="test/Icons.Test/Icons.Test.csproj" />
     <Project Path="test/Identity.IntegrationTest/Identity.IntegrationTest.csproj" />
     <Project Path="test/Identity.Test/Identity.Test.csproj" />

src/Api/Utilities/ModelStateValidationFilterAttribute.cs

@@ -14,15 +14,19 @@ public ModelStateValidationFilterAttribute(bool publicApi)
         _publicApi = publicApi;
     }
 
+    /// <remarks>
+    /// 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.
+    /// </remarks>
     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));
     }
 }

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;
 
 /// <summary>
 /// An error tied to a specific request property. Implementing this on an <see cref="Error"/> allows
@@ -10,4 +12,15 @@ public interface IValidationError
     string PropertyName { get; }
     string Message { get; }
     string Type { get; }
+
+    /// <summary>
+    /// The substitutions a client needs to render its own localized message for <see cref="Type"/> — 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.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    JsonObject? Parameters => null;
 }

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;
+
+/// <summary>
+/// Renders <see cref="IValidationError"/>s as the Bitwarden problem response. Lives in Core rather than beside
+/// <c>BitwardenValidationProblem</c> in HttpExtensions because it is the one layer that can see both sides:
+/// HttpExtensions does not reference Core, so it cannot name <see cref="IValidationError"/>.
+/// </summary>
+public static class ValidationErrorTypedResultsExtensions
+{
+    extension(TypedResults)
+    {
+        /// <summary>
+        /// Produces an RFC 7807 problem response keyed by <see cref="IValidationError.PropertyName"/>, with the
+        /// error's <see cref="IValidationError.Type"/> as the i18n code and
+        /// <see cref="IValidationError.Message"/> as the human-readable detail.
+        /// </summary>
+        /// <param name="statusCode">
+        /// 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.
+        /// </param>
+        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);
+        }
+
+        /// <inheritdoc cref="BitwardenValidationProblem(IValidationError, string, string, int)"/>
+        /// <remarks>
+        /// 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.
+        /// </remarks>
+        public static BitwardenValidationProblemResult BitwardenValidationProblem(
+            IEnumerable<IValidationError> 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);
+        }
+    }
+}

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";

src/HttpExtensions.Generator/AttributeTranslation.cs

@@ -0,0 +1,211 @@
+using System.Globalization;
+using Microsoft.CodeAnalysis;
+
+namespace Bit.HttpExtensions.Generator;
+
+/// <summary>One validation attribute, read at compile time and turned into the code it should report.</summary>
+/// <param name="Parameters">Substitution name to the C# expression producing its value.</param>
+/// <param name="Construction">
+/// 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.
+/// </param>
+internal sealed record AttributeTranslation(
+    string Code,
+    IReadOnlyList<KeyValuePair<string, string>> 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;
+    }
+
+    /// <summary>Translates one attribute, or returns null when it is not one we have a name for.</summary>
+    public static AttributeTranslation? Translate(AttributeData attribute)
+    {
+        var name = attribute.AttributeClass?.ToDisplayString();
+        if (name is null)
+        {
+            return null;
+        }
+
+        var parameters = new List<KeyValuePair<string, string>>();
+        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);
+    }
+
+    /// <summary>Merges another assignment into an object initializer, adding one if it has none.</summary>
+    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;
+    }
+
+    /// <summary>The value as a C# expression.</summary>
+    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") + "\"";
+}

src/HttpExtensions.Generator/HttpExtensions.Generator.csproj

@@ -0,0 +1,19 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <!-- Analyzers load into the compiler, which runs on .NET Standard 2.0. Overrides the repo default. -->
+    <TargetFramework>netstandard2.0</TargetFramework>
+    <LangVersion>latest</LangVersion>
+    <Nullable>enable</Nullable>
+    <IsPackable>false</IsPackable>
+    <IncludeBuildOutput>false</IncludeBuildOutput>
+    <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
+    <!-- RS2008: analyzer release tracking, which is for shipped analyzer packages rather than an in-repo generator. -->
+    <NoWarn>$(NoWarn);RS2008</NoWarn>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="[4.8.0]" PrivateAssets="all" />
+  </ItemGroup>
+
+</Project>