From b3536bc315c9527a1109f48e211fee69b772d4fc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 22:46:41 -0500 Subject: [PATCH 01/30] chore: Update rego to be partial execution friendly --- coderd/rbac/policy.rego | 171 +++++++++++++++++----------------------- 1 file changed, 72 insertions(+), 99 deletions(-) diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 8f5208aca138e..68af7ae7a59c1 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -1,11 +1,10 @@ package authz -import future.keywords.in -import future.keywords.every +import future.keywords +# Helpful cli commands to debug. +# opa eval --format=pretty 'data.authz.allow = true' -d policy.rego -i input.json +# opa eval --partial --format=pretty 'data.authz.allow = true' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner -i input.json + -# A great playground: https://play.openpolicyagent.org/ -# TODO: Add debug instructions to do in the cli. Running really short on time, the -# playground is sufficient for now imo. In the future we can provide a tidy bash -# script for running this against predefined input. # bool_flip lets you assign a value to an inverted bool. # You cannot do 'x := !false', but you can do 'x := bool_flip(false)' @@ -19,121 +18,95 @@ bool_flip(b) = flipped { flipped = true } -# perms_grant returns a set of boolean values {true, false}. -# True means a positive permission in the set, false is a negative permission. -# It will only return `bool_flip(perm.negate)` for permissions that affect a given -# resource_type, and action. -# The empty set is returned if no relevant permissions are found. -perms_grant(permissions) = grants { - # If there are no permissions, this value is the empty set {}. - grants := { x | - # All permissions ... - perm := permissions[_] - # Such that the permission action, and type matches - perm.action in [input.action, "*"] - perm.resource_type in [input.object.type, "*"] - x := bool_flip(perm.negate) - } +number(set) = c { + count(set) == 0 + c := 0 } -# Site & User are both very simple. We default both to the empty set '{}'. If no permissions are present, then the -# result is the default value. -default site = {} -site = grant { - # Boolean set for all site wide permissions. - grant = { v | # Use set comprehension to remove duplicate values - # For each role, grab the site permission. - # Find the grants on this permission list. - v = perms_grant(input.subject.roles[_].site)[_] - } +number(set) = c { + false in set + c := -1 } -default user = {} -user = grant { - # Only apply user permissions if the user owns the resource - input.object.owner != "" - input.object.owner == input.subject.id - grant = { v | - # For each role, grab the user permissions. - # Find the grants on this permission list. - v = perms_grant(input.subject.roles[_].user)[_] - } +number(set) = c { + not false in set + set[_] + c := 1 } -# Organizations are more complex. If the user has no roles that specifically indicate the org_id of the object, -# then we want to block the action. This is because that means the user is not a member of the org. -# A non-member cannot access any org resources. -# org_member returns the set of permissions associated with a user if the user is a member of the -# organization -org_member = grant { - input.object.org_owner != "" - grant = { v | - v = perms_grant(input.subject.roles[_].org[input.object.org_owner])[_] +default site = 0 +site := num { + # relevent are all the permissions that affect the given unknown object + allow := { x | + perm := input.subject.roles[_].site[_] + perm.action in [input.action, "*"] + perm.resource_type in [input.object.type, "*"] + x := bool_flip(perm.negate) } + num := number(allow) } -# If a user is not part of an organization, 'org_non_member' is set to true -org_non_member { - input.object.org_owner != "" - # Identify if the user is in the org - roles := input.subject.roles - every role in roles { - not role.org[input.object.org_owner] - } +org_members := { orgID | + input.subject.roles[_].org[orgID] } -# org is two rules that equate to the following -# if org_non_member { return {false} } -# else { org_member } -# -# It is important both rules cannot be true, as the `org` rules cannot produce multiple outputs. -default org = {} -org = set { - # We have to do !org_non_member because rego rules must evaluate to 'true' - # to have a value set. - # So we do "not not-org-member" which means "subject is in org" - not org_non_member - set = org_member +default org = 0 +org := num { + orgPerms := { id: num | + id := org_members[_] + set := { x | + perm := input.subject.roles[_].org[id][_] + perm.action in [input.action, "*"] + perm.resource_type in [input.object.type, "*"] + x := bool_flip(perm.negate) + } + num := number(set) + } + + num := orgPerms[input.object.org_owner] } -org = set { - org_non_member - set = {false} +# 'org_mem' is set to true if the user is an org member +# or if the object has no org. +org_mem := true { + input.object.org_owner != "" + input.object.org_owner in org_members } -# The allow block is quite simple. Any set with `false` cascades down in levels. -# Authorization looks for any `allow` statement that is true. Multiple can be true! -# Note that the absence of `allow` means "unauthorized". -# An explicit `"allow": true` is required. - -# site allow -allow { - # No site wide deny - not false in site - # And all permissions are positive - site[_] +org_mem := true { + input.object.org_owner == "" } -# OR +default user = 0 +user := num { + input.subject.id = input.object.owner + # relevent are all the permissions that affect the given unknown object + allow := { x | + perm := input.subject.roles[_].user[_] + perm.action in [input.action, "*"] + perm.resource_type in [input.object.type, "*"] + x := bool_flip(perm.negate) + } + num := number(allow) +} -# org allow +default allow = false +# Site allow { - # No site or org deny - not false in site - not false in org - # And all permissions are positive - org[_] + site = 1 } -# OR +# Org +allow { + not site = -1 + org = 1 +} -# user allow +# User allow { - # No site, org, or user deny - not false in site - not false in org - not false in user - # And all permissions are positive - user[_] + not site = -1 + not org = -1 + org_mem + user = 1 } From 75e3a120b00da967a773e883c6291420775f86f1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 23:02:23 -0500 Subject: [PATCH 02/30] test: Add unit test to verify partial query support --- coderd/rbac/authz.go | 34 ++++++++++++++++++++++++++++++++-- coderd/rbac/authz_test.go | 20 +++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index b9f9e1f825e9e..de7d76b2ac003 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -33,7 +33,8 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub // RegoAuthorizer will use a prepared rego query for performing authorize() type RegoAuthorizer struct { - query rego.PreparedEvalQuery + query rego.PreparedEvalQuery + partial rego.PreparedPartialQuery } // Load the policy from policy.rego in this directory. @@ -49,10 +50,19 @@ func NewAuthorizer() (*RegoAuthorizer, error) { rego.Module("policy.rego", policy), ).PrepareForEval(ctx) + partial, err := rego.New( + rego.Query("allowed = data.authz.allow"), + rego.Module("policy.rego", policy), + rego.Unknowns([]string{ + "input.object.owner", + "input.object.org_owner", + }), + ).PrepareForPartial(ctx) + if err != nil { return nil, xerrors.Errorf("prepare query: %w", err) } - return &RegoAuthorizer{query: query}, nil + return &RegoAuthorizer{query: query, partial: partial}, nil } type authSubject struct { @@ -107,3 +117,23 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ return nil } + +// CheckPartial will not authorize the request. This function is to be used for unit testing to verify the rego policy +// can be converted into ONLY queries. This ensures we can convert the queries into SQL WHERE clauses in the future. +// If this function returns an error, then there is a set of inputs that also returns support rules, which cannot +// be converted. +// This function will not be used to actually perform authorization on partial queries. +func (a RegoAuthorizer) CheckPartial(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*rego.PartialQueries, interface{}, error) { + input := map[string]interface{}{ + "subject": authSubject{ + ID: subjectID, + Roles: roles, + }, + "object": map[string]string{ + "type": objectType, + }, + "action": action, + } + result, err := a.partial.Partial(ctx, rego.EvalInput(input)) + return result, input, err +} diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 4c714e52e5844..a3817f6ec9ae5 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "testing" + "github.com/coder/coder/testutil" + "github.com/google/uuid" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -570,7 +572,9 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes for _, c := range cases { t.Run(name, func(t *testing.T) { for _, a := range c.actions { - err := authorizer.Authorize(context.Background(), subject.UserID, subject.Roles, a, c.resource) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + err := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) if c.allow { if err != nil { var uerr *rbac.UnauthorizedError @@ -593,6 +597,20 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes t.Log(string(d)) } require.Error(t, err, "expected unauthorized") + + // Also check the rego policy can form a valid partial query result. + result, input, err := authorizer.CheckPartial(ctx, subject.UserID, subject.Roles, a, c.resource.Type) + require.NoError(t, err, "check partial") + if len(result.Support) > 0 { + d, _ := json.Marshal(input) + t.Logf("input: %s", string(d)) + for _, q := range result.Queries { + t.Logf("query: %+v", q.String()) + } + for _, s := range result.Support { + t.Logf("support: %+v", s.String()) + } + } } }) } From 6cdf5755012e1035b29ea7d941c4c116269e021d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 00:01:05 -0500 Subject: [PATCH 03/30] Add partial filtering and evaluation Unfortunately this does slow down the unit tests :( --- coderd/rbac/authz.go | 129 +++++++++++++++++++++++++++----------- coderd/rbac/authz_test.go | 25 +++++--- coderd/rbac/fake_test.go | 20 ++++++ coderd/rbac/partial.go | 46 ++++++++++++++ 4 files changed, 176 insertions(+), 44 deletions(-) create mode 100644 coderd/rbac/partial.go diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index de7d76b2ac003..fd26a016e41aa 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -11,6 +11,11 @@ import ( type Authorizer interface { ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error + PrepareByRoleName(ctx context.Context, subjectID string, roles []string, action Action, objectType string) (PreparedAuthorized, error) +} + +type PreparedAuthorized interface { + Authorize(ctx context.Context, object Object) error } // Filter takes in a list of objects, and will filter the list removing all @@ -31,10 +36,26 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub return filtered } +func FilterPart[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) []O { + filtered := make([]O, 0) + prepared, err := auth.PrepareByRoleName(ctx, subjID, subjRoles, action, objectType) + if err != nil { + return filtered + } + + for i := range objects { + object := objects[i] + err := prepared.Authorize(ctx, object.RBACObject()) + if err == nil { + filtered = append(filtered, object) + } + } + return filtered +} + // RegoAuthorizer will use a prepared rego query for performing authorize() type RegoAuthorizer struct { - query rego.PreparedEvalQuery - partial rego.PreparedPartialQuery + query rego.PreparedEvalQuery } // Load the policy from policy.rego in this directory. @@ -46,23 +67,14 @@ func NewAuthorizer() (*RegoAuthorizer, error) { query, err := rego.New( // allowed is the `allow` field from the prepared query. This is the field to check if authorization is // granted. - rego.Query("allowed = data.authz.allow"), + rego.Query("data.authz.allow"), rego.Module("policy.rego", policy), ).PrepareForEval(ctx) - partial, err := rego.New( - rego.Query("allowed = data.authz.allow"), - rego.Module("policy.rego", policy), - rego.Unknowns([]string{ - "input.object.owner", - "input.object.org_owner", - }), - ).PrepareForPartial(ctx) - if err != nil { return nil, xerrors.Errorf("prepare query: %w", err) } - return &RegoAuthorizer{query: query, partial: partial}, nil + return &RegoAuthorizer{query: query}, nil } type authSubject struct { @@ -74,14 +86,11 @@ type authSubject struct { // This is the function intended to be used outside this package. // The role is fetched from the builtin map located in memory. func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error { - roles := make([]Role, 0, len(roleNames)) - for _, n := range roleNames { - r, err := RoleByName(n) - if err != nil { - return xerrors.Errorf("get role permissions: %w", err) - } - roles = append(roles, r) + roles, err := a.rolesByNames(roleNames) + if err != nil { + return err } + return a.Authorize(ctx, subjectID, roles, action, object) } @@ -102,28 +111,23 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ return ForbiddenWithInternal(xerrors.Errorf("eval rego: %w", err), input, results) } - if len(results) != 1 { - return ForbiddenWithInternal(xerrors.Errorf("expect only 1 result, got %d", len(results)), input, results) + if !results.Allowed() { + return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results) } - allowedResult, ok := (results[0].Bindings["allowed"]).(bool) - if !ok { - return ForbiddenWithInternal(xerrors.Errorf("expected allowed to be a bool but got %T", allowedResult), input, results) - } + return nil +} - if !allowedResult { - return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results) +func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) { + roles, err := a.rolesByNames(roleNames) + if err != nil { + return nil, err } - return nil + return a.Prepare(ctx, subjectID, roles, action, objectType) } -// CheckPartial will not authorize the request. This function is to be used for unit testing to verify the rego policy -// can be converted into ONLY queries. This ensures we can convert the queries into SQL WHERE clauses in the future. -// If this function returns an error, then there is a set of inputs that also returns support rules, which cannot -// be converted. -// This function will not be used to actually perform authorization on partial queries. -func (a RegoAuthorizer) CheckPartial(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*rego.PartialQueries, interface{}, error) { +func (a RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*partialAuthorizer, error) { input := map[string]interface{}{ "subject": authSubject{ ID: subjectID, @@ -134,6 +138,57 @@ func (a RegoAuthorizer) CheckPartial(ctx context.Context, subjectID string, role }, "action": action, } - result, err := a.partial.Partial(ctx, rego.EvalInput(input)) - return result, input, err + + rego := rego.New( + rego.Query("data.authz.allow"), + rego.Module("policy.rego", policy), + rego.Unknowns([]string{ + "input.object.owner", + "input.object.org_owner", + }), + rego.Input(input), + ) + + auth, err := newPartialAuthorizer(ctx, rego, input) + if err != nil { + return nil, xerrors.Errorf("new partial authorizer: %w", err) + } + + return auth, nil +} + +func (a RegoAuthorizer) rolesByNames(roleNames []string) ([]Role, error) { + roles := make([]Role, 0, len(roleNames)) + for _, n := range roleNames { + r, err := RoleByName(n) + if err != nil { + return nil, xerrors.Errorf("get role permissions: %w", err) + } + roles = append(roles, r) + } + return roles, nil +} + +// CheckPartial will not authorize the request. This function is to be used for unit testing to verify the rego policy +// can be converted into ONLY queries. This ensures we can convert the queries into SQL WHERE clauses in the future. +// If this function returns an error, then there is a set of inputs that also returns support rules, which cannot +// be converted. +// Unfortunately we cannot reuse a.Prepare for this purpose as a Partial() requires an expression, +// whereas a PartialResult() requires a reference. It's an annoying detail that we need to work around. +func (a RegoAuthorizer) CheckPartial(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*rego.PartialQueries, interface{}, error) { + partAuth, err := a.Prepare(ctx, subjectID, roles, action, objectType) + if err != nil { + return nil, nil, err + } + + result, err := rego.New( + rego.Query("true = data.authz.allow"), + rego.Module("policy.rego", policy), + rego.Unknowns([]string{ + "input.object.owner", + "input.object.org_owner", + }), + rego.Input(partAuth.Input), + ).Partial(ctx) + return result, partAuth.Input, err } diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index a3817f6ec9ae5..4e6277b067f7a 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -574,21 +574,21 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes for _, a := range c.actions { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - err := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) + authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) if c.allow { - if err != nil { + if authError != nil { var uerr *rbac.UnauthorizedError - xerrors.As(err, &uerr) + xerrors.As(authError, &uerr) d, _ := json.Marshal(uerr.Input()) t.Logf("input: %s", string(d)) t.Logf("internal error: %+v", uerr.Internal().Error()) t.Logf("output: %+v", uerr.Output()) } - require.NoError(t, err, "expected no error for testcase action %s", a) + require.NoError(t, authError, "expected no error for testcase action %s", a) continue } - if err == nil { + if authError == nil { d, _ := json.Marshal(map[string]interface{}{ "subject": subject, "object": c.resource, @@ -596,11 +596,11 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes }) t.Log(string(d)) } - require.Error(t, err, "expected unauthorized") + require.Error(t, authError, "expected unauthorized") // Also check the rego policy can form a valid partial query result. result, input, err := authorizer.CheckPartial(ctx, subject.UserID, subject.Roles, a, c.resource.Type) - require.NoError(t, err, "check partial") + require.NoError(t, err, "run partial") if len(result.Support) > 0 { d, _ := json.Marshal(input) t.Logf("input: %s", string(d)) @@ -611,6 +611,17 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes t.Logf("support: %+v", s.String()) } } + require.Equal(t, 0, len(result.Support), "expected 0 support rules") + + partialAuther, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) + require.NoError(t, err, "make prepared authorizer") + + partialErr := partialAuther.Authorize(ctx, c.resource) + if authError != nil { + require.Error(t, partialErr, "partial error blocked valid request (false negative)") + } else { + require.NoError(t, partialErr, "partial allowed invalid request (false positive)") + } } }) } diff --git a/coderd/rbac/fake_test.go b/coderd/rbac/fake_test.go index e8f6a90394cc7..d4041e8656652 100644 --- a/coderd/rbac/fake_test.go +++ b/coderd/rbac/fake_test.go @@ -13,3 +13,23 @@ type fakeAuthorizer struct { func (f fakeAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { return f.AuthFunc(ctx, subjectID, roleNames, action, object) } + +func (f fakeAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roles []string, action rbac.Action, objectType string) (rbac.PreparedAuthorized, error) { + return &fakePreparedAuthorizer{ + Original: f, + SubjectID: subjectID, + Roles: roles, + Action: action, + }, nil +} + +type fakePreparedAuthorizer struct { + Original rbac.Authorizer + SubjectID string + Roles []string + Action rbac.Action +} + +func (f fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error { + return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object) +} diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go new file mode 100644 index 0000000000000..ed0f271be3916 --- /dev/null +++ b/coderd/rbac/partial.go @@ -0,0 +1,46 @@ +package rbac + +import ( + "context" + + "golang.org/x/xerrors" + + "github.com/open-policy-agent/opa/rego" +) + +type partialAuthorizer struct { + // PartialRego is mainly used for unit testing. It is the rego source policy. + PartialRego *rego.Rego + PartialResult rego.PartialResult + Input map[string]interface{} +} + +func newPartialAuthorizer(ctx context.Context, partialRego *rego.Rego, input map[string]interface{}) (*partialAuthorizer, error) { + pResult, err := partialRego.PartialResult(ctx) + if err != nil { + return nil, xerrors.Errorf("partial results: %w", err) + } + + return &partialAuthorizer{ + PartialRego: partialRego, + PartialResult: pResult, + Input: input, + }, nil +} + +// Authorize authorizes a single object +func (a partialAuthorizer) Authorize(ctx context.Context, object Object) error { + results, err := a.PartialResult.Rego(rego.Input( + map[string]interface{}{ + "object": object, + })).Eval(ctx) + if err != nil { + return ForbiddenWithInternal(xerrors.Errorf("eval prepared"), a.Input, results) + } + + if !results.Allowed() { + return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), a.Input, results) + } + + return nil +} From f5eacd0de959a8106500e874938dd948f28812ef Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 00:10:36 -0500 Subject: [PATCH 04/30] Benmark Partials --- coderd/rbac/builtin_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index 9026dd7b213eb..088e8623d1210 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -75,7 +75,7 @@ func BenchmarkRBACFilter(b *testing.B) { b.Run(c.Name, func(b *testing.B) { objects := benchmarkSetup(orgs, users, b.N) b.ResetTimer() - allowed := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects) + allowed := rbac.FilterPart(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) var _ = allowed }) } From 4a7c68e1982af6f9da7d8204920dfa49ea0ece0e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 09:46:00 -0500 Subject: [PATCH 05/30] Linting --- coderd/coderd_test.go | 28 ++++++++++++++++++---------- coderd/rbac/auth_fake.go | 31 +++++++++++++++++++++++++++++++ coderd/rbac/authz.go | 22 +++++----------------- coderd/rbac/authz_test.go | 4 ++-- coderd/rbac/builtin.go | 12 ++++++++++++ coderd/rbac/fake_test.go | 35 ----------------------------------- coderd/rbac/partial.go | 8 ++++---- 7 files changed, 72 insertions(+), 68 deletions(-) create mode 100644 coderd/rbac/auth_fake.go delete mode 100644 coderd/rbac/fake_test.go diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 29f3a0a605fbb..2ca8b9ba6e93d 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -63,7 +63,7 @@ func TestBuildInfo(t *testing.T) { // TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered. func TestAuthorizeAllEndpoints(t *testing.T) { t.Parallel() - authorizer := &fakeAuthorizer{} + authorizer := newRecordingAuthorizer() ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -563,21 +563,29 @@ type authCall struct { Object rbac.Object } -type fakeAuthorizer struct { +type recordingAuthorizer struct { + *rbac.FakeAuthorizer Called *authCall AlwaysReturn error } -func (f *fakeAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { - f.Called = &authCall{ - SubjectID: subjectID, - Roles: roleNames, - Action: action, - Object: object, +func newRecordingAuthorizer() recordingAuthorizer { + r := recordingAuthorizer{} + // Use the fake authorizer by rbac to handle prepared authorizers. + r.FakeAuthorizer = &rbac.FakeAuthorizer{ + AuthFunc: func(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { + r.Called = &authCall{ + SubjectID: subjectID, + Roles: roleNames, + Action: action, + Object: object, + } + return r.AlwaysReturn + }, } - return f.AlwaysReturn + return r } -func (f *fakeAuthorizer) reset() { +func (f *recordingAuthorizer) reset() { f.Called = nil } diff --git a/coderd/rbac/auth_fake.go b/coderd/rbac/auth_fake.go new file mode 100644 index 0000000000000..a85080f2919d3 --- /dev/null +++ b/coderd/rbac/auth_fake.go @@ -0,0 +1,31 @@ +package rbac + +import "context" + +type FakeAuthorizer struct { + AuthFunc func(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error +} + +func (f FakeAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error { + return f.AuthFunc(ctx, subjectID, roleNames, action, object) +} + +func (f FakeAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, action Action, _ string) (PreparedAuthorized, error) { + return &fakePreparedAuthorizer{ + Original: f, + SubjectID: subjectID, + Roles: roles, + Action: action, + }, nil +} + +type fakePreparedAuthorizer struct { + Original Authorizer + SubjectID string + Roles []string + Action Action +} + +func (f fakePreparedAuthorizer) Authorize(ctx context.Context, object Object) error { + return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object) +} diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index fd26a016e41aa..cb1354367050f 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -86,7 +86,7 @@ type authSubject struct { // This is the function intended to be used outside this package. // The role is fetched from the builtin map located in memory. func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error { - roles, err := a.rolesByNames(roleNames) + roles, err := RolesByNames(roleNames) if err != nil { return err } @@ -119,7 +119,7 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ } func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) { - roles, err := a.rolesByNames(roleNames) + roles, err := RolesByNames(roleNames) if err != nil { return nil, err } @@ -127,7 +127,7 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, return a.Prepare(ctx, subjectID, roles, action, objectType) } -func (a RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*partialAuthorizer, error) { +func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { input := map[string]interface{}{ "subject": authSubject{ ID: subjectID, @@ -139,7 +139,7 @@ func (a RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []R "action": action, } - rego := rego.New( + regoPolicy := rego.New( rego.Query("data.authz.allow"), rego.Module("policy.rego", policy), rego.Unknowns([]string{ @@ -149,7 +149,7 @@ func (a RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []R rego.Input(input), ) - auth, err := newPartialAuthorizer(ctx, rego, input) + auth, err := newPartialAuthorizer(ctx, regoPolicy, input) if err != nil { return nil, xerrors.Errorf("new partial authorizer: %w", err) } @@ -157,18 +157,6 @@ func (a RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []R return auth, nil } -func (a RegoAuthorizer) rolesByNames(roleNames []string) ([]Role, error) { - roles := make([]Role, 0, len(roleNames)) - for _, n := range roleNames { - r, err := RoleByName(n) - if err != nil { - return nil, xerrors.Errorf("get role permissions: %w", err) - } - roles = append(roles, r) - } - return roles, nil -} - // CheckPartial will not authorize the request. This function is to be used for unit testing to verify the rego policy // can be converted into ONLY queries. This ensures we can convert the queries into SQL WHERE clauses in the future. // If this function returns an error, then there is a set of inputs that also returns support rules, which cannot diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 4e6277b067f7a..3af41276c7a2a 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -97,7 +97,7 @@ func TestFilter(t *testing.T) { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - authorizer := fakeAuthorizer{ + authorizer := rbac.FakeAuthorizer{ AuthFunc: func(_ context.Context, _ string, _ []string, _ rbac.Action, object rbac.Object) error { return c.Auth(object) }, @@ -573,7 +573,7 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes t.Run(name, func(t *testing.T) { for _, a := range c.actions { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() + t.Cleanup(cancel) authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) if c.allow { if authError != nil { diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index cd51d88361636..539d74275d417 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -221,6 +221,18 @@ func RoleByName(name string) (Role, error) { return role, nil } +func RolesByNames(roleNames []string) ([]Role, error) { + roles := make([]Role, 0, len(roleNames)) + for _, n := range roleNames { + r, err := RoleByName(n) + if err != nil { + return nil, xerrors.Errorf("get role permissions: %w", err) + } + roles = append(roles, r) + } + return roles, nil +} + func IsOrgRole(roleName string) (string, bool) { _, orgID, err := roleSplit(roleName) if err == nil && orgID != "" { diff --git a/coderd/rbac/fake_test.go b/coderd/rbac/fake_test.go deleted file mode 100644 index d4041e8656652..0000000000000 --- a/coderd/rbac/fake_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package rbac_test - -import ( - "context" - - "github.com/coder/coder/coderd/rbac" -) - -type fakeAuthorizer struct { - AuthFunc func(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error -} - -func (f fakeAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { - return f.AuthFunc(ctx, subjectID, roleNames, action, object) -} - -func (f fakeAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roles []string, action rbac.Action, objectType string) (rbac.PreparedAuthorized, error) { - return &fakePreparedAuthorizer{ - Original: f, - SubjectID: subjectID, - Roles: roles, - Action: action, - }, nil -} - -type fakePreparedAuthorizer struct { - Original rbac.Authorizer - SubjectID string - Roles []string - Action rbac.Action -} - -func (f fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error { - return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object) -} diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index ed0f271be3916..f6e0bf490b35b 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -8,20 +8,20 @@ import ( "github.com/open-policy-agent/opa/rego" ) -type partialAuthorizer struct { +type PartialAuthorizer struct { // PartialRego is mainly used for unit testing. It is the rego source policy. PartialRego *rego.Rego PartialResult rego.PartialResult Input map[string]interface{} } -func newPartialAuthorizer(ctx context.Context, partialRego *rego.Rego, input map[string]interface{}) (*partialAuthorizer, error) { +func newPartialAuthorizer(ctx context.Context, partialRego *rego.Rego, input map[string]interface{}) (*PartialAuthorizer, error) { pResult, err := partialRego.PartialResult(ctx) if err != nil { return nil, xerrors.Errorf("partial results: %w", err) } - return &partialAuthorizer{ + return &PartialAuthorizer{ PartialRego: partialRego, PartialResult: pResult, Input: input, @@ -29,7 +29,7 @@ func newPartialAuthorizer(ctx context.Context, partialRego *rego.Rego, input map } // Authorize authorizes a single object -func (a partialAuthorizer) Authorize(ctx context.Context, object Object) error { +func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { results, err := a.PartialResult.Rego(rego.Input( map[string]interface{}{ "object": object, From df75be5ebf08ffb9e8d3db49c65d5eaa4b98dd02 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 10:26:53 -0500 Subject: [PATCH 06/30] Spelling --- coderd/rbac/authz_test.go | 4 ++-- coderd/rbac/policy.rego | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 3af41276c7a2a..10b483f8d85c9 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -613,10 +613,10 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes } require.Equal(t, 0, len(result.Support), "expected 0 support rules") - partialAuther, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) + partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) require.NoError(t, err, "make prepared authorizer") - partialErr := partialAuther.Authorize(ctx, c.resource) + partialErr := partialAuthz.Authorize(ctx, c.resource) if authError != nil { require.Error(t, partialErr, "partial error blocked valid request (false negative)") } else { diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 68af7ae7a59c1..7a9b7bb72fb9b 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -37,7 +37,6 @@ number(set) = c { default site = 0 site := num { - # relevent are all the permissions that affect the given unknown object allow := { x | perm := input.subject.roles[_].site[_] perm.action in [input.action, "*"] @@ -81,7 +80,6 @@ org_mem := true { default user = 0 user := num { input.subject.id = input.object.owner - # relevent are all the permissions that affect the given unknown object allow := { x | perm := input.subject.roles[_].user[_] perm.action in [input.action, "*"] From e90ac2d570efb2673197d538010e7873693b7d36 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 10:59:06 -0500 Subject: [PATCH 07/30] Fix recording fake authorizer --- coderd/coderd_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 2ca8b9ba6e93d..93be45e3fc4d5 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -569,8 +569,8 @@ type recordingAuthorizer struct { AlwaysReturn error } -func newRecordingAuthorizer() recordingAuthorizer { - r := recordingAuthorizer{} +func newRecordingAuthorizer() *recordingAuthorizer { + r := &recordingAuthorizer{} // Use the fake authorizer by rbac to handle prepared authorizers. r.FakeAuthorizer = &rbac.FakeAuthorizer{ AuthFunc: func(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { From 1e774e0a5a0fcbf66ed9724aaba42c85bf18c429 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 12:53:59 -0500 Subject: [PATCH 08/30] Fix partial query tests --- coderd/rbac/authz_test.go | 175 +++++++++++++++++++------------------- 1 file changed, 89 insertions(+), 86 deletions(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 10b483f8d85c9..b2b9da502087f 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -5,13 +5,15 @@ import ( "encoding/json" "testing" + "golang.org/x/xerrors" + + "github.com/coder/coder/cryptorand" + "github.com/coder/coder/testutil" + "github.com/coder/coder/coderd/rbac" "github.com/google/uuid" "github.com/stretchr/testify/require" - "golang.org/x/xerrors" - - "github.com/coder/coder/coderd/rbac" ) // subject is required because rego needs @@ -23,89 +25,90 @@ type subject struct { Roles []rbac.Role `json:"roles"` } -func TestFilter(t *testing.T) { - t.Parallel() - - objectList := make([]rbac.Object, 0) - workspaceList := make([]rbac.Object, 0) - fileList := make([]rbac.Object, 0) - for i := 0; i < 10; i++ { - workspace := rbac.ResourceWorkspace.WithOwner("me") - file := rbac.ResourceFile.WithOwner("me") - - workspaceList = append(workspaceList, workspace) - fileList = append(fileList, file) +type fakeObject struct { + Owner uuid.UUID + OrgOwner uuid.UUID + Type string + Allowed bool +} - objectList = append(objectList, workspace) - objectList = append(objectList, file) +func (w fakeObject) RBACObject() rbac.Object { + return rbac.Object{ + Owner: w.Owner.String(), + OrgID: w.OrgOwner.String(), + Type: w.Type, } +} - // copyList is to prevent tests from sharing the same slice - copyList := func(list []rbac.Object) []rbac.Object { - tmp := make([]rbac.Object, len(list)) - copy(tmp, list) - return tmp +// TestFilter ensures the filter acts the same as an individual authorize. +func TestFilter(t *testing.T) { + orgIDs := make([]uuid.UUID, 10) + userIDs := make([]uuid.UUID, len(orgIDs)) + for i := range orgIDs { + orgIDs[i] = uuid.New() + userIDs[i] = uuid.New() + } + objects := make([]fakeObject, 100) + for i := range objects { + objects[i] = fakeObject{ + Owner: userIDs[must(cryptorand.Intn(len(userIDs)))], + OrgOwner: orgIDs[must(cryptorand.Intn(len(orgIDs)))], + Type: rbac.ResourceWorkspace.Type, + Allowed: false, + } } testCases := []struct { - Name string - List []rbac.Object - Expected []rbac.Object - Auth func(o rbac.Object) error + Name string + SubjectID string + Roles []string + Action rbac.Action + ObjectType string }{ { - Name: "FilterWorkspaceType", - List: copyList(objectList), - Expected: copyList(workspaceList), - Auth: func(o rbac.Object) error { - if o.Type != rbac.ResourceWorkspace.Type { - return xerrors.New("only workspace") - } - return nil - }, - }, - { - Name: "FilterFileType", - List: copyList(objectList), - Expected: copyList(fileList), - Auth: func(o rbac.Object) error { - if o.Type != rbac.ResourceFile.Type { - return xerrors.New("only file") - } - return nil - }, - }, - { - Name: "FilterAll", - List: copyList(objectList), - Expected: []rbac.Object{}, - Auth: func(o rbac.Object) error { - return xerrors.New("always fail") - }, + Name: "NoRoles", + SubjectID: userIDs[0].String(), + Roles: []string{}, + ObjectType: rbac.ResourceWorkspace.Type, + Action: rbac.ActionRead, }, { - Name: "FilterNone", - List: copyList(objectList), - Expected: copyList(objectList), - Auth: func(o rbac.Object) error { - return nil - }, + Name: "Admin", + SubjectID: userIDs[0].String(), + Roles: []string{rbac.RoleOrgMember(orgIDs[0]), "auditor", rbac.RoleAdmin(), rbac.RoleMember()}, + ObjectType: rbac.ResourceWorkspace.Type, + Action: rbac.ActionRead, }, } - for _, c := range testCases { - c := c - t.Run(c.Name, func(t *testing.T) { - t.Parallel() - authorizer := rbac.FakeAuthorizer{ - AuthFunc: func(_ context.Context, _ string, _ []string, _ rbac.Action, object rbac.Object) error { - return c.Auth(object) - }, + for _, tc := range testCases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + localObjects := make([]fakeObject, len(objects)) + copy(localObjects, objects) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + auth, err := rbac.NewAuthorizer() + require.NoError(t, err, "new auth") + + // Run auth 1 by 1 + var allowedCount int + for i, obj := range localObjects { + obj.Type = tc.ObjectType + err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, rbac.ActionRead, obj.RBACObject()) + localObjects[i].Allowed = err == nil + if err == nil { + allowedCount++ + } } - filtered := rbac.Filter(context.Background(), authorizer, "me", []string{}, rbac.ActionRead, c.List) - require.ElementsMatch(t, c.Expected, filtered, "expect same list") - require.Equal(t, len(c.Expected), len(filtered), "same length list") + // Run by filter + list := rbac.FilterPart(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects) + require.Equal(t, allowedCount, len(list), "expected number of allowed") + for _, obj := range list { + require.True(t, obj.Allowed, "expected allowed") + } }) } } @@ -575,20 +578,15 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) t.Cleanup(cancel) authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) - if c.allow { - if authError != nil { - var uerr *rbac.UnauthorizedError - xerrors.As(authError, &uerr) - d, _ := json.Marshal(uerr.Input()) - t.Logf("input: %s", string(d)) - t.Logf("internal error: %+v", uerr.Internal().Error()) - t.Logf("output: %+v", uerr.Output()) - } - require.NoError(t, authError, "expected no error for testcase action %s", a) - continue - } - - if authError == nil { + // Logging only + if authError != nil { + var uerr *rbac.UnauthorizedError + xerrors.As(authError, &uerr) + d, _ := json.Marshal(uerr.Input()) + t.Logf("input: %s", string(d)) + t.Logf("internal error: %+v", uerr.Internal().Error()) + t.Logf("output: %+v", uerr.Output()) + } else { d, _ := json.Marshal(map[string]interface{}{ "subject": subject, "object": c.resource, @@ -596,7 +594,12 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes }) t.Log(string(d)) } - require.Error(t, authError, "expected unauthorized") + + if c.allow { + require.NoError(t, authError, "expected no error for testcase action %s", a) + } else { + require.Error(t, authError, "expected unauthorized") + } // Also check the rego policy can form a valid partial query result. result, input, err := authorizer.CheckPartial(ctx, subject.UserID, subject.Roles, a, c.resource.Type) From 38917dc174a21b4eeb2d02546fab3434f64dde1b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 13:55:37 -0500 Subject: [PATCH 09/30] Working partial execution to queries --- coderd/rbac/authz.go | 12 +++-- coderd/rbac/authz_test.go | 49 ++++++++++++++++++++- coderd/rbac/builtin_test.go | 6 ++- coderd/rbac/partial.go | 87 +++++++++++++++++++++++++++++-------- coderd/rbac/policy.rego | 1 + 5 files changed, 129 insertions(+), 26 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index cb1354367050f..bb22e8b821950 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -36,21 +36,25 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub return filtered } -func FilterPart[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) []O { +func FilterPart[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) ([]O, error) { filtered := make([]O, 0) prepared, err := auth.PrepareByRoleName(ctx, subjID, subjRoles, action, objectType) if err != nil { - return filtered + return nil, xerrors.Errorf("prepare: %w", err) } for i := range objects { object := objects[i] - err := prepared.Authorize(ctx, object.RBACObject()) + rbacObj := object.RBACObject() + if rbacObj.Type != objectType { + return nil, xerrors.Errorf("object types must be %s, found %s", objectType, object.RBACObject().Type) + } + err := prepared.Authorize(ctx, rbacObj) if err == nil { filtered = append(filtered, object) } } - return filtered + return filtered, nil } // RegoAuthorizer will use a prepared rego query for performing authorize() diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index b2b9da502087f..3b7f88f839096 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -42,6 +42,8 @@ func (w fakeObject) RBACObject() rbac.Object { // TestFilter ensures the filter acts the same as an individual authorize. func TestFilter(t *testing.T) { + t.Parallel() + orgIDs := make([]uuid.UUID, 10) userIDs := make([]uuid.UUID, len(orgIDs)) for i := range orgIDs { @@ -79,11 +81,39 @@ func TestFilter(t *testing.T) { ObjectType: rbac.ResourceWorkspace.Type, Action: rbac.ActionRead, }, + { + Name: "OrgAdmin", + SubjectID: userIDs[0].String(), + Roles: []string{rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgAdmin(orgIDs[0]), rbac.RoleMember()}, + ObjectType: rbac.ResourceWorkspace.Type, + Action: rbac.ActionRead, + }, + { + Name: "OrgMember", + SubjectID: userIDs[0].String(), + Roles: []string{rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgMember(orgIDs[1]), rbac.RoleMember()}, + ObjectType: rbac.ResourceWorkspace.Type, + Action: rbac.ActionRead, + }, + { + Name: "ManyRoles", + SubjectID: userIDs[0].String(), + Roles: []string{ + rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgAdmin(orgIDs[0]), + rbac.RoleOrgMember(orgIDs[1]), rbac.RoleOrgAdmin(orgIDs[1]), + rbac.RoleOrgMember(orgIDs[2]), rbac.RoleOrgAdmin(orgIDs[2]), + rbac.RoleMember(), + }, + ObjectType: rbac.ResourceWorkspace.Type, + Action: rbac.ActionRead, + }, } for _, tc := range testCases { tc := tc t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + localObjects := make([]fakeObject, len(objects)) copy(localObjects, objects) @@ -104,7 +134,8 @@ func TestFilter(t *testing.T) { } // Run by filter - list := rbac.FilterPart(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects) + list, err := rbac.FilterPart(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects) + require.NoError(t, err) require.Equal(t, allowedCount, len(list), "expected number of allowed") for _, obj := range list { require.True(t, obj.Allowed, "expected allowed") @@ -113,6 +144,22 @@ func TestFilter(t *testing.T) { } } +func TestOne(t *testing.T) { + defOrg := uuid.New() + + user := subject{ + UserID: "me", + Roles: []rbac.Role{ + must(rbac.RoleByName(rbac.RoleMember())), + must(rbac.RoleByName(rbac.RoleOrgMember(defOrg))), + }, + } + testAuthorize(t, "Member", user, []authTestCase{ + // Org + me + {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + }) +} + // TestAuthorizeDomain test the very basic roles that are commonly used. func TestAuthorizeDomain(t *testing.T) { t.Parallel() diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index 088e8623d1210..be7ffa572dcc2 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -62,7 +62,8 @@ func BenchmarkRBACFilter(b *testing.B) { rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), rbac.RoleOrgMember(orgs[1]), rbac.RoleOrgAdmin(orgs[1]), rbac.RoleOrgMember(orgs[2]), rbac.RoleOrgAdmin(orgs[2]), - rbac.RoleMember()}, + rbac.RoleMember(), + }, UserID: users[0], }, } @@ -75,7 +76,8 @@ func BenchmarkRBACFilter(b *testing.B) { b.Run(c.Name, func(b *testing.B) { objects := benchmarkSetup(orgs, users, b.N) b.ResetTimer() - allowed := rbac.FilterPart(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) + allowed, err := rbac.FilterPart(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) + require.NoError(b, err) var _ = allowed }) } diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index f6e0bf490b35b..bf9a3674c4892 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -10,37 +10,86 @@ import ( type PartialAuthorizer struct { // PartialRego is mainly used for unit testing. It is the rego source policy. - PartialRego *rego.Rego - PartialResult rego.PartialResult - Input map[string]interface{} + PartialRego *rego.Rego + PartialQueries *rego.PartialQueries + PreparedQueries []rego.PreparedEvalQuery + AlwaysTrue bool + Input map[string]interface{} } -func newPartialAuthorizer(ctx context.Context, partialRego *rego.Rego, input map[string]interface{}) (*PartialAuthorizer, error) { - pResult, err := partialRego.PartialResult(ctx) +func newPartialAuthorizer(ctx context.Context, _ *rego.Rego, input map[string]interface{}) (*PartialAuthorizer, error) { + partialQueries, err := rego.New( + rego.Query("true = data.authz.allow"), + rego.Module("policy.rego", policy), + rego.Unknowns([]string{ + "input.object.owner", + "input.object.org_owner", + }), + rego.Input(input), + ).Partial(ctx) if err != nil { - return nil, xerrors.Errorf("partial results: %w", err) + return nil, xerrors.Errorf("prepare: %w", err) + } + + alwaysTrue := false + preparedQueries := make([]rego.PreparedEvalQuery, 0, len(partialQueries.Queries)) + for _, q := range partialQueries.Queries { + if q.String() == "" { + alwaysTrue = true + continue + } + results, err := rego.New( + rego.ParsedQuery(q), + ).PrepareForEval(ctx) + if err != nil { + return nil, xerrors.Errorf("prepare query %s: %w", q.String(), err) + } + preparedQueries = append(preparedQueries, results) } return &PartialAuthorizer{ - PartialRego: partialRego, - PartialResult: pResult, - Input: input, + PartialQueries: partialQueries, + PreparedQueries: preparedQueries, + Input: input, + AlwaysTrue: alwaysTrue, }, nil } -// Authorize authorizes a single object +// Authorize authorizes a single object using teh partially prepared queries. func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { - results, err := a.PartialResult.Rego(rego.Input( - map[string]interface{}{ - "object": object, - })).Eval(ctx) - if err != nil { - return ForbiddenWithInternal(xerrors.Errorf("eval prepared"), a.Input, results) + if a.AlwaysTrue { + return nil } - if !results.Allowed() { - return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), a.Input, results) +EachQueryLoop: + for _, q := range a.PreparedQueries { + results, err := q.Eval(ctx, rego.EvalInput(map[string]interface{}{ + "object": object, + })) + if err != nil { + continue EachQueryLoop + } + + if len(results) == 0 { + continue EachQueryLoop + } + + if len(results) > 1 { + continue EachQueryLoop + } + + if len(results[0].Bindings) > 0 { + continue EachQueryLoop + } + + for _, exp := range results[0].Expressions { + if exp.String() != "true" { + continue EachQueryLoop + } + } + + return nil } - return nil + return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), a.Input, nil) } diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 7a9b7bb72fb9b..bf1c74ce711bb 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -79,6 +79,7 @@ org_mem := true { default user = 0 user := num { + input.object.owner != "" input.subject.id = input.object.owner allow := { x | perm := input.subject.roles[_].user[_] From e139a1f4ee8556e9b7da4c7c85fb951f1f752438 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 14:04:05 -0500 Subject: [PATCH 10/30] Cleanup --- coderd/rbac/authz.go | 47 +-------------------------------------- coderd/rbac/authz_test.go | 36 +++++++++--------------------- coderd/rbac/partial.go | 20 ++++++++++++++--- 3 files changed, 28 insertions(+), 75 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index bb22e8b821950..d020fc126a643 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -132,55 +132,10 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, } func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { - input := map[string]interface{}{ - "subject": authSubject{ - ID: subjectID, - Roles: roles, - }, - "object": map[string]string{ - "type": objectType, - }, - "action": action, - } - - regoPolicy := rego.New( - rego.Query("data.authz.allow"), - rego.Module("policy.rego", policy), - rego.Unknowns([]string{ - "input.object.owner", - "input.object.org_owner", - }), - rego.Input(input), - ) - - auth, err := newPartialAuthorizer(ctx, regoPolicy, input) + auth, err := newPartialAuthorizer(ctx, subjectID, roles, action, objectType) if err != nil { return nil, xerrors.Errorf("new partial authorizer: %w", err) } return auth, nil } - -// CheckPartial will not authorize the request. This function is to be used for unit testing to verify the rego policy -// can be converted into ONLY queries. This ensures we can convert the queries into SQL WHERE clauses in the future. -// If this function returns an error, then there is a set of inputs that also returns support rules, which cannot -// be converted. -// Unfortunately we cannot reuse a.Prepare for this purpose as a Partial() requires an expression, -// whereas a PartialResult() requires a reference. It's an annoying detail that we need to work around. -func (a RegoAuthorizer) CheckPartial(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*rego.PartialQueries, interface{}, error) { - partAuth, err := a.Prepare(ctx, subjectID, roles, action, objectType) - if err != nil { - return nil, nil, err - } - - result, err := rego.New( - rego.Query("true = data.authz.allow"), - rego.Module("policy.rego", policy), - rego.Unknowns([]string{ - "input.object.owner", - "input.object.org_owner", - }), - rego.Input(partAuth.Input), - ).Partial(ctx) - return result, partAuth.Input, err -} diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 3b7f88f839096..ff2fd1ab5dddc 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -144,22 +144,6 @@ func TestFilter(t *testing.T) { } } -func TestOne(t *testing.T) { - defOrg := uuid.New() - - user := subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleMember())), - must(rbac.RoleByName(rbac.RoleOrgMember(defOrg))), - }, - } - testAuthorize(t, "Member", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - }) -} - // TestAuthorizeDomain test the very basic roles that are commonly used. func TestAuthorizeDomain(t *testing.T) { t.Parallel() @@ -648,23 +632,23 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes require.Error(t, authError, "expected unauthorized") } + partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) + require.NoError(t, err, "make prepared authorizer") + // Also check the rego policy can form a valid partial query result. - result, input, err := authorizer.CheckPartial(ctx, subject.UserID, subject.Roles, a, c.resource.Type) - require.NoError(t, err, "run partial") - if len(result.Support) > 0 { - d, _ := json.Marshal(input) + // This ensures we can convert the queries into SQL WHERE clauses in the future. + // If this function returns 'Support' sections, then we cannot convert the query into SQL. + if len(partialAuthz.PartialQueries.Support) > 0 { + d, _ := json.Marshal(partialAuthz.Input) t.Logf("input: %s", string(d)) - for _, q := range result.Queries { + for _, q := range partialAuthz.PartialQueries.Queries { t.Logf("query: %+v", q.String()) } - for _, s := range result.Support { + for _, s := range partialAuthz.PartialQueries.Support { t.Logf("support: %+v", s.String()) } } - require.Equal(t, 0, len(result.Support), "expected 0 support rules") - - partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) - require.NoError(t, err, "make prepared authorizer") + require.Equal(t, 0, len(partialAuthz.PartialQueries.Support), "expected 0 support rules") partialErr := partialAuthz.Authorize(ctx, c.resource) if authError != nil { diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index bf9a3674c4892..3249c29444a8a 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -9,15 +9,25 @@ import ( ) type PartialAuthorizer struct { - // PartialRego is mainly used for unit testing. It is the rego source policy. - PartialRego *rego.Rego + // PartialQueries is mainly used for unit testing. PartialQueries *rego.PartialQueries PreparedQueries []rego.PreparedEvalQuery AlwaysTrue bool Input map[string]interface{} } -func newPartialAuthorizer(ctx context.Context, _ *rego.Rego, input map[string]interface{}) (*PartialAuthorizer, error) { +func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { + input := map[string]interface{}{ + "subject": authSubject{ + ID: subjectID, + Roles: roles, + }, + "object": map[string]string{ + "type": objectType, + }, + "action": action, + } + partialQueries, err := rego.New( rego.Query("true = data.authz.allow"), rego.Module("policy.rego", policy), @@ -70,19 +80,23 @@ EachQueryLoop: continue EachQueryLoop } + // 0 results means the query is false. if len(results) == 0 { continue EachQueryLoop } + // We should never get more than 1 result if len(results) > 1 { continue EachQueryLoop } + // All queries should resolve, we should not have bindings if len(results[0].Bindings) > 0 { continue EachQueryLoop } for _, exp := range results[0].Expressions { + // Any other "true" expressions that are not "true" are not expected. if exp.String() != "true" { continue EachQueryLoop } From c44d4d112a042120fa0d7df467f77c97ab580949 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 14:13:10 -0500 Subject: [PATCH 11/30] Cleanup --- coderd/rbac/authz_test.go | 8 +++----- coderd/rbac/partial.go | 25 ++++++++++++++++--------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index ff2fd1ab5dddc..941c77e9e58db 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -5,15 +5,13 @@ import ( "encoding/json" "testing" + "github.com/google/uuid" + "github.com/stretchr/testify/require" "golang.org/x/xerrors" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/cryptorand" - "github.com/coder/coder/testutil" - - "github.com/coder/coder/coderd/rbac" - "github.com/google/uuid" - "github.com/stretchr/testify/require" ) // subject is required because rego needs diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index 3249c29444a8a..9879048b04662 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -41,12 +41,19 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a return nil, xerrors.Errorf("prepare: %w", err) } - alwaysTrue := false + pAuth := &PartialAuthorizer{ + PartialQueries: partialQueries, + PreparedQueries: []rego.PreparedEvalQuery{}, + Input: input, + } + preparedQueries := make([]rego.PreparedEvalQuery, 0, len(partialQueries.Queries)) for _, q := range partialQueries.Queries { if q.String() == "" { - alwaysTrue = true - continue + // No more work needed, this will always be true, + pAuth.AlwaysTrue = true + preparedQueries = []rego.PreparedEvalQuery{} + break } results, err := rego.New( rego.ParsedQuery(q), @@ -56,13 +63,9 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a } preparedQueries = append(preparedQueries, results) } + pAuth.PreparedQueries = preparedQueries - return &PartialAuthorizer{ - PartialQueries: partialQueries, - PreparedQueries: preparedQueries, - Input: input, - AlwaysTrue: alwaysTrue, - }, nil + return pAuth, nil } // Authorize authorizes a single object using teh partially prepared queries. @@ -73,6 +76,7 @@ func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { EachQueryLoop: for _, q := range a.PreparedQueries { + // We need to eval each query with the newly known fields. results, err := q.Eval(ctx, rego.EvalInput(map[string]interface{}{ "object": object, })) @@ -80,6 +84,9 @@ EachQueryLoop: continue EachQueryLoop } + // The below code is intended to fail safe. We only support queries that + // return simple results. + // 0 results means the query is false. if len(results) == 0 { continue EachQueryLoop From 19f3557e5880b3eb00a8ec80d7e03063f2f24795 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 14:33:54 -0500 Subject: [PATCH 12/30] Add comments --- coderd/rbac/builtin_test.go | 2 +- coderd/rbac/partial.go | 37 +++++++++++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index be7ffa572dcc2..ec89e055a1311 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -73,7 +73,7 @@ func BenchmarkRBACFilter(b *testing.B) { require.NoError(b, err) } for _, c := range benchCases { - b.Run(c.Name, func(b *testing.B) { + b.Run(c.Name+"Queries", func(b *testing.B) { objects := benchmarkSetup(orgs, users, b.N) b.ResetTimer() allowed, err := rbac.FilterPart(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index 9879048b04662..d4b28927da027 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -50,7 +50,10 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a preparedQueries := make([]rego.PreparedEvalQuery, 0, len(partialQueries.Queries)) for _, q := range partialQueries.Queries { if q.String() == "" { - // No more work needed, this will always be true, + // No more work needed. An empty query is the same as + // 'WHERE true' + // This is likely an admin. We don't even need to use rego going + // forward. pAuth.AlwaysTrue = true preparedQueries = []rego.PreparedEvalQuery{} break @@ -74,6 +77,16 @@ func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { return nil } + // How to interpret the results of the partial queries. + // We have a list of queries that are along the lines of: + // `input.object.org_owner = ""; "me" = input.object.owner` + // `input.object.org_owner in {"feda2e52-8bf1-42ce-ad75-6c5595cb297a"} ` + // All these queries are joined by an 'OR'. So we need to run through each + // query, and evaluate it. + // + // In each query, we have a list of the evaluation results, which should be + // all boolean expressions. In the above 1st example, there are 2 boolean + // expressions. These expressions within a single query are `AND` together by rego. EachQueryLoop: for _, q := range a.PreparedQueries { // We need to eval each query with the newly known fields. @@ -84,26 +97,34 @@ EachQueryLoop: continue EachQueryLoop } - // The below code is intended to fail safe. We only support queries that - // return simple results. - - // 0 results means the query is false. + // If there are no results, then the query is false. This is because rego + // treats false queries as "undefined". So if any expression is false, the + // result is an empty list. if len(results) == 0 { continue EachQueryLoop } - // We should never get more than 1 result + // If there is more than 1 result, that means there is more than 1 rule. + // This should not happen, because our query should always be an expression. + // If this every occurs, it is likely the original query was not an expression. if len(results) > 1 { continue EachQueryLoop } - // All queries should resolve, we should not have bindings + // Our queries should be simple, and should not yield any bindings. + // A binding is something like 'x := 1'. This binding as an expression is + // 'true', but in our case is unhelpful. We are not analyzing this ast to + // map bindings. So just error out. Similar to above, our queries should + // always be boolean expressions. if len(results[0].Bindings) > 0 { continue EachQueryLoop } + // We have a valid set of boolean expressions! All expressions are 'AND'd + // together. This is automatic by rego, so we should not actually need to + // inspect this any further. But just in case, we will verify each expression + // did resolve to 'true'. This is purely defensive programming. for _, exp := range results[0].Expressions { - // Any other "true" expressions that are not "true" are not expected. if exp.String() != "true" { continue EachQueryLoop } From f9dd9aa76bb3086a89cbf892acc92e1ab0eb73ba Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 14:46:48 -0500 Subject: [PATCH 13/30] Spelling --- coderd/rbac/partial.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index d4b28927da027..dfd2bf8b0edd0 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -71,7 +71,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a return pAuth, nil } -// Authorize authorizes a single object using teh partially prepared queries. +// Authorize authorizes a single object using the partially prepared queries. func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { if a.AlwaysTrue { return nil From bed9f4faf23823d87c63e6de9544ade22a6b6bae Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 15:01:46 -0500 Subject: [PATCH 14/30] Remove old Filter --- coderd/authorize.go | 22 ++++++++++++++++++++-- coderd/provisionerdaemons.go | 9 ++++++++- coderd/rbac/authz.go | 21 +++------------------ coderd/rbac/authz_test.go | 2 +- coderd/rbac/builtin_test.go | 4 ++-- coderd/templates.go | 9 ++++++++- coderd/users.go | 28 +++++++++++++++++++++++++--- coderd/workspaces.go | 9 ++++++++- 8 files changed, 75 insertions(+), 29 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 7437e436b8ead..f958b1e19bd2d 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -10,9 +10,27 @@ import ( "github.com/coder/coder/coderd/rbac" ) -func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) []O { +func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) ([]O, error) { roles := httpmw.AuthorizationUserRoles(r) - return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) + + if len(objects) == 0 { + return objects, nil + } + objecType := objects[0].RBACObject().Type + objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objecType, objects) + if err != nil { + api.Logger.Error(r.Context(), "filter failed", + slog.Error(err), + slog.F("object_type", objecType), + slog.F("user_id", roles.ID), + slog.F("username", roles.Username), + slog.F("route", r.URL.Path), + slog.F("action", action), + ) + // Hide the underlying error in case it has sensitive information + return nil, xerrors.Errorf("failed to filter requested objects") + } + return objects, nil } // Authorize will return false if the user is not authorized to do the action. diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index ac0ef2e2a3710..56af2eb68c0b1 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -50,7 +50,14 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { if daemons == nil { daemons = []database.ProvisionerDaemon{} } - daemons = AuthorizeFilter(api, r, rbac.ActionRead, daemons) + daemons, err = AuthorizeFilter(api, r, rbac.ActionRead, daemons) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching provisioner daemons.", + Detail: err.Error(), + }) + return + } httpapi.Write(rw, http.StatusOK, daemons) } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index d020fc126a643..ee57bf54dccf8 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -19,24 +19,9 @@ type PreparedAuthorized interface { } // Filter takes in a list of objects, and will filter the list removing all -// the elements the subject does not have permission for. -// Filter does not allocate a new slice, and will use the existing one -// passed in. This can cause memory leaks if the slice is held for a prolonged -// period of time. -func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) []O { - filtered := make([]O, 0) - - for i := range objects { - object := objects[i] - err := auth.ByRoleName(ctx, subjID, subjRoles, action, object.RBACObject()) - if err == nil { - filtered = append(filtered, object) - } - } - return filtered -} - -func FilterPart[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) ([]O, error) { +// the elements the subject does not have permission for. All objects must be +// of the same type. +func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) ([]O, error) { filtered := make([]O, 0) prepared, err := auth.PrepareByRoleName(ctx, subjID, subjRoles, action, objectType) if err != nil { diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 941c77e9e58db..b2c267a1cc7d7 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -132,7 +132,7 @@ func TestFilter(t *testing.T) { } // Run by filter - list, err := rbac.FilterPart(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects) + list, err := rbac.Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects) require.NoError(t, err) require.Equal(t, allowedCount, len(list), "expected number of allowed") for _, obj := range list { diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index ec89e055a1311..cb9f964944011 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -73,10 +73,10 @@ func BenchmarkRBACFilter(b *testing.B) { require.NoError(b, err) } for _, c := range benchCases { - b.Run(c.Name+"Queries", func(b *testing.B) { + b.Run(c.Name, func(b *testing.B) { objects := benchmarkSetup(orgs, users, b.N) b.ResetTimer() - allowed, err := rbac.FilterPart(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) + allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) require.NoError(b, err) var _ = allowed }) diff --git a/coderd/templates.go b/coderd/templates.go index 884b26fabe385..e6bab1c25c927 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -273,7 +273,14 @@ func (api *API) templatesByOrganization(rw http.ResponseWriter, r *http.Request) } // Filter templates based on rbac permissions - templates = AuthorizeFilter(api, r, rbac.ActionRead, templates) + templates, err = AuthorizeFilter(api, r, rbac.ActionRead, templates) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching templates.", + Detail: err.Error(), + }) + return + } templateIDs := make([]uuid.UUID, 0, len(templates)) diff --git a/coderd/users.go b/coderd/users.go index 51509b0cc800e..15ba09dfffcbe 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -155,7 +155,15 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { return } - users = AuthorizeFilter(api, r, rbac.ActionRead, users) + users, err = AuthorizeFilter(api, r, rbac.ActionRead, users) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching users.", + Detail: err.Error(), + }) + return + } + userIDs := make([]uuid.UUID, 0, len(users)) for _, user := range users { userIDs = append(userIDs, user.ID) @@ -489,7 +497,14 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { } // Only include ones we can read from RBAC. - memberships = AuthorizeFilter(api, r, rbac.ActionRead, memberships) + memberships, err = AuthorizeFilter(api, r, rbac.ActionRead, memberships) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching memberships.", + Detail: err.Error(), + }) + return + } for _, mem := range memberships { // If we can read the org member, include the roles. @@ -610,7 +625,14 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) { } // Only return orgs the user can read. - organizations = AuthorizeFilter(api, r, rbac.ActionRead, organizations) + organizations, err = AuthorizeFilter(api, r, rbac.ActionRead, organizations) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organizations.", + Detail: err.Error(), + }) + return + } publicOrganizations := make([]codersdk.Organization, 0, len(organizations)) for _, organization := range organizations { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index fd696d5f0edb1..07fbb27641031 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -145,7 +145,14 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } // Only return workspaces the user can read - workspaces = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) + workspaces, err = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspaces.", + Detail: err.Error(), + }) + return + } apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) if err != nil { From 74d90f492493564a5878142c3be67618e36c4925 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 15:15:15 -0500 Subject: [PATCH 15/30] Improve error message --- coderd/rbac/authz.go | 20 +++++++++++--------- coderd/rbac/authz_test.go | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index ee57bf54dccf8..a22dbc7023f5b 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -32,7 +32,7 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub object := objects[i] rbacObj := object.RBACObject() if rbacObj.Type != objectType { - return nil, xerrors.Errorf("object types must be %s, found %s", objectType, object.RBACObject().Type) + return nil, xerrors.Errorf("object types must be uniform across the set (%s), found %s", objectType, object.RBACObject().Type) } err := prepared.Authorize(ctx, rbacObj) if err == nil { @@ -107,20 +107,22 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ return nil } -func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) { - roles, err := RolesByNames(roleNames) +// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type). +// This will vastly speed up performance if batch authorization on the same type of objects is needed. +func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { + auth, err := newPartialAuthorizer(ctx, subjectID, roles, action, objectType) if err != nil { - return nil, err + return nil, xerrors.Errorf("new partial authorizer: %w", err) } - return a.Prepare(ctx, subjectID, roles, action, objectType) + return auth, nil } -func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { - auth, err := newPartialAuthorizer(ctx, subjectID, roles, action, objectType) +func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) { + roles, err := RolesByNames(roleNames) if err != nil { - return nil, xerrors.Errorf("new partial authorizer: %w", err) + return nil, err } - return auth, nil + return a.Prepare(ctx, subjectID, roles, action, objectType) } diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index b2c267a1cc7d7..33ceaebfb7114 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -38,7 +38,17 @@ func (w fakeObject) RBACObject() rbac.Object { } } +func TestFilterError(t *testing.T) { + auth, err := rbac.NewAuthorizer() + require.NoError(t, err) + + _, err = rbac.Filter(context.Background(), auth, uuid.NewString(), []string{}, rbac.ActionRead, rbac.ResourceWorkspace.Type, []rbac.Object{rbac.ResourceUser}) + require.ErrorContains(t, err, "object types must be uniform") +} + // TestFilter ensures the filter acts the same as an individual authorize. +// It generates a random set of objects, then runs the Filter batch function +// against the singular ByRoleName function. func TestFilter(t *testing.T) { t.Parallel() @@ -100,11 +110,33 @@ func TestFilter(t *testing.T) { rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgAdmin(orgIDs[0]), rbac.RoleOrgMember(orgIDs[1]), rbac.RoleOrgAdmin(orgIDs[1]), rbac.RoleOrgMember(orgIDs[2]), rbac.RoleOrgAdmin(orgIDs[2]), + rbac.RoleOrgMember(orgIDs[4]), + rbac.RoleOrgMember(orgIDs[5]), rbac.RoleMember(), }, ObjectType: rbac.ResourceWorkspace.Type, Action: rbac.ActionRead, }, + { + Name: "SiteMember", + SubjectID: userIDs[0].String(), + Roles: []string{rbac.RoleMember()}, + ObjectType: rbac.ResourceUser.Type, + Action: rbac.ActionRead, + }, + { + Name: "ReadOrgs", + SubjectID: userIDs[0].String(), + Roles: []string{ + rbac.RoleOrgMember(orgIDs[0]), + rbac.RoleOrgMember(orgIDs[1]), + rbac.RoleOrgMember(orgIDs[2]), + rbac.RoleOrgMember(orgIDs[3]), + rbac.RoleMember(), + }, + ObjectType: rbac.ResourceOrganization.Type, + Action: rbac.ActionRead, + }, } for _, tc := range testCases { @@ -125,10 +157,11 @@ func TestFilter(t *testing.T) { for i, obj := range localObjects { obj.Type = tc.ObjectType err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, rbac.ActionRead, obj.RBACObject()) - localObjects[i].Allowed = err == nil + obj.Allowed = err == nil if err == nil { allowedCount++ } + localObjects[i] = obj } // Run by filter From fe0d05a087450e179c8ed79e83510ae36d61f33e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 15:22:15 -0500 Subject: [PATCH 16/30] Comments --- coderd/rbac/partial.go | 19 +++++++++++++------ coderd/rbac/policy.rego | 5 +++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index dfd2bf8b0edd0..c2beae9501698 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -9,11 +9,15 @@ import ( ) type PartialAuthorizer struct { - // PartialQueries is mainly used for unit testing. + // PartialQueries is mainly used for unit testing to assert our rego policy + // can always be compressed into a set of queries. PartialQueries *rego.PartialQueries PreparedQueries []rego.PreparedEvalQuery - AlwaysTrue bool - Input map[string]interface{} + // Input is used purely for debugging and logging. + Input map[string]interface{} + // AlwaysTrue is if the subject can always perform the action on the + // resource type, regardless of the unknown fields. + AlwaysTrue bool } func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { @@ -28,6 +32,8 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a "action": action, } + // Run the rego policy with a few unknown fields. This should simplify our + // policy to a set of queries. partialQueries, err := rego.New( rego.Query("true = data.authz.allow"), rego.Module("policy.rego", policy), @@ -47,6 +53,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a Input: input, } + // Prepare each query to optimize the runtime when we iterate over the objects. preparedQueries := make([]rego.PreparedEvalQuery, 0, len(partialQueries.Queries)) for _, q := range partialQueries.Queries { if q.String() == "" { @@ -84,9 +91,9 @@ func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { // All these queries are joined by an 'OR'. So we need to run through each // query, and evaluate it. // - // In each query, we have a list of the evaluation results, which should be - // all boolean expressions. In the above 1st example, there are 2 boolean - // expressions. These expressions within a single query are `AND` together by rego. + // In each query, we have a list of the expressions, which should be + // all boolean expressions. In the above 1st example, there are 2. + // These expressions within a single query are `AND` together by rego. EachQueryLoop: for _, q := range a.PreparedQueries { // We need to eval each query with the newly known fields. diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index bf1c74ce711bb..90051bae36913 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -1,5 +1,6 @@ package authz import future.keywords +# A great playground: https://play.openpolicyagent.org/ # Helpful cli commands to debug. # opa eval --format=pretty 'data.authz.allow = true' -d policy.rego -i input.json # opa eval --partial --format=pretty 'data.authz.allow = true' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner -i input.json @@ -18,6 +19,10 @@ bool_flip(b) = flipped { flipped = true } +# number is a quick way to get a set of {true, false} and convert it to +# -1: {false, true} or {false} +# 0: {} +# 1: {true} number(set) = c { count(set) == 0 c := 0 From dd5c55cadb504a2c05e0886c4750e1eca81562c0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 15:23:35 -0500 Subject: [PATCH 17/30] Make test parallel --- coderd/rbac/authz_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 33ceaebfb7114..64133b9d130d4 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -39,6 +39,7 @@ func (w fakeObject) RBACObject() rbac.Object { } func TestFilterError(t *testing.T) { + t.Parallel() auth, err := rbac.NewAuthorizer() require.NoError(t, err) From ae22f890ee57026e5f66691193e751f67b74a859 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 16:10:13 -0500 Subject: [PATCH 18/30] Fix typo --- coderd/authorize.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index f958b1e19bd2d..95131b1a4d9be 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -16,12 +16,12 @@ func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Act if len(objects) == 0 { return objects, nil } - objecType := objects[0].RBACObject().Type - objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objecType, objects) + objectType := objects[0].RBACObject().Type + objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objectType, objects) if err != nil { api.Logger.Error(r.Context(), "filter failed", slog.Error(err), - slog.F("object_type", objecType), + slog.F("object_type", objectType), slog.F("user_id", roles.ID), slog.F("username", roles.Username), slog.F("route", r.URL.Path), From 02669631e3d686fb5f64686fb17417fc5700042b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 16:20:48 -0500 Subject: [PATCH 19/30] Drop an arg from Filter --- coderd/authorize.go | 12 +++--------- coderd/rbac/authz.go | 8 +++++++- coderd/rbac/authz_test.go | 4 ++-- coderd/rbac/builtin_test.go | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 95131b1a4d9be..855348aaf6184 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -12,23 +12,17 @@ import ( func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) ([]O, error) { roles := httpmw.AuthorizationUserRoles(r) - - if len(objects) == 0 { - return objects, nil - } - objectType := objects[0].RBACObject().Type - objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objectType, objects) + objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) if err != nil { + // Log the error as Filter should not be erroring. api.Logger.Error(r.Context(), "filter failed", slog.Error(err), - slog.F("object_type", objectType), slog.F("user_id", roles.ID), slog.F("username", roles.Username), slog.F("route", r.URL.Path), slog.F("action", action), ) - // Hide the underlying error in case it has sensitive information - return nil, xerrors.Errorf("failed to filter requested objects") + return nil, err } return objects, nil } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index a22dbc7023f5b..d970276860177 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -21,7 +21,13 @@ type PreparedAuthorized interface { // 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. -func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) ([]O, error) { +func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) ([]O, error) { + if len(objects) == 0 { + // Nothing to filter + return objects, nil + } + objectType := objects[0].RBACObject().Type + filtered := make([]O, 0) prepared, err := auth.PrepareByRoleName(ctx, subjID, subjRoles, action, objectType) if err != nil { diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 64133b9d130d4..4ed24d6b322bc 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -43,7 +43,7 @@ func TestFilterError(t *testing.T) { auth, err := rbac.NewAuthorizer() require.NoError(t, err) - _, err = rbac.Filter(context.Background(), auth, uuid.NewString(), []string{}, rbac.ActionRead, rbac.ResourceWorkspace.Type, []rbac.Object{rbac.ResourceUser}) + _, err = rbac.Filter(context.Background(), auth, uuid.NewString(), []string{}, rbac.ActionRead, []rbac.Object{rbac.ResourceUser, rbac.ResourceWorkspace}) require.ErrorContains(t, err, "object types must be uniform") } @@ -166,7 +166,7 @@ func TestFilter(t *testing.T) { } // Run by filter - list, err := rbac.Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects) + list, err := rbac.Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, localObjects) require.NoError(t, err) require.Equal(t, allowedCount, len(list), "expected number of allowed") for _, obj := range list { diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index cb9f964944011..231780e9eed56 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -76,7 +76,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.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects) + allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects) require.NoError(b, err) var _ = allowed }) From af457b9c042310c824c2fa77ceacf58244a6aec6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Aug 2022 16:23:57 -0500 Subject: [PATCH 20/30] Test timed out --- coderd/users_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 5c7777282541d..59506b6b00923 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1209,7 +1209,7 @@ func TestPaginatedUsers(t *testing.T) { t.Parallel() // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) defer cancel() assertPagination(ctx, t, client, tt.limit, tt.allUsers, tt.opt) From 21f4f21e1cef30014751fbd2d27addbff58e2d80 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 10:07:36 -0500 Subject: [PATCH 21/30] Test less random, rename param --- coderd/rbac/authz.go | 2 +- coderd/rbac/authz_test.go | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index d970276860177..4b99c29438bb4 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -11,7 +11,7 @@ import ( type Authorizer interface { ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error - PrepareByRoleName(ctx context.Context, subjectID string, roles []string, action Action, objectType string) (PreparedAuthorized, error) + PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) } type PreparedAuthorized interface { diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 4ed24d6b322bc..dd68475d868b1 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -10,7 +10,6 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/cryptorand" "github.com/coder/coder/testutil" ) @@ -59,13 +58,15 @@ func TestFilter(t *testing.T) { orgIDs[i] = uuid.New() userIDs[i] = uuid.New() } - objects := make([]fakeObject, 100) - for i := range objects { - objects[i] = fakeObject{ - Owner: userIDs[must(cryptorand.Intn(len(userIDs)))], - OrgOwner: orgIDs[must(cryptorand.Intn(len(orgIDs)))], - Type: rbac.ResourceWorkspace.Type, - Allowed: false, + objects := make([]fakeObject, 0, len(userIDs)*len(orgIDs)) + for i := range userIDs { + for j := range orgIDs { + objects = append(objects, fakeObject{ + Owner: userIDs[i], + OrgOwner: orgIDs[j], + Type: rbac.ResourceWorkspace.Type, + Allowed: false, + }) } } From 2c872200936556ed153b73f62ae4b1759861c964 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 12:52:48 -0500 Subject: [PATCH 22/30] Add rego comments with explainations --- coderd/rbac/policy.rego | 42 ++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 90051bae36913..7f42a209a949b 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -5,7 +5,13 @@ import future.keywords # opa eval --format=pretty 'data.authz.allow = true' -d policy.rego -i input.json # opa eval --partial --format=pretty 'data.authz.allow = true' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner -i input.json - +# +# This policy is specifically constructed to compress to a set of queries if the +# object's 'owner' and 'org_owner' fields are unknown. There is no specific set +# of rules that will guarantee that this policy has this property. However, there +# are some tricks. A unit test will +# +# # bool_flip lets you assign a value to an inverted bool. # You cannot do 'x := !false', but you can do 'x := bool_flip(false)' @@ -40,24 +46,33 @@ number(set) = c { } +# site, org, and user rules are all similar. Each rule should return a number +# from [-1, 1]. The number corrolates to "negative", "abstain", and "positive" +# for the given level. See the 'allow' rules for how these numbers are used. default site = 0 site := num { + # allow is a set of boolean values without duplicates. allow := { x | + # Iterate over all site permissions in all roles perm := input.subject.roles[_].site[_] perm.action in [input.action, "*"] perm.resource_type in [input.object.type, "*"] + # x is either 'true' or 'false' if a matching permission exists. x := bool_flip(perm.negate) } num := number(allow) } +# org_members is the list of organizations the actor is apart of. org_members := { orgID | input.subject.roles[_].org[orgID] } +# org is the same as 'site' except we need to iterate over each organization +# that the actor is a member of. default org = 0 org := num { - orgPerms := { id: num | + allow := { id: num | id := org_members[_] set := { x | perm := input.subject.roles[_].org[id][_] @@ -68,20 +83,28 @@ org := num { num := number(set) } - num := orgPerms[input.object.org_owner] + # Return only the org value of the input's org. + # The reason why we do not do this up front, is that we need to make sure + # this policy compresses down to simple queries. One way to ensure this is + # to keep unknown values out of comprehensions. + # (https://www.openpolicyagent.org/docs/latest/policy-language/#comprehensions) + num := allow[input.object.org_owner] } # 'org_mem' is set to true if the user is an org member -# or if the object has no org. org_mem := true { input.object.org_owner != "" input.object.org_owner in org_members } +# If the oject has no organization, then the user is also considered part of +# the non-existant org. org_mem := true { input.object.org_owner == "" } +# User is the same as the site, except it only applies if the user owns the object and +# the user is apart of the org (if the object has an org). default user = 0 user := num { input.object.owner != "" @@ -95,22 +118,27 @@ user := num { num := number(allow) } +# The allow block is quite simple. Any set with `-1` cascades down in levels. +# Authorization looks for any `allow` statement that is true. Multiple can be true! +# Note that the absence of `allow` means "unauthorized". +# An explicit `"allow": true` is required. + + default allow = false -# Site allow { site = 1 } -# Org allow { not site = -1 org = 1 } -# User allow { not site = -1 not org = -1 + # If we are not a member of an org, and the object has an org, then we are + # not authorized. This is an "implied -1" for not being in the org. org_mem user = 1 } From 1c407ab022a804c75ac408db75304ee991cb0dc7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 13:08:38 -0500 Subject: [PATCH 23/30] Add more rego comments --- coderd/rbac/policy.rego | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 7f42a209a949b..ca58fc4547c0a 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -9,9 +9,23 @@ import future.keywords # This policy is specifically constructed to compress to a set of queries if the # object's 'owner' and 'org_owner' fields are unknown. There is no specific set # of rules that will guarantee that this policy has this property. However, there -# are some tricks. A unit test will -# +# are some tricks. A unit test will enforce this property, so any edits that pass +# the unit test will be ok. # +# Tricks: (It's hard to really explain this, fiddling is required) +# 1. Do not use unknown fields in any comprehension or iteration. +# 2. Use the unknown fields as minimally as possible. +# 3. Avoid making code branches based on the value of the unknown field. +# Unknown values are like a "set" of possible values. +# (This is why rule 1 usually breaks things) +# For example: +# In the org section, we calculate the 'allow' number for all orgs, rather +# than just the input.object.org_owner. This is because if the org_owner +# changes, then we don't need to recompute any 'allow' sets. We already have +# the 'allow' for the changed value. So the answer is in a lookup table. +# The final statement 'num := allow[input.object.org_owner]' does not have +# different code branches based on the org_owner. 'num's value does, but +# that is the whole point of partial evaluation. # bool_flip lets you assign a value to an inverted bool. # You cannot do 'x := !false', but you can do 'x := bool_flip(false)' From 9f9b2d168f8d31749f7e328b0f1959b6803d455a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 13:24:17 -0500 Subject: [PATCH 24/30] Move fakeauthorizer code --- coderd/coderd_test.go | 44 +++++++++++++++++++++++++--------------- coderd/rbac/auth_fake.go | 31 ---------------------------- 2 files changed, 28 insertions(+), 47 deletions(-) delete mode 100644 coderd/rbac/auth_fake.go diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 93be45e3fc4d5..18b87e7bdba8a 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -63,7 +63,7 @@ func TestBuildInfo(t *testing.T) { // TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered. func TestAuthorizeAllEndpoints(t *testing.T) { t.Parallel() - authorizer := newRecordingAuthorizer() + authorizer := &recordingAuthorizer{} ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -564,28 +564,40 @@ type authCall struct { } type recordingAuthorizer struct { - *rbac.FakeAuthorizer Called *authCall AlwaysReturn error } -func newRecordingAuthorizer() *recordingAuthorizer { - r := &recordingAuthorizer{} - // Use the fake authorizer by rbac to handle prepared authorizers. - r.FakeAuthorizer = &rbac.FakeAuthorizer{ - AuthFunc: func(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { - r.Called = &authCall{ - SubjectID: subjectID, - Roles: roleNames, - Action: action, - Object: object, - } - return r.AlwaysReturn - }, +func (r *recordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { + r.Called = &authCall{ + SubjectID: subjectID, + Roles: roleNames, + Action: action, + Object: object, } - return r + return r.AlwaysReturn +} + +func (r *recordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) { + return &fakePreparedAuthorizer{ + Original: r, + SubjectID: subjectID, + Roles: roles, + Action: action, + }, nil } func (f *recordingAuthorizer) reset() { f.Called = nil } + +type fakePreparedAuthorizer struct { + Original *recordingAuthorizer + SubjectID string + Roles []string + Action rbac.Action +} + +func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error { + return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object) +} diff --git a/coderd/rbac/auth_fake.go b/coderd/rbac/auth_fake.go deleted file mode 100644 index a85080f2919d3..0000000000000 --- a/coderd/rbac/auth_fake.go +++ /dev/null @@ -1,31 +0,0 @@ -package rbac - -import "context" - -type FakeAuthorizer struct { - AuthFunc func(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error -} - -func (f FakeAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error { - return f.AuthFunc(ctx, subjectID, roleNames, action, object) -} - -func (f FakeAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, action Action, _ string) (PreparedAuthorized, error) { - return &fakePreparedAuthorizer{ - Original: f, - SubjectID: subjectID, - Roles: roles, - Action: action, - }, nil -} - -type fakePreparedAuthorizer struct { - Original Authorizer - SubjectID string - Roles []string - Action Action -} - -func (f fakePreparedAuthorizer) Authorize(ctx context.Context, object Object) error { - return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object) -} From ab55cf54cb361dd53c462a9aa552cbe7c2dd86fd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 13:36:40 -0500 Subject: [PATCH 25/30] Unexport partial fields - Move unit test to _internal - Remove example_test --- coderd/rbac/authz_internal_test.go | 708 +++++++++++++++++++++++++++++ coderd/rbac/authz_test.go | 701 ---------------------------- coderd/rbac/example_test.go | 61 --- coderd/rbac/partial.go | 33 +- 4 files changed, 726 insertions(+), 777 deletions(-) create mode 100644 coderd/rbac/authz_internal_test.go delete mode 100644 coderd/rbac/authz_test.go delete mode 100644 coderd/rbac/example_test.go diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go new file mode 100644 index 0000000000000..3b66031f3cd39 --- /dev/null +++ b/coderd/rbac/authz_internal_test.go @@ -0,0 +1,708 @@ +package rbac + +import ( + "context" + "encoding/json" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/testutil" +) + +// subject is required because rego needs +type subject struct { + UserID string `json:"id"` + // For the unit test we want to pass in the roles directly, instead of just + // by name. This allows us to test custom roles that do not exist in the product, + // but test edge cases of the implementation. + Roles []Role `json:"roles"` +} + +type fakeObject struct { + Owner uuid.UUID + OrgOwner uuid.UUID + Type string + Allowed bool +} + +func (w fakeObject) RBACObject() Object { + return Object{ + Owner: w.Owner.String(), + OrgID: w.OrgOwner.String(), + Type: w.Type, + } +} + +func TestFilterError(t *testing.T) { + t.Parallel() + auth, err := NewAuthorizer() + require.NoError(t, err) + + _, err = Filter(context.Background(), auth, uuid.NewString(), []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace}) + require.ErrorContains(t, err, "object types must be uniform") +} + +// TestFilter ensures the filter acts the same as an individual authorize. +// It generates a random set of objects, then runs the Filter batch function +// against the singular ByRoleName function. +func TestFilter(t *testing.T) { + t.Parallel() + + orgIDs := make([]uuid.UUID, 10) + userIDs := make([]uuid.UUID, len(orgIDs)) + for i := range orgIDs { + orgIDs[i] = uuid.New() + userIDs[i] = uuid.New() + } + objects := make([]fakeObject, 0, len(userIDs)*len(orgIDs)) + for i := range userIDs { + for j := range orgIDs { + objects = append(objects, fakeObject{ + Owner: userIDs[i], + OrgOwner: orgIDs[j], + Type: ResourceWorkspace.Type, + Allowed: false, + }) + } + } + + testCases := []struct { + Name string + SubjectID string + Roles []string + Action Action + ObjectType string + }{ + { + Name: "NoRoles", + SubjectID: userIDs[0].String(), + Roles: []string{}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "Admin", + SubjectID: userIDs[0].String(), + Roles: []string{RoleOrgMember(orgIDs[0]), "auditor", RoleAdmin(), RoleMember()}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "OrgAdmin", + SubjectID: userIDs[0].String(), + Roles: []string{RoleOrgMember(orgIDs[0]), RoleOrgAdmin(orgIDs[0]), RoleMember()}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "OrgMember", + SubjectID: userIDs[0].String(), + Roles: []string{RoleOrgMember(orgIDs[0]), RoleOrgMember(orgIDs[1]), RoleMember()}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "ManyRoles", + SubjectID: userIDs[0].String(), + Roles: []string{ + RoleOrgMember(orgIDs[0]), RoleOrgAdmin(orgIDs[0]), + RoleOrgMember(orgIDs[1]), RoleOrgAdmin(orgIDs[1]), + RoleOrgMember(orgIDs[2]), RoleOrgAdmin(orgIDs[2]), + RoleOrgMember(orgIDs[4]), + RoleOrgMember(orgIDs[5]), + RoleMember(), + }, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "SiteMember", + SubjectID: userIDs[0].String(), + Roles: []string{RoleMember()}, + ObjectType: ResourceUser.Type, + Action: ActionRead, + }, + { + Name: "ReadOrgs", + SubjectID: userIDs[0].String(), + Roles: []string{ + RoleOrgMember(orgIDs[0]), + RoleOrgMember(orgIDs[1]), + RoleOrgMember(orgIDs[2]), + RoleOrgMember(orgIDs[3]), + RoleMember(), + }, + ObjectType: ResourceOrganization.Type, + Action: ActionRead, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + localObjects := make([]fakeObject, len(objects)) + copy(localObjects, objects) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + auth, err := NewAuthorizer() + require.NoError(t, err, "new auth") + + // Run auth 1 by 1 + var allowedCount int + for i, obj := range localObjects { + obj.Type = tc.ObjectType + err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, ActionRead, obj.RBACObject()) + obj.Allowed = err == nil + if err == nil { + allowedCount++ + } + localObjects[i] = obj + } + + // Run by filter + list, err := Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, localObjects) + require.NoError(t, err) + require.Equal(t, allowedCount, len(list), "expected number of allowed") + for _, obj := range list { + require.True(t, obj.Allowed, "expected allowed") + } + }) + } +} + +// TestAuthorizeDomain test the very basic roles that are commonly used. +func TestAuthorizeDomain(t *testing.T) { + t.Parallel() + defOrg := uuid.New() + unuseID := uuid.New() + + user := subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleMember())), + must(RoleByName(RoleOrgMember(defOrg))), + }, + } + + testAuthorize(t, "Member", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), 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.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.WithOwner("not-me"), actions: allActions(), allow: false}, + }) + + user = subject{ + UserID: "me", + Roles: []Role{{ + Name: "deny-all", + // List out deny permissions explicitly + Site: []Permission{ + { + Negate: true, + ResourceType: WildcardSymbol, + Action: WildcardSymbol, + }, + }, + }}, + } + + testAuthorize(t, "DeletedMember", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), 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.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.WithOwner("not-me"), actions: allActions(), allow: false}, + }) + + user = subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleOrgAdmin(defOrg))), + must(RoleByName(RoleMember())), + }, + } + + testAuthorize(t, "OrgAdmin", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), 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.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.WithOwner("not-me"), actions: allActions(), allow: false}, + }) + + user = subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleAdmin())), + must(RoleByName(RoleMember())), + }, + } + + testAuthorize(t, "SiteAdmin", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: true}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), 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.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.WithOwner("not-me"), actions: allActions(), allow: true}, + }) + + // In practice this is a token scope on a regular subject. + // So this unit test does not represent a practical role. It is just + // testing the capabilities of the RBAC system. + user = subject{ + UserID: "me", + Roles: []Role{ + { + Name: "WorkspaceToken", + // This is at the site level to prevent the token from losing access if the user + // is kicked from the org + Site: []Permission{ + { + Negate: false, + ResourceType: ResourceWorkspace.Type, + Action: ActionRead, + }, + }, + }, + }, + } + + testAuthorize(t, "WorkspaceToken", user, + // Read Actions + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionRead} + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), allow: true}, + + {resource: ResourceWorkspace.All(), allow: true}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: true}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: true}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: true}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: true}, + }), + // Not read actions + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionCreate, ActionUpdate, ActionDelete} + c.allow = false + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(defOrg)}, + + {resource: ResourceWorkspace.WithOwner(user.UserID)}, + + {resource: ResourceWorkspace.All()}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + }), + ) + + // In practice this is a token scope on a regular subject + user = subject{ + UserID: "me", + Roles: []Role{ + { + Name: "ReadOnlyOrgAndUser", + Site: []Permission{}, + Org: map[string][]Permission{ + defOrg.String(): {{ + Negate: false, + ResourceType: "*", + Action: ActionRead, + }}, + }, + User: []Permission{ + { + Negate: false, + ResourceType: "*", + Action: ActionRead, + }, + }, + }, + }, + } + + testAuthorize(t, "ReadOnly", user, + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionRead} + return c + }, []authTestCase{ + // Read + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), allow: true}, + + {resource: ResourceWorkspace.All(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + }), + + // Pass non-read actions + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionCreate, ActionUpdate, ActionDelete} + c.allow = false + return c + }, []authTestCase{ + // Read + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(defOrg)}, + + {resource: ResourceWorkspace.WithOwner(user.UserID)}, + + {resource: ResourceWorkspace.All()}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + })) +} + +// TestAuthorizeLevels ensures level overrides are acting appropriately +//nolint:paralleltest +func TestAuthorizeLevels(t *testing.T) { + defOrg := uuid.New() + unusedID := uuid.New() + + user := subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleAdmin())), + { + Name: "org-deny:" + defOrg.String(), + Org: map[string][]Permission{ + defOrg.String(): { + { + Negate: true, + ResourceType: "*", + Action: "*", + }, + }, + }, + }, + { + Name: "user-deny-all", + // List out deny permissions explicitly + User: []Permission{ + { + Negate: true, + ResourceType: WildcardSymbol, + Action: WildcardSymbol, + }, + }, + }, + }, + } + + testAuthorize(t, "AdminAlwaysAllow", user, + cases(func(c authTestCase) authTestCase { + c.actions = allActions() + c.allow = true + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(defOrg)}, + + {resource: ResourceWorkspace.WithOwner(user.UserID)}, + + {resource: ResourceWorkspace.All()}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(unusedID)}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner("not-me")}, + {resource: ResourceWorkspace.InOrg(unusedID)}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + })) + + user = subject{ + UserID: "me", + Roles: []Role{ + { + Name: "site-noise", + Site: []Permission{ + { + Negate: true, + ResourceType: "random", + Action: WildcardSymbol, + }, + }, + }, + must(RoleByName(RoleOrgAdmin(defOrg))), + { + Name: "user-deny-all", + // List out deny permissions explicitly + User: []Permission{ + { + Negate: true, + ResourceType: WildcardSymbol, + Action: WildcardSymbol, + }, + }, + }, + }, + } + + testAuthorize(t, "OrgAllowAll", user, + cases(func(c authTestCase) authTestCase { + c.actions = allActions() + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), allow: false}, + + {resource: ResourceWorkspace.All(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID), allow: false}, + {resource: ResourceWorkspace.InOrg(unusedID), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), allow: false}, + {resource: ResourceWorkspace.InOrg(unusedID), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + })) +} + +// cases applies a given function to all test cases. This makes generalities easier to create. +func cases(opt func(c authTestCase) authTestCase, cases []authTestCase) []authTestCase { + if opt == nil { + return cases + } + for i := range cases { + cases[i] = opt(cases[i]) + } + return cases +} + +type authTestCase struct { + resource Object + actions []Action + allow bool +} + +func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTestCase) { + t.Helper() + authorizer, err := NewAuthorizer() + require.NoError(t, err) + for _, cases := range sets { + for _, c := range cases { + t.Run(name, func(t *testing.T) { + for _, a := range c.actions { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + t.Cleanup(cancel) + authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) + // Logging only + if authError != nil { + var uerr *UnauthorizedError + xerrors.As(authError, &uerr) + d, _ := json.Marshal(uerr.Input()) + t.Logf("input: %s", string(d)) + t.Logf("internal error: %+v", uerr.Internal().Error()) + t.Logf("output: %+v", uerr.Output()) + } else { + d, _ := json.Marshal(map[string]interface{}{ + "subject": subject, + "object": c.resource, + "action": a, + }) + t.Log(string(d)) + } + + if c.allow { + assert.NoError(t, authError, "expected no error for testcase action %s", a) + } else { + assert.Error(t, authError, "expected unauthorized") + } + + partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) + require.NoError(t, err, "make prepared authorizer") + + // Also check the rego policy can form a valid partial query result. + // This ensures we can convert the queries into SQL WHERE clauses in the future. + // If this function returns 'Support' sections, then we cannot convert the query into SQL. + if len(partialAuthz.partialQueries.Support) > 0 { + d, _ := json.Marshal(partialAuthz.input) + t.Logf("input: %s", string(d)) + for _, q := range partialAuthz.partialQueries.Queries { + t.Logf("query: %+v", q.String()) + } + for _, s := range partialAuthz.partialQueries.Support { + t.Logf("support: %+v", s.String()) + } + } + require.Equal(t, 0, len(partialAuthz.partialQueries.Support), "expected 0 support rules") + + partialErr := partialAuthz.Authorize(ctx, c.resource) + if authError != nil { + assert.Error(t, partialErr, "partial error blocked valid request (false negative)") + } else { + assert.NoError(t, partialErr, "partial allowed invalid request (false positive)") + } + } + }) + } + } +} + +// 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 { + panic(err) + } + return value +} diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go deleted file mode 100644 index dd68475d868b1..0000000000000 --- a/coderd/rbac/authz_test.go +++ /dev/null @@ -1,701 +0,0 @@ -package rbac_test - -import ( - "context" - "encoding/json" - "testing" - - "github.com/google/uuid" - "github.com/stretchr/testify/require" - "golang.org/x/xerrors" - - "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/testutil" -) - -// subject is required because rego needs -type subject struct { - UserID string `json:"id"` - // For the unit test we want to pass in the roles directly, instead of just - // by name. This allows us to test custom roles that do not exist in the product, - // but test edge cases of the implementation. - Roles []rbac.Role `json:"roles"` -} - -type fakeObject struct { - Owner uuid.UUID - OrgOwner uuid.UUID - Type string - Allowed bool -} - -func (w fakeObject) RBACObject() rbac.Object { - return rbac.Object{ - Owner: w.Owner.String(), - OrgID: w.OrgOwner.String(), - Type: w.Type, - } -} - -func TestFilterError(t *testing.T) { - t.Parallel() - auth, err := rbac.NewAuthorizer() - require.NoError(t, err) - - _, err = rbac.Filter(context.Background(), auth, uuid.NewString(), []string{}, rbac.ActionRead, []rbac.Object{rbac.ResourceUser, rbac.ResourceWorkspace}) - require.ErrorContains(t, err, "object types must be uniform") -} - -// TestFilter ensures the filter acts the same as an individual authorize. -// It generates a random set of objects, then runs the Filter batch function -// against the singular ByRoleName function. -func TestFilter(t *testing.T) { - t.Parallel() - - orgIDs := make([]uuid.UUID, 10) - userIDs := make([]uuid.UUID, len(orgIDs)) - for i := range orgIDs { - orgIDs[i] = uuid.New() - userIDs[i] = uuid.New() - } - objects := make([]fakeObject, 0, len(userIDs)*len(orgIDs)) - for i := range userIDs { - for j := range orgIDs { - objects = append(objects, fakeObject{ - Owner: userIDs[i], - OrgOwner: orgIDs[j], - Type: rbac.ResourceWorkspace.Type, - Allowed: false, - }) - } - } - - testCases := []struct { - Name string - SubjectID string - Roles []string - Action rbac.Action - ObjectType string - }{ - { - Name: "NoRoles", - SubjectID: userIDs[0].String(), - Roles: []string{}, - ObjectType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - { - Name: "Admin", - SubjectID: userIDs[0].String(), - Roles: []string{rbac.RoleOrgMember(orgIDs[0]), "auditor", rbac.RoleAdmin(), rbac.RoleMember()}, - ObjectType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - { - Name: "OrgAdmin", - SubjectID: userIDs[0].String(), - Roles: []string{rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgAdmin(orgIDs[0]), rbac.RoleMember()}, - ObjectType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - { - Name: "OrgMember", - SubjectID: userIDs[0].String(), - Roles: []string{rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgMember(orgIDs[1]), rbac.RoleMember()}, - ObjectType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - { - Name: "ManyRoles", - SubjectID: userIDs[0].String(), - Roles: []string{ - rbac.RoleOrgMember(orgIDs[0]), rbac.RoleOrgAdmin(orgIDs[0]), - rbac.RoleOrgMember(orgIDs[1]), rbac.RoleOrgAdmin(orgIDs[1]), - rbac.RoleOrgMember(orgIDs[2]), rbac.RoleOrgAdmin(orgIDs[2]), - rbac.RoleOrgMember(orgIDs[4]), - rbac.RoleOrgMember(orgIDs[5]), - rbac.RoleMember(), - }, - ObjectType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - { - Name: "SiteMember", - SubjectID: userIDs[0].String(), - Roles: []string{rbac.RoleMember()}, - ObjectType: rbac.ResourceUser.Type, - Action: rbac.ActionRead, - }, - { - Name: "ReadOrgs", - SubjectID: userIDs[0].String(), - Roles: []string{ - rbac.RoleOrgMember(orgIDs[0]), - rbac.RoleOrgMember(orgIDs[1]), - rbac.RoleOrgMember(orgIDs[2]), - rbac.RoleOrgMember(orgIDs[3]), - rbac.RoleMember(), - }, - ObjectType: rbac.ResourceOrganization.Type, - Action: rbac.ActionRead, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.Name, func(t *testing.T) { - t.Parallel() - - localObjects := make([]fakeObject, len(objects)) - copy(localObjects, objects) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() - auth, err := rbac.NewAuthorizer() - require.NoError(t, err, "new auth") - - // Run auth 1 by 1 - var allowedCount int - for i, obj := range localObjects { - obj.Type = tc.ObjectType - err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, rbac.ActionRead, obj.RBACObject()) - obj.Allowed = err == nil - if err == nil { - allowedCount++ - } - localObjects[i] = obj - } - - // Run by filter - list, err := rbac.Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, localObjects) - require.NoError(t, err) - require.Equal(t, allowedCount, len(list), "expected number of allowed") - for _, obj := range list { - require.True(t, obj.Allowed, "expected allowed") - } - }) - } -} - -// TestAuthorizeDomain test the very basic roles that are commonly used. -func TestAuthorizeDomain(t *testing.T) { - t.Parallel() - defOrg := uuid.New() - unuseID := uuid.New() - - user := subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleMember())), - must(rbac.RoleByName(rbac.RoleOrgMember(defOrg))), - }, - } - - testAuthorize(t, "Member", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - // Other org + other us - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - }) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{{ - Name: "deny-all", - // List out deny permissions explicitly - Site: []rbac.Permission{ - { - Negate: true, - ResourceType: rbac.WildcardSymbol, - Action: rbac.WildcardSymbol, - }, - }, - }}, - } - - testAuthorize(t, "DeletedMember", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - }) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleOrgAdmin(defOrg))), - must(rbac.RoleByName(rbac.RoleMember())), - }, - } - - testAuthorize(t, "OrgAdmin", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - }) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleAdmin())), - must(rbac.RoleByName(rbac.RoleMember())), - }, - } - - testAuthorize(t, "SiteAdmin", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: true}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, - }) - - // In practice this is a token scope on a regular subject. - // So this unit test does not represent a practical role. It is just - // testing the capabilities of the RBAC system. - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - { - Name: "WorkspaceToken", - // This is at the site level to prevent the token from losing access if the user - // is kicked from the org - Site: []rbac.Permission{ - { - Negate: false, - ResourceType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - }, - }, - }, - } - - testAuthorize(t, "WorkspaceToken", user, - // Read Actions - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionRead} - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), allow: true}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: true}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: true}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: true}, - }), - // Not read actions - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete} - c.allow = false - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - - {resource: rbac.ResourceWorkspace.All()}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - }), - ) - - // In practice this is a token scope on a regular subject - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - { - Name: "ReadOnlyOrgAndUser", - Site: []rbac.Permission{}, - Org: map[string][]rbac.Permission{ - defOrg.String(): {{ - Negate: false, - ResourceType: "*", - Action: rbac.ActionRead, - }}, - }, - User: []rbac.Permission{ - { - Negate: false, - ResourceType: "*", - Action: rbac.ActionRead, - }, - }, - }, - }, - } - - testAuthorize(t, "ReadOnly", user, - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionRead} - return c - }, []authTestCase{ - // Read - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - }), - - // Pass non-read actions - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete} - c.allow = false - return c - }, []authTestCase{ - // Read - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - - {resource: rbac.ResourceWorkspace.All()}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - })) -} - -// TestAuthorizeLevels ensures level overrides are acting appropriately -//nolint:paralleltest -func TestAuthorizeLevels(t *testing.T) { - defOrg := uuid.New() - unusedID := uuid.New() - - user := subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleAdmin())), - { - Name: "org-deny:" + defOrg.String(), - Org: map[string][]rbac.Permission{ - defOrg.String(): { - { - Negate: true, - ResourceType: "*", - Action: "*", - }, - }, - }, - }, - { - Name: "user-deny-all", - // List out deny permissions explicitly - User: []rbac.Permission{ - { - Negate: true, - ResourceType: rbac.WildcardSymbol, - Action: rbac.WildcardSymbol, - }, - }, - }, - }, - } - - testAuthorize(t, "AdminAlwaysAllow", user, - cases(func(c authTestCase) authTestCase { - c.actions = allActions() - c.allow = true - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - - {resource: rbac.ResourceWorkspace.All()}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - })) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - { - Name: "site-noise", - Site: []rbac.Permission{ - { - Negate: true, - ResourceType: "random", - Action: rbac.WildcardSymbol, - }, - }, - }, - must(rbac.RoleByName(rbac.RoleOrgAdmin(defOrg))), - { - Name: "user-deny-all", - // List out deny permissions explicitly - User: []rbac.Permission{ - { - Negate: true, - ResourceType: rbac.WildcardSymbol, - Action: rbac.WildcardSymbol, - }, - }, - }, - }, - } - - testAuthorize(t, "OrgAllowAll", user, - cases(func(c authTestCase) authTestCase { - c.actions = allActions() - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: false}, - - {resource: rbac.ResourceWorkspace.All(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - })) -} - -// cases applies a given function to all test cases. This makes generalities easier to create. -func cases(opt func(c authTestCase) authTestCase, cases []authTestCase) []authTestCase { - if opt == nil { - return cases - } - for i := range cases { - cases[i] = opt(cases[i]) - } - return cases -} - -type authTestCase struct { - resource rbac.Object - actions []rbac.Action - allow bool -} - -func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTestCase) { - t.Helper() - authorizer, err := rbac.NewAuthorizer() - require.NoError(t, err) - for _, cases := range sets { - for _, c := range cases { - t.Run(name, func(t *testing.T) { - for _, a := range c.actions { - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - t.Cleanup(cancel) - authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) - // Logging only - if authError != nil { - var uerr *rbac.UnauthorizedError - xerrors.As(authError, &uerr) - d, _ := json.Marshal(uerr.Input()) - t.Logf("input: %s", string(d)) - t.Logf("internal error: %+v", uerr.Internal().Error()) - t.Logf("output: %+v", uerr.Output()) - } else { - d, _ := json.Marshal(map[string]interface{}{ - "subject": subject, - "object": c.resource, - "action": a, - }) - t.Log(string(d)) - } - - if c.allow { - require.NoError(t, authError, "expected no error for testcase action %s", a) - } else { - require.Error(t, authError, "expected unauthorized") - } - - partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) - require.NoError(t, err, "make prepared authorizer") - - // Also check the rego policy can form a valid partial query result. - // This ensures we can convert the queries into SQL WHERE clauses in the future. - // If this function returns 'Support' sections, then we cannot convert the query into SQL. - if len(partialAuthz.PartialQueries.Support) > 0 { - d, _ := json.Marshal(partialAuthz.Input) - t.Logf("input: %s", string(d)) - for _, q := range partialAuthz.PartialQueries.Queries { - t.Logf("query: %+v", q.String()) - } - for _, s := range partialAuthz.PartialQueries.Support { - t.Logf("support: %+v", s.String()) - } - } - require.Equal(t, 0, len(partialAuthz.PartialQueries.Support), "expected 0 support rules") - - partialErr := partialAuthz.Authorize(ctx, c.resource) - if authError != nil { - require.Error(t, partialErr, "partial error blocked valid request (false negative)") - } else { - require.NoError(t, partialErr, "partial allowed invalid request (false positive)") - } - } - }) - } - } -} - -// allActions is a helper function to return all the possible actions types. -func allActions() []rbac.Action { - return []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete} -} diff --git a/coderd/rbac/example_test.go b/coderd/rbac/example_test.go deleted file mode 100644 index 98b23b9303c86..0000000000000 --- a/coderd/rbac/example_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package rbac_test - -import ( - "context" - "testing" - - "github.com/google/uuid" - - "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/rbac" -) - -// TestExample gives some examples on how to use the authz library. -// This serves to test syntax more than functionality. -func TestExample(t *testing.T) { - t.Parallel() - ctx := context.Background() - authorizer, err := rbac.NewAuthorizer() - require.NoError(t, err) - defaultOrg := uuid.New() - - // user will become an authn object, and can even be a database.User if it - // fulfills the interface. Until then, use a placeholder. - user := subject{ - UserID: "alice", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleMember())), - must(rbac.RoleByName(rbac.RoleOrgAdmin(defaultOrg))), - }, - } - - //nolint:paralleltest - t.Run("ReadAllWorkspaces", func(t *testing.T) { - // To read all workspaces on the site - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.All()) - var _ = err - require.Error(t, err, "this user cannot read all workspaces") - }) - - //nolint:paralleltest - t.Run("ReadOrgWorkspaces", func(t *testing.T) { - // To read all workspaces on the org 'default' - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg)) - require.NoError(t, err, "this user can read all org workspaces in 'default'") - }) - - //nolint:paralleltest - t.Run("ReadMyWorkspace", func(t *testing.T) { - // Note 'database.Workspace' could fulfill the object interface and be passed in directly - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg).WithOwner(user.UserID)) - require.NoError(t, err, "this user can their workspace") - }) -} - -func must[T any](value T, err error) T { - if err != nil { - panic(err) - } - return value -} diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index c2beae9501698..8ff0f1d17593f 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -9,15 +9,18 @@ import ( ) type PartialAuthorizer struct { - // PartialQueries is mainly used for unit testing to assert our rego policy + // partialQueries is mainly used for unit testing to assert our rego policy // can always be compressed into a set of queries. - PartialQueries *rego.PartialQueries - PreparedQueries []rego.PreparedEvalQuery - // Input is used purely for debugging and logging. - Input map[string]interface{} - // AlwaysTrue is if the subject can always perform the action on the + partialQueries *rego.PartialQueries + // input is used purely for debugging and logging. + input map[string]interface{} + // preparedQueries are the compiled set of queries after partial evaluation. + // Cache these prepared queries to avoid re-compiling the queries. + // If alwaysTrue is true, then ignore these. + preparedQueries []rego.PreparedEvalQuery + // alwaysTrue is if the subject can always perform the action on the // resource type, regardless of the unknown fields. - AlwaysTrue bool + alwaysTrue bool } func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { @@ -48,9 +51,9 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a } pAuth := &PartialAuthorizer{ - PartialQueries: partialQueries, - PreparedQueries: []rego.PreparedEvalQuery{}, - Input: input, + partialQueries: partialQueries, + preparedQueries: []rego.PreparedEvalQuery{}, + input: input, } // Prepare each query to optimize the runtime when we iterate over the objects. @@ -61,7 +64,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a // 'WHERE true' // This is likely an admin. We don't even need to use rego going // forward. - pAuth.AlwaysTrue = true + pAuth.alwaysTrue = true preparedQueries = []rego.PreparedEvalQuery{} break } @@ -73,14 +76,14 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, a } preparedQueries = append(preparedQueries, results) } - pAuth.PreparedQueries = preparedQueries + pAuth.preparedQueries = preparedQueries return pAuth, nil } // Authorize authorizes a single object using the partially prepared queries. func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { - if a.AlwaysTrue { + if a.alwaysTrue { return nil } @@ -95,7 +98,7 @@ func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { // all boolean expressions. In the above 1st example, there are 2. // These expressions within a single query are `AND` together by rego. EachQueryLoop: - for _, q := range a.PreparedQueries { + for _, q := range a.preparedQueries { // We need to eval each query with the newly known fields. results, err := q.Eval(ctx, rego.EvalInput(map[string]interface{}{ "object": object, @@ -140,5 +143,5 @@ EachQueryLoop: return nil } - return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), a.Input, nil) + return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), a.input, nil) } From abf098d6003be1270285cd329147cbe227fbe2fa Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 13:38:59 -0500 Subject: [PATCH 26/30] Reduce wait --- coderd/users_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 59506b6b00923..5c7777282541d 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1209,7 +1209,7 @@ func TestPaginatedUsers(t *testing.T) { t.Parallel() // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) defer cancel() assertPagination(ctx, t, client, tt.limit, tt.allUsers, tt.opt) From 44c7370f3cf9a599c31644d993d70d6f48a9766d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 15:03:05 -0500 Subject: [PATCH 27/30] Linting + Typos --- coderd/coderd_test.go | 4 ++-- coderd/rbac/policy.rego | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 18b87e7bdba8a..f3c4baaa227ab 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -587,8 +587,8 @@ func (r *recordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID str }, nil } -func (f *recordingAuthorizer) reset() { - f.Called = nil +func (r *recordingAuthorizer) reset() { + r.Called = nil } type fakePreparedAuthorizer struct { diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index ca58fc4547c0a..5bfdab233a5e6 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -61,7 +61,7 @@ number(set) = c { # site, org, and user rules are all similar. Each rule should return a number -# from [-1, 1]. The number corrolates to "negative", "abstain", and "positive" +# from [-1, 1]. The number correlates to "negative", "abstain", and "positive" # for the given level. See the 'allow' rules for how these numbers are used. default site = 0 site := num { @@ -112,7 +112,7 @@ org_mem := true { } # If the oject has no organization, then the user is also considered part of -# the non-existant org. +# the non-existent org. org_mem := true { input.object.org_owner == "" } From 4611322494dbea12a7924d7344b0451b0bd1b612 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 15:38:32 -0500 Subject: [PATCH 28/30] Wording --- coderd/rbac/policy.rego | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 5bfdab233a5e6..485def49f38fb 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -61,7 +61,7 @@ number(set) = c { # site, org, and user rules are all similar. Each rule should return a number -# from [-1, 1]. The number correlates to "negative", "abstain", and "positive" +# from [-1, 1]. The number corresponds to "negative", "abstain", and "positive" # for the given level. See the 'allow' rules for how these numbers are used. default site = 0 site := num { From c8e26a81a00f8ec76cbcceb607ccbbd23d2ee543 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 16:10:43 -0500 Subject: [PATCH 29/30] Test runs long --- coderd/users_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 5c7777282541d..dfbc8c23eca02 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1121,7 +1121,7 @@ func TestPaginatedUsers(t *testing.T) { coderdtest.CreateFirstUser(t, client) // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) defer cancel() me, err := client.User(ctx, codersdk.Me) From 510e94b418ad0303e477718aedba525d8a5604d5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Aug 2022 16:28:10 -0500 Subject: [PATCH 30/30] The single test is taking too long --- coderd/users_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index dfbc8c23eca02..0ff422d237ac1 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1121,7 +1121,7 @@ func TestPaginatedUsers(t *testing.T) { coderdtest.CreateFirstUser(t, client) // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) defer cancel() me, err := client.User(ctx, codersdk.Me) @@ -1196,7 +1196,6 @@ func TestPaginatedUsers(t *testing.T) { {name: "all users", limit: 10, allUsers: allUsers}, {name: "all users", limit: 5, allUsers: allUsers}, {name: "all users", limit: 3, allUsers: allUsers}, - {name: "all users", limit: 1, allUsers: allUsers}, {name: "gmail search", limit: 3, allUsers: specialUsers, opt: gmailSearch}, {name: "gmail search", limit: 7, allUsers: specialUsers, opt: gmailSearch}, {name: "username search", limit: 3, allUsers: specialUsers, opt: usernameSearch},