CarterCommunity / Carter

Carter is framework that is a thin layer of extension methods and functionality over ASP.NET Core allowing code to be more explicit and most importantly more enjoyable.
MIT License
2.05k stars 172 forks source link

Add possibility to specify policy names when requiring authorization at the module level #310

Closed mderriey closed 1 year ago

mderriey commented 1 year ago

Motivation

It was found by a member of the Carter community Slack that the current RequireAuthorization method on CarterModule doesn't allow to specify one or more policy names. The result is that the only policy that is appliable at the module level is the default authorization policy configured in the app.

Changes

This PR adds another overload of the RequireAuthorization that allows passing one or more policy names.

It's worth noting that ASP.NET Core has even more overloads, see https://source.dot.net/#q=requireauthorization%3CTBuilder%3E:

Do we want to implement some of those?

Tests

Writing tests for this involved quite a bit of plumbing code. If you'd rather not have tests, or if you feel there's an easier way to test this than building a server and firing a request, please let me know.

mderriey commented 1 year ago

I'm in two minds about the tests as what we're really doing is testing the underlying auth logic. I mean we should probably have tests to see if a module specifies RequiresAuthorization() we get a 401 response maybe

I rewrote the tests in https://github.com/CarterCommunity/Carter/pull/310/commits/f48051a6dcb8dc445ab15eedc3cceb59e01ed801 to inspect the endpoints to make sure that the authorization-specific metadata is there.

I'm still not super happy as we're testing the internal implementation of ASP.NET Core when we call RequireAuthorization, but at least we don't make HTTP calls to validate the behavior.

Thoughts?

cbrevik commented 1 year ago

Not sure what the release cycle on Carter is, but could possibly this feature be released with a 7.0.1 version? 😅

@jchannon

jchannon commented 1 year ago

@cbrevik done! =)

cbrevik commented 1 year ago

Wow I did not expect that quick of a feedback, thanks a bunch @jchannon! Running back from the coffee machine to continue work then I guess! 😂

jchannon commented 1 year ago

"Get back to work!" =)

On Wed, 15 Mar 2023 at 12:44, Christian Brevik @.***> wrote:

Wow I did not expect that quick of a feedback, thanks a bunch @jchannon https://github.com/jchannon! Running back from the coffee machine to continue work then I guess! 😂

— Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/pull/310#issuecomment-1469940833, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZVJSR4EJEMDRJRLOO5U3W4G2TDANCNFSM6AAAAAASIYTGAU . You are receiving this because you were mentioned.Message ID: @.***>