fix: update rbac validation to support applications in different name…

…spaces (argoproj#17777) * fix: policies in namespaces The introduction of applications in any namespace doesn't appear to include support for specifying namespaces in rbac policy objects. This causes valid rbac objects (like `some-project/some-namespace/some-application`) to fail when they are really valid. Update the regex to include the ability to specify a namespace. Signed-off-by: Bryce Lowe <[email protected]> * fix: update verbiage of failing test Signed-off-by: Bryce Lowe <[email protected]> * feedback: test for policy and project Signed-off-by: Bryce Lowe <[email protected]> * fix: update test name Signed-off-by: Bryce Lowe <[email protected]> --------- Signed-off-by: Bryce Lowe <[email protected]>
pasha-codefresh · Apr 10, 2024 · c2dfab5 · c2dfab5
1 parent ebe4804
commit c2dfab5
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 4 deletions.
diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
@@ -2084,6 +2084,12 @@ func isValidResource(resource string) bool {
 	return validResources[resource]
 }
 
+func isValidObject(proj string, object string) bool {
+	// match against <PROJECT>[/<NAMESPACE>]/<APPLICATION>
+	objectRegexp, err := regexp.Compile(fmt.Sprintf(`^%s(/[*\w-.]+)?/[*\w-.]+$`, regexp.QuoteMeta(proj)))
+	return objectRegexp.MatchString(object) && err == nil
+}
+
 func validatePolicy(proj string, role string, policy string) error {
 	policyComponents := strings.Split(policy, ",")
 	if len(policyComponents) != 6 || strings.Trim(policyComponents[0], " ") != "p" {
@@ -2107,9 +2113,8 @@ func validatePolicy(proj string, role string, policy string) error {
 	}
 	// object
 	object := strings.Trim(policyComponents[4], " ")
-	objectRegexp, err := regexp.Compile(fmt.Sprintf(`^%s/[*\w-.]+$`, regexp.QuoteMeta(proj)))
-	if err != nil || !objectRegexp.MatchString(object) {
-		return status.Errorf(codes.InvalidArgument, "invalid policy rule '%s': object must be of form '%s/*' or '%s/<APPNAME>', not '%s'", policy, proj, proj, object)
+	if !isValidObject(proj, object) {
+		return status.Errorf(codes.InvalidArgument, "invalid policy rule '%s': object must be of form '%s/*', '%s[/<NAMESPACE>]/<APPNAME>' or '%s/<APPNAME>', not '%s'", policy, proj, proj, proj, object)
 	}
 	// effect
 	effect := strings.Trim(policyComponents[5], " ")

diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go
@@ -3082,6 +3082,69 @@ func TestOrphanedResourcesMonitorSettings_IsWarn(t *testing.T) {
 	assert.True(t, settings.IsWarn())
 }
 
+func Test_isValidPolicy(t *testing.T) {
+	policyTests := []struct {
+		name    string
+		policy  string
+		isValid bool
+	}{
+		{
+			name:    "policy with full wildcard",
+			policy:  "some-project/*",
+			isValid: true,
+		},
+		{
+			name:    "policy with specified project and application",
+			policy:  "some-project/some-application",
+			isValid: true,
+		},
+		{
+			name:    "policy with full wildcard namespace and application",
+			policy:  "some-project/*/*",
+			isValid: true,
+		},
+		{
+			name:    "policy with wildcard namespace and specified application",
+			policy:  "some-project/*/some-application",
+			isValid: true,
+		},
+		{
+			name:    "policy with specified namespace and wildcard application",
+			policy:  "some-project/some-namespace/*",
+			isValid: true,
+		},
+		{
+			name:    "policy with wildcard prefix namespace and specified application",
+			policy:  "some-project/some-name*/some-application",
+			isValid: true,
+		},
+		{
+			name:    "policy with specified namespace and wildcard prefixed application",
+			policy:  "some-project/some-namespace/some-app*",
+			isValid: true,
+		},
+		{
+			name:    "policy with valid namespace and application",
+			policy:  "some-project/some-namespace/some-application",
+			isValid: true,
+		},
+		{
+			name:    "policy with invalid namespace character",
+			policy:  "some-project/some~namespace/some-application",
+			isValid: false,
+		},
+		{
+			name:    "policy with invalid application character",
+			policy:  "some-project/some-namespace/some^application",
+			isValid: false,
+		},
+	}
+
+	for _, policyTest := range policyTests {
+		assert.Equal(t, policyTest.isValid, isValidObject("some-project", policyTest.policy), policyTest.name)
+	}
+}
+
 func Test_validatePolicy_projIsNotRegex(t *testing.T) {
 	// Make sure the "." in "some.project" isn't treated as the regex wildcard.
 	err := validatePolicy("some.project", "org-admin", "p, proj:some.project:org-admin, applications, *, some-project/*, allow")

diff --git a/server/project/project_test.go b/server/project/project_test.go
@@ -585,7 +585,7 @@ p, role:admin, projects, update, *, allow`)
 		projectServer := NewServer("default", fake.NewSimpleClientset(), apps.NewSimpleClientset(projWithRole), enforcer, sync.NewKeyLock(), nil, nil, projInformer, settingsMgr, argoDB)
 		request := &project.ProjectUpdateRequest{Project: projWithRole}
 		_, err := projectServer.Update(context.Background(), request)
-		assert.Contains(t, err.Error(), "object must be of form 'test/*' or 'test/<APPNAME>'")
+		assert.Contains(t, err.Error(), "object must be of form 'test/*', 'test[/<NAMESPACE>]/<APPNAME>' or 'test/<APPNAME>'")
 	})
 
 	t.Run("TestValidateProjectIncorrectProjectInRoleFailure", func(t *testing.T) {