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

KEYCLOAK-11300 : Creating CustomEnforcer functionality for spring adapters #6448

Conversation

sushil-singh-guavus
Copy link

KEYCLOAK-11300 : Creating CustomEnforcer functionality for spring adapters

https://issues.jboss.org/browse/KEYCLOAK-11300

@sushil-singh-guavus sushil-singh-guavus changed the title CustomEnforcer functionality KEYCLOAK-11300 : Creating CustomEnforcer functionality for spring adapters Nov 6, 2019
@pedroigor pedroigor self-requested a review November 6, 2019 15:52
@stianst stianst added Discuss missing/docs Documentation is missing missing/tests Tests are missing labels Nov 8, 2019
@stianst
Copy link
Contributor

stianst commented Nov 8, 2019

Hi, thanks for your PR. Can you please open a discussion around this on the dev ML?

@sushil-singh-guavus
Copy link
Author

sushil-singh-guavus commented Nov 8, 2019 via email

@stianst
Copy link
Contributor

stianst commented Nov 8, 2019

Where? It would be good to have the discussion openly on the dev ML.

@sushil-singh-guavus
Copy link
Author

sushil-singh-guavus commented Nov 8, 2019

Where? It would be good to have the discussion openly on the dev ML.

Actually it is going on mail
@stianst, i have re-initiated the discussion

@pedroigor
Copy link
Contributor

Where? It would be good to have the discussion openly on the dev ML.

@stianst the JIRA provides some background and a document with more details about what @sushil-singh-guavus is proposing.

I asked him to send the PR so that we could understand better the use-case and start discussing the changes.

Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sushil-singh-guavus, now that we have the code and based on the description you have in the doc (attached to the JIRA), the intent is much more clear.

Correct me if I'm wrong, please. The objective here can be summarized as follows:

  • As a user I want to programmatically check whether or not a request is authorized to access a protected resource, so that I can pass the resource identifier/name and/or the set of scopes

  • As a user I want to be able to programmatically manage resources (CRUD) so that I can manage the resources that should be protected by Keycloak

If my assumption is correct, I think we can solve that differently by leveraging the AuthorizationContext class, which is available from the KeycloakSecurityContext that is usually available in the request scope.

The AuthorizationContext already provides methods for checking permissions, but today, they rely on any permissions carried by the token and do not interact with the server if no permission (for a resource/scope) is available.

It seems to me that we can simplify this implementation while addressing the same requirements.

In fact, recently, we have implemented a policy enforcer for Quarkus`which provides a similar approach to check permissions programmatically. While it also does not interact with the server to query for permissions, the hook is there to adapt and enhance programmatic authorization.

Please, let me know your thoughts about what I said.

@sushil-singh-guavus
Copy link
Author

sushil-singh-guavus commented Nov 9, 2019

@sushil-singh-guavus, now that we have the code and based on the description you have in the doc (attached to the JIRA), the intent is much more clear.

Correct me if I'm wrong, please. The objective here can be summarized as follows:

  • As a user I want to programmatically check whether or not a request is authorized to access a protected resource, so that I can pass the resource identifier/name and/or the set of scopes
  • As a user I want to be able to programmatically manage resources (CRUD) so that I can manage the resources that should be protected by Keycloak

If my assumption is correct, I think we can solve that differently by leveraging the AuthorizationContext class, which is available from the KeycloakSecurityContext that is usually available in the request scope.

The AuthorizationContext already provides methods for checking permissions, but today, they rely on any permissions carried by the token and do not interact with the server if no permission (for a resource/scope) is available.

It seems to me that we can simplify this implementation while addressing the same requirements.

In fact, recently, we have implemented a policy enforcer for Quarkus`which provides a similar approach to check permissions programmatically. While it also does not interact with the server to query for permissions, the hook is there to adapt and enhance programmatic authorization.

Please, let me know your thoughts about what I said.

@pedroigor Based on my understanding , When it comes to AuthorizationContext , it has methods to check permission for resource and a scope. So even if we extend it's functionalities for eg-: [Resource , Set of Scopes] , it won't be able to provide UMA and other functionalities that the policy-enforcer provides as it directly work on tokens

In our case client may not know the permission to be able to access that resource and scope. So firstly it will just call with an access token and based on the permission ticket , it will bring out the RPT

We have seen most of the options and then decided that policy-enforcer functionalities is what we are looking for , but some what in a custom fashion

Following things are there in PolicyEnforcer flow and needed for us

  • UMA i.e permission ticket flow working for us

  • Caching of resources

  • Lazy Loading (on demand resource)

  • wildcard resource matching

So, that's why I created CustomEnforcer functionality which can be enabled using keycloak.json. And you just specify the map of <resource , Set programatically. and just call the custom enforcer function. It will authenticate , authorize and set the applicable response status.

Here I am using PolicyEnforcer caching , wildcardresource matching , Lazy loading and have customized UMA flow based on "Map<Resource, Set of scopes>"

So , I am afraid that extending functionality of AuthorizationContext won't work for us.

I hope things are clear from my side , and also i will request to correct me if I am wrong somewhere

@stianst
Copy link
Contributor

stianst commented Nov 11, 2019

Didn't see any email about this on the dev ML yet. Could you please open a thread there so others can participate if they want to?

@sushil-singh-guavus
Copy link
Author

sushil-singh-guavus commented Nov 11, 2019 via email

@stianst
Copy link
Contributor

stianst commented Nov 13, 2019

@sushil-singh-guavus check out https://www.keycloak.org/community for details on ML. @pedroigor started a thread on this there, so let's continue the discussion on the ML

@sushil-singh-guavus
Copy link
Author

@stianst stianst added this to the 10.0.0 milestone Mar 17, 2020
@stianst stianst modified the milestones: 10.0.0, 11.0.0 Apr 21, 2020
@stianst
Copy link
Contributor

stianst commented Jun 15, 2020

Was a discussion opened on the dev mailing list around this? If so what was the conclusion here? From my perspective this may seem a bit to complex and seems like something we may not want to consider. @pedroigor wdyt?

@stianst stianst added status/needs-discussion PR needs discussion on developer mailing list and removed Discuss labels Jun 16, 2020
@pedroigor
Copy link
Contributor

pedroigor commented Jun 16, 2020

@stianst We had some discussion, can't find now the thread. But we ended up with this PR.

@sushil-singh-guavus After looking at your quickstart, I think I see now what you really want. And I still think we can do something along the lines with what I mentioned before.

It seems that we could just change the AuthorizationContext so that you can call specific methods to directly invoke the server. It should require fewer changes.

Pretty much what you have here https://github.com/sushil-singh-guavus/keycloak/commits/keycloak-11300? I'm not sure whether or not the changes herein are really needed by those from your branch.

@pedroigor pedroigor added the area/authorization-services Indicates an issue on Authorization area label Jun 16, 2020
@sushil-singh-guavus
Copy link
Author

@pedroigor , I am fine with the changes suggested by you that i have modified a bit because some things were breaking in https://github.com/sushil-singh-guavus/keycloak/commits/keycloak-11300

@pedroigor
Copy link
Contributor

@sushil-singh-guavus I see, so I think we should just have this PR with the changes you have in https://github.com/sushil-singh-guavus/keycloak/commits/keycloak-11300 plus the additional method and related changes to the AuthorizationContext so that you can pass that map with permissions.

Makes sense ? It should be much simpler, I think.

@pedroigor pedroigor added kind/enhancement Categorizes a PR related to an enhancement kind/feature Categorizes a PR related to a new feature labels Jun 18, 2020
@pedroigor pedroigor removed the kind/feature Categorizes a PR related to a new feature label Jun 18, 2020
@sushil-singh-guavus
Copy link
Author

sushil-singh-guavus commented Jun 19, 2020

@pedroigor right !!

Hi pedro, Correct me If I am wrong but I think changes related to passing map as a parameter is there in https://github.com/sushil-singh-guavus/keycloak/commits/keycloak-11300

I have also demonstrated that in quickstart also how we can use that

Let me put it here also,

AuthorizationContext authzContext = keycloakSecurityContext.getAuthorizationContext();
AdapterAuthorizationContext clientContext = AdapterAuthorizationContext.class.cast(authzContext);
Map<String, Set> permissionMap = new HashMap();
Set scopes = new HashSet();
scopes.add("CREATE");
String namespace = piplineData.getNamespace();
String name = piplineData.getName();
Object resource = new Object();
permissionMap.put(namespace,scopes);
clientContext.authorize(permissionMap);

@pedroigor
Copy link
Contributor

Yeah, so we should just change this PR to have that instead of all the changes herein ...

@sushil-singh-guavus
Copy link
Author

Yeah, so we should just change this PR to have that instead of all the changes herein ...

This PR was created from https://github.com/sushil-singh-guavus/keycloak/tree/feature/custom-policy-enforcer
Should i create new PR with https://github.com/sushil-singh-guavus/keycloak/commits/keycloak-11300
or
Should i revert the changes in this feature/custom-policy-enforcer branch and commit changes from branch keycloak-11300 in to it

plzz suggest

@pedroigor
Copy link
Contributor

Changes from keycloak-11300. They are enough, right?

@sushil-singh-guavus
Copy link
Author

sushil-singh-guavus commented Jun 20, 2020

Changes from keycloak-11300. They are enough, right?

yes correct all changes are there in https://github.com/sushil-singh-guavus/keycloak/commits/keycloak-11300 ,

I just want to know what needs to be changed from my side in this PR

@abstractj abstractj requested a review from pedroigor July 2, 2020 19:32
@pedroigor
Copy link
Contributor

It has been a while and plans have changed. We are avoiding drastic changes to this area of Keycloak.

Sorry and thanks for your contribution.

@pedroigor pedroigor closed this Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/authorization-services Indicates an issue on Authorization area kind/enhancement Categorizes a PR related to an enhancement missing/docs Documentation is missing missing/tests Tests are missing status/needs-discussion PR needs discussion on developer mailing list
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants