Let API create and edit system webhooks

"System" webhooks -- webhooks run on all repos on a Gitea instance -- were added in go-gitea#14537 (I believe?) but managing them by the API is buggy. - In routers/api/v1/utils/hook.go, correctly handle the distinction between system and default webhooks. This enables actually creating, editing and deleting both kinds. - In routers/api/, move `/api/v1/admin/hooks` to `/api/v1/admin/hooks/{system,default}`. This allows users to access the code in the previous point. - In routers/web/, move `/admin/{system,default}-hooks` and most of `/admin/hooks/` into `/admin/hooks/{system,default}` to match API. - In model/, normalize vocabulary. Since the new sub-type, the terminology has been a confusing mix of "SystemWebhook", "DefaultSystemWebhook", "SystemOrDefaultWebhook" and "DefaultWebhook". Standardize on "AdminWebhook" everywhere with `isSystemWebhook bool` to separate the two sub-types. - Using a bool made it easier to handle both cases without duplicating the router endpoints - Make PATCH /admin/hooks/{system,default}/:id appropriately return 404. Fixes go-gitea#23139. Supersedes go-gitea#23142.
neuropoly · Aug 11, 2023 · 8d6a9a0 · 8d6a9a0
1 parent 7e382a5
commit 8d6a9a0
Show file tree

Hide file tree

Showing 9 changed files with 212 additions and 109 deletions.
diff --git a/models/webhook/webhook_system.go b/models/webhook/webhook_system.go
@@ -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
@@ -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

diff --git a/routers/api/v1/admin/hooks.go b/routers/api/v1/admin/hooks.go
@@ -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
+	//   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)
@@ -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
@@ -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)
@@ -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
@@ -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
@@ -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
@@ -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
 	}

diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
@@ -1381,7 +1381,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() {
 				m.Combo("").Get(admin.ListHooks).
 					Post(bind(api.CreateHookOption{}), admin.CreateHook)
 				m.Combo("/{id}").Get(admin.GetHook).

diff --git a/routers/api/v1/utils/hook.go b/routers/api/v1/utils/hook.go
@@ -4,6 +4,7 @@
 package utils
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -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 {
@@ -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
 	}
@@ -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
 	}
@@ -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
 	}
@@ -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,
+		HTTPMethod:      "POST",
 		HookEvent: &webhook_module.HookEvent{
 			ChooseEvents: true,
 			HookEvents: webhook_module.HookEvents{
@@ -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()
+		} 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)