Allow creating and editing system webhooks. #23142

kousu · 2023-02-25T08:25:48Z

Fixes #23139, hopefully.

Also, allow editing (but not viewing) the secret associated with each hook.

kousu · 2023-02-25T08:27:12Z

This code is not elegant. It fixes the problems I ran into, but I'm sure it does it suboptimally. For one thing, AddSystemHook should be renamed. And maybe there should be separate /admin/hooks/system, /admin/hooks/default routes. And maybe you shouldn't be able to edit whether a hook is a system or default hook. And maybe there should be a POST /admin/hooks/{id}/secret which generates and returns a secret, instead of what I wrote here where the user submits a secret (and hence could easily craft it weakly).

kousu · 2023-02-25T16:45:43Z

Thank you for taking the time to curate this, @yardenshoham; I don't think this is an enhancement though, it is a bug fix to a feature that went in to 1.19 and isn't usable at the moment. I was hoping to get this fixed in 1.19.

yardenshoham · 2023-02-25T16:47:44Z

Labels make sense now?

kousu · 2023-02-25T16:49:28Z

Yes! Thank you :) :)

kousu · 2023-02-27T17:49:01Z

I want to be able to edit secret because I want to automate deployments with terraform/Docker/etc -- that's the main reason I'm using the API and not just giving install instructions for an admin to follow.

If I can edit a secret, I can do:

GET /admin/hooks
if a hook pointing at the expected URL exists: PATCH /admin/hooks/{id}
else: POST /admin/hooks

If I can't edit a secret, I can instead do:

GET /admin/hooks
if a hook pointing at the expected URL exists: DELETE /admin/hooks/{id}
POST /admin/hooks

This will wipe out logs of previous attempts on the hook, which is a little unfortunate, and requires 3 APIs calls then where the other needs 2. It's not the end of the world if you don't want to allow editing the secret; I can think of arguments in favour of that too, like making hooks symmetrical with tokens which also don't allow editing after their creation.

kousu · 2023-03-25T01:22:57Z

models/webhook/webhook_system.go

+// GetSystemOrDefaultWebhooks returns all admin webhooks.
+func GetSystemOrDefaultWebhooks(ctx context.Context, isActive util.OptionalBool) ([]*Webhook, error) {
+	webhooks := make([]*Webhook, 0, 5)
+	if isActive.IsNone() {
+		return webhooks, db.GetEngine(ctx).
+			Where("repo_id=? AND org_id=?", 0, 0).
+			Find(&webhooks)
+	}
+	return webhooks, db.GetEngine(ctx).
+		Where("repo_id=? AND org_id=? AND is_active = ?", 0, 0, isActive.IsTrue()).
+		Find(&webhooks)
+}
+


There's improvements to be made here. For example, if this exists, do we really still need GetSystemWebhooks()? Would you ever use both?

Fixes go-gitea#23139, hopefully. Also, allow creating/editing (but not viewing) the _secret_ associated with each hook.

codecov-commenter · 2023-04-12T13:18:52Z

Codecov Report

Merging #23142 (6fa610c) into main (f521e88) will decrease coverage by 47.15%.
The diff coverage is n/a.

❗ Current head 6fa610c differs from pull request most recent head f2f1bbf. Consider uploading reports for the commit f2f1bbf to get more accurate results

@@             Coverage Diff             @@
##             main   #23142       +/-   ##
===========================================
- Coverage   47.14%        0   -47.15%     
===========================================
  Files        1149        0     -1149     
  Lines      151446        0   -151446     
===========================================
- Hits        71397        0    -71397     
+ Misses      71611        0    -71611     
+ Partials     8438        0     -8438

see 1149 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kousu · 2023-04-24T20:14:24Z

Hi! I hope you're all doing well with working on Actions. Is there any attention available for looking this patch over?

It's a pretty obvious bug and what I think is a straightforward fix.

lunny · 2023-05-05T03:55:55Z

modules/structs/hook.go

@@ -24,6 +24,7 @@ type Hook struct {
 	Events              []string          `json:"events"`
 	AuthorizationHeader string            `json:"authorization_header"`
 	Active              bool              `json:"active"`
+	IsSystemWebhook     bool              `json:"is_system_webhook"`


Although the database stored as the name, but maybe we can have a better name like IsSystemOrDefault? Or even ScopeType?

I've gotten around to this again finally.

I decided to replace IsSystemWebhook by a path parameter hookType:

gitea/routers/api/v1/api.go

Lines 1384 to 1390 in 5384c1d

m.Group("/{hookType:system-hooks|default-hooks}", func() {

m.Combo("").Get(admin.ListHooks).

Post(bind(api.CreateHookOption{}), admin.CreateHook)

m.Combo("/{id}").Get(admin.GetHook).

Patch(bind(api.EditHookOption{}), admin.EditHook).

Delete(admin.DeleteHook)

})

gitea/routers/api/v1/admin/hooks.go

Line 24 in 5384c1d

// swagger:operation GET /admin/{hookType} admin adminListHooks

gitea/routers/api/v1/admin/hooks.go

Line 62 in 5384c1d

// swagger:operation GET /admin/{hookType}/{id} admin adminGetHook

gitea/routers/api/v1/admin/hooks.go

Line 107 in 5384c1d

// swagger:operation POST /admin/{hookType} admin adminCreateHook

gitea/routers/api/v1/admin/hooks.go

Line 139 in 5384c1d

// swagger:operation PATCH /admin/{hookType}/{id} admin adminEditHook

gitea/routers/api/v1/admin/hooks.go

Line 178 in 5384c1d

// swagger:operation DELETE /admin/{hookType}/{id} admin adminDeleteHook

Doing this ballooned the PR, but it makes the API more friendly:

and it also matches how the admin web UI works, where the two links on /admin/system-hooks/

go to /admin/system-hooks/{kind}/new and http://localhost:3000/admin/default-hooks/{kind}/new respectively.

What do you think?

lunny · 2023-05-05T03:57:21Z

routers/api/v1/admin/hooks.go

@@ -36,7 +36,7 @@ func ListHooks(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/HookList"

-	sysHooks, err := webhook.GetSystemWebhooks(ctx, util.OptionalBoolNone)
+	sysHooks, err := webhook.GetSystemOrDefaultWebhooks(ctx, util.OptionalBoolNone)


I don't think we should change the behaviour? Maybe we should have parameter or we could have another endpoint for default webhooks?

kousu · 2023-06-08T17:37:59Z

I'm still interested in this. I started to rewrite some parts to reply to Lunny's feedback -- I want to add separate endpoints for system vs default webhooks rather then pass it as a data field because I think that makes the API easier to understand -- but work got in the way before I got my rewrite building. I do want it to come though!

…/admin/default-hooks This should better address the ambiguity that led to go-gitea#23139. Rename parts of the supporting module to match this naming convention.

kousu · 2023-08-09T21:51:18Z

I've redone this to adopt #23142 (comment) into it, and then cleaned up the API/UI endpoints to match each other. Since that was a pretty extensive change, I've resubmitted this as #26418.

It's still not quite ready but it's coming along.

"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.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 25, 2023

yardenshoham added type/enhancement An improvement of existing functionality modifies/api This PR adds API routes or modifies them labels Feb 25, 2023

yardenshoham added this to the 1.20.0 milestone Feb 25, 2023

yardenshoham added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 and removed type/enhancement An improvement of existing functionality labels Feb 25, 2023

kousu force-pushed the system-webhooks-api branch 2 times, most recently from f031c51 to 3fcf7dd Compare March 25, 2023 01:21

kousu commented Mar 25, 2023

View reviewed changes

Allow creating and editing system webhooks.

66c356c

Fixes go-gitea#23139, hopefully. Also, allow creating/editing (but not viewing) the _secret_ associated with each hook.

kousu force-pushed the system-webhooks-api branch from 3fcf7dd to 66c356c Compare March 28, 2023 18:55

Merge branch 'main' into system-webhooks-api

f2f1bbf

Merge branch 'main' into system-webhooks-api

d9f7774

pull-request-size bot added the size/M label Apr 24, 2023

Merge branch 'main' into system-webhooks-api

a85c3ba

kousu force-pushed the system-webhooks-api branch from d3a7a6d to a85c3ba Compare May 4, 2023 17:39

lunny reviewed May 5, 2023

View reviewed changes

Merge branch 'main' into system-webhooks-api

5bc1f88

delvh removed this from the 1.20.0 milestone Jun 7, 2023

Merge branch 'main' into system-webhooks-api

040cb8b

pull-request-size bot added size/XL and removed size/M labels Jul 2, 2023

Split /api/v1/admin/hooks into /api/v1/admin/system-hooks and /api/v1…

5384c1d

…/admin/default-hooks This should better address the ambiguity that led to go-gitea#23139. Rename parts of the supporting module to match this naming convention.

kousu force-pushed the system-webhooks-api branch from 02db086 to 5384c1d Compare July 2, 2023 03:35

Merge remote-tracking branch 'upstream/main' into system-webhooks-api

2cadf91

kousu mentioned this pull request Aug 3, 2023

Include annexed files in zip and tar.gz downloads neuropoly/gitea#43

Open

4 tasks

kousu mentioned this pull request Aug 9, 2023

Let API create and edit system webhooks #26418

Closed

kousu closed this Aug 9, 2023

go-gitea locked as resolved and limited conversation to collaborators Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creating and editing system webhooks. #23142

Allow creating and editing system webhooks. #23142

kousu commented Feb 25, 2023 •

edited

Loading

kousu commented Feb 25, 2023 •

edited

Loading

kousu commented Feb 25, 2023

yardenshoham commented Feb 25, 2023

kousu commented Feb 25, 2023

kousu commented Feb 27, 2023

kousu Mar 25, 2023

codecov-commenter commented Apr 12, 2023

kousu commented Apr 24, 2023

lunny May 5, 2023

kousu Jul 2, 2023 •

edited

Loading

lunny May 5, 2023

kousu commented Jun 8, 2023 •

edited

Loading

kousu commented Aug 9, 2023 •

edited

Loading

	m.Group("/{hookType:system-hooks\|default-hooks}", func() {
	m.Combo("").Get(admin.ListHooks).
	Post(bind(api.CreateHookOption{}), admin.CreateHook)
	m.Combo("/{id}").Get(admin.GetHook).
	Patch(bind(api.EditHookOption{}), admin.EditHook).
	Delete(admin.DeleteHook)
	})

Allow creating and editing system webhooks. #23142

Allow creating and editing system webhooks. #23142

Conversation

kousu commented Feb 25, 2023 • edited Loading

kousu commented Feb 25, 2023 • edited Loading

kousu commented Feb 25, 2023

yardenshoham commented Feb 25, 2023

kousu commented Feb 25, 2023

kousu commented Feb 27, 2023

kousu Mar 25, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 12, 2023

Codecov Report

kousu commented Apr 24, 2023

lunny May 5, 2023

Choose a reason for hiding this comment

kousu Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

lunny May 5, 2023

Choose a reason for hiding this comment

kousu commented Jun 8, 2023 • edited Loading

kousu commented Aug 9, 2023 • edited Loading

kousu commented Feb 25, 2023 •

edited

Loading

kousu commented Feb 25, 2023 •

edited

Loading

kousu Jul 2, 2023 •

edited

Loading

kousu commented Jun 8, 2023 •

edited

Loading

kousu commented Aug 9, 2023 •

edited

Loading