From 6d41edceae37ac6e0494d3951944674b8f881dda Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 13 Apr 2023 08:58:01 -0500 Subject: [PATCH 1/8] test: Handle Fitler flake with ctx errors --- coderd/rbac/authz.go | 6 ++++++ coderd/rbac/authz_internal_test.go | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 43aeb68583be9..9f415567b1f3c 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -137,6 +137,9 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a err := auth.Authorize(ctx, subject, action, o.RBACObject()) if err == nil { filtered = append(filtered, o) + } else if ctx.Err() != nil { + // Exit early if the error comes from the context + return nil, ctx.Err() } } return filtered, nil @@ -155,6 +158,9 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a err := prepared.Authorize(ctx, rbacObj) if err == nil { filtered = append(filtered, object) + } else if ctx.Err() != nil { + // Exit early if the error comes from the context + return nil, ctx.Err() } } diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 041a8f7e704c7..7efcf8f4ad1c4 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -170,7 +170,7 @@ func TestFilter(t *testing.T) { localObjects := make([]fakeObject, len(objects)) copy(localObjects, objects) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) defer cancel() auth := NewAuthorizer(prometheus.NewRegistry()) From 633fab30a2a1d396cc719018e29f30da587386b5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 10:24:15 -0500 Subject: [PATCH 2/8] Add unit test to check filter for proper error --- coderd/rbac/authz_internal_test.go | 64 +++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 7efcf8f4ad1c4..886228e355c57 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -30,18 +30,64 @@ func (w fakeObject) RBACObject() Object { } } +// objectBomb is a wrapper around an Objecter that calls a function when +// RBACObject is called. +type objectBomb struct { + Objecter + bomb func() +} + +func (o *objectBomb) RBACObject() Object { + o.bomb() + return o.Objecter.RBACObject() +} + func TestFilterError(t *testing.T) { t.Parallel() - auth := NewAuthorizer(prometheus.NewRegistry()) - subject := Subject{ - ID: uuid.NewString(), - Roles: RoleNames{}, - Groups: []string{}, - Scope: ScopeAll, - } - _, err := Filter(context.Background(), auth, subject, ActionRead, []Object{ResourceUser, ResourceWorkspace}) - require.ErrorContains(t, err, "object types must be uniform") + t.Run("DifferentResourceTypes", func(t *testing.T) { + t.Parallel() + + auth := NewAuthorizer(prometheus.NewRegistry()) + subject := Subject{ + ID: uuid.NewString(), + Roles: RoleNames{}, + Groups: []string{}, + Scope: ScopeAll, + } + + _, err := Filter(context.Background(), auth, subject, ActionRead, []Object{ResourceUser, ResourceWorkspace}) + require.ErrorContains(t, err, "object types must be uniform") + }) + + t.Run("CancelledContext", func(t *testing.T) { + t.Parallel() + + auth := NewAuthorizer(prometheus.NewRegistry()) + subject := Subject{ + ID: uuid.NewString(), + Roles: RoleNames{ + RoleOwner(), + }, + Groups: []string{}, + Scope: ScopeAll, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + objects := []Objecter{ + ResourceUser, + ResourceUser, + &objectBomb{ + Objecter: ResourceUser, + bomb: cancel, + }, + ResourceUser, + } + + _, err := Filter(ctx, auth, subject, ActionRead, objects) + require.ErrorIs(t, err, context.Canceled, "expected context cancellation error") + }) } // TestFilter ensures the filter acts the same as an individual authorize. From 46b50997faca5b6216908076a55303ea6c9e3528 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 10:36:05 -0500 Subject: [PATCH 3/8] Correctly return category of errors --- coderd/rbac/authz.go | 20 ++++++++++++-------- coderd/rbac/authz_internal_test.go | 2 +- coderd/rbac/error.go | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 9f415567b1f3c..e8beeea1bd468 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -137,9 +137,10 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a err := auth.Authorize(ctx, subject, action, o.RBACObject()) if err == nil { filtered = append(filtered, o) - } else if ctx.Err() != nil { - // Exit early if the error comes from the context - return nil, ctx.Err() + } else if !IsUnauthorizedError(err) { + // If the error is not the expected "Unauthorized" error, then + // it is something unexpected. + return nil, err } } return filtered, nil @@ -158,9 +159,10 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a err := prepared.Authorize(ctx, rbacObj) if err == nil { filtered = append(filtered, object) - } else if ctx.Err() != nil { - // Exit early if the error comes from the context - return nil, ctx.Err() + } else if !IsUnauthorizedError(err) { + // If the error is not the expected "Unauthorized" error, then + // it is something unexpected. + return nil, err } } @@ -325,7 +327,8 @@ func (a RegoAuthorizer) authorize(ctx context.Context, subject Subject, action A results, err := a.query.Eval(ctx, rego.EvalParsedInput(astV)) if err != nil { - return ForbiddenWithInternal(xerrors.Errorf("eval rego: %w", err), subject, action, object, results) + err = correctCancelError(err) + return xerrors.Errorf("evaluate rego: %w", err) } if !results.Allowed() { @@ -436,7 +439,8 @@ EachQueryLoop: // We need to eval each query with the newly known fields. results, err := q.Eval(ctx, rego.EvalParsedInput(parsed)) if err != nil { - continue EachQueryLoop + err = correctCancelError(err) + return xerrors.Errorf("eval error: %w", err) } // If there are no results, then the query is false. This is because rego diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 886228e355c57..ee11abf34612b 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -86,7 +86,7 @@ func TestFilterError(t *testing.T) { } _, err := Filter(ctx, auth, subject, ActionRead, objects) - require.ErrorIs(t, err, context.Canceled, "expected context cancellation error") + require.ErrorIs(t, err, context.Canceled) }) } diff --git a/coderd/rbac/error.go b/coderd/rbac/error.go index 9492c0dff8d96..16e1ad9f466cd 100644 --- a/coderd/rbac/error.go +++ b/coderd/rbac/error.go @@ -1,11 +1,14 @@ package rbac import ( + "context" "errors" "flag" "fmt" "github.com/open-policy-agent/opa/rego" + "github.com/open-policy-agent/opa/topdown" + "golang.org/x/xerrors" ) const ( @@ -97,3 +100,17 @@ func (*UnauthorizedError) As(target interface{}) bool { } return false } + +// correctCancelError will return the correct error for a cancelled context. This +// is because rego changes a canceled context to a topdown.CancelErr. This error +// is not helpful if the code is "cancelled". To make the error conform with the +// rest of our cancelled errors, we will convert the error to a context.Canceled +// error. No good information is lost, as the topdown.CancelErr provides the +// location of the query that was cancelled, which does not matter. +func correctCancelError(err error) error { + e := new(topdown.Error) + if xerrors.As(err, &e) || e.Code == topdown.CancelErr { + return context.Canceled + } + return err +} From c150c53600d31e27aa0a0fa728fc6e26c120cfba Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 11:02:27 -0500 Subject: [PATCH 4/8] Add skip msg --- coderd/rbac/authz_internal_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index ee11abf34612b..c19a0d8e14472 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -62,6 +62,9 @@ func TestFilterError(t *testing.T) { t.Run("CancelledContext", func(t *testing.T) { t.Parallel() + t.Skipf("This test is racy as rego eval checks the ctx cancelled in a go routine. " + + "It is a coin flip if the query finishes before the 'cancel' is checked. " + + "So sometimes the 'Authorize' call succeeds even if ctx is cancelled.") auth := NewAuthorizer(prometheus.NewRegistry()) subject := Subject{ @@ -85,7 +88,7 @@ func TestFilterError(t *testing.T) { ResourceUser, } - _, err := Filter(ctx, auth, subject, ActionRead, objects) + resp, err := Filter(ctx, auth, subject, ActionRead, objects) require.ErrorIs(t, err, context.Canceled) }) } From 0c52c87c72f3e18b2823fca1c371728849520032 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 11:05:52 -0500 Subject: [PATCH 5/8] Fix --- coderd/rbac/authz_internal_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index c19a0d8e14472..e5d382ecf2db4 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -88,7 +88,7 @@ func TestFilterError(t *testing.T) { ResourceUser, } - resp, err := Filter(ctx, auth, subject, ActionRead, objects) + _, err := Filter(ctx, auth, subject, ActionRead, objects) require.ErrorIs(t, err, context.Canceled) }) } From 1d509f89300967cd411dee99038e7352e68aec20 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 11:13:18 -0500 Subject: [PATCH 6/8] Fix? --- coderd/rbac/authz_internal_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index e5d382ecf2db4..932555f01ed57 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -44,6 +44,7 @@ func (o *objectBomb) RBACObject() Object { func TestFilterError(t *testing.T) { t.Parallel() + var _ = objectBomb{} t.Run("DifferentResourceTypes", func(t *testing.T) { t.Parallel() From cddbafb3a4c950da6217389e89d89e469324cf78 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 16:13:52 +0000 Subject: [PATCH 7/8] Fix typos --- coderd/rbac/authz_internal_test.go | 4 ++-- coderd/rbac/error.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 932555f01ed57..808c7cf3abbfa 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -63,9 +63,9 @@ func TestFilterError(t *testing.T) { t.Run("CancelledContext", func(t *testing.T) { t.Parallel() - t.Skipf("This test is racy as rego eval checks the ctx cancelled in a go routine. " + + t.Skipf("This test is racy as rego eval checks the ctx canceled in a go routine. " + "It is a coin flip if the query finishes before the 'cancel' is checked. " + - "So sometimes the 'Authorize' call succeeds even if ctx is cancelled.") + "So sometimes the 'Authorize' call succeeds even if ctx is canceled.") auth := NewAuthorizer(prometheus.NewRegistry()) subject := Subject{ diff --git a/coderd/rbac/error.go b/coderd/rbac/error.go index 16e1ad9f466cd..36ff664d67145 100644 --- a/coderd/rbac/error.go +++ b/coderd/rbac/error.go @@ -101,12 +101,12 @@ func (*UnauthorizedError) As(target interface{}) bool { return false } -// correctCancelError will return the correct error for a cancelled context. This +// correctCancelError will return the correct error for a canceled context. This // is because rego changes a canceled context to a topdown.CancelErr. This error -// is not helpful if the code is "cancelled". To make the error conform with the -// rest of our cancelled errors, we will convert the error to a context.Canceled +// is not helpful if the code is "canceled". To make the error conform with the +// rest of our canceled errors, we will convert the error to a context.Canceled // error. No good information is lost, as the topdown.CancelErr provides the -// location of the query that was cancelled, which does not matter. +// location of the query that was canceled, which does not matter. func correctCancelError(err error) error { e := new(topdown.Error) if xerrors.As(err, &e) || e.Code == topdown.CancelErr { From 75b840aab57362b30d9bac7b1c672f9b010daba7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 14 Apr 2023 16:17:21 +0000 Subject: [PATCH 8/8] Fmt --- coderd/rbac/authz_internal_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 808c7cf3abbfa..2d7e33416c23a 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -44,7 +44,7 @@ func (o *objectBomb) RBACObject() Object { func TestFilterError(t *testing.T) { t.Parallel() - var _ = objectBomb{} + _ = objectBomb{} t.Run("DifferentResourceTypes", func(t *testing.T) { t.Parallel()