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

Refactor Webhook + Add X-Hub-Signature #16176

Merged
merged 10 commits into from
Jun 27, 2021
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,8 @@ var migrations = []Migration{
NewMigration("Add issue resource index table", addIssueResourceIndexTable),
// v183 -> v184
NewMigration("Create PushMirror table", createPushMirrorTable),
// v184 -> v185
NewMigration("Drop unneeded webhook related columns", dropWebhookColumns),
}

// GetCurrentDBVersion returns the current db version
Expand Down
46 changes: 46 additions & 0 deletions models/migrations/v184.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2021 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 migrations

import (
"xorm.io/xorm"
)

func dropWebhookColumns(x *xorm.Engine) error {
// Make sure the columns exist before dropping them
type Webhook struct {
Signature string `xorm:"TEXT"`
IsSSL bool `xorm:"is_ssl"`
}
if err := x.Sync2(new(Webhook)); err != nil {
return err
}

type HookTask struct {
Typ string `xorm:"VARCHAR(16) index"`
URL string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`
HTTPMethod string `xorm:"http_method"`
ContentType int
IsSSL bool
}
if err := x.Sync2(new(HookTask)); err != nil {
return err
}

sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return err
}
if err := dropTableColumns(sess, "webhook", "signature", "is_ssl"); err != nil {
return err
}
if err := dropTableColumns(sess, "hook_task", "typ", "url", "signature", "http_method", "content_type", "is_ssl"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

don't know why this columns should be dropped? webhook maybe changed after hook_task finished. So these columns will keep the original information when requesting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Webhook:
Signature and IsSSL are never used

HookTask:
Typ see #16176 (comment)
URL moved to HookRequest
Signature calculated on delivery. Is saved indirect in the Header map in HookRequest
HTTPMethod moved to HookRequest
ContentType of the Webhook is used on delivery. If the Webhook gets changed before sending the HookTask will use the current/correct settings. The original value is not available after a change but it's not visible on the UI anyway.
IsSSL is never used

return err
}

return sess.Commit()
}
52 changes: 23 additions & 29 deletions models/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,22 @@ type HookEvent struct {
HookEvents `json:"events"`
}

// HookType is the type of a webhook
type HookType = string

// Types of webhooks
const (
GITEA HookType = "gitea"
GOGS HookType = "gogs"
SLACK HookType = "slack"
DISCORD HookType = "discord"
DINGTALK HookType = "dingtalk"
TELEGRAM HookType = "telegram"
MSTEAMS HookType = "msteams"
FEISHU HookType = "feishu"
MATRIX HookType = "matrix"
)

// HookStatus is the status of a web hook
type HookStatus int

Expand All @@ -126,17 +142,15 @@ type Webhook struct {
OrgID int64 `xorm:"INDEX"`
IsSystemWebhook bool
URL string `xorm:"url TEXT"`
Signature string `xorm:"TEXT"`
HTTPMethod string `xorm:"http_method"`
ContentType HookContentType
Secret string `xorm:"TEXT"`
Events string `xorm:"TEXT"`
*HookEvent `xorm:"-"`
IsSSL bool `xorm:"is_ssl"`
IsActive bool `xorm:"INDEX"`
Type HookTaskType `xorm:"VARCHAR(16) 'type'"`
Meta string `xorm:"TEXT"` // store hook-specific attributes
LastStatus HookStatus // Last delivery status
IsActive bool `xorm:"INDEX"`
Type HookType `xorm:"VARCHAR(16) 'type'"`
Meta string `xorm:"TEXT"` // store hook-specific attributes
LastStatus HookStatus // Last delivery status

CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
Expand Down Expand Up @@ -558,22 +572,6 @@ func copyDefaultWebhooksToRepo(e Engine, repoID int64) error {
// \___|_ / \____/ \____/|__|_ \ |____| (____ /____ >__|_ \
// \/ \/ \/ \/ \/

// HookTaskType is the type of an hook task
type HookTaskType = string

// Types of hook tasks
const (
GITEA HookTaskType = "gitea"
GOGS HookTaskType = "gogs"
SLACK HookTaskType = "slack"
DISCORD HookTaskType = "discord"
DINGTALK HookTaskType = "dingtalk"
TELEGRAM HookTaskType = "telegram"
MSTEAMS HookTaskType = "msteams"
FEISHU HookTaskType = "feishu"
MATRIX HookTaskType = "matrix"
)

// HookEventType is the type of an hook event
type HookEventType string

Expand Down Expand Up @@ -635,7 +633,9 @@ func (h HookEventType) Event() string {

// HookRequest represents hook task request information.
type HookRequest struct {
Headers map[string]string `json:"headers"`
URL string `json:"url"`
HTTPMethod string `json:"http_method"`
Headers map[string]string `json:"headers"`
}

// HookResponse represents hook task response information.
Expand All @@ -651,15 +651,9 @@ type HookTask struct {
RepoID int64 `xorm:"INDEX"`
HookID int64
UUID string
Typ HookTaskType `xorm:"VARCHAR(16) index"`
lunny marked this conversation as resolved.
Show resolved Hide resolved
URL string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`
api.Payloader `xorm:"-"`
PayloadContent string `xorm:"TEXT"`
HTTPMethod string `xorm:"http_method"`
ContentType HookContentType
EventType HookEventType
IsSSL bool
IsDelivered bool
Delivered int64
DeliveredString string `xorm:"-"`
Expand Down
14 changes: 0 additions & 14 deletions models/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,6 @@ func TestCreateHookTask(t *testing.T) {
hookTask := &HookTask{
RepoID: 3,
HookID: 3,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
}
AssertNotExistsBean(t, hookTask)
Expand All @@ -233,8 +231,6 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) {
hookTask := &HookTask{
RepoID: 3,
HookID: 3,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
IsDelivered: true,
Delivered: time.Now().UnixNano(),
Expand All @@ -252,8 +248,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) {
hookTask := &HookTask{
RepoID: 2,
HookID: 4,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
IsDelivered: false,
}
Expand All @@ -270,8 +264,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) {
hookTask := &HookTask{
RepoID: 2,
HookID: 4,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
IsDelivered: true,
Delivered: time.Now().UnixNano(),
Expand All @@ -289,8 +281,6 @@ func TestCleanupHookTaskTable_OlderThan_DeletesDelivered(t *testing.T) {
hookTask := &HookTask{
RepoID: 3,
HookID: 3,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
IsDelivered: true,
Delivered: time.Now().AddDate(0, 0, -8).UnixNano(),
Expand All @@ -308,8 +298,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesUndelivered(t *testing.T) {
hookTask := &HookTask{
RepoID: 2,
HookID: 4,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
IsDelivered: false,
}
Expand All @@ -326,8 +314,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesTaskEarlierThanAgeToDelete(t *test
hookTask := &HookTask{
RepoID: 2,
HookID: 4,
Typ: GITEA,
URL: "http://www.example.com/unit_test",
Payloader: &api.PushPayload{},
IsDelivered: true,
Delivered: time.Now().AddDate(0, 0, -6).UnixNano(),
Expand Down
55 changes: 0 additions & 55 deletions modules/structs/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ type EditHookOption struct {

// Payloader payload is some part of one hook
type Payloader interface {
SetSecret(string)
JSONPayload() ([]byte, error)
}

Expand Down Expand Up @@ -124,19 +123,13 @@ var (

// CreatePayload FIXME
type CreatePayload struct {
Secret string `json:"secret"`
Sha string `json:"sha"`
Ref string `json:"ref"`
RefType string `json:"ref_type"`
Repo *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the CreatePayload
func (p *CreatePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload return payload information
func (p *CreatePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -181,19 +174,13 @@ const (

// DeletePayload represents delete payload
type DeletePayload struct {
Secret string `json:"secret"`
Ref string `json:"ref"`
RefType string `json:"ref_type"`
PusherType PusherType `json:"pusher_type"`
Repo *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the DeletePayload
func (p *DeletePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *DeletePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -209,17 +196,11 @@ func (p *DeletePayload) JSONPayload() ([]byte, error) {

// ForkPayload represents fork payload
type ForkPayload struct {
Secret string `json:"secret"`
Forkee *Repository `json:"forkee"`
Repo *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the ForkPayload
func (p *ForkPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *ForkPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -238,7 +219,6 @@ const (

// IssueCommentPayload represents a payload information of issue comment event.
type IssueCommentPayload struct {
Secret string `json:"secret"`
Action HookIssueCommentAction `json:"action"`
Issue *Issue `json:"issue"`
Comment *Comment `json:"comment"`
Expand All @@ -248,11 +228,6 @@ type IssueCommentPayload struct {
IsPull bool `json:"is_pull"`
}

// SetSecret modifies the secret of the IssueCommentPayload
func (p *IssueCommentPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *IssueCommentPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -278,18 +253,12 @@ const (

// ReleasePayload represents a payload information of release event.
type ReleasePayload struct {
Secret string `json:"secret"`
Action HookReleaseAction `json:"action"`
Release *Release `json:"release"`
Repository *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the ReleasePayload
func (p *ReleasePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *ReleasePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -305,7 +274,6 @@ func (p *ReleasePayload) JSONPayload() ([]byte, error) {

// PushPayload represents a payload information of push event.
type PushPayload struct {
Secret string `json:"secret"`
Ref string `json:"ref"`
Before string `json:"before"`
After string `json:"after"`
Expand All @@ -317,11 +285,6 @@ type PushPayload struct {
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the PushPayload
func (p *PushPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload FIXME
func (p *PushPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -389,7 +352,6 @@ const (

// IssuePayload represents the payload information that is sent along with an issue event.
type IssuePayload struct {
Secret string `json:"secret"`
Action HookIssueAction `json:"action"`
Index int64 `json:"number"`
Changes *ChangesPayload `json:"changes,omitempty"`
Expand All @@ -398,11 +360,6 @@ type IssuePayload struct {
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the IssuePayload.
func (p *IssuePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces.
func (p *IssuePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -430,7 +387,6 @@ type ChangesPayload struct {

// PullRequestPayload represents a payload information of pull request event.
type PullRequestPayload struct {
Secret string `json:"secret"`
Action HookIssueAction `json:"action"`
Index int64 `json:"number"`
Changes *ChangesPayload `json:"changes,omitempty"`
Expand All @@ -440,11 +396,6 @@ type PullRequestPayload struct {
Review *ReviewPayload `json:"review"`
}

// SetSecret modifies the secret of the PullRequestPayload.
func (p *PullRequestPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload FIXME
func (p *PullRequestPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -476,18 +427,12 @@ const (

// RepositoryPayload payload for repository webhooks
type RepositoryPayload struct {
Secret string `json:"secret"`
Action HookRepoAction `json:"action"`
Repository *Repository `json:"repository"`
Organization *User `json:"organization"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the RepositoryPayload
func (p *RepositoryPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload JSON representation of the payload
func (p *RepositoryPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down
Loading