feat: support domain hierarchical and GetNamedRoleManager #1040

tangyang9464 · 2022-06-25T09:13:15Z

Signed-off-by: tangyang9464 tangyang9464@163.com

casbin-bot · 2022-06-25T09:13:19Z

@closetool @sagilio please review

tangyang9464 · 2022-06-25T09:17:46Z

@shipovalovyuriy plz review. You can ues it like the example in the test code

func TestDomainHierarchical(t *testing.T) {
	e, _ := NewEnforcer("examples/rbac_with_domains_hierarchical_model.conf", "examples/rbac_with_domains_hierarchical_policy.csv")
	origin := e.GetNamedRoleManager("g").(*defaultrolemanager.RoleManager)
	dest := e.GetNamedRoleManager("g2").(*defaultrolemanager.RoleManager)
	origin.SetTenantManager(dest)

	testDomainEnforce(t, e, "alice", "dom2", "data", "read", true)
}

zsh1995 · 2022-06-30T13:08:20Z

Great job! Could you add SetNamedRoleManager method for semantic consistency?

tangyang9464 · 2022-07-03T11:48:16Z

Great job! Could you add SetNamedRoleManager method for semantic consistency?

done

hsluoyz · 2022-07-03T11:55:30Z

@zsh1995

Sefriol

Nice that people are finally taking this into consideration. I am not a golang expert, but I gave my two cents about this pull request.

Reference issues:
#718
#872
#1032

Sefriol · 2022-08-03T08:35:01Z

rbac/default-role-manager/role_manager.go

@@ -557,6 +574,17 @@ func (dm *DomainManager) getRoleManager(domain string, store bool) *RoleManagerI
 				return true
 			})
 		}
+		if dm.tenantManager != nil {


Even though I do not know much about golang, this feels little repetitive, don't you think?

I think some comments would also be warranted. These specific conditions should be explained, otherwise somebody would just scratch their head in the future when they try to figure out why these are done as they are.

did some refactoring

Sefriol · 2022-08-03T08:39:51Z

examples/rbac_with_domains_hierarchical_policy.csv

@@ -0,0 +1,6 @@
+p, admin, dom3, data, read


Also little more complexity to the domain is warranted. At least one extra user to test out that they do not have access to domain1 if they only have access to domain2

Added more tests

Sefriol · 2022-08-03T08:41:12Z

rbac_api_with_domains_test.go

@@ -267,3 +268,12 @@ func TestGetAllDomains(t *testing.T) {

 	testGetAllDomains(t, e, []string{"domain1", "domain2"})
 }
+


I think singular test case is not sufficient for such a complex feature. At least one which supports deep hierarchy should also be done. Like here:

#718 (comment)

More specifically: Multiple subjects, multiple objects, multiple domains

Test cases for accessing data in an upper domain from a lower domain (Failure)

Test cases for accessing data in a lower domain from a middle domain (Success)

Test cases for accessing data in a sibling domain from another sibling (Failure)

Test cases for accessing data in a sibling domain from parent (success)

Added more tests

I tried to implement this in casbin/node-casbin#380

I think it's not possible to define g2 in the format of child, parent. At least in nodeJS, it provided incorrect results, but maybe golang has a different RoleManager definition.

You can try to replicate the test case I did for nodeJS. I think it provides quite good coverage on all the possible use cases. Especially the 4 domain structure is essential.

I have tried to comment on why certain decisions were made in the pull request and a little bit about the thought behind the hierarchy model.

Sefriol · 2022-08-03T08:43:21Z

rbac_api_with_domains_test.go

+	dest := e.GetNamedRoleManager("g2").(*defaultrolemanager.RoleManager)
+	origin.SetTenantManager(dest)
+
+	testDomainEnforce(t, e, "alice", "dom2", "data", "read", true)


I think in terms of the nature of this test case, alice should be tested from domains 1, 2 and 3 since she has access to all of them.

Added more tests

Sefriol · 2022-08-03T09:08:21Z

rbac/default-role-manager/role_manager.go

@@ -528,6 +535,16 @@ func (dm *DomainManager) rangeAffectedRoleManagers(domain string, fn func(rm *Ro
 			return true
 		})
 	}
+	if dm.tenantManager != nil {


Same thing about repetition as in my review.

Sefriol · 2022-08-03T09:13:44Z

rbac/default-role-manager/role_manager.go

@@ -449,6 +451,11 @@ func NewDomainManager(maxHierarchyLevel int) *DomainManager {
 	return dm
 }

+func (dm *DomainManager) SetTenantManager(tm rbac.RoleManager) {


Needs documentation on how to use this. Though at the same time, shouldn't the hierarchy be enabled by default?

What is base definition for Named Grouping Policies? If you set up a grouping policy for domains shouldn't they be hierarchical by default? @hsluoyz

I don't quite understand what you are saying. This PR is to use the RoleManager to manage the hierarchy of the domain

I was thinking out loud and more broadly how hierarchy should be enabled for the DomainManager. Not something that is necessary to solve in this pull request.

Sefriol · 2022-08-03T09:19:03Z

rbac/default-role-manager/role_manager.go

@@ -438,6 +439,7 @@ type DomainManager struct {
 	domainMatchingFunc MatchingFunc
 	logger             log.Logger
 	matchingFuncCache  *util.SyncLRUCache
+	tenantManager      rbac.RoleManager // Used to manage hierarchical relationships between domains, see


, see what? I think the naming should also be OKed by @hsluoyz, so that it's clear for the purpose.

hierarchyManager could also be a name option. Or something similar. Tenant feels little bit generic and might cause confusion. What's the definition of tenant in casbin?

tangyang9464 · 2022-08-13T13:42:41Z

@Sefriol Some changes have been made in response to your suggestion, please review again.

Sefriol

I think we need somebody else to review this than just me 😀.

Sefriol · 2022-08-13T14:02:36Z

rbac_api_with_domains_test.go

@@ -267,3 +268,12 @@ func TestGetAllDomains(t *testing.T) {

 	testGetAllDomains(t, e, []string{"domain1", "domain2"})
 }
+


I tried to implement this in casbin/node-casbin#380

I think it's not possible to define g2 in the format of child, parent. At least in nodeJS, it provided incorrect results, but maybe golang has a different RoleManager definition.

You can try to replicate the test case I did for nodeJS. I think it provides quite good coverage on all the possible use cases. Especially the 4 domain structure is essential.

I have tried to comment on why certain decisions were made in the pull request and a little bit about the thought behind the hierarchy model.

Sefriol · 2022-08-13T14:06:02Z

rbac/default-role-manager/role_manager.go

@@ -449,6 +451,11 @@ func NewDomainManager(maxHierarchyLevel int) *DomainManager {
 	return dm
 }

+func (dm *DomainManager) SetTenantManager(tm rbac.RoleManager) {


I was thinking out loud and more broadly how hierarchy should be enabled for the DomainManager. Not something that is necessary to solve in this pull request.

rbac/default-role-manager/role_manager.go

Sefriol · 2022-08-13T14:19:22Z

rbac_api_with_domains_test.go

+	e, _ := NewEnforcer("examples/rbac_with_domains_hierarchical_model.conf", "examples/rbac_with_domains_hierarchical_policy.csv")
+	origin := e.GetNamedRoleManager("g").(*defaultrolemanager.RoleManager)
+	dest := e.GetNamedRoleManager("g2").(*defaultrolemanager.RoleManager)
+	origin.SetDomainHierarchyManager(dest)


Should we add a helper function, so that you can enable the hierarchy without digging up the RoleManager? Like EnableDomainHierarchy which would set DomainHierachy to g with g2 by default.

It might be good, but I don't have time to do it right now

tangyang9464 · 2022-08-13T15:10:43Z

@Sefriol

I think it's not possible to define g2 in the format of child, parent. At least in nodeJS, it provided incorrect results, but maybe golang has a different RoleManager definition.
You can try to replicate the test case I did for nodeJS. I think it provides quite good coverage on all the possible use cases. Especially the 4 domain structure is essential.
I have tried to comment on why certain decisions were made in the pull request and a little bit about the thought behind the hierarchy model.

Maybe you should use conf and csv files so that we can replicate each other's use cases

Sefriol · 2022-08-14T13:15:01Z

Maybe you should use conf and csv files so that we can replicate each other's use cases

@tangyang9464 Sure. I changed the implementation to use csv and conf files.

https://github.com/casbin/node-casbin/pull/380/files

tangyang9464 · 2022-08-14T14:02:50Z

Maybe you should use conf and csv files so that we can replicate each other's use cases

@tangyang9464 Sure. I changed the implementation to use csv and conf files.

https://github.com/casbin/node-casbin/pull/380/files

@zsh1995 I tested all your use cases and all passed. Maybe we don't need to change g2 to g2(parent, child).
You may need to note that the definition of g2 in the model cannot be reversed: g2(p.dom, r.dom) instead of g2(r.dom, p.dom)

tangyang9464 · 2022-08-14T14:24:42Z

@zsh1995 At the same time, I want to discuss the difference between m = g(r.sub, p.sub, r.dom) && r.dom==p.dom && r.obj == p.obj && regexMatch(r.act, p.act) and m = g(r.sub, p.sub, r.dom) && g2(p.dom, r.dom) && r.obj == p.obj && regexMatch(r.act, p.act). The former means that users can be automatically assigned roles in multiple domains according to the domain hierarchy, but defining what permissions role has in each domain is still required. The latter is a role that automatically assigns multiple domains to the user according to the domain hierarchy, and the permissions of the role will also be automatically assigned according to the domain hierarchy. Please pay attention to the similarities and differences of the results of your test cases expect(e.enforceSync('alice', 'domain1', 'data2', 'read')).toBe(true) under the two models

Sefriol · 2022-08-14T17:35:12Z

Yeah I am not sure what kind of exact solution we should be looking for. My intent with following policy lines:

p, admin, domain1, data1, (read|write)
p, admin, domain2, data2, (read|write)
p, admin, domain3, data3, (write)
p, admin, sibling2, sdata2, (read|write)

g, alice, admin, domain1
g, bob, admin, domain2
g2, domain1, domain2
g2, domain1, sibling2
g2, domain2, domain3

is to create domain structure like

domain1
├── domain2
│   └── domain3
└── sibling2

Which would result that Alice is an admin in all domains and Bob only in domains 2 and 3 with as few g definitions as possible.

Key things I am trying to achieve here:

Each domain can have their custom permission table and can define their roles as they want to.
Easy role addition / removal from an user

Policy testing should include that:

Bob has no rights to domain1, sibling2´, but does for domain2anddomain3`
Alice has access to all domains

Checks like expect(e.enforceSync('alice', 'domain1', 'data2', 'read')).toBe(true) do not matter too much. Personally just expect(e.enforceSync('alice', 'domain2, 'data2', 'read')).toBe(true) is good enough.

I guess more important factor in this case is performance. In a case of not knowing which domain object dataX belongs to, is it faster to check all domains or just use reversed g2 model instead? Sometimes we are interested whether subject has access to an object and not if subject has access to an object in specific domain.

I derived my implementation from a model used in our production, so using hierarchy would not require us to do any conversation work.

I'll add your model too to node-casbin test cases to see that it works as well. I might need to add some performance testing as well to see how much it takes time in each model.

Sefriol · 2022-08-16T07:38:50Z

For some reason these tests in node-casbin only work when HasLink inputs are implemented in reverse order compared to this (this.domainHierarchyManager.syncedHasLink(domain, domain2) vs dm.domainHierarchyManager.HasLink(domain2, domain)). But maybe I just cannot read the golang code correctly.

@tangyang9464 Can you send me the results of golang HasLink function per domain from your example? Like domain, domain2, hasLinkResult. I need to investigate whether the function works correctly.

Signed-off-by: tangyang9464 <tangyang9464@163.com>

Sefriol · 2022-09-04T10:28:48Z

@nodece Commented on this in the node-casbin pull request. casbin/node-casbin#380 (comment)

In general I am in favor of implementing this directly to the model definition.

tangyang9464 · 2022-09-04T13:23:20Z

@nodece Commented on this in the node-casbin pull request. casbin/node-casbin#380 (comment)

In general I am in favor of implementing this directly to the model definition.

@Sefriol Agree, modify later

Sefriol · 2022-10-25T11:45:01Z

@tangyang9464 Have you heard any updates on the issue? Is there currently a plan on how this could be implemented?

@hsluoyz Pinging you as well if there is anything to update :)

hsluoyz · 2022-10-25T13:30:31Z

@tangyang9464 @imp2002

sujit-baniya · 2022-12-04T08:47:35Z

@tangyang9464 on latest casbin code, following test cases are not working.

	testDomainEnforce(t, e, "alice", "domain1", "data2", "read", true)  // Failed
	testDomainEnforce(t, e, "alice", "domain1", "sdata2", "read", true) // Failed
	testDomainEnforce(t, e, "alice", "domain1", "data3", "write", true) // Failed
	testDomainEnforce(t, e, "alice", "domain2", "data3", "write", true) // Failed
	testDomainEnforce(t, e, "bob", "domain2", "data3", "write", true)   // Failed

Sefriol · 2022-12-04T19:40:05Z

@sujit-baniya Current code is unlikely to be accepted anyway since there was more preferred idea to support this directly in the model definition.

sujit-baniya · 2022-12-04T23:27:36Z

@Sefriol Can you please suggest preferred ways to implement via model definition?

nodece · 2022-12-05T05:38:31Z

@Sefriol Can you please suggest preferred ways to implement via model definition?

I suggest we should do so to avoid diversification.

Sefriol · 2022-12-05T14:47:55Z

@Sefriol Can you please suggest preferred ways to implement via model definition?

I think Casbin Team should decide what kind of approach we should be going into. They have a better idea than I do on what needs to be taken into account. I can be part of the implementation on the NodeJS side, but

Sefriol · 2022-12-27T13:34:21Z

@hsluoyz @nodece Any progress on the design front? This seems to be quite a popular feature request among users.

Sefriol · 2023-01-16T13:40:25Z

@nodece @tangyang9464 @hsluoyz Have you had a discussion about this? NodeJS version of this was kinda just merged with unsolved questions.

casbin-bot requested review from kilosonc and sagilio June 25, 2022 09:13

tangyang9464 force-pushed the domain branch from f08e87a to a94b75e Compare July 3, 2022 11:47

Sefriol suggested changes Aug 3, 2022

View reviewed changes

Sefriol reviewed Aug 3, 2022

View reviewed changes

Sefriol mentioned this pull request Aug 3, 2022

[Enhancement] Implement hierarchical domain support casbin/node-casbin#377

Closed

Sefriol mentioned this pull request Aug 10, 2022

feat(defaultrolemanager.ts): add hierarchical domain support for rbac casbin/node-casbin#380

Merged

4 tasks

tangyang9464 force-pushed the domain branch 6 times, most recently from c537f74 to de04510 Compare August 13, 2022 13:39

Sefriol suggested changes Aug 13, 2022

View reviewed changes

tangyang9464 mentioned this pull request Aug 16, 2022

[Question] Role Manager with Resource Role #1075

Closed

feat: support domain hierarchical and GetNamedRoleManager

7e3646e

Signed-off-by: tangyang9464 <tangyang9464@163.com>

tangyang9464 force-pushed the domain branch from de04510 to 7e3646e Compare September 4, 2022 07:37

sujit-baniya mentioned this pull request Dec 4, 2022

[Question] - Need some suggestion on role manager implementation. #1151

Open

hsluoyz force-pushed the master branch from 0c44d27 to c0c6033 Compare August 15, 2023 15:26

Sefriol mentioned this pull request Sep 19, 2023

[Question] Hierarchical domains with (hierarchical) roles #872

Open

		@@ -267,3 +268,12 @@ func TestGetAllDomains(t *testing.T) {

		testGetAllDomains(t, e, []string{"domain1", "domain2"})
		}

feat: support domain hierarchical and GetNamedRoleManager #1040

Are you sure you want to change the base?

feat: support domain hierarchical and GetNamedRoleManager #1040

Conversation

tangyang9464 commented Jun 25, 2022

casbin-bot commented Jun 25, 2022

tangyang9464 commented Jun 25, 2022 • edited by hsluoyz

zsh1995 commented Jun 30, 2022

tangyang9464 commented Jul 3, 2022

hsluoyz commented Jul 3, 2022

Sefriol left a 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

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

tangyang9464 commented Aug 13, 2022

Sefriol left a 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

tangyang9464 commented Aug 13, 2022

Sefriol commented Aug 14, 2022 • edited

tangyang9464 commented Aug 14, 2022 • edited

tangyang9464 commented Aug 14, 2022

Sefriol commented Aug 14, 2022

Sefriol commented Aug 16, 2022

Sefriol commented Sep 4, 2022

tangyang9464 commented Sep 4, 2022

Sefriol commented Oct 25, 2022

hsluoyz commented Oct 25, 2022

sujit-baniya commented Dec 4, 2022

Sefriol commented Dec 4, 2022

sujit-baniya commented Dec 4, 2022

nodece commented Dec 5, 2022

Sefriol commented Dec 5, 2022

Sefriol commented Dec 27, 2022

Sefriol commented Jan 16, 2023

tangyang9464 commented Jun 25, 2022 •

edited by hsluoyz

Sefriol commented Aug 14, 2022 •

edited

tangyang9464 commented Aug 14, 2022 •

edited