From 4e0a3b557e235ad354f6dfb4567d2a20ebde403a Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <jliggitt@redhat.com>
Date: Thu, 9 Aug 2018 15:51:52 -0400
Subject: [PATCH] Make the request attributes clearer in forbidden messages

---
 .../endpoints/handlers/responsewriters/errors.go   |  7 ++-----
 .../handlers/responsewriters/errors_test.go        | 14 +++++++-------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go
index 007efe9d884..eac752573ca 100644
--- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go
@@ -67,18 +67,15 @@ func forbiddenMessage(attributes authorizer.Attributes) string {
 	}
 
 	resource := attributes.GetResource()
-	if group := attributes.GetAPIGroup(); len(group) > 0 {
-		resource = resource + "." + group
-	}
 	if subresource := attributes.GetSubresource(); len(subresource) > 0 {
 		resource = resource + "/" + subresource
 	}
 
 	if ns := attributes.GetNamespace(); len(ns) > 0 {
-		return fmt.Sprintf("User %q cannot %s %s in the namespace %q", username, attributes.GetVerb(), resource, ns)
+		return fmt.Sprintf("User %q cannot %s resource %q in API group %q in the namespace %q", username, attributes.GetVerb(), resource, attributes.GetAPIGroup(), ns)
 	}
 
-	return fmt.Sprintf("User %q cannot %s %s at the cluster scope", username, attributes.GetVerb(), resource)
+	return fmt.Sprintf("User %q cannot %s resource %q in API group %q at the cluster scope", username, attributes.GetVerb(), resource, attributes.GetAPIGroup())
 }
 
 // InternalError renders a simple internal error
diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go
index 0dcf7adf97f..846e76638dc 100644
--- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go
@@ -72,12 +72,12 @@ func TestForbidden(t *testing.T) {
 `, authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/whatever"}, "", "application/json"},
 		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"forbidden: User \"NAME\" cannot GET path \"/\u0026lt;script\u0026gt;\"","reason":"Forbidden","details":{},"code":403}
 `, authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/<script>"}, "", "application/json"},
-		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pod is forbidden: User \"NAME\" cannot GET pod at the cluster scope","reason":"Forbidden","details":{"kind":"pod"},"code":403}
-`, authorizer.AttributesRecord{User: u, Verb: "GET", Resource: "pod", ResourceRequest: true}, "", "application/json"},
-		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pod \"mypod\" is forbidden: User \"NAME\" cannot GET pod at the cluster scope","reason":"Forbidden","details":{"name":"mypod","kind":"pod"},"code":403}
-`, authorizer.AttributesRecord{User: u, Verb: "GET", Resource: "pod", ResourceRequest: true, Name: "mypod"}, "", "application/json"},
-		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pod.v2 is forbidden: User \"NAME\" cannot GET pod.v2/quota in the namespace \"test\"","reason":"Forbidden","details":{"group":"v2","kind":"pod"},"code":403}
-`, authorizer.AttributesRecord{User: u, Verb: "GET", Namespace: "test", APIGroup: "v2", Resource: "pod", Subresource: "quota", ResourceRequest: true}, "", "application/json"},
+		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pod is forbidden: User \"NAME\" cannot get resource \"pod\" in API group \"\" at the cluster scope","reason":"Forbidden","details":{"kind":"pod"},"code":403}
+`, authorizer.AttributesRecord{User: u, Verb: "get", Resource: "pod", ResourceRequest: true}, "", "application/json"},
+		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pod \"mypod\" is forbidden: User \"NAME\" cannot get resource \"pod\" in API group \"\" at the cluster scope","reason":"Forbidden","details":{"name":"mypod","kind":"pod"},"code":403}
+`, authorizer.AttributesRecord{User: u, Verb: "get", Resource: "pod", ResourceRequest: true, Name: "mypod"}, "", "application/json"},
+		{`{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pod.v2 is forbidden: User \"NAME\" cannot get resource \"pod/quota\" in API group \"v2\" in the namespace \"test\"","reason":"Forbidden","details":{"group":"v2","kind":"pod"},"code":403}
+`, authorizer.AttributesRecord{User: u, Verb: "get", Namespace: "test", APIGroup: "v2", Resource: "pod", Subresource: "quota", ResourceRequest: true}, "", "application/json"},
 	}
 	for _, test := range cases {
 		observer := httptest.NewRecorder()
@@ -86,7 +86,7 @@ func TestForbidden(t *testing.T) {
 		Forbidden(request.NewDefaultContext(), test.attributes, observer, &http.Request{}, test.reason, negotiatedSerializer)
 		result := string(observer.Body.Bytes())
 		if result != test.expected {
-			t.Errorf("Forbidden response body(%#v...) != %#v, got %#v", test.attributes, test.expected, result)
+			t.Errorf("Forbidden response body(%#v...)\n expected: %v\ngot:       %v", test.attributes, test.expected, result)
 		}
 		resultType := observer.HeaderMap.Get("Content-Type")
 		if resultType != test.contentType {