diff --git a/models/webhook/webhook_system.go b/models/webhook/webhook_system.go index a2a9ee321..62e828620 100644 --- a/models/webhook/webhook_system.go +++ b/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 webhooks, db.GetEngine(ctx). - Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, false). - Find(&webhooks) + return getAdminWebhooks(ctx, false) } // 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) } diff --git a/routers/api/v1/admin/hooks.go b/routers/api/v1/admin/hooks.go index 4c168b55b..b246cb61b 100644 --- a/routers/api/v1/admin/hooks.go +++ b/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 diff --git a/routers/web/admin/hooks.go b/routers/web/admin/hooks.go index 8d59fbb85..c1f42c006 100644 --- a/routers/web/admin/hooks.go +++ b/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 diff --git a/services/webhook/webhook.go b/services/webhook/webhook.go index dc68cae84..cf4f2fdfd 100644 --- a/services/webhook/webhook.go +++ b/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) } diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 3375c0f1e..9a278e706 100644 --- a/tests/integration/repo_webhook_test.go +++ b/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") - menus.Each(func(i int, menu *goquery.Selection) { - url, exist := menu.Attr("href") - assert.True(t, exist) - 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") + 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 ", s.AttrOr("name", "")) + }) t.Errorf("field 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 - resp := session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/settings/hooks/"+name+"/new"), http.StatusOK) - htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`) + t.Run("repo1", func(t *testing.T) { + testWebhookFormsShared(t, "/user2/repo1/settings/hooks", name, session, validFields, invalidPatches...) + }) + 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 - payload := map[string]string{ - "_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 - } +func testWebhookFormsShared(t *testing.T, endpoint, name string, session *TestSession, validFields map[string]string, invalidPatches ...map[string]string) { + // new webhook form + resp := session.MakeRequest(t, NewRequest(t, "GET", endpoint+"/"+name+"/new"), http.StatusOK) + htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`) - // create the webhook (this redirects back to the hook list) - resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/settings/hooks/"+name+"/new", payload), http.StatusSeeOther) - assertHasFlashMessages(t, resp, "success") + // fill the form + payload := map[string]string{ + "_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 - // (a bit hacky, but the list should be sorted) - resp = session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/settings/hooks"), http.StatusOK) - htmlDoc := NewHTMLParser(t, resp.Body) - editFormURL := htmlDoc.Find(`a[href^="/user2/repo1/settings/hooks/"]`).Last().AttrOr("href", "") - assert.NotEmpty(t, editFormURL) + // create the webhook (this redirects back to the hook list) + resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", endpoint+"/"+name+"/new", payload), http.StatusSeeOther) + assertHasFlashMessages(t, resp, "success") + listEndpoint := resp.Header().Get("Location") + updateEndpoint := endpoint + "/" + if endpoint == "/admin/system-hooks" || endpoint == "/admin/default-hooks" { + updateEndpoint = "/admin/hooks/" + } - // edit webhook form - resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK) - htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`) - editPostURL := htmlForm.AttrOr("action", "") - assert.NotEmpty(t, editPostURL) + // find last created hook in the hook list + // (a bit hacky, but the list should be sorted) + resp = session.MakeRequest(t, NewRequest(t, "GET", listEndpoint), http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + 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 - payload = map[string]string{ - "_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 - } + // edit webhook form + resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK) + htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + updateEndpoint + `"]`) + editPostURL := htmlForm.AttrOr("action", "") + assert.NotEmpty(t, editPostURL) - // update the webhook - resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", editPostURL, payload), http.StatusSeeOther) - assertHasFlashMessages(t, resp, "success") + // fill the form + payload = map[string]string{ + "_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 - resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK) - 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) - } + // update the webhook + resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", editPostURL, payload), http.StatusSeeOther) + assertHasFlashMessages(t, resp, "success") - if len(invalidPatches) > 0 { - // check that invalid fields are rejected - resp := session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/settings/hooks/"+name+"/new"), http.StatusOK) - htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`) + // check the updated webhook + resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK) + htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + updateEndpoint + `"]`) + 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 { - t.Run("invalid", func(t *testing.T) { - // fill the form - payload := map[string]string{ - "_csrf": htmlForm.Find(`input[name="_csrf"]`).AttrOr("value", ""), - "events": "send_everything", - } - for k, v := range validFields { + if len(invalidPatches) > 0 { + // check that invalid fields are rejected + resp := session.MakeRequest(t, NewRequest(t, "GET", endpoint+"/"+name+"/new"), http.StatusOK) + htmlForm := NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`) + + for _, invalidPatch := range invalidPatches { + t.Run("invalid", func(t *testing.T) { + // 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) - // check that the invalid form is pre-filled - htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="/user2/repo1/settings/hooks/"]`) - for k, v := range payload { - if k == "_csrf" || k == "events" || v == "" { - // the 'events' is a radio input, which is buggy below - continue - } - assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v) + resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", endpoint+"/"+name+"/new", payload), http.StatusUnprocessableEntity) + // check that the invalid form is pre-filled + htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`) + for k, v := range payload { + if k == "_csrf" || k == "events" || v == "" { + // the 'events' is a radio input, which is buggy below + continue } - if t.Failed() { - t.Log(invalidPatch) - } - }) - } + assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v) + } + if t.Failed() { + t.Log(invalidPatch) + } + }) } } }