feat: Add cachable authorizer to elimate duplicate rbac calls (#6107)

* feat: Add cachable authorizer to elimate duplicate rbac calls

Cache is context bound, so only prevents duplicate rbac calls in
the same request context.

coderd/rbac/action.go

@@ -9,3 +9,8 @@ const (
 	ActionUpdate Action = "update"
 	ActionDelete Action = "delete"
 )
+
+// AllActions is a helper function to return all the possible actions types.
+func AllActions() []Action {
+	return []Action{ActionCreate, ActionRead, ActionUpdate, ActionDelete}
+}

coderd/rbac/authz_internal_test.go

@@ -222,16 +222,16 @@ func TestAuthorizeDomain(t *testing.T) {
 	testAuthorize(t, "UserACLList", user, []authTestCase{
 		{
 			resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
-				user.ID: allActions(),
+				user.ID: AllActions(),
 			}),
-			actions: allActions(),
+			actions: AllActions(),
 			allow:   true,
 		},
 		{
 			resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
 				user.ID: {WildcardSymbol},
 			}),
-			actions: allActions(),
+			actions: AllActions(),
 			allow:   true,
 		},
 		{
@@ -254,16 +254,16 @@ func TestAuthorizeDomain(t *testing.T) {
 	testAuthorize(t, "GroupACLList", user, []authTestCase{
 		{
 			resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(defOrg).WithGroupACL(map[string][]Action{
-				allUsersGroup: allActions(),
+				allUsersGroup: AllActions(),
 			}),
-			actions: allActions(),
+			actions: AllActions(),
 			allow:   true,
 		},
 		{
 			resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(defOrg).WithGroupACL(map[string][]Action{
 				allUsersGroup: {WildcardSymbol},
 			}),
-			actions: allActions(),
+			actions: AllActions(),
 			allow:   true,
 		},
 		{
@@ -285,27 +285,27 @@ func TestAuthorizeDomain(t *testing.T) {
 
 	testAuthorize(t, "Member", user, []authTestCase{
 		// Org + me
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: allActions(), allow: true},
-		{resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: AllActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner(user.ID), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.WithOwner(user.ID), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.All(), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.All(), actions: AllActions(), allow: false},
 
 		// Other org + me
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: false},
 
 		// Other org + other user
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: false},
 
 		// Other org + other us
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: false},
 	})
 
 	user = Subject{
@@ -326,27 +326,27 @@ func TestAuthorizeDomain(t *testing.T) {
 
 	testAuthorize(t, "DeletedMember", user, []authTestCase{
 		// Org + me
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(defOrg), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner(user.ID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner(user.ID), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.All(), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.All(), actions: AllActions(), allow: false},
 
 		// Other org + me
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: false},
 
 		// Other org + other user
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: false},
 
 		// Other org + other use
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: false},
 	})
 
 	user = Subject{
@@ -360,27 +360,27 @@ func TestAuthorizeDomain(t *testing.T) {
 
 	testAuthorize(t, "OrgAdmin", user, []authTestCase{
 		// Org + me
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: allActions(), allow: true},
-		{resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: AllActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.WithOwner(user.ID), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.WithOwner(user.ID), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.All(), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.All(), actions: AllActions(), allow: false},
 
 		// Other org + me
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: false},
 
 		// Other org + other user
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: false},
 
 		// Other org + other use
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: AllActions(), allow: false},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: false},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: false},
 	})
 
 	user = Subject{
@@ -394,27 +394,27 @@ func TestAuthorizeDomain(t *testing.T) {
 
 	testAuthorize(t, "SiteAdmin", user, []authTestCase{
 		// Org + me
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: allActions(), allow: true},
-		{resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: AllActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.WithOwner(user.ID), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.WithOwner(user.ID), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.All(), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.All(), actions: AllActions(), allow: true},
 
 		// Other org + me
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: allActions(), allow: true},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.ID), actions: AllActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: true},
 
 		// Other org + other user
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: true},
 
 		// Other org + other use
-		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: true},
-		{resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: AllActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(unuseID), actions: AllActions(), allow: true},
 
-		{resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true},
+		{resource: ResourceWorkspace.WithOwner("not-me"), actions: AllActions(), allow: true},
 	})
 
 	user = Subject{
@@ -642,7 +642,7 @@ func TestAuthorizeLevels(t *testing.T) {
 
 	testAuthorize(t, "AdminAlwaysAllow", user,
 		cases(func(c authTestCase) authTestCase {
-			c.actions = allActions()
+			c.actions = AllActions()
 			c.allow = true
 			return c
 		}, []authTestCase{
@@ -701,7 +701,7 @@ func TestAuthorizeLevels(t *testing.T) {
 
 	testAuthorize(t, "OrgAllowAll", user,
 		cases(func(c authTestCase) authTestCase {
-			c.actions = allActions()
+			c.actions = AllActions()
 			return c
 		}, []authTestCase{
 			// Org + me
@@ -1034,10 +1034,6 @@ func testAuthorize(t *testing.T, name string, subject Subject, sets ...[]authTes
 	}
 }
 
-// allActions is a helper function to return all the possible actions types.
-func allActions() []Action {
-	return []Action{ActionCreate, ActionRead, ActionUpdate, ActionDelete}
-}
 
 func must[T any](value T, err error) T {
 	if err != nil {

coderd/rbac/cache.go

@@ -0,0 +1,107 @@
+package rbac
+
+import (
+	"context"
+	"sync"
+)
+
+type AuthCall struct {
+	Actor  Subject
+	Action Action
+	Object Object
+}
+
+type cachedCalls 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.
+// If no cache is found on the context, the Authorizer is called as normal.
+func Cacher(authz Authorizer) Authorizer {
+	return &cachedCalls{authz: authz}
+}
+
+func (c *cachedCalls) Authorize(ctx context.Context, subject Subject, action Action, object Object) error {
+	cache := cacheFromContext(ctx)
+
+	resp, ok := cache.Load(subject, action, object)
+	if ok {
+		return resp
+	}
+
+	err := c.authz.Authorize(ctx, subject, action, object)
+	cache.Save(subject, action, object, err)
+	return err
+}
+
+// Prepare returns the underlying PreparedAuthorized. The cache does not apply
+// to prepared authorizations. These should be using a SQL filter, and
+// therefore the cache is not needed.
+func (c *cachedCalls) Prepare(ctx context.Context, subject Subject, action Action, objectType string) (PreparedAuthorized, error) {
+	return c.authz.Prepare(ctx, subject, action, objectType)
+}
+
+type cachedAuthCall struct {
+	AuthCall
+	Err error
+}
+
+type authorizeCache struct {
+	sync.Mutex
+	// calls is a list of all calls made to the Authorizer.
+	// This list is cached per request context. The size of this list is expected
+	// to be incredibly small. Often 1 or 2 calls.
+	calls []cachedAuthCall
+}
+
+//nolint:error-return,revive
+func (c *authorizeCache) Load(subject Subject, action Action, object Object) (error, bool) {
+	if c == nil {
+		return nil, false
+	}
+	c.Lock()
+	defer c.Unlock()
+
+	for _, call := range c.calls {
+		if call.Action == action && call.Object.Equal(object) && call.Actor.Equal(subject) {
+			return call.Err, true
+		}
+	}
+	return nil, false
+}
+
+func (c *authorizeCache) Save(subject Subject, action Action, object Object, err error) {
+	if c == nil {
+		return
+	}
+	c.Lock()
+	defer c.Unlock()
+
+	c.calls = append(c.calls, cachedAuthCall{
+		AuthCall: AuthCall{
+			Actor:  subject,
+			Action: action,
+			Object: object,
+		},
+		Err: err,
+	})
+}
+
+// cacheContextKey is a context key used to store the cache in the context.
+type cacheContextKey struct{}
+
+// cacheFromContext returns the cache from the context.
+// If there is no cache, a nil value is returned.
+// The nil cache can still be called as a normal cache, but will not cache or
+// return any values.
+func cacheFromContext(ctx context.Context) *authorizeCache {
+	cache, _ := ctx.Value(cacheContextKey{}).(*authorizeCache)
+	return cache
+}
+
+func WithCacheCtx(ctx context.Context) context.Context {
+	return context.WithValue(ctx, cacheContextKey{}, &authorizeCache{})
+}

coderd/rbac/cache_test.go

@@ -0,0 +1,86 @@
+package rbac_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/coderd/coderdtest"
+	"github.com/coder/coder/coderd/rbac"
+)
+
+func TestCacher(t *testing.T) {
+	t.Parallel()
+
+	t.Run("EmptyCacheCtx", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := context.Background()
+		rec := &coderdtest.RecordingAuthorizer{
+			Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
+		}
+		authz := rbac.Cacher(rec)
+		subj, obj, action := coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+
+		// Two identical calls
+		_ = authz.Authorize(ctx, subj, action, obj)
+		_ = authz.Authorize(ctx, subj, action, obj)
+
+		// Yields two calls to the wrapped Authorizer
+		rec.AssertActor(t, subj, rec.Pair(action, obj), rec.Pair(action, obj))
+		require.NoError(t, rec.AllAsserted(), "all assertions should have been made")
+	})
+
+	t.Run("CacheCtx", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := rbac.WithCacheCtx(context.Background())
+		rec := &coderdtest.RecordingAuthorizer{
+			Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
+		}
+		authz := rbac.Cacher(rec)
+		subj, obj, action := coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+
+		// Two identical calls
+		_ = authz.Authorize(ctx, subj, action, obj)
+		_ = authz.Authorize(ctx, subj, action, obj)
+
+		// Yields only 1 call to the wrapped Authorizer for that subject
+		rec.AssertActor(t, subj, rec.Pair(action, obj))
+		require.NoError(t, rec.AllAsserted(), "all assertions should have been made")
+	})
+
+	t.Run("MultipleSubjects", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := rbac.WithCacheCtx(context.Background())
+		rec := &coderdtest.RecordingAuthorizer{
+			Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
+		}
+		authz := rbac.Cacher(rec)
+		subj1, obj1, action1 := coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+
+		// Two identical calls
+		_ = authz.Authorize(ctx, subj1, action1, obj1)
+		_ = authz.Authorize(ctx, subj1, action1, obj1)
+
+		// Extra unique calls
+		var pairs []coderdtest.ActionObjectPair
+		subj2, obj2, action2 := coderdtest.RandomRBACSubject(), coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+		_ = authz.Authorize(ctx, subj2, action2, obj2)
+		pairs = append(pairs, rec.Pair(action2, obj2))
+
+		obj3, action3 := coderdtest.RandomRBACObject(), coderdtest.RandomRBACAction()
+		_ = authz.Authorize(ctx, subj2, action3, obj3)
+		pairs = append(pairs, rec.Pair(action3, obj3))
+
+		// Extra identical call after some unique calls
+		_ = authz.Authorize(ctx, subj1, action1, obj1)
+
+		// Yields 3 calls, 1 for the first subject, 2 for the unique subjects
+		rec.AssertActor(t, subj1, rec.Pair(action1, obj1))
+		rec.AssertActor(t, subj2, pairs...)
+		require.NoError(t, rec.AllAsserted(), "all assertions should have been made")
+	})
+}