From e7a49f219ce761c76ef71cc981195f353a8972db Mon Sep 17 00:00:00 2001
From: Anumita Shenoy <ansheno@microsoft.com>
Date: Mon, 4 Apr 2022 22:10:00 +0530
Subject: [PATCH] Avoid storing result in cache on checkaccess error (#340)

* dont store error in cache

Signed-off-by: Anumita <ansheno@microsoft.com>

* added tests

Signed-off-by: Anumita <ansheno@microsoft.com>
---
 authz/providers/azure/azure.go      |  2 +-
 authz/providers/azure/azure_test.go | 26 +++++++++++++++++++++++---
 authz/providers/azure/rbac/rbac.go  |  2 ++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/authz/providers/azure/azure.go b/authz/providers/azure/azure.go
index b6adf29ae..672c29bd2 100644
--- a/authz/providers/azure/azure.go
+++ b/authz/providers/azure/azure.go
@@ -125,7 +125,7 @@ func (s Authorizer) Check(request *authzv1.SubjectAccessReviewSpec, store authz.
 		klog.V(5).Infof(response.Reason)
 		_ = s.rbacClient.SetResultInCache(request, response.Allowed, store)
 	} else {
-		_ = s.rbacClient.SetResultInCache(request, false, store)
+		err = errors.Errorf(rbac.CheckAccessErrorFormat, err)
 	}
 
 	return response, err
diff --git a/authz/providers/azure/azure_test.go b/authz/providers/azure/azure_test.go
index 6cb93b213..9432886c8 100644
--- a/authz/providers/azure/azure_test.go
+++ b/authz/providers/azure/azure_test.go
@@ -95,8 +95,8 @@ func serverSetup(loginResp, checkaccessResp string, loginStatus, checkaccessStat
 	return srv, nil
 }
 
-func getServerAndClient(t *testing.T, loginResp, checkaccessResp string) (*httptest.Server, *Authorizer, authz.Store) {
-	srv, err := serverSetup(loginResp, checkaccessResp, http.StatusOK, http.StatusOK)
+func getServerAndClient(t *testing.T, loginResp, checkaccessResp string, checkaccessStatus int) (*httptest.Server, *Authorizer, authz.Store) {
+	srv, err := serverSetup(loginResp, checkaccessResp, http.StatusOK, checkaccessStatus)
 	if err != nil {
 		t.Fatalf("Error when creating server, reason: %v", err)
 	}
@@ -126,7 +126,7 @@ func TestCheck(t *testing.T) {
 		"actionId":"Microsoft.Kubernetes/connectedClusters/pods/delete",
 		"isDataAction":true,"roleAssignment":null,"denyAssignment":null,"timeToLiveInMs":300000}]`
 
-		srv, client, store := getServerAndClient(t, loginResp, validBody)
+		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusOK)
 		defer srv.Close()
 		defer store.Close()
 
@@ -144,4 +144,24 @@ func TestCheck(t *testing.T) {
 		assert.Equal(t, resp.Allowed, true)
 		assert.Equal(t, resp.Denied, false)
 	})
+
+	t.Run("unsuccessful request", func(t *testing.T) {
+		validBody := `""`
+		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusInternalServerError)
+		defer srv.Close()
+		defer store.Close()
+
+		request := &authzv1.SubjectAccessReviewSpec{
+			User: "beta@bing.com",
+			ResourceAttributes: &authzv1.ResourceAttributes{
+				Namespace: "dev", Group: "", Resource: "pods",
+				Subresource: "status", Version: "v1", Name: "test", Verb: "delete",
+			}, Extra: map[string]authzv1.ExtraValue{"oid": {"00000000-0000-0000-0000-000000000000"}},
+		}
+
+		resp, err := client.Check(request, store)
+		assert.Nilf(t, resp, "response should be nil")
+		assert.NotNilf(t, err, "should get error")
+		assert.Contains(t, err.Error(), "Error occured during authorization check")
+	})
 }
diff --git a/authz/providers/azure/rbac/rbac.go b/authz/providers/azure/rbac/rbac.go
index b9dc8a212..e17bd211b 100644
--- a/authz/providers/azure/rbac/rbac.go
+++ b/authz/providers/azure/rbac/rbac.go
@@ -93,6 +93,8 @@ var (
 		Name: "guard_azure_checkaccess_success_total",
 		Help: "Azure checkaccess success calls.",
 	})
+
+	CheckAccessErrorFormat = "Error occured during authorization check. Please retry again. Error: %s"
 )
 
 func getClusterType(clsType string) string {