fix(coderd/rbac): do not cache context cancellation errors (#11840)

johnstcn · web-flow · commit 42e997d39edc · 2024-01-26T16:19:55.000Z
#7439 added global caching of RBAC results. Calls are cached based on hash(subject, object, action). We often use dbauthz.AsSystemRestricted to handle "internal" authz calls, and these are often repeated with similar arguments and are likely to get cached. So a transient error doing an authz check on a system function will be cached for up to a minute. I'm just starting off with excluding context.Canceled but there's likely a whole suite of different errors we want to also exclude from the global cache.
diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -5,6 +5,7 @@ import (
 	"crypto/sha256"
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"strings"
 	"sync"
 	"time"
@@ -653,10 +654,10 @@ type authCache struct {
 	authz Authorizer
 }
 
-// Cacher returns an Authorizer that can use a cache stored on a context
-// to short circuit duplicate calls to the Authorizer. This is useful when
-// multiple calls are made to the Authorizer for the same subject, action, and
-// object. The cache is on each `ctx` and is not shared between requests.
+// Cacher returns an Authorizer that can use a cache to short circuit duplicate
+// calls to the Authorizer. This is useful when multiple calls are made to the
+// Authorizer for the same subject, action, and object.
+// This is a GLOBAL cache shared between all requests.
 // If no cache is found on the context, the Authorizer is called as normal.
 //
 // Cacher is safe for multiple actors.
@@ -676,8 +677,12 @@ func (c *authCache) Authorize(ctx context.Context, subject Subject, action Actio
 	err, _, ok := c.cache.Get(authorizeCacheKey)
 	if !ok {
 		err = c.authz.Authorize(ctx, subject, action, object)
-		// In case there is a caching bug, bound the TTL to 1 minute.
-		c.cache.Set(authorizeCacheKey, err, time.Minute)
+		// If there is a transient error such as a context cancellation, do not
+		// cache it.
+		if !errors.Is(err, context.Canceled) {
+			// In case there is a caching bug, bound the TTL to 1 minute.
+			c.cache.Set(authorizeCacheKey, err, time.Minute)
+		}
 	}
 
 	return err
diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go
@@ -7,10 +7,12 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/rbac"
+	"github.com/coder/coder/v2/testutil"
 )
 
 type benchmarkCase struct {
@@ -351,6 +353,47 @@ func TestCacher(t *testing.T) {
 		require.NoError(t, rec.AllAsserted(), "all assertions should have been made")
 	})
 
+	t.Run("DontCacheTransientErrors", func(t *testing.T) {
+		t.Parallel()
+
+		var (
+			ctx           = testutil.Context(t, testutil.WaitShort)
+			authOut       = make(chan error, 1) // buffered to not block
+			authorizeFunc = func(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error {
+				// Just return what you're told.
+				return testutil.RequireRecvCtx(ctx, t, authOut)
+			}
+			ma                = &rbac.MockAuthorizer{AuthorizeFunc: authorizeFunc}
+			rec               = &coderdtest.RecordingAuthorizer{Wrapped: ma}
+			authz             = rbac.Cacher(rec)
+			subj, obj, action = coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+		)
+
+		// First call will result in a transient error. This should not be cached.
+		testutil.RequireSendCtx(ctx, t, authOut, context.Canceled)
+		err := authz.Authorize(ctx, subj, action, obj)
+		assert.ErrorIs(t, err, context.Canceled)
+
+		// A subsequent call should still hit the authorizer.
+		testutil.RequireSendCtx(ctx, t, authOut, nil)
+		err = authz.Authorize(ctx, subj, action, obj)
+		assert.NoError(t, err)
+		// This should be cached and not hit the wrapped authorizer again.
+		err = authz.Authorize(ctx, subj, action, obj)
+		assert.NoError(t, err)
+
+		// Let's change the subject.
+		subj, obj, action = coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+
+		// A third will be a legit error
+		testutil.RequireSendCtx(ctx, t, authOut, assert.AnError)
+		err = authz.Authorize(ctx, subj, action, obj)
+		assert.EqualError(t, err, assert.AnError.Error())
+		// This should be cached and not hit the wrapped authorizer again.
+		err = authz.Authorize(ctx, subj, action, obj)
+		assert.EqualError(t, err, assert.AnError.Error())
+	})
+
 	t.Run("MultipleSubjects", func(t *testing.T) {
 		t.Parallel()
 
