From 8187992e7f359ef96eba60135a768dc44511420b Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 27 Mar 2023 13:54:01 +0200 Subject: [PATCH] fix: Validate template version name (#6804) * WIP * Update * Validation --- coderd/database/dbauthz/querier.go | 15 ++++++-- coderd/httpapi/httpapi.go | 2 +- coderd/templateversions.go | 39 ++++++++++++++++++--- coderd/templateversions_test.go | 55 +++++++++++++++++++++++++++++- codersdk/organizations.go | 2 +- codersdk/templateversions.go | 2 +- 6 files changed, 105 insertions(+), 10 deletions(-) diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go index 2ee0579281..b926506bd5 100644 --- a/coderd/database/dbauthz/querier.go +++ b/coderd/database/dbauthz/querier.go @@ -859,11 +859,22 @@ func (q *querier) UpdateTemplateScheduleByID(ctx context.Context, arg database.U } func (q *querier) UpdateTemplateVersionByID(ctx context.Context, arg database.UpdateTemplateVersionByIDParams) (database.TemplateVersion, error) { - template, err := q.db.GetTemplateByID(ctx, arg.TemplateID.UUID) + // An actor is allowed to update the template version if they are authorized to update the template. + tv, err := q.db.GetTemplateVersionByID(ctx, arg.ID) if err != nil { return database.TemplateVersion{}, err } - if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + var obj rbac.Objecter + if !tv.TemplateID.Valid { + obj = rbac.ResourceTemplate.InOrg(tv.OrganizationID) + } else { + tpl, err := q.db.GetTemplateByID(ctx, tv.TemplateID.UUID) + if err != nil { + return database.TemplateVersion{}, err + } + obj = tpl + } + if err := q.authorizeContext(ctx, rbac.ActionUpdate, obj); err != nil { return database.TemplateVersion{}, err } return q.db.UpdateTemplateVersionByID(ctx, arg) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 467b93fc93..a832d825ab 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -44,7 +44,7 @@ func init() { valid := NameValid(str) return valid == nil } - for _, tag := range []string{"username", "template_name", "workspace_name"} { + for _, tag := range []string{"username", "template_name", "workspace_name", "template_version_name"} { err := Validate.RegisterValidation(tag, nameValidator) if err != nil { panic(err) diff --git a/coderd/templateversions.go b/coderd/templateversions.go index ef2df44d5f..ebbf4ae79f 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -92,12 +92,43 @@ func (api *API) patchTemplateVersion(rw http.ResponseWriter, r *http.Request) { if params.Name != "" { updateParams.Name = params.Name } - // It is not allowed to "patch" the template ID, and reassign it. - updatedTemplateVersion, err := api.Database.UpdateTemplateVersionByID(ctx, updateParams) + + errTemplateVersionNameConflict := xerrors.New("template version name must be unique for a template") + + var updatedTemplateVersion database.TemplateVersion + err := api.Database.InTx(func(tx database.Store) error { + if templateVersion.TemplateID.Valid && templateVersion.Name != updateParams.Name { + // User wants to rename the template version + + _, err := tx.GetTemplateVersionByTemplateIDAndName(ctx, database.GetTemplateVersionByTemplateIDAndNameParams{ + TemplateID: templateVersion.TemplateID, + Name: updateParams.Name, + }) + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + return xerrors.Errorf("error on retrieving conflicting template version: %v", err) + } + if err == nil { + return errTemplateVersionNameConflict + } + } + + // It is not allowed to "patch" the template ID, and reassign it. + var err error + updatedTemplateVersion, err = tx.UpdateTemplateVersionByID(ctx, updateParams) + if err != nil { + return xerrors.Errorf("error on patching template version: %v", err) + } + return nil + }, nil) + if errors.Is(err, errTemplateVersionNameConflict) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: err.Error(), + }) + return + } if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Error on patching template version.", - Detail: err.Error(), + Message: err.Error(), }) return } diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 7dc139c5c4..ac7a39a707 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -1347,7 +1347,7 @@ func TestTemplateVersionPatch(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - const newName = "new_name" + const newName = "new-name" updatedVersion, err := client.UpdateTemplateVersion(ctx, version.ID, codersdk.PatchTemplateVersionRequest{ Name: newName, }) @@ -1376,6 +1376,7 @@ func TestTemplateVersionPatch(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) user := coderdtest.CreateFirstUser(t, client) + version1 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) coderdtest.CreateTemplate(t, client, user.OrganizationID, version1.ID) version2 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) @@ -1398,4 +1399,56 @@ func TestTemplateVersionPatch(t *testing.T) { assert.NotEqual(t, updatedVersion1.ID, updatedVersion2.ID) assert.Equal(t, updatedVersion1.Name, updatedVersion2.Name) }) + + t.Run("Use the same name for two versions for the same templates", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version1 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version1.ID) + + version2 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.TemplateID = template.ID + }) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + _, err := client.UpdateTemplateVersion(ctx, version2.ID, codersdk.PatchTemplateVersionRequest{ + Name: version1.Name, + }) + require.Error(t, err) + }) + + t.Run("Rename the unassigned template", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version1 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + const commonTemplateVersionName = "common-template-version-name" + updatedVersion1, err := client.UpdateTemplateVersion(ctx, version1.ID, codersdk.PatchTemplateVersionRequest{ + Name: commonTemplateVersionName, + }) + require.NoError(t, err) + assert.Equal(t, commonTemplateVersionName, updatedVersion1.Name) + }) + + t.Run("Use incorrect template version name", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version1 := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + const incorrectTemplateVersionName = "incorrect/name" + _, err := client.UpdateTemplateVersion(ctx, version1.ID, codersdk.PatchTemplateVersionRequest{ + Name: incorrectTemplateVersionName, + }) + require.Error(t, err) + }) } diff --git a/codersdk/organizations.go b/codersdk/organizations.go index f3a30bf4f9..c232242540 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -42,7 +42,7 @@ type OrganizationMember struct { // CreateTemplateVersionRequest enables callers to create a new Template Version. type CreateTemplateVersionRequest struct { - Name string `json:"name,omitempty" validate:"omitempty,template_name"` + Name string `json:"name,omitempty" validate:"omitempty,template_version_name"` // TemplateID optionally associates a version with a template. TemplateID uuid.UUID `json:"template_id,omitempty" format:"uuid"` StorageMethod ProvisionerStorageMethod `json:"storage_method" validate:"oneof=file,required" enums:"file"` diff --git a/codersdk/templateversions.go b/codersdk/templateversions.go index c823a79282..cc2087027b 100644 --- a/codersdk/templateversions.go +++ b/codersdk/templateversions.go @@ -77,7 +77,7 @@ type TemplateVersionVariable struct { } type PatchTemplateVersionRequest struct { - Name string `json:"name"` + Name string `json:"name" validate:"omitempty,template_version_name"` } // TemplateVersion returns a template version by ID.