Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Microsoft.AspNetCore.Authorization but only for .NET Core #13

Closed
wants to merge 3 commits into from

Conversation

RehanSaeed
Copy link

@RehanSaeed RehanSaeed commented Aug 16, 2018

This is a fairly big PR that makes use of the Microsoft.AspNetCore.Authorization NuGet package when using .NET Core, while continueing to use the duplicate code for full .NET Framework 4.6. It lets you do this:

    .AddTransient<IValidationRule, AuthorizationValidationRule>()
    .AddAuthorization(
        options =>
        {
            options.AddPolicy("Admin", x => x.RequireClaim("role", "admin"));
        })

See #11 for more details.

@RehanSaeed
Copy link
Author

It would be nice if we could add an AddGraphQLAuthorization extension method to the IGraphQLBuilder interface but that exists in a separate repository. Perhaps this project should move into the Server repo?

@RehanSaeed
Copy link
Author

I forgot to mention that the codebase uses GetAwaiter().GetResult() a fair bit. Would be good to switch to using Task everywhere for performance.

@joemcbride
Copy link
Member

There's a lot here so I haven't been able to fully look at it yet. Honestly I mostly just don't want to have to deal with the complexity of maintaining such a big fork in the codebase so admittedly this has languished a bit.

I forgot to mention that the codebase uses GetAwaiter().GetResult() a fair bit. Would be good to switch to using Task everywhere for performance.

I'm not sure where all you're referring to, though the ones I'm aware of are due to Validation Rules in the core project do not yet support Task, whereas this library was built to support Task. So what really needs the update is the Validation Rules in the core GraphQL project.

@RehanSaeed
Copy link
Author

Hesiatation is understandable. Take your time.

@benmccallum
Copy link
Contributor

Nice one @RehanSaeed ! I'd be keen for this as I'm about to implement a bunch of authorization requirements and am on dotnet core.

@RehanSaeed
Copy link
Author

@benmccallum This PR is not going to be merged. See #11 for more info and my next steps.

@benmccallum
Copy link
Contributor

Thanks @RehanSaeed , will continue conversation over there!

@RehanSaeed
Copy link
Author

New PR raised in the Server project at graphql-dotnet/server#171

@RehanSaeed RehanSaeed closed this Oct 10, 2018
@sungam3r sungam3r deleted the use-aspnetcore-authorization branch March 22, 2021 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants