Add AllowAnonymous extension method and attribute

[Shane32]

Closes #3078

For 5.1.1

[sungam3r]

I suggest not to narrow meaning of allowanonymous only to fields like for actions in ASP.NET controllers. It's GraphQL world.

[Shane32]

In this case, I'm not really thinking about how it relates to ASP.NET (apart from the naming and general concept), but rather the practical usage of this attribute in the GraphQL world. I'm just not sure how AllowAnonymous would be useful to apply to a graph, since graphs are generally abstract from other graphs. For instance, if Product is marked as AllowAnonymous, and if MasterProductsTable is protected, querying for all products within MasterProductsTable should still be refused. The same thought process applies to input object graphs and scalars, leaving only fields at this time.

Within ASP.NET Core, only one action can ever execute, so AllowAnonymous simply overrides all Authorize attributes that are parent to it in the chain. In this respect, my suggestion is unlike ASP.NET Core. It's simply what works best for GraphQL.

I can remove the attribute usage restriction, but even if we did so, I figured that the xml comments should explain anticipated usage of the attributes, even knowing that implementation is not part of this library. If nothing else, use the word "typically" and retain the wording/example.

Do you suggest I remove the attribute usage restrictions? Do you have a suggestion of rewording here?

[sungam3r]

implementation is not part of this library

This is true and we must leave some space for maneuver.

word "typically" and retain the wording/example

Yes, let's insert typically somewhere. The general meaning will be the same, but the phrase will sound more accurate.

[Shane32]

Suggested change

	`AllowAnonymous` indicates that anonymous access should be allowed to a field of a graph type requiring authorization,
	`AllowAnonymous` typically indicates that anonymous access should be allowed to a field of a graph type requiring authorization,

[Shane32]

See above (merged).

[Shane32]

@sungam3r I'm working on an authorization implementation that works with all the PRs over the last couple days. One question that is interesting to consider is: Should the introspection field types be marked with .AllowAnonymous()? I think the answer is 'no', but rather the validation rule requires some special logic, and here's why:

Consider these queries:

# this query requires authentication because ProtectedType requires authentication
{
  protectedType {   # the type, not the field here, is marked as needing authentication
    unmarkedField
    anonymousField
  }
}

# this query does not require authentication becauase although ProtectedType requires
# authentication, all of the selected fields are marked with AllowAnonymous
{
  protectedType {
    anonymousField
  }
}

# this query also does not require authentication
{
  protectedType {
    __typeName
    anonymousField
  }
}

# this query SHOULD require authentication, because it could unintentionally reveal information to the caller,
# if for example the schema designer did not mark any fields with AllowAnonymous
{
  protectedType {
    __typeName
  }
}

So as you can see, in my implementation __typeName will only be treated as AllowAnonymous when all other selected fields are also marked with AllowAnonymous. But if __typeName is the only selected field, then the authentication requirements of the type will take effect.

Now for __type and __schema, they can safely be marked with .AllowAnonymous() (or treated as such), because they only exist on the root Query type; if no other fields are requested, no secure information is returned. I suggest we do so, and then users can override such behavior in their schema constructor by modifying the metadata. But even that should not be required; users can just mark the schema itself with .Authorize() and then any unauthenticated request will fail, regardless of whether or not only introspection fields were requested. On the other hand, since we are not marking __typeName, we can skip marking __type and __schema, leaving it up to the validation rule to treat them as .AllowAnonymous() (or as desired by the authentication implementation).

[Shane32]

p.s. Dealing with fragments and the AllowAnonymous() concept is a real pain. Everything else is pretty straightforward to implement. It would help a TON if the document validator sorted the document so dependent fragments were processed prior to the fragments or query that needed those fragments. Instead I will need to store a list of the types waiting on a fragment to finish validation, and once the fragment is processed, add a validation error if necessary. And since I can't easily clone/preserve the state of the ValidationContext at the point of the field node that caused the validation error (or more specifically the TypeInfo state), I need to gather and store all information relevant to the potential error necessary to create an error message (e.g. field name that the type was referenced by).

[codecov-commenter]

Codecov Report

Merging #3079 (2da7941) into master (f7146d7) will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@           Coverage Diff           @@
##           master    #3079   +/-   ##
=======================================
  Coverage   84.38%   84.38%           
=======================================
  Files         360      361    +1     
  Lines       15830    15842   +12     
  Branches     2576     2576           
=======================================
+ Hits        13358    13369   +11     
- Misses       1858     1859    +1     
  Partials      614      614           

Impacted Files	Coverage Δ
...c/GraphQL/Authorization/AuthorizationExtensions.cs	71.42% <83.33%> (+1.00%)	⬆️
...c/GraphQL/Authorization/AllowAnonymousAttribute.cs	100.00% <100.00%> (ø)
...GraphQL/Authorization/GraphQLAuthorizeAttribute.cs
src/GraphQL/Authorization/AuthorizeAttribute.cs	72.54% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7146d7...2da7941. Read the comment docs.

[sungam3r]

It would help a TON if the document validator sorted the document so dependent fragments were processed prior to the fragments or query that needed those fragments.

:) I was biten by that when doing complexity analizer.

Regarding all other notes. Yes, auth validation is a pain. There are so much questions to think about. I have already thought about them, because in my work we use an extensive system for checking auth directives. I came to the conclusion that there is no ideal solution. Implementation can pursue different purposes.

Also remember of graphql-dotnet/authorization#186 (abandoned).

[Shane32]

@sungam3r I am going to finish my auth code tonight, add a million tests and all that stuff, and make sure it is operating as I am anticipating that it should. I know you're wrapping up for the night, but by your morning, you'll be able to see my progress in Shane32/GraphQL.AspNetCore3#6 . (Don't look now, it's not nearly finished.) I will plan to release 5.1.1 in the morning; just reply if you have any comments or objections between now and then.

[Shane32]

If you would like to help review my implementation, particularly the public API, let me know. I know you are busy and also support a lot of open-source projects, so if you don't have time, that's fine too.

[Shane32]

@sungam3r I'm going to release 5.1.1 now.

[sungam3r]

OK. I opened your PR. Now the main thing is not to lose it among the other 150 open tabs.