Disallow AttributeRestrictions in PolicyRules

Signed-off-by: Monis Khan <[email protected]>
openshift · Mar 20, 2017 · a29c8a7 · a29c8a7
1 parent 0781662
commit a29c8a7
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 38 deletions.
diff --git a/pkg/authorization/api/helpers.go b/pkg/authorization/api/helpers.go
@@ -366,6 +366,10 @@ func (r *PolicyRuleBuilder) RuleOrDie() PolicyRule {
 }
 
 func (r *PolicyRuleBuilder) Rule() (PolicyRule, error) {
+	if r.PolicyRule.AttributeRestrictions != nil {
+		return PolicyRule{}, fmt.Errorf("rule may not have attributeRestrictions because they are deprecated and ignored: %#v", r.PolicyRule)
+	}
+
 	if len(r.PolicyRule.Verbs) == 0 {
 		return PolicyRule{}, fmt.Errorf("verbs are required: %#v", r.PolicyRule)
 	}

diff --git a/pkg/authorization/api/validation/validation.go b/pkg/authorization/api/validation/validation.go
@@ -252,7 +252,13 @@ func ValidateRole(role *authorizationapi.Role, isNamespaced bool) field.ErrorLis
 }
 
 func validateRole(role *authorizationapi.Role, isNamespaced bool, fldPath *field.Path) field.ErrorList {
-	return validation.ValidateObjectMeta(&role.ObjectMeta, isNamespaced, path.ValidatePathSegmentName, fldPath.Child("metadata"))
+	allErrs := validation.ValidateObjectMeta(&role.ObjectMeta, isNamespaced, path.ValidatePathSegmentName, fldPath.Child("metadata"))
+	for i, rule := range role.Rules {
+		if rule.AttributeRestrictions != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("rules").Index(i).Child("attributeRestrictions"), rule.AttributeRestrictions, "must be null"))
+		}
+	}
+	return allErrs
 }
 
 func ValidateRoleUpdate(role *authorizationapi.Role, oldRole *authorizationapi.Role, isNamespaced bool) field.ErrorList {

diff --git a/pkg/authorization/api/validation/validation_test.go b/pkg/authorization/api/validation/validation_test.go
@@ -58,6 +58,21 @@ func TestValidatePolicy(t *testing.T) {
 			T: field.ErrorTypeInvalid,
 			F: "roles[any1].metadata.name",
 		},
+		"invalid role": {
+			A: authorizationapi.Policy{
+				ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: authorizationapi.PolicyName},
+				Roles: map[string]*authorizationapi.Role{
+					"any1": {
+						ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: "any1"},
+						Rules: []authorizationapi.PolicyRule{
+							{AttributeRestrictions: &authorizationapi.RoleBinding{}},
+						},
+					},
+				},
+			},
+			T: field.ErrorTypeInvalid,
+			F: "roles[any1].rules[0].attributeRestrictions",
+		},
 	}
 	for k, v := range errorCases {
 		errs := ValidatePolicy(&v.A, true)
@@ -370,6 +385,16 @@ func TestValidateRole(t *testing.T) {
 			T: field.ErrorTypeRequired,
 			F: "metadata.name",
 		},
+		"invalid rule": {
+			A: authorizationapi.Role{
+				ObjectMeta: kapi.ObjectMeta{Name: authorizationapi.PolicyName, Namespace: kapi.NamespaceDefault},
+				Rules: []authorizationapi.PolicyRule{
+					{AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
+				},
+			},
+			T: field.ErrorTypeInvalid,
+			F: "rules[0].attributeRestrictions",
+		},
 	}
 	for k, v := range errorCases {
 		errs := ValidateRole(&v.A, true)

diff --git a/test/extended/testdata/roles/policy-clusterroles.yaml b/test/extended/testdata/roles/policy-clusterroles.yaml
@@ -33,15 +33,6 @@ items:
     - projects
     verbs:
     - list
-  - apiGroups: null
-    attributeRestrictions:
-      apiVersion: v1
-      kind: IsPersonalSubjectAccessReview
-    resources:
-    - localsubjectaccessreviews
-    - subjectaccessreviews
-    verbs:
-    - create
 - apiVersion: v1
   groupNames:
   - system:authenticated

diff --git a/test/extended/testdata/roles/policy-roles.yaml b/test/extended/testdata/roles/policy-roles.yaml
@@ -42,15 +42,6 @@ objects:
       - projects
       verbs:
       - list
-    - apiGroups: null
-      attributeRestrictions:
-        apiVersion: v1
-        kind: IsPersonalSubjectAccessReview
-      resources:
-      - localsubjectaccessreviews
-      - subjectaccessreviews
-      verbs:
-      - create
   - apiVersion: v1
     groupNames:
     - system:authenticated
@@ -72,4 +63,4 @@ objects:
     subjects:
     - kind: SystemGroup
       name: system:authenticated
-    userNames: null
+    userNames: null
diff --git a/test/testdata/basic-user-with-annotations-labels-groups-without-projectrequests.yaml b/test/testdata/basic-user-with-annotations-labels-groups-without-projectrequests.yaml
@@ -40,15 +40,6 @@ rules:
   verbs:
   - list
   - watch
-- apiGroups: null
-  attributeRestrictions:
-    apiVersion: v1
-    kind: IsPersonalSubjectAccessReview
-  resources:
-  - localsubjectaccessreviews
-  - subjectaccessreviews
-  verbs:
-  - create
 - apiGroups: null
   attributeRestrictions: null
   resources:

diff --git a/test/testdata/basic-user-with-groups-without-projectrequests.yaml b/test/testdata/basic-user-with-groups-without-projectrequests.yaml
@@ -34,15 +34,6 @@ rules:
   verbs:
   - list
   - watch
-- apiGroups: null
-  attributeRestrictions:
-    apiVersion: v1
-    kind: IsPersonalSubjectAccessReview
-  resources:
-  - localsubjectaccessreviews
-  - subjectaccessreviews
-  verbs:
-  - create
 - apiGroups: null
   attributeRestrictions: null
   resources: