Allow any validation error to prefer a status code over 400 (#1141)

graphql-dotnet · Aug 5, 2024 · 39aef15 · 39aef15
1 parent 544a5e2
commit 39aef15
Show file tree

Hide file tree

Showing 11 changed files with 175 additions and 11 deletions.
diff --git a/docs/migration/migration8.md b/docs/migration/migration8.md
@@ -20,6 +20,11 @@
   the `CsrfProtectionHeaders` property on the same class.  See the readme for more details.
 - Form POST requests are disabled by default; to enable them, set the `ReadFormOnPost` setting
   to `true`.
+- Validation errors such as authentication errors may now be returned with a 'preferred' status
+  code instead of a 400 status code.  This occurs when (1) the response would otherwise contain
+  a 400 status code (e.g. the execution of the document has not yet begun), and (2) all errors
+  in the response prefer the same status code.  For practical purposes, this means that the included
+  errors triggered by the authorization validation rule will now return 401 or 403 when appropriate.
 
 ## Other changes
 

diff --git a/src/Transports.AspNetCore/Errors/HttpMethodValidationError.cs b/src/Transports.AspNetCore/Errors/HttpMethodValidationError.cs
@@ -4,11 +4,14 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors;
 /// Represents a validation error indicating that the requested operation is not valid
 /// for the type of HTTP request.
 /// </summary>
-public class HttpMethodValidationError : ValidationError
+public class HttpMethodValidationError : ValidationError, IHasPreferredStatusCode
 {
     /// <inheritdoc cref="HttpMethodValidationError"/>
     public HttpMethodValidationError(GraphQLParser.ROM originalQuery, ASTNode node, string message)
         : base(originalQuery, null!, message, node)
     {
     }
+
+    /// <inheritdoc/>
+    public HttpStatusCode PreferredStatusCode { get; set; } = HttpStatusCode.MethodNotAllowed;
 }
diff --git a/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs b/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs
@@ -584,12 +584,26 @@ protected virtual async Task HandleRequestAsync(
         var userContext = await BuildUserContextAsync(context, null);
         var result = await ExecuteRequestAsync(context, gqlRequest, context.RequestServices, userContext);
         HttpStatusCode statusCode = HttpStatusCode.OK;
+        // when the request fails validation (this logic does not apply to execution errors)
         if (!result.Executed)
         {
+            // always return 405 Method Not Allowed when applicable, as this is a transport problem, not really a validation error,
+            // even though it occurs during validation (because the query text must be parsed to know if the request is a query or a mutation)
             if (result.Errors?.Any(e => e is HttpMethodValidationError) == true)
+            {
                 statusCode = HttpStatusCode.MethodNotAllowed;
+            }
+            // otherwise use 4xx error codes when configured to do so
             else if (_options.ValidationErrorsReturnBadRequest)
+            {
                 statusCode = HttpStatusCode.BadRequest;
+                // if all errors being returned prefer the same status code, use that
+                if (result.Errors?.Count > 0 && result.Errors[0] is IHasPreferredStatusCode initialError)
+                {
+                    if (result.Errors.All(e => e is IHasPreferredStatusCode e2 && e2.PreferredStatusCode == initialError.PreferredStatusCode))
+                        statusCode = initialError.PreferredStatusCode;
+                }
+            }
         }
         await WriteJsonResponseAsync(context, statusCode, result);
     }

diff --git a/src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs b/src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs
@@ -43,8 +43,8 @@ public class GraphQLHttpMiddlewareOptions : IAuthorizationOptions
     public bool ExecuteBatchedRequestsInParallel { get; set; } = true;
 
     /// <summary>
-    /// When enabled, GraphQL requests with validation errors
-    /// have the HTTP status code set to 400 Bad Request.
+    /// When enabled, GraphQL requests with validation errors have the HTTP status code
+    /// set to 400 Bad Request or the error status code dictated by the error.
     /// GraphQL requests with execution errors are unaffected.
     /// <br/><br/>
     /// Does not apply to batched or WebSocket requests.

diff --git a/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -220,9 +220,10 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors
         public FileSizeExceededError() { }
         public System.Net.HttpStatusCode PreferredStatusCode { get; set; }
     }
-    public class HttpMethodValidationError : GraphQL.Validation.ValidationError
+    public class HttpMethodValidationError : GraphQL.Validation.ValidationError, GraphQL.Server.Transports.AspNetCore.Errors.IHasPreferredStatusCode
     {
         public HttpMethodValidationError(GraphQLParser.ROM originalQuery, GraphQLParser.AST.ASTNode node, string message) { }
+        public System.Net.HttpStatusCode PreferredStatusCode { get; set; }
     }
     public interface IHasPreferredStatusCode
     {

diff --git a/...pprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt b/...pprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -238,9 +238,10 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors
         public FileSizeExceededError() { }
         public System.Net.HttpStatusCode PreferredStatusCode { get; set; }
     }
-    public class HttpMethodValidationError : GraphQL.Validation.ValidationError
+    public class HttpMethodValidationError : GraphQL.Validation.ValidationError, GraphQL.Server.Transports.AspNetCore.Errors.IHasPreferredStatusCode
     {
         public HttpMethodValidationError(GraphQLParser.ROM originalQuery, GraphQLParser.AST.ASTNode node, string message) { }
+        public System.Net.HttpStatusCode PreferredStatusCode { get; set; }
     }
     public interface IHasPreferredStatusCode
     {

diff --git a/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -220,9 +220,10 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors
         public FileSizeExceededError() { }
         public System.Net.HttpStatusCode PreferredStatusCode { get; set; }
     }
-    public class HttpMethodValidationError : GraphQL.Validation.ValidationError
+    public class HttpMethodValidationError : GraphQL.Validation.ValidationError, GraphQL.Server.Transports.AspNetCore.Errors.IHasPreferredStatusCode
     {
         public HttpMethodValidationError(GraphQLParser.ROM originalQuery, GraphQLParser.AST.ASTNode node, string message) { }
+        public System.Net.HttpStatusCode PreferredStatusCode { get; set; }
     }
     public interface IHasPreferredStatusCode
     {

diff --git a/tests/Samples.Authorization.Tests/EndToEndTests.cs b/tests/Samples.Authorization.Tests/EndToEndTests.cs
@@ -21,15 +21,15 @@ public Task GraphQLGet_Success()
 
     [Fact]
     public Task GraphQLGet_AccessDenied()
-        => new ServerTests<Program>().VerifyGraphQLGetAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.BadRequest);
+        => new ServerTests<Program>().VerifyGraphQLGetAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.Unauthorized);
 
     [Fact]
     public Task GraphQLPost_Success()
         => new ServerTests<Program>().VerifyGraphQLPostAsync("/graphql", SUCCESS_QUERY, SUCCESS_RESPONSE);
 
     [Fact]
     public Task GraphQPost_AccessDenied()
-        => new ServerTests<Program>().VerifyGraphQLPostAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.BadRequest);
+        => new ServerTests<Program>().VerifyGraphQLPostAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.Unauthorized);
 
     [Fact]
     public Task GraphQLWebSocket_Success()

diff --git a/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs b/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs
@@ -765,7 +765,7 @@ public async Task EndToEnd(bool authenticated)
 
         using var client = server.CreateClient();
         using var response = await client.GetAsync("/graphql?query={ parent { child } }");
-        response.StatusCode.ShouldBe(authenticated ? System.Net.HttpStatusCode.OK : System.Net.HttpStatusCode.BadRequest);
+        response.StatusCode.ShouldBe(authenticated ? System.Net.HttpStatusCode.OK : System.Net.HttpStatusCode.Unauthorized);
         var actual = await response.Content.ReadAsStringAsync();
 
         if (authenticated)

diff --git a/tests/Transports.AspNetCore.Tests/Middleware/GetTests.cs b/tests/Transports.AspNetCore.Tests/Middleware/GetTests.cs
@@ -1,12 +1,14 @@
 using System.Net;
+using GraphQL.Server.Transports.AspNetCore.Errors;
+using GraphQL.Validation;
 
 namespace Tests.Middleware;
 
 public class GetTests : IDisposable
 {
     private GraphQLHttpMiddlewareOptions _options = null!;
     private GraphQLHttpMiddlewareOptions _options2 = null!;
-    private readonly Action<ExecutionOptions> _configureExecution = _ => { };
+    private Action<ExecutionOptions> _configureExecution = _ => { };
     private readonly TestServer _server;
 
     public GetTests()
@@ -55,6 +57,14 @@ private class Query2
     {
         public static string? Ext(IResolveFieldContext context)
             => context.InputExtensions.TryGetValue("test", out var value) ? value?.ToString() : null;
+
+        public static string? CustomError() => throw new CustomError();
+    }
+
+    public class CustomError : ValidationError, IHasPreferredStatusCode
+    {
+        public CustomError() : base("Custom error") { }
+        public HttpStatusCode PreferredStatusCode => HttpStatusCode.NotAcceptable;
     }
 
     public void Dispose() => _server.Dispose();
@@ -250,6 +260,67 @@ public async Task Disabled()
         response.StatusCode.ShouldBe(HttpStatusCode.NotFound);
     }
 
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task PreferredStatusCode_ExecutionErrors(bool badRequest)
+    {
+        _options2.ValidationErrorsReturnBadRequest = badRequest;
+        var client = _server.CreateClient();
+        using var response = await client.GetAsync("/graphql2?query={customError}");
+        await response.ShouldBeAsync(
+            HttpStatusCode.OK,
+            """{"errors":[{"message":"Custom error","locations":[{"line":1,"column":2}],"path":["customError"],"extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}],"data":{"customError":null}}""");
+    }
+
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task PreferredStatusCode_ValidationErrors(bool badRequest)
+    {
+        _options2.ValidationErrorsReturnBadRequest = badRequest;
+        var mockRule = new Mock<IValidationRule>(MockBehavior.Loose);
+        mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny<ValidationContext>())).Returns<ValidationContext>(context =>
+        {
+            context.ReportError(new CustomError());
+            context.ReportError(new CustomError());
+            return default;
+        });
+        _configureExecution = o =>
+        {
+            o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object);
+        };
+        var client = _server.CreateClient();
+        using var response = await client.GetAsync("/graphql2?query={__typename}");
+        await response.ShouldBeAsync(
+            badRequest ? HttpStatusCode.NotAcceptable : HttpStatusCode.OK,
+            """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}]}""");
+    }
+
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task PreferredStatusCode_MixedValidationErrors(bool badRequest)
+    {
+        _options2.ValidationErrorsReturnBadRequest = badRequest;
+        var mockRule = new Mock<IValidationRule>(MockBehavior.Loose);
+        mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny<ValidationContext>())).Returns<ValidationContext>(context =>
+        {
+            context.ReportError(new CustomError());
+            context.ReportError(new ValidationError("test"));
+            return default;
+        });
+        _configureExecution = o =>
+        {
+            o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object);
+        };
+        var client = _server.CreateClient();
+        using var response = await client.GetAsync("/graphql2?query={__typename}");
+        await response.ShouldBeAsync(
+            badRequest ? HttpStatusCode.BadRequest : HttpStatusCode.OK,
+            """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"test","extensions":{"code":"VALIDATION_ERROR","codes":["VALIDATION_ERROR"]}}]}""");
+    }
+
     [Theory]
     [InlineData(false)]
     [InlineData(true)]

diff --git a/tests/Transports.AspNetCore.Tests/Middleware/PostTests.cs b/tests/Transports.AspNetCore.Tests/Middleware/PostTests.cs
@@ -1,12 +1,14 @@
 using System.Net;
+using GraphQL.Server.Transports.AspNetCore.Errors;
+using GraphQL.Validation;
 
 namespace Tests.Middleware;
 
 public class PostTests : IDisposable
 {
     private GraphQLHttpMiddlewareOptions _options = null!;
     private GraphQLHttpMiddlewareOptions _options2 = null!;
-    private readonly Action<ExecutionOptions> _configureExecution = _ => { };
+    private Action<ExecutionOptions> _configureExecution = _ => { };
     private readonly TestServer _server;
 
     public PostTests()
@@ -65,6 +67,14 @@ private class Query2
         public static MyFile File3(MyFileInput arg) => new(arg.File);
         public static IEnumerable<MyFile> File4(IEnumerable<MyFileInput> args) => args.Select(x => new MyFile(x.File));
         public static IEnumerable<MyFile> File5(MyFileInput2 args) => args.Files.Select(x => new MyFile(x));
+
+        public static string? CustomError() => throw new CustomError();
+    }
+
+    public class CustomError : ValidationError, IHasPreferredStatusCode
+    {
+        public CustomError() : base("Custom error") { }
+        public HttpStatusCode PreferredStatusCode => HttpStatusCode.NotAcceptable;
     }
 
     private record MyFileInput(IFormFile File);
@@ -542,6 +552,64 @@ public async Task Disabled()
         response.StatusCode.ShouldBe(HttpStatusCode.NotFound);
     }
 
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task PreferredStatusCode_ExecutionErrors(bool badRequest)
+    {
+        _options2.ValidationErrorsReturnBadRequest = badRequest;
+        using var response = await PostRequestAsync("/graphql2", new() { Query = "{customError}" });
+        await response.ShouldBeAsync(
+            HttpStatusCode.OK,
+            """{"errors":[{"message":"Custom error","locations":[{"line":1,"column":2}],"path":["customError"],"extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}],"data":{"customError":null}}""");
+    }
+
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task PreferredStatusCode_ValidationErrors(bool badRequest)
+    {
+        _options2.ValidationErrorsReturnBadRequest = badRequest;
+        var mockRule = new Mock<IValidationRule>(MockBehavior.Loose);
+        mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny<ValidationContext>())).Returns<ValidationContext>(context =>
+        {
+            context.ReportError(new CustomError());
+            context.ReportError(new CustomError());
+            return default;
+        });
+        _configureExecution = o =>
+        {
+            o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object);
+        };
+        using var response = await PostRequestAsync("/graphql2", new() { Query = "{__typename}" });
+        await response.ShouldBeAsync(
+            badRequest ? HttpStatusCode.NotAcceptable : HttpStatusCode.OK,
+            """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}]}""");
+    }
+
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task PreferredStatusCode_MixedValidationErrors(bool badRequest)
+    {
+        _options2.ValidationErrorsReturnBadRequest = badRequest;
+        var mockRule = new Mock<IValidationRule>(MockBehavior.Loose);
+        mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny<ValidationContext>())).Returns<ValidationContext>(context =>
+        {
+            context.ReportError(new CustomError());
+            context.ReportError(new ValidationError("test"));
+            return default;
+        });
+        _configureExecution = o =>
+        {
+            o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object);
+        };
+        using var response = await PostRequestAsync("/graphql2", new() { Query = "{__typename}" });
+        await response.ShouldBeAsync(
+            badRequest ? HttpStatusCode.BadRequest : HttpStatusCode.OK,
+            """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"test","extensions":{"code":"VALIDATION_ERROR","codes":["VALIDATION_ERROR"]}}]}""");
+    }
+
     [Theory]
     [InlineData(false)]
     [InlineData(true)]