modify the union authorizer to return on the first Approve or Deny and to continue on Unknown

staging/src/k8s.io/apiserver/pkg/authorization/union/union.go

@@ -14,6 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+// Package union implements an authorizer that combines multiple subauthorizer.
+// The union authorizer iterates over each subauthorizer and returns the first
+// decision that is either an Allow decision or a Deny decision. If a
+// subauthorizer returns a NoOpinion, then the union authorizer moves onto the
+// next authorizer or, if the subauthorizer was the last authorizer, returns
+// NoOpinion as the aggregate decision. I.e. union authorizer creates an
+// aggregate decision and supports short-circut allows and denies from
+// subauthorizers.
 package union
 
 import (
@@ -33,14 +41,14 @@ func New(authorizationHandlers ...authorizer.Authorizer) authorizer.Authorizer {
 }
 
 // Authorizes against a chain of authorizer.Authorizer objects and returns nil if successful and returns error if unsuccessful
-func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (bool, string, error) {
+func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) {
 	var (
 		errlist    []error
 		reasonlist []string
 	)
 
 	for _, currAuthzHandler := range authzHandler {
-		authorized, reason, err := currAuthzHandler.Authorize(a)
+		decision, reason, err := currAuthzHandler.Authorize(a)
 
 		if err != nil {
 			errlist = append(errlist, err)
@@ -48,13 +56,15 @@ func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (bool,
 		if len(reason) != 0 {
 			reasonlist = append(reasonlist, reason)
 		}
-		if !authorized {
-			continue
+		switch decision {
+		case authorizer.DecisionAllow, authorizer.DecisionDeny:
+			return decision, reason, err
+		case authorizer.DecisionNoOpinion:
+			// continue to the next authorizer
 		}
-		return true, reason, nil
 	}
 
-	return false, strings.Join(reasonlist, "\n"), utilerrors.NewAggregate(errlist)
+	return authorizer.DecisionNoOpinion, strings.Join(reasonlist, "\n"), utilerrors.NewAggregate(errlist)
 }
 
 // unionAuthzRulesHandler authorizer against a chain of authorizer.RuleResolver

staging/src/k8s.io/apiserver/pkg/authorization/union/union_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package union
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
@@ -26,49 +27,43 @@ import (
 )
 
 type mockAuthzHandler struct {
-	isAuthorized bool
-	err          error
+	decision authorizer.Decision
+	err      error
 }
 
-func (mock *mockAuthzHandler) Authorize(a authorizer.Attributes) (bool, string, error) {
-	if mock.err != nil {
-		return false, "", mock.err
-	}
-	if !mock.isAuthorized {
-		return false, "", nil
-	}
-	return true, "", nil
+func (mock *mockAuthzHandler) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) {
+	return mock.decision, "", mock.err
 }
 
 func TestAuthorizationSecondPasses(t *testing.T) {
-	handler1 := &mockAuthzHandler{isAuthorized: false}
-	handler2 := &mockAuthzHandler{isAuthorized: true}
+	handler1 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}
+	handler2 := &mockAuthzHandler{decision: authorizer.DecisionAllow}
 	authzHandler := New(handler1, handler2)
 
 	authorized, _, _ := authzHandler.Authorize(nil)
-	if !authorized {
+	if authorized != authorizer.DecisionAllow {
 		t.Errorf("Unexpected authorization failure")
 	}
 }
 
 func TestAuthorizationFirstPasses(t *testing.T) {
-	handler1 := &mockAuthzHandler{isAuthorized: true}
-	handler2 := &mockAuthzHandler{isAuthorized: false}
+	handler1 := &mockAuthzHandler{decision: authorizer.DecisionAllow}
+	handler2 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}
 	authzHandler := New(handler1, handler2)
 
 	authorized, _, _ := authzHandler.Authorize(nil)
-	if !authorized {
+	if authorized != authorizer.DecisionAllow {
 		t.Errorf("Unexpected authorization failure")
 	}
 }
 
 func TestAuthorizationNonePasses(t *testing.T) {
-	handler1 := &mockAuthzHandler{isAuthorized: false}
-	handler2 := &mockAuthzHandler{isAuthorized: false}
+	handler1 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}
+	handler2 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}
 	authzHandler := New(handler1, handler2)
 
 	authorized, _, _ := authzHandler.Authorize(nil)
-	if authorized {
+	if authorized == authorizer.DecisionAllow {
 		t.Errorf("Expected failed authorization")
 	}
 }
@@ -223,3 +218,49 @@ func getNonResourceRules(infos []authorizer.NonResourceRuleInfo) []authorizer.De
 	}
 	return rules
 }
+
+func TestAuthorizationUnequivocalDeny(t *testing.T) {
+	cs := []struct {
+		authorizers []authorizer.Authorizer
+		decision    authorizer.Decision
+	}{
+		{
+			authorizers: []authorizer.Authorizer{},
+			decision:    authorizer.DecisionNoOpinion,
+		},
+		{
+			authorizers: []authorizer.Authorizer{
+				&mockAuthzHandler{decision: authorizer.DecisionNoOpinion},
+				&mockAuthzHandler{decision: authorizer.DecisionAllow},
+				&mockAuthzHandler{decision: authorizer.DecisionDeny},
+			},
+			decision: authorizer.DecisionAllow,
+		},
+		{
+			authorizers: []authorizer.Authorizer{
+				&mockAuthzHandler{decision: authorizer.DecisionNoOpinion},
+				&mockAuthzHandler{decision: authorizer.DecisionDeny},
+				&mockAuthzHandler{decision: authorizer.DecisionAllow},
+			},
+			decision: authorizer.DecisionDeny,
+		},
+		{
+			authorizers: []authorizer.Authorizer{
+				&mockAuthzHandler{decision: authorizer.DecisionNoOpinion},
+				&mockAuthzHandler{decision: authorizer.DecisionDeny, err: errors.New("webhook failed closed")},
+				&mockAuthzHandler{decision: authorizer.DecisionAllow},
+			},
+			decision: authorizer.DecisionDeny,
+		},
+	}
+	for i, c := range cs {
+		t.Run(fmt.Sprintf("case %v", i), func(t *testing.T) {
+			authzHandler := New(c.authorizers...)
+
+			decision, _, _ := authzHandler.Authorize(nil)
+			if decision != c.decision {
+				t.Errorf("Unexpected authorization failure: %v, expected: %v", decision, c.decision)
+			}
+		})
+	}
+}