graphql-dotnet / server

ASP.NET Core GraphQL Server
MIT License
581 stars 164 forks source link

Release v6 auth nuget package #765

Closed Shane32 closed 2 years ago

Shane32 commented 2 years ago

I would like to release the 6.0.0 authorization nuget package (only) - it does not depend on any of the other server nuget packages and will allow users to use it in conjunction with GraphQL.AspNetCore3.

We could also release v6 of the UI packages, but the 5.2.1 versions do not use GraphQL, and I doubt they even changed, so I don't really think it matters for them.

@sungam3r ?

To do so I would create a branch with only the auth package and issue a release from that branch. If/when v6 is ready for the other projects, we can release 6.0.1 for the whole set.

Of course if there is any planned updates for the auth project, then we may not want to release it now. I don't see any open issues or pull requests from anytime recently that has to do with the auth project.

sungam3r commented 2 years ago

I would create a branch with only the auth package

By deleting all other code?

if there is any planned updates

Nothing in near future. OK, go on. Recently, I need to spend more time for my daily work, so I will review with a delay.

Shane32 commented 2 years ago

By deleting all other code?

yes

Mithras commented 2 years ago

What about .AddServer() and .UseGraphQL()? These are in GraphQL.Server.Core.dll and GraphQL.Server.Transports.AspNetCore.dll. Are there plans to make them work with GraphQL 5.x?

Mithras commented 2 years ago

I'm not sure I understand how responsibilities change between GraphQL and Server. All serialization stuff is now part of GraphQL, right? Does that mean we don't need GraphQL.Server.Transports.AspNetCore.NewtonsoftJson and GraphQL.Server.Transports.AspNetCore.SystemTextJson anymore? I also saw GraphQLMiddleware.cs in GraphQL 5.x. Does it replace GraphQL.Server.Core?

Shane32 commented 2 years ago

@Mithras :

With the above changes, there is literally NOTHING left in Server.Core. Not one class or interface at all. And as stated above, the serialization projects are unnecessary, leaving the following:

I do not see a need to separate the websocket projects from the main server project, nor the need for a websocket abstraction library separate from the implementation, nor even Core or All. However, All may continue to live on containing the references to all the common GraphQL libraries.

The current websocket implementation does not support the newer graphql-ws protocol, and there some bugs in the implementation. It also has a dependency on the DataFlow library.

I have taken the server project and rewritten it to address all of the above issues. I have also added more configurable features, made it easier to use, added more sample projects, implemented more bug fixes, added newer subscription protocol support, removed the DataFlow dependency, increased code coverage to 96%, and so on. There is just a single nuget package; separate nuget packages are not necessary for websockets support.

With the GraphQL.AspNetCore3 nuget package, you can install middleware with a single line of code - app.UseGraphQL("/graphql"); - which enables websockets middleware as well as GET and POST middleware. (You will need the AspNetCore app.UseWebsockets(); to enable websocket support to begin with.) There is no need for AddServer or any other code. You can also configure a number of options by passing options to the call to UseGraphQL. See package and documentation below:

The repo does not include the authentication nuget package, nor the UI packages. Since the UI packages do not depend on GraphQL at all, the UI packages from server 5.2.1 can continue to be used. It's just the auth package that needs a new release that doesn't depend on GraphQL.Server.Core so that it can be used with GraphQL.AspNetCore3

I will continue to issue PRs in order to move the functionality from GraphQL.AspNetCore3 to GraphQL.Server - but as of now the server project is notably behind.

I also saw GraphQLMiddleware.cs in GraphQL 5.x. Does it replace GraphQL.Server.Core?

That file is part of a basic sample that is provided. It is not included in any published nuget package.

Mithras commented 2 years ago

Thank you, @Shane32! Can you please reply to the thread when there is an Authentication package that works with GraphQL 5.x? I don't think I can migrate until it's available. I will try GraphQL.AspNetCore3 though, thanks!

Mithras commented 2 years ago

Also, do you have anything for code-first Federation? I currently have a pretty robust Federation implementation in my repo for GraphQL 4.7.x. I'm planning on refactoring it for GraphQL 5.x sometime next week.

Shane32 commented 2 years ago

I’m also thinking about adding an auth validation rule to GraphQL.AspNetCore3 - since all the auth dependencies are within AspNetCore, I think it makes sense. I could also make the auth code run a lot faster than it does now, and fix/remove the calls to GetAwaiter().GetResult(). I wish the auth stuff supported roles not just policies but the helper methods exist in GraphQL so a change would need to be made there for that feature. This would put authorization on par with typical ASP.Net MVC Authorize attribute

Mithras commented 2 years ago

@Shane32 I successfully migrated all our projects to GraphQL 5.x + GraphQL.AspNetCore3 and everything seems to be working even my code-first Federation hack. I saw you are working on Auth in GraphQL.AspNetCore3, will it work with AuthorizeWithPolicy() when done or only with attributes?

Shane32 commented 2 years ago

Yes it will. As of now only output graphs and fields, not input graphs or their fields. Do you need support for authorization attributes on input graphs?

Mithras commented 2 years ago

Does mutation count as an output type? If so, then no. We only use auth in output types.

Shane32 commented 2 years ago

Yes it will work for mutations. I’ll try to issue a release by tomorrow. Maybe tonight.

sungam3r commented 2 years ago

@Shane32 Are you ready to release all server packages?

Shane32 commented 2 years ago

I wouldn't; it should be deprecated. GraphQL.AspNetCore3 completely replaces the need for the server project. It has many bug fixes, supports the newer websockets sub-protocol, it is all in a single nuget package, and it has no dependencies besides GraphQL. And it will shortly have much better support for authentication - here's just one example:

When any browser initiates a WebSocket connection, the only type of authentication supported is cookie-based authentication. This means that JWT authentication, fairly typical in a GraphQL project, cannot be implemented over a WebSocket connection to a browser due to a browser limitation, not a technical one. So this is why both the old graphql-ws and graphql-transport-ws sub-protocols in use today require an initialization message prior to any communication. (See MessageType.GQL_CONNECTION_INIT.) The purpose of this message is for the endpoint to provide authentication prior to allowing GraphQL requests. But with the current implementation, there is no way to process or handle this message, and the implementation does not even require the message to be sent in accordance with the spec at all. (See ProtocolMessageListener's HandleAsync and HandleInitAsync methods.)

A second example is the user context. In the server project, the user context for any request over WebSockets is a MessageHandlingContext instance that contains connection properties. The IUserContextBuilder is completely ignored.

The GraphQL.AspNetCore3 project is much easier to use and provides much more control. You can set it up with a single line of code, and you can add properties to enable disable GET, POST or WebSocket requests. (For instance, I usually have GET requests return a UI, and only POST requests are handled.) It properly validates incoming queries to be sure that mutations are not executed over GET connections, per spec, which the current implementation does not. It is configurable as to whether the query string can override POST content. It has 96% code coverage. Almost everything can be customized through inheritance if not already supported. It has better support for scoped services. Subscriptions propagate cancellation tokens correctly, including application shutdown requests. It has better documentation. It has better samples. It includes a ExecutionResultActionResult class and sample application for those running GraphQL through a MVC controller. The list goes on and on (and on and on).

I don't see any reason to release a new version of the Server project in its current state. It should be deprecated and/or the code copied over from GraphQL.AspNetCore3. And if we do release v6, we should still deprecate it.

I should have a version with authentication support released today. And hopefully within the next few days I will have time to add a sample SPA project with full JWT token support (including subscriptions over WebSocket connections).

Mithras commented 2 years ago

or the code copied over from GraphQL.AspNetCore3

I'd love to see this happen. Right now it's hard to discover GraphQL.AspNetCore3 and I only learnt about it in this thread.

A bit of of topic. @Shane32 what's your take on implicit graph types (aka AutoRegisteringObjectGraphType)? I'm thinking about switching as some things seem to be easier (e.g. today we have a custom .AddAllProperties() extension that uses reflection to add fields for all simple properties which won't be necessary with AutoRegisteringObjectGraphType) but at the same time not everything seems to be supported yet (e.g. no support for interfaces). I'll give it a try for sure but I'm still not sold on it.

Shane32 commented 2 years ago

@Mithras I spear-headed the work on the new design of the AutoRegisteringObjectGraphType; wrote almost all of the code that exists in it now. It should be ideal for building out mutation graphs and query root graphs or subgraphs. And I think AutoRegisteringInputGraphType is ideal for input graphs of any type.

As for output data models, I find that unless it's a smaller project, I may not want to mix my database models with my GraphQL code models. So either you mix them and end up with something like this:

[Name("Widget")]
class Widget
{
    [Id]
    public int Id { get; set; }

    public string Name { get; set; }

    [Ignore] //don't map to GraphQL
    public int InStock { get; set; }

    [Ignore] //don't map to GraphQL
    public int Allocated { get; set; }

    [NotMapped] //don't map to database
    [Name("InStock")]
    public int Available => InStock - Allocated;

    public string PictureUrl([FromServices] PicService picService) => picService.GetPictureUrl(Id);
}

Or, you can use a 'variant', a derived class from AutoRegisteringObjectGraphType to allow separation of the data model from the GraphQL model. A sample of this can be found here:

https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL.Tests/Bugs/Issue2932_DemoDIGraphType.cs

It lets me do this:

// data model
class Person
{
    public int Id;
    public string Name = null!;
    public int InStock { get; set; }
    public int Allocated { get; set; }
}

// graphql model
[Name("Widget")]
public class WidgetGraph : DIGraph<Widget>
{
    [Id]
    public int Id() => Source.Id;

    public string Name() => source.Name;

    public int InStock => Source.InStock - Source.Allocated;

    public string PictureUrl([FromServices] PicService picService) => picService.GetPictureUrl(Source.Id);
}

Of course then for every field another object gets created; if you're concerned with GC then you can use static methods along with [FromSource] but the code gets more complicated. You can look at the file linked above for more samples -- see the SampleGraph class. Another benefit to that coding pattern is that it supports DI-injected services (see the file linked above).

But for database queries, I don't actually do that either very often, because I have a whole layer of code that takes GraphQL requests and turns it into SQL queries, only selecting the fields necessary.

Shane32 commented 2 years ago

Here is my 'wish list' that I did not finish for v5. Support for interfaces is probably the biggest thing on there. Most of the reflection code is written out; the challenge is just trying to figure out the logical way to make it both flexible and easy to use. Does it infer from .NET inheritance, or do you use an attribute? Or maybe it looks at .NET inheritance, but you have to mark the interface with an attribute that indicate it's intended to be a GraphQL interface. Then does it get automatically added to the schema, or does you need to scan the assembly with an extension method?

https://github.com/graphql-dotnet/graphql-dotnet/issues/2893

Shane32 commented 2 years ago

Keep in mind that with a few lines of code, you can easily write your own attribute which will add code to the configuration of the auto-generated graph type. So then you'd just write graphs like this:

[SupportsInterface(typeof(AnimalGraphType))]
class Cat
{
   public string Name { get; set; }
}

You'd still need to use the code-first style to define AnimalGraphType for the time being.

Mithras commented 2 years ago

I have a whole layer of code that takes GraphQL requests and turns it into SQL queries

I didn't go as far as generating SQL but I did something like image where the main trick is ctx.GetMembers() returning list of fields from query and .ProjectTo<GeographyDto>(members) is just Automapper .ProjectTo() combined with Explicit expansion

Shane32 commented 2 years ago

Nice!

sungam3r commented 2 years ago

it should be deprecated

very debatable

I don't see any reason to release a new version of the Server project in its current state.

The reason is (at least) the ability to update already written code once before possible deprecation.

It should be deprecated and/or the code copied over from GraphQL.AspNetCore3. And if we do release v6, we should still deprecate it.

This project is familiar to many developers. And it is included in the graphql-dotnet organization. Your project can be as far as good, but this is not enough to abandon that "official" one. They solve the same task, good. If GraphQL.AspNetCore3 is a deep rework of graphql-server then of course it makes sense to consider the possibility of transferring code from GraphQL.AspNetCore3 to graphql-server. I do not know how quickly this can be done (reviewed) if you want to go this way. While both projects can be used in parallel. I will definitely use graphql-server v6 in next "update iteration" for company projects.

Shane32 commented 2 years ago

The reason is (at least) the ability to update already written code once before possible deprecation.

Reasonable

This project is familiar to many developers. And it is included in the graphql-dotnet organization.

True. But excluding the changes already made to the server project, the changes between the projects are minimal -- much less than any transition from GraphQL 4 to 5. All that's typically necessary is to remove a few lines in Startup.cs - that's it. So "familiar" here refers to the name of the nuget package, not the API.

Your project can be as far as good, but this is not enough to abandon that "official" one.

This is an absurd statement. "Can be as far as good"? It is infinitely better. Did you not read my message above? Have you not been following any of the complaints and issues with the project? "Not enough"? How many deficiencies and bugs do I have to list in the "official" version until it is abandoned? 10? 50? 100? Have you actually looked any any of my code? I've asked for help reviewing it. I would welcome your input. But I need reliable software to run my company's servers. I need subscription support, and I can't have issues like some users have had here, waiting for months with no answers (see #621).

I could just take my copy off github, and keep it closed-source. What good does that do the community? I'm trying to help here and keep this project alive.

As for abandoning the "official" project, even Apollo abandoned their "own" websocket protocol in favor of a better protocol and actively recommend users to switch. This is open source software! Microsoft has done the same for parts of their API. Everyone should be trying to do their best for the community.

If GraphQL.AspNetCore3 is a deep rework of graphql-server then of course it makes sense to consider the possibility of transferring code from GraphQL.AspNetCore3 to graphql-server.

It is a deep rework, and I have been requesting this.

I do not know how quickly this can be done (reviewed) if you want to go this way.

You mean you would want to review it all in deep detail. You don't want to trust me to this, and you don't have the time to go through it all. 🤷‍♂️ The software should not be hindered because of your schedule.

I will definitely use graphql-server v6 in next "update iteration" for company projects.

If you want to use old obsolete buggy code, go ahead. Perhaps you do not use subscriptions and the bugs don't affect you. We should not impose your will on what's best for the community.

In my opinion, we should not recommend anyone to use the server project if they need authorization or subscription support. If neither, okay, but 90%+ of the project's codebase exists to support authorization and subscription. Once GraphQL.Server has been brought up to par, then sure. But that simply is not the case now.

Mithras commented 2 years ago

The software should not be hindered because of your schedule

I see that a lot here. I'd like to enable code-first Federation but I don't want to create a PR which will be ignored for years like https://github.com/graphql-dotnet/graphql-dotnet/pull/1669 (which is so obsolete today that it's easier to start from scratch). Unlike server, creating a separate repo makes very little sense as most Federation code is already in GraphQL repo, just not code-first part.

Shane32 commented 2 years ago

@Mithras Do you have a working federation implementation now? Also see https://github.com/graphql-dotnet/graphql-dotnet/issues/2997 . I have not spent the time to try to get the sample running, but I "should". It seemed that @killjoy2013 had problems running it with 4.7.1. In any case, I have spent all my free time recently rewriting subscription and authorization support, and consequently the entire server project, and have not had time to even look at what the issue was.

It would be oh-so-cool if we could take the sample project he's written, and somehow implement it as a CI script to test federation functionality with a "live" server. Once that works, it would be easy enough to make changes to GraphQL.NET and know that federation isn't broken in the process. But I am not knowledgeable on bash, Node applications, or federation, and I don't have the time to spend learning it right now.

Also, the Apollo team requested support for @key, @requires and @provides. See https://github.com/graphql-dotnet/graphql-dotnet/issues/2968 . Never having used federation, I have no idea what those directives are for and I wouldn't know where to begin.

If you want to get a PR approved, the best way is to be able to demonstrate with tests that it works. That, and to just keep "bump"ing the PR. I can find time to do anything, if I'm pestered enough! At the moment I'm under a lot of pressure at work, so 110% of my energy is going into subscription/authorization support, since it is needed there "yesterday".

Shane32 commented 2 years ago

@Mithras I released GraphQL.AspNetCore3 v2.0.0 - Please let me know if it works as expected in regards to authorization.

sungam3r commented 2 years ago

I could just take my copy off github, and keep it closed-source. What good does that do the community?

What does this have to do with my words?

The software should not be hindered because of your schedule.

Do I somehow prevent the development of your new project?

If you want to use old obsolete buggy code, go ahead. Perhaps you do not use subscriptions and the bugs don't affect you. We should not impose your will on what's best for the community.

We are talking about different things and you do not hear me. Community can be happy to use your new project and I do not have any desire to interfere. Good job. At the same time, it is impossible in 100% cases to migrate the code base from one time-tested solution to another. Even if this is another solution is positioned as a super-super improvement with many-many-many bugfixes and new features. Even if from the point of view of changes in public API, everything comes down to several rows of code. Even if in public API all comes down to changing several lines of code. In some environments, the stability of the work of proven solutions (even buggy one) is preferable. Yes, the main use cases may not be affected by these bugs.

You don't want to trust me to this

Again. Do not misunderstand me. This is not a question of my trust. I just can't come to my boss and say "Hey! OK, now we will update all our 500 banking microservices for this new project. It appeared quite recently and supported by one developer but it is so cool! Let's start!" Such a model is suitable for tech startups maybe, but not for huge enterprise solutions (where I work).

That is how it works for enterprise solutions:

  1. Update GraphQL.NET core execution engine to next major v5 across all tech stack of libs.
  2. Test
  3. Update graphql-server project to next major v6 on top of GraphQL v5 across all tech stack of libs.
  4. Test
  5. Update 1/3/5/10 services to the "brand-new" stack.
  6. Test. Make gren-blue deployment.
  7. Running some time (~week or so) and waiting fos bugs.
  8. Fix found bugs if any. Iterate 1-8 if needed.

And only after that it is possible to replicate that approved and well-tested software to absolutely all services of the company.

GraphQL API is a core part of our infrastructure across company. I do not have the right to take on the risks associated with the update without any thoughtful acquaintance with the new code. And I'm sure that this is not a kind of special situation in enterprise.

Once again I repeat - nothing prevents (and should not prevent of course) to use both projects in parallel (at least for some time, maybe for graphql-server v6.x.x timeline). It is worth allowing the choice and time to update.

Summing up - I think it is necessary to release graphql-server v6 in coming days. Do you have any issues that need to be included in this release?

Shane32 commented 2 years ago

I would only remove dependency on Dataloader from core and move to All. Use only GraphQL in core

Shane32 commented 2 years ago

Well with core gone, that’s moot.

Shane32 commented 2 years ago

No there’s nothing in the immediate timeframe that I would change.

Shane32 commented 2 years ago

We do have a oft-repeated problem of misunderstanding each other!

Shane32 commented 2 years ago

@sungam3r If you want me to, I can post a PR containing my changes to GraphQLHttpMiddleware. It is quite similar, as the executing methods generally contain mostly the old code. From a high-level view, the differences are:

I also added a validation rule to ensure that GET requests do not execute a mutation, per the "GraphQL over HTTP" spec, and that subscription requests do not arrive over GET or POST calls but only WebSocket connections.

I have removed the protected virtual CancellationToken property. I still can't understand in what scenario it would be useful. It could be re-added easily.

Let me know if you want me to post a PR containing this code for inclusion prior to v6.

sungam3r commented 2 years ago

I'm glag to migrate/update this codebase to your reworked solution especially if it can be done step by step.

subscription requests do not arrive over GET or POST calls but only WebSocket connections

🤔 Initial subscription request or what?

I have removed the protected virtual CancellationToken property. I still can't understand in what scenario it would be useful. It could be re-added easily

Well, you may try remove it and I'll review if we can change our code in a way that does not require this method. But for now we do use this method to mix token from http context with one more special token builded from some async-local custom context.

sungam3r commented 2 years ago

755 #766 ?

sungam3r commented 2 years ago

OK to merge #772 ?

Shane32 commented 2 years ago

I would merge #755 and close #766 . Yes #772 whatever you think.

Shane32 commented 2 years ago

Initial subscription request or what?

No, a subscription request over GET or POST right now would return a valid ExecutionResult from the DocumentExecuter that the Server project would attempt to serialize and return, which is impossible. Attempting a subscription request should fail unless it is over a transport that supports streaming the results, which in this case is a WebSocket connection.

especially if it can be done step by step

Well, 100% of the subscription support is new and completely different. There's no 'stepping' here. I've already posted a draft in #749 and my final implementation is based on that prior work. There's not a single line of code that's the same. My authorization project is also new, but there wasn't much to the old project to begin with.

It probably would be:

  1. Middleware (not a big change; likely easy to review)
  2. Authentication (100% new implementation; complicated implementation, but only a few files)
  3. WebSockets (100% new implementation; many classes and has a complex nature)
  4. Sample projects (100% new; very small and easy to review)

In addition there would be a few PRs to change misc stuff like builder methods, code style/formatting, the build process and so on.

The most notable change in my authorization implementation is that authorization errors, by default, do not reveal any information about what caused the authorization failure. I feel this is a security breach to do so. So rather than "User failed to possess the role SuperAdmin", you get "Access denied accessing node 'price' on type 'Widget'.". This seems in line with all other typical security implementations; I do not know of any that respond with a message telling you what role or policy is required. However, a user may implement IErrorInfoProvider or derive from the middleware to make a change. We could implement a backwards-compatible IErrorInfoProvider instance or static method or something, but I wouldn't.

We should also keep in mind that as of now there is no way to filter a query or introspection query based on authentication status. If we were to do so, ideally it would act as though "price" did not even exist in the schema if you were not authorized to view it, and we would also need to modify the code that generates error messages such as "Field 'pricing' was not found; did you mean 'price'?". However this would need to be implemented in the base GraphQL library; it cannot be implemented in a third party library alone.

As for the WebSocket implementation, I have removed all uses of ILogger. Users would have to inherit from my implementation (but then it would be very easy to add logging). It is also much more stringent on the requirements of the protocol, both the new and old version. Other than that, it should not matter to users which version to use. The old version offered no virtual methods and could not be derived from for custom behavior. There's no reason to retain a compatible API.

The only part of the websocket implementation I'm not positive about is the keep-alive implementation for the new websocket protocol. For the old protocol, if a keep-alive was sent once, the client expected it to be sent again within a fixed timeout. This is not the fault of my code or the protocol, but a convention implemented by the common library in use today. The new protocol is specifically designed with a different approach to keep-alive packets. So my code should work correctly, but if the common implementation expects differently, then perhaps a change would need to be made. (It can be altered through options and inheritance, so the code can still be used even as-is.) I need a working sample that uses the new protocol for further testing; I'm just going off the spec at this point.

sungam3r commented 2 years ago

No, a subscription request over GET or POST right now would return a valid ExecutionResult from the DocumentExecuter that the Server project would attempt to serialize and return, which is impossible. Attempting a subscription request should fail unless it is over a transport that supports streaming the results, which in this case is a WebSocket connection.

I guess I do not understand the basic things. subscription a { name } is a GraphQL request and I can send it via HTTP POST now (at least now, I don't know what the spec says).

Agree about authentication changes around error messages. I don't have experience with WebSockets.

Shane32 commented 2 years ago

Essentially you can send it over POST, but you cannot receive it; there is no initial reply, and so the HTTP connection is terminated before any streaming results are streamed. I'm sure you would understand once you take the time, as I had to do, to thoroughly understand subscriptions and its interaction with WebSockets.

sungam3r commented 2 years ago

Yeah

Mithras commented 2 years ago

Do you have a working federation implementation now?

Yes, we use 4.7.1 with Federation in prod and I have working Federation with 5.1.1 locally. I still want to refactor it some so I'll be able to share some code like next week if you are interested.

Also see https://github.com/graphql-dotnet/graphql-dotnet/issues/2997 .

As far as I can tell there is no Federation in starwars-subgraph at all. At the end of the day whole Federation is just about adding two fields to Query: _services and _entities. Gateway is using { _services: { sdl } } to get slightly modified graph schema with additional metadata (@key, external, etc) and then Gateway sends requests to subgraphs like

{
    _entities(representations: [{ __typename: "Droid", id: 123 }]) {
        ... on Droid {
            name
        }
    }
}

to get name for Droid with id 123, for example. That's pretty much it.

It would be oh-so-cool if we could take the sample project he's written, and somehow implement it as a CI script to test federation functionality with a "live" server.

As you can see from my example above, you don't need any gateways, "live" servers or whatever. You can just unit test that { _services } and { _entities } return expected results. I can share some working code with unit tests after I finish refactoring it for 5.x.

Also, the Apollo team requested support for @key, @requires and @provides. See https://github.com/graphql-dotnet/graphql-dotnet/issues/2968 . Never having used federation, I have no idea what those directives are for and I wouldn't know where to begin.

These are Federation metadata that's added to graph schema when you query it via { _services { sdl } }. See Federation schema specification

The thing is most of the Federation is already there: Federation. There are already all types, FederatedSchemaBuilder, FederatedSchemaBuilder, everything. It's just schema first only as of right now. So the only code that's missing is adding Federation metadata to types/fields and adding two fields to Query in a code-first way.

I released GraphQL.AspNetCore3 v2.0.0 - Please let me know if it works as expected in regards to authorization.

Thank you! I'll try it right now.

sungam3r commented 2 years ago

There are already all types, FederatedSchemaBuilder, FederatedSchemaBuilder, everything. It's just schema first only as of right now.

Yes. Everything but schema first only. I do not leave hope to finish the PR with GraphType-first approach. All these directives, metadata, introspection and so on.

Event more - there is PR (if I posted it, I did not remember) with complete rework of SchemaPrinter and FederatedSchemaPrinter. It was blocked by federation stuff. Alas, free time is always not enough.

Shane32 commented 2 years ago

@Mithras Ok, perhaps we can come up with unit tests to validate a schema works for federation, and does so for both schema-first and subsequently code-first. I don't have time to help on this, but maybe @sungam3r does, or perhaps @sungam3r has already made progress there?

Perhaps if I have some free time I'll read up a bit on the links you provided. I really don't have any time to spend on developing or validating it though. Thanks for being willing to help here!

Mithras commented 2 years ago

@Shane32 I think I'm missing something. I've changed version to 2.0.0 and added .AddAuthorization() call on IGraphQLBuilder but it seems that this.AuthorizeWithRoles("qwe"); and this.AuthorizeWithPolicy("rty"); don't do anything. I have them in graph type constructors and field resolvers, e.g. image I also tried adding [Authorize("qwerty")] to classes with no result either. The only difference from AuthorizationSample is that I'm not using .AddAutoSchema(). Is this somehow related?

Shane32 commented 2 years ago

did you add the validation rule in the builder?

services.AddGraphQL(b => b
    .AddAuthorization()
    //other stuff here
);
Mithras commented 2 years ago

I think I see what's happening. Only the first query skips authorization. .AuthorizeWithPolicy() gets called only when the query runs for the first time and only subsequent requests seems to take that into account.

Another issue is that only this seems to be working (for subsequent requests): image In server implementation I was always calling this.AuthorizeWith() even in field resolvers.

This doesn't do anything even for subsequent requests image

Shane32 commented 2 years ago

Do you use the builder methods at all? Do you use a custom document executer? If you post your DI config - at least the AddGraphQL part, it will probably help me understand what's going on and what to fix it. Chances are somehow the validation rule is not getting added to the document executer options.

Meanwhile, there is caching of policy checks. I'm going to double check they don't carry over from one execution to the next.

Mithras commented 2 years ago

well, I have

  1. builder.Services.AddAuthentication(...);
  2. builder.Services.AddAuthorization();
  3. builder.Services.AddGraphQL(gqlBuilder => gqlBuilder
        .AddSystemTextJson()
        .AddDataLoader()
        .AddAuthorization()
        .AddErrorInfoProvider(opt => opt.ExposeExceptionStackTrace = builder.Environment.IsDevelopment())
        .AddSchema<EcoManagerSchema>()
        .AddGraphTypes(typeof(EcoManagerSchema).Assembly)
    );
  4. app.UseAuthentication();
  5. app.UseAuthorization();
  6. app.UseGraphQL<EcoManagerSchema>();
Shane32 commented 2 years ago

looks good there.

Mithras commented 2 years ago

I mean it was working with server@5.2.0 and GraphQL@4.7.1

Shane32 commented 2 years ago

If you do this:

image

Then it will not be assigning the policy until after the field is executed. Also, you are opening yourself up to threading problems with multiple threads attempting to write to the metadata dictionary at once.

But this should work correctly:

image

Perhaps I can alter the sample in a branch to be code-first instead of "type-first".