Add middleware for automatic authorization based on request endpoint #55

wanderzirkus · 2024-01-07T16:48:32Z

Hi there.

First things first: Thanks for this repo and the good work! :) This is also my first PR on GitHub ever, so I hope I didn't violate any policies with my PR.

Lets get to the topic: I like using the Keycloak authorization services in a way, that I don't have to recompile if any thing about authorization changes (users, roles, permissions or even policies). More over I want a clean separation of concerns and that for I don't want to annotate the request endpoints in the controllers with authorization specific information (except "AllowAnonymous" of course).
To achieve this, I'd like to have a middleware, which takes path and method of the request, pass them to the "IKeycloakProtectionClient.VerifyAccessToResource()"-method and proceed with the request depending on the result.

What do you think about it?

Please let me know if I should any unittests for this, too.

NikiforovAll · 2024-02-29T08:50:41Z

@wanderzirkus
First of all, thank you for your contribution. This kind of middleware is useful for specific scenarios as you described. I will think about how to include this kind of contribution to this project, but I don't think it is a good idea to include it in a core due to reason I described above.

We could create something like Keycloak.AuthServices.Contrib project and put it there

NikiforovAll · 2024-04-24T10:42:52Z

This could be interesting, could you please consider adding some attributes that provide metadata regarding Resouce and Scope?

Also, how do we validate based on entity Id? E.g: we have a rule for specific entities to be protected. The default path convention is interesting, but it requires certain conventions on the side of the authorization server, which is not always true. I think we need to be able to adapt C# code

wanderzirkus · 2024-04-27T17:47:20Z

Hey thanks for your interest, glad to here that! :)
What exactly do you mean with attributes providing metadata regarding Resource and Scope? Do you mean additional comments?

The idea is that the authorization server has appropriate resources prepared:
Resource uri = request path
Resource scope = request method
If there is a usecase that a specific entity needs to be protected, which dont match this pattern, another attribute analogous to "AllowAnonymous" could be introduced for bypassing the middleware. The name could be "ExplicitAuthorization" for example. The clearity of the automatic authorization concept would suffer a bit from this, I think. Especially at keycloak site.

Let me know if I understood you correctly and what you think about my thoughts.

NikiforovAll · 2024-04-28T06:39:10Z

I would like to suggest you have a quick call to discuss this functionality. Could you please write me an email so we continue discussing time slots?

… working example for URI based keycloak authorization

…tor Middleware; Extend example app

…essful auth in middleware

… working example for URI based keycloak authorization

…tor Middleware; Extend example app

…essful auth in middleware

…thub.com/wanderzirkus/keycloak-authorization-services-dotnet into feature/auto-protect-resource-middleware

NikiforovAll

Thank you for the PR, I left some comments. We might need to have a call to resolve them to save your time

NikiforovAll · 2024-05-06T06:56:29Z

samples/AutoAuthorization/Controllers/ApiController.cs

+    [HttpGet]
+    [Route("/api/explicit-auth")]
+    [ProducesResponseType(statusCode: 200, type: typeof(string))]
+    [ExplicitResourceProtection("/api/auto-auth", "GET")]


I think we can rename it to something simple:

ProtectedResource(string resource, string scope)

Ok edit: after giving it a second thought, I don't like ProtectedResource as much, because it implies, that all methods, that dont have the attribute are not protected. Thats why I put in "explicit" as a prefix.

NikiforovAll · 2024-05-06T06:58:06Z

samples/AutoAuthorization/Controllers/ApiController.cs

+    [HttpGet]
+    [Route("/api/no-auth")]
+    [ProducesResponseType(statusCode: 200, type: typeof(string))]
+    [DisableUriBasedResourceProtection]


I suggest not focus on URI based, but on the general one, so it could be [ProtectedResouce(Disable = true)]

Let me know what you think about this design. I like it because you don't need to remember a lot of other attributes.

Good idea :)

NikiforovAll · 2024-05-06T06:58:53Z

samples/AutoAuthorization/ServiceCollectionExtensions.Auth.cs

+    {
+        services.AddKeycloakAuthentication(configuration);
+
+        services.AddAuthorization(authorizationOptions => authorizationOptions.FallbackPolicy = new AuthorizationPolicyBuilder()


There is a new helpful time called, please consider using it:

https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.authorization.authorizationbuilder?view=aspnetcore-8.0

What do you mean exactly?

NikiforovAll · 2024-05-06T06:59:45Z

samples/AutoAuthorization/ServiceCollectionExtensions.Auth.cs

+{
+    public static IServiceCollection AddAuth(this IServiceCollection services, IConfiguration configuration)
+    {
+        services.AddKeycloakAuthentication(configuration);


Migrate to 2.0.0. Basically, you need to rebase on the main branch to see the latest changes. We can setup a call to migrate to 2.0.0 together if needed

How much trouble would it be for you to do the migration? I don't know if I have the time to do this currently, depending how much it is.

NikiforovAll · 2024-05-06T07:03:08Z

...uthServices.Authorization/UriBasedResourceProtection/UriBasedResourceProtectionMiddleware.cs

+/// b) annotate controller methods with the "[Authorize]" attribute
+/// It is possible to exclude methods from the policy enforcement by annotate them with the "[AllowAnonymous]" attribute, however.
+/// </summary>
+public class UriBasedResourceProtectionMiddleware


Can we add some tests? We can discuss what types of tests are needed for this one.

There is a collection of unittest. Which additional tets do you have in mind?

NikiforovAll · 2024-05-06T07:06:55Z

....Authorization.Tests/UriBasedResourceProtection/UriBasedResourceProtectionMiddlewareTests.cs

+using Moq;
+
+#pragma warning disable CS8625 // Cannot convert null literal to non-nullable reference type.
+[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1707:Identifiers should not contain underscores", Justification = "<Pending>")]


Inline suppression should be avoided, let's configure it via .editorconfig for test project.

Sure thing. I didn't want to change it, because its your project after all and you are making the style rules :)

NikiforovAll · 2024-05-06T07:07:10Z

....Authorization.Tests/UriBasedResourceProtection/UriBasedResourceProtectionMiddlewareTests.cs

+        var requestDelegateMock = new Mock<RequestDelegate>();
+
+        // Act & Assert
+        Assert.Throws<ArgumentNullException>(() => _ = new UriBasedResourceProtectionMiddleware(requestDelegateMock.Object, null));


Consider Fluent Assertions

NikiforovAll · 2024-05-06T10:05:11Z

Also, I think this case is partially covered by ProtectedResourcePolicyProvider. Could you please take a look?

https://nikiforovall.github.io/keycloak-authorization-services-dotnet/authorization/policy-provider.html

NikiforovAll · 2024-05-06T10:51:45Z

This is a very niche functionality, We should consider creating Contribution repository that will contain components from the Community.

…rceProtectionAttribute; adapted tests; adapted samples; disable to rules by test specific .editorconfig

… working example for URI based keycloak authorization

…tor Middleware; Extend example app

…essful auth in middleware

… working example for URI based keycloak authorization

…tor Middleware; Extend example app

…rceProtectionAttribute; adapted tests; adapted samples; disable to rules by test specific .editorconfig

…thub.com/wanderzirkus/keycloak-authorization-services-dotnet into feature/auto-protect-resource-middleware

rename AutoProtectResource to UriBasedResourceProtection; add minimal working example for URI based keycloak authorization introduce DisableAutoProtection and ExplicitResourceProtection; Refactor Middleware; Extend example app add testclass for middleware; fix bug where status set to 401 on successful auth in middleware add middleware and builder extension method for usage rename AutoProtectResource to UriBasedResourceProtection; add minimal working example for URI based keycloak authorization introduce DisableAutoProtection and ExplicitResourceProtection; Refactor Middleware; Extend example app merged DisableUriBasedResourceProtection attribute into ExplicitResourceProtectionAttribute; adapted tests; adapted samples; disable to rules by test specific .editorconfig add middleware and builder extension method for usage rename AutoProtectResource to UriBasedResourceProtection; add minimal working example for URI based keycloak authorization introduce DisableAutoProtection and ExplicitResourceProtection; Refactor Middleware; Extend example app add testclass for middleware; fix bug where status set to 401 on successful auth in middleware add middleware and builder extension method for usage rename AutoProtectResource to UriBasedResourceProtection; add minimal working example for URI based keycloak authorization introduce DisableAutoProtection and ExplicitResourceProtection; Refactor Middleware; Extend example app merged DisableUriBasedResourceProtection attribute into ExplicitResourceProtectionAttribute; adapted tests; adapted samples; disable to rules by test specific .editorconfig

…thub.com/wanderzirkus/keycloak-authorization-services-dotnet into feature/auto-protect-resource-middleware

…t-resource-middleware

wanderzirkus · 2024-05-11T08:04:11Z

Alright, I think I got the latest version of main merged into my feature branch now. I forgot to fetch the latest version from remote before rebasing/merging *facepalm
I see the migration issue now, too. What should we use instead of "VerifyAccessToResource" on the IKeycloakProtectionClient?

wanderzirkus · 2024-05-11T08:17:04Z

This is a very niche functionality, We should consider creating Contribution repository that will contain components from the Community.

Well if you don't want this feature at all feel free to just close the PR. I find it most useful and thought I could give s.th. back, but I am not upset, if you say its to niche to be considered valuable enough for the repo. :)

NikiforovAll · 2024-05-11T10:44:19Z

@wanderzirkus let's discuss the feature in discord. Do you have time for that?

NikiforovAll · 2024-05-11T10:52:34Z

Alright, I think I got the latest version of main merged into my feature branch now. I forgot to fetch the latest version from remote before rebasing/merging *facepalm I see the migration issue now, too. What should we use instead of "VerifyAccessToResource" on the IKeycloakProtectionClient?

It's a https://nikiforovall.github.io/keycloak-authorization-services-dotnet/authorization/resources-client.html

add middleware and builder extension method for usage

aeef9ac

Friedemann Braune added 8 commits May 4, 2024 13:28

rename AutoProtectResource to UriBasedResourceProtection; add minimal…

afb7d27

… working example for URI based keycloak authorization

introduce DisableAutoProtection and ExplicitResourceProtection; Refac…

377c015

…tor Middleware; Extend example app

add testclass for middleware; fix bug where status set to 401 on succ…

4ccedef

…essful auth in middleware

add middleware and builder extension method for usage

80aef80

rename AutoProtectResource to UriBasedResourceProtection; add minimal…

dcc0749

… working example for URI based keycloak authorization

introduce DisableAutoProtection and ExplicitResourceProtection; Refac…

b47b424

…tor Middleware; Extend example app

add testclass for middleware; fix bug where status set to 401 on succ…

eb80664

…essful auth in middleware

Merge branch 'feature/auto-protect-resource-middleware' of https://gi…

ab7fb03

…thub.com/wanderzirkus/keycloak-authorization-services-dotnet into feature/auto-protect-resource-middleware

NikiforovAll requested changes May 6, 2024

View reviewed changes

NikiforovAll mentioned this pull request May 6, 2024

Idea: Use IEndpointConventionBuilder to apply protected resource access via Middleware #87

Closed

Friedemann Braune added 13 commits May 11, 2024 09:10

merged DisableUriBasedResourceProtection attribute into ExplicitResou…

1827e38

…rceProtectionAttribute; adapted tests; adapted samples; disable to rules by test specific .editorconfig

add middleware and builder extension method for usage

f36946c

rename AutoProtectResource to UriBasedResourceProtection; add minimal…

7651a02

… working example for URI based keycloak authorization

introduce DisableAutoProtection and ExplicitResourceProtection; Refac…

610e876

…tor Middleware; Extend example app

add testclass for middleware; fix bug where status set to 401 on succ…

c6d60bc

…essful auth in middleware

add middleware and builder extension method for usage

048b029

rename AutoProtectResource to UriBasedResourceProtection; add minimal…

da72dd7

… working example for URI based keycloak authorization

introduce DisableAutoProtection and ExplicitResourceProtection; Refac…

facf0ba

…tor Middleware; Extend example app

merged DisableUriBasedResourceProtection attribute into ExplicitResou…

71e995e

…rceProtectionAttribute; adapted tests; adapted samples; disable to rules by test specific .editorconfig

Merge branch 'feature/auto-protect-resource-middleware' of https://gi…

8f4a29b

…thub.com/wanderzirkus/keycloak-authorization-services-dotnet into feature/auto-protect-resource-middleware

Merge branch 'feature/auto-protect-resource-middleware' of https://gi…

1bdb892

…thub.com/wanderzirkus/keycloak-authorization-services-dotnet into feature/auto-protect-resource-middleware

Merge remote-tracking branch 'upstream/main' into feature/auto-protec…

2ebf6d2

…t-resource-middleware

wanderzirkus requested a review from NikiforovAll May 11, 2024 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add middleware for automatic authorization based on request endpoint #55

Add middleware for automatic authorization based on request endpoint #55

wanderzirkus commented Jan 7, 2024

NikiforovAll commented Feb 29, 2024 •

edited

Loading

NikiforovAll commented Apr 24, 2024

wanderzirkus commented Apr 27, 2024

NikiforovAll commented Apr 28, 2024

NikiforovAll left a comment

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024 •

edited

Loading

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024

NikiforovAll May 6, 2024

wanderzirkus May 11, 2024

NikiforovAll commented May 6, 2024 •

edited

Loading

NikiforovAll commented May 6, 2024

wanderzirkus commented May 11, 2024

wanderzirkus commented May 11, 2024

NikiforovAll commented May 11, 2024

NikiforovAll commented May 11, 2024

Add middleware for automatic authorization based on request endpoint #55

Are you sure you want to change the base?

Add middleware for automatic authorization based on request endpoint #55

Conversation

wanderzirkus commented Jan 7, 2024

NikiforovAll commented Feb 29, 2024 • edited Loading

NikiforovAll commented Apr 24, 2024

wanderzirkus commented Apr 27, 2024

NikiforovAll commented Apr 28, 2024

NikiforovAll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanderzirkus May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikiforovAll commented May 6, 2024 • edited Loading

NikiforovAll commented May 6, 2024

wanderzirkus commented May 11, 2024

wanderzirkus commented May 11, 2024

NikiforovAll commented May 11, 2024

NikiforovAll commented May 11, 2024

NikiforovAll commented Feb 29, 2024 •

edited

Loading

wanderzirkus May 11, 2024 •

edited

Loading

NikiforovAll commented May 6, 2024 •

edited

Loading