Fix SAR groups and Extra

tektoncd · Oct 11, 2023 · 56c15b2 · 56c15b2
1 parent e203c93
commit 56c15b2
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 19 deletions.
diff --git a/pkg/api/server/v1alpha2/auth/rbac.go b/pkg/api/server/v1alpha2/auth/rbac.go
@@ -16,6 +16,7 @@ package auth
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log"
 	"strings"
@@ -75,7 +76,7 @@ func (r *RBAC) Check(ctx context.Context, namespace, resource, verb string) erro
 		impersonator, err = impersonation.NewImpersonation(md)
 		// Ignore ErrorNoImpersonationData errors. This means that the request does not have any
 		// impersonation headers and should be processed normally.
-		if err != nil && err != impersonation.ErrNoImpersonationData {
+		if err != nil && !errors.Is(err, impersonation.ErrNoImpersonationData) {
 			log.Println(err)
 			return status.Error(codes.Unauthenticated, "invalid impersonation data")
 		}
@@ -119,8 +120,8 @@ func (r *RBAC) Check(ctx context.Context, namespace, resource, verb string) erro
 
 		user := tr.Status.User.Username
 		UID := tr.Status.User.UID
-		groups := []string{"tekton.dev"}
-		extra := map[string]authzv1.ExtraValue{}
+		groups := tr.Status.User.Groups
+		extra := convertExtra[authnv1.ExtraValue](tr.Status.User.Extra)
 
 		// Check whether the authenticated user has permission to impersonate
 		if impersonator != nil {
@@ -134,7 +135,7 @@ func (r *RBAC) Check(ctx context.Context, namespace, resource, verb string) erro
 			user = userInfo.GetName()
 			UID = userInfo.GetUID()
 			groups = userInfo.GetGroups()
-			extra = convertExtra(userInfo.GetExtra())
+			extra = convertExtra[[]string](userInfo.GetExtra())
 		}
 
 		// Authorize the request by checking the RBAC permissions for the resource.
@@ -166,11 +167,11 @@ func (r *RBAC) Check(ctx context.Context, namespace, resource, verb string) erro
 	return status.Error(codes.Unauthenticated, retMsg)
 }
 
-// convertExtra converts the map[string][]string to map[string]ExtraValue for Subject Access Review.
-func convertExtra(extra map[string][]string) map[string]authzv1.ExtraValue {
+// convertExtra converts the map[string][]string or authnv1.ExtraValue to map[string]ExtraValue for Subject Access Review.
+func convertExtra[T []string | authnv1.ExtraValue](extra map[string]T) map[string]authzv1.ExtraValue {
 	var newExtra = make(map[string]authzv1.ExtraValue)
 	for key, value := range extra {
-		newExtra[key] = value
+		newExtra[key] = authzv1.ExtraValue(value)
 	}
 	return newExtra
 }

diff --git a/pkg/api/server/v1alpha2/auth/rbac_test.go b/pkg/api/server/v1alpha2/auth/rbac_test.go
@@ -39,8 +39,22 @@ import (
 func TestRBAC(t *testing.T) {
 	// Map of users -> tokens. The 'authorized' user has full permissions.
 	users := map[string]string{
-		"authorized":   "a",
-		"unauthorized": "b",
+		"authorized":         "a",
+		"unauthorized":       "b",
+		"authorized-group":   "c",
+		"unauthorized-group": "d",
+		"authorized-extra":   "e",
+		"unauthorized-extra": "f",
+	}
+	// Map of users -> groups. The 'authorized' group has full permissions.
+	groups := map[string][]string{
+		"authorized-group":   {"authorized"},
+		"unauthorized-group": {"unauthorized"},
+	}
+	// Map of users -> extra. The 'authorized' scope has full permissions.
+	extra := map[string]map[string]authnv1.ExtraValue{
+		"authorized-extra":   {"scope": {"authorized", "stage"}},
+		"unauthorized-extra": {"scope": {"unauthorized", "stage"}},
 	}
 	k8s := fake.NewSimpleClientset()
 	k8s.PrependReactor("create", "tokenreviews", func(action test.Action) (handled bool, ret runtime.Object, err error) {
@@ -51,6 +65,8 @@ func TestRBAC(t *testing.T) {
 					Authenticated: true,
 					User: authnv1.UserInfo{
 						Username: user,
+						Groups:   groups[user],
+						Extra:    extra[user],
 					},
 				}
 				return true, tr, nil
@@ -63,7 +79,7 @@ func TestRBAC(t *testing.T) {
 	})
 	k8s.PrependReactor("create", "subjectaccessreviews", func(action test.Action) (handled bool, ret runtime.Object, err error) {
 		sar := action.(test.CreateActionImpl).Object.(*authzv1.SubjectAccessReview)
-		if sar.Spec.User == "authorized" || slices.Contains(sar.Spec.Groups, "authorized") {
+		if sar.Spec.User == "authorized" || slices.Contains(sar.Spec.Groups, "authorized") || slices.Contains(sar.Spec.Extra["scope"], "authorized") {
 			sar.Status = authzv1.SubjectAccessReviewStatus{
 				Allowed: true,
 			}
@@ -81,63 +97,75 @@ func TestRBAC(t *testing.T) {
 	record := "foo/results/bar/records/baz"
 	for _, tc := range []struct {
 		name             string
-		user             string
 		token            string
 		impersonateUser  string
 		impersonateGroup string
 		want             codes.Code
 	}{
 		{
 			name:  "authorized user",
-			user:  "authorized",
 			token: users["authorized"],
 			want:  codes.OK,
 		},
 		{
 			name:  "unauthorized user",
-			user:  "unauthorized",
 			token: users["unauthorized"],
 			want:  codes.Unauthenticated,
 		},
+		{
+			name:  "authorized group",
+			token: users["authorized-group"],
+			want:  codes.OK,
+		},
+		{
+			name:  "unauthorized group",
+			token: users["unauthorized-group"],
+			want:  codes.Unauthenticated,
+		},
+		{
+			name:  "authorized extra",
+			token: users["authorized-extra"],
+			want:  codes.OK,
+		},
+		{
+			name:  "unauthorized extra",
+			token: users["unauthorized-extra"],
+			want:  codes.Unauthenticated,
+		},
 		{
 			name:  "unauthenticated user",
-			user:  "unauthenticated",
 			token: "",
 			want:  codes.Unauthenticated,
 		},
 		{
 			name:            "authorized impersonated user",
-			user:            "authorized",
 			token:           users["authorized"],
 			impersonateUser: "authorized",
 			want:            codes.OK,
 		},
 		{
 			name:            "unauthorized impersonated user",
-			user:            "authorized",
 			token:           users["authorized"],
 			impersonateUser: "unauthorized",
 			want:            codes.Unauthenticated,
 		},
 		{
 			name:             "authorized impersonated group",
-			user:             "authorized",
 			token:            users["authorized"],
 			impersonateUser:  "unauthorized",
 			impersonateGroup: "authorized",
 			want:             codes.OK,
 		},
 		{
 			name:             "unauthorized impersonated group",
-			user:             "authorized",
 			token:            users["authorized"],
 			impersonateUser:  "unauthorized",
 			impersonateGroup: "unauthorized",
 			want:             codes.Unauthenticated,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			// Simulates a oauth.TokenSource. We avoid using the real
+			// Simulates an oauth.TokenSource. We avoid using the real
 			// oauth.TokenSource here since it requires a higher SecurityLevel
 			// + TLS.
 			ctx := metadata.AppendToOutgoingContext(ctx, "authorization", fmt.Sprintf("Bearer %s", tc.token))