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

GetImplicitPermissionsForUser only takes into account subject grouping #174

Closed
lanmarti opened this issue Apr 13, 2021 · 15 comments · Fixed by #199
Closed

GetImplicitPermissionsForUser only takes into account subject grouping #174

lanmarti opened this issue Apr 13, 2021 · 15 comments · Fixed by #199

Comments

@lanmarti
Copy link

lanmarti commented Apr 13, 2021

Would it be feasible to extend this method so it takes into account any grouping, not just the subject grouping?
e.g. with the following model:

[request_definition]
r = sub, obj, act

[policy_definition]
p = sub, obj, act

[role_definition]
g = _, _
g2 = _, _
g3 = _, _

[policy_effect]
e = some(where (p.eft == allow))

[matchers]
m = g(r.sub, p.sub) && g2(r.obj, p.obj) && g3(r.act, p.act)

getImplicitPermissionsForUser will take into account g, but not g2 or g3.

@hsluoyz hsluoyz self-assigned this Apr 13, 2021
@hsluoyz
Copy link
Member

hsluoyz commented Apr 17, 2021

@shink

@shink
Copy link
Member

shink commented Apr 17, 2021

@shink

OK, I'll do it in the next few days.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 19, 2021

We should add an optional ptype parameter to the getImplicitPermissionsForUser() API.

If the arg is not provided, then default g, but user can specify it to g2, g3, etc.

@lanmarti
Copy link
Author

Would this be limited to one ptype at a time, or would we be able to specify multiple groupings at once? In my specific case, the latter would be preferred, but I have no clue if this is realistic.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 19, 2021

@lanmarti why you want to specify multiple groupings at once? Does it have special meaning?

@lanmarti
Copy link
Author

lanmarti commented Apr 19, 2021

We have a bit of a complex setup, but basically: a subject belongs to one or more teams (g), these teams are given a set of permissions on specific objects, these objects might themselves be supersets of other objects.
So given an action allowed by team 1 on objectGroup1 should allow team member A to perform that action on object X.

p, team1, objectGroup1, read
g, lanmarti, team1
g2, objectA, objectGroup1
g2, objectB, objectGroup1
lanmarti, objectA, read => true

Ideally, getImplicitPermissionsForUser(lanmarti) would return

	lanmarti, objectA, read
	lanmarti, objectB, read
	lanmarti, objectGroup1, read

In our current use case, we have the subject, but need to know on what objects this subject is allowed to perform a specific action (implicitly).
The simplest approach from our standpoint would be to get all implicit permissions and filter objects with the permission we are looking for.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 20, 2021

@lanmarti currently getImplicitPermissionsForUser() API only supports retrieving the implicit permissions from the subject grouping (g), it doesn't support object grouping (g2) at the same time.

We can choose to add that support but why don't you consider using the BatchEnforce() API here: https://casbin.org/docs/en/data-permissions ? It's more powerful as it utilizes all matcher powers.

@lanmarti
Copy link
Author

The reason we're looking at getImplicitPermissionsForUser() is because we don't have the objects readily at hand. We have a subject and a specific permission, and wish to know on what objects the user has that permission.

If getImplicitPermissionsForUser() would not support multiple groupings at a time, we see two different options:

  1. Our security provider iterates over all groupings and builds a complete set of objects itself.
  2. Each client relying on the permission manager builds the set of objects to check permissions for by obtaining references to the objects elsewhere, and can then rely on the traditional enforce methods.

Neither option is very appealing in our current setup. Therefore, my reason for opening this ticket was to see if it was possible to have a third option, i.e. getting permissions the subject has on any object, implicitly, and filtering out objects on which the user has the permission we are interested in.

This is probably a niche use case and therefore might not be worth the trouble, but as it stands now, getImplicitPermissionsForUser() only seems to support standard RBAC grouping and would fail for RBAC with resource roles or any other more complex model. I can understand if you feel extending getImplicitPermissionsForUser() in this way is out of scope for casbin, and if so, we will look at one of the aforementioned options in implementing our use case.

@hsluoyz
Copy link
Member

hsluoyz commented May 26, 2021

@shink plz port the PR: casbin/casbin#798 to Java.

@shink
Copy link
Member

shink commented May 26, 2021

@hsluoyz OK, I will do it right away.

shink added a commit to shink/jcasbin that referenced this issue Jun 9, 2021
shink added a commit to shink/jcasbin that referenced this issue Jun 10, 2021
shink added a commit to shink/jcasbin that referenced this issue Jun 11, 2021
@github-actions
Copy link

🎉 This issue has been resolved in version 1.10.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@doctormacky
Copy link

Could you please let me know on the Group with domain model issue here

p, admin, domain1, data1, read
g, alice, group1
g2, group1, admin, domain1

With this fix, It will be false with following testing:
testDomainEnforce(e, "alice", "domain1", "data1", "read", false);

Then, how we need to do so that we can let alice access the domain1 ?

@hsluoyz
Copy link
Member

hsluoyz commented Nov 5, 2021

@doctormacky is this the same as issue: #232 ?

@doctormacky
Copy link

doctormacky commented Nov 5, 2021

@doctormacky is this the same as issue: #232 ?

Thanks for you quick response, there are not the same issue. actually, the issue caused by the fix 4d91e84

Originally, the Unit test can pass, and the test case can not pass after above fix. and the developer updated the unit test directly... I do not think it's reasonable. please take a look that issue. thanks a lot.

@hsluoyz
Copy link
Member

hsluoyz commented Nov 5, 2021

@seriouszyx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants