Update repo with the latest-and-greatest GraphQL.NET/CI/Code Style

[sungam3r]

Inspired by #253

• Bumps GraphQL.NET to v7
• CI with NET7
• Implicit usings + file-scoped namespaces
• Updated samples
• Changes in code style from the main repo

It has not been updated, and if I update the GraphQL library references to 7.2.1 (current version), it no longer compiles as it appears some types have moved elsewhere.

ping @BillBaird

⚠️ The goal of this PR is to make it possible to use this package with the latest available (7.x) version of GraphQL.NET. No new features.

UPDATE: #256 is a new feature

[Shane32]

As far as I'm concerned, this repo is obsolete. It only supports policy-based authentication; a proper upgrade should support [AllowAnonymous], role-based authentication, authenticated-only (no specific policy/role), and support validating against ValidationContext.User. It should also include IGraphQLBuilder method(s) if it does not.

I'm okay updating it, but assuming it does not have an overhaul, an explicit notice near the top of the readme should note all of the features this repo does not support, along with a suggestion that GraphQL.NET Server 7's authentication rule be used. I might even mark the entry point as [Obsolete] (e.g. the validation rule class and builder methods), such as was done with the older authentication rule in GraphQL.NET 7 Server, with the idea that it should produce exactly 1 obsolete warning, which can be suppressed easily if need be.

[Shane32]

Note that the authorization rule in GraphQL.NET Server 7 can be extensively customized and need not rely on ASP.NET Core's authentication subsystem. The base class and validation error class does reference the AuthorizationResult class from ASP.NET Core. This is the only reference to ASP.NET Core in the underlying validation code.

[Shane32]

Some other options (perhaps for v8) are:

1. Move the base validation code into GraphQL.NET, as it is somewhat tied to the metadata and attributes. Derive from it within the server repo and/or here.

2. Move the base validation code from GraphQL.NET Server into this repo, and add a dependency within GraphQL.NET Server on this repo.

[sungam3r]

this repo is obsolete

Until GraphQL is tightly coupled with HTTP transport - no.

a proper upgrade should support ...

It is not a goal of this PR. Later.

an explicit notice near the top of the readme should note all of the features this repo does not support, along with a suggestion that GraphQL.NET Server 7's authentication rule be used.

Post a suggestion and I will merge it.

I might even mark the entry point as [Obsolete]

Again - until GraphQL is tightly coupled with HTTP transport - no. At least I do not consider this action as correct, because it deliberately narrows authorization applicability to only HTTP / ASP.NET Core transport / framework.

Note that the authorization rule in GraphQL.NET Server 7 can be extensively customized and need not rely on ASP.NET Core's authentication subsystem. The base class and validation error class does reference the AuthorizationResult class from ASP.NET Core. This is the only reference to ASP.NET Core in the underlying validation code.

Maybe, maybe. But can be != to be. For now server package is ASP.NET Core package with all references/docs and other stuff. A long time ago I already asked @joemcbride about the possibility of merging two projects (this one and server bits). I will only be glad if we manage to cut a common bits of the authorization so that it will not depend on the transport/ASP.NET framework at all. So far this is not so, then this project will live.

Regarding https://github.com/graphql-dotnet/authorization/pull/254#issuecomment-1374576945 - both options are viable. I lean to 1.

[sungam3r]

It should also include IGraphQLBuilder method(s) if it does not.

AuthorizationGraphQLBuilderExtensions

[Shane32]

I added some additional changes in PR #255 which currently targets this PR.

[Shane32]

FYI, removing IProvideUserPrincipal means that users will certainly need to change their code to use the new version.

[Shane32]

52/54 reviewed

[Shane32]

I've finished my review. Once the BomTest is removed, or it verifies the sln is in the root of the scanned folder tree, I'll approve.

[sungam3r]

I still have not received an answer to https://github.com/graphql-dotnet/graphql-dotnet/pull/3478#discussion_r1063246769 / https://github.com/graphql-dotnet/authorization/pull/254#discussion_r1064167201 and waiting. So far I have not yet understood what is the point of checking the presence/absense of sln.

[Shane32]

I still have not received an answer to graphql-dotnet/graphql-dotnet#3478 (comment) / #254 (comment) and waiting. So far I have not yet understood what is the point of checking the presence/absense of sln.

To be sure the correct folder is being scanned by looking for a file unique to this repository. Has nothing to do with the editorconfig. I do not think this test should be included at all.

[sungam3r]

What does "correct folder" mean at all? Found folder is always repository root. It's guaranteed by compiler. Since editorconfig resides in the root and since it should work for all files starting from the root then scan should be done in the root as well. I do not understand what is wrong in this simple logical chain. Also https://github.com/graphql-dotnet/graphql-dotnet/pull/3478#discussion_r1064887869

[Shane32]

I’ve tried to explain my view.

What does "correct folder" mean at all? Found folder is always repository root. It's guaranteed by compiler.

No it’s not. That’s what I would like to see. Rather the only thing that’s guaranteed is the file path to this test. Traversing an arbitrary number of Parent nodes will certainly fail the first time this test is copied to another repo or the folder structure changed, etc. Once again that’s the point of checking for the sln

[joemcbride]

The whole point of this library was to purposefully NOT have ASP.NET dependencies, have a simplified policy-based auth, and to be able to use this library regardless of what transport was used with GraphQL .NET. Using policy based authorization you CAN use roles in the policy. I would consider using only role-based auth an anti-pattern when Policies are a much better design, that also supports checking for roles, as well as many other scenarios. Help the user make the right decision instead of allowing them to use bad patterns.

You two are always bickering over changes. Please act more professional. It is tiresome to see your constant bickering. It seriously makes me reconsider fully taking over the project again.

[joemcbride]

And just to be clear - instead of creating an entirely new authorization feature in the other projects (with ASP.NET dependencies), this one should have been updated instead. I fought off several attempts at adding ASP.NET Authorization in the other projects previously. So it is a bit frustrating to see that was done anyways and this one left to languish.

[Shane32]

The whole point of this library was to purposefully NOT have ASP.NET dependencies

:+1:

instead of creating an entirely new authorization feature in the other projects (with ASP.NET dependencies), this one should have been updated instead.

I'd be happy to discuss the future of this project and/or the server project with you, if you like. Without input I typically work on features that I intend to use, hoping that such features will benefit the community.

It is tiresome to see your constant bickering.

I apologize.

[sungam3r]

The whole point of this library was to purposefully NOT have ASP.NET dependencies, have a simplified policy-based auth, and to be able to use this library regardless of what transport was used with GraphQL .NET.

So far, these criteria are carried out.

[codecov-commenter]

Codecov Report

Merging #254 (95d8409) into master (e3f95d1) will decrease coverage by 0.71%. The diff coverage is 77.20%.

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   80.27%   79.56%   -0.72%     
==========================================
  Files          10       10              
  Lines         289      274      -15     
  Branches       44       45       +1     
==========================================
- Hits          232      218      -14     
  Misses         44       44              
+ Partials       13       12       -1     

Impacted Files	Coverage Δ
...xtensions/AuthorizationGraphQLBuilderExtensions.cs	0.00% <0.00%> (ø)
src/GraphQL.Authorization/AuthorizationSettings.cs	33.33% <36.84%> (ø)
...raphQL.Authorization/AuthorizationPolicyBuilder.cs	45.45% <47.61%> (ø)
...ation/Requirements/AuthenticatedUserRequirement.cs	75.00% <60.00%> (ø)
src/GraphQL.Authorization/AuthorizationPolicy.cs	85.71% <77.77%> (ø)
src/GraphQL.Authorization/AuthorizationContext.cs	83.33% <83.33%> (ø)
...tion/Requirements/ClaimAuthorizationRequirement.cs	86.95% <84.61%> (ø)
...aphQL.Authorization/AuthorizationValidationRule.cs	95.14% <95.83%> (+0.23%)	:arrow_up:
...rc/GraphQL.Authorization/AuthorizationEvaluator.cs	100.00% <100.00%> (ø)
src/GraphQL.Authorization/AuthorizationResult.cs	100.00% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sungam3r]

I'll wait some more time. Maybe there is something else.

[Shane32]

@sungam3r We have not issued a release with 7.0 compatibility. Should I do so? cc: @julienFlexsoft

[sungam3r]

I published it just after have read https://github.com/graphql-dotnet/server/issues/987

[Shane32]

I would only suggest that we remove the many dependabot commits, and most other PRs that do not affect the operation of the code, from the release notes. I typically do so. It does not benefit users to know that Shouldly was bumped, for example, as this change does not affect the released code. The only exception is when a contributor expends a significant amount of time to improve the codebase, such as extensive reformatting, or contributions made by a new contributor. But when you or I make documentation fixes, CI changes, build-time dependency bumps, changes to tests, and similar I almost always remove those from the release notes.

[sungam3r]

done https://github.com/graphql-dotnet/authorization/releases/tag/7.0.0

[Shane32]

thanks!