From a4316ff24b59018960c5a65f4aba20c5b437691d Mon Sep 17 00:00:00 2001
From: Simon Howe <simon@weave.works>
Date: Wed, 10 Aug 2022 10:02:50 +0200
Subject: [PATCH] Fixes evaluating whether cluster-admin can access a namespace

- We don't handle wildcards in the code right now.
---
 core/nsaccess/nsaccess.go      | 26 ++++++++++++++++-----
 core/nsaccess/nsaccess_test.go | 42 ++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/core/nsaccess/nsaccess.go b/core/nsaccess/nsaccess.go
index 832b6e9a28..f371cc60d6 100644
--- a/core/nsaccess/nsaccess.go
+++ b/core/nsaccess/nsaccess.go
@@ -112,6 +112,11 @@ func hasAllRules(status authorizationv1.SubjectRulesReviewStatus, rules []rbacv1
 	derivedAccess := map[string]map[string]map[string]bool{}
 
 	for _, statusRule := range status.ResourceRules {
+		// cluster-admin etc
+		if containsWildcard(statusRule.APIGroups) && containsWildcard(statusRule.Resources) && containsWildcard(statusRule.Verbs) {
+			return true
+		}
+
 		for _, apiGroup := range statusRule.APIGroups {
 			if _, ok := derivedAccess[apiGroup]; !ok {
 				derivedAccess[apiGroup] = map[string]map[string]bool{}
@@ -122,12 +127,12 @@ func hasAllRules(status authorizationv1.SubjectRulesReviewStatus, rules []rbacv1
 					derivedAccess[apiGroup][resource] = map[string]bool{}
 				}
 
-				for _, verb := range statusRule.Verbs {
-					if verb == "*" {
-						for _, v := range allK8sVerbs {
-							derivedAccess[apiGroup][resource][v] = true
-						}
-					} else {
+				if containsWildcard(statusRule.Verbs) {
+					for _, v := range allK8sVerbs {
+						derivedAccess[apiGroup][resource][v] = true
+					}
+				} else {
+					for _, verb := range statusRule.Verbs {
 						derivedAccess[apiGroup][resource][verb] = true
 					}
 				}
@@ -172,6 +177,15 @@ Rules:
 	return hasAccess
 }
 
+func containsWildcard(permissions []string) bool {
+	for _, p := range permissions {
+		if p == "*" {
+			return true
+		}
+	}
+	return false
+}
+
 func newAuthClient(cfg *rest.Config) (typedauth.AuthorizationV1Interface, error) {
 	cs, err := kubernetes.NewForConfig(cfg)
 	if err != nil {
diff --git a/core/nsaccess/nsaccess_test.go b/core/nsaccess/nsaccess_test.go
index b2995166d8..7653edf99c 100644
--- a/core/nsaccess/nsaccess_test.go
+++ b/core/nsaccess/nsaccess_test.go
@@ -208,7 +208,7 @@ func TestFilterAccessibleNamespaces(t *testing.T) {
 
 		g.Expect(filtered).To(HaveLen(1))
 	})
-	t.Run("works when a user has * permissions on a resource", func(t *testing.T) {
+	t.Run("works when a user has * permissions on verbs", func(t *testing.T) {
 		g := NewGomegaWithT(t)
 		ns := newNamespace(context.Background(), adminClient, NewGomegaWithT(t))
 		defer removeNs(t, adminClient, ns)
@@ -244,7 +244,45 @@ func TestFilterAccessibleNamespaces(t *testing.T) {
 
 		g.Expect(filtered).To(HaveLen(1))
 	})
-	t.Run("works with cluster-admin", func(t *testing.T) {
+	t.Run("works when a user has * permissions on a resource", func(t *testing.T) {
+		t.Skip("Doesn't work right now")
+
+		g := NewGomegaWithT(t)
+		ns := newNamespace(context.Background(), adminClient, NewGomegaWithT(t))
+		defer removeNs(t, adminClient, ns)
+
+		userName = userName + "-" + rand.String(5)
+
+		roleName := makeRole(ns)
+
+		roleRules := []rbacv1.PolicyRule{
+			{
+				APIGroups: []string{""},
+				Resources: []string{"*"},
+				Verbs:     []string{"*"},
+			},
+		}
+
+		userCfg := newRestConfigWithRole(t, testCfg, roleName, roleRules)
+
+		requiredRules := []rbacv1.PolicyRule{
+			{
+				APIGroups: []string{""},
+				Resources: []string{"secrets", "events", "namespaces"},
+				Verbs:     []string{"get", "list"},
+			},
+		}
+
+		list := &corev1.NamespaceList{}
+		g.Expect(adminClient.List(ctx, list)).To(Succeed())
+
+		checker := NewChecker(requiredRules)
+		filtered, err := checker.FilterAccessibleNamespaces(ctx, userCfg, list.Items)
+		g.Expect(err).NotTo(HaveOccurred())
+
+		g.Expect(filtered).To(HaveLen(1))
+	})
+	t.Run("works with cluster-admin who have * for everything", func(t *testing.T) {
 		g := NewGomegaWithT(t)
 		ns := newNamespace(context.Background(), adminClient, NewGomegaWithT(t))
 		defer removeNs(t, adminClient, ns)