Disallow AttributeRestrictions in PolicyRules

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

Hide file tree

Showing 8 changed files with 63 additions and 39 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/bindata.go b/test/extended/testdata/bindata.go
diff --git a/test/extended/testdata/roles/policy-clusterroles.yaml b/test/extended/testdata/roles/policy-clusterroles.yaml
@@ -33,13 +33,11 @@ items:
     - projects
     verbs:
     - list
-  - apiGroups: null
-    attributeRestrictions:
-      apiVersion: v1
-      kind: IsPersonalSubjectAccessReview
+  - apiGroups:
+    - authorization.k8s.io
+    attributeRestrictions: null
     resources:
-    - localsubjectaccessreviews
-    - subjectaccessreviews
+    - selfsubjectaccessreviews
     verbs:
     - create
 - apiVersion: v1

diff --git a/test/extended/testdata/roles/policy-roles.yaml b/test/extended/testdata/roles/policy-roles.yaml
@@ -42,13 +42,11 @@ objects:
       - projects
       verbs:
       - list
-    - apiGroups: null
-      attributeRestrictions:
-        apiVersion: v1
-        kind: IsPersonalSubjectAccessReview
+    - apiGroups:
+      - authorization.k8s.io
+      attributeRestrictions: null
       resources:
-      - localsubjectaccessreviews
-      - subjectaccessreviews
+      - selfsubjectaccessreviews
       verbs:
       - create
   - apiVersion: v1
@@ -72,4 +70,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,13 +40,11 @@ rules:
   verbs:
   - list
   - watch
-- apiGroups: null
-  attributeRestrictions:
-    apiVersion: v1
-    kind: IsPersonalSubjectAccessReview
+- apiGroups:
+  - authorization.k8s.io
+  attributeRestrictions: null
   resources:
-  - localsubjectaccessreviews
-  - subjectaccessreviews
+  - selfsubjectaccessreviews
   verbs:
   - create
 - apiGroups: null

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