From df52ecb6640495da76f10b85fd9f93af0b945b91 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 17 Nov 2023 09:54:50 -0600 Subject: [PATCH 1/2] chore: return context.Canceled when in Prepare for rbac Was returning a custom rego canceled error. This conforms with how Authorize handles this error. --- coderd/rbac/authz.go | 1 + coderd/rbac/authz_internal_test.go | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 42625b07c3e0a..f97f593d20b50 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -395,6 +395,7 @@ func (a RegoAuthorizer) Prepare(ctx context.Context, subject Subject, action Act prepared, err := a.newPartialAuthorizer(ctx, subject, action, objectType) if err != nil { + err = correctCancelError(err) return nil, xerrors.Errorf("new partial authorizer: %w", err) } diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index e264e31c73a8c..555d4491639af 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -1023,6 +1023,24 @@ func TestAuthorizeScope(t *testing.T) { ) } +func TestCanceledPrepare(t *testing.T) { + t.Parallel() + + authorizer := NewAuthorizer(prometheus.NewRegistry()) + // This context is canceled intentionally to test the prepare error. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // The error should be a `context.Canceled` error. + // By default Rego throws a custom cancelled error. + _, err := authorizer.Prepare(ctx, Subject{ + ID: "foo", + Roles: RoleNames{RoleOwner()}, + Scope: ScopeAll, + }, ActionRead, ResourceWorkspace.Type) + require.ErrorIs(t, err, context.Canceled, "expected canceled context") +} + // 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 { From 038f7973b94d50860f4b6b17817a3fc7b7abd09a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 17 Nov 2023 10:43:55 -0600 Subject: [PATCH 2/2] Had to remove the unit test for cancel Rego uses a go routine to track the ctx cancelled, so if the rego query completes before that go routine even runs, or checks, then there is no error. This is a race condition in how rego handles context cancels. --- coderd/rbac/authz_internal_test.go | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 555d4491639af..e264e31c73a8c 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -1023,24 +1023,6 @@ func TestAuthorizeScope(t *testing.T) { ) } -func TestCanceledPrepare(t *testing.T) { - t.Parallel() - - authorizer := NewAuthorizer(prometheus.NewRegistry()) - // This context is canceled intentionally to test the prepare error. - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - // The error should be a `context.Canceled` error. - // By default Rego throws a custom cancelled error. - _, err := authorizer.Prepare(ctx, Subject{ - ID: "foo", - Roles: RoleNames{RoleOwner()}, - Scope: ScopeAll, - }, ActionRead, ResourceWorkspace.Type) - require.ErrorIs(t, err, context.Canceled, "expected canceled context") -} - // 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 {