search

ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.23k stars 744 forks source link

Align `HotChocolate.AspNetCore.AuthorizeAttribute` to `Microsoft.AspNetCore.AuthorizeAttribute` #3847

Closed tobias-tengler closed 1 year ago

tobias-tengler commented 3 years ago

Is your feature request related to a problem? Please describe. The way of defining Roles using the Authorize attribute differs between Hot Chocolate and the official attribute.

Hot Chocolate:

[Authorize(Roles = new[] { "Role" })]
[Authorize(Roles = new[] { "Role1", "Role2" })]

ASP.NET Core:

[Authorize(Roles = "Role")]
[Authorize(Roles = "Role1,Role2"]

Hot Chocolate's version is more annoying to type, especially if you only have one role. It also makes it harder to just copy over existing Authorize definitions from a REST API project for example.

Describe the solution you'd like Hot Chocolate's Authorize attribute should also accept a comma separated string for its Roles property. This change can't be performed without introducing a breaking change immediately or making a compromise by renaming the property. Therefor we could also introduce a new attribute GraphQLAuthorize where the type of the property is correct. This would also solve the ambiguity between the two Authorize attributes, which could be the cause of some confusion currently.

michaelstaib commented 3 years ago

Maybe we could just accept Microsofts attribute and internally translate it to ours.

reinux commented 3 years ago

Maybe we could just accept Microsofts attribute and internally translate it to ours.

That would be helpful. I think this should be treated as a high priority, since it's a security risk.

I wouldn't be surprised if there are APIs out there right now which are vulnerable because people had thought they were using the HotChocolate one, but were instead using the Microsoft one.

Just spent an hour trying to figure out why authorization wasn't doing anything, but I was lucky enough to have noticed.

michaelstaib commented 1 year ago

I upgraded @tobias-tengler code and got it to work but realized that this would actually be not good. The main issue with reusing microsoft's attribute is that you are only able to apply things in a certain execution lifecycle stage. This would lead to suboptimal usage of authorization.

The initial issue @tobias-tengler has was with the roles sting ... and we can take this over for our attribute instead ... since we in any case have breaking changes with authorization in HC 13 we might as well break this one.

michaelstaib commented 1 year ago

I think we prefer no change for this as the simplification is not worth the cost it has on the user base.

tobias-tengler commented 1 year ago

Sounds good to me.

This should probably be its own issue, but I mentioned it in my original issue, so I'll just start the discussion here: This alignment would increase the ambiguity between HC's and MS' Authorize attribute. Should we introduce some guard rails like throwing an error when encountering the MS attribute or renaming our attribute? I feel like it's too easy to make a mistake here.

Edit: I was writing this before Michael closed the issue. The ambiguity point still holds true though.

michaelstaib commented 1 year ago

Its difficult ... but maybe as an opt-in?

tobias-tengler commented 1 year ago

Should be opt-out IMO. I'd expect in 99% of the cases the MS attribute on a GraphQL field or type is a mistake caused by the ambiguity. There might be some people that do weird stuff like mapping their API controller as a GraphQL type and those people can opt-out of the error.