diff --git a/coderd/authorize.go b/coderd/authorize.go index 6f2cf01cd4..5ded52030a 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -22,7 +22,7 @@ import ( // This is faster than calling Authorize() on each object. func AuthorizeFilter[O rbac.Objecter](h *HTTPAuthorizer, r *http.Request, action policy.Action, objects []O) ([]O, error) { roles := httpmw.UserAuthorization(r.Context()) - objects, err := rbac.Filter(r.Context(), h.Authorizer, roles, action, objects) + objects, err := rbac.Filter(r.Context(), h.Authorizer, roles, action, objects, rbac.DefaultFilterThreshold) if err != nil { // Log the error as Filter should not be erroring. h.Logger.Error(r.Context(), "authorization filter failed", @@ -154,6 +154,30 @@ func (h *HTTPAuthorizer) AuthorizeSQLFilterContext(ctx context.Context, action p return prepared, nil } +// authcheckFilterThreshold is the per-(action, resource type) group size at or +// above which checkAuthorization lets rbac.Filter switch to a single partial +// evaluation. For a subject in many organizations, a group can hold one object +// per organization while the subject also carries one role per organization, so +// Prepare cost grows with the group size. The measured crossover where batching +// beats per-object evaluation is ~35 (DEVEX-608), so this sits above it: +// subjects with few objects of a given type keep the per-object path and cannot +// regress, and only large groups pay for and benefit from partial evaluation. +// It is higher than rbac.DefaultFilterThreshold because that default assumes a +// Prepare cost independent of the input size. +const authcheckFilterThreshold = 50 + +// authorizeCheck carries an authorization check's response key alongside its +// resolved RBAC object. It implements rbac.Objecter so a batch of same-typed +// checks can be run through rbac.Filter; because the key travels with the +// object, the filtered subset maps back to keys by reading the field, without +// relying on element identity. +type authorizeCheck struct { + key string + object rbac.Object +} + +func (c authorizeCheck) RBACObject() rbac.Object { return c.object } + // checkAuthorization returns if the current API key can use the given // permissions, factoring in the current user's roles and the API key scopes. // @@ -206,6 +230,15 @@ func (api *API) checkAuthorization(rw http.ResponseWriter, r *http.Request) { return } + // Group the checks by (action, resource type) and authorize each group with + // rbac.Filter, which amortizes a single partial evaluation across the group + // once it is large enough. Each check carries its response key so the + // filtered subset maps back without relying on element identity. + type checkGroup struct { + action policy.Action + objectType string + } + groups := make(map[checkGroup][]authorizeCheck) for k, v := range params.Checks { if v.Object.ResourceType == "" { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ @@ -263,8 +296,43 @@ func (api *API) checkAuthorization(rw http.ResponseWriter, r *http.Request) { obj = dbObj.RBACObject() } - err := api.Authorizer.Authorize(ctx, auth, policy.Action(v.Action), obj) - response[k] = err == nil + // AnyOrgOwner objects have no verified semantics under partial + // evaluation: Filter's prepared path can deny objects that a full + // evaluation allows (authz_internal_test.go skips the full-vs-partial + // equivalence assertion for them). Authorize them per-object with a full + // evaluation instead of grouping them into the batched Filter path. + if obj.AnyOrgOwner { + err := api.Authorizer.Authorize(ctx, auth, policy.Action(v.Action), obj) + response[k] = err == nil + continue + } + + group := checkGroup{action: policy.Action(v.Action), objectType: obj.Type} + groups[group] = append(groups[group], authorizeCheck{key: k, object: obj}) + } + + for group, checks := range groups { + allowed, err := rbac.Filter(ctx, api.Authorizer, auth, group.action, checks, authcheckFilterThreshold) + if err != nil { + // A Filter error is never a per-object denial: per-object rejections + // are filtered out inside Filter, so only Prepare failures and context + // errors reach here. Reporting the group as denied would hide an + // evaluation failure behind a "not permitted" answer, so surface it. + if ctx.Err() != nil { + // The client went away or the request was canceled; there is no + // useful response to write and nothing worth logging. + return + } + httpapi.InternalServerError(rw, xerrors.Errorf("authorize %q %q: %w", group.action, group.objectType, err)) + return + } + // Default to denied, then mark the checks Filter allowed. + for _, c := range checks { + response[c.key] = false + } + for _, c := range allowed { + response[c.key] = true + } } httpapi.Write(ctx, rw, http.StatusOK, response) diff --git a/coderd/authorize_anyorg_test.go b/coderd/authorize_anyorg_test.go new file mode 100644 index 0000000000..d177a1f1c1 --- /dev/null +++ b/coderd/authorize_anyorg_test.go @@ -0,0 +1,58 @@ +package coderd_test + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +// TestCheckPermissionsAnyOrg demonstrates that batching authcheck permissions +// through rbac.Filter breaks checks with any_org=true: partial evaluation +// denies AnyOrgOwner objects that full evaluation allows. A single any_org +// check (below the batching threshold) returns true, while the same check +// repeated 55 times (pushing the group over the threshold) returns false. +func TestCheckPermissionsAnyOrg(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + t.Cleanup(cancel) + + adminClient := coderdtest.New(t, nil) + adminUser := coderdtest.CreateFirstUser(t, adminClient) + memberClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID) + + check := codersdk.AuthorizationCheck{ + Object: codersdk.AuthorizationObject{ + ResourceType: codersdk.ResourceWorkspace, + OwnerID: "me", + AnyOrgOwner: true, + }, + Action: "create", + } + + // Below the batching threshold: full evaluation, allowed. + single, err := memberClient.AuthCheck(ctx, codersdk.AuthorizationRequest{ + Checks: map[string]codersdk.AuthorizationCheck{"can-create-workspace": check}, + }) + require.NoError(t, err) + require.True(t, single["can-create-workspace"], "single any_org check should be allowed") + + // Same check, 55 copies: the (create, workspace) group crosses the + // batching threshold and is evaluated with a prepared partial query, + // which denies AnyOrgOwner objects. + grouped := make(map[string]codersdk.AuthorizationCheck) + for i := 0; i < 55; i++ { + grouped[fmt.Sprintf("can-create-workspace-%d", i)] = check + } + groupedResp, err := memberClient.AuthCheck(ctx, codersdk.AuthorizationRequest{Checks: grouped}) + require.NoError(t, err) + for key, allowed := range groupedResp { + require.True(t, allowed, "grouped any_org check %q should be allowed but was denied", key) + } +} diff --git a/coderd/authorize_test.go b/coderd/authorize_test.go index e3ce4b922f..6de1ddb144 100644 --- a/coderd/authorize_test.go +++ b/coderd/authorize_test.go @@ -2,6 +2,7 @@ package coderd_test import ( "context" + "fmt" "testing" "github.com/google/uuid" @@ -137,4 +138,139 @@ func TestCheckPermissions(t *testing.T) { require.Equal(t, c.Check, resp) }) } + + // Enough same-typed checks in one request to push a group past the batching + // threshold, exercising the grouped partial-evaluation path and the key + // mapping. Reading org members is allowed for any member; updating them is + // admin-only, so the two actions must map back to their keys distinctly. + t.Run("CheckAuthorization/Grouped", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + t.Cleanup(cancel) + + grouped := make(map[string]codersdk.AuthorizationCheck) + adminExpected := make(map[string]bool) + memberExpected := make(map[string]bool) + for i := 0; i < 55; i++ { + readKey := fmt.Sprintf("read-members-%d", i) + grouped[readKey] = codersdk.AuthorizationCheck{ + Object: codersdk.AuthorizationObject{ + ResourceType: codersdk.ResourceOrganizationMember, + OrganizationID: adminUser.OrganizationID.String(), + }, + Action: "read", + } + adminExpected[readKey] = true + memberExpected[readKey] = true + + updateKey := fmt.Sprintf("update-members-%d", i) + grouped[updateKey] = codersdk.AuthorizationCheck{ + Object: codersdk.AuthorizationObject{ + ResourceType: codersdk.ResourceOrganizationMember, + OrganizationID: adminUser.OrganizationID.String(), + }, + Action: "update", + } + adminExpected[updateKey] = true + memberExpected[updateKey] = false + } + + adminResp, err := adminClient.AuthCheck(ctx, codersdk.AuthorizationRequest{Checks: grouped}) + require.NoError(t, err) + require.Equal(t, adminExpected, map[string]bool(adminResp)) + + memberResp, err := memberClient.AuthCheck(ctx, codersdk.AuthorizationRequest{Checks: grouped}) + require.NoError(t, err) + require.Equal(t, memberExpected, map[string]bool(memberResp)) + }) + + // A member may create a workspace in an org it belongs to, so an + // any_org=true check is allowed. Repeating it past the batching threshold + // must not change the answer: AnyOrgOwner objects have no verified partial- + // evaluation semantics, so they must stay on the per-object path rather than + // being grouped into rbac.Filter's prepared query, which denies them. + t.Run("CheckAuthorization/AnyOrg", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + t.Cleanup(cancel) + + check := codersdk.AuthorizationCheck{ + Object: codersdk.AuthorizationObject{ + ResourceType: codersdk.ResourceWorkspace, + OwnerID: "me", + AnyOrgOwner: true, + }, + Action: "create", + } + + // Below the threshold: a single check is evaluated in full and allowed. + single, err := memberClient.AuthCheck(ctx, codersdk.AuthorizationRequest{ + Checks: map[string]codersdk.AuthorizationCheck{"create-any-org": check}, + }) + require.NoError(t, err) + require.True(t, single["create-any-org"], "single any_org check should be allowed") + + // The same check repeated past the threshold must stay allowed. + grouped := make(map[string]codersdk.AuthorizationCheck) + expected := make(map[string]bool) + for i := 0; i < 55; i++ { + key := fmt.Sprintf("create-any-org-%d", i) + grouped[key] = check + expected[key] = true + } + resp, err := memberClient.AuthCheck(ctx, codersdk.AuthorizationRequest{Checks: grouped}) + require.NoError(t, err) + require.Equal(t, expected, map[string]bool(resp)) + }) + + // A single (read, workspace) group past the batching threshold mixing + // resource_id checks (concrete workspaces fetched via the maxFetch path) + // with org-scoped checks (synthetic org-level objects). The member owns the + // fetched workspace so those keys are allowed, but cannot read all org + // workspaces so the org-scoped keys are denied. Both object shapes must map + // back to the correct per-key verdict through the one prepared query. + t.Run("CheckAuthorization/MixedGroup", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + t.Cleanup(cancel) + + workspace := coderdtest.CreateWorkspace(t, memberClient, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, memberClient, workspace.LatestBuild.ID) + + grouped := make(map[string]codersdk.AuthorizationCheck) + expected := make(map[string]bool) + // resource_id checks are capped at maxFetch (10); the member owns the + // workspace, so reading it is allowed. + for i := 0; i < 10; i++ { + key := fmt.Sprintf("read-own-workspace-%d", i) + grouped[key] = codersdk.AuthorizationCheck{ + Object: codersdk.AuthorizationObject{ + ResourceType: codersdk.ResourceWorkspace, + ResourceID: workspace.ID.String(), + }, + Action: "read", + } + expected[key] = true + } + // Org-scoped checks push the (read, workspace) group past the threshold. + // A member cannot read every workspace in the org, so these are denied. + for i := 0; i < 45; i++ { + key := fmt.Sprintf("read-org-workspace-%d", i) + grouped[key] = codersdk.AuthorizationCheck{ + Object: codersdk.AuthorizationObject{ + ResourceType: codersdk.ResourceWorkspace, + OrganizationID: adminUser.OrganizationID.String(), + }, + Action: "read", + } + expected[key] = false + } + + resp, err := memberClient.AuthCheck(ctx, codersdk.AuthorizationRequest{Checks: grouped}) + require.NoError(t, err) + require.Equal(t, expected, map[string]bool(resp)) + }) } diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 3d036e150e..58e8b989b2 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1273,7 +1273,7 @@ func fetchWithPostFilter[ } // Authorize the action - return rbac.Filter(ctx, authorizer, act, action, objects) + return rbac.Filter(ctx, authorizer, act, action, objects, rbac.DefaultFilterThreshold) } } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index c1b94300a9..55998ef5e1 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -210,13 +210,32 @@ type PreparedAuthorized interface { CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) } +// DefaultFilterThreshold is the object count at or above which Filter switches +// from a full evaluation per object to a single partial evaluation (Prepare) +// reused across the set. Benchmarks show Authorize is faster than the Prepare +// overhead below ~10 objects. Callers whose Prepare cost grows with the input +// size (for example a subject carrying one role per object) should pass a +// higher threshold. +const DefaultFilterThreshold = 10 + // Filter takes in a list of objects, and will filter the list removing all // the elements the subject does not have permission for. All objects must be // of the same type. // +// prepareThreshold is the object count at or above which Filter uses a single +// partial evaluation reused across the set instead of a full evaluation per +// object. Pass DefaultFilterThreshold unless the caller has a reason to tune +// it. +// // Ideally the 'CompileToSQL' is used instead for large sets. This cost scales // linearly with the number of objects passed in. -func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, action policy.Action, objects []O) ([]O, error) { +func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, action policy.Action, objects []O, prepareThreshold int) ([]O, error) { + if prepareThreshold <= 0 { + // A non-positive threshold would force the Prepare path for every + // non-empty input, the opposite of what a caller passing 0 as a stand-in + // for "default" expects. Fail loudly on an authorization function. + return nil, xerrors.New("prepareThreshold must be positive; pass DefaultFilterThreshold") + } if len(objects) == 0 { // Nothing to filter return objects, nil @@ -238,11 +257,9 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a ) defer span.End() - // Running benchmarks on this function, it is **always** faster to call - // auth.Authorize on <10 objects. This is because the overhead of - // 'Prepare'. Once we cross 10 objects, then it starts to become - // faster - if len(objects) < 10 { + // Below the threshold, a full evaluation per object is faster than paying + // the Prepare overhead once and reusing it. + if len(objects) < prepareThreshold { for _, o := range objects { rbacObj := o.RBACObject() if rbacObj.Type != objectType { diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index c461590040..f648752c53 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -61,10 +61,27 @@ func TestFilterError(t *testing.T) { Scope: ScopeAll, } - _, err := Filter(context.Background(), auth, subject, policy.ActionRead, []Object{ResourceUser, ResourceWorkspace}) + _, err := Filter(context.Background(), auth, subject, policy.ActionRead, []Object{ResourceUser, ResourceWorkspace}, DefaultFilterThreshold) require.ErrorContains(t, err, "object types must be uniform") }) + t.Run("NonPositiveThreshold", func(t *testing.T) { + t.Parallel() + + auth := NewAuthorizer(prometheus.NewRegistry()) + subject := Subject{ + ID: uuid.NewString(), + Roles: RoleIdentifiers{}, + Groups: []string{}, + Scope: ScopeAll, + } + + for _, threshold := range []int{0, -1} { + _, err := Filter(context.Background(), auth, subject, policy.ActionRead, []Object{ResourceWorkspace}, threshold) + require.ErrorContains(t, err, "prepareThreshold must be positive") + } + }) + t.Run("CancelledContext", func(t *testing.T) { t.Parallel() @@ -99,7 +116,7 @@ func TestFilterError(t *testing.T) { ResourceUser, } - _, err := Filter(ctx, auth, subject, policy.ActionRead, objects) + _, err := Filter(ctx, auth, subject, policy.ActionRead, objects, DefaultFilterThreshold) require.ErrorIs(t, err, context.Canceled) }) @@ -119,7 +136,7 @@ func TestFilterError(t *testing.T) { bomb: cancel, } - _, err := Filter(ctx, auth, subject, policy.ActionRead, objects) + _, err := Filter(ctx, auth, subject, policy.ActionRead, objects, DefaultFilterThreshold) require.ErrorIs(t, err, context.Canceled) }) }) @@ -267,7 +284,7 @@ func TestFilter(t *testing.T) { } // Run by filter - list, err := Filter(ctx, auth, actor, tc.Action, localObjects) + list, err := Filter(ctx, auth, actor, tc.Action, localObjects, DefaultFilterThreshold) require.NoError(t, err) require.Equal(t, allowedCount, len(list), "expected number of allowed") for _, obj := range list { diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 2595513124..f4de303deb 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -255,7 +255,7 @@ func BenchmarkRBACFilter(b *testing.B) { b.Run(c.Name, func(b *testing.B) { objects := benchmarkSetup(orgs, users, b.N) b.ResetTimer() - allowed, err := rbac.Filter(context.Background(), authorizer, c.Actor, policy.ActionRead, objects) + allowed, err := rbac.Filter(context.Background(), authorizer, c.Actor, policy.ActionRead, objects, rbac.DefaultFilterThreshold) require.NoError(b, err) _ = allowed })