when applicable, make wildcard verb for stages include promote

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
akuity · Oct 30, 2024 · fcc0075 · fcc0075
1 parent f30f66b
commit fcc0075
Show file tree

Hide file tree

Showing 3 changed files with 141 additions and 69 deletions.
diff --git a/internal/api/rbac/policy_rules.go b/internal/api/rbac/policy_rules.go
@@ -12,15 +12,36 @@ import (
 	rolloutsapi "github.com/akuity/kargo/internal/controller/rollouts/api/v1alpha1"
 )
 
-var allVerbs = []string{
-	"create",
-	"delete",
-	"deletecollection",
-	"get",
-	"list",
-	"patch",
-	"update",
-	"watch",
+var (
+	allVerbs = []string{
+		"create",
+		"delete",
+		"deletecollection",
+		"get",
+		"list",
+		"patch",
+		"update",
+		"watch",
+	}
+
+	allStagesVerbs = append(allVerbs, "promote")
+)
+
+func init() {
+	slices.Sort(allVerbs)
+	slices.Sort(allStagesVerbs)
+}
+
+type PolicyRuleNormalizationOptions struct {
+	// IncludeCustomVerbsInExpansion indicates whether custom verbs (like
+	// "promote" for Stages) should be included in the expansion of the "*"
+	// wildcard verb. This is optional because when normalizing PolicyRules with
+	// the intent to create or update a Role, this is how we would like "*" to be
+	// interpreted. However, when normalizing PolicyRules with the intent to
+	// display them to the user, we will not want to expand "*" to include custom
+	// verbs because Kubernetes own interpretation of "*" does not include custom
+	// verbs.
+	IncludeCustomVerbsInExpansion bool
 }
 
 // NormalizePolicyRules returns a predictably ordered slice of Normalized
@@ -30,8 +51,11 @@ var allVerbs = []string{
 // necessary such that each rule references a single APIGroup, a single
 // Resource, and at most a single ResourceName, with wildcard verbs expanded and
 // all applicable verbs de-duplicated and sorted.
-func NormalizePolicyRules(rules []rbacv1.PolicyRule) ([]rbacv1.PolicyRule, error) {
-	rulesMap, err := BuildNormalizedPolicyRulesMap(rules)
+func NormalizePolicyRules(
+	rules []rbacv1.PolicyRule,
+	opts *PolicyRuleNormalizationOptions,
+) ([]rbacv1.PolicyRule, error) {
+	rulesMap, err := BuildNormalizedPolicyRulesMap(rules, opts)
 	if err != nil {
 		return nil, err
 	}
@@ -49,6 +73,7 @@ func NormalizePolicyRules(rules []rbacv1.PolicyRule) ([]rbacv1.PolicyRule, error
 // de-duplicated and sorted.
 func BuildNormalizedPolicyRulesMap(
 	rules []rbacv1.PolicyRule,
+	opts *PolicyRuleNormalizationOptions,
 ) (map[string]rbacv1.PolicyRule, error) {
 	rulesMap := make(map[string]rbacv1.PolicyRule)
 	for _, rule := range rules {
@@ -68,7 +93,7 @@ func BuildNormalizedPolicyRulesMap(
 				if existingRule, ok := rulesMap[key]; ok {
 					verbs = append(existingRule.Verbs, verbs...)
 				}
-				rulesMap[key] = buildRule(group, resource, resourceName, verbs)
+				rulesMap[key] = buildRule(group, resource, resourceName, verbs, opts)
 			}
 		}
 	}
@@ -115,14 +140,18 @@ func buildRule(
 	resource string,
 	resourceName string,
 	verbs []string,
+	opts *PolicyRuleNormalizationOptions,
 ) rbacv1.PolicyRule {
+	if opts == nil {
+		opts = &PolicyRuleNormalizationOptions{}
+	}
 	// De-dupe verbs and expand verb wildcards
 	verbsMap := make(map[string]struct{})
 	for _, verb := range verbs {
 		verb = strings.TrimSpace(verb)
 		if verb == "*" {
 			verbsMap = make(map[string]struct{})
-			for _, verb := range allVerbs {
+			for _, verb := range allVerbsFor(resource, opts.IncludeCustomVerbsInExpansion) {
 				verbsMap[verb] = struct{}{}
 			}
 		} else {
@@ -184,3 +213,15 @@ func getGroupName(resourceType string) string {
 		return "" // If the resourceType was validated, this will never happen
 	}
 }
+
+func allVerbsFor(resourceType string, includeCustom bool) []string {
+	if !includeCustom {
+		return allVerbs
+	}
+	switch resourceType {
+	case "stages":
+		return allStagesVerbs
+	default:
+		return allVerbs
+	}
+}
diff --git a/internal/api/rbac/policy_rules_test.go b/internal/api/rbac/policy_rules_test.go
@@ -11,36 +11,39 @@ import (
 
 func TestNormalizePolicyRules(t *testing.T) {
 	t.Run("invalid resource type", func(t *testing.T) {
-		_, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		_, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{""},
 				Resources: []string{"fake-resource"},
 				Verbs:     []string{"get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.ErrorContains(t, err, "unrecognized resource type")
 	})
 
 	t.Run("singular resource type", func(t *testing.T) {
-		_, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		_, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{""},
 				Resources: []string{"stage"},
 				Verbs:     []string{"get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.ErrorContains(t, err, `unrecognized resource type "stage"`)
 		require.ErrorContains(t, err, `did you mean "stages"`)
 	})
 
 	t.Run("multiple resources expand", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{""},
 				Resources: []string{"secrets", "serviceaccounts"},
 				Verbs:     []string{"get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -61,14 +64,15 @@ func TestNormalizePolicyRules(t *testing.T) {
 	})
 
 	t.Run("multiple resources names expand", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups:     []string{""},
 				Resources:     []string{"serviceaccounts"},
 				ResourceNames: []string{"foo", "bar"},
 				Verbs:         []string{"get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -91,13 +95,14 @@ func TestNormalizePolicyRules(t *testing.T) {
 	})
 
 	t.Run("verbs get sorted", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{""},
 				Resources: []string{"serviceaccounts"},
 				Verbs:     []string{"list", "get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -113,13 +118,14 @@ func TestNormalizePolicyRules(t *testing.T) {
 	})
 
 	t.Run("verbs get de-duped", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{""},
 				Resources: []string{"serviceaccounts"},
 				Verbs:     []string{"get", "get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -135,13 +141,14 @@ func TestNormalizePolicyRules(t *testing.T) {
 	})
 
 	t.Run("wildcard verbs expand", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{""},
 				Resources: []string{"serviceaccounts"},
 				Verbs:     []string{"*"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -157,13 +164,14 @@ func TestNormalizePolicyRules(t *testing.T) {
 	})
 
 	t.Run("correct groups are determined automatically", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{{
 				APIGroups: []string{"", "foo", "bar"},
 				Resources: []string{"stages"},
 				Verbs:     []string{"get"},
-			},
-		})
+			}},
+			nil,
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -179,24 +187,27 @@ func TestNormalizePolicyRules(t *testing.T) {
 	})
 
 	t.Run("kitchen sink", func(t *testing.T) {
-		rules, err := NormalizePolicyRules([]rbacv1.PolicyRule{
-			{ // Never mind that this doesn't make sense. It should all get fixed
-				APIGroups: []string{""},
-				Resources: []string{"serviceaccounts", "stages"},
-				Verbs:     []string{"*"},
-			},
-			{ // These should get de-duped
-				APIGroups: []string{kargoapi.GroupVersion.Group},
-				Resources: []string{"stages"},
-				Verbs:     []string{"*"},
-			},
-			{
-				APIGroups:     []string{kargoapi.GroupVersion.Group},
-				Resources:     []string{"warehouses"},
-				ResourceNames: []string{"foo", "bar"},
-				Verbs:         []string{"get", "list"},
+		rules, err := NormalizePolicyRules(
+			[]rbacv1.PolicyRule{
+				{ // Never mind that this doesn't make sense. It should all get fixed
+					APIGroups: []string{""},
+					Resources: []string{"serviceaccounts", "stages"},
+					Verbs:     []string{"*"},
+				},
+				{ // These should get de-duped
+					APIGroups: []string{kargoapi.GroupVersion.Group},
+					Resources: []string{"stages"},
+					Verbs:     []string{"*"},
+				},
+				{
+					APIGroups:     []string{kargoapi.GroupVersion.Group},
+					Resources:     []string{"warehouses"},
+					ResourceNames: []string{"foo", "bar"},
+					Verbs:         []string{"get", "list"},
+				},
 			},
-		})
+			&PolicyRuleNormalizationOptions{IncludeCustomVerbsInExpansion: true},
+		)
 		require.NoError(t, err)
 		require.Equal(
 			t,
@@ -209,7 +220,7 @@ func TestNormalizePolicyRules(t *testing.T) {
 				{
 					APIGroups: []string{kargoapi.GroupVersion.Group},
 					Resources: []string{"stages"},
-					Verbs:     allVerbs,
+					Verbs:     allStagesVerbs,
 				},
 				{
 					APIGroups:     []string{kargoapi.GroupVersion.Group},

diff --git a/internal/api/rbac/roles.go b/internal/api/rbac/roles.go
@@ -353,6 +353,17 @@ func (r *rolesDatabase) GrantPermissionsToRole(
 
 	group := getGroupName(resourceDetails.ResourceType)
 
+	// Deal with wildcard verb
+	for _, verb := range resourceDetails.Verbs {
+		if strings.TrimSpace(verb) == "*" {
+			resourceDetails.Verbs = append(
+				resourceDetails.Verbs,
+				allVerbsFor(resourceDetails.ResourceType, true)...,
+			)
+			break
+		}
+	}
+
 	newRole := role
 	if newRole == nil {
 		newRole = buildNewRole(project, name)
@@ -365,7 +376,7 @@ func (r *rolesDatabase) GrantPermissionsToRole(
 	if resourceDetails.ResourceName != "" {
 		newRule.ResourceNames = []string{resourceDetails.ResourceName}
 	}
-	if newRole.Rules, err = NormalizePolicyRules(append(newRole.Rules, newRule)); err != nil {
+	if newRole.Rules, err = NormalizePolicyRules(append(newRole.Rules, newRule), nil); err != nil {
 		return nil, fmt.Errorf("error normalizing RBAC policy rules: %w", err)
 	}
 
@@ -494,14 +505,17 @@ func (r *rolesDatabase) RevokePermissionsFromRole(
 	}
 
 	// Normalize the rules before attempting to modify them
-	if role.Rules, err = NormalizePolicyRules(role.Rules); err != nil {
+	if role.Rules, err = NormalizePolicyRules(role.Rules, nil); err != nil {
 		return nil, fmt.Errorf("error normalizing RBAC policy rules: %w", err)
 	}
 
 	// Deal with wildcard verb
 	for _, verb := range resourceDetails.Verbs {
 		if strings.TrimSpace(verb) == "*" {
-			resourceDetails.Verbs = append(resourceDetails.Verbs, allVerbs...)
+			resourceDetails.Verbs = append(
+				resourceDetails.Verbs,
+				allVerbsFor(resourceDetails.ResourceType, true)...,
+			)
 			break
 		}
 	}
@@ -601,7 +615,10 @@ func (r *rolesDatabase) Update(
 	if newRole == nil {
 		newRole = buildNewRole(kargoRole.Namespace, kargoRole.Name)
 	}
-	if newRole.Rules, err = NormalizePolicyRules(kargoRole.Rules); err != nil {
+	if newRole.Rules, err = NormalizePolicyRules(
+		kargoRole.Rules,
+		&PolicyRuleNormalizationOptions{IncludeCustomVerbsInExpansion: true},
+	); err != nil {
 		return nil, fmt.Errorf("error normalizing RBAC policy rules: %w", err)
 	}
 	if role == nil {
@@ -681,7 +698,7 @@ func ResourcesToRole(
 	// underlying resources are not Kargo-managed.
 	if kargoRole.KargoManaged {
 		var err error
-		if kargoRole.Rules, err = NormalizePolicyRules(kargoRole.Rules); err != nil {
+		if kargoRole.Rules, err = NormalizePolicyRules(kargoRole.Rules, nil); err != nil {
 			return nil, fmt.Errorf("error normalizing RBAC policy rules: %w", err)
 		}
 	}
@@ -699,7 +716,10 @@ func RoleToResources(
 
 	role := buildNewRole(kargoRole.Namespace, kargoRole.Name)
 	var err error
-	if role.Rules, err = NormalizePolicyRules(kargoRole.Rules); err != nil {
+	if role.Rules, err = NormalizePolicyRules(
+		kargoRole.Rules,
+		&PolicyRuleNormalizationOptions{IncludeCustomVerbsInExpansion: true},
+	); err != nil {
 		return nil, nil, nil, fmt.Errorf("error normalizing RBAC policy rules: %w", err)
 	}