switch to using interface for authorization matches

Author: deads2k

pkg/authorization/authorizer/attributes.go

@@ -21,7 +21,7 @@ type DefaultAuthorizationAttributes struct {
 
 // ToDefaultAuthorizationAttributes coerces Action to DefaultAuthorizationAttributes.  Namespace is not included
 // because the authorizer takes that information on the context
-func ToDefaultAuthorizationAttributes(in authorizationapi.Action) DefaultAuthorizationAttributes {
+func ToDefaultAuthorizationAttributes(in authorizationapi.Action) Action {
 	return DefaultAuthorizationAttributes{
 		Verb:           in.Verb,
 		APIGroup:       in.Group,
@@ -33,10 +33,10 @@ func ToDefaultAuthorizationAttributes(in authorizationapi.Action) DefaultAuthori
 	}
 }
 
-func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.PolicyRule) (bool, error) {
+func RuleMatches(a Action, rule authorizationapi.PolicyRule) (bool, error) {
 	if a.IsNonResourceURL() {
-		if a.nonResourceMatches(rule) {
-			if a.verbMatches(rule.Verbs) {
+		if nonResourceMatches(a, rule) {
+			if verbMatches(a, rule.Verbs) {
 				return true, nil
 			}
 		}
@@ -50,12 +50,12 @@ func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.Policy
 		return false, nil
 	}
 
-	if a.verbMatches(rule.Verbs) {
-		if a.apiGroupMatches(rule.APIGroups) {
+	if verbMatches(a, rule.Verbs) {
+		if apiGroupMatches(a, rule.APIGroups) {
 
 			allowedResourceTypes := authorizationapi.NormalizeResources(rule.Resources)
-			if a.resourceMatches(allowedResourceTypes) {
-				if a.nameMatches(rule.ResourceNames) {
+			if resourceMatches(a, allowedResourceTypes) {
+				if nameMatches(a, rule.ResourceNames) {
 					return true, nil
 				}
 			}
@@ -65,7 +65,7 @@ func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.Policy
 	return false, nil
 }
 
-func (a DefaultAuthorizationAttributes) apiGroupMatches(allowedGroups []string) bool {
+func apiGroupMatches(a Action, allowedGroups []string) bool {
 	// allowedGroups is expected to be small, so I don't feel bad about this.
 	for _, allowedGroup := range allowedGroups {
 		if allowedGroup == authorizationapi.APIGroupAll {
@@ -80,32 +80,28 @@ func (a DefaultAuthorizationAttributes) apiGroupMatches(allowedGroups []string)
 	return false
 }
 
-func (a DefaultAuthorizationAttributes) verbMatches(verbs sets.String) bool {
+func verbMatches(a Action, verbs sets.String) bool {
 	return verbs.Has(authorizationapi.VerbAll) || verbs.Has(strings.ToLower(a.GetVerb()))
 }
 
-func (a DefaultAuthorizationAttributes) resourceMatches(allowedResourceTypes sets.String) bool {
+func resourceMatches(a Action, allowedResourceTypes sets.String) bool {
 	return allowedResourceTypes.Has(authorizationapi.ResourceAll) || allowedResourceTypes.Has(strings.ToLower(a.GetResource()))
 }
 
 // nameMatches checks to see if the resourceName of the action is in a the specified whitelist.  An empty whitelist indicates that any name is allowed.
 // An empty string in the whitelist should only match the action's resourceName if the resourceName itself is empty string.  This behavior allows for the
 // combination of a whitelist for gets in the same rule as a list that won't have a resourceName.  I don't recommend writing such a rule, but we do
 // handle it like you'd expect: white list is respected for gets while not preventing the list you explicitly asked for.
-func (a DefaultAuthorizationAttributes) nameMatches(allowedResourceNames sets.String) bool {
+func nameMatches(a Action, allowedResourceNames sets.String) bool {
 	if len(allowedResourceNames) == 0 {
 		return true
 	}
 
 	return allowedResourceNames.Has(a.GetResourceName())
 }
 
-func (a DefaultAuthorizationAttributes) GetVerb() string {
-	return a.Verb
-}
-
 // nonResourceMatches take the remainer of a URL and attempts to match it against a series of explicitly allowed steps that can end in a wildcard
-func (a DefaultAuthorizationAttributes) nonResourceMatches(rule authorizationapi.PolicyRule) bool {
+func nonResourceMatches(a Action, rule authorizationapi.PolicyRule) bool {
 	for allowedNonResourcePath := range rule.NonResourceURLs {
 		// if the allowed resource path ends in a wildcard, check to see if the URL starts with it
 		if strings.HasSuffix(allowedNonResourcePath, "*") {
@@ -135,6 +131,10 @@ func splitPath(thePath string) []string {
 // DefaultAuthorizationAttributes satisfies the Action interface
 var _ Action = DefaultAuthorizationAttributes{}
 
+func (a DefaultAuthorizationAttributes) GetVerb() string {
+	return a.Verb
+}
+
 func (a DefaultAuthorizationAttributes) GetAPIVersion() string {
 	return a.APIVersion
 }

pkg/authorization/authorizer/authorizer.go

@@ -20,9 +20,7 @@ func NewAuthorizer(ruleResolver rulevalidation.AuthorizationRuleResolver, forbid
 	return &openshiftAuthorizer{ruleResolver, forbiddenMessageMaker}
 }
 
-func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, passedAttributes Action) (bool, string, error) {
-	attributes := CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
+func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, attributes Action) (bool, string, error) {
 	user, ok := kapi.UserFrom(ctx)
 	if !ok {
 		return false, "", errors.New("no user available on context")
@@ -54,9 +52,7 @@ func (a *openshiftAuthorizer) GetAllowedSubjects(ctx kapi.Context, attributes Ac
 	return a.getAllowedSubjectsFromNamespaceBindings(namespace, attributes)
 }
 
-func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace string, passedAttributes Action) (sets.String, sets.String, error) {
-	attributes := CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
+func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace string, attributes Action) (sets.String, sets.String, error) {
 	var errs []error
 
 	roleBindings, err := a.ruleResolver.GetRoleBindings(namespace)
@@ -77,7 +73,7 @@ func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace
 		}
 
 		for _, rule := range role.Rules() {
-			matches, err := attributes.RuleMatches(rule)
+			matches, err := RuleMatches(attributes, rule)
 			if err != nil {
 				errs = append(errs, err)
 				continue
@@ -96,14 +92,12 @@ func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace
 // authorizeWithNamespaceRules returns isAllowed, reason, and error.  If an error is returned, isAllowed and reason are still valid.  This seems strange
 // but errors are not always fatal to the authorization process.  It is entirely possible to get an error and be able to continue determine authorization
 // status in spite of it.  This is most common when a bound role is missing, but enough roles are still present and bound to authorize the request.
-func (a *openshiftAuthorizer) authorizeWithNamespaceRules(user user.Info, namespace string, passedAttributes Action) (bool, string, error) {
-	attributes := CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
+func (a *openshiftAuthorizer) authorizeWithNamespaceRules(user user.Info, namespace string, attributes Action) (bool, string, error) {
 	allRules, ruleRetrievalError := a.ruleResolver.RulesFor(user, namespace)
 
 	var errs []error
 	for _, rule := range allRules {
-		matches, err := attributes.RuleMatches(rule)
+		matches, err := RuleMatches(attributes, rule)
 		if err != nil {
 			errs = append(errs, err)
 			continue
@@ -126,24 +120,6 @@ func (a *openshiftAuthorizer) authorizeWithNamespaceRules(user user.Info, namesp
 	return false, "", kerrors.NewAggregate(errs)
 }
 
-// TODO this may or may not be the behavior we want for managing rules.  As a for instance, a verb might be specified
-// that our attributes builder will never satisfy.  For now, I think gets us close.  Maybe a warning message of some kind?
-func CoerceToDefaultAuthorizationAttributes(passedAttributes Action) *DefaultAuthorizationAttributes {
-	attributes, ok := passedAttributes.(*DefaultAuthorizationAttributes)
-	if !ok {
-		attributes = &DefaultAuthorizationAttributes{
-			APIGroup:       passedAttributes.GetAPIGroup(),
-			Verb:           passedAttributes.GetVerb(),
-			Resource:       passedAttributes.GetResource(),
-			ResourceName:   passedAttributes.GetResourceName(),
-			NonResourceURL: passedAttributes.IsNonResourceURL(),
-			URL:            passedAttributes.GetURL(),
-		}
-	}
-
-	return attributes
-}
-
 func doesApplyToUser(ruleUsers, ruleGroups sets.String, user user.Info) bool {
 	if ruleUsers.Has(user.GetName()) {
 		return true

pkg/authorization/authorizer/non_resource_match_test.go

@@ -67,7 +67,7 @@ func (test *nonResourceMatchTest) run(t *testing.T) {
 
 	rule := authorizationapi.PolicyRule{NonResourceURLs: sets.NewString(test.matcher)}
 
-	result := attributes.nonResourceMatches(rule)
+	result := nonResourceMatches(attributes, rule)
 
 	if result != test.expectedResult {
 		t.Errorf("Expected %v, got %v", test.expectedResult, result)

pkg/authorization/authorizer/scope/authorizer.go

@@ -23,15 +23,15 @@ func NewAuthorizer(delegate defaultauthorizer.Authorizer, clusterPolicyGetter cl
 	return &scopeAuthorizer{delegate: delegate, clusterPolicyGetter: clusterPolicyGetter, forbiddenMessageMaker: forbiddenMessageMaker}
 }
 
-func (a *scopeAuthorizer) Authorize(ctx kapi.Context, passedAttributes defaultauthorizer.Action) (bool, string, error) {
+func (a *scopeAuthorizer) Authorize(ctx kapi.Context, attributes defaultauthorizer.Action) (bool, string, error) {
 	user, exists := kapi.UserFrom(ctx)
 	if !exists {
 		return false, "", fmt.Errorf("user missing from context")
 	}
 
 	scopes := user.GetExtra()[authorizationapi.ScopesKey]
 	if len(scopes) == 0 {
-		return a.delegate.Authorize(ctx, passedAttributes)
+		return a.delegate.Authorize(ctx, attributes)
 	}
 
 	nonFatalErrors := []error{}
@@ -43,17 +43,15 @@ func (a *scopeAuthorizer) Authorize(ctx kapi.Context, passedAttributes defaultau
 		nonFatalErrors = append(nonFatalErrors, err)
 	}
 
-	attributes := defaultauthorizer.CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
 	for _, rule := range rules {
 		// check rule against attributes
-		matches, err := attributes.RuleMatches(rule)
+		matches, err := defaultauthorizer.RuleMatches(attributes, rule)
 		if err != nil {
 			nonFatalErrors = append(nonFatalErrors, err)
 			continue
 		}
 		if matches {
-			return a.delegate.Authorize(ctx, passedAttributes)
+			return a.delegate.Authorize(ctx, attributes)
 		}
 	}
 

pkg/authorization/authorizer/scope/converter.go

@@ -357,7 +357,7 @@ func (e clusterRoleEvaluator) ResolveGettableNamespaces(scope string, clusterPol
 
 	errors := []error{}
 	for _, rule := range rules {
-		matches, err := attributes.RuleMatches(rule)
+		matches, err := authorizer.RuleMatches(attributes, rule)
 		if err != nil {
 			errors = append(errors, err)
 			continue