8000 chore: break down dbauthz.System into smaller roles by johnstcn · Pull Request #6218 · coder/coder · GitHub
[go: up one dir, main page]

Skip to content

chore: break down dbauthz.System into smaller roles #6218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion coderd/activitybump.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto
}, nil)
if err != nil {
if !xerrors.Is(err, context.Canceled) {
// Bump will fail if the context is cancelled, but this is ok.
// Bump will fail if the context is canceled, but this is ok.
log.Error(ctx, "bump failed", slog.Error(err),
slog.F("workspace_id", workspaceID),
)
Expand Down
4 changes: 2 additions & 2 deletions coderd/autobuild/executor/lifecycle_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ type Stats struct {
// New returns a new autobuild executor.
func New(ctx context.Context, db database.Store, log slog.Logger, tick <-chan time.Time) *Executor {
le := &Executor{
//nolint:gocritic // TODO: make an autostart role instead of using System
ctx: dbauthz.AsSystem(ctx),
//nolint:gocritic // Autostart has a limited set of permissions.
ctx: dbauthz.AsAutostart(ctx),
db: db,
tick: tick,
log: log,
Expand Down
86 changes: 65 additions & 21 deletions coderd/database/dbauthz/dbauthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ func logNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) e
if err != nil && xerrors.As(err, &internalError) {
e := new(topdown.Error)
if xerrors.As(err, &e) || e.Code == topdown.CancelErr {
// For some reason rego changes a cancelled context to a topdown.CancelErr. We
// expect to check for cancelled context errors if the user cancels the request,
// For some reason rego changes a canceled context to a topdown.CancelErr. We
// expect to check for canceled context errors if the user cancels the request,
// so we should change the error to a context.Canceled error.
//
// NotAuthorizedError is == to sql.ErrNoRows, which is not correct
// if it's actually a cancelled context.
// if it's actually a canceled context.
internalError.SetInternal(context.Canceled)
return internalError
}
Expand Down Expand Up @@ -117,29 +117,73 @@ func ActorFromContext(ctx context.Context) (rbac.Subject, bool) {
return a, ok
}

// AsSystem returns a context with a system actor. This is used for internal
// system operations that are not tied to any particular actor.
// When you use this function, be sure to add a //nolint comment
// explaining why it is necessary.
//
// We trust you have received the usual lecture from the local System
// Administrator. It usually boils down to these three things:
// #1) Respect the privacy of others.
// #2) Think before you type.
// #3) With great power comes great responsibility.
func AsSystem(ctx context.Context) context.Context {
// AsProvisionerd returns a context with an actor that has permissions required
// for provisionerd to function.
func AsProvisionerd(ctx context.Context) context.Context {
return context.WithValue(ctx, authContextKey{}, rbac.Subject{
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "provisionerd",
DisplayName: "Provisioner Daemon",
Site: rbac.Permissions(map[string][]rbac.Action{
rbac.ResourceFile.Type: {rbac.ActionRead},
rbac.ResourceTemplate.Type: {rbac.ActionRead, rbac.ActionUpdate},
rbac.ResourceUser.Type: {rbac.ActionRead},
rbac.ResourceWorkspace.Type: {rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete},
}),
Org: map[string][]rbac.Permission{},
User: []rbac.Permission{},
},
}),
Scope: rbac.ScopeAll,
},
)
}

// AsAutostart returns a context with an actor that has permissions required
// for autostart to function.
func AsAutostart(ctx context.Context) context.Context {
return context.WithValue(ctx, authContextKey{}, rbac.Subject{
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "autostart",
DisplayName: "Autostart Daemon",
Site: rbac.Permissions(map[string][]rbac.Action{
rbac.ResourceTemplate.Type: {rbac.ActionRead, rbac.ActionUpdate},
rbac.ResourceWorkspace.Type: {rbac.ActionRead, rbac.ActionUpdate},
}),
Org: map[string][]rbac.Permission{},
User: []rbac.Permission{},
},
}),
Scope: rbac.ScopeAll,
},
)
}

// AsSystemRestricted returns a context with an actor that has permissions
// required for various system operations (login, logout, metrics cache).
func AsSystemRestricted(ctx context.Context) context.Context {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan on keeping this around? Or is it a catch all for the remaining stuff for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a catch-all for the remaining stuff. It's mostly used for HTTP middleware.
I've pared down all the perms except read, which can still be pared down to the bare minimum if need be.

If we need to break it down further in future, we can do so. I think this is fine for now though.

return context.WithValue(ctx, authContextKey{}, rbac.Subject{
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "system",
DisplayName: "System",
Site: []rbac.Permission{
{
ResourceType: rbac.ResourceWildcard.Type,
Action: rbac.WildcardSymbol,
},
},
DisplayName: "Coder",
Site: rbac.Permissions(map[string][]rbac.Action{
rbac.ResourceWildcard.Type: {rbac.ActionRead},
rbac.ResourceAPIKey.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
rbac.ResourceGroup.Type: {rbac.ActionCreate, rbac.ActionUpdate},
rbac.ResourceRoleAssignment.Type: {rbac.ActionCreate},
rbac.ResourceOrganization.Type: {rbac.ActionCreate},
rbac.ResourceOrganizationMember.Type: {rbac.ActionCreate},
rbac.ResourceOrgRoleAssignment.Type: {rbac.ActionCreate},
rbac.ResourceUser.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
rbac.ResourceUserData.Type: {rbac.ActionCreate, rbac.ActionUpdate},
rbac.ResourceWorkspace.Type: {rbac.ActionUpdate},
}),
Org: map[string][]rbac.Permission{},
User: []rbac.Permission{},
},
Expand Down
26 changes: 0 additions & 26 deletions coderd/database/dbauthz/querier.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,13 +327,6 @@ func (q *querier) GetProvisionerDaemons(ctx context.Context) ([]database.Provisi
return fetchWithPostFilter(q.auth, fetch)(ctx, nil)
}

func (q *querier) GetDeploymentDAUs(ctx context.Context) ([]database.GetDeploymentDAUsRow, error) {
if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceUser.All()); err != nil {
return nil, err
}
return q.db.GetDeploymentDAUs(ctx)
}

func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) {
return fetchWithPostFilter(q.auth, q.db.GetGroupsByOrganizationID)(ctx, organizationID)
}
Expand Down Expand Up @@ -622,16 +615,6 @@ func (q *querier) GetPreviousTemplateVersion(ctx context.Context, arg database.G
return q.db.GetPreviousTemplateVersion(ctx, arg)
}

func (q *querier) GetTemplateAverageBuildTime(ctx context.Context, arg database.GetTemplateAverageBuildTimeParams) (database.GetTemplateAverageBuildTimeRow, error) {
// An actor can read the average build time if they can read the related template.
// It doesn't make any sense to get the average build time for a template that doesn't
// exist, so omitting this check here.
if _, err := q.GetTemplateByID(ctx, arg.TemplateID.UUID); err != nil {
return database.GetTemplateAverageBuildTimeRow{}, err
}
return q.db.GetTemplateAverageBuildTime(ctx, arg)
}

func (q *querier) GetTemplateByID(ctx context.Context, id uuid.UUID) (database.Template, error) {
return fetch(q.log, q.auth, q.db.GetTemplateByID)(ctx, id)
}
Expand All @@ -640,15 +623,6 @@ func (q *querier) GetTemplateByOrganizationAndName(ctx context.Context, arg data
return fetch(q.log, q.auth, q.db.GetTemplateByOrganizationAndName)(ctx, arg)
}

func (q *querier) GetTemplateDAUs(ctx context.Context, templateID uuid.UUID) ([]database.GetTemplateDAUsRow, error) {
// An actor can read the DAUs if they can read the related template.
// Again, it doesn't make sense to get DAUs for a template that doesn't exist.
if _, err := q.GetTemplateByID(ctx, templateID); err != nil {
return nil, err
}
return q.db.GetTemplateDAUs(ctx, templateID)
}

func (q *querier) GetTemplateVersionByID(ctx context.Context, tvid uuid.UUID) (database.TemplateVersion, error) {
tv, err := q.db.GetTemplateVersionByID(ctx, tvid)
if err != nil {
Expand Down
13 changes: 0 additions & 13 deletions coderd/database/dbauthz/querier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,12 +540,6 @@ func (s *MethodTestSuite) TestTemplate() {
TemplateID: uuid.NullUUID{UUID: t1.ID, Valid: true},
}).Asserts(t1, rbac.ActionRead).Returns(b)
}))
s.Run("GetTemplateAverageBuildTime", s.Subtest(func(db database.Store, check *expects) {
t1 := dbgen.Template(s.T(), db, database.Template{})
check.Args(database.GetTemplateAverageBuildTimeParams{
TemplateID: uuid.NullUUID{UUID: t1.ID, Valid: true},
}).Asserts(t1, rbac.ActionRead)
}))
s.Run("GetTemplateByID", s.Subtest(func(db database.Store, check *expects) {
t1 := dbgen.Template(s.T(), db, database.Template{})
check.Args(t1.ID).Asserts(t1, rbac.ActionRead).Returns(t1)
Expand All @@ -560,10 +554,6 @@ func (s *MethodTestSuite) TestTemplate() {
OrganizationID: o1.ID,
}).Asserts(t1, rbac.ActionRead).Returns(t1)
}))
s.Run("GetTemplateDAUs", s.Subtest(func(db database.Store, check *expects) {
t1 := dbgen.Template(s.T(), db, database.Template{})
check.Args(t1.ID).Asserts(t1, rbac.ActionRead)
}))
s.Run("GetTemplateVersionByJobID", s.Subtest(func(db database.Store, check *expects) {
t1 := dbgen.Template(s.T(), db, database.Template{})
tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
Expand Down Expand Up @@ -1220,7 +1210,4 @@ func (s *MethodTestSuite) TestExtraMethods() {
s.NoError(err, "insert provisioner daemon")
check.Args().Asserts(d, rbac.ActionRead)
}))
s.Run("GetDeploymentDAUs", s.Subtest(func(db database.Store, check *expects) {
check.Args().Asserts(rbac.ResourceUser.All(), rbac.ActionRead)
}))
}
4 changes: 2 additions & 2 deletions coderd/database/dbauthz/setup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,8 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd
}
})

s.Run("Cancelled", func() {
// Pass in a cancelled context
s.Run("Canceled", func() {
// Pass in a canceled context
ctx, cancel := context.WithCancel(ctx)
cancel()
az.AlwaysReturn = rbac.ForbiddenWithInternal(&topdown.Error{Code: topdown.CancelErr},
Expand Down
15 changes: 15 additions & 0 deletions coderd/database/dbauthz/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,21 @@ func (q *querier) GetTemplates(ctx context.Context) ([]database.Template, error)
return q.db.GetTemplates(ctx)
}

// Only used by metrics cache.
func (q *querier) GetTemplateAverageBuildTime(ctx context.Context, arg database.GetTemplateAverageBuildTimeParams) (database.GetTemplateAverageBuildTimeRow, error) {
return q.db.GetTemplateAverageBuildTime(ctx, arg)
}

// Only used by metrics cache.
func (q *querier) GetTemplateDAUs(ctx context.Context, templateID uuid.UUID) ([]database.GetTemplateDAUsRow, error) {
return q.db.GetTemplateDAUs(ctx, templateID)
}

// Only used by metrics cache.
func (q *querier) GetDeploymentDAUs(ctx context.Context) ([]database.GetDeploymentDAUsRow, error) {
return q.db.GetDeploymentDAUs(ctx)
}

// UpdateWorkspaceBuildCostByID is used by the provisioning system to update the cost of a workspace build.
func (q *querier) UpdateWorkspaceBuildCostByID(ctx context.Context, arg database.UpdateWorkspaceBuildCostByIDParams) (database.WorkspaceBuild, error) {
return q.db.UpdateWorkspaceBuildCostByID(ctx, arg)
Expand Down
12 changes: 6 additions & 6 deletions coderd/httpmw/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
}

//nolint:gocritic // System needs to fetch API key to check if it's valid.
key, err := cfg.DB.GetAPIKeyByID(dbauthz.AsSystem(ctx), keyID)
key, err := cfg.DB.GetAPIKeyByID(dbauthz.AsSystemRestricted(ctx), keyID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
optionalWrite(http.StatusUnauthorized, codersdk.Response{
Expand Down Expand Up @@ -195,7 +195,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
)
if key.LoginType == database.LoginTypeGithub || key.LoginType == database.LoginTypeOIDC {
//nolint:gocritic // System needs to fetch UserLink to check if it's valid.
link, err = cfg.DB.GetUserLinkByUserIDLoginType(dbauthz.AsSystem(ctx), database.GetUserLinkByUserIDLoginTypeParams{
link, err = cfg.DB.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
UserID: key.UserID,
LoginType: key.LoginType,
})
Expand Down Expand Up @@ -279,7 +279,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
}
if changed {
//nolint:gocritic // System needs to update API Key LastUsed< F987 /span>
err := cfg.DB.UpdateAPIKeyByID(dbauthz.AsSystem(ctx), database.UpdateAPIKeyByIDParams{
err := cfg.DB.UpdateAPIKeyByID(dbauthz.AsSystemRestricted(ctx), database.UpdateAPIKeyByIDParams{
ID: key.ID,
LastUsed: key.LastUsed,
ExpiresAt: key.ExpiresAt,
Expand All @@ -296,7 +296,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
// then we want to update the relevant oauth fields.
if link.UserID != uuid.Nil {
// nolint:gocritic
link, err = cfg.DB.UpdateUserLink(dbauthz.AsSystem(ctx), database.UpdateUserLinkParams{
link, err = cfg.DB.UpdateUserLink(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLinkParams{
UserID: link.UserID,
LoginType: link.LoginType,
OAuthAccessToken: link.OAuthAccessToken,
Expand All @@ -316,7 +316,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
// load. We update alongside the UserLink and APIKey since it's
// easier on the DB to colocate writes.
// nolint:gocritic
_, err = cfg.DB.UpdateUserLastSeenAt(dbauthz.AsSystem(ctx), database.UpdateUserLastSeenAtParams{
_, err = cfg.DB.UpdateUserLastSeenAt(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLastSeenAtParams{
ID: key.UserID,
LastSeenAt: database.Now(),
UpdatedAt: database.Now(),
Expand All @@ -334,7 +334,7 @@ func ExtractAPIKey(cfg ExtractAPIKeyConfig) func(http.Handler) http.Handler {
// The roles are used for RBAC authorize checks, and the status
// is to block 'suspended' users from accessing the platform.
// nolint:gocritic
roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystem(ctx), key.UserID)
roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), key.UserID)
if err != nil {
write(http.StatusUnauthorized, codersdk.Response{
Message: internalErrorMessage,
Expand Down
2 changes: 1 addition & 1 deletion coderd/httpmw/authz.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func AsAuthzSystem(mws ...func(http.Handler) http.Handler) func(http.Handler) ht
}

// nolint:gocritic // AsAuthzSystem needs to do this.
r = r.WithContext(dbauthz.AsSystem(ctx))
r = r.WithContext(dbauthz.AsSystemRestricted(ctx))
chain.Handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
r = r.WithContext(dbauthz.As(r.Context(), before))
next.ServeHTTP(rw, r)
Expand Down
2 changes: 1 addition & 1 deletion coderd/httpmw/hsts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func TestHSTS(t *testing.T) {
req := httptest.NewRequest("GET", "/", nil)
res := httptest.NewRecorder()
got.ServeHTTP(res, req)

require.Equal(t, tt.expectHeader, res.Header().Get("Strict-Transport-Security"), "expected header value")
})
}
Expand Down
6 changes: 3 additions & 3 deletions coderd/httpmw/userparam.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
return
}
//nolint:gocritic // System needs to be able to get user from param.
user, err = db.GetUserByID(dbauthz.AsSystem(ctx), apiKey.UserID)
user, err = db.GetUserByID(dbauthz.AsSystemRestricted(ctx), apiKey.UserID)
if xerrors.Is(err, sql.ErrNoRows) {
httpapi.ResourceNotFound(rw)
return
Expand All @@ -84,7 +84,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
}
} else if userID, err := uuid.Parse(userQuery); err == nil {
//nolint:gocritic // If the userQuery is a valid uuid
user, err = db.GetUserByID(dbauthz.AsSystem(ctx), userID)
user, err = db.GetUserByID(dbauthz.AsSystemRestricted(ctx), userID)
if err != nil {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: userErrorMessage,
Expand All @@ -93,7 +93,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han
}
} else {
// nolint:gocritic // Try as a username last
user, err = db.GetUserByEmailOrUsername(dbauthz.AsSystem(ctx), database.GetUserByEmailOrUsernameParams{
user, err = db.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{
Username: userQuery,
})
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions coderd/httpmw/workspaceagent.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func ExtractWorkspaceAgent(db database.Store) func(http.Handler) http.Handler {
return
}
//nolint:gocritic // System needs to be able to get workspace agents.
agent, err := db.GetWorkspaceAgentByAuthToken(dbauthz.AsSystem(ctx), token)
agent, err := db.GetWorkspaceAgentByAuthToken(dbauthz.AsSystemRestricted(ctx), token)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
Expand All @@ -66,7 +66,7 @@ func ExtractWorkspaceAgent(db database.Store) func(http.Handler) http.Handler {
}

//nolint:gocritic // System needs to be able to get workspace agents.
subject, err := getAgentSubject(dbauthz.AsSystem(ctx), db, agent)
subject, err := getAgentSubject(dbauthz.AsSystemRestricted(ctx), db, agent)
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error fetching workspace agent.",
Expand Down
2 changes: 1 addition & 1 deletion coderd/metricscache/metricscache.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func countUniqueUsers(rows []database.GetTemplateDAUsRow) int {

func (c *Cache) refresh(ctx context.Context) error {
//nolint:gocritic // This is a system service.
ctx = dbauthz.AsSystem(ctx)
ctx = dbauthz.AsSystemRestricted(ctx)
err := c.database.DeleteOldAgentStats(ctx)
if err != nil {
return xerrors.Errorf("delete old stats: %w", err)
Expand Down
Loading
0