Skip to content

Commit f0f39b4

Browse files
johnstcnjohnstcn
authored
chore: break down dbauthz.System into smaller roles (coder#6218)
- rbac: export rbac.Permissions - dbauthz: move GetDeploymentDAUs, GetTemplateDAUs, GetTemplateAverageBuildTime from querier.go to system.go and removes auth checks - dbauthz: remove AsSystem(), add individual roles for autostart, provisionerd, add restricted system role for everything else
1 parent 84da605 commit f0f39b4

25 files changed

+180
-141
lines changed

‎coderd/activitybump.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto
8383
}, nil)
8484
if err != nil {
8585
if !xerrors.Is(err, context.Canceled) {
86-
// Bump will fail if the context is cancelled, but this is ok.
86+
// Bump will fail if the context is canceled, but this is ok.
8787
log.Error(ctx, "bump failed", slog.Error(err),
8888
slog.F("workspace_id", workspaceID),
8989
)

‎coderd/autobuild/executor/lifecycle_executor.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ type Stats struct {
3434
// New returns a new autobuild executor.
3535
func New(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor {
3636
le := &Executor{
37-
//nolint:gocritic // TODO: make an autostart role instead of using System
38-
ctx: dbauthz.AsSystem(ctx),
37+
//nolint:gocritic // Autostart has a limited set of permissions.
38+
ctx: dbauthz.AsAutostart(ctx),
3939
db: db,
4040
tick: tick,
4141
log: log,

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 65 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,12 @@ func logNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) e
4646
if err != nil && xerrors.As(err, &internalError) {
4747
e := new(topdown.Error)
4848
if xerrors.As(err, &e) || e.Code == topdown.CancelErr {
49-
// For some reason rego changes a cancelled context to a topdown.CancelErr. We
50-
// expect to check for cancelled context errors if the user cancels the request,
49+
// For some reason rego changes a canceled context to a topdown.CancelErr. We
50+
// expect to check for canceled context errors if the user cancels the request,
5151
// so we should change the error to a context.Canceled error.
5252
//
5353
// NotAuthorizedError is == to sql.ErrNoRows, which is not correct
54-
// if it's actually a cancelled context.
54+
// if it's actually a canceled context.
5555
internalError.SetInternal(context.Canceled)
5656
return internalError
5757
}
@@ -117,29 +117,73 @@ func ActorFromContext(ctx context.Context) (rbac.Subject, bool) {
117117
return a, ok
118118
}
119119

120-
// AsSystem returns a context with a system actor. This is used for internal
121-
// system operations that are not tied to any particular actor.
122-
// When you use this function, be sure to add a //nolint comment
123-
// explaining why it is necessary.
124-
//
125-
// We trust you have received the usual lecture from the local System
126-
// Administrator. It usually boils down to these three things:
127-
// #1) Respect the privacy of others.
128-
// #2) Think before you type.
129-
// #3) With great power comes great responsibility.
130-
func AsSystem(ctx context.Context) context.Context {
120+
// AsProvisionerd returns a context with an actor that has permissions required
121+
// for provisionerd to function.
122+
func AsProvisionerd(ctx context.Context) context.Context {
123+
return context.WithValue(ctx, authContextKey{}, rbac.Subject{
124+
ID: uuid.Nil.String(),
125+
Roles: rbac.Roles([]rbac.Role{
126+
{
127+
Name: "provisionerd",
128+
DisplayName: "Provisioner Daemon",
129+
Site: rbac.Permissions(map[string][]rbac.Action{
130+
rbac.ResourceFile.Type: {rbac.ActionRead},
131+
rbac.ResourceTemplate.Type: {rbac.ActionRead, rbac.ActionUpdate},
132+
rbac.ResourceUser.Type: {rbac.ActionRead},
133+
rbac.ResourceWorkspace.Type: {rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete},
134+
}),
135+
Org: map[string][]rbac.Permission{},
136+
User: []rbac.Permission{},
137+
},
138+
}),
139+
Scope: rbac.ScopeAll,
140+
},
141+
)
142+
}
143+
144+
// AsAutostart returns a context with an actor that has permissions required
145+
// for autostart to function.
146+
func AsAutostart(ctx context.Context) context.Context {
147+
return context.WithValue(ctx, authContextKey{}, rbac.Subject{
148+
ID: uuid.Nil.String(),
149+
Roles: rbac.Roles([]rbac.Role{
150+
{
151+
Name: "autostart",
152+
DisplayName: "Autostart Daemon",
153+
Site: rbac.Permissions(map[string][]rbac.Action{
154+
rbac.ResourceTemplate.Type: {rbac.ActionRead, rbac.ActionUpdate},
155+
rbac.ResourceWorkspace.Type: {rbac.ActionRead, rbac.ActionUpdate},
156+
}),
157+
Org: map[string][]rbac.Permission{},
158+
User: []rbac.Permission{},
159+
},
160+
}),
161+
Scope: rbac.ScopeAll,
162+
},
163+
)
164+
}
165+
166+
// AsSystemRestricted returns a context with an actor that has permissions
167+
// required for various system operations (login, logout, metrics cache).
168+
func AsSystemRestricted(ctx context.Context) context.Context {
131169
return context.WithValue(ctx, authContextKey{}, rbac.Subject{
132170
ID: uuid.Nil.String(),
133171
Roles: rbac.Roles([]rbac.Role{
134172
{
135173
Name: "system",
136-
DisplayName: "System",
137-
Site: []rbac.Permission{
138-
{
139-
ResourceType: rbac.ResourceWildcard.Type,
140-
Action: rbac.WildcardSymbol,
141-
},
142-
},
174+
DisplayName: "Coder",
175+
Site: rbac.Permissions(map[string][]rbac.Action{
176+
rbac.ResourceWildcard.Type: {rbac.ActionRead},
177+
rbac.ResourceAPIKey.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
178+
rbac.ResourceGroup.Type: {rbac.ActionCreate, rbac.ActionUpdate},
179+
rbac.ResourceRoleAssignment.Type: {rbac.ActionCreate},
180+
rbac.ResourceOrganization.Type: {rbac.ActionCreate},
181+
rbac.ResourceOrganizationMember.Type: {rbac.ActionCreate},
182+
rbac.ResourceOrgRoleAssignment.Type: {rbac.ActionCreate},
183+
rbac.ResourceUser.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
184+
rbac.ResourceUserData.Type: {rbac.ActionCreate, rbac.ActionUpdate},
185+
rbac.ResourceWorkspace.Type: {rbac.ActionUpdate},
186+
}),
143187
Org: map[string][]rbac.Permission{},
144188
User: []rbac.Permission{},
145189
},

‎coderd/database/dbauthz/querier.go

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -327,13 +327,6 @@ func (q *querier) GetProvisionerDaemons(ctx context.Context) ([]database.Provisi
327327
return fetchWithPostFilter(q.auth, fetch)(ctx, nil)
328328
}
329329

330-
func (q *querier) GetDeploymentDAUs(ctx context.Context) ([]database.GetDeploymentDAUsRow, error) {
331-
if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceUser.All()); err != nil {
332-
return nil, err
333-
}
334-
return q.db.GetDeploymentDAUs(ctx)
335-
}
336-
337330
func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) {
338331
return fetchWithPostFilter(q.auth, q.db.GetGroupsByOrganizationID)(ctx, organizationID)
339332
}
@@ -622,16 +615,6 @@ func (q *querier) GetPreviousTemplateVersion(ctx context.Context, arg database.G
622615
return q.db.GetPreviousTemplateVersion(ctx, arg)
623616
}
624617

625-
func (q *querier) GetTemplateAverageBuildTime(ctx context.Context, arg database.GetTemplateAverageBuildTimeParams) (database.GetTemplateAverageBuildTimeRow, error) {
626-
// An actor can read the average build time if they can read the related template.
627-
// It doesn't make any sense to get the average build time for a template that doesn't
628-
// exist, so omitting this check here.
629-
if _, err := q.GetTemplateByID(ctx, arg.TemplateID.UUID); err != nil {
630-
return database.GetTemplateAverageBuildTimeRow{}, err
631-
}
632-
return q.db.GetTemplateAverageBuildTime(ctx, arg)
633-
}
634-
635618
func (q *querier) GetTemplateByID(ctx context.Context, id uuid.UUID) (database.Template, error) {
636619
return fetch(q.log, q.auth, q.db.GetTemplateByID)(ctx, id)
637620
}
@@ -640,15 +623,6 @@ func (q *querier) GetTemplateByOrganizationAndName(ctx context.Context, arg data
640623
return fetch(q.log, q.auth, q.db.GetTemplateByOrganizationAndName)(ctx, arg)
641624
}
642625

643-
func (q *querier) GetTemplateDAUs(ctx context.Context, templateID uuid.UUID) ([]database.GetTemplateDAUsRow, error) {
644-
// An actor can read the DAUs if they can read the related template.
645-
// Again, it doesn't make sense to get DAUs for a template that doesn't exist.
646-
if _, err := q.GetTemplateByID(ctx, templateID); err != nil {
647-
return nil, err
648-
}
649-
return q.db.GetTemplateDAUs(ctx, templateID)
650-
}
651-
652626
func (q *querier) GetTemplateVersionByID(ctx context.Context, tvid uuid.UUID) (database.TemplateVersion, error) {
653627
tv, err := q.db.GetTemplateVersionByID(ctx, tvid)
654628
if err != nil {

‎coderd/database/dbauthz/querier_test.go

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -540,12 +540,6 @@ func (s *MethodTestSuite) TestTemplate() {
540540
TemplateID: uuid.NullUUID{UUID: t1.ID, Valid: true},
541541
}).Asserts(t1, rbac.ActionRead).Returns(b)
542542
}))
543-
s.Run("GetTemplateAverageBuildTime", s.Subtest(func(db database.Store, check *expects) {
544-
t1 := dbgen.Template(s.T(), db, database.Template{})
545-
check.Args(database.GetTemplateAverageBuildTimeParams{
546-
TemplateID: uuid.NullUUID{UUID: t1.ID, Valid: true},
547-
}).Asserts(t1, rbac.ActionRead)
548-
}))
549543
s.Run("GetTemplateByID", s.Subtest(func(db database.Store, check *expects) {
550544
t1 := dbgen.Template(s.T(), db, database.Template{})
551545
check.Args(t1.ID).Asserts(t1, rbac.ActionRead).Returns(t1)
@@ -560,10 +554,6 @@ func (s *MethodTestSuite) TestTemplate() {
560554
OrganizationID: o1.ID,
561555
}).Asserts(t1, rbac.ActionRead).Returns(t1)
562556
}))
563-
s.Run("GetTemplateDAUs", s.Subtest(func(db database.Store, check *expects) {
564-
t1 := dbgen.Template(s.T(), db, database.Template{})
565-
check.Args(t1.ID).Asserts(t1, rbac.ActionRead)
566-
}))
567557
s.Run("GetTemplateVersionByJobID", s.Subtest(func(db database.Store, check *expects) {
568558
t1 := dbgen.Template(s.T(), db, database.Template{})
569559
tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
@@ -1220,7 +1210,4 @@ func (s *MethodTestSuite) TestExtraMethods() {
12201210
s.NoError(err, "insert provisioner daemon")
12211211
check.Args().Asserts(d, rbac.ActionRead)
12221212
}))
1223-
s.Run("GetDeploymentDAUs", s.Subtest(func(db database.Store, check *expects) {
1224-
check.Args().Asserts(rbac.ResourceUser.All(), rbac.ActionRead)
1225-
}))
12261213
}

‎coderd/database/dbauthz/setup_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,8 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd
226226
}
227227
})
228228

229-
s.Run("Cancelled", func() {
230-
// Pass in a cancelled context
229+
s.Run("Canceled", func() {
230+
// Pass in a canceled context
231231
ctx, cancel := context.WithCancel(ctx)
232232
cancel()
233233
az.AlwaysReturn = rbac.ForbiddenWithInternal(&topdown.Error{Code: topdown.CancelErr},

‎coderd/database/dbauthz/system.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,21 @@ func (q *querier) GetTemplates(ctx context.Context) ([]database.Template, error)
9696
return q.db.GetTemplates(ctx)
9797
}
9898

99+
// Only used by metrics cache.
100+
func (q *querier) GetTemplateAverageBuildTime(ctx context.Context, arg database.GetTemplateAverageBuildTimeParams) (database.GetTemplateAverageBuildTimeRow, error) {
101+
return q.db.GetTemplateAverageBuildTime(ctx, arg)
102+
}
103+
104+
// Only used by metrics cache.
105+
func (q *querier) GetTemplateDAUs(ctx context.Context, templateID uuid.UUID) ([]database.GetTemplateDAUsRow, error) {
106+
return q.db.GetTemplateDAUs(ctx, templateID)
107+
}
108+
109+
// Only used by metrics cache.
110+
func (q *querier) GetDeploymentDAUs(ctx context.Context) ([]database.GetDeploymentDAUsRow, error) {
111+
return q.db.GetDeploymentDAUs(ctx)
112+
}
113+
99114
// UpdateWorkspaceBuildCostByID is used by the provisioning system to update the cost of a workspace build.
100115
func (q *querier) UpdateWorkspaceBuildCostByID(ctx context.Context, arg database.UpdateWorkspaceBuildCostByIDParams) (database.WorkspaceBuild, error) {
101116
return q.db.UpdateWorkspaceBuildCostByID(ctx, arg)

‎coderd/httpmw/apikey.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
161161
}
162162

163163
//nolint:gocritic // System needs to fetch API key to check if it's valid.
164-
key, err := cfg.DB.GetAPIKeyByID(dbauthz.AsSystem(ctx), keyID)
164+
key, err := cfg.DB.GetAPIKeyByID(dbauthz.AsSystemRestricted(ctx), keyID)
165165
if err != nil {
166166
if errors.Is(err, sql.ErrNoRows) {
167167
optionalWrite(http.StatusUnauthorized, codersdk.Response{
@@ -195,7 +195,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
195195
)
196196
if key.LoginType == database.LoginTypeGithub || key.LoginType == database.LoginTypeOIDC {
197197
//nolint:gocritic // System needs to fetch UserLink to check if it's valid.
198-
link, err = cfg.DB.GetUserLinkByUserIDLoginType(dbauthz.AsSystem(ctx), database.GetUserLinkByUserIDLoginTypeParams{
198+
link, err = cfg.DB.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
199199
UserID: key.UserID,
200200
LoginType: key.LoginType,
201201
})
@@ -279,7 +279,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
279279
}
280280
if changed {
281281
//nolint:gocritic // System needs to update API Key LastUsed
282-
err := cfg.DB.UpdateAPIKeyByID(dbauthz.AsSystem(ctx), database.UpdateAPIKeyByIDParams{
282+
err := cfg.DB.UpdateAPIKeyByID(dbauthz.AsSystemRestricted(ctx), database.UpdateAPIKeyByIDParams{
283283
ID: key.ID,
284284
LastUsed: key.LastUsed,
285285
ExpiresAt: key.ExpiresAt,
@@ -296,7 +296,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
296296
// then we want to update the relevant oauth fields.
297297
if link.UserID != uuid.Nil {
298298
// nolint:gocritic
299-
link, err = cfg.DB.UpdateUserLink(dbauthz.AsSystem(ctx), database.UpdateUserLinkParams{
299+
link, err = cfg.DB.UpdateUserLink(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLinkParams{
300300
UserID: link.UserID,
301301
LoginType: link.LoginType,
302302
OAuthAccessToken: link.OAuthAccessToken,
@@ -316,7 +316,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
316316
// load. We update alongside the UserLink and APIKey since it's
317317
// easier on the DB to colocate writes.
318318
// nolint:gocritic
319-
_, err = cfg.DB.UpdateUserLastSeenAt(dbauthz.AsSystem(ctx), database.UpdateUserLastSeenAtParams{
319+
_, err = cfg.DB.UpdateUserLastSeenAt(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLastSeenAtParams{
320320
ID: key.UserID,
321321
LastSeenAt: database.Now(),
322322
UpdatedAt: database.Now(),
@@ -334,7 +334,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
334334
// The roles are used for RBAC authorize checks, and the status
335335
// is to block 'suspended' users from accessing the platform.
336336
// nolint:gocritic
337-
roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystem(ctx), key.UserID)
337+
roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), key.UserID)
338338
if err != nil {
339339
write(http.StatusUnauthorized, codersdk.Response{
340340
Message: internalErrorMessage,

‎coderd/httpmw/authz.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func AsAuthzSystem(mws ...func(http.Handler) http.Handler) func(http.Handler) ht
2828
}
2929

3030
// nolint:gocritic // AsAuthzSystem needs to do this.
31-
r = r.WithContext(dbauthz.AsSystem(ctx))
31+
r = r.WithContext(dbauthz.AsSystemRestricted(ctx))
3232
chain.Handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
3333
r = r.WithContext(dbauthz.As(r.Context(), before))
3434
next.ServeHTTP(rw, r)

‎coderd/httpmw/hsts_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ func TestHSTS(t *testing.T) {
9696
req := httptest.NewRequest("GET", "/", nil)
9797
res := httptest.NewRecorder()
9898
got.ServeHTTP(res, req)
99-
99+
100100
require.Equal(t, tt.expectHeader, res.Header().Get("Strict-Transport-Security"), "expected header value")
101101
})
102102
}

‎coderd/httpmw/userparam.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
7070
return
7171
}
7272
//nolint:gocritic // System needs to be able to get user from param.
73-
user, err = db.GetUserByID(dbauthz.AsSystem(ctx), apiKey.UserID)
73+
user, err = db.GetUserByID(dbauthz.AsSystemRestricted(ctx), apiKey.UserID)
7474
if xerrors.Is(err, sql.ErrNoRows) {
7575
httpapi.ResourceNotFound(rw)
7676
return
@@ -84,7 +84,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
8484
}
8585
} else if userID, err := uuid.Parse(userQuery); err == nil {
8686
//nolint:gocritic // If the userQuery is a valid uuid
87-
user, err = db.GetUserByID(dbauthz.AsSystem(ctx), userID)
87+
user, err = db.GetUserByID(dbauthz.AsSystemRestricted(ctx), userID)
8888
if err != nil {
8989
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
9090
Message: userErrorMessage,
@@ -93,7 +93,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
9393
}
9494
} else {
9595
// nolint:gocritic // Try as a username last
96-
user, err = db.GetUserByEmailOrUsername(dbauthz.AsSystem(ctx), database.GetUserByEmailOrUsernameParams{
96+
user, err = db.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{
9797
Username: userQuery,
9898
})
9999
if err != nil {

‎coderd/httpmw/workspaceagent.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func ExtractWorkspaceAgent(db database.Store) func(http.Handler) http.Handler {
4848
return
4949
}
5050
//nolint:gocritic // System needs to be able to get workspace agents.
51-
agent, err := db.GetWorkspaceAgentByAuthToken(dbauthz.AsSystem(ctx), token)
51+
agent, err := db.GetWorkspaceAgentByAuthToken(dbauthz.AsSystemRestricted(ctx), token)
5252
if err != nil {
5353
if errors.Is(err, sql.ErrNoRows) {
5454
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
@@ -66,7 +66,7 @@ func ExtractWorkspaceAgent(db database.Store) func(http.Handler) http.Handler {
6666
}
6767

6868
//nolint:gocritic // System needs to be able to get workspace agents.
69-
subject, err := getAgentSubject(dbauthz.AsSystem(ctx), db, agent)
69+
subject, err := getAgentSubject(dbauthz.AsSystemRestricted(ctx), db, agent)
7070
if err != nil {
7171
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
7272
Message: "Internal error fetching workspace agent.",

‎coderd/metricscache/metricscache.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func countUniqueUsers(rows []database.GetTemplateDAUsRow) int {
144144

145145
func (c *Cache) refresh(ctx context.Context) error {
146146
//nolint:gocritic // This is a system service.
147-
ctx = dbauthz.AsSystem(ctx)
147+
ctx = dbauthz.AsSystemRestricted(ctx)
148148
err := c.database.DeleteOldAgentStats(ctx)
149149
if err != nil {
150150
return xerrors.Errorf("delete old stats: %w", err)

0 commit comments

Comments
 (0)