Skip to content

Let API create and edit system webhooks #26418

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Let API create and edit system webhooks #26418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d6a9a0
Let API create and edit system webhooks
kousu Feb 25, 2023
606e5da
Merge branch 'main' into system-webhooks-api-2
kousu Sep 18, 2023
0e251f6
Merge branch 'main' into system-webhooks-api-2
kousu Oct 2, 2023
6d1b6fa
Merge branch 'main' into system-webhooks-api-2
kousu Oct 19, 2023
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
39 changes: 23 additions & 16 deletions models/webhook/webhook_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,11 @@ import (
"code.gitea.io/gitea/modules/util"
)

// GetDefaultWebhooks returns all admin-default webhooks.
func GetDefaultWebhooks(ctx context.Context) ([]*Webhook, error) {
webhooks := make([]*Webhook, 0, 5)
return webhooks, db.GetEngine(ctx).
Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, false).
Find(&webhooks)
}

// GetSystemOrDefaultWebhook returns admin system or default webhook by given ID.
func GetSystemOrDefaultWebhook(ctx context.Context, id int64) (*Webhook, error) {
// GetSystemWebhook returns admin default webhook by given ID.
func GetAdminWebhook(ctx context.Context, id int64, isSystemWebhook bool) (*Webhook, error) {
webhook := &Webhook{ID: id}
has, err := db.GetEngine(ctx).
Where("repo_id=? AND owner_id=?", 0, 0).
Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, isSystemWebhook).
Get(webhook)
if err != nil {
return nil, err
Expand All @@ -35,22 +27,37 @@ func GetSystemOrDefaultWebhook(ctx context.Context, id int64) (*Webhook, error)

// GetSystemWebhooks returns all admin system webhooks.
func GetSystemWebhooks(ctx context.Context, isActive util.OptionalBool) ([]*Webhook, error) {
return GetAdminWebhooks(ctx, true, isActive)
}

// GetDefaultWebhooks returns all webhooks that are copied to new repos.
func GetDefaultWebhooks(ctx context.Context) ([]*Webhook, error) {
return GetAdminWebhooks(ctx, false, util.OptionalBoolNone)
}

// returns all admin system or default webhooks.
// isSystemWebhook == true gives system webhooks, otherwise gives default webhooks.
// isActive filters system webhooks to those currently enabled or disabled; pass util.OptionalBoolNone to get both.
func GetAdminWebhooks(ctx context.Context, isSystemWebhook bool, isActive util.OptionalBool) ([]*Webhook, error) {
if !isSystemWebhook && isActive.IsTrue() {
return nil, fmt.Errorf("GetAdminWebhooks: active (isActive) default (!isSystemWebhook) hooks are impossible")
}
webhooks := make([]*Webhook, 0, 5)
if isActive.IsNone() {
return webhooks, db.GetEngine(ctx).
Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, true).
Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, isSystemWebhook).
Find(&webhooks)
}
return webhooks, db.GetEngine(ctx).
Where("repo_id=? AND owner_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, true, isActive.IsTrue()).
Where("repo_id=? AND owner_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, isSystemWebhook, isActive.IsTrue()).
Find(&webhooks)
}

// DeleteDefaultSystemWebhook deletes an admin-configured default or system webhook (where Org and Repo ID both 0)
func DeleteDefaultSystemWebhook(ctx context.Context, id int64) error {
// DeleteWebhook deletes an admin-configured default or system webhook (where Org and Repo ID both 0)
func DeleteAdminWebhook(ctx context.Context, id int64, isSystemWebhook bool) error {
return db.WithTx(ctx, func(ctx context.Context) error {
count, err := db.GetEngine(ctx).
Where("repo_id=? AND owner_id=?", 0, 0).
Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, isSystemWebhook).
Delete(&Webhook{ID: id})
if err != nil {
return err
Expand Down
90 changes: 62 additions & 28 deletions routers/api/v1/admin/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,33 +17,34 @@ import (
webhook_service "code.gitea.io/gitea/services/webhook"
)

// ListHooks list system's webhooks
// list system or default webhooks
func ListHooks(ctx *context.APIContext) {
// swagger:operation GET /admin/hooks admin adminListHooks
// swagger:operation GET /admin/hooks/{configType} admin adminListHooks
// ---
// summary: List system's webhooks
// produces:
// - application/json
// parameters:
// - name: page
// in: query
// description: page number of results to return (1-based)
// type: integer
// - name: limit
// in: query
// description: page size of results
// type: integer
// - name: configType
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a break change and with no deprecated mark?

// in: path
// description: whether the hook is system-wide or copied-to-each-new-repo
// type: string
// enum: [system, default]
// required: true
// responses:
// "200":
// "$ref": "#/responses/HookList"

sysHooks, err := webhook.GetSystemWebhooks(ctx, util.OptionalBoolNone)
isSystemWebhook := ctx.Params(":configType") == "system"

adminHooks, err := webhook.GetAdminWebhooks(ctx, isSystemWebhook, util.OptionalBoolNone)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetSystemWebhooks", err)
ctx.Error(http.StatusInternalServerError, "GetAdminWebhooks", err)
return
}
hooks := make([]*api.Hook, len(sysHooks))
for i, hook := range sysHooks {

hooks := make([]*api.Hook, len(adminHooks))
for i, hook := range adminHooks {
h, err := webhook_service.ToHook(setting.AppURL+"/admin", hook)
if err != nil {
ctx.Error(http.StatusInternalServerError, "convert.ToHook", err)
Expand All @@ -54,14 +55,20 @@ func ListHooks(ctx *context.APIContext) {
ctx.JSON(http.StatusOK, hooks)
}

// GetHook get an organization's hook by id
// get a system/default hook by id
func GetHook(ctx *context.APIContext) {
// swagger:operation GET /admin/hooks/{id} admin adminGetHook
// swagger:operation GET /admin/hooks/{configType}/{id} admin adminGetHook
// ---
// summary: Get a hook
// produces:
// - application/json
// parameters:
// - name: configType
// in: path
// description: whether the hook is system-wide or copied-to-each-new-repo
// type: string
// enum: [system, default]
// required: true
// - name: id
// in: path
// description: id of the hook to get
Expand All @@ -72,16 +79,19 @@ func GetHook(ctx *context.APIContext) {
// "200":
// "$ref": "#/responses/Hook"

isSystemWebhook := ctx.Params(":configType") == "system"

hookID := ctx.ParamsInt64(":id")
hook, err := webhook.GetSystemOrDefaultWebhook(ctx, hookID)
hook, err := webhook.GetAdminWebhook(ctx, hookID, isSystemWebhook)
if err != nil {
if errors.Is(err, util.ErrNotExist) {
ctx.NotFound()
} else {
ctx.Error(http.StatusInternalServerError, "GetSystemOrDefaultWebhook", err)
ctx.Error(http.StatusInternalServerError, "GetAdminWebhook", err)
}
return
}

h, err := webhook_service.ToHook("/admin/", hook)
if err != nil {
ctx.Error(http.StatusInternalServerError, "convert.ToHook", err)
Expand All @@ -90,16 +100,22 @@ func GetHook(ctx *context.APIContext) {
ctx.JSON(http.StatusOK, h)
}

// CreateHook create a hook for an organization
// create a system or default hook
func CreateHook(ctx *context.APIContext) {
// swagger:operation POST /admin/hooks admin adminCreateHook
// swagger:operation POST /admin/hooks/{configType} admin adminCreateHook
// ---
// summary: Create a hook
// consumes:
// - application/json
// produces:
// - application/json
// parameters:
// - name: configType
// in: path
// description: whether the hook is system-wide or copied-to-each-new-repo
// type: string
// enum: [system, default]
// required: true
// - name: body
// in: body
// required: true
Expand All @@ -109,21 +125,29 @@ func CreateHook(ctx *context.APIContext) {
// "201":
// "$ref": "#/responses/Hook"

isSystemWebhook := ctx.Params(":configType") == "system"

form := web.GetForm(ctx).(*api.CreateHookOption)

utils.AddSystemHook(ctx, form)
utils.AddAdminHook(ctx, form, isSystemWebhook)
}

// EditHook modify a hook of a repository
// modify a system or default hook
func EditHook(ctx *context.APIContext) {
// swagger:operation PATCH /admin/hooks/{id} admin adminEditHook
// swagger:operation PATCH /admin/hooks/{configType}/{id} admin adminEditHook
// ---
// summary: Update a hook
// consumes:
// - application/json
// produces:
// - application/json
// parameters:
// - name: configType
// in: path
// description: whether the hook is system-wide or copied-to-each-new-repo
// type: string
// enum: [system, default]
// required: true
// - name: id
// in: path
// description: id of the hook to update
Expand All @@ -138,21 +162,29 @@ func EditHook(ctx *context.APIContext) {
// "200":
// "$ref": "#/responses/Hook"

isSystemWebhook := ctx.Params(":configType") == "system"

form := web.GetForm(ctx).(*api.EditHookOption)

// TODO in body params
hookID := ctx.ParamsInt64(":id")
utils.EditSystemHook(ctx, form, hookID)
utils.EditAdminHook(ctx, form, hookID, isSystemWebhook)
}

// DeleteHook delete a system hook
// delete a system or default hook
func DeleteHook(ctx *context.APIContext) {
// swagger:operation DELETE /admin/hooks/{id} admin adminDeleteHook
// swagger:operation DELETE /admin/hooks/{configType}/{id} admin adminDeleteHook
// ---
// summary: Delete a hook
// produces:
// - application/json
// parameters:
// - name: configType
// in: path
// description: whether the hook is system-wide or copied-to-each-new-repo
// type: string
// enum: [system, default]
// required: true
// - name: id
// in: path
// description: id of the hook to delete
Expand All @@ -163,12 +195,14 @@ func DeleteHook(ctx *context.APIContext) {
// "204":
// "$ref": "#/responses/empty"

isSystemWebhook := ctx.Params(":configType") == "system"

hookID := ctx.ParamsInt64(":id")
if err := webhook.DeleteDefaultSystemWebhook(ctx, hookID); err != nil {
if err := webhook.DeleteAdminWebhook(ctx, hookID, isSystemWebhook); err != nil {
if errors.Is(err, util.ErrNotExist) {
ctx.NotFound()
} else {
ctx.Error(http.StatusInternalServerError, "DeleteDefaultSystemWebhook", err)
ctx.Error(http.StatusInternalServerError, "DeleteAdminWebhook", err)
}
return
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,7 +1520,7 @@ func Routes() *web.Route {
m.Post("/{username}/{reponame}", admin.AdoptRepository)
m.Delete("/{username}/{reponame}", admin.DeleteUnadoptedRepository)
})
m.Group("/hooks", func() {
m.Group("/hooks/{configType:system|default}", func() {
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 is the core change. Almost everything else is fall-out from making this one line change.

Copy link
Contributor Author

@kousu kousu Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In #23142 (comment), lunny suggested what I added as /api/v1/admin/hooks/default, but also wanted to keep the old behaviour too.

Respectfully, I think we must not. The old behaviour was inconsistent:

For "backwards compatibility", we could chose to keep the POST /hooks == POST /hooks/default, but then we would have to break GET /hooks; we could choose to keep GET /hooks == GET /hooks/system, but then we would have to break POST /hooks.

The confused behaviour was in the original version of /api/v1/admin/hooks, so I doubt anyone is using it in the wild -- I was probably the first person to try and immediately hit this bug. Anyone else who tried would have hit it too.

m.Combo("").Get(admin.ListHooks).
Post(bind(api.CreateHookOption{}), admin.CreateHook)
m.Combo("/{id}").Get(admin.GetHook).
Expand Down
59 changes: 38 additions & 21 deletions routers/api/v1/utils/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package utils

import (
"errors"
"fmt"
"net/http"
"strings"
Expand Down Expand Up @@ -100,9 +101,9 @@ func checkCreateHookOption(ctx *context.APIContext, form *api.CreateHookOption)
return true
}

// AddSystemHook add a system hook
func AddSystemHook(ctx *context.APIContext, form *api.CreateHookOption) {
hook, ok := addHook(ctx, form, 0, 0)
// add a system or default hook
func AddAdminHook(ctx *context.APIContext, form *api.CreateHookOption, isSystemWebhook bool) {
hook, ok := addHook(ctx, form, 0, 0, isSystemWebhook)
if ok {
h, err := webhook_service.ToHook(setting.AppSubURL+"/admin", hook)
if err != nil {
Expand All @@ -115,7 +116,7 @@ func AddSystemHook(ctx *context.APIContext, form *api.CreateHookOption) {

// AddOwnerHook adds a hook to an user or organization
func AddOwnerHook(ctx *context.APIContext, owner *user_model.User, form *api.CreateHookOption) {
hook, ok := addHook(ctx, form, owner.ID, 0)
hook, ok := addHook(ctx, form, owner.ID, 0, false)
if !ok {
return
}
Expand All @@ -129,7 +130,7 @@ func AddOwnerHook(ctx *context.APIContext, owner *user_model.User, form *api.Cre
// AddRepoHook add a hook to a repo. Writes to `ctx` accordingly
func AddRepoHook(ctx *context.APIContext, form *api.CreateHookOption) {
repo := ctx.Repo
hook, ok := addHook(ctx, form, 0, repo.Repository.ID)
hook, ok := addHook(ctx, form, 0, repo.Repository.ID, false)
if !ok {
return
}
Expand Down Expand Up @@ -159,9 +160,18 @@ func pullHook(events []string, event string) bool {
return util.SliceContainsString(events, event, true) || util.SliceContainsString(events, string(webhook_module.HookEventPullRequest), true)
}

// addHook add the hook specified by `form`, `ownerID` and `repoID`. If there is
// an error, write to `ctx` accordingly. Return (webhook, ok)
func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoID int64) (*webhook.Webhook, bool) {
// addHook add the hook specified by `form`, `ownerID`, `repoID`, and `isSystemWebhook`.
// `isSystemWebhook` == true means it's a hook attached automatically to all repos
// `isSystemWebhook` == false && ownerID == 0 && repoID == 0 means it's a default hook, automatically copied to all new repos
// `ownerID` != 0 means it runs on all their repos
// `repoID` != 0 means it is an active hook, attached to that repo
// If there is an error, write to `ctx` accordingly. Return (webhook, ok)
func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoID int64, isSystemWebhook bool) (*webhook.Webhook, bool) {
if isSystemWebhook && (ownerID != 0 || repoID != 0) {
ctx.Error(http.StatusInternalServerError, "addHook", fmt.Errorf("cannot create a hook with an owner or repo that is also a system hook"))
return nil, false
}

if !checkCreateHookOption(ctx, form) {
return nil, false
}
Expand All @@ -170,12 +180,13 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI
form.Events = []string{"push"}
}
w := &webhook.Webhook{
OwnerID: ownerID,
RepoID: repoID,
URL: form.Config["url"],
ContentType: webhook.ToHookContentType(form.Config["content_type"]),
Secret: form.Config["secret"],
HTTPMethod: "POST",
OwnerID: ownerID,
RepoID: repoID,
URL: form.Config["url"],
ContentType: webhook.ToHookContentType(form.Config["content_type"]),
Secret: form.Config["secret"],
IsSystemWebhook: isSystemWebhook,
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 line is what communicates the one line change above into the backend.

HTTPMethod: "POST",
HookEvent: &webhook_module.HookEvent{
ChooseEvents: true,
HookEvents: webhook_module.HookEvents{
Expand Down Expand Up @@ -246,22 +257,28 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI
return w, true
}

// EditSystemHook edit system webhook `w` according to `form`. Writes to `ctx` accordingly
func EditSystemHook(ctx *context.APIContext, form *api.EditHookOption, hookID int64) {
hook, err := webhook.GetSystemOrDefaultWebhook(ctx, hookID)
// EditAdminHook edits system/default webhook `w` according to `form`. Writes to `ctx` accordingly.
func EditAdminHook(ctx *context.APIContext, form *api.EditHookOption, hookID int64, isSystemWebhook bool) {
hook, err := webhook.GetAdminWebhook(ctx, hookID, isSystemWebhook)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetSystemOrDefaultWebhook", err)
if errors.Is(err, util.ErrNotExist) {
ctx.NotFound()
Comment on lines +264 to +265
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 is what adds the correct 404 to the PATCH method.

} else {
ctx.Error(http.StatusInternalServerError, "GetAdminWebhook", err)
}
return
}

if !editHook(ctx, form, hook) {
ctx.Error(http.StatusInternalServerError, "editHook", err)
return
}
updated, err := webhook.GetSystemOrDefaultWebhook(ctx, hookID)

updated, err := webhook.GetAdminWebhook(ctx, hookID, isSystemWebhook)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetSystemOrDefaultWebhook", err)
ctx.Error(http.StatusInternalServerError, "GetAdminWebhook", err)
return
}

h, err := webhook_service.ToHook(setting.AppURL+"/admin", updated)
if err != nil {
ctx.Error(http.StatusInternalServerError, "convert.ToHook", err)
Expand Down
Loading