refactor(accesscontrol): add unit tests for CacheKey (#245)

This is a follow up to 435e220 with a small refactor to make the function easier to test, and adding unit tests for CacheKey.

pkg/accesscontrol/access_store.go

@@ -4,10 +4,12 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"hash"
 	"sort"
 	"time"
 
 	v1 "github.com/rancher/wrangler/v3/pkg/generated/controllers/rbac/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/util/cache"
 	"k8s.io/apiserver/pkg/authentication/user"
 )
@@ -19,10 +21,21 @@ type AccessSetLookup interface {
 	PurgeUserData(id string)
 }
 
+type policyRules interface {
+	get(string) *AccessSet
+	getRoleBindings(string) []*rbacv1.RoleBinding
+	getClusterRoleBindings(string) []*rbacv1.ClusterRoleBinding
+}
+
+type roleRevisions interface {
+	roleRevision(string, string) string
+}
+
 type AccessStore struct {
-	users  *policyRuleIndex
-	groups *policyRuleIndex
-	cache  *cache.LRUExpireCache
+	usersPolicyRules  policyRules
+	groupsPolicyRules policyRules
+	roles             roleRevisions
+	cache             *cache.LRUExpireCache
 }
 
 type roleKey struct {
@@ -31,10 +44,10 @@ type roleKey struct {
 }
 
 func NewAccessStore(ctx context.Context, cacheResults bool, rbac v1.Interface) *AccessStore {
-	revisions := newRoleRevision(ctx, rbac)
 	as := &AccessStore{
-		users:  newPolicyRuleIndex(true, revisions, rbac),
-		groups: newPolicyRuleIndex(false, revisions, rbac),
+		usersPolicyRules:  newPolicyRuleIndex(true, rbac),
+		groupsPolicyRules: newPolicyRuleIndex(false, rbac),
+		roles:             newRoleRevision(ctx, rbac),
 	}
 	if cacheResults {
 		as.cache = cache.NewLRUExpireCache(50)
@@ -53,9 +66,9 @@ func (l *AccessStore) AccessFor(user user.Info) *AccessSet {
 		}
 	}
 
-	result := l.users.get(user.GetName())
+	result := l.usersPolicyRules.get(user.GetName())
 	for _, group := range user.GetGroups() {
-		result.Merge(l.groups.get(group))
+		result.Merge(l.groupsPolicyRules.get(group))
 	}
 
 	if l.cache != nil {
@@ -73,16 +86,30 @@ func (l *AccessStore) PurgeUserData(id string) {
 func (l *AccessStore) CacheKey(user user.Info) string {
 	d := sha256.New()
 
-	l.users.addRolesToHash(d, user.GetName())
-
 	groupBase := user.GetGroups()
 	groups := make([]string, len(groupBase))
 	copy(groups, groupBase)
 	sort.Strings(groups)
 
+	l.addRolesToHash(d, user.GetName(), l.usersPolicyRules)
 	for _, group := range groups {
-		l.groups.addRolesToHash(d, group)
+		l.addRolesToHash(d, group, l.groupsPolicyRules)
 	}
 
 	return hex.EncodeToString(d.Sum(nil))
 }
+
+func (l *AccessStore) addRolesToHash(digest hash.Hash, subjectName string, rules policyRules) {
+	for _, crb := range rules.getClusterRoleBindings(subjectName) {
+		digest.Write([]byte(crb.RoleRef.Name))
+		digest.Write([]byte(l.roles.roleRevision("", crb.RoleRef.Name)))
+	}
+
+	for _, rb := range rules.getRoleBindings(subjectName) {
+		digest.Write([]byte(rb.RoleRef.Name))
+		if rb.Namespace != "" {
+			digest.Write([]byte(rb.Namespace))
+		}
+		digest.Write([]byte(l.roles.roleRevision(rb.Namespace, rb.RoleRef.Name)))
+	}
+}

pkg/accesscontrol/access_store_test.go

@@ -0,0 +1,328 @@
+package accesscontrol
+
+import (
+	"fmt"
+	"slices"
+	"testing"
+
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/cache"
+	"k8s.io/apiserver/pkg/authentication/user"
+)
+
+func TestAccessStore_CacheKey(t *testing.T) {
+	testUser := &user.DefaultInfo{
+		Name: "user-12345",
+		Groups: []string{
+			"users",
+			"mygroup",
+		},
+	}
+
+	tests := []struct {
+		name   string
+		store  *AccessStore
+		verify func(t *testing.T, store *AccessStore, res string)
+	}{
+		{
+			name: "consistently produces the same value",
+			store: &AccessStore{
+				usersPolicyRules: &policyRulesMock{
+					getRBFunc: func(s string) []*rbacv1.RoleBinding {
+						return []*rbacv1.RoleBinding{
+							makeRB("testns", "testrb", testUser.Name, "testrole"),
+						}
+					},
+					getCRBFunc: func(s string) []*rbacv1.ClusterRoleBinding {
+						return []*rbacv1.ClusterRoleBinding{
+							makeCRB("testcrb", testUser.Name, "testclusterrole"),
+						}
+					},
+				},
+				groupsPolicyRules: &policyRulesMock{},
+				roles: roleRevisionsMock(func(ns, name string) string {
+					return fmt.Sprintf("%s%srev", ns, name)
+				}),
+			},
+			verify: func(t *testing.T, store *AccessStore, res string) {
+				// iterate enough times to make possibly random iterators repeating order by coincidence
+				for range 5 {
+					if res != store.CacheKey(testUser) {
+						t.Fatal("CacheKey is not the same on consecutive runs")
+					}
+				}
+			},
+		},
+		{
+			name: "group permissions are taken into account",
+			store: &AccessStore{
+				usersPolicyRules: &policyRulesMock{},
+				groupsPolicyRules: &policyRulesMock{
+					getRBFunc: func(s string) []*rbacv1.RoleBinding {
+						return []*rbacv1.RoleBinding{
+							makeRB("testns", "testrb", testUser.Name, "testrole"),
+						}
+					},
+					getCRBFunc: func(s string) []*rbacv1.ClusterRoleBinding {
+						return []*rbacv1.ClusterRoleBinding{
+							makeCRB("testcrb", testUser.Name, "testclusterrole"),
+						}
+					},
+				},
+				roles: roleRevisionsMock(func(ns, name string) string {
+					return fmt.Sprintf("%s%srev", ns, name)
+				}),
+			},
+			verify: func(t *testing.T, store *AccessStore, res string) {
+				// remove users
+				testUserAlt := *testUser
+				testUserAlt.Groups = []string{}
+
+				if store.CacheKey(&testUserAlt) == res {
+					t.Fatal("CacheKey does not use groups for hashing")
+				}
+			},
+		},
+		{
+			name: "different groups order produces the same value",
+			store: &AccessStore{
+				usersPolicyRules: &policyRulesMock{},
+				groupsPolicyRules: &policyRulesMock{
+					getRBFunc: func(s string) []*rbacv1.RoleBinding {
+						if s == testUser.Groups[0] {
+							return []*rbacv1.RoleBinding{
+								makeRB("testns", "testrb", testUser.Name, "testrole"),
+							}
+						}
+						return nil
+					},
+					getCRBFunc: func(s string) []*rbacv1.ClusterRoleBinding {
+						if s == testUser.Groups[1] {
+							return []*rbacv1.ClusterRoleBinding{
+								makeCRB("testcrb", testUser.Name, "testclusterrole"),
+							}
+						}
+						return nil
+					},
+				},
+				roles: roleRevisionsMock(func(ns, name string) string {
+					return fmt.Sprintf("%s%srev", ns, name)
+				}),
+			},
+			verify: func(t *testing.T, store *AccessStore, res string) {
+				// swap order of groups
+				testUserAlt := &user.DefaultInfo{Name: testUser.Name, Groups: slices.Clone(testUser.Groups)}
+				slices.Reverse(testUserAlt.Groups)
+
+				if store.CacheKey(testUserAlt) != res {
+					t.Fatal("CacheKey varies depending on groups order")
+				}
+			},
+		},
+		{
+			name: "role changes produce a different value",
+			store: &AccessStore{
+				usersPolicyRules: &policyRulesMock{
+					getRBFunc: func(s string) []*rbacv1.RoleBinding {
+						return []*rbacv1.RoleBinding{
+							makeRB("testns", "testrb", testUser.Name, "testrole"),
+						}
+					},
+				},
+				groupsPolicyRules: &policyRulesMock{},
+				roles: roleRevisionsMock(func(ns, name string) string {
+					return "rev1"
+				}),
+			},
+			verify: func(t *testing.T, store *AccessStore, res string) {
+				store.roles = roleRevisionsMock(func(ns, name string) string {
+					return "rev2"
+
+				})
+				if store.CacheKey(testUser) == res {
+					t.Fatal("CacheKey did not change when on role change")
+				}
+			},
+		},
+		{
+			name: "new groups produce a different value",
+			store: &AccessStore{
+				usersPolicyRules: &policyRulesMock{},
+				groupsPolicyRules: &policyRulesMock{
+					getRBFunc: func(s string) []*rbacv1.RoleBinding {
+						return []*rbacv1.RoleBinding{
+							makeRB("testns", "testrb", testUser.Name, "testrole"),
+						}
+					},
+					getCRBFunc: func(s string) []*rbacv1.ClusterRoleBinding {
+						if s == "newgroup" {
+							return []*rbacv1.ClusterRoleBinding{
+								makeCRB("testcrb", testUser.Name, "testclusterrole"),
+							}
+						}
+						return nil
+					},
+				},
+				roles: roleRevisionsMock(func(ns, name string) string {
+					return fmt.Sprintf("%s%srev", ns, name)
+				}),
+			},
+			verify: func(t *testing.T, store *AccessStore, res string) {
+				testUserAlt := &user.DefaultInfo{
+					Name:   testUser.Name,
+					Groups: append(slices.Clone(testUser.Groups), "newgroup"),
+				}
+				if store.CacheKey(testUserAlt) == res {
+					t.Fatal("CacheKey did not change when new group was added")
+				}
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.verify(t, tt.store, tt.store.CacheKey(testUser))
+		})
+	}
+}
+
+func TestAccessStore_AccessFor(t *testing.T) {
+	testUser := &user.DefaultInfo{
+		Name:   "test-user",
+		Groups: []string{"users", "mygroup"},
+	}
+	asCache := cache.NewLRUExpireCache(10)
+	store := &AccessStore{
+		usersPolicyRules: &policyRulesMock{
+			getRBFunc: func(s string) []*rbacv1.RoleBinding {
+				return []*rbacv1.RoleBinding{
+					makeRB("testns", "testrb", testUser.Name, "testrole"),
+				}
+			},
+			getFunc: func(_ string) *AccessSet {
+				return &AccessSet{
+					set: map[key]resourceAccessSet{
+						{"get", corev1.Resource("ConfigMap")}: map[Access]bool{
+							{Namespace: All, ResourceName: All}: true,
+						},
+					},
+				}
+			},
+		},
+		groupsPolicyRules: &policyRulesMock{
+			getCRBFunc: func(s string) []*rbacv1.ClusterRoleBinding {
+				if s == "mygroup" {
+					return []*rbacv1.ClusterRoleBinding{
+						makeCRB("testcrb", testUser.Name, "testclusterrole"),
+					}
+				}
+				return nil
+			},
+			getFunc: func(_ string) *AccessSet {
+				return &AccessSet{
+					set: map[key]resourceAccessSet{
+						{"list", appsv1.Resource("Deployment")}: map[Access]bool{
+							{Namespace: "testns", ResourceName: All}: true,
+						},
+					},
+				}
+			},
+		},
+		roles: roleRevisionsMock(func(ns, name string) string {
+			return fmt.Sprintf("%s%srev", ns, name)
+		}),
+		cache: asCache,
+	}
+
+	validateAccessSet := func(as *AccessSet) {
+		if as == nil {
+			t.Fatal("AccessFor() returned nil")
+		}
+		if as.ID == "" {
+			t.Fatal("AccessSet has empty ID")
+		}
+		if !as.Grants("get", corev1.Resource("ConfigMap"), "default", "cm") ||
+			!as.Grants("list", appsv1.Resource("Deployment"), "testns", "deploy") {
+			t.Error("AccessSet does not grant desired permissions")
+		}
+		// wrong verbs
+		if as.Grants("delete", corev1.Resource("ConfigMap"), "default", "cm") ||
+			as.Grants("get", appsv1.Resource("Deployment"), "testns", "deploy") ||
+			// wrong resource
+			as.Grants("get", corev1.Resource("Secret"), "testns", "s") {
+			t.Error("AccessSet grants undesired permissions")
+		}
+	}
+
+	as := store.AccessFor(testUser)
+	validateAccessSet(as)
+	if got, want := len(asCache.Keys()), 1; got != want {
+		t.Errorf("Unexpected number of cache keys: got %d, want %d", got, want)
+	}
+
+	as = store.AccessFor(testUser)
+	validateAccessSet(as)
+	if got, want := len(asCache.Keys()), 1; got != want {
+		t.Errorf("Unexpected increase in cache size, got %d, want %d", got, want)
+	}
+
+	store.PurgeUserData(as.ID)
+	if got, want := len(asCache.Keys()), 0; got != want {
+		t.Errorf("Cache was not purged, got len %d, want %d", got, want)
+	}
+}
+
+func makeRB(ns, name, user, role string) *rbacv1.RoleBinding {
+	return &rbacv1.RoleBinding{
+		ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name},
+		Subjects: []rbacv1.Subject{
+			{Name: user},
+		},
+		RoleRef: rbacv1.RoleRef{Name: role},
+	}
+}
+
+func makeCRB(name, user, role string) *rbacv1.ClusterRoleBinding {
+	return &rbacv1.ClusterRoleBinding{
+		ObjectMeta: metav1.ObjectMeta{Name: name},
+		Subjects: []rbacv1.Subject{
+			{Name: user},
+		},
+		RoleRef: rbacv1.RoleRef{Name: role},
+	}
+}
+
+type policyRulesMock struct {
+	getFunc    func(string) *AccessSet
+	getRBFunc  func(string) []*rbacv1.RoleBinding
+	getCRBFunc func(string) []*rbacv1.ClusterRoleBinding
+}
+
+func (p policyRulesMock) get(s string) *AccessSet {
+	if p.getFunc == nil {
+		return nil
+	}
+	return p.getFunc(s)
+}
+
+func (p policyRulesMock) getRoleBindings(s string) []*rbacv1.RoleBinding {
+	if p.getRBFunc == nil {
+		return nil
+	}
+	return p.getRBFunc(s)
+}
+
+func (p policyRulesMock) getClusterRoleBindings(s string) []*rbacv1.ClusterRoleBinding {
+	if p.getCRBFunc == nil {
+		return nil
+	}
+	return p.getCRBFunc(s)
+}
+
+type roleRevisionsMock func(ns, name string) string
+
+func (fn roleRevisionsMock) roleRevision(ns, name string) string {
+	return fn(ns, name)
+}

pkg/accesscontrol/policy_rule_index.go

@@ -2,7 +2,6 @@ package accesscontrol
 
 import (
 	"fmt"
-	"hash"
 	"sort"
 
 	v1 "github.com/rancher/wrangler/v3/pkg/generated/controllers/rbac/v1"
@@ -20,13 +19,12 @@ type policyRuleIndex struct {
 	rCache              v1.RoleCache
 	crbCache            v1.ClusterRoleBindingCache
 	rbCache             v1.RoleBindingCache
-	revisions           *roleRevisionIndex
 	kind                string
 	roleIndexKey        string
 	clusterRoleIndexKey string
 }
 
-func newPolicyRuleIndex(user bool, revisions *roleRevisionIndex, rbac v1.Interface) *policyRuleIndex {
+func newPolicyRuleIndex(user bool, rbac v1.Interface) *policyRuleIndex {
 	key := "Group"
 	if user {
 		key = "User"
@@ -39,7 +37,6 @@ func newPolicyRuleIndex(user bool, revisions *roleRevisionIndex, rbac v1.Interfa
 		rbCache:             rbac.RoleBinding().Cache(),
 		clusterRoleIndexKey: "crb" + key,
 		roleIndexKey:        "rb" + key,
-		revisions:           revisions,
 	}
 
 	pi.crbCache.AddIndexer(pi.clusterRoleIndexKey, pi.clusterRoleBindingBySubjectIndexer)
@@ -72,30 +69,6 @@ func (p *policyRuleIndex) roleBindingBySubject(rb *rbacv1.RoleBinding) (result [
 	return
 }
 
-var null = []byte{'\x00'}
-
-func (p *policyRuleIndex) addRolesToHash(digest hash.Hash, subjectName string) {
-	for _, crb := range p.getClusterRoleBindings(subjectName) {
-		digest.Write([]byte(crb.RoleRef.Name))
-		digest.Write([]byte(p.revisions.roleRevision("", crb.RoleRef.Name)))
-		digest.Write(null)
-	}
-
-	for _, rb := range p.getRoleBindings(subjectName) {
-		switch rb.RoleRef.Kind {
-		case "Role":
-			digest.Write([]byte(rb.RoleRef.Name))
-			digest.Write([]byte(rb.Namespace))
-			digest.Write([]byte(p.revisions.roleRevision(rb.Namespace, rb.RoleRef.Name)))
-			digest.Write(null)
-		case "ClusterRole":
-			digest.Write([]byte(rb.RoleRef.Name))
-			digest.Write([]byte(p.revisions.roleRevision("", rb.RoleRef.Name)))
-			digest.Write(null)
-		}
-	}
-}
-
 func (p *policyRuleIndex) get(subjectName string) *AccessSet {
 	result := &AccessSet{}