Skip to content

Commit 8134d1b

Browse files
EmyrkEmyrk
committed
Refactor recording authorizer
1 parent 432a261 commit 8134d1b

File tree

3 files changed

+81
-49
lines changed

3 files changed

+81
-49
lines changed

‎coderd/authzquery/methods_test.go

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,13 @@ MethodLoop:
129129
// be expected to be a NotAuthorizedError.
130130
erroredResp := reflect.ValueOf(az).Method(i).Call(append([]reflect.Value{reflect.ValueOf(ctx)}, testCase.Inputs...))
131131
err := findError(t, erroredResp)
132-
require.Errorf(t, err, "method %q should an error with disallow authz", testName)
133-
require.ErrorIsf(t, err, sql.ErrNoRows, "error should match sql.ErrNoRows")
134-
require.ErrorAs(t, err, &authzquery.NotAuthorizedError{}, "error should be NotAuthorizedError")
132+
// This is unfortunate, but if we are using `Filter` the error returned will be nil. So filter out
133+
// any case where the error is nil and the response is an empty slice.
134+
if err != nil || !hasEmptySliceResponse(erroredResp) {
135+
require.Errorf(t, err, "method %q should an error with disallow authz", testName)
136+
require.ErrorIsf(t, err, sql.ErrNoRows, "error should match sql.ErrNoRows")
137+
require.ErrorAs(t, err, &authzquery.NotAuthorizedError{}, "error should be NotAuthorizedError")
138+
}
135139
// Set things back to normal.
136140
fakeAuthorizer.AlwaysReturn = nil
137141
rec.Reset()
@@ -162,6 +166,17 @@ MethodLoop:
162166
require.NoError(t, rec.AllAsserted(), "all rbac calls must be asserted")
163167
}
164168

169+
func hasEmptySliceResponse(values []reflect.Value) bool {
170+
for _, r := range values {
171+
if r.Kind() == reflect.Slice || r.Kind() == reflect.Array {
172+
if r.Len() == 0 {
173+
return true
174+
}
175+
}
176+
}
177+
return false
178+
}
179+
165180
func findError(t *testing.T, values []reflect.Value) error {
166181
for _, r := range values {
167182
if r.Type().Implements(reflect.TypeOf((*error)(nil)).Elem()) {

‎coderd/authzquery/organization.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,11 @@ func (q *AuthzQuerier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, add
115115
}
116116

117117
if len(added) > 0 && q.authorizeContext(ctx, rbac.ActionCreate, roleAssign) != nil {
118-
return xerrors.Errorf("not authorized to assign roles")
118+
return LogNotAuthorizedError(ctx, q.log, xerrors.Errorf("not authorized to assign roles"))
119119
}
120120

121121
if len(removed) > 0 && q.authorizeContext(ctx, rbac.ActionDelete, roleAssign) != nil {
122-
return xerrors.Errorf("not authorized to deleteQ roles")
122+
return LogNotAuthorizedError(ctx, q.log, xerrors.Errorf("not authorized to delete roles"))
123123
}
124124

125125
for _, roleName := range grantedRoles {

‎coderd/coderdtest/authorize.go

Lines changed: 61 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,6 @@ func NewAuthTester(ctx context.Context, t *testing.T, client *codersdk.Client, a
444444
func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck, skipRoutes map[string]string) {
445445
// Always fail auth from this point forward
446446
a.authorizer.Wrapped = &FakeAuthorizer{
447-
Original: a.authorizer,
448447
AlwaysReturn: rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil),
449448
}
450449

@@ -639,40 +638,38 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did
639638
assert.Equalf(t, len(did), ptr, "assert actor: didn't find all actions, %d missing actions", len(did)-ptr)
640639
}
641640

642-
// _AuthorizeSQL does not record the call. This matches the postgres behavior
643-
// of not calling Authorize()
644-
func (r *RecordingAuthorizer) _AuthorizeSQL(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error {
645-
if r.Wrapped == nil {
646-
panic("Developer error: RecordingAuthorizer.Wrapped is nil")
647-
}
648-
return r.Wrapped.Authorize(ctx, subject, action, object)
649-
}
650-
651-
func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error {
641+
func (r *RecordingAuthorizer) RecordAuthorize(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) {
652642
r.Lock()
653643
defer r.Unlock()
654644
r.Called = append(r.Called, authCall{
655645
Actor: subject,
656646
Action: action,
657647
Object: object,
658648
})
649+
}
650+
651+
func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error {
652+
r.RecordAuthorize(ctx, subject, action, object)
659653
if r.Wrapped == nil {
660654
panic("Developer error: RecordingAuthorizer.Wrapped is nil")
661655
}
662656
return r.Wrapped.Authorize(ctx, subject, action, object)
663657
}
664658

665-
func (r *RecordingAuthorizer) Prepare(_ context.Context, subject rbac.Subject, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
659+
func (r *RecordingAuthorizer) Prepare(ctx context.Context, subject rbac.Subject, action rbac.Action, objectType string) (rbac.PreparedAuthorized, error) {
666660
r.RLock()
667661
defer r.RUnlock()
668662
if r.Wrapped == nil {
669663
panic("Developer error: RecordingAuthorizer.Wrapped is nil")
670664
}
671-
return &fakePreparedAuthorizer{
672-
Original: r,
673-
Subject: subject,
674-
Action: action,
675-
HardCodedSQLString: "true",
665+
666+
prep, err := r.Wrapped.Prepare(ctx, subject, action, objectType)
667+
if err != nil {
668+
return nil, err
669+
}
670+
return &PreparedRecorder{
671+
rec: r,
672+
prepped: prep,
676673
}, nil
677674
}
678675

@@ -682,46 +679,63 @@ func (r *RecordingAuthorizer) Reset() {
682679
r.Called = nil
683680
}
684681

682+
// lastCall is implemented to support legacy tests.
683+
// Deprecated
684+
func (r *RecordingAuthorizer) lastCall() *authCall {
685+
r.RLock()
686+
defer r.RUnlock()
687+
if len(r.Called) == 0 {
688+
return nil
689+
}
690+
return &r.Called[len(r.Called)-1]
691+
}
692+
693+
type PreparedRecorder struct {
694+
rec *RecordingAuthorizer
695+
prepped rbac.PreparedAuthorized
696+
subject rbac.Subject
697+
action rbac.Action
698+
699+
rw sync.Mutex
700+
usingSQL bool
701+
}
702+
703+
func (s *PreparedRecorder) Authorize(ctx context.Context, object rbac.Object) error {
704+
s.rw.Lock()
705+
defer s.rw.Unlock()
706+
707+
if !s.usingSQL {
708+
s.rec.RecordAuthorize(ctx, s.subject, s.action, object)
709+
}
710+
return s.prepped.Authorize(ctx, object)
711+
}
712+
func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) {
713+
s.rw.Lock()
714+
defer s.rw.Unlock()
715+
716+
s.usingSQL = true
717+
return s.prepped.CompileToSQL(ctx, cfg)
718+
}
719+
685720
type fakePreparedAuthorizer struct {
686721
sync.RWMutex
687-
Original *RecordingAuthorizer
722+
Original *FakeAuthorizer
688723
Subject rbac.Subject
689724
Action rbac.Action
690-
HardCodedSQLString string
691725
ShouldCompileToSQL bool
692726
}
693727

694728
func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error {
695-
f.RLock()
696-
defer f.RUnlock()
697-
if f.ShouldCompileToSQL {
698-
return f.Original._AuthorizeSQL(ctx, f.Subject, f.Action, object)
699-
}
700729
return f.Original.Authorize(ctx, f.Subject, f.Action, object)
701730
}
702731

703732
// CompileToSQL returns a compiled version of the authorizer that will work for
704733
// in memory databases. This fake version will not work against a SQL database.
705734
func (f *fakePreparedAuthorizer) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) {
706-
f.Lock()
707-
f.ShouldCompileToSQL = true
708-
f.Unlock()
709-
return f.HardCodedSQLString, nil
710-
}
711-
712-
// lastCall is implemented to support legacy tests.
713-
// Deprecated
714-
func (r *RecordingAuthorizer) lastCall() *authCall {
715-
r.RLock()
716-
defer r.RUnlock()
717-
if len(r.Called) == 0 {
718-
return nil
719-
}
720-
return &r.Called[len(r.Called)-1]
735+
return "not a valid sql string", nil
721736
}
722737

723738
type FakeAuthorizer struct {
724-
Original *RecordingAuthorizer
725739
// AlwaysReturn is the error that will be returned by Authorize.
726740
AlwaysReturn error
727741
}
@@ -732,11 +746,14 @@ func (d *FakeAuthorizer) Authorize(_ context.Context, _ rbac.Subject, _ rbac.Act
732746
return d.AlwaysReturn
733747
}
734748

749+
func (d *FakeAuthorizer) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) {
750+
return "not a valid sql string", nil
751+
}
752+
735753
func (d *FakeAuthorizer) Prepare(_ context.Context, subject rbac.Subject, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
736754
return &fakePreparedAuthorizer{
737-
Original: d.Original,
738-
Subject: subject,
739-
Action: action,
740-
HardCodedSQLString: "true",
755+
Original: d,
756+
Subject: subject,
757+
Action: action,
741758
}, nil
742759
}

0 commit comments

Comments
 (0)