selfhosted
/
coder
mirror of https://github.com/coder/coder.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

chore: Skip authz on various functions used for api data building (#6366) 

* chore: Skip authz on various functions used for api data building

API already fetches the parent object and does the rbac check. Until
these functions are optimized, skipping authz is better.
It leaves us no worse off than the status quo
This commit is contained in:
Steven Masley 2023-02-28 12:04:39 -05:00 committed by GitHub
parent d30da81f42
commit 418a8a77dc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 65 additions and 127 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

115
coderd/database/dbauthz/querier.go
View File

@ -251,12 +251,6 @@ func (q *querier) GetProvisionerJobByID(ctx context.Context, id uuid.UUID) (data
return job, nil
}
func (q *querier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.ProvisionerJob, error) {
// TODO: This is missing authorization and is incorrect. This call is used by telemetry, and by 1 http route.
// That http handler should find a better way to fetch these jobs with easier rbac authz.
return q.db.GetProvisionerJobsByIDs(ctx, ids)
}
func (q *querier) GetProvisionerLogsByIDBetween(ctx context.Context, arg database.GetProvisionerLogsByIDBetweenParams) ([]database.ProvisionerJobLog, error) {
// Authorized read on job lets the actor also read the logs.
_, err := q.GetProvisionerJobByID(ctx, arg.JobID)
@ -725,35 +719,6 @@ func (q *querier) GetTemplateVersionVariables(ctx context.Context, templateVersi
return q.db.GetTemplateVersionVariables(ctx, templateVersionID)
}
func (q *querier) GetTemplateVersionsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.TemplateVersion, error) {
// TODO: This is so inefficient
versions, err := q.db.GetTemplateVersionsByIDs(ctx, ids)
if err != nil {
return nil, err
}
checked := make(map[uuid.UUID]bool)
for _, v := range versions {
if _, ok := checked[v.TemplateID.UUID]; ok {
continue
}
obj := v.RBACObjectNoTemplate()
template, err := q.db.GetTemplateByID(ctx, v.TemplateID.UUID)
if err == nil {
obj = v.RBACObject(template)
}
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
return nil, err
}
if err := q.authorizeContext(ctx, rbac.ActionRead, obj); err != nil {
return nil, err
}
checked[v.TemplateID.UUID] = true
}
return versions, nil
}
func (q *querier) GetTemplateVersionsByTemplateID(ctx context.Context, arg database.GetTemplateVersionsByTemplateIDParams) ([]database.TemplateVersion, error) {
// An actor can read template versions if they can read the related template.
template, err := q.db.GetTemplateByID(ctx, arg.TemplateID)
@ -1013,11 +978,6 @@ func (q *querier) GetUsersWithCount(ctx context.Context, arg database.GetUsersPa
return users, rowUsers[0].Count, nil
}
// TODO: Remove this and use a filter on GetUsers
func (q *querier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]database.User, error) {
return fetchWithPostFilter(q.auth, q.db.GetUsersByIDs)(ctx, ids)
}
func (q *querier) InsertUser(ctx context.Context, arg database.InsertUserParams) (database.User, error) {
// Always check if the assigned roles can actually be assigned by this actor.
impliedRoles := append([]string{rbac.RoleMember()}, arg.RBACRoles...)
@ -1222,37 +1182,6 @@ func (q *querier) GetWorkspaceAgentByInstanceID(ctx context.Context, authInstanc
return agent, nil
}
// GetWorkspaceAgentsByResourceIDs is an all or nothing call. If the user cannot read
// a single agent, the entire call will fail.
func (q *querier) GetWorkspaceAgentsByResourceIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceAgent, error) {
if _, ok := ActorFromContext(ctx); !ok {
return nil, NoActorError
}
// TODO: Make this more efficient. This is annoying because all these resources should be owned by the same workspace.
// So the authz check should just be 1 check, but we cannot do that easily here. We should see if all callers can
// instead do something like GetWorkspaceAgentsByWorkspaceID.
agents, err := q.db.GetWorkspaceAgentsByResourceIDs(ctx, ids)
if err != nil {
return nil, err
}
for _, a := range agents {
// Check if we can fetch the workspace by the agent ID.
_, err := q.GetWorkspaceByAgentID(ctx, a.ID)
if err == nil {
continue
}
if errors.Is(err, sql.ErrNoRows) && !errors.As(err, &NotAuthorizedError{}) {
// The agent is not tied to a workspace, likely from an orphaned template version.
// Just return it.
continue
}
// Otherwise, we cannot read the workspace, so we cannot read the agent.
return nil, err
}
return agents, nil
}
func (q *querier) UpdateWorkspaceAgentLifecycleStateByID(ctx context.Context, arg database.UpdateWorkspaceAgentLifecycleStateByIDParams) error {
agent, err := q.db.GetWorkspaceAgentByID(ctx, arg.ID)
if err != nil {
@ -1305,20 +1234,6 @@ func (q *querier) GetWorkspaceAppsByAgentID(ctx context.Context, agentID uuid.UU
return q.db.GetWorkspaceAppsByAgentID(ctx, agentID)
}
// GetWorkspaceAppsByAgentIDs is an all or nothing call. If the user cannot read a single app, the entire call will fail.
func (q *querier) GetWorkspaceAppsByAgentIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceApp, error) {
// TODO: This should be reworked. All these apps are likely owned by the same workspace, so we should be able to
// do 1 authz call. We should refactor this to be GetWorkspaceAppsByWorkspaceID.
for _, id := range ids {
_, err := q.GetWorkspaceAgentByID(ctx, id)
if err != nil {
return nil, err
}
}
return q.db.GetWorkspaceAppsByAgentIDs(ctx, ids)
}
func (q *querier) GetWorkspaceBuildByID(ctx context.Context, buildID uuid.UUID) (database.WorkspaceBuild, error) {
build, err := q.db.GetWorkspaceBuildByID(ctx, buildID)
if err != nil {
@ -1395,21 +1310,6 @@ func (q *querier) GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID) (d
return resource, nil
}
// GetWorkspaceResourceMetadataByResourceIDs is an all or nothing call. If a single resource is not authorized, then
// an error is returned.
func (q *querier) GetWorkspaceResourceMetadataByResourceIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceResourceMetadatum, error) {
// TODO: This is very inefficient. Since all these resources are likely asscoiated with the same workspace.
for _, id := range ids {
// If we can read the resource, we can read the metadata.
_, err := q.GetWorkspaceResourceByID(ctx, id)
if err != nil {
return nil, err
}
}
return q.db.GetWorkspaceResourceMetadataByResourceIDs(ctx, ids)
}
func (q *querier) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]database.WorkspaceResource, error) {
job, err := q.db.GetProvisionerJobByID(ctx, jobID)
if err != nil {
@ -1455,21 +1355,6 @@ func (q *querier) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.U
return q.db.GetWorkspaceResourcesByJobID(ctx, jobID)
}
// GetWorkspaceResourcesByJobIDs is an all or nothing call. If a single resource is not authorized, then
// an error is returned.
func (q *querier) GetWorkspaceResourcesByJobIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceResource, error) {
// TODO: This is very inefficient. Since all these resources are likely asscoiated with the same workspace.
for _, id := range ids {
// If we can read the resource, we can read the metadata.
_, err := q.GetProvisionerJobByID(ctx, id)
if err != nil {
return nil, err
}
}
return q.db.GetWorkspaceResourcesByJobIDs(ctx, ids)
}
func (q *querier) InsertWorkspace(ctx context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) {
obj := rbac.ResourceWorkspace.WithOwner(arg.OwnerID.String()).InOrg(arg.OrganizationID)
return insert(q.log, q.auth, obj, q.db.InsertWorkspace)(ctx, arg)

14
coderd/database/dbauthz/querier_test.go
View File

@ -622,7 +622,7 @@ func (s *MethodTestSuite) TestTemplate() {
TemplateID: uuid.NullUUID{UUID: t2.ID, Valid: true},
})
check.Args([]uuid.UUID{tv1.ID, tv2.ID, tv3.ID}).
Asserts(t1, rbac.ActionRead, t2, rbac.ActionRead).
Asserts( /*t1, rbac.ActionRead, t2, rbac.ActionRead*/ ).
Returns(slice.New(tv1, tv2, tv3))
}))
s.Run("GetTemplateVersionsByTemplateID", s.Subtest(func(db database.Store, check *expects) {
@ -797,7 +797,7 @@ func (s *MethodTestSuite) TestUser() {
a := dbgen.User(s.T(), db, database.User{CreatedAt: database.Now().Add(-time.Hour)})
b := dbgen.User(s.T(), db, database.User{CreatedAt: database.Now()})
check.Args([]uuid.UUID{a.ID, b.ID}).
Asserts(a, rbac.ActionRead, b, rbac.ActionRead).
Asserts( /*a, rbac.ActionRead, b, rbac.ActionRead*/ ).
Returns(slice.New(a, b))
}))
s.Run("InsertUser", s.Subtest(func(db database.Store, check *expects) {
@ -972,7 +972,7 @@ func (s *MethodTestSuite) TestWorkspace() {
build := dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: uuid.New()})
res := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: build.JobID})
agt := dbgen.WorkspaceAgent(s.T(), db, database.WorkspaceAgent{ResourceID: res.ID})
check.Args([]uuid.UUID{res.ID}).Asserts(ws, rbac.ActionRead).
check.Args([]uuid.UUID{res.ID}).Asserts( /*ws, rbac.ActionRead*/ ).
Returns([]database.WorkspaceAgent{agt})
}))
s.Run("UpdateWorkspaceAgentLifecycleStateByID", s.Subtest(func(db database.Store, check *expects) {
@ -1030,7 +1030,7 @@ func (s *MethodTestSuite) TestWorkspace() {
b := dbgen.WorkspaceApp(s.T(), db, database.WorkspaceApp{AgentID: bAgt.ID})
check.Args([]uuid.UUID{a.AgentID, b.AgentID}).
Asserts(aWs, rbac.ActionRead, bWs, rbac.ActionRead).
Asserts( /*aWs, rbac.ActionRead, bWs, rbac.ActionRead*/ ).
Returns([]database.WorkspaceApp{a, b})
}))
s.Run("GetWorkspaceBuildByID", s.Subtest(func(db database.Store, check *expects) {
@ -1093,7 +1093,7 @@ func (s *MethodTestSuite) TestWorkspace() {
a := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: build.JobID})
b := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: build.JobID})
check.Args([]uuid.UUID{a.ID, b.ID}).
Asserts(ws, []rbac.Action{rbac.ActionRead, rbac.ActionRead})
Asserts( /*ws, []rbac.Action{rbac.ActionRead, rbac.ActionRead}*/ )
}))
s.Run("Build/GetWorkspaceResourcesByJobID", s.Subtest(func(db database.Store, check *expects) {
ws := dbgen.Workspace(s.T(), db, database.Workspace{})
@ -1115,7 +1115,9 @@ func (s *MethodTestSuite) TestWorkspace() {
ws := dbgen.Workspace(s.T(), db, database.Workspace{})
build := dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: uuid.New()})
wJob := dbgen.ProvisionerJob(s.T(), db, database.ProvisionerJob{ID: build.JobID, Type: database.ProvisionerJobTypeWorkspaceBuild})
check.Args([]uuid.UUID{tJob.ID, wJob.ID}).Asserts(v.RBACObject(tpl), rbac.ActionRead, ws, rbac.ActionRead).Returns([]database.WorkspaceResource{})
check.Args([]uuid.UUID{tJob.ID, wJob.ID}).
Asserts( /*v.RBACObject(tpl), rbac.ActionRead, ws, rbac.ActionRead*/ ).
Returns([]database.WorkspaceResource{})
}))
s.Run("InsertWorkspace", s.Subtest(func(db database.Store, check *expects) {
u := dbgen.User(s.T(), db, database.User{})

50
coderd/database/dbauthz/system.go
View File

@ -14,6 +14,56 @@ import (
// to these objects. Might need a negative permission on the `Owner` role to
// prevent owners.
// GetWorkspaceAppsByAgentIDs
// The workspace/job is already fetched.
// TODO: This function should be removed/replaced with something with proper auth.
func (q *querier) GetWorkspaceAppsByAgentIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceApp, error) {
return q.db.GetWorkspaceAppsByAgentIDs(ctx, ids)
}
// GetWorkspaceAgentsByResourceIDs
// The workspace/job is already fetched.
// TODO: This function should be removed/replaced with something with proper auth.
func (q *querier) GetWorkspaceAgentsByResourceIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceAgent, error) {
return q.db.GetWorkspaceAgentsByResourceIDs(ctx, ids)
}
// GetWorkspaceResourceMetadataByResourceIDs is only used for build data.
// The workspace/job is already fetched.
// TODO: This function should be removed/replaced with something with proper auth.
func (q *querier) GetWorkspaceResourceMetadataByResourceIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceResourceMetadatum, error) {
return q.db.GetWorkspaceResourceMetadataByResourceIDs(ctx, ids)
}
// GetUsersByIDs is only used for usernames on workspace return data.
// This function should be replaced by joining this data to the workspace query
// itself.
// TODO: This function should be removed/replaced with something with proper auth.
// A SQL compiled filter is an option.
func (q *querier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]database.User, error) {
return q.db.GetUsersByIDs(ctx, ids)
}
func (q *querier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.ProvisionerJob, error) {
// TODO: This is missing authorization and is incorrect. This call is used by telemetry, and by 1 http route.
// That http handler should find a better way to fetch these jobs with easier rbac authz.
return q.db.GetProvisionerJobsByIDs(ctx, ids)
}
// GetTemplateVersionsByIDs is only used for workspace build data.
// The workspace is already fetched.
// TODO: Find a way to replace this with proper authz.
func (q *querier) GetTemplateVersionsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.TemplateVersion, error) {
return q.db.GetTemplateVersionsByIDs(ctx, ids)
}
// GetWorkspaceResourcesByJobIDs is only used for workspace build data.
// The workspace is already fetched.
// TODO: Find a way to replace this with proper authz.
func (q *querier) GetWorkspaceResourcesByJobIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceResource, error) {
return q.db.GetWorkspaceResourcesByJobIDs(ctx, ids)
}
func (q *querier) UpdateUserLinkedID(ctx context.Context, arg database.UpdateUserLinkedIDParams) (database.UserLink, error) {
return q.db.UpdateUserLinkedID(ctx, arg)
}

10
coderd/templates.go
View File

@ -15,6 +15,7 @@ import (
"github.com/coder/coder/coderd/audit"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/dbauthz"
"github.com/coder/coder/coderd/httpapi"
"github.com/coder/coder/coderd/httpmw"
"github.com/coder/coder/coderd/rbac"
@ -82,11 +83,10 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) {
return
}
// TODO: This just returns the workspaces a user can view. We should use
// a system function to get all workspaces that use this template.
// This data should never be exposed to the user aside from a non-zero count.
// Or we move this into a postgres constraint.
workspaces, err := api.Database.GetWorkspaces(ctx, database.GetWorkspacesParams{
// This is just to get the workspace count, so we use a system context to
// return ALL workspaces. Not just workspaces the user can view.
// nolint:gocritic
workspaces, err := api.Database.GetWorkspaces(dbauthz.AsSystemRestricted(ctx), database.GetWorkspacesParams{
TemplateIds: []uuid.UUID{template.ID},
})
if err != nil && !errors.Is(err, sql.ErrNoRows) {

3
coderd/workspaces.go
View File

@ -570,7 +570,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
}
aReq.New = workspace
users, err := api.Database.GetUsersByIDs(ctx, []uuid.UUID{user.ID, workspaceBuild.InitiatorID})
initiator, err := api.Database.GetUserByID(ctx, workspaceBuild.InitiatorID)
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error fetching user.",
@ -584,6 +584,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
WorkspaceBuilds: []telemetry.WorkspaceBuild{telemetry.ConvertWorkspaceBuild(workspaceBuild)},
})
users := []database.User{user, initiator}
apiBuild, err := api.convertWorkspaceBuild(
workspaceBuild,
workspace,