diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 807adeb361..51a28882c4 100644 --- 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 index 05f402abe4..9a38c94e33 100644 --- 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()