Skip to content

Commit

Permalink
Avoid storing result in cache on checkaccess error (#340)
Browse files Browse the repository at this point in the history
* dont store error in cache

Signed-off-by: Anumita <[email protected]>

* added tests

Signed-off-by: Anumita <[email protected]>
  • Loading branch information
Anumita authored Apr 4, 2022
1 parent d2a160d commit e7a49f2
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 4 deletions.
2 changes: 1 addition & 1 deletion authz/providers/azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 23 additions & 3 deletions authz/providers/azure/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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()

Expand All @@ -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: "[email protected]",
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")
})
}
2 changes: 2 additions & 0 deletions authz/providers/azure/rbac/rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit e7a49f2

Please sign in to comment.