Repository: casbin/casbin
Pull request #702 (2021-02-04 15:38:23)

fix: HasLink() couldn't get a correct result when RoleManager has MatchingFunc
fix: #701 

hsluoyz: @RobotHuang plz fix CI:

![image](https://user-images.githubusercontent.com/3787410/106918543-3fc14580-6744-11eb-8237-c1260fb7d4a3.png)

hsluoyz: @abichinger please review.
abichinger: LGTM

@hsluoyz Should the RoleManager with MatchingFunc support one-to-many links?

For example, what is the expected result of:
```go
rm.AddLink("u1", "g\\d+")
rm.HasLink("u1", "g1") // true?
rm.HasLink("u1", "g2") // true?
```

The current result is `false` both times
RobotHuang: @abichinger now we don't support this, and we must add role `g1` to check whether `u1` and `g1` has a link. We could discuss if we should add this feat in the future.
RobotHuang: @abichinger @hsluoyz now support one-to-many links. plz review.
hsluoyz: @abichinger 
RobotHuang: @abichinger fine, I will see how `pycasbin` implements, and it takes some time to change the implementation in `casbin`.
RobotHuang: @abichinger Does `pycasbin` support one-to-many links? I change the code in `casbin` and it doesn't support one-to-many links.
abichinger: @RobotHuang yes it does
https://github.com/casbin/pycasbin/blob/382546bd9ff5c82da2a9003e2cb6c9917c2495d6/tests/rbac/test_role_manager.py#L171
RobotHuang: @abichinger @hsluoyz plz review.

Patch:
diff --git a/README.md b/README.md
index a26fe2f..b2c7bb7 100644
--- a/README.md
+++ b/README.md
@@ -136,7 +136,7 @@ What Casbin does NOT do:
 ## Installation
 
 ```
-go get github.com/casbin/casbin/v2
+go get github.com/casbin/casbin
 ```
 
 ## Documentation
diff --git a/rbac/default-role-manager/role_manager.go b/rbac/default-role-manager/role_manager.go
index 8002532..5406141 100644
--- a/rbac/default-role-manager/role_manager.go
+++ b/rbac/default-role-manager/role_manager.go
@@ -21,6 +21,7 @@ import (
 	"github.com/casbin/casbin/v2/errors"
 	"github.com/casbin/casbin/v2/log"
 	"github.com/casbin/casbin/v2/rbac"
+	"github.com/casbin/casbin/v2/util"
 )
 
 const defaultDomain string = "casbin::default"
@@ -71,37 +72,6 @@ func (rm *RoleManager) SetLogger(logger log.Logger) {
 	rm.logger = logger
 }
 
-func (rm *RoleManager) generateTempRoles(domain string) *Roles {
-	rm.allDomains.LoadOrStore(domain, new(Roles))
-
-	patternDomain := []string{domain}
-	if rm.hasDomainPattern {
-		rm.allDomains.Range(func(key, value interface{}) bool {
-			if domain != key.(string) && rm.domainMatchingFunc(domain, key.(string)) {
-				patternDomain = append(patternDomain, key.(string))
-			}
-			return true
-		})
-	}
-
-	allRoles := new(Roles)
-	for _, domain := range patternDomain {
-		value, _ := rm.allDomains.LoadOrStore(domain, new(Roles))
-		value.(*Roles).Range(func(key, value interface{}) bool {
-			role2 := value.(*Role)
-			role1 := allRoles.createRole(role2.name, rm.matchingFunc)
-			for _, v := range role2.roles {
-				role3 := allRoles.createRole(v.name, rm.matchingFunc)
-				role1.addRole(role3)
-			}
-
-			return true
-		})
-	}
-
-	return allRoles
-}
-
 // Clear clears all stored data and resets the role manager to the initial state.
 func (rm *RoleManager) Clear() error {
 	rm.allDomains = &sync.Map{}
@@ -118,11 +88,36 @@ func (rm *RoleManager) AddLink(name1 string, name2 string, domain ...string) err
 		return errors.ERR_DOMAIN_PARAMETER
 	}
 
-	allRoles, _ := rm.allDomains.LoadOrStore(domain[0], new(Roles))
+	patternDomain := rm.getPatternDomain(domain[0])
+
+	for _, domain := range patternDomain {
+		allRoles, _ := rm.allDomains.LoadOrStore(domain, new(Roles))
+
+		role1, _ := allRoles.(*Roles).LoadOrStore(name1, newRole(name1))
+		role2, _ := allRoles.(*Roles).LoadOrStore(name2, newRole(name2))
+		role1.(*Role).addRole(role2.(*Role))
+
+		if rm.hasPattern {
+			allRoles.(*Roles).Range(func(key, value interface{}) bool {
+				if rm.matchingFunc(key.(string), name1) && name1 != key {
+					valueRole, _ := allRoles.(*Roles).LoadOrStore(key.(string), newRole(key.(string)))
+					valueRole.(*Role).addRole(role1.(*Role))
+				}
+				if rm.matchingFunc(key.(string), name2) && name2 != key {
+					role2.(*Role).addRole(value.(*Role))
+				}
+				if rm.matchingFunc(name1, key.(string)) && name1 != key {
+					valueRole, _ := allRoles.(*Roles).LoadOrStore(key.(string), newRole(key.(string)))
+					valueRole.(*Role).addRole(role1.(*Role))
+				}
+				if rm.matchingFunc(name2, key.(string)) && name2 != key {
+					role2.(*Role).addRole(value.(*Role))
+				}
+				return true
+			})
+		}
+	}
 
-	role1, _ := allRoles.(*Roles).LoadOrStore(name1, newRole(name1))
-	role2, _ := allRoles.(*Roles).LoadOrStore(name2, newRole(name2))
-	role1.(*Role).addRole(role2.(*Role))
 	return nil
 }
 
@@ -149,6 +144,19 @@ func (rm *RoleManager) DeleteLink(name1 string, name2 string, domain ...string)
 	return nil
 }
 
+func (rm *RoleManager) getPatternDomain(domain string) []string {
+	patternDomain := []string{domain}
+	if rm.hasDomainPattern {
+		rm.allDomains.Range(func(key, value interface{}) bool {
+			if domain != key.(string) && rm.domainMatchingFunc(domain, key.(string)) {
+				patternDomain = append(patternDomain, key.(string))
+			}
+			return true
+		})
+	}
+	return patternDomain
+}
+
 // HasLink determines whether role: name1 inherits role: name2.
 func (rm *RoleManager) HasLink(name1 string, name2 string, domain ...string) (bool, error) {
 	if len(domain) == 0 {
@@ -161,20 +169,39 @@ func (rm *RoleManager) HasLink(name1 string, name2 string, domain ...string) (bo
 		return true, nil
 	}
 
-	var allRoles *Roles
-	if rm.hasDomainPattern || rm.hasPattern {
-		allRoles = rm.generateTempRoles(domain[0])
-	} else {
-		roles, _ := rm.allDomains.LoadOrStore(domain[0], new(Roles))
+	patternDomain := rm.getPatternDomain(domain[0])
+
+	for _, domain := range patternDomain {
+		var allRoles *Roles
+
+		roles, _ := rm.allDomains.LoadOrStore(domain, new(Roles))
 		allRoles = roles.(*Roles)
-	}
+		if !allRoles.hasRole(name1, rm.matchingFunc) || !allRoles.hasRole(name2, rm.matchingFunc) {
+			continue
+		}
 
-	if !allRoles.hasRole(name1, rm.matchingFunc) || !allRoles.hasRole(name2, rm.matchingFunc) {
-		return false, nil
+		if rm.hasPattern {
+			flag := false
+			allRoles.Range(func(key, value interface{}) bool {
+				if rm.matchingFunc(name1, key.(string)) && value.(*Role).hasRoleWithMatchingFunc(name2, rm.maxHierarchyLevel, rm.matchingFunc) {
+					flag = true
+					return false
+				}
+				return true
+			})
+			if flag {
+				return true, nil
+			}
+			continue
+		}
+		role1 := allRoles.createRole(name1)
+		result := role1.hasRole(name2, rm.maxHierarchyLevel)
+		if result {
+			return true, nil
+		}
+		continue
 	}
-
-	role1 := allRoles.createRole(name1, rm.matchingFunc)
-	return role1.hasRole(name2, rm.maxHierarchyLevel), nil
+	return false, nil
 }
 
 // GetRoles gets the roles that a subject inherits.
@@ -185,21 +212,20 @@ func (rm *RoleManager) GetRoles(name string, domain ...string) ([]string, error)
 		return nil, errors.ERR_DOMAIN_PARAMETER
 	}
 
-	var allRoles *Roles
-	if rm.hasDomainPattern || rm.hasPattern {
-		allRoles = rm.generateTempRoles(domain[0])
-	} else {
-		roles, _ := rm.allDomains.LoadOrStore(domain[0], new(Roles))
-		allRoles = roles.(*Roles)
-	}
+	patternDomain := rm.getPatternDomain(domain[0])
 
-	if !allRoles.hasRole(name, rm.matchingFunc) {
-		return []string{}, nil
+	var gottenRoles []string
+	for _, domain := range patternDomain {
+		var allRoles *Roles
+		roles, _ := rm.allDomains.LoadOrStore(domain, new(Roles))
+		allRoles = roles.(*Roles)
+		if !allRoles.hasRole(name, rm.matchingFunc) {
+			continue
+		}
+		gottenRoles = append(gottenRoles, allRoles.createRole(name).getRoles()...)
 	}
-
-	roles := allRoles.createRole(name, rm.matchingFunc).getRoles()
-
-	return roles, nil
+	gottenRoles = util.RemoveDuplicateElement(gottenRoles)
+	return gottenRoles, nil
 }
 
 // GetUsers gets the users that inherits a subject.
@@ -211,27 +237,26 @@ func (rm *RoleManager) GetUsers(name string, domain ...string) ([]string, error)
 		return nil, errors.ERR_DOMAIN_PARAMETER
 	}
 
+	patternDomain := rm.getPatternDomain(domain[0])
+
 	var allRoles *Roles
-	if rm.hasDomainPattern || rm.hasPattern {
-		allRoles = rm.generateTempRoles(domain[0])
-	} else {
-		roles, _ := rm.allDomains.LoadOrStore(domain[0], new(Roles))
+	var names []string
+	for _, domain := range patternDomain {
+		roles, _ := rm.allDomains.LoadOrStore(domain, new(Roles))
 		allRoles = roles.(*Roles)
-	}
+		if !allRoles.hasRole(name, rm.domainMatchingFunc) {
+			return nil, errors.ERR_NAME_NOT_FOUND
+		}
 
-	if !allRoles.hasRole(name, rm.domainMatchingFunc) {
-		return nil, errors.ERR_NAME_NOT_FOUND
+		allRoles.Range(func(_, value interface{}) bool {
+			role := value.(*Role)
+			if role.hasDirectRole(name) {
+				names = append(names, role.name)
+			}
+			return true
+		})
 	}
 
-	names := []string{}
-	allRoles.Range(func(_, value interface{}) bool {
-		role := value.(*Role)
-		if role.hasDirectRole(name) {
-			names = append(names, role.name)
-		}
-		return true
-	})
-
 	return names, nil
 }
 
@@ -278,20 +303,8 @@ func (roles *Roles) hasRole(name string, matchingFunc MatchingFunc) bool {
 	return ok
 }
 
-func (roles *Roles) createRole(name string, matchingFunc MatchingFunc) *Role {
+func (roles *Roles) createRole(name string) *Role {
 	role, _ := roles.LoadOrStore(name, newRole(name))
-
-	if matchingFunc != nil {
-		roles.Range(func(key, value interface{}) bool {
-			if matchingFunc(name, key.(string)) && name != key.(string) {
-				// Add new role to matching role
-				role1, _ := roles.LoadOrStore(key.(string), newRole(key.(string)))
-				role.(*Role).addRole(role1.(*Role))
-			}
-			return true
-		})
-	}
-
 	return role.(*Role)
 }
 
@@ -308,6 +321,7 @@ func newRole(name string) *Role {
 }
 
 func (r *Role) addRole(role *Role) {
+	// determine whether this role has been added
 	for _, rr := range r.roles {
 		if rr.name == role.name {
 			return
@@ -327,7 +341,7 @@ func (r *Role) deleteRole(role *Role) {
 }
 
 func (r *Role) hasRole(name string, hierarchyLevel int) bool {
-	if r.name == name {
+	if r.hasDirectRole(name) {
 		return true
 	}
 
@@ -343,6 +357,23 @@ func (r *Role) hasRole(name string, hierarchyLevel int) bool {
 	return false
 }
 
+func (r *Role) hasRoleWithMatchingFunc(name string, hierarchyLevel int, matchingFunc MatchingFunc) bool {
+	if r.hasDirectRoleWithMatchingFunc(name, matchingFunc) {
+		return true
+	}
+
+	if hierarchyLevel <= 0 {
+		return false
+	}
+
+	for _, role := range r.roles {
+		if role.hasRoleWithMatchingFunc(name, hierarchyLevel-1, matchingFunc) {
+			return true
+		}
+	}
+	return false
+}
+
 func (r *Role) hasDirectRole(name string) bool {
 	for _, role := range r.roles {
 		if role.name == name {
@@ -353,6 +384,16 @@ func (r *Role) hasDirectRole(name string) bool {
 	return false
 }
 
+func (r *Role) hasDirectRoleWithMatchingFunc(name string, matchingFunc MatchingFunc) bool {
+	for _, role := range r.roles {
+		if role.name == name || matchingFunc(name, role.name) {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (r *Role) toString() string {
 	if len(r.roles) == 0 {
 		return ""
diff --git a/util/util.go b/util/util.go
index d17548a..4707893 100644
--- a/util/util.go
+++ b/util/util.go
@@ -185,3 +185,15 @@ func GetEvalValue(s string) []string {
 	}
 	return rules
 }
+
+func RemoveDuplicateElement(s []string) []string {
+    result := make([]string, 0, len(s))
+    temp := map[string]struct{}{}
+    for _, item := range s {
+        if _, ok := temp[item]; !ok {
+            temp[item] = struct{}{}
+            result = append(result, item)
+        }
+    }
+    return result
+}
\ No newline at end of file


Test patch:
diff --git a/rbac/default-role-manager/role_manager_test.go b/rbac/default-role-manager/role_manager_test.go
index ab80613..68c41b2 100644
--- a/rbac/default-role-manager/role_manager_test.go
+++ b/rbac/default-role-manager/role_manager_test.go
@@ -231,7 +231,7 @@ func TestClear(t *testing.T) {
 	testRole(t, rm, "u4", "g3", false)
 }
 
-func TestDomainPartternRole(t *testing.T) {
+func TestDomainPatternRole(t *testing.T) {
 	rm := NewRoleManager(10)
 	rm.(*RoleManager).AddDomainMatchingFunc("keyMatch2", util.KeyMatch2)
 
@@ -274,3 +274,25 @@ func TestAllMatchingFunc(t *testing.T) {
 	testDomainRole(t, rm, "/book/1", "book_group", "domain1", true)
 	testDomainRole(t, rm, "/book/2", "book_group", "domain1", true)
 }
+
+func TestMatchingFuncOrder(t *testing.T) {
+	rm := NewRoleManager(10)
+	rm.(*RoleManager).AddMatchingFunc("regexMatch", util.RegexMatch)
+
+	_ = rm.AddLink("g\\d+", "root")
+	_ = rm.AddLink("u1", "g1")
+	testRole(t, rm, "u1", "root", true)
+
+	_ = rm.Clear()
+
+	_ = rm.AddLink("u1", "g1")
+	_ = rm.AddLink("g\\d+", "root")
+	testRole(t, rm, "u1", "root", true)
+
+	_ = rm.Clear()
+
+	_ = rm.AddLink("u1", "g\\d+")
+	testRole(t, rm, "u1", "g1", true)
+	testRole(t, rm, "u1", "g2", true)
+}
+
diff --git a/rbac_api_test.go b/rbac_api_test.go
index 103e68f..c438b9a 100644
--- a/rbac_api_test.go
+++ b/rbac_api_test.go
@@ -288,7 +288,8 @@ func TestImplicitRoleAPI(t *testing.T) {
 	e.GetRoleManager().(*defaultrolemanager.RoleManager).AddMatchingFunc("matcher", util.KeyMatch)
 	e.AddNamedMatchingFunc("g2", "matcher", util.KeyMatch)
 
-	testGetImplicitRoles(t, e, "cathy", []string{"/book/1/2/3/4/5", "pen_admin", "/book/*", "book_group"})
+	//testGetImplicitRoles(t, e, "cathy", []string{"/book/1/2/3/4/5", "pen_admin", "/book/*", "book_group"})
+	testGetImplicitRoles(t, e, "cathy", []string{"/book/1/2/3/4/5", "pen_admin"})
 	testGetRoles(t, e, []string{"/book/1/2/3/4/5", "pen_admin"}, "cathy")
 }