provide something to hook into AuthorizationError.AppendFailureLine

[gha-zund]

I have a feature wish: I have some custom AuthorizationRequirements. Currently I see no easy way to get a informative and beautified error message for them, there is only the type name (default case in the switch in AppendFailureLine).

Can you make this part virtual, so it's possible to override it? Or even more convenient would be to have some sort of error message builder which is injectible to the AuthorizationValidationRule.

[sungam3r]

See #480, it may help. There is no need to make AppendFailureLine virtual. The idea is that AppendFailureLine prints a bunch of known requirements and for other you may use custom ErrorInfoProvider.

[gha-zund]

For me, implementing a custom ErrorInfoProvider feels like too much for this (cosmetic) problem. Especially I only want to adapt the default message partly, not in whole.

Would it be possible to change the default case of AppendFailureLine to use .ToString()? So I could simply overwrite that. Another way would be an additional FailureMessage property on the AuthorizationRequriement (maybe with an additional interface e.g. IFailureMessageProvider which could be casted for in the default case of AppendFailureLine).

Another alternative which would work for me is to provider an additional constructor, so the OperationType could be without GnererateMessage to be called, or make add a protected setter to OperationType.

A question here: What's the impact of not setting OperationType?

[sungam3r]

What's the impact of not setting OperationType?

Another way would be an additional FailureMessage property on the AuthorizationRequriement (maybe with an additional interface e.g. IFailureMessageProvider which could be casted for in the default case of AppendFailureLine).

I would not, 50/50. This complicates the API and message rendering is not a task for auth requirement. CustomErrorInfoProvider is intended to solve exactly your case - it can handle custom messages for both custom requirements and "built-in" ones.

ping @rose-a @Shane32 What do you think?

[gha-zund]

So OperationType is only used for generating the message and it's read from the parameter, never read from the property directly? So, I think I could safely inherit from the class to serve my purpose as it is, without setting OperationType, defining the message in another way.

Thanks for the hint with the Error Provider anyways. I will look into it one more time.

I agree with you, an additional interface would complicate the API. I didn't see it that way before.

Thanks for your quick responses! I really appreciate that.

[sungam3r]

So OperationType is only used for generating the message and it's read from the parameter, never read from the property directly?

https://github.com/graphql-dotnet/server/blob/master/src/Authorization.AspNetCore/AuthorizationError.cs

[sungam3r]

OperationType property may be used by custom error info provider (as GraphQL.Samples.Server.CustomErrorInfoProvider does).

[rose-a]

CustomErrorInfoProvider is intended to solve exactly your case - it can handle custom messages for both custom requirements and "built-in" ones.

Agree, I'd stick with the current simple API, let's not introduce a "second" way to generate custom error messages

[gha-zund]

I'm still stumbling across this: The default ErrorInfoProvider sets the message as following: _options.ExposeExceptionStackTrace ? executionError.ToString() : executionError.Message,

Case that options.ExposeExceptionStackTrace is false: It's easy to replace the message with an overriden ErrorInfoProvider. So no problem here.

In case options.ExposeExceptionStackTrace is true there is no deterministic way to replace the message without replacing all other exceptions details. At least, I don't know a way without having redundant information in the string. I don't want to loose the option to enable exposing stack traces...

Do you have any suggestions?

Additionally, I still think it's not the optimal solution to have all the message building code inside the error class. That code is coupled to other types, although it's not 100% needed for the purpose of the AuthorizationError class. Why not moving the message building code to an (overridable) service, which is injected to AuthorizationValidationRule. There, the message is passed to the constructor of AuthorizationError. You would gain more flexibility for generating the message and an AuthorizationError class with less dependencies :)

[Shane32]

I would not, 50/50. This complicates the API and message rendering is not a task for auth requirement. CustomErrorInfoProvider is intended to solve exactly your case - it can handle custom messages for both custom requirements and "built-in" ones.

ping @rose-a @Shane32 What do you think?

I can only provide general comments, because I do not know the authorization code. I wasn't going to comment at all, but only since you asked....

I did not pay a lot of attention to #480 because I write my own authorization code. However, if I recall correctly, I was disappointed in the lack of extensibility provided. It seemed to me that to change a single error message would require a rewrite of all of the messages, since the messages is generated from a list. (But perhaps and hopefully I am wrong.)

Now speaking generally, I often try to write my classes with many small protected virtual methods so any single piece can easily be changed or patched in the field. For example, I may have written something like this:

var message = GetErrorMessages(reasons);

protected virtual string GetErrorMessages(List<ErrorReason> reasons)
{
    var sb = new StringBuilder();
    foreach (var reason in reasons)
        GetErrorDescription(sb);
    return sb.ToString();
}

protected virtual void GetErrorDescription(ErrorReason reason, StringBuilder sb)
{
    //existing code here - but only for a single validation error
}

Then overriding a single error description is easy:

override void GetErrorDescription(ErrorReason reason, StringBuilder sb)
{
    if (reason.Cause == "whatever")
        sb.AppendLine("Hey you can't do that");
    else
        base.GetErrorDescription(reason, sb);
}

I try not to use static methods at all in cases where it's pretty obvious the code could change or is subject to opinion. I may have static methods internally to provide default implementations to other parts of the code. Of course static methods are great in many cases and as small helper methods. But I know from experience -- lots of experience -- that having static or non-virtual methods within GraphQL.NET has greatly hampered my code. Sometimes where extensibility was needed, and sometimes where a bug existed in the code. For example, over time we have fixed multiple bugs in the schema printer. If all the methods were virtual, then a user could override the buggy code to provide a fix for themselves without waiting for a new release. This does not hurt the functionality of the schema printer at all by simply marking the methods as virtual, nor is it any more difficult to use. Some of the problematic code in GraphQL.NET I've fixed but there is more to be done.

Now, my ideas above may not be feasible because the validation rule may generate the authorization error class directly, and so a derived class would require deriving another class to make any use of it, requiring more virtual methods and etc. I don't know enough about the authorization code to consider the design into my answer. But as a general principle, I would make the methods protected/virtual in that class if it made sense. @gha-zund or others may wish to write a complicated set of derived classes in their code. Having protected virtual members does not hurt any normal use, and allows extendibility in the case of advanced uses. But if it's completely unpractical to derive from the class, then it doesn't matter much.

There's also the matter of separation of responsibility. I feel that the validation rule should be responsible for creating a valid error message. ErrorInfoProvider was designed to be a catch-all to mask or change exception messages, or to provide or mask additional data returned within the error. Although possible, it is not designed to be a replacement for proper validation message generation.

Validation of queries is pretty fixed -- if it's an invalid query, it's an invalid query. But validation of authorization rules, as we have seen from multiple issues on this topic, can change depending on the nature of the authorization error. Perhaps one user would like a message to say that the token has expired, and another user would like a message that says that a password was invalid. (Again, bear with me, I know nothing about this authorization framework.) In any case it seems to be the responsibility of the validation framework to provide a correct message, unique to the user's needs, rather than to have to catch the validation error within an ErrorInfoProvider just to provide an applicable error message.

So, one way or another I think the validation framework should provide a way to extend/override the validation error since it seems to me that each user's authorization framework is unique and often may require a unique validation error message(s). It should not automatically fall to the ErrorInfoProvider's responsibility. And it should be easy to tailor specific error situations without a rewrite of all of the error messages.

Now that's just my opinion, and it's only based on my limited understanding of the authorization framework. I may have a different opinion for this particular scenario if I understood the framework better. So please understand my viewpoint here, and I would rely on someone who uses the framework to better answer the question of what the most makes sense considering the existing design.

[gha-zund]

@Shane32 thank you for your words! That perfectly describes my feelings about this and what I want to do.

[Shane32]

In my opinion, the areas of responsibility are as follows:

Area	Responsibility
Validation rule	Generate a proper error message as "client" errors to be returned to the user
Field resolvers	Throw ExecutionError for "client" errors to be returned to the user; only allow "server" exceptions to fall through
UnhandledErrorDelegate	Log unhandled "server" errors to a database for further review
ErrorInfoProvider	Format the error prior to it being returned

As such, ErrorInfoProvider really normally shouldn't be tampering with the messages, but doing stuff like placing the stack trace in a separate property under extensions instead of including it with the message. Or changing how the code property is used. Of course ErrorInfoProvider CAN look for certain exceptions and change them, but I'd say that's an exception to the rule. (no pun intended!)

In the same manner UnhandledErrorDelegate normally shouldn't change errors, but could be used to change server exceptions to client exceptions where applicable, such as changing a BraintreeCvvException to an ExecutionError that says "The entered CVV code is invalid". It's just not the designed purpose of the delegate.

[Shane32]

I am very glad that we provide a number of different avenues to handle exceptions and errors within the codebase. It provides flexibility with users' code so they can handle errors when and where they like. Back prior to 3.0, it was terrible; the UnhandledErrorDelegate executed for client errors sometimes (but not always), and there was no options for formatting except to expose the stack trace or not. Now "client" and "server" are clearly separated throughout the codebase so there is no unintended logging of client errors, for example. We even have proper handling of OperationCancelledException throughout so that a closed client connection does not improperly show up as a server exception.

I have no complaint if a user wishes to use ErrorInfoProvider to change an exception around - that's great. It's very easy to implement such a class, especially with the builder methods now, and we have documentation describing how to use it. But it's a 'bonus' in a sense, not exactly the responsibility of the class.

[Shane32]

In case options.ExposeExceptionStackTrace is true there is no deterministic way to replace the message without replacing all other exceptions details. At least, I don't know a way without having redundant information in the string. I don't want to loose the option to enable exposing stack traces...

Probably a poor design, but it may be a matter of perspective. I designed it like this to match prior functionality. We could change this to only provide a stack trace for "server" exceptions -- those that do not inherit from ExecutionError. But I do not know enough of users' wishes to be able to say what is best as a default.

Keep in mind that a user could override UnhandledErrorDelegate to return an ExecutionError with the message property containing the ToString of the inner server error message. I believe this is how old code may have functioned in the past, before the ErrorInfoProvider was available.

Personally I don't use the expose stack trace option, and rather always log server errors to the database for further review. For internal projects, I typically expose the original exception Message property to the caller, and this is displayed through alert(); I should probably put the stack trace inside the extensions property also. Then I can use F12 to inspect a network trace to view the exception details. Similarly, for public projects, I mask server exceptions and just return a generic failure message. So I could never see the expose-stack-trace option as useful -- at least, not for myself.

If you wish to request a change to this behavior for v5, please add an issue to the graphql-dotnet repo and we can continue a discussion over there.

[sungam3r]

In case options.ExposeExceptionStackTrace is true there is no deterministic way to replace the message without replacing all other exceptions details.

I don't understand.

[sungam3r]

What is all other exceptions details?

[gha-zund]

What is all other exceptions details?

exception's data dictionary or inner exceptions, for example.

All of that is compiled together into one string. And I want to have all that information in the string. But I would like to replace the part of the string which describes the exception message only, leaving the other parts untouched.

Sure, I can assume that the default Exception.ToString() implementation is used and work with Substring or string.Split. But if one exceptions's ToString is overriden, that would not work anymore ...

But that's just one reason why I don't think that using a Custom ErrorInfoProvider is the way to go for me. Other reasons were listed above by @Shane32

Requesting a change of the default ErrorInfoProvider would be a possible solution, but I don't think that ErrorInfoProvider is the right place for my custom authorization error message, due to intended responsibility of it.

[sungam3r]

Maybe I do not understand something. PR is welcome.

[gha-zund]

alright. I will create one when I have some free time. Next week or the week after that.

[Shane32]

@gha-zund Please branch from develop and have your PR target develop - thanks!!

[gha-zund]

here you are: https://github.com/graphql-dotnet/server/pull/711 sorry that it took longer

[sungam3r]

Will be released in v6, thanks.