fix(acl): preserve user permissions across namespaces

edgraph/predicate_acl.go

@@ -303,9 +303,10 @@ func authorizePreds(ctx context.Context, userData *userData, preds []string,
 		return &PredResult{Allowed: nil, Blocked: blockedPreds}
 	}
 	// User can have multiple permission for same predicate, add predicate
-	allowedPreds := make([]string, 0, len(worker.AclCachePtr.GetUserPredPerms(userId)))
+	userPredPerms := worker.AclCachePtr.GetUserPredPerms(userId)
+	allowedPreds := make([]string, 0, len(userPredPerms))
 	// only if the acl.Op is covered in the set of permissions for the user
-	for predicate, perm := range worker.AclCachePtr.GetUserPredPerms(userId) {
+	for predicate, perm := range userPredPerms {
 		if (perm & aclOp.Code) > 0 {
 			allowedPreds = append(allowedPreds, predicate)
 		}

worker/acl_cache.go

@@ -51,9 +51,18 @@ var AclCachePtr = &AclCache{
 }
 
 func (cache *AclCache) GetUserPredPerms(userId string) map[string]int32 {
-	cache.Lock()
-	defer cache.Unlock()
-	return cache.userPredPerms[userId]
+	cache.RLock()
+	defer cache.RUnlock()
+
+	perms, found := cache.userPredPerms[userId]
+	if !found {
+		return nil
+	}
+	snapshot := make(map[string]int32, len(perms))
+	for predicate, permission := range perms {
+		snapshot[predicate] = permission
+	}
+	return snapshot
 }
 
 func (cache *AclCache) Update(ns uint64, groups []acl.Group) {
@@ -135,21 +144,33 @@ func (cache *AclCache) Update(ns uint64, groups []acl.Group) {
 		}
 	}
 
-	for _, v := range AclCachePtr.userPredPerms {
-		for k := range v {
+	for userID, perms := range AclCachePtr.userPredPerms {
+		for k := range perms {
 			if x.ParseNamespace(k) == ns {
-				delete(v, k)
+				delete(perms, k)
 			}
 		}
+		if len(perms) == 0 {
+			delete(AclCachePtr.userPredPerms, userID)
+		}
 	}
 
 	// Set new rules in the cache
 	for k, v := range predPerms {
 		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]
+		if !found {
+			perms = make(map[string]int32)
+			AclCachePtr.userPredPerms[userID] = perms
+		}
+		for predicate, permission := range newPerms {
+			perms[predicate] = permission
+		}
 	}
 }
 

worker/acl_cache_test.go

@@ -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"
@@ -51,3 +61,92 @@ 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")
+
+			AclCachePtr.Update(nsTwo, nil)
+			require.NotContains(t, AclCachePtr.userPredPerms, userID,
+				"clearing the last namespace should remove the empty user entry")
+		})
+	}
+}
+
+func TestGetUserPredPermsReturnsSnapshot(t *testing.T) {
+	resetAclCacheForTest(t)
+
+	const (
+		userID = "alice"
+		ns     = uint64(1)
+	)
+	groups := func(predicate string, permission int32) []acl.Group {
+		return []acl.Group{{
+			GroupID: "dev",
+			Users:   []acl.User{{UserID: userID}},
+			Rules:   []acl.Acl{{Predicate: predicate, Perm: permission}},
+		}}
+	}
+
+	oldPredicate := x.NamespaceAttr(ns, "old-predicate")
+	AclCachePtr.Update(ns, groups("old-predicate", acl.Read.Code))
+	snapshot := AclCachePtr.GetUserPredPerms(userID)
+
+	AclCachePtr.Update(ns, groups("new-predicate", acl.Write.Code))
+	require.Equal(t, map[string]int32{oldPredicate: acl.Read.Code}, snapshot,
+		"updating the cache should not mutate a previously returned snapshot")
+
+	snapshot[x.NamespaceAttr(ns, "caller-only")] = acl.Modify.Code
+	require.NotContains(t, AclCachePtr.GetUserPredPerms(userID),
+		x.NamespaceAttr(ns, "caller-only"),
+		"mutating a snapshot should not mutate the cache")
+}