From e273817154e98b7b675e43e97cd17f48a603cf9e Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 9 Feb 2020 15:33:03 +0100 Subject: [PATCH] [API] Fix inconsistent label color format (#10129) * update and use labelColorPattern * add TestCases * fix lint * # optional for templates * fix typo * some more * fix lint of **master** --- integrations/api_issue_label_test.go | 71 ++++++++++++++++++ models/issue_label.go | 104 +++++++++++++++------------ models/issue_label_test.go | 5 +- models/user.go | 2 +- modules/repository/create.go | 6 +- modules/structs/issue_label.go | 2 +- routers/api/v1/repo/label.go | 24 ++++++- routers/repo/issue_label.go | 4 +- templates/swagger/v1_json.tmpl | 6 ++ 9 files changed, 170 insertions(+), 54 deletions(-) diff --git a/integrations/api_issue_label_test.go b/integrations/api_issue_label_test.go index bf50252ed8..6cdb3a0dad 100644 --- a/integrations/api_issue_label_test.go +++ b/integrations/api_issue_label_test.go @@ -7,6 +7,7 @@ package integrations import ( "fmt" "net/http" + "strings" "testing" "code.gitea.io/gitea/models" @@ -15,6 +16,76 @@ import ( "github.com/stretchr/testify/assert" ) +func TestAPIModifyLabels(t *testing.T) { + assert.NoError(t, models.LoadFixtures()) + + repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) + owner := models.AssertExistsAndLoadBean(t, &models.User{ID: repo.OwnerID}).(*models.User) + session := loginUser(t, owner.Name) + token := getTokenForLoggedInUser(t, session) + urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/labels?token=%s", owner.Name, repo.Name, token) + + // CreateLabel + req := NewRequestWithJSON(t, "POST", urlStr, &api.CreateLabelOption{ + Name: "TestL 1", + Color: "abcdef", + Description: "test label", + }) + resp := session.MakeRequest(t, req, http.StatusCreated) + apiLabel := new(api.Label) + DecodeJSON(t, resp, &apiLabel) + dbLabel := models.AssertExistsAndLoadBean(t, &models.Label{ID: apiLabel.ID, RepoID: repo.ID}).(*models.Label) + assert.EqualValues(t, dbLabel.Name, apiLabel.Name) + assert.EqualValues(t, strings.TrimLeft(dbLabel.Color, "#"), apiLabel.Color) + + req = NewRequestWithJSON(t, "POST", urlStr, &api.CreateLabelOption{ + Name: "TestL 2", + Color: "#123456", + Description: "jet another test label", + }) + session.MakeRequest(t, req, http.StatusCreated) + req = NewRequestWithJSON(t, "POST", urlStr, &api.CreateLabelOption{ + Name: "WrongTestL", + Color: "#12345g", + }) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + + //ListLabels + req = NewRequest(t, "GET", urlStr) + resp = session.MakeRequest(t, req, http.StatusOK) + var apiLabels []*api.Label + DecodeJSON(t, resp, &apiLabels) + assert.Len(t, apiLabels, 2) + + //GetLabel + singleURLStr := fmt.Sprintf("/api/v1/repos/%s/%s/labels/%d?token=%s", owner.Name, repo.Name, dbLabel.ID, token) + req = NewRequest(t, "GET", singleURLStr) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &apiLabel) + assert.EqualValues(t, strings.TrimLeft(dbLabel.Color, "#"), apiLabel.Color) + + //EditLabel + newName := "LabelNewName" + newColor := "09876a" + newColorWrong := "09g76a" + req = NewRequestWithJSON(t, "PATCH", singleURLStr, &api.EditLabelOption{ + Name: &newName, + Color: &newColor, + }) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &apiLabel) + assert.EqualValues(t, newColor, apiLabel.Color) + req = NewRequestWithJSON(t, "PATCH", singleURLStr, &api.EditLabelOption{ + Color: &newColorWrong, + }) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + + //DeleteLabel + req = NewRequest(t, "DELETE", singleURLStr) + resp = session.MakeRequest(t, req, http.StatusNoContent) + +} + func TestAPIAddIssueLabels(t *testing.T) { assert.NoError(t, models.LoadFixtures()) diff --git a/models/issue_label.go b/models/issue_label.go index abf0521cea..9e492dbec1 100644 --- a/models/issue_label.go +++ b/models/issue_label.go @@ -18,47 +18,8 @@ import ( "xorm.io/xorm" ) -var labelColorPattern = regexp.MustCompile("#([a-fA-F0-9]{6})") - -// GetLabelTemplateFile loads the label template file by given name, -// then parses and returns a list of name-color pairs and optionally description. -func GetLabelTemplateFile(name string) ([][3]string, error) { - data, err := GetRepoInitFile("label", name) - if err != nil { - return nil, fmt.Errorf("GetRepoInitFile: %v", err) - } - - lines := strings.Split(string(data), "\n") - list := make([][3]string, 0, len(lines)) - for i := 0; i < len(lines); i++ { - line := strings.TrimSpace(lines[i]) - if len(line) == 0 { - continue - } - - parts := strings.SplitN(line, ";", 2) - - fields := strings.SplitN(parts[0], " ", 2) - if len(fields) != 2 { - return nil, fmt.Errorf("line is malformed: %s", line) - } - - if !labelColorPattern.MatchString(fields[0]) { - return nil, fmt.Errorf("bad HTML color code in line: %s", line) - } - - var description string - - if len(parts) > 1 { - description = strings.TrimSpace(parts[1]) - } - - fields[1] = strings.TrimSpace(fields[1]) - list = append(list, [3]string{fields[1], fields[0], description}) - } - - return list, nil -} +// LabelColorPattern is a regexp witch can validate LabelColor +var LabelColorPattern = regexp.MustCompile("^#[0-9a-fA-F]{6}$") // Label represents a label of repository for issues. type Label struct { @@ -86,6 +47,50 @@ func (label *Label) APIFormat() *api.Label { } } +// GetLabelTemplateFile loads the label template file by given name, +// then parses and returns a list of name-color pairs and optionally description. +func GetLabelTemplateFile(name string) ([][3]string, error) { + data, err := GetRepoInitFile("label", name) + if err != nil { + return nil, fmt.Errorf("GetRepoInitFile: %v", err) + } + + lines := strings.Split(string(data), "\n") + list := make([][3]string, 0, len(lines)) + for i := 0; i < len(lines); i++ { + line := strings.TrimSpace(lines[i]) + if len(line) == 0 { + continue + } + + parts := strings.SplitN(line, ";", 2) + + fields := strings.SplitN(parts[0], " ", 2) + if len(fields) != 2 { + return nil, fmt.Errorf("line is malformed: %s", line) + } + + color := strings.Trim(fields[0], " ") + if len(color) == 6 { + color = "#" + color + } + if !LabelColorPattern.MatchString(color) { + return nil, fmt.Errorf("bad HTML color code in line: %s", line) + } + + var description string + + if len(parts) > 1 { + description = strings.TrimSpace(parts[1]) + } + + fields[1] = strings.TrimSpace(fields[1]) + list = append(list, [3]string{fields[1], color, description}) + } + + return list, nil +} + // CalOpenIssues calculates the open issues of label. func (label *Label) CalOpenIssues() { label.NumOpenIssues = label.NumIssues - label.NumClosedIssues @@ -152,7 +157,7 @@ func LoadLabelsFormatted(labelTemplate string) (string, error) { return strings.Join(labels, ", "), err } -func initalizeLabels(e Engine, repoID int64, labelTemplate string) error { +func initializeLabels(e Engine, repoID int64, labelTemplate string) error { list, err := GetLabelTemplateFile(labelTemplate) if err != nil { return ErrIssueLabelTemplateLoad{labelTemplate, err} @@ -175,9 +180,9 @@ func initalizeLabels(e Engine, repoID int64, labelTemplate string) error { return nil } -// InitalizeLabels adds a label set to a repository using a template -func InitalizeLabels(ctx DBContext, repoID int64, labelTemplate string) error { - return initalizeLabels(ctx.e, repoID, labelTemplate) +// InitializeLabels adds a label set to a repository using a template +func InitializeLabels(ctx DBContext, repoID int64, labelTemplate string) error { + return initializeLabels(ctx.e, repoID, labelTemplate) } func newLabel(e Engine, label *Label) error { @@ -187,6 +192,9 @@ func newLabel(e Engine, label *Label) error { // NewLabel creates a new label for a repository func NewLabel(label *Label) error { + if !LabelColorPattern.MatchString(label.Color) { + return fmt.Errorf("bad color code: %s", label.Color) + } return newLabel(x, label) } @@ -198,6 +206,9 @@ func NewLabels(labels ...*Label) error { return err } for _, label := range labels { + if !LabelColorPattern.MatchString(label.Color) { + return fmt.Errorf("bad color code: %s", label.Color) + } if err := newLabel(sess, label); err != nil { return err } @@ -359,6 +370,9 @@ func updateLabel(e Engine, l *Label) error { // UpdateLabel updates label information. func UpdateLabel(l *Label) error { + if !LabelColorPattern.MatchString(l.Color) { + return fmt.Errorf("bad color code: %s", l.Color) + } return updateLabel(x, l) } diff --git a/models/issue_label_test.go b/models/issue_label_test.go index e0aaf82f76..d831792861 100644 --- a/models/issue_label_test.go +++ b/models/issue_label_test.go @@ -45,8 +45,11 @@ func TestNewLabels(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) labels := []*Label{ {RepoID: 2, Name: "labelName2", Color: "#123456"}, - {RepoID: 3, Name: "labelName3", Color: "#234567"}, + {RepoID: 3, Name: "labelName3", Color: "#23456F"}, } + assert.Error(t, NewLabel(&Label{RepoID: 3, Name: "invalid Color", Color: ""})) + assert.Error(t, NewLabel(&Label{RepoID: 3, Name: "invalid Color", Color: "123456"})) + assert.Error(t, NewLabel(&Label{RepoID: 3, Name: "invalid Color", Color: "#12345G"})) for _, label := range labels { AssertNotExistsBean(t, label) } diff --git a/models/user.go b/models/user.go index a68db6cf40..220a9f9a9a 100644 --- a/models/user.go +++ b/models/user.go @@ -1044,7 +1044,7 @@ func ChangeUserName(u *User, newUserName string) (err error) { } else if isExist { return ErrUserAlreadyExist{newUserName} } - + sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { diff --git a/modules/repository/create.go b/modules/repository/create.go index dc96b856d9..255bf09731 100644 --- a/modules/repository/create.go +++ b/modules/repository/create.go @@ -58,15 +58,15 @@ func CreateRepository(doer, u *models.User, opts models.CreateRepoOptions) (_ *m // Initialize Issue Labels if selected if len(opts.IssueLabels) > 0 { - if err = models.InitalizeLabels(ctx, repo.ID, opts.IssueLabels); err != nil { - return fmt.Errorf("initalizeLabels: %v", err) + if err = models.InitializeLabels(ctx, repo.ID, opts.IssueLabels); err != nil { + return fmt.Errorf("InitializeLabels: %v", err) } } if stdout, err := git.NewCommand("update-server-info"). SetDescription(fmt.Sprintf("CreateRepository(git update-server-info): %s", repoPath)). RunInDir(repoPath); err != nil { - log.Error("CreateRepitory(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) + log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) return fmt.Errorf("CreateRepository(git update-server-info): %v", err) } } diff --git a/modules/structs/issue_label.go b/modules/structs/issue_label.go index 0789624ab1..2aadd8638e 100644 --- a/modules/structs/issue_label.go +++ b/modules/structs/issue_label.go @@ -22,7 +22,7 @@ type CreateLabelOption struct { Name string `json:"name" binding:"Required"` // required:true // example: #00aabb - Color string `json:"color" binding:"Required;Size(7)"` + Color string `json:"color" binding:"Required"` Description string `json:"description"` } diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go index 6d438610be..422046001d 100644 --- a/routers/api/v1/repo/label.go +++ b/routers/api/v1/repo/label.go @@ -6,8 +6,10 @@ package repo import ( + "fmt" "net/http" "strconv" + "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" @@ -135,6 +137,17 @@ func CreateLabel(ctx *context.APIContext, form api.CreateLabelOption) { // responses: // "201": // "$ref": "#/responses/Label" + // "422": + // "$ref": "#/responses/validationError" + + form.Color = strings.Trim(form.Color, " ") + if len(form.Color) == 6 { + form.Color = "#" + form.Color + } + if !models.LabelColorPattern.MatchString(form.Color) { + ctx.Error(http.StatusUnprocessableEntity, "ColorPattern", fmt.Errorf("bad color code: %s", form.Color)) + return + } label := &models.Label{ Name: form.Name, @@ -182,6 +195,8 @@ func EditLabel(ctx *context.APIContext, form api.EditLabelOption) { // responses: // "200": // "$ref": "#/responses/Label" + // "422": + // "$ref": "#/responses/validationError" label, err := models.GetLabelInRepoByID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")) if err != nil { @@ -197,7 +212,14 @@ func EditLabel(ctx *context.APIContext, form api.EditLabelOption) { label.Name = *form.Name } if form.Color != nil { - label.Color = *form.Color + label.Color = strings.Trim(*form.Color, " ") + if len(label.Color) == 6 { + label.Color = "#" + label.Color + } + if !models.LabelColorPattern.MatchString(label.Color) { + ctx.Error(http.StatusUnprocessableEntity, "ColorPattern", fmt.Errorf("bad color code: %s", label.Color)) + return + } } if form.Description != nil { label.Description = *form.Description diff --git a/routers/repo/issue_label.go b/routers/repo/issue_label.go index e61fcbe5cb..8ac9b8d336 100644 --- a/routers/repo/issue_label.go +++ b/routers/repo/issue_label.go @@ -35,14 +35,14 @@ func InitializeLabels(ctx *context.Context, form auth.InitializeLabelsForm) { return } - if err := models.InitalizeLabels(models.DefaultDBContext(), ctx.Repo.Repository.ID, form.TemplateName); err != nil { + if err := models.InitializeLabels(models.DefaultDBContext(), ctx.Repo.Repository.ID, form.TemplateName); err != nil { if models.IsErrIssueLabelTemplateLoad(err) { originalErr := err.(models.ErrIssueLabelTemplateLoad).OriginalError ctx.Flash.Error(ctx.Tr("repo.issues.label_templates.fail_to_load_file", form.TemplateName, originalErr)) ctx.Redirect(ctx.Repo.RepoLink + "/labels") return } - ctx.ServerError("InitalizeLabels", err) + ctx.ServerError("InitializeLabels", err) return } ctx.Redirect(ctx.Repo.RepoLink + "/labels") diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 35c700a765..ad2d2cca95 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -5317,6 +5317,9 @@ "responses": { "201": { "$ref": "#/responses/Label" + }, + "422": { + "$ref": "#/responses/validationError" } } } @@ -5443,6 +5446,9 @@ "responses": { "200": { "$ref": "#/responses/Label" + }, + "422": { + "$ref": "#/responses/validationError" } } }