diff --git a/.dredd/hooks/capabilities.go b/.dredd/hooks/capabilities.go index 869dda4653..224916f1d4 100644 --- a/.dredd/hooks/capabilities.go +++ b/.dredd/hooks/capabilities.go @@ -294,6 +294,10 @@ func alterRequestBody(t *trans.Transaction) { if invite != nil { bodyFieldProcessor("invite_id", 4, &request) } + if t.Request.Method == "PUT" && strings.Contains(t.Request.URI, "/templates/") { + bodyFieldProcessor("name", "Test-"+getUUID(), &request) + } + bodyFieldProcessor("environment_id", environmentID, &request) bodyFieldProcessor("environment_ids", []int{environmentID}, &request) bodyFieldProcessor("inventory_id", inventoryID, &request) diff --git a/api-docs.yml b/api-docs.yml index 6a76d48c64..d57e068384 100644 --- a/api-docs.yml +++ b/api-docs.yml @@ -3445,11 +3445,17 @@ paths: - name: task in: body required: true + description: > + Either template_id or template_name must be given. When both are + given, template_id is used. schema: type: object properties: template_id: type: integer + template_name: + type: string + example: Build website debug: type: boolean dry_run: diff --git a/api/projects/tasks.go b/api/projects/tasks.go index 41a55ca1c6..fc2a475c28 100644 --- a/api/projects/tasks.go +++ b/api/projects/tasks.go @@ -37,12 +37,40 @@ func taskPool(r *http.Request) *tasks.TaskPool { } // AddTask inserts a task into the database and returns a header or returns error +// resolveTaskTemplate returns the template of the task, which may be referenced +// either by id or by name. The resolved id is written back to the task so the +// rest of the pipeline only deals with ids. +func (c *TaskController) resolveTaskTemplate(projectID int, task *db.Task) (tpl db.Template, err error) { + // The name is resolved to an id here, so it is cleared to keep it out of the + // stored task and the response. + name := task.TemplateName + task.TemplateName = "" + + if task.TemplateID == 0 && name == "" { + err = common_errors.NewValidationError("template_id or template_name is required") + return + } + + if task.TemplateID != 0 { + tpl, err = c.store.GetTemplate(projectID, task.TemplateID) + return + } + + tpl, err = c.store.GetTemplateByName(projectID, name) + if err != nil { + return + } + + task.TemplateID = tpl.ID + return +} + func (c *TaskController) AddTask(w http.ResponseWriter, r *http.Request) { project := helpers.GetFromContext(r, "project").(db.Project) user := helpers.GetFromContext(r, "user").(*db.User) taskObj := helpers.GetFromContext(r, "task").(db.Task) - tpl, err := c.store.GetTemplate(project.ID, taskObj.TemplateID) + tpl, err := c.resolveTaskTemplate(project.ID, &taskObj) if err != nil { helpers.WriteError(w, err) return diff --git a/api/projects/tasks_test.go b/api/projects/tasks_test.go index 21d6993ea6..61e195d4ff 100644 --- a/api/projects/tasks_test.go +++ b/api/projects/tasks_test.go @@ -5,7 +5,9 @@ import ( "testing" "github.com/semaphoreui/semaphore/db" + "github.com/semaphoreui/semaphore/db/sql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseTasksPageParams(t *testing.T) { @@ -52,3 +54,127 @@ func TestParseTasksPageParams_PreservesBase(t *testing.T) { assert.Equal(t, maxTasksPageSize, pageSize) assert.Equal(t, maxTasksPageSize+1, params.Count) } + +// createTaskTestTemplate creates a template usable by the task tests. Templates +// are not unique by name, so the name is a parameter to cover ambiguity. +func createTaskTestTemplate(t *testing.T, store db.Store, projectID int, repositoryID int, name string) db.Template { + t.Helper() + + tpl, err := store.CreateTemplate(db.Template{ + Name: name, + Playbook: "test.yml", + ProjectID: projectID, + RepositoryID: repositoryID, + }) + require.NoError(t, err) + + return tpl +} + +func TestResolveTaskTemplate(t *testing.T) { + store := sql.InitConfigCreateTestStore() + + project, err := store.CreateProject(db.Project{Name: "task template resolution"}) + require.NoError(t, err) + + key, err := store.CreateAccessKey(db.AccessKey{ + ProjectID: &project.ID, + Name: "none", + Type: db.AccessKeyNone, + }) + require.NoError(t, err) + + repo, err := store.CreateRepository(db.Repository{ + ProjectID: project.ID, + SSHKeyID: key.ID, + Name: "repo", + GitURL: "git@example.com:test/test", + GitBranch: "master", + }) + require.NoError(t, err) + + build := createTaskTestTemplate(t, store, project.ID, repo.ID, "Build website") + + otherProject, err := store.CreateProject(db.Project{Name: "other"}) + require.NoError(t, err) + + c := &TaskController{store: store} + + t.Run("resolves by id", func(t *testing.T) { + task := db.Task{TemplateID: build.ID} + + tpl, err := c.resolveTaskTemplate(project.ID, &task) + + require.NoError(t, err) + assert.Equal(t, build.ID, tpl.ID) + }) + + t.Run("resolves by name and writes the id back", func(t *testing.T) { + task := db.Task{TemplateName: "Build website"} + + tpl, err := c.resolveTaskTemplate(project.ID, &task) + + require.NoError(t, err) + assert.Equal(t, build.ID, tpl.ID) + assert.Equal(t, build.ID, task.TemplateID, "the resolved id must be written back to the task") + assert.Empty(t, task.TemplateName, "the name must not survive into the stored task or the response") + }) + + t.Run("id wins when both are given", func(t *testing.T) { + task := db.Task{TemplateID: build.ID, TemplateName: "does not exist"} + + tpl, err := c.resolveTaskTemplate(project.ID, &task) + + require.NoError(t, err) + assert.Equal(t, build.ID, tpl.ID) + assert.Empty(t, task.TemplateName) + }) + + t.Run("neither id nor name is rejected", func(t *testing.T) { + task := db.Task{} + + _, err := c.resolveTaskTemplate(project.ID, &task) + + require.Error(t, err) + assert.Contains(t, err.Error(), "template_id or template_name is required") + }) + + t.Run("unknown name is not found", func(t *testing.T) { + task := db.Task{TemplateName: "no such template"} + + _, err := c.resolveTaskTemplate(project.ID, &task) + + assert.ErrorIs(t, err, db.ErrNotFound) + }) + + t.Run("a template of another project is not found", func(t *testing.T) { + task := db.Task{TemplateName: "Build website"} + + _, err := c.resolveTaskTemplate(otherProject.ID, &task) + + assert.ErrorIs(t, err, db.ErrNotFound) + }) + + t.Run("an ambiguous name is rejected", func(t *testing.T) { + // Both the store and the unique index reject a duplicate name, so the + // collision has to be made behind their backs. This is a database which + // lost the index, for example one restored from a schema-less dump: the + // task must still refuse to guess rather than run the wrong template. + createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate") + legacy := createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate (2)") + + _, err = store.Sql().Exec("drop index project__template__project_id_name") + require.NoError(t, err) + + _, err = store.Sql().Exec( + "update project__template set name=? where id=?", "Duplicate", legacy.ID) + require.NoError(t, err) + + task := db.Task{TemplateName: "Duplicate"} + + _, err = c.resolveTaskTemplate(project.ID, &task) + + require.Error(t, err) + assert.Contains(t, err.Error(), "more than one template") + }) +} diff --git a/db/Migration.go b/db/Migration.go index 3f31649415..eefbc17149 100644 --- a/db/Migration.go +++ b/db/Migration.go @@ -137,6 +137,7 @@ func GetMigrations(dialect string) []Migration { {Version: "2.20.1"}, {Version: "2.20.2"}, {Version: "2.20.3"}, + {Version: "2.20.4"}, } return append(initScripts, commonScripts...) diff --git a/db/Store.go b/db/Store.go index 52dd346658..1012b7cd5b 100644 --- a/db/Store.go +++ b/db/Store.go @@ -268,6 +268,7 @@ type TemplateManager interface { CreateTemplate(template Template) (Template, error) UpdateTemplate(template Template) error GetTemplate(projectID int, templateID int) (Template, error) + GetTemplateByName(projectID int, name string) (Template, error) DeleteTemplate(projectID int, templateID int) error SetTemplateDescription(projectID int, templateID int, description string) error GetTemplateVaults(projectID int, templateID int) ([]TemplateVault, error) diff --git a/db/Task.go b/db/Task.go index 1959b24e7b..b147051e8f 100644 --- a/db/Task.go +++ b/db/Task.go @@ -41,9 +41,14 @@ type AnsibleTaskParams struct { // Task is a model of a task which will be executed by the runner type Task struct { ID int `db:"id" json:"id"` - TemplateID int `db:"template_id" json:"template_id" binding:"required"` + TemplateID int `db:"template_id" json:"template_id"` ProjectID int `db:"project_id" json:"project_id"` + // TemplateName allows a task to reference its template by name instead of + // by id when it is created through the API. It is resolved to TemplateID by + // the API and never stored. + TemplateName string `db:"-" json:"template_name,omitempty"` + Status task_logger.TaskStatus `db:"status" json:"status"` // override variables diff --git a/db/sql/migration.go b/db/sql/migration.go index 78f2ae2f8a..cf8f4598f3 100644 --- a/db/sql/migration.go +++ b/db/sql/migration.go @@ -244,6 +244,8 @@ func (d *SqlDb) ApplyMigration(migration db.Migration) error { err = migration_2_16_8{db: d}.PreApply(tx) case "2.18.4": err = migration_2_18_4{db: d}.PreApply(tx) + case "2.20.4": + err = migration_2_20_4{db: d}.PreApply(tx) } if err != nil { diff --git a/db/sql/migration_2_20_4.go b/db/sql/migration_2_20_4.go new file mode 100644 index 0000000000..e53d5a8ba7 --- /dev/null +++ b/db/sql/migration_2_20_4.go @@ -0,0 +1,85 @@ +package sql + +import ( + "strconv" + + "github.com/go-gorp/gorp/v3" +) + +type migration_2_20_4 struct { + db *SqlDb +} + +// PreApply renames templates which share a name inside a project, so that +// v2.20.4.sql can put a unique index on (project_id, name). Template names were +// never unique before, so any installation may hold duplicates and the index +// would otherwise fail the upgrade. +// +// Which names collide is decided by the database rather than by comparing them +// here: the unique index uses the collation of the column, and on the default +// MySQL and MariaDB collations "Build", "build" and "Build " are all the same +// key while Go sees three different strings. Comparing in Go would leave those +// rows in place and the index would still fail. +// +// The rename is a loop rather than a single statement because a generated name +// can itself be taken ("Build" twice next to a real "Build (2)"). +func (m migration_2_20_4) PreApply(tx *gorp.Transaction) error { + type templateName struct { + ID int `db:"id"` + ProjectID int `db:"project_id"` + Name string `db:"name"` + } + + var duplicates []templateName + + // Every template which an older one of the project already shadows. Ordered + // by id so the oldest keeps its name and the result does not depend on the + // order rows come back in. + _, err := tx.Select(&duplicates, m.db.PrepareQuery( + "select `id`, `project_id`, `name` from `project__template` t "+ + "where exists (select 1 from (select `id`, `project_id`, `name` from `project__template`) o "+ + "where o.`project_id` = t.`project_id` and o.`name` = t.`name` and o.`id` < t.`id`) "+ + "order by t.`id`")) + + if err != nil { + return err + } + + for _, template := range duplicates { + name, err := m.freeTemplateName(tx, template.ProjectID, template.Name) + if err != nil { + return err + } + + _, err = tx.Exec( + m.db.PrepareQuery("update `project__template` set `name`=? where `id`=?"), + name, template.ID) + + if err != nil { + return err + } + } + + return nil +} + +// freeTemplateName returns a name based on base which no template of the project +// uses. Availability is asked of the database so that the answer follows the +// same collation as the unique index. +func (m migration_2_20_4) freeTemplateName(tx *gorp.Transaction, projectID int, base string) (string, error) { + for i := 2; ; i++ { + name := base + " (" + strconv.Itoa(i) + ")" + + count, err := tx.SelectInt(m.db.PrepareQuery( + "select count(*) from `project__template` where `project_id`=? and `name`=?"), + projectID, name) + + if err != nil { + return "", err + } + + if count == 0 { + return name, nil + } + } +} diff --git a/db/sql/migration_2_20_4_test.go b/db/sql/migration_2_20_4_test.go new file mode 100644 index 0000000000..4cb1de16dc --- /dev/null +++ b/db/sql/migration_2_20_4_test.go @@ -0,0 +1,77 @@ +package sql + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMigration_2_20_2_EnforcesUniqueTemplateName checks the index exists after +// the migrations the test store runs, so that a name identifies one template. +func TestMigration_2_20_2_EnforcesUniqueTemplateName(t *testing.T) { + store := InitConfigCreateTestStore() + projectID, repositoryID := newTemplateTestProject(t, store) + + insert := func(name string) error { + _, err := store.exec( + "insert into project__template (project_id, repository_id, name, playbook, arguments, allow_override_args_in_task, app) values (?, ?, ?, 'site.yml', '[]', false, 'ansible')", + projectID, repositoryID, name) + return err + } + + require.NoError(t, insert("Build website")) + + // The store also rejects this, but here the insert goes straight to the + // database, so only the index can stop it. + assert.Error(t, insert("Build website")) + + assert.NoError(t, insert("Deploy website")) +} + +// TestMigration_2_20_2_RenamesExistingDuplicates covers PreApply: templates which +// already share a name must be renamed rather than failing the upgrade. +func TestMigration_2_20_2_RenamesExistingDuplicates(t *testing.T) { + store := InitConfigCreateTestStore() + projectID, repositoryID := newTemplateTestProject(t, store) + + // Recreate the pre-migration state: drop the index, then write the duplicates + // an old installation would be holding. + _, err := store.exec("drop index project__template__project_id_name") + require.NoError(t, err) + + names := []string{"Build", "Build", "Build", "Build (2)", "Deploy"} + for _, name := range names { + _, err = store.exec( + "insert into project__template (project_id, repository_id, name, playbook, arguments, allow_override_args_in_task, app) values (?, ?, ?, 'site.yml', '[]', false, 'ansible')", + projectID, repositoryID, name) + require.NoError(t, err) + } + + tx, err := store.Sql().Begin() + require.NoError(t, err) + + require.NoError(t, migration_2_20_4{db: store}.PreApply(tx)) + require.NoError(t, tx.Commit()) + + var renamed []struct { + ID int `db:"id"` + Name string `db:"name"` + } + _, err = store.Sql().Select(&renamed, + "select id, name from project__template order by id") + require.NoError(t, err) + + // The oldest template of each name keeps it; the rest get a suffix, skipping + // "Build (2)" because that name is already taken. + actual := make([]string, 0, len(renamed)) + for _, template := range renamed { + actual = append(actual, template.Name) + } + assert.Equal(t, []string{"Build", "Build (3)", "Build (4)", "Build (2)", "Deploy"}, actual) + + // The renames must leave the table indexable. + _, err = store.exec( + "create unique index project__template__project_id_name on project__template (project_id, name)") + assert.NoError(t, err) +} diff --git a/db/sql/migrations/v2.20.4.sql b/db/sql/migrations/v2.20.4.sql new file mode 100644 index 0000000000..ffc705c689 --- /dev/null +++ b/db/sql/migrations/v2.20.4.sql @@ -0,0 +1 @@ +create unique index `project__template__project_id_name` on `project__template` (`project_id`, `name`); diff --git a/db/sql/template.go b/db/sql/template.go index bd945da0ea..db40bc61b3 100644 --- a/db/sql/template.go +++ b/db/sql/template.go @@ -10,11 +10,39 @@ import ( log "github.com/sirupsen/logrus" ) +// validateTemplateNameIsFree rejects a template name which is already used by +// another template of the same project, so that a template can be referred to by +// name. templateID is the template being updated, or 0 when creating one. +// +// Templates created before this check may still share a name, which is why +// GetTemplateByName rejects an ambiguous name rather than relying on this. +func (d *SqlDb) validateTemplateNameIsFree(projectID int, templateID int, name string) error { + var count int + + err := d.selectOne(&count, + "select count(*) from project__template where project_id=? and name=? and id<>?", + projectID, name, templateID) + + if err != nil { + return err + } + + if count > 0 { + return common_errors.NewValidationError("template with name " + name + " already exists") + } + + return nil +} + func (d *SqlDb) CreateTemplate(tmpl db.Template) (db.Template, error) { if err := tmpl.Validate(); err != nil { return db.Template{}, err } + if err := d.validateTemplateNameIsFree(tmpl.ProjectID, 0, tmpl.Name); err != nil { + return db.Template{}, err + } + tmpl.ApplyLegacyEnvironmentField() query, args, err := sq.Insert("project__template"). @@ -78,6 +106,10 @@ func (d *SqlDb) UpdateTemplate(tmpl db.Template) error { return err } + if err = d.validateTemplateNameIsFree(tmpl.ProjectID, tmpl.ID, tmpl.Name); err != nil { + return err + } + query, args, err := sq.Update("project__template"). SetMap(map[string]any{ "inventory_id": tmpl.InventoryID, @@ -415,6 +447,37 @@ func (d *SqlDb) GetTemplates(projectID int, filter db.TemplateFilter, params db. return } +// GetTemplateByName returns the template of the project with the given name. +// Template names are not unique per project, so an ambiguous name is rejected +// instead of silently running one of the matching templates. +func (d *SqlDb) GetTemplateByName(projectID int, name string) (template db.Template, err error) { + var templates []db.Template + + _, err = d.selectAll( + &templates, + "select * from project__template where project_id=? and name=? limit 2", + projectID, + name) + + if err != nil { + return + } + + switch len(templates) { + case 0: + err = db.ErrNotFound + return + case 1: + default: + err = common_errors.NewValidationError("more than one template is named " + name + ", use template_id") + return + } + + template = templates[0] + err = db.FillTemplate(d, &template) + return +} + func (d *SqlDb) GetTemplate(projectID int, templateID int) (template db.Template, err error) { err = d.selectOne( &template, diff --git a/db/sql/template_test.go b/db/sql/template_test.go index 0a8cb0d35a..b2652b074d 100644 --- a/db/sql/template_test.go +++ b/db/sql/template_test.go @@ -122,6 +122,68 @@ func TestTemplateWithoutExecutorImage(t *testing.T) { assert.Nil(t, loaded.ExecutorImage) } +// TestTemplateNameUniqueness covers the check which lets a template be referred +// to by name: a name may be reused across projects, but not inside one. +func TestTemplateNameUniqueness(t *testing.T) { + store := InitConfigCreateTestStore() + projectID, repositoryID := newTemplateTestProject(t, store) + + newTemplate := func(name string) db.Template { + return db.Template{ + ProjectID: projectID, + RepositoryID: repositoryID, + Name: name, + Playbook: "site.yml", + } + } + + build, err := store.CreateTemplate(newTemplate("Build website")) + require.NoError(t, err) + + t.Run("a duplicate name is rejected", func(t *testing.T) { + _, err := store.CreateTemplate(newTemplate("Build website")) + + require.Error(t, err) + assert.ErrorContains(t, err, "already exists") + }) + + t.Run("a free name is accepted", func(t *testing.T) { + _, err := store.CreateTemplate(newTemplate("Deploy website")) + + assert.NoError(t, err) + }) + + t.Run("the same name in another project is accepted", func(t *testing.T) { + otherProjectID, otherRepositoryID := newTemplateTestProject(t, store) + + _, err := store.CreateTemplate(db.Template{ + ProjectID: otherProjectID, + RepositoryID: otherRepositoryID, + Name: "Build website", + Playbook: "site.yml", + }) + + assert.NoError(t, err) + }) + + t.Run("a template keeps its own name on update", func(t *testing.T) { + description := "edited" + build.Description = &description + + assert.NoError(t, store.UpdateTemplate(build)) + }) + + t.Run("renaming onto another template is rejected", func(t *testing.T) { + renamed := build + renamed.Name = "Deploy website" + + err := store.UpdateTemplate(renamed) + + require.Error(t, err) + assert.ErrorContains(t, err, "already exists") + }) +} + // TestTemplate_WorkingDirectoryRoundTrip checks // project__template.working_directory is written, read back, updated, and // cleared through the store. diff --git a/web/public/swagger/api-docs.yml b/web/public/swagger/api-docs.yml index 5aad742cc0..9a026b4cdf 100644 --- a/web/public/swagger/api-docs.yml +++ b/web/public/swagger/api-docs.yml @@ -2740,11 +2740,17 @@ paths: - name: task in: body required: true + description: > + Either template_id or template_name must be given. When both are + given, template_id is used. schema: type: object properties: template_id: type: integer + template_name: + type: string + example: Build website debug: type: boolean dry_run: