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

johnstcn · johnstcn · commit 9a52b4b405cc · 2024-01-26T14:58:51.000Z
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()
 
