Skip to content

Commit b909207

Browse files
EmyrkEmyrk
committed
chore: unit tests for cache authz
1 parent 496a096 commit b909207

File tree

7 files changed

+148
-13
lines changed

7 files changed

+148
-13
lines changed

‎coderd/coderd.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -572,13 +572,10 @@ func New(options *Options) *API {
572572
TemplateScheduleStore: options.TemplateScheduleStore,
573573
UserQuietHoursScheduleStore: options.UserQuietHoursScheduleStore,
574574
AccessControlStore: options.AccessControlStore,
575-
FileCache: files.NewFromStore(options.Database, options.PrometheusRegistry, func(ctx context.Context, action policy.Action, object rbac.Object) error {
576-
subject := httpmw.UserAuthorizationCtx(ctx)
577-
return options.Authorizer.Authorize(ctx, subject, action, object)
578-
}),
579-
Experiments: experiments,
580-
WebpushDispatcher: options.WebPushDispatcher,
581-
healthCheckGroup: &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
575+
FileCache: files.NewFromStore(options.Database, options.PrometheusRegistry, options.Authorizer.Authorize),
576+
Experiments: experiments,
577+
WebpushDispatcher: options.WebPushDispatcher,
578+
healthCheckGroup: &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
582579
Acquirer: provisionerdserver.NewAcquirer(
583580
ctx,
584581
options.Logger.Named("acquirer"),

‎coderd/coderdtest/authorize.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,10 @@ func (r *RecordingAuthorizer) AssertOutOfOrder(t *testing.T, actor rbac.Subject,
234234
// AssertActor asserts in order. If the order of authz calls does not match,
235235
// this will fail.
236236
func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did ...ActionObjectPair) {
237+
r.AssertActorID(t, actor.ID, did...)
238+
}
239+
240+
func (r *RecordingAuthorizer) AssertActorID(t *testing.T, id string, did ...ActionObjectPair) {
237241
r.Lock()
238242
defer r.Unlock()
239243
ptr := 0
@@ -242,7 +246,7 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did
242246
// Finished all assertions
243247
return
244248
}
245-
if call.Actor.ID == actor.ID {
249+
if call.Actor.ID == id {
246250
action, object := did[ptr].Action, did[ptr].Object
247251
assert.Equalf(t, action, call.Action, "assert action %d", ptr)
248252
assert.Equalf(t, object, call.Object, "assert object %d", ptr)

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,8 @@ var (
436436
subjectFileReader = rbac.Subject{
437437
Type: rbac.SubjectTypeFileReader,
438438
FriendlyName: "Can Read All Files",
439-
ID: uuid.Nil.String(),
439+
// Arbitrary uuid to have a unique ID for this subject.
440+
ID: rbac.SubjectTypeFileReaderID,
440441
Roles: rbac.Roles([]rbac.Role{
441442
{
442443
Identifier: rbac.RoleIdentifier{Name: "file-reader"},

‎coderd/files/cache.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import (
1919
"github.com/coder/coder/v2/coderd/util/lazy"
2020
)
2121

22-
type AuthorizeFile func(ctx context.Context, action policy.Action, object rbac.Object) error
22+
type AuthorizeFile func(ctx context.Context, subject rbac.Subject, action policy.Action, object rbac.Object) error
2323

2424
// NewFromStore returns a file cache that will fetch files from the provided
2525
// database.
@@ -158,8 +158,12 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
158158
return nil, err
159159
}
160160

161+
subject, ok := dbauthz.ActorFromContext(ctx)
162+
if !ok {
163+
return nil, dbauthz.ErrNoActor
164+
}
161165
// Always check the caller can actually read the file.
162-
if c.authz(ctx, policy.ActionRead, it.object) != nil {
166+
if err := c.authz(ctx, subject, policy.ActionRead, it.object); err != nil {
163167
c.Release(fileID)
164168
return nil, err
165169
}

‎coderd/files/cache_internal_test.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,15 @@ import (
1313
"golang.org/x/sync/errgroup"
1414

1515
"github.com/coder/coder/v2/coderd/coderdtest/promhelp"
16+
"github.com/coder/coder/v2/coderd/rbac"
17+
"github.com/coder/coder/v2/coderd/rbac/policy"
1618
"github.com/coder/coder/v2/testutil"
1719
)
1820

21+
func authzAlwaysTrue(_ context.Context, _ rbac.Subject, _ policy.Action, _ rbac.Object) error {
22+
return nil
23+
}
24+
1925
func cachePromMetricName(metric string) string {
2026
return "coderd_file_cache_" + metric
2127
}
@@ -33,7 +39,7 @@ func TestConcurrency(t *testing.T) {
3339
// will be waiting in line, ensuring that no one duplicated a fetch.
3440
time.Sleep(testutil.IntervalMedium)
3541
return cacheEntryValue{FS: emptyFS, size: fileSize}, nil
36-
}, reg)
42+
}, reg, authzAlwaysTrue)
3743

3844
batches := 1000
3945
groups := make([]*errgroup.Group, 0, batches)
@@ -83,7 +89,7 @@ func TestRelease(t *testing.T) {
8389
FS: emptyFS,
8490
size: fileSize,
8591
}, nil
86-
}, reg)
92+
}, reg, authzAlwaysTrue)
8793

8894
batches := 100
8995
ids := make([]uuid.UUID, 0, batches)

‎coderd/files/cache_test.go

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
package files_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/google/uuid"
7+
"github.com/prometheus/client_golang/prometheus"
8+
"github.com/stretchr/testify/require"
9+
10+
"cdr.dev/slog/sloggers/slogtest"
11+
"github.com/coder/coder/v2/coderd/coderdtest"
12+
"github.com/coder/coder/v2/coderd/database"
13+
"github.com/coder/coder/v2/coderd/database/dbauthz"
14+
"github.com/coder/coder/v2/coderd/database/dbgen"
15+
"github.com/coder/coder/v2/coderd/database/dbtestutil"
16+
"github.com/coder/coder/v2/coderd/files"
17+
"github.com/coder/coder/v2/coderd/rbac"
18+
"github.com/coder/coder/v2/coderd/rbac/policy"
19+
"github.com/coder/coder/v2/testutil"
20+
)
21+
22+
func TestCacheRBAC(t *testing.T) {
23+
t.Parallel()
24+
25+
db, cache, rec := cacheAuthzSetup(t)
26+
ctx := testutil.Context(t, testutil.WaitMedium)
27+
28+
file := dbgen.File(t, db, database.File{})
29+
30+
nobodyID := uuid.New()
31+
nobody := dbauthz.As(ctx, rbac.Subject{
32+
ID: nobodyID.String(),
33+
Roles: rbac.Roles{},
34+
Scope: rbac.ScopeAll,
35+
})
36+
37+
userID := uuid.New()
38+
userReader := dbauthz.As(ctx, rbac.Subject{
39+
ID: userID.String(),
40+
Roles: rbac.Roles{
41+
must(rbac.RoleByName(rbac.RoleTemplateAdmin())),
42+
},
43+
Scope: rbac.ScopeAll,
44+
})
45+
46+
cacheReader := dbauthz.AsFileReader(ctx)
47+
48+
t.Run("NoRolesOpen", func(t *testing.T) {
49+
// Ensure start is clean
50+
require.Equal(t, 0, cache.Count())
51+
rec.Reset()
52+
53+
_, err := cache.Acquire(nobody, file.ID)
54+
require.Error(t, err)
55+
require.True(t, rbac.IsUnauthorizedError(err))
56+
57+
// Ensure that the cache is empty
58+
require.Equal(t, 0, cache.Count())
59+
60+
// Check the assertions
61+
rec.AssertActorID(t, nobodyID.String(), rec.Pair(policy.ActionRead, file))
62+
rec.AssertActorID(t, rbac.SubjectTypeFileReaderID, rec.Pair(policy.ActionRead, file))
63+
})
64+
65+
t.Run("CacheHasFile", func(t *testing.T) {
66+
rec.Reset()
67+
require.Equal(t, 0, cache.Count())
68+
69+
// Read the file with a file reader to put it into the cache.
70+
_, err := cache.Acquire(cacheReader, file.ID)
71+
require.NoError(t, err)
72+
require.Equal(t, 1, cache.Count())
73+
74+
// "nobody" should not be able to read the file.
75+
_, err = cache.Acquire(nobody, file.ID)
76+
require.Error(t, err)
77+
require.True(t, rbac.IsUnauthorizedError(err))
78+
require.Equal(t, 1, cache.Count())
79+
80+
// UserReader can
81+
_, err = cache.Acquire(userReader, file.ID)
82+
require.NoError(t, err)
83+
require.Equal(t, 1, cache.Count())
84+
85+
cache.Release(file.ID)
86+
cache.Release(file.ID)
87+
require.Equal(t, 0, cache.Count())
88+
89+
rec.AssertActorID(t, nobodyID.String(), rec.Pair(policy.ActionRead, file))
90+
rec.AssertActorID(t, rbac.SubjectTypeFileReaderID, rec.Pair(policy.ActionRead, file))
91+
rec.AssertActorID(t, userID.String(), rec.Pair(policy.ActionRead, file))
92+
})
93+
}
94+
95+
func cacheAuthzSetup(t *testing.T) (database.Store, *files.Cache, *coderdtest.RecordingAuthorizer) {
96+
t.Helper()
97+
98+
logger := slogtest.Make(t, &slogtest.Options{})
99+
reg := prometheus.NewRegistry()
100+
101+
db, _ := dbtestutil.NewDB(t)
102+
authz := rbac.NewAuthorizer(reg)
103+
rec := &coderdtest.RecordingAuthorizer{
104+
Called: nil,
105+
Wrapped: authz,
106+
}
107+
108+
// Dbauthz wrap the db
109+
db = dbauthz.New(db, rec, logger, coderdtest.AccessControlStorePointer())
110+
c := files.NewFromStore(db, reg, rec.Authorize)
111+
return db, c, rec
112+
}
113+
114+
func must[T any](t T, err error) T {
115+
if err != nil {
116+
panic(err)
117+
}
118+
return t
119+
}

‎coderd/rbac/authz.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,10 @@ const (
7777
SubjectTypeFileReader SubjectType = "file_reader"
7878
)
7979

80+
const (
81+
SubjectTypeFileReaderID = "acbf0be6-6fed-47b6-8c43-962cb5cab994"
82+
)
83+
8084
// Subject is a struct that contains all the elements of a subject in an rbac
8185
// authorize.
8286
type Subject struct {

0 commit comments

Comments
 (0)