fix: keep domain-pattern role links that are still granted after DeleteLink

rbac/default-role-manager/role_manager.go

@@ -494,6 +494,18 @@ type DomainManager struct {
 	matchingFunc       rbac.MatchingFunc
 	domainMatchingFunc rbac.MatchingFunc
 	matchingFuncCache  *util.SyncLRUCache
+
+	// links holds the links as they were added, by (name1, name2), with the
+	// domains they were added in. The role manager of a domain also holds the
+	// links it inherits from matching domain patterns, so it cannot tell on its
+	// own whether a link is still granted after one of its sources is deleted.
+	links   map[roleLink]map[string]struct{}
+	linksMu sync.Mutex
+}
+
+type roleLink struct {
+	name1 string
+	name2 string
 }
 
 // NewDomainManager is the constructor for creating an instance of the
@@ -524,29 +536,74 @@ func (dm *DomainManager) AddDomainMatchingFunc(name string, fn rbac.MatchingFunc
 	dm.rebuild()
 }
 
-// clears the map of RoleManagers.
+// rebuilds the map of RoleManagers from the links as they were added.
 func (dm *DomainManager) rebuild() {
-	rmMap := dm.rmMap
-	_ = dm.Clear()
-	rmMap.Range(func(key, value interface{}) bool {
-		domain := key.(string)
-		rm := value.(*RoleManagerImpl)
+	type domainLink struct {
+		roleLink
+		domain string
+	}
 
-		rm.Range(func(name1, name2 string, _ ...string) bool {
-			_ = dm.AddLink(name1, name2, domain)
-			return true
-		})
-		return true
-	})
+	var links []domainLink
+	dm.linksMu.Lock()
+	for link, domains := range dm.links {
+		for domain := range domains {
+			links = append(links, domainLink{link, domain})
+		}
+	}
+	dm.linksMu.Unlock()
+
+	_ = dm.Clear()
+	for _, link := range links {
+		_ = dm.AddLink(link.name1, link.name2, link.domain)
+	}
 }
 
 // Clear clears all stored data and resets the role manager to the initial state.
 func (dm *DomainManager) Clear() error {
 	dm.rmMap = &sync.Map{}
 	dm.matchingFuncCache = util.NewSyncLRUCache(100)
+	dm.linksMu.Lock()
+	dm.links = map[roleLink]map[string]struct{}{}
+	dm.linksMu.Unlock()
 	return nil
 }
 
+func (dm *DomainManager) addLinkRecord(name1, name2, domain string) {
+	dm.linksMu.Lock()
+	defer dm.linksMu.Unlock()
+
+	link := roleLink{name1, name2}
+	if dm.links[link] == nil {
+		dm.links[link] = map[string]struct{}{}
+	}
+	dm.links[link][domain] = struct{}{}
+}
+
+func (dm *DomainManager) deleteLinkRecord(name1, name2, domain string) {
+	dm.linksMu.Lock()
+	defer dm.linksMu.Unlock()
+
+	link := roleLink{name1, name2}
+	delete(dm.links[link], domain)
+	if len(dm.links[link]) == 0 {
+		delete(dm.links, link)
+	}
+}
+
+// linkGranted reports whether a remaining link still grants name1 -> name2 in
+// domain, either added in domain itself or in a domain pattern matching it.
+func (dm *DomainManager) linkGranted(name1, name2, domain string) bool {
+	dm.linksMu.Lock()
+	defer dm.linksMu.Unlock()
+
+	for linkDomain := range dm.links[roleLink{name1, name2}] {
+		if dm.Match(domain, linkDomain) {
+			return true
+		}
+	}
+	return false
+}
+
 func (dm *DomainManager) getDomain(domains ...string) (domain string, err error) {
 	switch len(domains) {
 	case 0:
@@ -618,6 +675,8 @@ func (dm *DomainManager) AddLink(name1 string, name2 string, domains ...string)
 	if err != nil {
 		return err
 	}
+	dm.addLinkRecord(name1, name2, domain)
+
 	roleManager := dm.getRoleManager(domain, true) // create role manager if it does not exist
 	_ = roleManager.AddLink(name1, name2, domains...)
 
@@ -634,12 +693,25 @@ func (dm *DomainManager) DeleteLink(name1 string, name2 string, domains ...strin
 	if err != nil {
 		return err
 	}
+	dm.deleteLinkRecord(name1, name2, domain)
+
+	// A domain keeps the link while another link still grants it there: the
+	// same link added in a matching domain pattern, or, for the domains matched
+	// by a deleted pattern, the link added in that domain itself.
 	roleManager := dm.getRoleManager(domain, true) // create role manager if it does not exist
-	_ = roleManager.DeleteLink(name1, name2, domains...)
+	if !dm.linkGranted(name1, name2, domain) {
+		_ = roleManager.DeleteLink(name1, name2, domains...)
+	}
 
-	dm.rangeAffectedRoleManagers(domain, func(rm *RoleManagerImpl) {
-		_ = rm.DeleteLink(name1, name2, domains...)
-	})
+	if dm.domainMatchingFunc != nil {
+		dm.rmMap.Range(func(key, value interface{}) bool {
+			domain2 := key.(string)
+			if domain != domain2 && dm.Match(domain2, domain) && !dm.linkGranted(name1, name2, domain2) {
+				_ = value.(*RoleManagerImpl).DeleteLink(name1, name2, domains...)
+			}
+			return true
+		})
+	}
 	return nil
 }
 
@@ -735,6 +807,15 @@ func (dm *DomainManager) BuildRelationship(name1 string, name2 string, domain ..
 // DeleteDomain deletes the specified domain from DomainManager.
 func (dm *DomainManager) DeleteDomain(domain string) error {
 	dm.rmMap.Delete(domain)
+
+	dm.linksMu.Lock()
+	for link, domains := range dm.links {
+		delete(domains, domain)
+		if len(domains) == 0 {
+			delete(dm.links, link)
+		}
+	}
+	dm.linksMu.Unlock()
 	return nil
 }
 

rbac/default-role-manager/role_manager_test.go

@@ -343,6 +343,51 @@ func TestDomainMatchingFuncWithDifferentDomain(t *testing.T) {
 	testDomainRole(t, rm, "alice", "admin", "domain2", false)
 }
 
+func TestDomainPatternDeleteLinkKeepsLinksStillGranted(t *testing.T) {
+	rm := NewRoleManager(10)
+	rm.AddDomainMatchingFunc("keyMatch", util.KeyMatch)
+
+	// alice is admin in every domain and also in domain1 on its own.
+	_ = rm.AddLink("alice", "admin", "*")
+	_ = rm.AddLink("alice", "admin", "domain1")
+
+	// Deleting the domain1 link must not drop what "*" still grants there.
+	_ = rm.DeleteLink("alice", "admin", "domain1")
+	testDomainRole(t, rm, "alice", "admin", "domain1", true)
+	testDomainRole(t, rm, "alice", "admin", "domain2", true)
+
+	// bob is admin in domain2 on his own and also through "*".
+	_ = rm.AddLink("bob", "admin", "domain2")
+	_ = rm.AddLink("bob", "admin", "*")
+
+	// Deleting the "*" link must not drop bob's own domain2 link.
+	_ = rm.DeleteLink("bob", "admin", "*")
+	testDomainRole(t, rm, "bob", "admin", "domain2", true)
+	testDomainRole(t, rm, "bob", "admin", "domain1", false)
+
+	// Once no link grants it any more, the link is gone everywhere.
+	_ = rm.DeleteLink("alice", "admin", "*")
+	testDomainRole(t, rm, "alice", "admin", "domain1", false)
+	testDomainRole(t, rm, "alice", "admin", "domain2", false)
+	_ = rm.DeleteLink("bob", "admin", "domain2")
+	testDomainRole(t, rm, "bob", "admin", "domain2", false)
+}
+
+func TestDomainPatternRebuildUsesAddedLinks(t *testing.T) {
+	rm := NewRoleManager(10)
+	rm.AddDomainMatchingFunc("keyMatch", util.KeyMatch)
+
+	_ = rm.AddLink("carol", "admin", "domain1")
+	_ = rm.AddLink("alice", "admin", "*")
+
+	// Setting the matching function again rebuilds the role managers; the
+	// "*" link copied into domain1 must not become a domain1 link of its own.
+	rm.AddDomainMatchingFunc("keyMatch", util.KeyMatch)
+	_ = rm.DeleteLink("alice", "admin", "*")
+	testDomainRole(t, rm, "alice", "admin", "domain1", false)
+	testDomainRole(t, rm, "carol", "admin", "domain1", true)
+}
+
 func TestTemporaryRoles(t *testing.T) {
 	rm := NewRoleManager(10)
 	rm.AddMatchingFunc("regexMatch", util.RegexMatch)

rbac_api_with_domains_test.go

@@ -426,3 +426,27 @@ func TestGetRolesForUserInDomainWithConditionalFunctions(t *testing.T) {
 		}
 	})
 }
+
+func TestRemoveGroupingPolicyWithDomainPattern(t *testing.T) {
+	e, _ := NewEnforcer("examples/rbac_with_domain_pattern_model.conf", "examples/rbac_with_domain_pattern_policy.csv")
+	e.AddNamedDomainMatchingFunc("g", "KeyMatch", util.KeyMatch)
+
+	// alice is admin in every domain ("g, alice, admin, *"). Granting and then
+	// revoking domain1 on its own must leave the "*" grant in place.
+	_, _ = e.AddGroupingPolicy("alice", "admin", "domain1")
+	_, _ = e.RemoveGroupingPolicy("alice", "admin", "domain1")
+	testDomainEnforce(t, e, "alice", "domain1", "data1", "read", true)
+
+	// bob is admin in domain2 ("g, bob, admin, domain2"). Granting and then
+	// revoking "*" must leave his domain2 grant in place.
+	_, _ = e.AddGroupingPolicy("bob", "admin", "*")
+	_, _ = e.RemoveGroupingPolicy("bob", "admin", "*")
+	testDomainEnforce(t, e, "bob", "domain2", "data2", "read", true)
+	testDomainEnforce(t, e, "bob", "domain1", "data1", "read", false)
+
+	// A reload from the same policy gives the same answers.
+	_ = e.LoadPolicy()
+	testDomainEnforce(t, e, "alice", "domain1", "data1", "read", true)
+	testDomainEnforce(t, e, "bob", "domain2", "data2", "read", true)
+	testDomainEnforce(t, e, "bob", "domain1", "data1", "read", false)
+}