From 5ba7ba6bfc55112574c7b10f6acb542ea5926dce Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 17 Feb 2025 14:16:45 +0200 Subject: [PATCH] fix(coderd): add strict org ID joins for provisioner job metadata (#16588) References #16558 --- coderd/database/dbauthz/dbauthz_test.go | 4 +- coderd/database/dbmem/dbmem.go | 2 +- coderd/database/queries.sql.go | 42 +++++++++++++++---- .../database/queries/provisionerdaemons.sql | 23 ++++++++-- coderd/database/queries/provisionerjobs.sql | 17 ++++++-- coderd/provisionerjobs.go | 2 +- 6 files changed, 69 insertions(+), 21 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 1291c27236..24ecf0b8ec 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -3354,11 +3354,11 @@ func (s *MethodTestSuite) TestExtraMethods() { dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{ID: wbID, WorkspaceID: w.ID, TemplateVersionID: tv.ID, JobID: j2.ID}) ds, err := db.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner(context.Background(), database.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerParams{ - OrganizationID: uuid.NullUUID{Valid: true, UUID: org.ID}, + OrganizationID: org.ID, }) s.NoError(err, "get provisioner jobs by org") check.Args(database.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerParams{ - OrganizationID: uuid.NullUUID{Valid: true, UUID: org.ID}, + OrganizationID: org.ID, }).Asserts(j1, policy.ActionRead, j2, policy.ActionRead).Returns(ds) })) } diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 6bace66f53..fb997e64a9 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -4221,7 +4221,7 @@ func (q *FakeQuerier) GetProvisionerJobsByOrganizationAndStatusWithQueuePosition for _, rowQP := range rowsWithQueuePosition { job := rowQP.ProvisionerJob - if arg.OrganizationID.Valid && job.OrganizationID != arg.OrganizationID.UUID { + if job.OrganizationID != arg.OrganizationID { continue } if len(arg.Status) > 0 && !slices.Contains(arg.Status, job.JobStatus) { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c19f6922e5..576d516c48 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5756,6 +5756,7 @@ JOIN LEFT JOIN provisioner_jobs current_job ON ( current_job.worker_id = pd.id + AND current_job.organization_id = pd.organization_id AND current_job.completed_at IS NULL ) LEFT JOIN @@ -5767,26 +5768,40 @@ LEFT JOIN provisioner_jobs WHERE worker_id = pd.id + AND organization_id = pd.organization_id AND completed_at IS NOT NULL ORDER BY completed_at DESC LIMIT 1 ) + AND previous_job.organization_id = pd.organization_id ) LEFT JOIN workspace_builds current_build ON current_build.id = CASE WHEN current_job.input ? 'workspace_build_id' THEN (current_job.input->>'workspace_build_id')::uuid END LEFT JOIN -- We should always have a template version, either explicitly or implicitly via workspace build. - template_versions current_version ON current_version.id = CASE WHEN current_job.input ? 'template_version_id' THEN (current_job.input->>'template_version_id')::uuid ELSE current_build.template_version_id END + template_versions current_version ON ( + current_version.id = CASE WHEN current_job.input ? 'template_version_id' THEN (current_job.input->>'template_version_id')::uuid ELSE current_build.template_version_id END + AND current_version.organization_id = pd.organization_id + ) LEFT JOIN - templates current_template ON current_template.id = current_version.template_id + templates current_template ON ( + current_template.id = current_version.template_id + AND current_template.organization_id = pd.organization_id + ) LEFT JOIN workspace_builds previous_build ON previous_build.id = CASE WHEN previous_job.input ? 'workspace_build_id' THEN (previous_job.input->>'workspace_build_id')::uuid END LEFT JOIN -- We should always have a template version, either explicitly or implicitly via workspace build. - template_versions previous_version ON previous_version.id = CASE WHEN previous_job.input ? 'template_version_id' THEN (previous_job.input->>'template_version_id')::uuid ELSE previous_build.template_version_id END + template_versions previous_version ON ( + previous_version.id = CASE WHEN previous_job.input ? 'template_version_id' THEN (previous_job.input->>'template_version_id')::uuid ELSE previous_build.template_version_id END + AND previous_version.organization_id = pd.organization_id + ) LEFT JOIN - templates previous_template ON previous_template.id = previous_version.template_id + templates previous_template ON ( + previous_template.id = previous_version.template_id + AND previous_template.organization_id = pd.organization_id + ) WHERE pd.organization_id = $2::uuid AND (COALESCE(array_length($3::uuid[], 1), 0) = 0 OR pd.id = ANY($3::uuid[])) @@ -6487,14 +6502,23 @@ LEFT JOIN LEFT JOIN workspace_builds wb ON wb.id = CASE WHEN pj.input ? 'workspace_build_id' THEN (pj.input->>'workspace_build_id')::uuid END LEFT JOIN - workspaces w ON wb.workspace_id = w.id + workspaces w ON ( + w.id = wb.workspace_id + AND w.organization_id = pj.organization_id + ) LEFT JOIN -- We should always have a template version, either explicitly or implicitly via workspace build. - template_versions tv ON tv.id = CASE WHEN pj.input ? 'template_version_id' THEN (pj.input->>'template_version_id')::uuid ELSE wb.template_version_id END + template_versions tv ON ( + tv.id = CASE WHEN pj.input ? 'template_version_id' THEN (pj.input->>'template_version_id')::uuid ELSE wb.template_version_id END + AND tv.organization_id = pj.organization_id + ) LEFT JOIN - templates t ON tv.template_id = t.id + templates t ON ( + t.id = tv.template_id + AND t.organization_id = pj.organization_id + ) WHERE - ($1::uuid IS NULL OR pj.organization_id = $1) + pj.organization_id = $1::uuid AND (COALESCE(array_length($2::uuid[], 1), 0) = 0 OR pj.id = ANY($2::uuid[])) AND (COALESCE(array_length($3::provisioner_job_status[], 1), 0) = 0 OR pj.job_status = ANY($3::provisioner_job_status[])) AND ($4::tagset = 'null'::tagset OR provisioner_tagset_contains(pj.tags::tagset, $4::tagset)) @@ -6516,7 +6540,7 @@ LIMIT ` type GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerParams struct { - OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` IDs []uuid.UUID `db:"ids" json:"ids"` Status []ProvisionerJobStatus `db:"status" json:"status"` Tags StringMap `db:"tags" json:"tags"` diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 2aaf23ec0d..ab1668e537 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -58,6 +58,7 @@ JOIN LEFT JOIN provisioner_jobs current_job ON ( current_job.worker_id = pd.id + AND current_job.organization_id = pd.organization_id AND current_job.completed_at IS NULL ) LEFT JOIN @@ -69,28 +70,42 @@ LEFT JOIN provisioner_jobs WHERE worker_id = pd.id + AND organization_id = pd.organization_id AND completed_at IS NOT NULL ORDER BY completed_at DESC LIMIT 1 ) + AND previous_job.organization_id = pd.organization_id ) -- Current job information. LEFT JOIN workspace_builds current_build ON current_build.id = CASE WHEN current_job.input ? 'workspace_build_id' THEN (current_job.input->>'workspace_build_id')::uuid END LEFT JOIN -- We should always have a template version, either explicitly or implicitly via workspace build. - template_versions current_version ON current_version.id = CASE WHEN current_job.input ? 'template_version_id' THEN (current_job.input->>'template_version_id')::uuid ELSE current_build.template_version_id END + template_versions current_version ON ( + current_version.id = CASE WHEN current_job.input ? 'template_version_id' THEN (current_job.input->>'template_version_id')::uuid ELSE current_build.template_version_id END + AND current_version.organization_id = pd.organization_id + ) LEFT JOIN - templates current_template ON current_template.id = current_version.template_id + templates current_template ON ( + current_template.id = current_version.template_id + AND current_template.organization_id = pd.organization_id + ) -- Previous job information. LEFT JOIN workspace_builds previous_build ON previous_build.id = CASE WHEN previous_job.input ? 'workspace_build_id' THEN (previous_job.input->>'workspace_build_id')::uuid END LEFT JOIN -- We should always have a template version, either explicitly or implicitly via workspace build. - template_versions previous_version ON previous_version.id = CASE WHEN previous_job.input ? 'template_version_id' THEN (previous_job.input->>'template_version_id')::uuid ELSE previous_build.template_version_id END + template_versions previous_version ON ( + previous_version.id = CASE WHEN previous_job.input ? 'template_version_id' THEN (previous_job.input->>'template_version_id')::uuid ELSE previous_build.template_version_id END + AND previous_version.organization_id = pd.organization_id + ) LEFT JOIN - templates previous_template ON previous_template.id = previous_version.template_id + templates previous_template ON ( + previous_template.id = previous_version.template_id + AND previous_template.organization_id = pd.organization_id + ) WHERE pd.organization_id = @organization_id::uuid AND (COALESCE(array_length(@ids::uuid[], 1), 0) = 0 OR pd.id = ANY(@ids::uuid[])) diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 9888fb11df..592b228af2 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -148,14 +148,23 @@ LEFT JOIN LEFT JOIN workspace_builds wb ON wb.id = CASE WHEN pj.input ? 'workspace_build_id' THEN (pj.input->>'workspace_build_id')::uuid END LEFT JOIN - workspaces w ON wb.workspace_id = w.id + workspaces w ON ( + w.id = wb.workspace_id + AND w.organization_id = pj.organization_id + ) LEFT JOIN -- We should always have a template version, either explicitly or implicitly via workspace build. - template_versions tv ON tv.id = CASE WHEN pj.input ? 'template_version_id' THEN (pj.input->>'template_version_id')::uuid ELSE wb.template_version_id END + template_versions tv ON ( + tv.id = CASE WHEN pj.input ? 'template_version_id' THEN (pj.input->>'template_version_id')::uuid ELSE wb.template_version_id END + AND tv.organization_id = pj.organization_id + ) LEFT JOIN - templates t ON tv.template_id = t.id + templates t ON ( + t.id = tv.template_id + AND t.organization_id = pj.organization_id + ) WHERE - (sqlc.narg('organization_id')::uuid IS NULL OR pj.organization_id = @organization_id) + pj.organization_id = @organization_id::uuid AND (COALESCE(array_length(@ids::uuid[], 1), 0) = 0 OR pj.id = ANY(@ids::uuid[])) AND (COALESCE(array_length(@status::provisioner_job_status[], 1), 0) = 0 OR pj.job_status = ANY(@status::provisioner_job_status[])) AND (@tags::tagset = 'null'::tagset OR provisioner_tagset_contains(pj.tags::tagset, @tags::tagset)) diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index b12187e682..b51c38021c 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -130,7 +130,7 @@ func (api *API) handleAuthAndFetchProvisionerJobs(rw http.ResponseWriter, r *htt } jobs, err := api.Database.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner(ctx, database.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerParams{ - OrganizationID: uuid.NullUUID{UUID: org.ID, Valid: true}, + OrganizationID: org.ID, Status: slice.StringEnums[database.ProvisionerJobStatus](status), Limit: sql.NullInt32{Int32: limit, Valid: limit > 0}, IDs: ids,