Parse forbidden errors to return forbidden actions (#1463)

cmd/kubeops/internal/handler/handler.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"encoding/json"
 	"net/http"
 	"strconv"
 
@@ -131,11 +132,37 @@ func AddRouteWith(
 	}
 }
 
+func returnForbiddenActions(forbiddenActions []auth.Action, w http.ResponseWriter) {
+	w.Header().Set("Content-Type", "application/json")
+	body, err := json.Marshal(forbiddenActions)
+	if err != nil {
+		returnErrMessage(err, w)
+		return
+	}
+	response.NewErrorResponse(http.StatusForbidden, string(body)).Write(w)
+}
+
+func returnErrMessage(err error, w http.ResponseWriter) {
+	code := handlerutil.ErrorCode(err)
+	errMessage := err.Error()
+	if code == http.StatusForbidden {
+		forbiddenActions := auth.ParseForbiddenActions(errMessage)
+		if len(forbiddenActions) > 0 {
+			returnForbiddenActions(forbiddenActions, w)
+		} else {
+			// Unable to parse forbidden actions, return the raw message
+			response.NewErrorResponse(code, errMessage).Write(w)
+		}
+	} else {
+		response.NewErrorResponse(code, errMessage).Write(w)
+	}
+}
+
 // ListReleases list existing releases.
 func ListReleases(cfg Config, w http.ResponseWriter, req *http.Request, params handlerutil.Params) {
 	apps, err := agent.ListReleases(cfg.ActionConfig, params[namespaceParam], cfg.Options.ListLimit, req.URL.Query().Get("statuses"))
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	response.NewDataResponse(apps).Write(w)
@@ -150,7 +177,7 @@ func ListAllReleases(cfg Config, w http.ResponseWriter, req *http.Request, _ han
 func CreateRelease(cfg Config, w http.ResponseWriter, req *http.Request, params handlerutil.Params) {
 	chartDetails, chartMulti, err := handlerutil.ParseAndGetChart(req, cfg.ChartClient, isV1SupportRequired)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	ch := chartMulti.Helm3Chart
@@ -159,7 +186,7 @@ func CreateRelease(cfg Config, w http.ResponseWriter, req *http.Request, params
 	valuesString := chartDetails.Values
 	release, err := agent.CreateRelease(cfg.ActionConfig, releaseName, namespace, valuesString, ch)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	response.NewDataResponse(release).Write(w)
@@ -183,18 +210,18 @@ func upgradeRelease(cfg Config, w http.ResponseWriter, req *http.Request, params
 	releaseName := params[nameParam]
 	chartDetails, chartMulti, err := handlerutil.ParseAndGetChart(req, cfg.ChartClient, isV1SupportRequired)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	ch := chartMulti.Helm3Chart
 	rel, err := agent.UpgradeRelease(cfg.ActionConfig, releaseName, chartDetails.Values, ch)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	compatRelease, err := helm3to2.Convert(*rel)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	response.NewDataResponse(compatRelease).Write(w)
@@ -209,17 +236,17 @@ func rollbackRelease(cfg Config, w http.ResponseWriter, req *http.Request, param
 	}
 	revisionInt, err := strconv.ParseInt(revision, 10, 32)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	rel, err := agent.RollbackRelease(cfg.ActionConfig, releaseName, int(revisionInt))
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	compatRelease, err := helm3to2.Convert(*rel)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	response.NewDataResponse(compatRelease).Write(w)
@@ -231,12 +258,12 @@ func GetRelease(cfg Config, w http.ResponseWriter, req *http.Request, params han
 	releaseName := params[nameParam]
 	release, err := agent.GetRelease(cfg.ActionConfig, releaseName)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	compatRelease, err := helm3to2.Convert(*release)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	response.NewDataResponse(compatRelease).Write(w)
@@ -251,7 +278,7 @@ func DeleteRelease(cfg Config, w http.ResponseWriter, req *http.Request, params
 	keepHistory := !purge
 	err := agent.DeleteRelease(cfg.ActionConfig, releaseName, keepHistory)
 	if err != nil {
-		response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w)
+		returnErrMessage(err, w)
 		return
 	}
 	w.Header().Set("Status-Code", "200")

cmd/kubeops/internal/handler/handler_test.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -31,13 +32,13 @@ var (
 
 // newConfigFixture returns a Config with fake clients
 // and memory storage.
-func newConfigFixture(t *testing.T) *Config {
+func newConfigFixture(t *testing.T, k *kubefake.FailingKubeClient) *Config {
 	t.Helper()
 
 	return &Config{
 		ActionConfig: &action.Configuration{
 			Releases:     storage.Init(driver.NewMemory()),
-			KubeClient:   &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}},
+			KubeClient:   k,
 			Capabilities: chartutil.DefaultCapabilities,
 			Log: func(format string, v ...interface{}) {
 				t.Helper()
@@ -81,6 +82,7 @@ func TestActions(t *testing.T) {
 		Description      string
 		ExistingReleases []*release.Release
 		Skip             bool //TODO: Remove this when the memory bug is fixed
+		KubeError        error
 		// Request params
 		RequestBody  string
 		RequestQuery string
@@ -244,6 +246,23 @@ func TestActions(t *testing.T) {
 			RemainingReleases: nil,
 			ResponseBody:      "",
 		},
+		{
+			// Scenario params
+			Description:      "Creates a release with missing permissions",
+			ExistingReleases: []*release.Release{},
+			KubeError:        errors.New(`Failed to create: secrets is forbidden: User "foo" cannot create resource "secrets" in API group "" in the namespace "default"`),
+			// Request params
+			RequestBody: `{"chartName": "foo", "releaseName": "foobar",	"version": "1.0.0"}`,
+			RequestQuery: "",
+			Action:       "create",
+			Params:       map[string]string{"namespace": "default"},
+			// Expected result
+			StatusCode: 403,
+			RemainingReleases: []*release.Release{
+				createRelease("foo", "foobar", "default", 1, release.StatusFailed),
+			},
+			ResponseBody: `{"code":403,"message":"[{\"apiGroup\":\"\",\"resource\":\"secrets\",\"namespace\":\"default\",\"clusterWide\":false,\"verbs\":[\"create\"]}]"}`,
+		},
 	}
 
 	for _, test := range tests {
@@ -256,7 +275,13 @@ func TestActions(t *testing.T) {
 			// Initialize environment for test
 			req := httptest.NewRequest("GET", fmt.Sprintf("http://foo.bar%s", test.RequestQuery), strings.NewReader(test.RequestBody))
 			response := httptest.NewRecorder()
-			cfg := newConfigFixture(t)
+			k := &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}}
+			if test.KubeError != nil {
+				k.CreateError = test.KubeError
+				k.UpdateError = test.KubeError
+				k.DeleteError = test.KubeError
+			}
+			cfg := newConfigFixture(t, k)
 			createExistingReleases(t, cfg, test.ExistingReleases)
 
 			// Perform request
@@ -403,7 +428,8 @@ func TestRollbackAction(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			cfg := newConfigFixture(t)
+			k := &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}}
+			cfg := newConfigFixture(t, k)
 			createExistingReleases(t, cfg, tc.existingReleases)
 			req := httptest.NewRequest("PUT", fmt.Sprintf("https://example.com/whatever?%s", tc.queryString), strings.NewReader(""))
 			response := httptest.NewRecorder()
@@ -472,7 +498,8 @@ func TestUpgradeAction(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			cfg := newConfigFixture(t)
+			k := &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}}
+			cfg := newConfigFixture(t, k)
 			createExistingReleases(t, cfg, tc.existingReleases)
 			req := httptest.NewRequest("PUT", fmt.Sprintf("https://example.com/whatever?%s", tc.queryString), strings.NewReader(tc.requestBody))
 			response := httptest.NewRecorder()

go.sum

@@ -222,6 +222,7 @@ github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5a
 github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
 github.com/google/go-cmp v0.3.1 h1:Xye71clBPdm5HgqGwUkwhbynsUJZhDbS20FvLhQ2izg=
 github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
+github.com/google/go-cmp v0.4.0 h1:xsAVV57WRhGj6kEIi8ReJzQlHHqcBYCElAvkovg3B/4=
 github.com/google/gofuzz v0.0.0-20161122191042-44d81051d367/go.mod h1:HP5RmnzzSNb993RKQDq4+1A4ia9nllfqcQFTQJedwGI=
 github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
@@ -551,6 +552,7 @@ gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I=
 gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw=
+helm.sh/helm v2.16.1+incompatible h1:np11uYeEtlYcFIFRya8Xs5ZweV1z6MvaWQqJAW+1SZQ=
 helm.sh/helm/v3 v3.0.0 h1:or/9cs1GgfcTQeEnR2CVJNw893/rmqIG1KsNHmUiSFw=
 helm.sh/helm/v3 v3.0.0/go.mod h1:sI7B9yfvMgxtTPMWdk1jSKJ2aa59UyP9qhPydqW6mgo=
 helm.sh/helm/v3 v3.0.2 h1:BggvLisIMrAc+Is5oAHVrlVxgwOOrMN8nddfQbm5gKo=

pkg/auth/auth.go

@@ -18,6 +18,7 @@ package auth
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	yamlUtils "github.com/kubeapps/kubeapps/pkg/yaml"
@@ -214,24 +215,40 @@ func (u *UserAuth) isAllowed(verb string, itemsToCheck []resource) ([]Action, er
 	return rejectedActions, nil
 }
 
+func uniqVerbs(current []string, new []string) []string {
+	resMap := map[string]bool{}
+	for _, v := range current {
+		if !resMap[v] {
+			resMap[v] = true
+		}
+	}
+	res := append([]string{}, current...)
+	for _, v := range new {
+		if !resMap[v] {
+			resMap[v] = true
+			res = append(res, v)
+		}
+	}
+	return res
+}
+
 func reduceActionsByVerb(actions []Action) []Action {
 	resMap := map[string]Action{}
-	res := []Action{}
 	for _, action := range actions {
 		req := fmt.Sprintf("%s/%s/%s", action.Namespace, action.APIVersion, action.Resource)
 		if _, ok := resMap[req]; ok {
 			// Element already exists
-			verbs := append(resMap[req].Verbs, action.Verbs...)
 			resMap[req] = Action{
 				APIVersion: action.APIVersion,
 				Resource:   action.Resource,
 				Namespace:  action.Namespace,
-				Verbs:      verbs,
+				Verbs:      uniqVerbs(resMap[req].Verbs, action.Verbs),
 			}
 		} else {
 			resMap[req] = action
 		}
 	}
+	res := []Action{}
 	for _, a := range resMap {
 		res = append(res, a)
 	}
@@ -267,3 +284,24 @@ func (u *UserAuth) GetForbiddenActions(namespace, action, manifest string) ([]Ac
 	}
 	return forbiddenActions, nil
 }
+
+// ParseForbiddenActions parses a forbidden error returned by the Kubernetes API and return the list of forbidden actions
+func ParseForbiddenActions(message string) []Action {
+	// TODO(andresmgot): Helm may not return all the required permissions in the same message. At the moment of writing this
+	// the only supported format is an error string so we can only parse the message with a regex
+	// More info: https://github.com/helm/helm/issues/7453
+	re := regexp.MustCompile(`User "(.*?)" cannot (.*?) resource "(.*?)" in API group "(.*?)"(?: in the namespace "(.*?)")?`)
+	match := re.FindAllStringSubmatch(message, -1)
+	forbiddenActions := []Action{}
+	for _, role := range match {
+		forbiddenActions = append(forbiddenActions, Action{
+			// TODO(andresmgot): Return the user/serviceaccount trying to perform the action
+			Verbs:       []string{role[2]},
+			Resource:    role[3],
+			APIVersion:  role[4],
+			Namespace:   role[5],
+			ClusterWide: role[5] == "",
+		})
+	}
+	return reduceActionsByVerb(forbiddenActions)
+}

pkg/auth/auth_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package auth
 
 import (
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"reflect"
 	"strings"
 	"testing"
@@ -211,3 +213,54 @@ kind: FooBar
 		}
 	}
 }
+
+func TestParseForbiddenActions(t *testing.T) {
+	testSuite := []struct {
+		Description     string
+		Error           string
+		ExpectedActions []Action
+	}{
+		{
+			"parses an error with a single resource",
+			`User "foo" cannot create resource "secrets" in API group "" in the namespace "default"`,
+			[]Action{
+				{APIVersion: "", Resource: "secrets", Namespace: "default", Verbs: []string{"create"}},
+			},
+		},
+		{
+			"parses an error with a cluster-wide resource",
+			`User "foo" cannot create resource "clusterroles" in API group "v1"`,
+			[]Action{
+				{APIVersion: "v1", Resource: "clusterroles", Namespace: "", Verbs: []string{"create"}, ClusterWide: true},
+			},
+		},
+		{
+			"parses several resources",
+			`User "foo" cannot create resource "secrets" in API group "" in the namespace "default";
+			User "foo" cannot create resource "pods" in API group "" in the namespace "default"`,
+			[]Action{
+				{APIVersion: "", Resource: "secrets", Namespace: "default", Verbs: []string{"create"}},
+				{APIVersion: "", Resource: "pods", Namespace: "default", Verbs: []string{"create"}},
+			},
+		},
+		{
+			"includes different verbs and remove duplicates",
+			`User "foo" cannot create resource "secrets" in API group "" in the namespace "default";
+			User "foo" cannot create resource "secrets" in API group "" in the namespace "default";
+			User "foo" cannot delete resource "secrets" in API group "" in the namespace "default"`,
+			[]Action{
+				{APIVersion: "", Resource: "secrets", Namespace: "default", Verbs: []string{"create", "delete"}},
+			},
+		},
+	}
+	for _, test := range testSuite {
+		t.Run(test.Description, func(t *testing.T) {
+			actions := ParseForbiddenActions(test.Error)
+			// order actions by resource
+			less := func(x, y Action) bool { return strings.Compare(x.Resource, y.Resource) < 0 }
+			if !cmp.Equal(actions, test.ExpectedActions, cmpopts.SortSlices(less)) {
+				t.Errorf("Unexpected forbidden actions: %v", cmp.Diff(actions, test.ExpectedActions))
+			}
+		})
+	}
+}