fix: align RoleManager behavior with documentation (#1404)

Author: bugoutianzhen123

rbac/default-role-manager/role_manager.go

@@ -35,6 +35,7 @@ type Role struct {
 	matchedBy                  *sync.Map
 	linkConditionFuncMap       *sync.Map
 	linkConditionFuncParamsMap *sync.Map
+	level                      int
 }
 
 func newRole(name string) *Role {
@@ -46,17 +47,41 @@ func newRole(name string) *Role {
 	r.matchedBy = &sync.Map{}
 	r.linkConditionFuncMap = &sync.Map{}
 	r.linkConditionFuncParamsMap = &sync.Map{}
+	r.level = -1
 	return &r
 }
 
+func (r *Role) updateLevel() {
+	maxLevel := 0
+	r.users.Range(func(_, value interface{}) bool {
+		if parentRole := value.(*Role); parentRole.level >= 0 && parentRole.level+1 > maxLevel {
+			maxLevel = parentRole.level + 1
+		}
+		return true
+	})
+	if r.level == maxLevel {
+		return
+	}
+	r.level = maxLevel
+	r.roles.Range(func(_, value interface{}) bool {
+		value.(*Role).updateLevel()
+		return true
+	})
+	return
+}
+
 func (r *Role) addRole(role *Role) {
 	r.roles.Store(role.name, role)
 	role.addUser(r)
+	r.updateLevel()
+	role.updateLevel()
 }
 
 func (r *Role) removeRole(role *Role) {
 	r.roles.Delete(role.name)
 	role.removeUser(r)
+	r.updateLevel()
+	role.updateLevel()
 }
 
 // should only be called inside addRole.
@@ -228,7 +253,9 @@ func (rm *RoleManagerImpl) rebuild() {
 	roles := rm.allRoles
 	_ = rm.Clear()
 	rangeLinks(roles, func(name1, name2 string, domain ...string) bool {
-		_ = rm.AddLink(name1, name2, domain...)
+		if err := rm.AddLink(name1, name2, domain...); err != nil {
+			rm.logger.LogError(err)
+		}
 		return true
 	})
 }
@@ -329,9 +356,21 @@ func (rm *RoleManagerImpl) Clear() error {
 // AddLink adds the inheritance link between role: name1 and role: name2.
 // aka role: name1 inherits role: name2.
 func (rm *RoleManagerImpl) AddLink(name1 string, name2 string, domains ...string) error {
+	if hasLink, err := rm.HasLink(name2, name1, domains...); err != nil {
+		return err
+	} else if hasLink {
+		return fmt.Errorf("cannot add link from '%s' to '%s': would create a cycle", name1, name2)
+	}
+
 	user, _ := rm.getRole(name1)
 	role, _ := rm.getRole(name2)
 	user.addRole(role)
+
+	if rm.maxHierarchyLevel > 0 && user.level > rm.maxHierarchyLevel-1 {
+		user.removeRole(role)
+		return fmt.Errorf("cannot add link from '%s' to '%s': role '%s' is at level %d, exceeding the maximum allowed hierarchy level of %d",
+			name1, name2, name2, role.level, rm.maxHierarchyLevel)
+	}
 	return nil
 }
 
@@ -392,7 +431,8 @@ func (rm *RoleManagerImpl) GetRoles(name string, domains ...string) ([]string, e
 	if created {
 		defer rm.removeRole(user.name)
 	}
-	return user.getRoles(), nil
+
+	return rm.filterHelper(user.getRoles()), nil
 }
 
 // GetUsers gets the users of a role.
@@ -402,7 +442,37 @@ func (rm *RoleManagerImpl) GetUsers(name string, domain ...string) ([]string, er
 	if created {
 		defer rm.removeRole(role.name)
 	}
-	return role.getUsers(), nil
+
+	return rm.filterHelper(role.getUsers()), nil
+}
+
+func (rm *RoleManagerImpl) filterHelper(names []string) []string {
+	if rm.maxHierarchyLevel <= 0 {
+		return names
+	}
+
+	filteredNames := make([]string, 0, len(names))
+	var toRemove []string
+
+	for _, name := range names {
+		role, created := rm.getRole(name)
+		if created {
+			toRemove = append(toRemove, role.name)
+		}
+		if role.level <= rm.maxHierarchyLevel {
+			filteredNames = append(filteredNames, name)
+		}
+	}
+
+	if toRemove == nil {
+		return filteredNames
+	}
+
+	for _, removeName := range toRemove {
+		rm.removeRole(removeName)
+	}
+
+	return filteredNames
 }
 
 func (rm *RoleManagerImpl) toString() []string {
@@ -443,7 +513,9 @@ func (rm *RoleManagerImpl) GetAllDomains() ([]string, error) {
 
 func (rm *RoleManagerImpl) copyFrom(other *RoleManagerImpl) {
 	other.Range(func(name1, name2 string, domain ...string) bool {
-		_ = rm.AddLink(name1, name2, domain...)
+		if err := rm.AddLink(name1, name2, domain...); err != nil {
+			rm.logger.LogError(err)
+		}
 		return true
 	})
 }
@@ -605,12 +677,16 @@ func (dm *DomainManager) AddLink(name1 string, name2 string, domains ...string)
 		return err
 	}
 	roleManager := dm.getRoleManager(domain, true) // create role manager if it does not exist
-	_ = roleManager.AddLink(name1, name2, domains...)
+	if err := roleManager.AddLink(name1, name2, domains...); err != nil {
+		return err
+	}
 
 	dm.rangeAffectedRoleManagers(domain, func(rm *RoleManagerImpl) {
-		_ = rm.AddLink(name1, name2, domains...)
+		if err == nil {
+			err = rm.AddLink(name1, name2, domains...)
+		}
 	})
-	return nil
+	return err
 }
 
 // DeleteLink deletes the inheritance link between role: name1 and role: name2.
@@ -733,7 +809,9 @@ type ConditionalRoleManager struct {
 
 func (crm *ConditionalRoleManager) copyFrom(other *ConditionalRoleManager) {
 	other.Range(func(name1, name2 string, domain ...string) bool {
-		_ = crm.AddLink(name1, name2, domain...)
+		if err := crm.AddLink(name1, name2, domain...); err != nil {
+			crm.logger.LogError(err)
+		}
 		return true
 	})
 }
@@ -966,12 +1044,16 @@ func (cdm *ConditionalDomainManager) AddLink(name1 string, name2 string, domains
 		return err
 	}
 	conditionalRoleManager := cdm.getConditionalRoleManager(domain, true) // create role manager if it does not exist
-	_ = conditionalRoleManager.AddLink(name1, name2, domain)
+	if err := conditionalRoleManager.AddLink(name1, name2, domain); err != nil {
+		return err
+	}
 
 	cdm.rangeAffectedRoleManagers(domain, func(rm *RoleManagerImpl) {
-		_ = rm.AddLink(name1, name2, domain)
+		if err == nil {
+			err = rm.AddLink(name1, name2, domain)
+		}
 	})
-	return nil
+	return err
 }
 
 // DeleteLink deletes the inheritance link between role: name1 and role: name2.

rbac/default-role-manager/role_manager_test.go

@@ -364,29 +364,47 @@ func TestTemporaryRoles(t *testing.T) {
 }
 
 func TestMaxHierarchyLevel(t *testing.T) {
-	rm := NewRoleManager(1)
-	_ = rm.AddLink("level0", "level1")
-	_ = rm.AddLink("level1", "level2")
-	_ = rm.AddLink("level2", "level3")
-
-	testRole(t, rm, "level0", "level0", true)
-	testRole(t, rm, "level0", "level1", true)
-	testRole(t, rm, "level0", "level2", false)
-	testRole(t, rm, "level0", "level3", false)
-	testRole(t, rm, "level1", "level2", true)
-	testRole(t, rm, "level1", "level3", false)
-
-	rm = NewRoleManager(2)
-	_ = rm.AddLink("level0", "level1")
-	_ = rm.AddLink("level1", "level2")
-	_ = rm.AddLink("level2", "level3")
-
-	testRole(t, rm, "level0", "level0", true)
-	testRole(t, rm, "level0", "level1", true)
-	testRole(t, rm, "level0", "level2", true)
-	testRole(t, rm, "level0", "level3", false)
-	testRole(t, rm, "level1", "level2", true)
-	testRole(t, rm, "level1", "level3", true)
+	rm1 := NewRoleManager(1)
+	_ = rm1.AddLink("level0", "level1")
+	_ = rm1.AddLink("level1", "level2")
+	_ = rm1.AddLink("level2", "level3")
+
+	testRole(t, rm1, "level0", "level0", true)
+	testRole(t, rm1, "level0", "level1", true)
+	testRole(t, rm1, "level0", "level2", false)
+	testRole(t, rm1, "level0", "level3", false)
+	testRole(t, rm1, "level1", "level2", false)
+	testRole(t, rm1, "level1", "level3", false)
+
+	rm2 := NewRoleManager(2)
+	_ = rm2.AddLink("level0", "level1")
+	_ = rm2.AddLink("level1", "level2")
+	_ = rm2.AddLink("level2", "level3")
+
+	testRole(t, rm2, "level0", "level0", true)
+	testRole(t, rm2, "level0", "level1", true)
+	testRole(t, rm2, "level0", "level2", true)
+	testRole(t, rm2, "level0", "level3", false)
+	testRole(t, rm2, "level1", "level2", true)
+	testRole(t, rm2, "level1", "level3", false)
+	testRole(t, rm2, "level2", "level3", false)
+
+	rm3 := NewRoleManager(3)
+	_ = rm3.AddLink("level0", "level1")
+	_ = rm3.AddLink("level1", "level2")
+	_ = rm3.AddLink("level2", "level3")
+	_ = rm3.AddLink("level3", "level4")
+	_ = rm3.AddLink("level4", "level5")
+
+	testRole(t, rm3, "level0", "level0", true)
+	testRole(t, rm3, "level0", "level1", true)
+	testRole(t, rm3, "level0", "level2", true)
+	testRole(t, rm3, "level0", "level3", true)
+	testRole(t, rm3, "level1", "level2", true)
+	testRole(t, rm3, "level1", "level3", true)
+	testRole(t, rm3, "level2", "level3", true)
+	testRole(t, rm3, "level2", "level4", false)
+	testRole(t, rm3, "level2", "level5", false)
 }
 
 // TestConcurrentHasLink tests concurrent HasLink calls for race conditions.
@@ -431,3 +449,71 @@ func TestConcurrentHasLink(t *testing.T) {
 			inconsistencies, numGoroutines*numIterations)
 	}
 }
+
+func TestAddLink_Cycle(t *testing.T) {
+	rm := NewRoleManager(10)
+	_ = rm.AddLink("a", "b")
+	_ = rm.AddLink("b", "c")
+	err := rm.AddLink("c", "a")
+
+	if err == nil {
+		t.Errorf("Expected an error when creating a cycle, but got nil")
+	} else {
+		t.Logf("Successfully detected cycle: %s", err)
+	}
+}
+
+func TestHierarchyLimitAndGetRoles(t *testing.T) {
+	rm := NewRoleManager(3)
+
+	_ = rm.AddLink("g0", "g1")
+	_ = rm.AddLink("g2", "g3")
+	_ = rm.AddLink("g1", "g2")
+	_ = rm.AddLink("g3", "g4")
+
+	expectedLevels := map[string]int{
+		"g0": 0,
+		"g1": 1,
+		"g2": 2,
+		"g3": 3,
+		"g4": 0,
+	}
+
+	rm.rmMap.Range(func(key, value interface{}) bool {
+		domain := key.(string)
+		rm := value.(*RoleManagerImpl)
+		fmt.Printf("Domain: %s\n", domain)
+		rm.allRoles.Range(func(k, v interface{}) bool {
+			role := v.(*Role)
+			if expectedLevels[role.name] != role.level {
+				t.Errorf("Role %s level mismatch: got %d, want %d", role.name, role.level, expectedLevels[role.name])
+			}
+			//fmt.Printf("  Role name: %s, Level: %d\n", role.name, role.level)
+			return true
+		})
+		return true
+	})
+
+	_ = rm.DeleteLink("g1", "g2")
+
+	expectedLevelsAfterDelete := map[string]int{
+		"g0": 0,
+		"g1": 1,
+		"g2": 0,
+		"g3": 1,
+	}
+
+	rm.rmMap.Range(func(key, value interface{}) bool {
+		domain := key.(string)
+		rm := value.(*RoleManagerImpl)
+		fmt.Printf("After delete, Domain: %s\n", domain)
+		rm.allRoles.Range(func(k, v interface{}) bool {
+			role := v.(*Role)
+			if expectedLevelsAfterDelete[role.name] != role.level {
+				t.Errorf("[After delete] Role %s level mismatch: got %d, want %d", role.name, role.level, expectedLevelsAfterDelete[role.name])
+			}
+			return true
+		})
+		return true
+	})
+}