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

sungam3r · 2023-01-07T18:24:04Z

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

src/Harness/appsettings.json

.github/workflows/publish.yml

Shane32 · 2023-01-07T18:55:01Z

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 · 2023-01-07T19:01:50Z

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 · 2023-01-07T19:04:36Z

Some other options (perhaps for v8) are:

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.
Move the base validation code from GraphQL.NET Server into this repo, and add a dependency within GraphQL.NET Server on this repo.

.github/workflows/test.yml

sungam3r · 2023-01-07T20:51:23Z

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 #254 (comment) - both options are viable. I lean to 1.

sungam3r · 2023-01-07T21:31:29Z

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

AuthorizationGraphQLBuilderExtensions

Shane32 · 2023-01-08T05:27:04Z

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

Shane32 · 2023-01-08T14:34:46Z

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

src/GraphQL.Authorization.ApiTests/BomTests.cs

src/Harness/Program.cs

Shane32 · 2023-01-08T16:36:54Z

src/Harness/Program.cs

    {
-        public static Task Main(string[] args) => CreateHostBuilder(args).Build().RunAsync();
+        opt.ThrowOnUnhandledException = true;


I'm curious why you added this. This breaks proper operation within ASP.NET Core

Just the first thing that turned up by the arm.

Shane32 · 2023-01-08T17:10:03Z

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 · 2023-01-08T19:03:59Z

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.

Shane32 · 2023-01-08T19:42:42Z

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 · 2023-01-09T17:32:29Z

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 graphql-dotnet/graphql-dotnet#3478 (comment)

Shane32 · 2023-01-09T18:45:00Z

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 · 2023-01-10T01:11:39Z

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 · 2023-01-10T01:19:19Z

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 · 2023-01-10T01:45:26Z

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

👍

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.

src/GraphQL.Authorization.ApiTests/BomTests.cs

sungam3r · 2023-01-10T17:43:10Z

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.

Co-authored-by: Shane Krueger <shane@acdmail.com>

codecov-commenter · 2023-01-10T17:53:56Z

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%)`	⬆️
...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 · 2023-01-10T22:44:21Z

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

src/GraphQL.Authorization/AuthorizationValidationRule.cs

+            if ((node is GraphQLOperationDefinition astType && astType == context.Operation) ||
+                (node is GraphQLFragmentDefinition fragment && (context.GetRecursivelyReferencedFragments(context.Operation)?.Contains(fragment) ?? false)))


Shane32 · 2023-02-08T16:51:53Z

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

sungam3r · 2023-02-08T17:39:46Z

I published it just after have read graphql-dotnet/server#987

Shane32 · 2023-02-08T17:45:46Z

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 · 2023-02-08T17:49:03Z

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

Shane32 · 2023-02-08T17:50:07Z

thanks!

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

d8d09ae

sungam3r requested a review from Shane32 January 7, 2023 18:24

github-actions bot added CI CI configuration issue or pull request code style Pull request that applies code style rules documentation test Pull request that adds new or changes existing tests labels Jan 7, 2023

sungam3r commented Jan 7, 2023

View reviewed changes

src/Harness/appsettings.json Show resolved Hide resolved

format

eabcee0

sungam3r commented Jan 7, 2023

View reviewed changes

.github/workflows/publish.yml Show resolved Hide resolved

format

ef3f49f

Shane32 reviewed Jan 7, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

sungam3r added 3 commits January 8, 2023 00:15

fix codeql

dd4561b

format

58e29f2

tab

c264831

sungam3r self-assigned this Jan 7, 2023

sungam3r added 2 commits January 8, 2023 00:44

remove BOM

b78aea7

format

1109583

sungam3r mentioned this pull request Jan 8, 2023

Separate formatting check graphql-dotnet/graphql-dotnet#3491

Merged

Shane32 mentioned this pull request Jan 8, 2023

Additional changes for v7 compatibility #255

Merged

Shane32 and others added 2 commits January 8, 2023 15:09

Additional changes for v7 compatibility (#255)

85b2e4d

remove IProvideClaimsPrincipal

71b3612

Shane32 reviewed Jan 8, 2023

View reviewed changes

src/GraphQL.Authorization.ApiTests/BomTests.cs Show resolved Hide resolved

Shane32 reviewed Jan 8, 2023

View reviewed changes

src/Harness/Program.cs Outdated Show resolved Hide resolved

sungam3r added 2 commits January 8, 2023 19:31

fix

4aadd34

oops

52048f7

Shane32 reviewed Jan 8, 2023

View reviewed changes

fix

65faded

Shane32 approved these changes Jan 10, 2023

View reviewed changes

src/GraphQL.Authorization.ApiTests/BomTests.cs Show resolved Hide resolved

sungam3r and others added 2 commits January 10, 2023 20:47

Update src/GraphQL.Authorization.ApiTests/BomTests.cs

b0600d6

Co-authored-by: Shane Krueger <shane@acdmail.com>

comment

95d8409

Shane32 added this to the 7.0 milestone Jan 10, 2023

Shane32 mentioned this pull request Jan 10, 2023

Fix security flaw with nested fragments #256

Merged

Fix security flaw with nested fragments (#256)

5179bf3

github-advanced-security bot found potential problems Jan 10, 2023

View reviewed changes

sungam3r merged commit bff2776 into master Jan 14, 2023

sungam3r deleted the bump branch January 14, 2023 18:56

Shane32 mentioned this pull request Feb 8, 2023

Validation rules and Policies errors after update to v7 graphql-dotnet/server#987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sungam3r commented Jan 7, 2023 •

edited

Loading

Shane32 commented Jan 7, 2023

Shane32 commented Jan 7, 2023

Shane32 commented Jan 7, 2023

sungam3r commented Jan 7, 2023

sungam3r commented Jan 7, 2023

Shane32 commented Jan 8, 2023

Shane32 commented Jan 8, 2023

Shane32 Jan 8, 2023

sungam3r Jan 8, 2023

Shane32 commented Jan 8, 2023

sungam3r commented Jan 8, 2023

Shane32 commented Jan 8, 2023

sungam3r commented Jan 9, 2023

Shane32 commented Jan 9, 2023

joemcbride commented Jan 10, 2023

joemcbride commented Jan 10, 2023 •

edited

Loading

Shane32 commented Jan 10, 2023

sungam3r commented Jan 10, 2023

codecov-commenter commented Jan 10, 2023

sungam3r commented Jan 10, 2023

Shane32 commented Feb 8, 2023

sungam3r commented Feb 8, 2023

Shane32 commented Feb 8, 2023 •

edited

Loading

sungam3r commented Feb 8, 2023

Shane32 commented Feb 8, 2023

		if ((node is GraphQLOperationDefinition astType && astType == context.Operation) \|\|
		(node is GraphQLFragmentDefinition fragment && (context.GetRecursivelyReferencedFragments(context.Operation)?.Contains(fragment) ?? false)))

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

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

Conversation

sungam3r commented Jan 7, 2023 • edited Loading

Shane32 commented Jan 7, 2023

Shane32 commented Jan 7, 2023

Shane32 commented Jan 7, 2023

sungam3r commented Jan 7, 2023

sungam3r commented Jan 7, 2023

Shane32 commented Jan 8, 2023

Shane32 commented Jan 8, 2023

Shane32 Jan 8, 2023

Choose a reason for hiding this comment

sungam3r Jan 8, 2023

Choose a reason for hiding this comment

Shane32 commented Jan 8, 2023

sungam3r commented Jan 8, 2023

Shane32 commented Jan 8, 2023

sungam3r commented Jan 9, 2023

Shane32 commented Jan 9, 2023

joemcbride commented Jan 10, 2023

joemcbride commented Jan 10, 2023 • edited Loading

Shane32 commented Jan 10, 2023

sungam3r commented Jan 10, 2023

codecov-commenter commented Jan 10, 2023

Codecov Report

sungam3r commented Jan 10, 2023

Shane32 commented Feb 8, 2023

sungam3r commented Feb 8, 2023

Shane32 commented Feb 8, 2023 • edited Loading

sungam3r commented Feb 8, 2023

Shane32 commented Feb 8, 2023

sungam3r commented Jan 7, 2023 •

edited

Loading

joemcbride commented Jan 10, 2023 •

edited

Loading

Shane32 commented Feb 8, 2023 •

edited

Loading