Add AuthorizationError (pickup of #479) #480
Conversation
| } | ||
| // TODO: check to see if only a single failure result is returned in result.Failure.FailedRequirements, | ||
| // and report a single specific error class for that result. fall back to the below when multiple | ||
| // errors are reported. or instead of the fallback, report separate error instances for each entry. |
There was a problem hiding this comment.
As already expressed in #478 I'm opposed to generating a separate error for each failed requirement. It would result in many lines starting with "You are not authorized to run this [operation] because ...", and IMO failure to authorize should be a single validation error.
Any opinions?
There was a problem hiding this comment.
I have no strong opinion. Feel free to delete the comment if that is the consensus.
There was a problem hiding this comment.
@rose-a See graphql-dotnet/authorization#117
It is also unnecessarily complex to query for the error code in the json, since it is an array of errors and in one query there could be errors relating to something other than authentication.
There was a problem hiding this comment.
Any opinions?
I'm 50/50 but I'm OK to merge PR as is since AddValidationError is virtual now and one can override it to make multiple calls to context.ReportError with customized messages. Moreover then actual messages written in response could be customized in custom IErrorInfoProvider.
There was a problem hiding this comment.
For graphql-dotnet/authorization#117 they would have to override GraphQLHttpMiddleware to create a 401 status code on an AuthorizationError containing a DenyAnonymousAuthorizationRequirement as failed requirement... This PR only lays the foundations for this, currently it is not possible to change the status code of the HTTP response based on GraphQL execution errors
There was a problem hiding this comment.
I know. Thanks. FYI @tobias-tengler.
|
I will review this tomorrow. Thanks. |
|
It would be nice to put example of this #478 (comment) into samples. |
|
I pushed CodeQL analysis action along with dotnet-format checks into |
|
samples/Samples.Server/Startup.cs(2,1): Using directive is unnecessary. (IDE0005) |
|
👍 |
I picked up #479 and rebased it to the current
developbranch. It currently is a "non-breaking" change which opens up the possibility to generate custom error messages by injecting a customIErrorInfoProvideras suggested here by @Shane32.What's needed for this do be ready to merge?