From 4e57b9fbdc463460b8c3d954ab19418422a31b5e Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 13 Oct 2022 18:02:52 -0500 Subject: [PATCH] fix: allow regular users to push files (#4500) - As part of merging support for Template RBAC and user groups a permission check on reading files was relaxed. With the addition of admin roles on individual templates, regular users are now able to push template versions if they have inherited the 'admin' role for a template. In order to do so they need to be able to create and read their own files. Since collisions on hash in the past were ignored, this means that a regular user who pushes a template version with a file hash that collides with an existing hash will not be able to read the file (since it belongs to another user). This commit fixes the underlying problem which was that the files table had a primary key on the 'hash' column. This was not a problem at the time because only template admins and other users with similar elevated roles were able to read all files regardless of ownership. To fix this a new column and primary key 'id' has been introduced to the files table. The unique constraint has been updated to be hash+created_by. Tables (provisioner_jobs) that referenced files.hash have been updated to reference files.id. Relevant API endpoints have also been updated. --- cli/templatecreate.go | 7 +- cli/templatepull.go | 2 +- cli/templatepush.go | 2 +- .../autobuild/executor/lifecycle_executor.go | 2 +- coderd/coderd.go | 2 +- coderd/coderdtest/authorize.go | 4 +- coderd/coderdtest/coderdtest.go | 4 +- coderd/database/databasefake/databasefake.go | 19 ++++- coderd/database/dump.sql | 12 ++- .../migrations/000059_file_id.down.sql | 41 ++++++++++ .../database/migrations/000059_file_id.up.sql | 42 +++++++++++ coderd/database/models.go | 3 +- coderd/database/querier.go | 3 +- coderd/database/queries.sql.go | 74 ++++++++++++++----- coderd/database/queries/files.sql | 19 ++++- coderd/database/queries/provisionerjobs.sql | 2 +- coderd/database/unique_constraint.go | 1 + coderd/files.go | 32 ++++++-- coderd/files_test.go | 5 +- coderd/provisionerdaemons.go | 2 +- coderd/provisionerdaemons_test.go | 4 +- coderd/provisionerjobs.go | 8 +- coderd/templates.go | 3 +- coderd/templateversions.go | 16 ++-- coderd/templateversions_test.go | 8 +- coderd/workspacebuilds.go | 2 +- coderd/workspaces.go | 2 +- codersdk/files.go | 8 +- codersdk/organizations.go | 2 +- codersdk/provisionerdaemons.go | 18 ++--- enterprise/coderd/templates_test.go | 4 +- site/src/api/typesGenerated.ts | 4 +- site/src/testHelpers/entities.ts | 2 +- 33 files changed, 265 insertions(+), 94 deletions(-) create mode 100644 coderd/database/migrations/000059_file_id.down.sql create mode 100644 coderd/database/migrations/000059_file_id.up.sql diff --git a/cli/templatecreate.go b/cli/templatecreate.go index a0f4014f71..de0b8eab8f 100644 --- a/cli/templatecreate.go +++ b/cli/templatecreate.go @@ -10,6 +10,7 @@ import ( "unicode/utf8" "github.com/briandowns/spinner" + "github.com/google/uuid" "github.com/spf13/cobra" "golang.org/x/xerrors" @@ -91,7 +92,7 @@ func templateCreate() *cobra.Command { Client: client, Organization: organization, Provisioner: database.ProvisionerType(provisioner), - FileHash: resp.Hash, + FileID: resp.ID, ParameterFile: parameterFile, }) if err != nil { @@ -148,7 +149,7 @@ type createValidTemplateVersionArgs struct { Client *codersdk.Client Organization codersdk.Organization Provisioner database.ProvisionerType - FileHash string + FileID uuid.UUID ParameterFile string // Template is only required if updating a template's active version. Template *codersdk.Template @@ -165,7 +166,7 @@ func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVers req := codersdk.CreateTemplateVersionRequest{ Name: args.Name, StorageMethod: codersdk.ProvisionerStorageMethodFile, - StorageSource: args.FileHash, + FileID: args.FileID, Provisioner: codersdk.ProvisionerType(args.Provisioner), ParameterValues: parameters, } diff --git a/cli/templatepull.go b/cli/templatepull.go index 5660261c51..09f70c91b8 100644 --- a/cli/templatepull.go +++ b/cli/templatepull.go @@ -66,7 +66,7 @@ func templatePull() *cobra.Command { latest := versions[0] // Download the tar archive. - raw, ctype, err := client.Download(ctx, latest.Job.StorageSource) + raw, ctype, err := client.Download(ctx, latest.Job.FileID) if err != nil { return xerrors.Errorf("download template: %w", err) } diff --git a/cli/templatepush.go b/cli/templatepush.go index f858c6daec..40bafed0ef 100644 --- a/cli/templatepush.go +++ b/cli/templatepush.go @@ -80,7 +80,7 @@ func templatePush() *cobra.Command { Client: client, Organization: organization, Provisioner: database.ProvisionerType(provisioner), - FileHash: resp.Hash, + FileID: resp.ID, ParameterFile: parameterFile, Template: &template, ReuseParameters: !alwaysPrompt, diff --git a/coderd/autobuild/executor/lifecycle_executor.go b/coderd/autobuild/executor/lifecycle_executor.go index 22536583b6..f21fde9a8a 100644 --- a/coderd/autobuild/executor/lifecycle_executor.go +++ b/coderd/autobuild/executor/lifecycle_executor.go @@ -276,7 +276,7 @@ func build(ctx context.Context, store database.Store, workspace database.Workspa Provisioner: template.Provisioner, Type: database.ProvisionerJobTypeWorkspaceBuild, StorageMethod: priorJob.StorageMethod, - StorageSource: priorJob.StorageSource, + FileID: priorJob.FileID, Input: input, }) if err != nil { diff --git a/coderd/coderd.go b/coderd/coderd.go index 29439a2001..b067035b3a 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -280,7 +280,7 @@ func New(options *Options) *API { // file content is expensive so it should be small. httpmw.RateLimitPerMinute(12), ) - r.Get("/{hash}", api.fileByHash) + r.Get("/{fileID}", api.fileByID) r.Post("/", api.postFile) }) diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index fa77898864..a5183f2b6e 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -142,7 +142,7 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) { AssertObject: rbac.ResourceTemplate.InOrg(a.Template.OrganizationID), }, "POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile}, - "GET:/api/v2/files/{hash}": { + "GET:/api/v2/files/{fileID}": { AssertAction: rbac.ActionRead, AssertObject: rbac.ResourceFile.WithOwner(a.Admin.UserID.String()), }, @@ -369,7 +369,7 @@ func NewAuthTester(ctx context.Context, t *testing.T, client *codersdk.Client, a "{workspaceagent}": workspace.LatestBuild.Resources[0].Agents[0].ID.String(), "{buildnumber}": strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10), "{template}": template.ID.String(), - "{hash}": file.Hash, + "{fileID}": file.ID.String(), "{workspaceresource}": workspace.LatestBuild.Resources[0].ID.String(), "{workspaceapp}": workspace.LatestBuild.Resources[0].Agents[0].Apps[0].Name, "{templateversion}": version.ID.String(), diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index d7ac4eb14b..7ec28668ce 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -383,7 +383,7 @@ func CreateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID file, err := client.Upload(context.Background(), codersdk.ContentTypeTar, data) require.NoError(t, err) templateVersion, err := client.CreateTemplateVersion(context.Background(), organizationID, codersdk.CreateTemplateVersionRequest{ - StorageSource: file.Hash, + FileID: file.ID, StorageMethod: codersdk.ProvisionerStorageMethodFile, Provisioner: codersdk.ProvisionerTypeEcho, }) @@ -431,7 +431,7 @@ func UpdateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID require.NoError(t, err) templateVersion, err := client.CreateTemplateVersion(context.Background(), organizationID, codersdk.CreateTemplateVersionRequest{ TemplateID: templateID, - StorageSource: file.Hash, + FileID: file.ID, StorageMethod: codersdk.ProvisionerStorageMethodFile, Provisioner: codersdk.ProvisionerTypeEcho, }) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 36bf079231..2f5d57e5d6 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -316,12 +316,24 @@ func (q *fakeQuerier) DeleteAPIKeyByID(_ context.Context, id string) error { return sql.ErrNoRows } -func (q *fakeQuerier) GetFileByHash(_ context.Context, hash string) (database.File, error) { +func (q *fakeQuerier) GetFileByHashAndCreator(_ context.Context, arg database.GetFileByHashAndCreatorParams) (database.File, error) { q.mutex.RLock() defer q.mutex.RUnlock() for _, file := range q.files { - if file.Hash == hash { + if file.Hash == arg.Hash && file.CreatedBy == arg.CreatedBy { + return file, nil + } + } + return database.File{}, sql.ErrNoRows +} + +func (q *fakeQuerier) GetFileByID(_ context.Context, id uuid.UUID) (database.File, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + for _, file := range q.files { + if file.ID == id { return file, nil } } @@ -1901,6 +1913,7 @@ func (q *fakeQuerier) InsertFile(_ context.Context, arg database.InsertFileParam //nolint:gosimple file := database.File{ + ID: arg.ID, Hash: arg.Hash, CreatedAt: arg.CreatedAt, CreatedBy: arg.CreatedBy, @@ -2085,7 +2098,7 @@ func (q *fakeQuerier) InsertProvisionerJob(_ context.Context, arg database.Inser InitiatorID: arg.InitiatorID, Provisioner: arg.Provisioner, StorageMethod: arg.StorageMethod, - StorageSource: arg.StorageSource, + FileID: arg.FileID, Type: arg.Type, Input: arg.Input, } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index eb16074e90..9345907d04 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -151,7 +151,8 @@ CREATE TABLE files ( created_at timestamp with time zone NOT NULL, created_by uuid NOT NULL, mimetype character varying(64) NOT NULL, - data bytea NOT NULL + data bytea NOT NULL, + id uuid DEFAULT gen_random_uuid() NOT NULL ); CREATE TABLE gitsshkeys ( @@ -270,10 +271,10 @@ CREATE TABLE provisioner_jobs ( initiator_id uuid NOT NULL, provisioner provisioner_type NOT NULL, storage_method provisioner_storage_method NOT NULL, - storage_source text NOT NULL, type provisioner_job_type NOT NULL, input jsonb NOT NULL, - worker_id uuid + worker_id uuid, + file_id uuid NOT NULL ); CREATE TABLE site_configs ( @@ -432,7 +433,10 @@ ALTER TABLE ONLY audit_logs ADD CONSTRAINT audit_logs_pkey PRIMARY KEY (id); ALTER TABLE ONLY files - ADD CONSTRAINT files_pkey PRIMARY KEY (hash); + ADD CONSTRAINT files_hash_created_by_key UNIQUE (hash, created_by); + +ALTER TABLE ONLY files + ADD CONSTRAINT files_pkey PRIMARY KEY (id); ALTER TABLE ONLY gitsshkeys ADD CONSTRAINT gitsshkeys_pkey PRIMARY KEY (user_id); diff --git a/coderd/database/migrations/000059_file_id.down.sql b/coderd/database/migrations/000059_file_id.down.sql new file mode 100644 index 0000000000..56dbb13eeb --- /dev/null +++ b/coderd/database/migrations/000059_file_id.down.sql @@ -0,0 +1,41 @@ +BEGIN; + +-- Add back the storage_source column. This must be nullable temporarily. +ALTER TABLE provisioner_jobs ADD COLUMN storage_source text; + +-- Set the storage_source to the hash of the files.id reference. +UPDATE + provisioner_jobs +SET + storage_source=files.hash +FROM + files +WHERE + provisioner_jobs.file_id = files.id; + +-- Now that we've populated storage_source drop the file_id column. +ALTER TABLE provisioner_jobs DROP COLUMN file_id; +-- We can set the storage_source column as NOT NULL now. +ALTER TABLE provisioner_jobs ALTER COLUMN storage_source SET NOT NULL; + +-- Delete all the duplicate rows where hashes collide. +-- We filter on 'id' to ensure only 1 unique row. +DELETE FROM + files a +USING + files b +WHERE + a.created_by < b.created_by +AND + a.hash = b.hash; + +-- Drop the primary key on files.id. +ALTER TABLE files DROP CONSTRAINT files_pkey; +-- Drop the id column. +ALTER TABLE files DROP COLUMN id; +-- Drop the unique constraint on hash + owner. +ALTER TABLE files DROP CONSTRAINT files_hash_created_by_key; +-- Set the primary key back to hash. +ALTER TABLE files ADD PRIMARY KEY (hash); + +COMMIT; diff --git a/coderd/database/migrations/000059_file_id.up.sql b/coderd/database/migrations/000059_file_id.up.sql new file mode 100644 index 0000000000..7e6e919fa9 --- /dev/null +++ b/coderd/database/migrations/000059_file_id.up.sql @@ -0,0 +1,42 @@ +-- This migration updates the files table to move the unique +-- constraint to be hash + created_by. This is necessary to +-- allow regular users who have been granted admin to a specific +-- template to be able to push and read files used for template +-- versions they create. +-- Prior to this collisions on file.hash were not an issue +-- since users who could push files could also read all files. +-- +-- This migration also adds a 'files.id' column as the primary +-- key. As a side effect the provisioner_jobs must now reference +-- the files.id column since the 'hash' column is now ambiguous. +BEGIN; + +-- Drop the primary key on hash. +ALTER TABLE files DROP CONSTRAINT files_pkey; + +-- Add an 'id' column and designate it the primary key. +ALTER TABLE files ADD COLUMN + id uuid NOT NULL PRIMARY KEY DEFAULT gen_random_uuid (); + +-- Update the constraint to include the user who created it. +ALTER TABLE files ADD UNIQUE(hash, created_by); + +-- Update provisioner_jobs to include a file_id column. +-- This must be temporarily nullable. +ALTER TABLE provisioner_jobs ADD COLUMN file_id uuid; + +-- Update all the rows to point to key in the files table. +UPDATE provisioner_jobs +SET + file_id = files.id +FROM + files +WHERE + provisioner_jobs.storage_source = files.hash; + +-- Enforce NOT NULL on file_id now. +ALTER TABLE provisioner_jobs ALTER COLUMN file_id SET NOT NULL; +-- Drop storage_source since it is no longer useful for anything. +ALTER TABLE provisioner_jobs DROP COLUMN storage_source; + +COMMIT; diff --git a/coderd/database/models.go b/coderd/database/models.go index f669b5e618..2e50d2b647 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -404,6 +404,7 @@ type File struct { CreatedBy uuid.UUID `db:"created_by" json:"created_by"` Mimetype string `db:"mimetype" json:"mimetype"` Data []byte `db:"data" json:"data"` + ID uuid.UUID `db:"id" json:"id"` } type GitSSHKey struct { @@ -501,10 +502,10 @@ type ProvisionerJob struct { InitiatorID uuid.UUID `db:"initiator_id" json:"initiator_id"` Provisioner ProvisionerType `db:"provisioner" json:"provisioner"` StorageMethod ProvisionerStorageMethod `db:"storage_method" json:"storage_method"` - StorageSource string `db:"storage_source" json:"storage_source"` Type ProvisionerJobType `db:"type" json:"type"` Input json.RawMessage `db:"input" json:"input"` WorkerID uuid.NullUUID `db:"worker_id" json:"worker_id"` + FileID uuid.UUID `db:"file_id" json:"file_id"` } type ProvisionerJobLog struct { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index b58f6abbcc..e9c577bcb1 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -39,7 +39,8 @@ type sqlcQuerier interface { // are included. GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) GetDeploymentID(ctx context.Context) (string, error) - GetFileByHash(ctx context.Context, hash string) (File, error) + GetFileByHashAndCreator(ctx context.Context, arg GetFileByHashAndCreatorParams) (File, error) + GetFileByID(ctx context.Context, id uuid.UUID) (File, error) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetGroupByID(ctx context.Context, id uuid.UUID) (Group, error) GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2ff1805cd4..8673822ca4 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -647,19 +647,26 @@ func (q *sqlQuerier) InsertAuditLog(ctx context.Context, arg InsertAuditLogParam return i, err } -const getFileByHash = `-- name: GetFileByHash :one +const getFileByHashAndCreator = `-- name: GetFileByHashAndCreator :one SELECT - hash, created_at, created_by, mimetype, data + hash, created_at, created_by, mimetype, data, id FROM files WHERE hash = $1 +AND + created_by = $2 LIMIT 1 ` -func (q *sqlQuerier) GetFileByHash(ctx context.Context, hash string) (File, error) { - row := q.db.QueryRowContext(ctx, getFileByHash, hash) +type GetFileByHashAndCreatorParams struct { + Hash string `db:"hash" json:"hash"` + CreatedBy uuid.UUID `db:"created_by" json:"created_by"` +} + +func (q *sqlQuerier) GetFileByHashAndCreator(ctx context.Context, arg GetFileByHashAndCreatorParams) (File, error) { + row := q.db.QueryRowContext(ctx, getFileByHashAndCreator, arg.Hash, arg.CreatedBy) var i File err := row.Scan( &i.Hash, @@ -667,18 +674,45 @@ func (q *sqlQuerier) GetFileByHash(ctx context.Context, hash string) (File, erro &i.CreatedBy, &i.Mimetype, &i.Data, + &i.ID, + ) + return i, err +} + +const getFileByID = `-- name: GetFileByID :one +SELECT + hash, created_at, created_by, mimetype, data, id +FROM + files +WHERE + id = $1 +LIMIT + 1 +` + +func (q *sqlQuerier) GetFileByID(ctx context.Context, id uuid.UUID) (File, error) { + row := q.db.QueryRowContext(ctx, getFileByID, id) + var i File + err := row.Scan( + &i.Hash, + &i.CreatedAt, + &i.CreatedBy, + &i.Mimetype, + &i.Data, + &i.ID, ) return i, err } const insertFile = `-- name: InsertFile :one INSERT INTO - files (hash, created_at, created_by, mimetype, "data") + files (id, hash, created_at, created_by, mimetype, "data") VALUES - ($1, $2, $3, $4, $5) RETURNING hash, created_at, created_by, mimetype, data + ($1, $2, $3, $4, $5, $6) RETURNING hash, created_at, created_by, mimetype, data, id ` type InsertFileParams struct { + ID uuid.UUID `db:"id" json:"id"` Hash string `db:"hash" json:"hash"` CreatedAt time.Time `db:"created_at" json:"created_at"` CreatedBy uuid.UUID `db:"created_by" json:"created_by"` @@ -688,6 +722,7 @@ type InsertFileParams struct { func (q *sqlQuerier) InsertFile(ctx context.Context, arg InsertFileParams) (File, error) { row := q.db.QueryRowContext(ctx, insertFile, + arg.ID, arg.Hash, arg.CreatedAt, arg.CreatedBy, @@ -701,6 +736,7 @@ func (q *sqlQuerier) InsertFile(ctx context.Context, arg InsertFileParams) (File &i.CreatedBy, &i.Mimetype, &i.Data, + &i.ID, ) return i, err } @@ -2237,7 +2273,7 @@ WHERE SKIP LOCKED LIMIT 1 - ) RETURNING id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, storage_source, type, input, worker_id + ) RETURNING id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, type, input, worker_id, file_id ` type AcquireProvisionerJobParams struct { @@ -2267,17 +2303,17 @@ func (q *sqlQuerier) AcquireProvisionerJob(ctx context.Context, arg AcquireProvi &i.InitiatorID, &i.Provisioner, &i.StorageMethod, - &i.StorageSource, &i.Type, &i.Input, &i.WorkerID, + &i.FileID, ) return i, err } const getProvisionerJobByID = `-- name: GetProvisionerJobByID :one SELECT - id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, storage_source, type, input, worker_id + id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, type, input, worker_id, file_id FROM provisioner_jobs WHERE @@ -2299,17 +2335,17 @@ func (q *sqlQuerier) GetProvisionerJobByID(ctx context.Context, id uuid.UUID) (P &i.InitiatorID, &i.Provisioner, &i.StorageMethod, - &i.StorageSource, &i.Type, &i.Input, &i.WorkerID, + &i.FileID, ) return i, err } const getProvisionerJobsByIDs = `-- name: GetProvisionerJobsByIDs :many SELECT - id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, storage_source, type, input, worker_id + id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, type, input, worker_id, file_id FROM provisioner_jobs WHERE @@ -2337,10 +2373,10 @@ func (q *sqlQuerier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUI &i.InitiatorID, &i.Provisioner, &i.StorageMethod, - &i.StorageSource, &i.Type, &i.Input, &i.WorkerID, + &i.FileID, ); err != nil { return nil, err } @@ -2356,7 +2392,7 @@ func (q *sqlQuerier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUI } const getProvisionerJobsCreatedAfter = `-- name: GetProvisionerJobsCreatedAfter :many -SELECT id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, storage_source, type, input, worker_id FROM provisioner_jobs WHERE created_at > $1 +SELECT id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, type, input, worker_id, file_id FROM provisioner_jobs WHERE created_at > $1 ` func (q *sqlQuerier) GetProvisionerJobsCreatedAfter(ctx context.Context, createdAt time.Time) ([]ProvisionerJob, error) { @@ -2380,10 +2416,10 @@ func (q *sqlQuerier) GetProvisionerJobsCreatedAfter(ctx context.Context, created &i.InitiatorID, &i.Provisioner, &i.StorageMethod, - &i.StorageSource, &i.Type, &i.Input, &i.WorkerID, + &i.FileID, ); err != nil { return nil, err } @@ -2408,12 +2444,12 @@ INSERT INTO initiator_id, provisioner, storage_method, - storage_source, + file_id, "type", "input" ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) RETURNING id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, storage_source, type, input, worker_id + ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) RETURNING id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, type, input, worker_id, file_id ` type InsertProvisionerJobParams struct { @@ -2424,7 +2460,7 @@ type InsertProvisionerJobParams struct { InitiatorID uuid.UUID `db:"initiator_id" json:"initiator_id"` Provisioner ProvisionerType `db:"provisioner" json:"provisioner"` StorageMethod ProvisionerStorageMethod `db:"storage_method" json:"storage_method"` - StorageSource string `db:"storage_source" json:"storage_source"` + FileID uuid.UUID `db:"file_id" json:"file_id"` Type ProvisionerJobType `db:"type" json:"type"` Input json.RawMessage `db:"input" json:"input"` } @@ -2438,7 +2474,7 @@ func (q *sqlQuerier) InsertProvisionerJob(ctx context.Context, arg InsertProvisi arg.InitiatorID, arg.Provisioner, arg.StorageMethod, - arg.StorageSource, + arg.FileID, arg.Type, arg.Input, ) @@ -2455,10 +2491,10 @@ func (q *sqlQuerier) InsertProvisionerJob(ctx context.Context, arg InsertProvisi &i.InitiatorID, &i.Provisioner, &i.StorageMethod, - &i.StorageSource, &i.Type, &i.Input, &i.WorkerID, + &i.FileID, ) return i, err } diff --git a/coderd/database/queries/files.sql b/coderd/database/queries/files.sql index a91513d31a..1f54386bb3 100644 --- a/coderd/database/queries/files.sql +++ b/coderd/database/queries/files.sql @@ -1,15 +1,28 @@ --- name: GetFileByHash :one +-- name: GetFileByID :one +SELECT + * +FROM + files +WHERE + id = $1 +LIMIT + 1; + +-- name: GetFileByHashAndCreator :one SELECT * FROM files WHERE hash = $1 +AND + created_by = $2 LIMIT 1; + -- name: InsertFile :one INSERT INTO - files (hash, created_at, created_by, mimetype, "data") + files (id, hash, created_at, created_by, mimetype, "data") VALUES - ($1, $2, $3, $4, $5) RETURNING *; + ($1, $2, $3, $4, $5, $6) RETURNING *; diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 4775d574e2..027bd25bc9 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -59,7 +59,7 @@ INSERT INTO initiator_id, provisioner, storage_method, - storage_source, + file_id, "type", "input" ) diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index 44cfd89d43..b4263c09b4 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -6,6 +6,7 @@ type UniqueConstraint string // UniqueConstraint enums. const ( + UniqueFilesHashCreatedByKey UniqueConstraint = "files_hash_created_by_key" // ALTER TABLE ONLY files ADD CONSTRAINT files_hash_created_by_key UNIQUE (hash, created_by); UniqueGroupMembersUserIDGroupIDKey UniqueConstraint = "group_members_user_id_group_id_key" // ALTER TABLE ONLY group_members ADD CONSTRAINT group_members_user_id_group_id_key UNIQUE (user_id, group_id); UniqueGroupsNameOrganizationIDKey UniqueConstraint = "groups_name_organization_id_key" // ALTER TABLE ONLY groups ADD CONSTRAINT groups_name_organization_id_key UNIQUE (name, organization_id); UniqueLicensesJWTKey UniqueConstraint = "licenses_jwt_key" // ALTER TABLE ONLY licenses ADD CONSTRAINT licenses_jwt_key UNIQUE (jwt); diff --git a/coderd/files.go b/coderd/files.go index a0b7be2c8b..2c304921f8 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -10,6 +10,7 @@ import ( "net/http" "github.com/go-chi/chi/v5" + "github.com/google/uuid" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" @@ -50,15 +51,20 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) { } hashBytes := sha256.Sum256(data) hash := hex.EncodeToString(hashBytes[:]) - file, err := api.Database.GetFileByHash(ctx, hash) + file, err := api.Database.GetFileByHashAndCreator(ctx, database.GetFileByHashAndCreatorParams{ + Hash: hash, + CreatedBy: apiKey.UserID, + }) if err == nil { // The file already exists! httpapi.Write(ctx, rw, http.StatusOK, codersdk.UploadResponse{ - Hash: file.Hash, + ID: file.ID, }) return } + id := uuid.New() file, err = api.Database.InsertFile(ctx, database.InsertFileParams{ + ID: id, Hash: hash, CreatedBy: apiKey.UserID, CreatedAt: database.Now(), @@ -74,20 +80,30 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) { } httpapi.Write(ctx, rw, http.StatusCreated, codersdk.UploadResponse{ - Hash: file.Hash, + ID: file.ID, }) } -func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { +func (api *API) fileByID(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - hash := chi.URLParam(r, "hash") - if hash == "" { + + fileID := chi.URLParam(r, "fileID") + if fileID == "" { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "File hash must be provided in url.", + Message: "File id must be provided in url.", }) return } - file, err := api.Database.GetFileByHash(ctx, hash) + + id, err := uuid.Parse(fileID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "File id must be a valid UUID.", + }) + return + } + + file, err := api.Database.GetFileByID(ctx, id) if errors.Is(err, sql.ErrNoRows) { httpapi.ResourceNotFound(rw) return diff --git a/coderd/files_test.go b/coderd/files_test.go index 28d4a33104..b3a3953a43 100644 --- a/coderd/files_test.go +++ b/coderd/files_test.go @@ -5,6 +5,7 @@ import ( "net/http" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" @@ -64,7 +65,7 @@ func TestDownload(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, _, err := client.Download(ctx, "something") + _, _, err := client.Download(ctx, uuid.New()) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) @@ -80,7 +81,7 @@ func TestDownload(t *testing.T) { resp, err := client.Upload(ctx, codersdk.ContentTypeTar, make([]byte, 1024)) require.NoError(t, err) - data, contentType, err := client.Download(ctx, resp.Hash) + data, contentType, err := client.Download(ctx, resp.ID) require.NoError(t, err) require.Len(t, data, 1024) require.Equal(t, codersdk.ContentTypeTar, contentType) diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index bef1110ec3..1863de54c6 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -315,7 +315,7 @@ func (server *provisionerdServer) AcquireJob(ctx context.Context, _ *proto.Empty } switch job.StorageMethod { case database.ProvisionerStorageMethodFile: - file, err := server.Database.GetFileByHash(ctx, job.StorageSource) + file, err := server.Database.GetFileByID(ctx, job.FileID) if err != nil { return nil, failJob(fmt.Sprintf("get file by hash: %s", err)) } diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index 8fe4831480..d3b0be35cd 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -33,11 +33,11 @@ func TestProvisionerDaemons(t *testing.T) { resp, err := client.Upload(ctx, codersdk.ContentTypeTar, data) require.NoError(t, err) - t.Log(resp.Hash) + t.Log(resp.ID) version, err := client.CreateTemplateVersion(ctx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ StorageMethod: codersdk.ProvisionerStorageMethodFile, - StorageSource: resp.Hash, + FileID: resp.ID, Provisioner: codersdk.ProvisionerTypeEcho, }) require.NoError(t, err) diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index e278411224..294b013e00 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -316,10 +316,10 @@ func convertProvisionerJobLog(provisionerJobLog database.ProvisionerJobLog) code func convertProvisionerJob(provisionerJob database.ProvisionerJob) codersdk.ProvisionerJob { job := codersdk.ProvisionerJob{ - ID: provisionerJob.ID, - CreatedAt: provisionerJob.CreatedAt, - Error: provisionerJob.Error.String, - StorageSource: provisionerJob.StorageSource, + ID: provisionerJob.ID, + CreatedAt: provisionerJob.CreatedAt, + Error: provisionerJob.Error.String, + FileID: provisionerJob.FileID, } // Applying values optional to the struct. if provisionerJob.StartedAt.Valid { diff --git a/coderd/templates.go b/coderd/templates.go index 3366e02b14..fbc25a7b55 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -600,6 +600,7 @@ func (api *API) autoImportTemplate(ctx context.Context, opts autoImportTemplateO now = database.Now() ) file, err := tx.InsertFile(ctx, database.InsertFileParams{ + ID: uuid.New(), Hash: hex.EncodeToString(hash[:]), CreatedAt: now, CreatedBy: opts.userID, @@ -639,7 +640,7 @@ func (api *API) autoImportTemplate(ctx context.Context, opts autoImportTemplateO InitiatorID: opts.userID, Provisioner: database.ProvisionerTypeTerraform, StorageMethod: database.ProvisionerStorageMethodFile, - StorageSource: file.Hash, + FileID: file.ID, Type: database.ProvisionerJobTypeTemplateVersionImport, Input: []byte{'{', '}'}, }) diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 4d5b54b993..ccc1cd9761 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -285,7 +285,7 @@ func (api *API) postTemplateVersionDryRun(rw http.ResponseWriter, r *http.Reques InitiatorID: apiKey.UserID, Provisioner: job.Provisioner, StorageMethod: job.StorageMethod, - StorageSource: job.StorageSource, + FileID: job.FileID, Type: database.ProvisionerJobTypeTemplateVersionDryRun, Input: input, }) @@ -717,7 +717,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht return } - file, err := api.Database.GetFileByHash(ctx, req.StorageSource) + file, err := api.Database.GetFileByID(ctx, req.FileID) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "File not found.", @@ -732,12 +732,10 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht return } - // TODO(JonA): Readd this check once we update the unique constraint - // on files to be owner + hash. - // if !api.Authorize(r, rbac.ActionRead, file) { - // httpapi.ResourceNotFound(rw) - // return - // } + if !api.Authorize(r, rbac.ActionRead, file) { + httpapi.ResourceNotFound(rw) + return + } var templateVersion database.TemplateVersion var provisionerJob database.ProvisionerJob @@ -814,7 +812,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht InitiatorID: apiKey.UserID, Provisioner: database.ProvisionerType(req.Provisioner), StorageMethod: database.ProvisionerStorageMethodFile, - StorageSource: file.Hash, + FileID: file.ID, Type: database.ProvisionerJobTypeTemplateVersionImport, Input: []byte{'{', '}'}, }) diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 5521be8ec3..1bcb5ba8a0 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -66,7 +66,7 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { _, err := client.CreateTemplateVersion(ctx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ TemplateID: templateID, StorageMethod: codersdk.ProvisionerStorageMethodFile, - StorageSource: "hash", + FileID: uuid.New(), Provisioner: codersdk.ProvisionerTypeEcho, }) var apiErr *codersdk.Error @@ -84,7 +84,7 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { _, err := client.CreateTemplateVersion(ctx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ StorageMethod: codersdk.ProvisionerStorageMethodFile, - StorageSource: "hash", + FileID: uuid.New(), Provisioner: codersdk.ProvisionerTypeEcho, }) var apiErr *codersdk.Error @@ -112,7 +112,7 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { version, err := client.CreateTemplateVersion(ctx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ Name: "bananas", StorageMethod: codersdk.ProvisionerStorageMethodFile, - StorageSource: file.Hash, + FileID: file.ID, Provisioner: codersdk.ProvisionerTypeEcho, ParameterValues: []codersdk.CreateParameterRequest{{ Name: "example", @@ -842,7 +842,7 @@ func TestPaginatedTemplateVersions(t *testing.T) { eg.Go(func() error { templateVersion, err := client.CreateTemplateVersion(egCtx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ TemplateID: template.ID, - StorageSource: file.Hash, + FileID: file.ID, StorageMethod: codersdk.ProvisionerStorageMethodFile, Provisioner: codersdk.ProvisionerTypeEcho, }) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index b2d8424907..6881137cf8 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -458,7 +458,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { Provisioner: template.Provisioner, Type: database.ProvisionerJobTypeWorkspaceBuild, StorageMethod: templateVersionJob.StorageMethod, - StorageSource: templateVersionJob.StorageSource, + FileID: templateVersionJob.FileID, Input: input, }) if err != nil { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index c96967b126..d847af7032 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -435,7 +435,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req Provisioner: template.Provisioner, Type: database.ProvisionerJobTypeWorkspaceBuild, StorageMethod: templateVersionJob.StorageMethod, - StorageSource: templateVersionJob.StorageSource, + FileID: templateVersionJob.FileID, Input: input, }) if err != nil { diff --git a/codersdk/files.go b/codersdk/files.go index 52fcf02150..6d728fbfbc 100644 --- a/codersdk/files.go +++ b/codersdk/files.go @@ -6,6 +6,8 @@ import ( "fmt" "io" "net/http" + + "github.com/google/uuid" ) const ( @@ -14,7 +16,7 @@ const ( // UploadResponse contains the hash to reference the uploaded file. type UploadResponse struct { - Hash string `json:"hash"` + ID uuid.UUID `json:"hash"` } // Upload uploads an arbitrary file with the content type provided. @@ -35,8 +37,8 @@ func (c *Client) Upload(ctx context.Context, contentType string, content []byte) } // Download fetches a file by uploaded hash. -func (c *Client) Download(ctx context.Context, hash string) ([]byte, string, error) { - res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/files/%s", hash), nil) +func (c *Client) Download(ctx context.Context, id uuid.UUID) ([]byte, string, error) { + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/files/%s", id.String()), nil) if err != nil { return nil, "", err } diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 003b9156dd..de5e42122c 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -39,7 +39,7 @@ type CreateTemplateVersionRequest struct { TemplateID uuid.UUID `json:"template_id,omitempty"` StorageMethod ProvisionerStorageMethod `json:"storage_method" validate:"oneof=file,required"` - StorageSource string `json:"storage_source" validate:"required"` + FileID uuid.UUID `json:"file_id" validate:"required"` Provisioner ProvisionerType `json:"provisioner" validate:"oneof=terraform echo,required"` // ParameterValues allows for additional parameters to be provided // during the dry-run provision stage. diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 5eb8872fe6..adce0321be 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -64,15 +64,15 @@ const ( ) type ProvisionerJob struct { - ID uuid.UUID `json:"id"` - CreatedAt time.Time `json:"created_at"` - StartedAt *time.Time `json:"started_at,omitempty"` - CompletedAt *time.Time `json:"completed_at,omitempty"` - CanceledAt *time.Time `json:"canceled_at,omitempty"` - Error string `json:"error,omitempty"` - Status ProvisionerJobStatus `json:"status"` - WorkerID *uuid.UUID `json:"worker_id,omitempty"` - StorageSource string `json:"storage_source"` + ID uuid.UUID `json:"id"` + CreatedAt time.Time `json:"created_at"` + StartedAt *time.Time `json:"started_at,omitempty"` + CompletedAt *time.Time `json:"completed_at,omitempty"` + CanceledAt *time.Time `json:"canceled_at,omitempty"` + Error string `json:"error,omitempty"` + Status ProvisionerJobStatus `json:"status"` + WorkerID *uuid.UUID `json:"worker_id,omitempty"` + FileID uuid.UUID `json:"file_id"` } type ProvisionerJobLog struct { diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index b4f96eb610..fe6dd6f687 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -286,7 +286,7 @@ func TestTemplateACL(t *testing.T) { _, err = client1.CreateTemplateVersion(ctx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ Name: "testme", TemplateID: template.ID, - StorageSource: file.Hash, + FileID: file.ID, StorageMethod: codersdk.ProvisionerStorageMethodFile, Provisioner: codersdk.ProvisionerTypeEcho, }) @@ -302,7 +302,7 @@ func TestTemplateACL(t *testing.T) { _, err = client1.CreateTemplateVersion(ctx, user.OrganizationID, codersdk.CreateTemplateVersionRequest{ Name: "testme", TemplateID: template.ID, - StorageSource: file.Hash, + FileID: file.ID, StorageMethod: codersdk.ProvisionerStorageMethodFile, Provisioner: codersdk.ProvisionerTypeEcho, }) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 92088b978f..09c2da80fb 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -206,7 +206,7 @@ export interface CreateTemplateVersionRequest { readonly name?: string readonly template_id?: string readonly storage_method: ProvisionerStorageMethod - readonly storage_source: string + readonly file_id: string readonly provisioner: ProvisionerType readonly parameter_values?: CreateParameterRequest[] } @@ -504,7 +504,7 @@ export interface ProvisionerJob { readonly error?: string readonly status: ProvisionerJobStatus readonly worker_id?: string - readonly storage_source: string + readonly file_id: string } // From codersdk/provisionerdaemons.go diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index fe779f3789..b7c7750158 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -132,7 +132,7 @@ export const MockProvisionerJob: TypesGen.ProvisionerJob = { created_at: "", id: "test-provisioner-job", status: "succeeded", - storage_source: "asdf", + file_id: "fc0774ce-cc9e-48d4-80ae-88f7a4d4a8b0", completed_at: "2022-05-17T17:39:01.382927298Z", }