Merge pull request '[BUG] webhook: fix admin-hooks and add more tests' (#3125) from oliverpool/forgejo:webhook_admin_fix into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3125
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/webhook/webhook_system.go

@@ -8,15 +8,11 @@ import (
 	"fmt"
 
 	"code.gitea.io/gitea/models/db"
-	"code.gitea.io/gitea/modules/optional"
 )
 
 // GetDefaultWebhooks returns all admin-default webhooks.
 func GetDefaultWebhooks(ctx context.Context) ([]*Webhook, error) {
-	webhooks := make([]*Webhook, 0, 5)
+	return getAdminWebhooks(ctx, false)
-	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.
@@ -34,15 +30,21 @@ func GetSystemOrDefaultWebhook(ctx context.Context, id int64) (*Webhook, error)
 }
 
 // GetSystemWebhooks returns all admin system webhooks.
-func GetSystemWebhooks(ctx context.Context, isActive optional.Option[bool]) ([]*Webhook, error) {
+func GetSystemWebhooks(ctx context.Context, onlyActive bool) ([]*Webhook, error) {
+	return getAdminWebhooks(ctx, true, onlyActive)
+}
+
+func getAdminWebhooks(ctx context.Context, systemWebhooks bool, onlyActive ...bool) ([]*Webhook, error) {
 	webhooks := make([]*Webhook, 0, 5)
-	if !isActive.Has() {
+	if len(onlyActive) > 0 && onlyActive[0] {
 		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=? AND is_active = ?", 0, 0, systemWebhooks, true).
+			OrderBy("id").
 			Find(&webhooks)
 	}
 	return webhooks, db.GetEngine(ctx).
-		Where("repo_id=? AND owner_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, true, isActive.Value()).
+		Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, systemWebhooks).
+		OrderBy("id").
 		Find(&webhooks)
 }
 

routers/api/v1/admin/hooks.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 
 	"code.gitea.io/gitea/models/webhook"
-	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/util"
@@ -38,7 +37,7 @@ func ListHooks(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/HookList"
 
-	sysHooks, err := webhook.GetSystemWebhooks(ctx, optional.None[bool]())
+	sysHooks, err := webhook.GetSystemWebhooks(ctx, false)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "GetSystemWebhooks", err)
 		return

routers/web/admin/hooks.go

@@ -8,9 +8,9 @@ import (
 
 	"code.gitea.io/gitea/models/webhook"
 	"code.gitea.io/gitea/modules/base"
-	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/services/context"
+	webhook_service "code.gitea.io/gitea/services/webhook"
 )
 
 const (
@@ -35,9 +35,10 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
 
 	sys["Title"] = ctx.Tr("admin.systemhooks")
 	sys["Description"] = ctx.Tr("admin.systemhooks.desc")
-	sys["Webhooks"], err = webhook.GetSystemWebhooks(ctx, optional.None[bool]())
+	sys["Webhooks"], err = webhook.GetSystemWebhooks(ctx, false)
 	sys["BaseLink"] = setting.AppSubURL + "/admin/hooks"
 	sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks"
+	sys["WebhookList"] = webhook_service.List()
 	if err != nil {
 		ctx.ServerError("GetWebhooksAdmin", err)
 		return
@@ -48,6 +49,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
 	def["Webhooks"], err = webhook.GetDefaultWebhooks(ctx)
 	def["BaseLink"] = setting.AppSubURL + "/admin/hooks"
 	def["BaseLinkNew"] = setting.AppSubURL + "/admin/default-hooks"
+	def["WebhookList"] = webhook_service.List()
 	if err != nil {
 		ctx.ServerError("GetWebhooksAdmin", err)
 		return

services/webhook/webhook.go

@@ -243,7 +243,7 @@ func PrepareWebhooks(ctx context.Context, source EventSource, event webhook_modu
 	}
 
 	// Add any admin-defined system webhooks
-	systemHooks, err := webhook_model.GetSystemWebhooks(ctx, optional.Some(true))
+	systemHooks, err := webhook_model.GetSystemWebhooks(ctx, true)
 	if err != nil {
 		return fmt.Errorf("GetSystemWebhooks: %w", err)
 	}

tests/integration/repo_webhook_test.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"strings"
 	"testing"
 
 	gitea_context "code.gitea.io/gitea/services/context"
@@ -37,30 +36,58 @@ func TestNewWebHookLink(t *testing.T) {
 	for _, url := range tests {
 		resp := session.MakeRequest(t, NewRequest(t, "GET", url), http.StatusOK)
 		htmlDoc := NewHTMLParser(t, resp.Body)
-		menus := htmlDoc.doc.Find(".ui.top.attached.header .ui.dropdown .menu a")
+		assert.Equal(t,
-		menus.Each(func(i int, menu *goquery.Selection) {
+			webhooksLen,
-			url, exist := menu.Attr("href")
+			htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(),
-			assert.True(t, exist)
+			"not all webhooks are listed in the 'new' dropdown")
-			assert.True(t, strings.HasPrefix(url, baseurl))
+
-		})
-		assert.Equal(t, webhooksLen, htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(), "not all webhooks are listed in the 'new' dropdown")
 		csrfToken = htmlDoc.GetCSRF()
 	}
 
 	// ensure that the "failure" pages has the full dropdown as well
 	resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", baseurl+"/gitea/new", map[string]string{"_csrf": csrfToken}), http.StatusUnprocessableEntity)
 	htmlDoc := NewHTMLParser(t, resp.Body)
-	assert.Equal(t, webhooksLen, htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(), "not all webhooks are listed in the 'new' dropdown on failure")
+	assert.Equal(t,
+		webhooksLen,
+		htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(),
+		"not all webhooks are listed in the 'new' dropdown on failure")
 
 	resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", baseurl+"/1", map[string]string{"_csrf": csrfToken}), http.StatusUnprocessableEntity)
 	htmlDoc = NewHTMLParser(t, resp.Body)
-	assert.Equal(t, webhooksLen, htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(), "not all webhooks are listed in the 'new' dropdown on failure")
+	assert.Equal(t,
+		webhooksLen,
+		htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(),
+		"not all webhooks are listed in the 'new' dropdown on failure")
+
+	adminSession := loginUser(t, "user1")
+	t.Run("org3", func(t *testing.T) {
+		baseurl := "/org/org3/settings/hooks"
+		resp := adminSession.MakeRequest(t, NewRequest(t, "GET", baseurl), http.StatusOK)
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		assert.Equal(t,
+			webhooksLen,
+			htmlDoc.Find(`a[href^="`+baseurl+`/"][href$="/new"]`).Length(),
+			"not all webhooks are listed in the 'new' dropdown")
+	})
+	t.Run("admin", func(t *testing.T) {
+		baseurl := "/admin/hooks"
+		resp := adminSession.MakeRequest(t, NewRequest(t, "GET", baseurl), http.StatusOK)
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		assert.Equal(t,
+			webhooksLen,
+			htmlDoc.Find(`a[href^="/admin/default-hooks/"][href$="/new"]`).Length(),
+			"not all webhooks are listed in the 'new' dropdown for default-hooks")
+		assert.Equal(t,
+			webhooksLen,
+			htmlDoc.Find(`a[href^="/admin/system-hooks/"][href$="/new"]`).Length(),
+			"not all webhooks are listed in the 'new' dropdown for system-hooks")
+	})
 }
 
 func TestWebhookForms(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	session := loginUser(t, "user2")
+	session := loginUser(t, "user1")
 
 	t.Run("forgejo/required", testWebhookForms("forgejo", session, map[string]string{
 		"payload_url":  "https://forgejo.example.com",
@@ -272,7 +299,9 @@ func assertInput(t testing.TB, form *goquery.Selection, name string) string {
 	t.Helper()
 	input := form.Find(`input[name="` + name + `"]`)
 	if input.Length() != 1 {
-		t.Log(form.Html())
+		form.Find("input").Each(func(i int, s *goquery.Selection) {
+			t.Logf("found <input name=%q />", s.AttrOr("name", ""))
+		})
 		t.Errorf("field <input name=%q /> found %d times, expected once", name, input.Length())
 	}
 	switch input.AttrOr("type", "") {
@@ -288,99 +317,126 @@ func assertInput(t testing.TB, form *goquery.Selection, name string) string {
 
 func testWebhookForms(name string, session *TestSession, validFields map[string]string, invalidPatches ...map[string]string) func(t *testing.T) {
 	return func(t *testing.T) {
-		// new webhook form
+		t.Run("repo1", func(t *testing.T) {
-		resp := session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/settings/hooks/"+name+"/new"), http.StatusOK)
+			testWebhookFormsShared(t, "/user2/repo1/settings/hooks", name, session, validFields, invalidPatches...)
-		htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`)
+		})
+		t.Run("org3", func(t *testing.T) {
+			testWebhookFormsShared(t, "/org/org3/settings/hooks", name, session, validFields, invalidPatches...)
+		})
+		t.Run("system", func(t *testing.T) {
+			testWebhookFormsShared(t, "/admin/system-hooks", name, session, validFields, invalidPatches...)
+		})
+		t.Run("default", func(t *testing.T) {
+			testWebhookFormsShared(t, "/admin/default-hooks", name, session, validFields, invalidPatches...)
+		})
+	}
+}
 
-		// fill the form
+func testWebhookFormsShared(t *testing.T, endpoint, name string, session *TestSession, validFields map[string]string, invalidPatches ...map[string]string) {
-		payload := map[string]string{
+	// new webhook form
-			"_csrf":  htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""),
+	resp := session.MakeRequest(t, NewRequest(t, "GET", endpoint+"/"+name+"/new"), http.StatusOK)
-			"events": "send_everything",
+	htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`)
-		}
-		for k, v := range validFields {
-			assertInput(t, htmlForm, k)
-			payload[k] = v
-		}
-		if t.Failed() {
-			t.FailNow() // prevent further execution if the form could not be filled properly
-		}
 
-		// create the webhook (this redirects back to the hook list)
+	// fill the form
-		resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/settings/hooks/"+name+"/new", payload), http.StatusSeeOther)
+	payload := map[string]string{
-		assertHasFlashMessages(t, resp, "success")
+		"_csrf":  htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""),
+		"events": "send_everything",
+	}
+	for k, v := range validFields {
+		assertInput(t, htmlForm, k)
+		payload[k] = v
+	}
+	if t.Failed() {
+		t.FailNow() // prevent further execution if the form could not be filled properly
+	}
 
-		// find last created hook in the hook list
+	// create the webhook (this redirects back to the hook list)
-		// (a bit hacky, but the list should be sorted)
+	resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", endpoint+"/"+name+"/new", payload), http.StatusSeeOther)
-		resp = session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/settings/hooks"), http.StatusOK)
+	assertHasFlashMessages(t, resp, "success")
-		htmlDoc := NewHTMLParser(t, resp.Body)
+	listEndpoint := resp.Header().Get("Location")
-		editFormURL := htmlDoc.Find(`a[href^="/user2/repo1/settings/hooks/"]`).Last().AttrOr("href", "")
+	updateEndpoint := endpoint + "/"
-		assert.NotEmpty(t, editFormURL)
+	if endpoint == "/admin/system-hooks" || endpoint == "/admin/default-hooks" {
+		updateEndpoint = "/admin/hooks/"
+	}
 
-		// edit webhook form
+	// find last created hook in the hook list
-		resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK)
+	// (a bit hacky, but the list should be sorted)
-		htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`)
+	resp = session.MakeRequest(t, NewRequest(t, "GET", listEndpoint), http.StatusOK)
-		editPostURL := htmlForm.AttrOr("action", "")
+	htmlDoc := NewHTMLParser(t, resp.Body)
-		assert.NotEmpty(t, editPostURL)
+	selector := `a[href^="` + updateEndpoint + `"]`
+	if endpoint == "/admin/system-hooks" {
+		// system-hooks and default-hooks are listed on the same page
+		// add a specifier to select the latest system-hooks
+		// (the default-hooks are at the end, so no further specifier needed)
+		selector = `.admin-setting-content > div:first-of-type ` + selector
+	}
+	editFormURL := htmlDoc.Find(selector).Last().AttrOr("href", "")
+	assert.NotEmpty(t, editFormURL)
 
-		// fill the form
+	// edit webhook form
-		payload = map[string]string{
+	resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK)
-			"_csrf":  htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""),
+	htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + updateEndpoint + `"]`)
-			"events": "push_only",
+	editPostURL := htmlForm.AttrOr("action", "")
-		}
+	assert.NotEmpty(t, editPostURL)
-		for k, v := range validFields {
-			assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v)
-			payload[k] = v
-		}
 
-		// update the webhook
+	// fill the form
-		resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", editPostURL, payload), http.StatusSeeOther)
+	payload = map[string]string{
-		assertHasFlashMessages(t, resp, "success")
+		"_csrf":  htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""),
+		"events": "push_only",
+	}
+	for k, v := range validFields {
+		assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v)
+		payload[k] = v
+	}
 
-		// check the updated webhook
+	// update the webhook
-		resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK)
+	resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", editPostURL, payload), http.StatusSeeOther)
-		htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`)
+	assertHasFlashMessages(t, resp, "success")
-		for k, v := range validFields {
-			assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v)
-		}
 
-		if len(invalidPatches) > 0 {
+	// check the updated webhook
-			// check that invalid fields are rejected
+	resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK)
-			resp := session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/settings/hooks/"+name+"/new"), http.StatusOK)
+	htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + updateEndpoint + `"]`)
-			htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`)
+	for k, v := range validFields {
+		assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v)
+	}
 
-			for _, invalidPatch := range invalidPatches {
+	if len(invalidPatches) > 0 {
-				t.Run("invalid", func(t *testing.T) {
+		// check that invalid fields are rejected
-					// fill the form
+		resp := session.MakeRequest(t, NewRequest(t, "GET", endpoint+"/"+name+"/new"), http.StatusOK)
-					payload := map[string]string{
+		htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`)
-						"_csrf":  htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""),
+
-						"events": "send_everything",
+		for _, invalidPatch := range invalidPatches {
-					}
+			t.Run("invalid", func(t *testing.T) {
-					for k, v := range validFields {
+				// fill the form
+				payload := map[string]string{
+					"_csrf":  htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""),
+					"events": "send_everything",
+				}
+				for k, v := range validFields {
+					payload[k] = v
+				}
+				for k, v := range invalidPatch {
+					if v == "" {
+						delete(payload, k)
+					} else {
 						payload[k] = v
 					}
-					for k, v := range invalidPatch {
+				}
-						if v == "" {
-							delete(payload, k)
-						} else {
-							payload[k] = v
-						}
-					}
 
-					resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/settings/hooks/"+name+"/new", payload), http.StatusUnprocessableEntity)
+				resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", endpoint+"/"+name+"/new", payload), http.StatusUnprocessableEntity)
-					// check that the invalid form is pre-filled
+				// check that the invalid form is pre-filled
-					htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`)
+				htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`)
-					for k, v := range payload {
+				for k, v := range payload {
-						if k == "_csrf" || k == "events" || v == "" {
+					if k == "_csrf" || k == "events" || v == "" {
-							// the 'events' is a radio input, which is buggy below
+						// the 'events' is a radio input, which is buggy below
-							continue
+						continue
-						}
-						assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v)
 					}
-					if t.Failed() {
+					assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v)
-						t.Log(invalidPatch)
+				}
-					}
+				if t.Failed() {
-				})
+					t.Log(invalidPatch)
-			}
+				}
+			})
 		}
 	}
 }