From b528b8d9830d1208986d4fad914b714099856851 Mon Sep 17 00:00:00 2001 From: breken-ai <312387581+breken-ai@users.noreply.github.com> Date: Tue, 29 Sep 2026 19:47:46 -0700 Subject: [PATCH] fix: keep domain-pattern role links that are still granted after DeleteLink With a domain matching function, the role manager of a concrete domain also holds the links it inherits from matching patterns such as "*". DeleteLink removed the link from that role manager and from every domain matched by the deleted pattern without checking whether another link still granted it, so: - removing "g, alice, admin, domain1" also dropped the grant that "g, alice, admin, *" still gives in domain1, and - removing "g, bob, admin, *" also dropped bob's own "g, bob, admin, domain2". Enforce then denied these requests until the policy was reloaded. DomainManager now records the links as they were added and only deletes a link from a domain when no remaining link grants it there. rebuild() replays these records instead of the per-domain role managers, which also hold the copied pattern links. Generated-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 --- rbac/default-role-manager/role_manager.go | 113 +++++++++++++++--- .../default-role-manager/role_manager_test.go | 45 +++++++ rbac_api_with_domains_test.go | 24 ++++ 3 files changed, 166 insertions(+), 16 deletions(-) diff --git a/rbac/default-role-manager/role_manager.go b/rbac/default-role-manager/role_manager.go index 2e35d571c..44ce7ce88 100644 --- a/rbac/default-role-manager/role_manager.go +++ b/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 } diff --git a/rbac/default-role-manager/role_manager_test.go b/rbac/default-role-manager/role_manager_test.go index bd5e6e092..36477e3b5 100644 --- a/rbac/default-role-manager/role_manager_test.go +++ b/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) diff --git a/rbac_api_with_domains_test.go b/rbac_api_with_domains_test.go index 1437b28dd..c913b589f 100644 --- a/rbac_api_with_domains_test.go +++ b/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) +}