Skip to content

fix(acl): preserve user permissions across namespaces #9812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gooohgb wants to merge 2 commits into
base: main
Choose a base branch
from
+133 −12
Open

fix(acl): preserve user permissions across namespaces #9812

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a41196
fix(acl): preserve user permissions across namespaces
gooohgb Aug 20, 2026
619ebab
fix(acl): snapshot cached user permissions
gooohgb Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions worker/acl_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,17 @@ func (cache *AclCache) Update(ns uint64, groups []acl.Group) {
AclCachePtr.predPerms[k] = v
}

for k, v := range userPredPerms {
AclCachePtr.userPredPerms[k] = v
// User IDs are namespace-local, so the same ID can exist in multiple namespaces. Merge the
// namespaced predicates instead of replacing permissions collected from another namespace.
for userID, newPerms := range userPredPerms {
perms, found := AclCachePtr.userPredPerms[userID]

@matthewmcneely matthewmcneely Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking design note, since what you have is correct.

The collision is possible at all because userPredPerms is keyed by bare user ID while every value key is namespace-qualified. Keying the outer map by x.NamespaceAttr(ns, userID) would remove it at the source, and take two costs with it that this change slightly worsens:

  • The clear loop (L138-L144) walks every user's full predicate map on each refresh, and RefreshACLs calls Update once per namespace, so a full refresh is O(namespaces x total entries).
  • authorizePreds now iterates a user's predicates across all namespaces to build allowedPreds, and expand(_all_) builds a hash map over that slice on every query. For a user ID present in many namespaces (common in multi-tenant setups) that grows linearly with namespace count.

authorizePreds already has ns := userData.namespace in hand, so GetUserPredPerms(ns, userId) should be a fairly contained change. Happy to take it as a follow-up if you'd rather keep this PR tight.

if !found {
perms = make(map[string]int32)
AclCachePtr.userPredPerms[userID] = perms
}
for predicate, permission := range newPerms {
perms[predicate] = permission

@matthewmcneely matthewmcneely Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge itself is correct, and this is pre-existing rather than something you introduced. But the new write loop widens it, so it's worth knowing about.

GetUserPredPerms (line 53) returns the live inner map and then drops the lock. edgraph.authorizePreds then ranges it twice with no lock held (edgraph/access.go L650-L655), while the SubscribeForAclUpdates goroutine calls Update and mutates those same inner maps. I ran a probe under -race against both commits:

  • base: mapdelete (the clear loop above) vs mapIterStart
  • this branch: mapassign_faststr (this line) vs mapIterStart

Delete-vs-iterate was already there. Insert-vs-iterate is the nastier form, because a grow/rehash mid-iteration is what trips Go's unrecoverable concurrent map read and map write throw, which takes the Alpha down. This change also makes the maps genuinely larger (the bug was collapsing each user down to a single namespace), so the iteration window widens along with it.

Cheap to close while you're in here:

func (cache *AclCache) GetUserPredPerms(userId string) map[string]int32 {
	cache.RLock()
	defer cache.RUnlock()
	perms := make(map[string]int32, len(cache.userPredPerms[userId]))
	for pred, perm := range cache.userPredPerms[userId] {
		perms[pred] = perm
	}
	return perms
}

While you're there, authorizePreds calls GetUserPredPerms twice per query, once for the len() in the make and again for the range. One call would do.

}
}

@matthewmcneely matthewmcneely Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low priority, and pre-existing: once a user's last permission in their only namespace is cleared, the entry sticks around forever with an empty inner map. The merge branch doesn't prune it either. One line at the end of the loop:

if len(perms) == 0 {
	delete(AclCachePtr.userPredPerms, userID)
}

Callers behave the same either way, since allowedPreds ends up []string{} whether the lookup returns nil or an empty map.

}

Expand Down
70 changes: 68 additions & 2 deletions worker/acl_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,20 @@ import (
"github.com/dgraph-io/dgraph/v25/x"
)

func TestAclCache(t *testing.T) {
func resetAclCacheForTest(t *testing.T) {
t.Helper()
original := AclCachePtr
AclCachePtr = &AclCache{
predPerms: make(map[string]map[string]int32),
predPerms: make(map[string]map[string]int32),
userPredPerms: make(map[string]map[string]int32),
}
t.Cleanup(func() {
AclCachePtr = original
})
}

func TestAclCache(t *testing.T) {
resetAclCacheForTest(t)

var emptyGroups []string
group := "dev"
Expand Down Expand Up @@ -51,3 +61,59 @@ func TestAclCache(t *testing.T) {
require.Error(t, AclCachePtr.AuthorizePredicate(emptyGroups, predicate, acl.Read),
"the anonymous user should not have access when the acl cache is empty")
}

func TestAclCacheMergesSameUserAcrossNamespaces(t *testing.T) {
const (
userID = "shared-user"
nsOne = uint64(1)
nsTwo = uint64(2)
)

groups := func(groupID, predicate string, permission int32) []acl.Group {
return []acl.Group{{
GroupID: groupID,
Users: []acl.User{{UserID: userID}},
Rules: []acl.Acl{{Predicate: predicate, Perm: permission}},
}}
}

for _, tc := range []struct {
name string
order []uint64
}{
{name: "namespace one then two", order: []uint64{nsOne, nsTwo}},
{name: "namespace two then one", order: []uint64{nsTwo, nsOne}},
} {
t.Run(tc.name, func(t *testing.T) {
resetAclCacheForTest(t)

for _, ns := range tc.order {
switch ns {
case nsOne:
AclCachePtr.Update(nsOne, groups("group-one", "pred-one", acl.Read.Code))
case nsTwo:
AclCachePtr.Update(nsTwo, groups("group-two", "pred-two", acl.Write.Code))
}
}

require.Equal(t, map[string]int32{
x.NamespaceAttr(nsOne, "pred-one"): acl.Read.Code,
x.NamespaceAttr(nsTwo, "pred-two"): acl.Write.Code,
}, AclCachePtr.GetUserPredPerms(userID))

AclCachePtr.Update(nsOne,
groups("group-one", "pred-one-new", acl.Modify.Code))
require.Equal(t, map[string]int32{
x.NamespaceAttr(nsOne, "pred-one-new"): acl.Modify.Code,
x.NamespaceAttr(nsTwo, "pred-two"): acl.Write.Code,
}, AclCachePtr.GetUserPredPerms(userID),
"refreshing one namespace should replace only that namespace's permissions")

AclCachePtr.Update(nsOne, nil)
require.Equal(t, map[string]int32{
x.NamespaceAttr(nsTwo, "pred-two"): acl.Write.Code,
}, AclCachePtr.GetUserPredPerms(userID),
"clearing one namespace should preserve permissions from other namespaces")
})
}
}
Loading