diff --git a/edgraph/access.go b/edgraph/access.go index 15c0fb458ce..0453fdb9920 100644 --- a/edgraph/access.go +++ b/edgraph/access.go @@ -647,9 +647,10 @@ func authorizePreds(ctx context.Context, userData *userData, preds []string, return &authPredResult{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) } diff --git a/worker/acl_cache.go b/worker/acl_cache.go index cd322cd503f..5d02d35745c 100644 --- a/worker/acl_cache.go +++ b/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,12 +144,15 @@ 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 @@ -148,8 +160,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] + if !found { + perms = make(map[string]int32) + AclCachePtr.userPredPerms[userID] = perms + } + for predicate, permission := range newPerms { + perms[predicate] = permission + } } } diff --git a/worker/acl_cache_test.go b/worker/acl_cache_test.go index badf52f02a8..63353ed79c6 100644 --- a/worker/acl_cache_test.go +++ b/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") +}