Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement webhook branch filter #7791

Merged
merged 4 commits into from
Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ require (
github.com/go-sql-driver/mysql v1.4.1
github.com/go-swagger/go-swagger v0.20.1
github.com/go-xorm/xorm v0.7.7-0.20190822154023-17592d96b35b
github.com/gobwas/glob v0.2.3
github.com/gogits/chardet v0.0.0-20150115103509-2404f7772561
github.com/gogs/cron v0.0.0-20171120032916-9f6c956d3e14
github.com/google/go-github/v24 v24.0.1
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ github.com/go-xorm/sqlfiddle v0.0.0-20180821085327-62ce714f951a/go.mod h1:56xuuq
github.com/go-xorm/xorm v0.7.6/go.mod h1:nqz2TAsuOHWH2yk4FYWtacCGgdbrcdZ5mF1XadqEHls=
github.com/go-xorm/xorm v0.7.7-0.20190822154023-17592d96b35b h1:Y0hWUheXDHpIs7BWtJcykO4d1VOsVDKg1PsP5YJwxxM=
github.com/go-xorm/xorm v0.7.7-0.20190822154023-17592d96b35b/go.mod h1:nqz2TAsuOHWH2yk4FYWtacCGgdbrcdZ5mF1XadqEHls=
github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y=
github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8=
github.com/gogits/chardet v0.0.0-20150115103509-2404f7772561 h1:deE7ritpK04PgtpyVOS2TYcQEld9qLCD5b5EbVNOuLA=
github.com/gogits/chardet v0.0.0-20150115103509-2404f7772561/go.mod h1:YgYOrVn3Nj9Tq0EvjmFbphRytDj7JNRoWSStJZWDJTQ=
github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
Expand Down
7 changes: 7 additions & 0 deletions models/fixtures/webhook.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,10 @@
content_type: 1 # json
events: '{"push_only":false,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":true}}'
is_active: true
-
id: 4
repo_id: 2
url: www.example.com/url4
content_type: 1 # json
events: '{"push_only":true,"branch_filter":"{master,feature*}"}'
is_active: true
52 changes: 49 additions & 3 deletions models/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ import (
"strings"
"time"

"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/sync"
"code.gitea.io/gitea/modules/timeutil"

"github.com/gobwas/glob"
gouuid "github.com/satori/go.uuid"
"github.com/unknwon/com"
)
Expand Down Expand Up @@ -84,9 +86,10 @@ type HookEvents struct {

// HookEvent represents events that will delivery hook.
type HookEvent struct {
PushOnly bool `json:"push_only"`
SendEverything bool `json:"send_everything"`
ChooseEvents bool `json:"choose_events"`
PushOnly bool `json:"push_only"`
SendEverything bool `json:"send_everything"`
ChooseEvents bool `json:"choose_events"`
BranchFilter string `json:"branch_filter"`

HookEvents `json:"events"`
}
Expand Down Expand Up @@ -256,6 +259,21 @@ func (w *Webhook) EventsArray() []string {
return events
}

func (w *Webhook) checkBranch(branch string) bool {
if w.BranchFilter == "" || w.BranchFilter == "*" {
return true
}

g, err := glob.Compile(w.BranchFilter)
if err != nil {
// should not really happen as BranchFilter is validated
log.Error("CheckBranch failed: %s", err)
return false
}

return g.Match(branch)
}

// CreateWebhook creates a new web hook.
func CreateWebhook(w *Webhook) error {
return createWebhook(x, w)
Expand Down Expand Up @@ -651,6 +669,25 @@ func PrepareWebhook(w *Webhook, repo *Repository, event HookEventType, p api.Pay
return prepareWebhook(x, w, repo, event, p)
}

// getPayloadBranch returns branch for hook event, if applicable.
func getPayloadBranch(p api.Payloader) string {
switch pp := p.(type) {
case *api.CreatePayload:
if pp.RefType == "branch" {
return pp.Ref
}
case *api.DeletePayload:
if pp.RefType == "branch" {
return pp.Ref
}
case *api.PushPayload:
if strings.HasPrefix(pp.Ref, git.BranchPrefix) {
return pp.Ref[len(git.BranchPrefix):]
}
}
return ""
}

func prepareWebhook(e Engine, w *Webhook, repo *Repository, event HookEventType, p api.Payloader) error {
for _, e := range w.eventCheckers() {
if event == e.typ {
Expand All @@ -660,6 +697,15 @@ func prepareWebhook(e Engine, w *Webhook, repo *Repository, event HookEventType,
}
}

// If payload has no associated branch (e.g. it's a new tag, issue, etc.),
// branch filter has no effect.
if branch := getPayloadBranch(p); branch != "" {
if !w.checkBranch(branch) {
log.Info("Branch %q doesn't match branch filter %q, skipping", branch, w.BranchFilter)
return nil
}
}

var payloader api.Payloader
var err error
// Use separate objects so modifications won't be made on payload on non-Gogs/Gitea type hooks.
Expand Down
34 changes: 34 additions & 0 deletions models/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,40 @@ func TestPrepareWebhooks(t *testing.T) {
}
}

func TestPrepareWebhooksBranchFilterMatch(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

repo := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
hookTasks := []*HookTask{
{RepoID: repo.ID, HookID: 4, EventType: HookEventPush},
}
for _, hookTask := range hookTasks {
AssertNotExistsBean(t, hookTask)
}
// this test also ensures that * doesn't handle / in any special way (like shell would)
assert.NoError(t, PrepareWebhooks(repo, HookEventPush, &api.PushPayload{Ref: "refs/heads/feature/7791"}))
for _, hookTask := range hookTasks {
AssertExistsAndLoadBean(t, hookTask)
}
}

func TestPrepareWebhooksBranchFilterNoMatch(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

repo := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
hookTasks := []*HookTask{
{RepoID: repo.ID, HookID: 4, EventType: HookEventPush},
}
for _, hookTask := range hookTasks {
AssertNotExistsBean(t, hookTask)
}
assert.NoError(t, PrepareWebhooks(repo, HookEventPush, &api.PushPayload{Ref: "refs/heads/fix_weird_bug"}))

for _, hookTask := range hookTasks {
AssertNotExistsBean(t, hookTask)
}
}

// TODO TestHookTask_deliver

// TODO TestDeliverHooks
17 changes: 8 additions & 9 deletions modules/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,10 @@ func validate(errs binding.Errors, data map[string]interface{}, f Form, l macaro
}

data["HasError"] = true
// If the field with name errs[0].FieldNames[0] is not found in form
// somehow, some code later on will panic on Data["ErrorMsg"].(string).
// So initialize it to some default.
data["ErrorMsg"] = l.Tr("form.unknown_error")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not really needed for my pull request. Before I made changes below, the code was unable to find the field that failed validation inside embedded anonymous struct, and I got panic due ErrorMsg being not set. I decided to add this so the code won't crash in similar cases (which should not happen unless there's a bug).

In my opinion, the entire function is somewhat sloppy and needs rewriting. I only modified it as much as I needed, and didn't try to refactor it.

AssignForm(f, data)

typ := reflect.TypeOf(f)
Expand All @@ -320,16 +324,9 @@ func validate(errs binding.Errors, data map[string]interface{}, f Form, l macaro
val = val.Elem()
}

for i := 0; i < typ.NumField(); i++ {
field := typ.Field(i)

if field, ok := typ.FieldByName(errs[0].FieldNames[0]); ok {
fieldName := field.Tag.Get("form")
// Allow ignored fields in the struct
if fieldName == "-" {
continue
}

if errs[0].FieldNames[0] == field.Name {
if fieldName != "-" {
data["Err_"+field.Name] = true

trName := field.Tag.Get("locale")
Expand Down Expand Up @@ -360,6 +357,8 @@ func validate(errs binding.Errors, data map[string]interface{}, f Form, l macaro
data["ErrorMsg"] = trName + l.Tr("form.url_error")
case binding.ERR_INCLUDE:
data["ErrorMsg"] = trName + l.Tr("form.include_error", GetInclude(field))
case validation.ErrGlobPattern:
data["ErrorMsg"] = trName + l.Tr("form.glob_pattern_error", errs[0].Message)
default:
data["ErrorMsg"] = l.Tr("form.unknown_error") + " " + errs[0].Classification
}
Expand Down
1 change: 1 addition & 0 deletions modules/auth/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ type WebhookForm struct {
PullRequest bool
Repository bool
Active bool
BranchFilter string `binding:"GlobPattern"`
}

// PushOnly if the hook will be triggered when push
Expand Down
12 changes: 7 additions & 5 deletions modules/structs/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,19 @@ type CreateHookOption struct {
// enum: gitea,gogs,slack,discord
Type string `json:"type" binding:"Required"`
// required: true
Config map[string]string `json:"config" binding:"Required"`
Events []string `json:"events"`
Config map[string]string `json:"config" binding:"Required"`
Events []string `json:"events"`
BranchFilter string `json:"branch_filter" binding:"GlobPattern"`
// default: false
Active bool `json:"active"`
}

// EditHookOption options when modify one hook
type EditHookOption struct {
Config map[string]string `json:"config"`
Events []string `json:"events"`
Active *bool `json:"active"`
Config map[string]string `json:"config"`
Events []string `json:"events"`
BranchFilter string `json:"branch_filter" binding:"GlobPattern"`
Active *bool `json:"active"`
}

// Payloader payload is some part of one hook
Expand Down
25 changes: 25 additions & 0 deletions modules/validation/binding.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@ import (
"strings"

"gitea.com/macaron/binding"
"github.com/gobwas/glob"
)

const (
// ErrGitRefName is git reference name error
ErrGitRefName = "GitRefNameError"

// ErrGlobPattern is returned when glob pattern is invalid
ErrGlobPattern = "GlobPattern"
)

var (
Expand All @@ -28,6 +32,7 @@ var (
func AddBindingRules() {
addGitRefNameBindingRule()
addValidURLBindingRule()
addGlobPatternRule()
}

func addGitRefNameBindingRule() {
Expand Down Expand Up @@ -82,6 +87,26 @@ func addValidURLBindingRule() {
})
}

func addGlobPatternRule() {
binding.AddRule(&binding.Rule{
IsMatch: func(rule string) bool {
return rule == "GlobPattern"
},
IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) {
str := fmt.Sprintf("%v", val)

if len(str) != 0 {
if _, err := glob.Compile(str); err != nil {
errs.Add([]string{name}, ErrGlobPattern, err.Error())
return false, errs
}
}

return true, errs
},
})
}

func portOnly(hostport string) string {
colon := strings.IndexByte(hostport, ':')
if colon == -1 {
Expand Down
5 changes: 3 additions & 2 deletions modules/validation/binding_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ type (
}

TestForm struct {
BranchName string `form:"BranchName" binding:"GitRefName"`
URL string `form:"ValidUrl" binding:"ValidUrl"`
BranchName string `form:"BranchName" binding:"GitRefName"`
URL string `form:"ValidUrl" binding:"ValidUrl"`
GlobPattern string `form:"GlobPattern" binding:"GlobPattern"`
}
)

Expand Down
62 changes: 62 additions & 0 deletions modules/validation/glob_pattern_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package validation
WGH- marked this conversation as resolved.
Show resolved Hide resolved

import (
"testing"

"gitea.com/macaron/binding"
"github.com/gobwas/glob"
)

func getGlobPatternErrorString(pattern string) string {
// It would be unwise to rely on that glob
// compilation errors don't ever change.
if _, err := glob.Compile(pattern); err != nil {
return err.Error()
}
return ""
}

var globValidationTestCases = []validationTestCase{
{
description: "Empty glob pattern",
data: TestForm{
GlobPattern: "",
},
expectedErrors: binding.Errors{},
},
{
description: "Valid glob",
data: TestForm{
GlobPattern: "{master,release*}",
},
expectedErrors: binding.Errors{},
},

{
description: "Invalid glob",
data: TestForm{
GlobPattern: "[a-",
},
expectedErrors: binding.Errors{
binding.Error{
FieldNames: []string{"GlobPattern"},
Classification: ErrGlobPattern,
Message: getGlobPatternErrorString("[a-"),
},
},
},
}

func Test_GlobPatternValidation(t *testing.T) {
AddBindingRules()

for _, testCase := range globValidationTestCases {
t.Run(testCase.description, func(t *testing.T) {
performValidationTest(t, testCase)
})
}
}
3 changes: 3 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ max_size_error = ` must contain at most %s characters.`
email_error = ` is not a valid email address.`
url_error = ` is not a valid URL.`
include_error = ` must contain substring '%s'.`
glob_pattern_error = ` glob pattern is invalid: %s.`
unknown_error = Unknown error:
captcha_incorrect = The CAPTCHA code is incorrect.
password_not_match = The passwords do not match.
Expand Down Expand Up @@ -1258,6 +1259,8 @@ settings.event_pull_request = Pull Request
settings.event_pull_request_desc = Pull request opened, closed, reopened, edited, approved, rejected, review comment, assigned, unassigned, label updated, label cleared or synchronized.
settings.event_push = Push
settings.event_push_desc = Git push to a repository.
settings.branch_filter = Branch filter
settings.branch_filter_desc = Branch whitelist for push, branch creation and branch deletion events, specified as glob pattern. If empty or <code>*</code>, events for all branches are reported. See <a href="https://godoc.org/github.com/gobwas/glob#Compile">github.com/gobwas/glob</a> documentation for syntax. Examples: <code>master</code>, <code>{master,release*}</code>.
settings.event_repository = Repository
settings.event_repository_desc = Repository created or deleted.
settings.active = Active
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/utils/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, orgID, repoID
Repository: com.IsSliceContainsStr(form.Events, string(models.HookEventRepository)),
Release: com.IsSliceContainsStr(form.Events, string(models.HookEventRelease)),
},
BranchFilter: form.BranchFilter,
},
IsActive: form.Active,
HookTaskType: models.ToHookTaskType(form.Type),
Expand Down Expand Up @@ -236,6 +237,7 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *models.Webho
w.PullRequest = com.IsSliceContainsStr(form.Events, string(models.HookEventPullRequest))
w.Repository = com.IsSliceContainsStr(form.Events, string(models.HookEventRepository))
w.Release = com.IsSliceContainsStr(form.Events, string(models.HookEventRelease))
w.BranchFilter = form.BranchFilter

if err := w.UpdateEvent(); err != nil {
ctx.Error(500, "UpdateEvent", err)
Expand Down
Loading