Merge pull request #2319 from deads2k/forbidden-message

Merged by openshift-bot

Author: OpenShift Bot

pkg/authorization/authorizer/authorizer.go

@@ -12,11 +12,12 @@ import (
 )
 
 type openshiftAuthorizer struct {
-	ruleResolver rulevalidation.AuthorizationRuleResolver
+	ruleResolver          rulevalidation.AuthorizationRuleResolver
+	forbiddenMessageMaker ForbiddenMessageMaker
 }
 
-func NewAuthorizer(ruleResolver rulevalidation.AuthorizationRuleResolver) Authorizer {
-	return &openshiftAuthorizer{ruleResolver}
+func NewAuthorizer(ruleResolver rulevalidation.AuthorizationRuleResolver, forbiddenMessageMaker ForbiddenMessageMaker) Authorizer {
+	return &openshiftAuthorizer{ruleResolver, forbiddenMessageMaker}
 }
 
 func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, passedAttributes AuthorizationAttributes) (bool, string, error) {
@@ -51,21 +52,10 @@ func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, passedAttributes Autho
 		return false, "", kerrors.NewAggregate(errs)
 	}
 
-	username := "MISSING"
-	if user, userExists := kapi.UserFrom(ctx); userExists {
-		username = user.GetName()
-	}
-
-	denyReason := "denied by default"
-	if passedAttributes.IsNonResourceURL() {
-		denyReason = fmt.Sprintf("%v cannot %v on %v", username, attributes.GetVerb(), attributes.GetURL())
-
-	} else {
-		resourceNamePart := ""
-		if len(attributes.GetResourceName()) > 0 {
-			resourceNamePart = fmt.Sprintf(" with name \"%v\"", attributes.GetResourceName())
-		}
-		denyReason = fmt.Sprintf("%v cannot %v on %v%v in %v", username, attributes.GetVerb(), attributes.GetResource(), resourceNamePart, namespace)
+	user, _ := kapi.UserFrom(ctx)
+	denyReason, err := a.forbiddenMessageMaker.MakeMessage(MessageContext{user, namespace, attributes})
+	if err != nil {
+		denyReason = err.Error()
 	}
 
 	return false, denyReason, nil

pkg/authorization/authorizer/authorizer_test.go

@@ -44,7 +44,7 @@ func TestResourceNameDeny(t *testing.T) {
 			ResourceName: "just-a-user",
 		},
 		expectedAllowed: false,
-		expectedReason:  `just-a-user cannot get on users with name "just-a-user"`,
+		expectedReason:  `User "just-a-user" cannot get users`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.clusterBindings = newDefaultClusterPolicyBindings()
@@ -165,7 +165,7 @@ func TestNonResourceDeny(t *testing.T) {
 			URL:            "not-allowed",
 		},
 		expectedAllowed: false,
-		expectedReason:  `no-one cannot get on not-allowed`,
+		expectedReason:  `User "no-one" cannot "get" on "not-allowed"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.clusterBindings = newDefaultClusterPolicyBindings()
@@ -182,7 +182,7 @@ func TestHealthDeny(t *testing.T) {
 			URL:            "/healthz",
 		},
 		expectedAllowed: false,
-		expectedReason:  `no-one cannot get on /healthz`,
+		expectedReason:  `User "no-one" cannot "get" on "/healthz"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.clusterBindings = newDefaultClusterPolicyBindings()
@@ -214,7 +214,7 @@ func TestDisallowedViewingGlobalPods(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  `SomeYahoo cannot get on pods`,
+		expectedReason:  `User "SomeYahoo" cannot get pods`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.clusterBindings = newDefaultClusterPolicyBindings()
@@ -281,7 +281,7 @@ func TestResourceRestrictionsWork(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  `Rachel cannot get on pods in adze`,
+		expectedReason:  `User "Rachel" cannot get pods in project "adze"`,
 	}
 	test2.clusterPolicies = newDefaultClusterPolicies()
 	test2.policies = newAdzePolicies()
@@ -330,7 +330,7 @@ func TestLocalRightsDoNotGrantGlobalRights(t *testing.T) {
 			Resource: "buildConfigs",
 		},
 		expectedAllowed: false,
-		expectedReason:  `Rachel cannot get on buildConfigs in backsaw`,
+		expectedReason:  `User "Rachel" cannot get buildConfigs in project "backsaw"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = append(test.policies, newAdzePolicies()...)
@@ -363,7 +363,7 @@ func TestVerbRestrictionsWork(t *testing.T) {
 			Resource: "buildConfigs",
 		},
 		expectedAllowed: false,
-		expectedReason:  `Valerie cannot create on buildConfigs in adze`,
+		expectedReason:  `User "Valerie" cannot create buildConfigs in project "adze"`,
 	}
 	test2.clusterPolicies = newDefaultClusterPolicies()
 	test2.policies = newAdzePolicies()
@@ -377,7 +377,7 @@ func (test *authorizeTest) test(t *testing.T) {
 	policyBindingRegistry := testpolicyregistry.NewPolicyBindingRegistry(test.bindings, test.bindingRetrievalError)
 	clusterPolicyRegistry := clusterpolicyregistry.NewSimulatedRegistry(testpolicyregistry.NewClusterPolicyRegistry(test.clusterPolicies, test.policyRetrievalError))
 	clusterPolicyBindingRegistry := clusterpolicybindingregistry.NewSimulatedRegistry(testpolicyregistry.NewClusterPolicyBindingRegistry(test.clusterBindings, test.bindingRetrievalError))
-	authorizer := NewAuthorizer(rulevalidation.NewDefaultRuleResolver(policyRegistry, policyBindingRegistry, clusterPolicyRegistry, clusterPolicyBindingRegistry))
+	authorizer := NewAuthorizer(rulevalidation.NewDefaultRuleResolver(policyRegistry, policyBindingRegistry, clusterPolicyRegistry, clusterPolicyBindingRegistry), NewForbiddenMessageResolver(""))
 
 	actualAllowed, actualReason, actualError := authorizer.Authorize(test.context, *test.attributes)
 

pkg/authorization/authorizer/bootstrap_policy_test.go

@@ -74,7 +74,7 @@ func TestViewerGetAllowedKindInAdze(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Victor cannot get on pods in adze",
+		expectedReason:  `User "Victor" cannot get pods in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -94,7 +94,7 @@ func TestViewerGetDisallowedKindInMallet(t *testing.T) {
 			Resource: "policies",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Victor cannot get on policies in mallet",
+		expectedReason:  `User "Victor" cannot get policies in project "mallet"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -113,7 +113,7 @@ func TestViewerGetDisallowedKindInAdze(t *testing.T) {
 			Resource: "policies",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Victor cannot get on policies in adze",
+		expectedReason:  `User "Victor" cannot get policies in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -133,7 +133,7 @@ func TestViewerCreateAllowedKindInMallet(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Victor cannot create on pods in mallet",
+		expectedReason:  `User "Victor" cannot create pods in project "mallet"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -152,7 +152,7 @@ func TestViewerCreateAllowedKindInAdze(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Victor cannot create on pods in adze",
+		expectedReason:  `User "Victor" cannot create pods in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -191,7 +191,7 @@ func TestEditorUpdateAllowedKindInAdze(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Edgar cannot update on pods in adze",
+		expectedReason:  `User "Edgar" cannot update pods in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -211,7 +211,7 @@ func TestEditorUpdateDisallowedKindInMallet(t *testing.T) {
 			Resource: "roleBindings",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Edgar cannot update on roleBindings in mallet",
+		expectedReason:  `User "Edgar" cannot update roleBindings in project "mallet"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -230,7 +230,7 @@ func TestEditorUpdateDisallowedKindInAdze(t *testing.T) {
 			Resource: "roleBindings",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Edgar cannot update on roleBindings in adze",
+		expectedReason:  `User "Edgar" cannot update roleBindings in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -269,7 +269,7 @@ func TestEditorGetAllowedKindInAdze(t *testing.T) {
 			Resource: "pods",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Edgar cannot get on pods in adze",
+		expectedReason:  `User "Edgar" cannot get pods in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -308,7 +308,7 @@ func TestAdminUpdateAllowedKindInAdze(t *testing.T) {
 			Resource: "roleBindings",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Matthew cannot update on roleBindings in adze",
+		expectedReason:  `User "Matthew" cannot update roleBindings in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -328,7 +328,7 @@ func TestAdminUpdateStatusInMallet(t *testing.T) {
 			Resource: "pods/status",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Matthew cannot update on pods/status in mallet",
+		expectedReason:  `User "Matthew" cannot update pods/status in project "mallet"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -367,7 +367,7 @@ func TestAdminUpdateDisallowedKindInMallet(t *testing.T) {
 			Resource: "policies",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Matthew cannot update on policies in mallet",
+		expectedReason:  `User "Matthew" cannot update policies in project "mallet"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -386,7 +386,7 @@ func TestAdminUpdateDisallowedKindInAdze(t *testing.T) {
 			Resource: "roles",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Matthew cannot update on roles in adze",
+		expectedReason:  `User "Matthew" cannot update roles in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()
@@ -425,7 +425,7 @@ func TestAdminGetAllowedKindInAdze(t *testing.T) {
 			Resource: "policies",
 		},
 		expectedAllowed: false,
-		expectedReason:  "Matthew cannot get on policies in adze",
+		expectedReason:  `User "Matthew" cannot get policies in project "adze"`,
 	}
 	test.clusterPolicies = newDefaultClusterPolicies()
 	test.policies = newAdzePolicies()

pkg/authorization/authorizer/interfaces.go

@@ -4,6 +4,7 @@ import (
 	"net/http"
 
 	kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/auth/user"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
@@ -29,3 +30,15 @@ type AuthorizationAttributes interface {
 	// GetURL returns the URL path being requested, including the leading '/'
 	GetURL() string
 }
+
+// ForbiddenMessageMaker creates a forbidden message from a MessageContext
+type ForbiddenMessageMaker interface {
+	MakeMessage(ctx MessageContext) (string, error)
+}
+
+// MessageContext contains sufficient information to create a forbidden message.  It is bundled in this one object to make it easy and obvious how to build a golang template
+type MessageContext struct {
+	User       user.Info
+	Namespace  string
+	Attributes AuthorizationAttributes
+}

pkg/authorization/authorizer/messages.go

@@ -0,0 +1,118 @@
+package authorizer
+
+import (
+	"bytes"
+	"text/template"
+
+	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
+)
+
+const DefaultProjectRequestForbidden = "You may not request a new project via this API."
+
+type ForbiddenMessageResolver struct {
+	// TODO if these maps were map[string]map[util.StringSet]ForbiddenMessageMaker, we'd be able to handle cases where sets of resources wanted slightly different messages
+	// unfortunately, maps don't support keys like that, requiring StringSet serialization and deserialization.
+	namespacedVerbsToResourcesToForbiddenMessageMaker map[string]map[string]ForbiddenMessageMaker
+	rootScopedVerbsToResourcesToForbiddenMessageMaker map[string]map[string]ForbiddenMessageMaker
+
+	nonResourceURLForbiddenMessageMaker ForbiddenMessageMaker
+	defaultForbiddenMessageMaker        ForbiddenMessageMaker
+}
+
+func NewForbiddenMessageResolver(projectRequestForbiddenTemplate string) *ForbiddenMessageResolver {
+	messageResolver := &ForbiddenMessageResolver{
+		namespacedVerbsToResourcesToForbiddenMessageMaker: map[string]map[string]ForbiddenMessageMaker{},
+		rootScopedVerbsToResourcesToForbiddenMessageMaker: map[string]map[string]ForbiddenMessageMaker{},
+		nonResourceURLForbiddenMessageMaker:               newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot "{{.Attributes.GetVerb}}" on "{{.Attributes.GetURL}}"`),
+		defaultForbiddenMessageMaker:                      newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot "{{.Attributes.GetVerb}}" "{{.Attributes.GetResource}}" with name "{{.Attributes.GetResourceName}}" in project "{{.Namespace}}"`),
+	}
+
+	// general messages
+	messageResolver.addNamespacedForbiddenMessageMaker("create", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot create {{.Attributes.GetResource}} in project "{{.Namespace}}"`))
+	messageResolver.addRootScopedForbiddenMessageMaker("create", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot create {{.Attributes.GetResource}} at the cluster scope`))
+	messageResolver.addNamespacedForbiddenMessageMaker("get", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot get {{.Attributes.GetResource}} in project "{{.Namespace}}"`))
+	messageResolver.addRootScopedForbiddenMessageMaker("get", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot get {{.Attributes.GetResource}} at the cluster scope`))
+	messageResolver.addNamespacedForbiddenMessageMaker("list", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot list {{.Attributes.GetResource}} in project "{{.Namespace}}"`))
+	messageResolver.addRootScopedForbiddenMessageMaker("list", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot list all {{.Attributes.GetResource}} in the cluster`))
+	messageResolver.addNamespacedForbiddenMessageMaker("update", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot update {{.Attributes.GetResource}} in project "{{.Namespace}}"`))
+	messageResolver.addRootScopedForbiddenMessageMaker("update", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot update {{.Attributes.GetResource}} at the cluster scope`))
+	messageResolver.addNamespacedForbiddenMessageMaker("delete", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot delete {{.Attributes.GetResource}} in project "{{.Namespace}}"`))
+	messageResolver.addRootScopedForbiddenMessageMaker("delete", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot delete {{.Attributes.GetResource}} at the cluster scope`))
+
+	// project request rejection
+	projectRequestDeny := projectRequestForbiddenTemplate
+	if len(projectRequestDeny) == 0 {
+		projectRequestDeny = DefaultProjectRequestForbidden
+	}
+	messageResolver.addRootScopedForbiddenMessageMaker("create", "projectrequests", newTemplateForbiddenMessageMaker(projectRequestDeny))
+
+	return messageResolver
+}
+
+func (m *ForbiddenMessageResolver) addNamespacedForbiddenMessageMaker(verb, resource string, messageMaker ForbiddenMessageMaker) {
+	m.addForbiddenMessageMaker(m.namespacedVerbsToResourcesToForbiddenMessageMaker, verb, resource, messageMaker)
+}
+
+func (m *ForbiddenMessageResolver) addRootScopedForbiddenMessageMaker(verb, resource string, messageMaker ForbiddenMessageMaker) {
+	m.addForbiddenMessageMaker(m.rootScopedVerbsToResourcesToForbiddenMessageMaker, verb, resource, messageMaker)
+}
+
+func (m *ForbiddenMessageResolver) addForbiddenMessageMaker(target map[string]map[string]ForbiddenMessageMaker, verb, resource string, messageMaker ForbiddenMessageMaker) {
+	resourcesToForbiddenMessageMaker, exists := target[verb]
+	if !exists {
+		resourcesToForbiddenMessageMaker = map[string]ForbiddenMessageMaker{}
+		target[verb] = resourcesToForbiddenMessageMaker
+	}
+
+	resourcesToForbiddenMessageMaker[resource] = messageMaker
+}
+
+func (m *ForbiddenMessageResolver) MakeMessage(ctx MessageContext) (string, error) {
+	if ctx.Attributes.IsNonResourceURL() {
+		return m.nonResourceURLForbiddenMessageMaker.MakeMessage(ctx)
+	}
+
+	messageMakerMap := m.namespacedVerbsToResourcesToForbiddenMessageMaker
+	if len(ctx.Namespace) == 0 {
+		messageMakerMap = m.rootScopedVerbsToResourcesToForbiddenMessageMaker
+	}
+
+	resourcesToForbiddenMessageMaker, exists := messageMakerMap[ctx.Attributes.GetVerb()]
+	if !exists {
+		resourcesToForbiddenMessageMaker, exists = messageMakerMap[authorizationapi.VerbAll]
+		if !exists {
+			return m.defaultForbiddenMessageMaker.MakeMessage(ctx)
+		}
+	}
+
+	messageMaker, exists := resourcesToForbiddenMessageMaker[ctx.Attributes.GetResource()]
+	if !exists {
+		messageMaker, exists = resourcesToForbiddenMessageMaker[authorizationapi.ResourceAll]
+		if !exists {
+			return m.defaultForbiddenMessageMaker.MakeMessage(ctx)
+		}
+	}
+
+	specificMessage, err := messageMaker.MakeMessage(ctx)
+	if err != nil {
+		return m.defaultForbiddenMessageMaker.MakeMessage(ctx)
+	}
+
+	return specificMessage, nil
+}
+
+type templateForbiddenMessageMaker struct {
+	parsedTemplate *template.Template
+}
+
+func newTemplateForbiddenMessageMaker(text string) templateForbiddenMessageMaker {
+	parsedTemplate := template.Must(template.New("").Parse(text))
+
+	return templateForbiddenMessageMaker{parsedTemplate}
+}
+
+func (m templateForbiddenMessageMaker) MakeMessage(ctx MessageContext) (string, error) {
+	buffer := &bytes.Buffer{}
+	err := m.parsedTemplate.Execute(buffer, ctx)
+	return string(buffer.Bytes()), err
+}