Merge pull request #17785 from deads2k/rbac-05-noopinion

Automatic merge from submit-queue (batch tested with PRs 17549, 17785).

fix authorization to use no opinion where possible

Followup to the rebase.  NoOpinion matches previous behavior.

/assign @liggitt 
/assign @simo5 

@openshift/sig-security

pkg/authorization/authorizer/authorizer.go

@@ -29,15 +29,15 @@ func NewSubjectLocator(delegate authorizerrbac.SubjectLocator) SubjectLocator {
 
 func (a *openshiftAuthorizer) Authorize(attributes authorizer.Attributes) (authorizer.Decision, string, error) {
 	if attributes.GetUser() == nil {
-		return authorizer.DecisionDeny, "", errors.New("no user available on context")
+		return authorizer.DecisionNoOpinion, "", errors.New("no user available on context")
 	}
-	authorized, delegateReason, err := a.delegate.Authorize(attributes)
-	if authorized == authorizer.DecisionAllow {
+	authorizationDecision, delegateReason, err := a.delegate.Authorize(attributes)
+	if authorizationDecision == authorizer.DecisionAllow {
 		return authorizer.DecisionAllow, reason(attributes), nil
 	}
 	// errors are allowed to occur
 	if err != nil {
-		return authorizer.DecisionDeny, "", err
+		return authorizationDecision, "", err
 	}
 
 	denyReason, err := a.forbiddenMessageMaker.MakeMessage(attributes)
@@ -48,7 +48,7 @@ func (a *openshiftAuthorizer) Authorize(attributes authorizer.Attributes) (autho
 		denyReason += ": " + delegateReason
 	}
 
-	return authorizer.DecisionDeny, denyReason, nil
+	return authorizationDecision, denyReason, nil
 }
 
 // GetAllowedSubjects returns the subjects it knows can perform the action.

pkg/authorization/authorizer/browsersafe/authorizer_test.go

@@ -71,5 +71,5 @@ type recordingAuthorizer struct {
 
 func (t *recordingAuthorizer) Authorize(a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) {
 	t.attributes = a
-	return authorizer.DecisionDeny, "", nil
+	return authorizer.DecisionNoOpinion, "", nil
 }

pkg/authorization/authorizer/scope/authorizer.go

@@ -26,7 +26,7 @@ func NewAuthorizer(delegate authorizer.Authorizer, clusterRoleGetter rbaclisters
 func (a *scopeAuthorizer) Authorize(attributes authorizer.Attributes) (authorizer.Decision, string, error) {
 	user := attributes.GetUser()
 	if user == nil {
-		return authorizer.DecisionDeny, "", fmt.Errorf("user missing from context")
+		return authorizer.DecisionNoOpinion, "", fmt.Errorf("user missing from context")
 	}
 
 	scopes := user.GetExtra()[authorizationapi.ScopesKey]
@@ -52,5 +52,6 @@ func (a *scopeAuthorizer) Authorize(attributes authorizer.Attributes) (authorize
 		denyReason = err.Error()
 	}
 
+	// the scope prevent this.  We need to authoritatively deny
 	return authorizer.DecisionDeny, fmt.Sprintf("scopes %v prevent this action; %v", scopes, denyReason), kerrors.NewAggregate(nonFatalErrors)
 }

pkg/authorization/authorizer/scope/authorizer_test.go

@@ -28,7 +28,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 			},
-			expectedErr: `user missing from context`,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
+			expectedErr:     `user missing from context`,
 		},
 		{
 			name: "no extra",
@@ -37,7 +38,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 			},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 		{
 			name: "empty extra",
@@ -46,7 +48,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 			},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 		{
 			name: "empty scopes",
@@ -55,7 +58,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 			},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 		{
 			name: "bad scope",
@@ -64,8 +68,9 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 			},
-			expectedMsg: `scopes [does-not-exist] prevent this action; User "" cannot "" "" with name "" in project "ns"`,
-			expectedErr: `no scope evaluator found for "does-not-exist"`,
+			expectedAllowed: kauthorizer.DecisionDeny,
+			expectedMsg:     `scopes [does-not-exist] prevent this action; User "" cannot "" "" with name "" in project "ns"`,
+			expectedErr:     `no scope evaluator found for "does-not-exist"`,
 		},
 		{
 			name: "bad scope 2",
@@ -74,8 +79,9 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 			},
-			expectedMsg: `scopes [user:dne] prevent this action; User "" cannot "" "" with name "" in project "ns"`,
-			expectedErr: `unrecognized scope: user:dne`,
+			expectedAllowed: kauthorizer.DecisionDeny,
+			expectedMsg:     `scopes [user:dne] prevent this action; User "" cannot "" "" with name "" in project "ns"`,
+			expectedErr:     `unrecognized scope: user:dne`,
 		},
 		{
 			name: "scope doesn't cover",
@@ -84,7 +90,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 				Verb:            "get", Resource: "users", Name: "harold"},
-			expectedMsg: `scopes [user:info] prevent this action; User "" cannot get users in project "ns"`,
+			expectedAllowed: kauthorizer.DecisionDeny,
+			expectedMsg:     `scopes [user:info] prevent this action; User "" cannot get users in project "ns"`,
 		},
 		{
 			name: "scope covers",
@@ -93,7 +100,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 				Verb:            "get", Resource: "users", Name: "~"},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 		{
 			name: "scope covers for discovery",
@@ -102,7 +110,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: false,
 				Namespace:       "ns",
 				Verb:            "get", Path: "/api"},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 		{
 			name: "user:full covers any resource",
@@ -111,7 +120,8 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: true,
 				Namespace:       "ns",
 				Verb:            "update", Resource: "users", Name: "harold"},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 		{
 			name: "user:full covers any non-resource",
@@ -120,35 +130,38 @@ func TestAuthorize(t *testing.T) {
 				ResourceRequest: false,
 				Namespace:       "ns",
 				Verb:            "post", Path: "/foo/bar/baz"},
-			expectedCalled: true,
+			expectedCalled:  true,
+			expectedAllowed: kauthorizer.DecisionNoOpinion,
 		},
 	}
 
 	for _, tc := range testCases {
-		delegate := &fakeAuthorizer{allowed: tc.delegateAuthAllowed}
-		authorizer := NewAuthorizer(delegate, nil, defaultauthorizer.NewForbiddenMessageResolver(""))
+		t.Run(tc.name, func(t *testing.T) {
+			delegate := &fakeAuthorizer{allowed: tc.delegateAuthAllowed}
+			authorizer := NewAuthorizer(delegate, nil, defaultauthorizer.NewForbiddenMessageResolver(""))
 
-		actualAllowed, actualMsg, actualErr := authorizer.Authorize(tc.attributes)
-		switch {
-		case len(tc.expectedErr) == 0 && actualErr == nil:
-		case len(tc.expectedErr) == 0 && actualErr != nil:
-			t.Errorf("%s: unexpected error: %v", tc.name, actualErr)
-		case len(tc.expectedErr) != 0 && actualErr == nil:
-			t.Errorf("%s: missing error: %v", tc.name, tc.expectedErr)
-		case len(tc.expectedErr) != 0 && actualErr != nil:
-			if !strings.Contains(actualErr.Error(), tc.expectedErr) {
-				t.Errorf("%s: expected %v, got %v", tc.name, tc.expectedErr, actualErr)
+			actualAllowed, actualMsg, actualErr := authorizer.Authorize(tc.attributes)
+			switch {
+			case len(tc.expectedErr) == 0 && actualErr == nil:
+			case len(tc.expectedErr) == 0 && actualErr != nil:
+				t.Errorf("%s: unexpected error: %v", tc.name, actualErr)
+			case len(tc.expectedErr) != 0 && actualErr == nil:
+				t.Errorf("%s: missing error: %v", tc.name, tc.expectedErr)
+			case len(tc.expectedErr) != 0 && actualErr != nil:
+				if !strings.Contains(actualErr.Error(), tc.expectedErr) {
+					t.Errorf("expected %v, got %v", tc.expectedErr, actualErr)
+				}
 			}
-		}
-		if tc.expectedMsg != actualMsg {
-			t.Errorf("%s: expected %v, got %v", tc.name, tc.expectedMsg, actualMsg)
-		}
-		if tc.expectedAllowed != actualAllowed {
-			t.Errorf("%s: expected %v, got %v", tc.name, tc.expectedAllowed, actualAllowed)
-		}
-		if tc.expectedCalled != delegate.called {
-			t.Errorf("%s: expected %v, got %v", tc.name, tc.expectedCalled, delegate.called)
-		}
+			if tc.expectedMsg != actualMsg {
+				t.Errorf("expected %v, got %v", tc.expectedMsg, actualMsg)
+			}
+			if tc.expectedAllowed != actualAllowed {
+				t.Errorf("expected %v, got %v", tc.expectedAllowed, actualAllowed)
+			}
+			if tc.expectedCalled != delegate.called {
+				t.Errorf("expected %v, got %v", tc.expectedCalled, delegate.called)
+			}
+		})
 	}
 }
 
@@ -162,7 +175,7 @@ func (a *fakeAuthorizer) Authorize(passedAttributes kauthorizer.Attributes) (kau
 	if a.allowed {
 		return kauthorizer.DecisionAllow, "", nil
 	}
-	return kauthorizer.DecisionDeny, "", nil
+	return kauthorizer.DecisionNoOpinion, "", nil
 }
 
 func (a *fakeAuthorizer) GetAllowedSubjects(attributes kauthorizer.Attributes) (sets.String, sets.String, error) {

pkg/authorization/registry/localresourceaccessreview/rest_test.go

@@ -36,7 +36,7 @@ func (a *testAuthorizer) Authorize(attributes kauthorizer.Attributes) (decision
 		return kauthorizer.DecisionAllow, "", nil
 	}
 
-	return kauthorizer.DecisionDeny, "", errors.New("Unsupported")
+	return kauthorizer.DecisionNoOpinion, "", errors.New("Unsupported")
 }
 func (a *testAuthorizer) GetAllowedSubjects(passedAttributes kauthorizer.Attributes) (sets.String, sets.String, error) {
 	a.actualAttributes = passedAttributes

pkg/authorization/registry/localsubjectaccessreview/rest_test.go

@@ -54,7 +54,7 @@ func (a *testAuthorizer) GetAllowedSubjects(passedAttributes kauthorizer.Attribu
 func TestNoNamespace(t *testing.T) {
 	test := &subjectAccessTest{
 		authorizer: &testAuthorizer{
-			allowed: kauthorizer.DecisionDeny,
+			allowed: kauthorizer.DecisionNoOpinion,
 		},
 		reviewRequest: &authorizationapi.LocalSubjectAccessReview{
 			Action: authorizationapi.Action{
@@ -73,7 +73,7 @@ func TestNoNamespace(t *testing.T) {
 
 func TestConflictingNamespace(t *testing.T) {
 	authorizer := &testAuthorizer{
-		allowed: kauthorizer.DecisionDeny,
+		allowed: kauthorizer.DecisionNoOpinion,
 	}
 	reviewRequest := &authorizationapi.LocalSubjectAccessReview{
 		Action: authorizationapi.Action{
@@ -99,7 +99,7 @@ func TestConflictingNamespace(t *testing.T) {
 func TestEmptyReturn(t *testing.T) {
 	test := &subjectAccessTest{
 		authorizer: &testAuthorizer{
-			allowed: kauthorizer.DecisionDeny,
+			allowed: kauthorizer.DecisionNoOpinion,
 			reason:  "because reasons",
 		},
 		reviewRequest: &authorizationapi.LocalSubjectAccessReview{

pkg/authorization/registry/resourceaccessreview/rest_test.go

@@ -35,13 +35,13 @@ func (a *testAuthorizer) Authorize(attributes kauthorizer.Attributes) (allowed k
 	// allow the initial check for "can I run this RAR at all"
 	if attributes.GetResource() == "localresourceaccessreviews" {
 		if len(a.deniedNamespaces) != 0 && a.deniedNamespaces.Has(attributes.GetNamespace()) {
-			return kauthorizer.DecisionDeny, "denied initial check", nil
+			return kauthorizer.DecisionNoOpinion, "no decision on initial check", nil
 		}
 
 		return kauthorizer.DecisionAllow, "", nil
 	}
 
-	return kauthorizer.DecisionDeny, "", errors.New("unsupported")
+	return kauthorizer.DecisionNoOpinion, "", errors.New("unsupported")
 }
 func (a *testAuthorizer) GetAllowedSubjects(passedAttributes kauthorizer.Attributes) (sets.String, sets.String, error) {
 	a.actualAttributes = passedAttributes
@@ -56,7 +56,7 @@ func TestDeniedNamespace(t *testing.T) {
 		authorizer: &testAuthorizer{
 			users:            sets.String{},
 			groups:           sets.String{},
-			err:              "denied initial check",
+			err:              "no decision on initial check",
 			deniedNamespaces: sets.NewString("foo"),
 		},
 		reviewRequest: &authorizationapi.ResourceAccessReview{

pkg/authorization/registry/subjectaccessreview/rest_test.go

@@ -39,7 +39,7 @@ func (a *testAuthorizer) Authorize(passedAttributes kauthorizer.Attributes) (all
 	// allow the initial check for "can I run this SAR at all"
 	if passedAttributes.GetResource() == "localsubjectaccessreviews" {
 		if len(a.deniedNamespaces) != 0 && a.deniedNamespaces.Has(passedAttributes.GetNamespace()) {
-			return kauthorizer.DecisionDeny, "denied initial check", nil
+			return kauthorizer.DecisionNoOpinion, "no decision on initial check", nil
 		}
 
 		return kauthorizer.DecisionAllow, "", nil
@@ -59,7 +59,7 @@ func (a *testAuthorizer) GetAllowedSubjects(passedAttributes kauthorizer.Attribu
 func TestDeniedNamespace(t *testing.T) {
 	test := &subjectAccessTest{
 		authorizer: &testAuthorizer{
-			allowed:          kauthorizer.DecisionDeny,
+			allowed:          kauthorizer.DecisionNoOpinion,
 			err:              "denied initial check",
 			deniedNamespaces: sets.NewString("foo"),
 		},
@@ -81,7 +81,7 @@ func TestDeniedNamespace(t *testing.T) {
 func TestEmptyReturn(t *testing.T) {
 	test := &subjectAccessTest{
 		authorizer: &testAuthorizer{
-			allowed: kauthorizer.DecisionDeny,
+			allowed: kauthorizer.DecisionNoOpinion,
 			reason:  "because reasons",
 		},
 		reviewRequest: &authorizationapi.SubjectAccessReview{

pkg/ingress/admission/ingress_admission_test.go

@@ -95,7 +95,7 @@ func TestAdmission(t *testing.T) {
 		},
 		{
 			admit:    false,
-			allow:    authorizer.DecisionDeny,
+			allow:    authorizer.DecisionNoOpinion,
 			config:   emptyConfig(),
 			op:       admission.Create,
 			newHost:  "foo.com",

pkg/scheduler/admission/podnodeconstraints/admission_test.go

@@ -425,14 +425,14 @@ func fakeAuthorizer(t *testing.T) authorizer.Authorizer {
 func (a *fakeTestAuthorizer) Authorize(attributes authorizer.Attributes) (authorizer.Decision, string, error) {
 	ui := attributes.GetUser()
 	if ui == nil {
-		return authorizer.DecisionDeny, "", fmt.Errorf("No valid UserInfo for Context")
+		return authorizer.DecisionNoOpinion, "", fmt.Errorf("No valid UserInfo for Context")
 	}
 	// User with pods/bindings. permission:
 	if ui.GetName() == "system:serviceaccount:openshift-infra:daemonset-controller" {
 		return authorizer.DecisionAllow, "", nil
 	}
 	// User without pods/bindings. permission:
-	return authorizer.DecisionDeny, "", nil
+	return authorizer.DecisionNoOpinion, "", nil
 }
 
 func reviewResponse(allowed bool, msg string) *authorizationapi.SubjectAccessReviewResponse {