From d044b8bac477db00f7a70f0d7ef3bd7aa2c30bb6 Mon Sep 17 00:00:00 2001 From: Wayne Wollesen Date: Mon, 15 Jun 2026 13:31:45 -0500 Subject: [PATCH] MM-69266: Fall back to org-level webhooks for repo subscriptions When subscribing a channel to a repository, checkIfConfiguredWebhookExists only listed repository-level hooks. If the webhook was instead configured at the organization level (which still delivers events for the repo), the check found no match and the subscription confirmation wrongly appended the "No webhook was found... run /github setup webhook" note. Refactor the lookup into a small paginated helper called twice: repo-level first, then an organization-level fallback when the repo list returns cleanly but matches nothing. Listing org hooks requires org-admin scope that a repo subscriber often lacks (403/404), so the fallback is best-effort: its error is logged at debug and swallowed, falling back to "not found" rather than propagating. This preserves the caller's existing "404 Not Found" suppression behavior and never regresses pre-fix behavior. Adds tests for checkIfConfiguredWebhookExists (previously untested) covering repo hook present, repo-absent+org-present, both absent, and org-list forbidden, plus org-subscription paths. Ref: support ticket 51590 Co-Authored-By: Claude Opus 4.8 (1M context) --- server/plugin/command.go | 79 +++++++---- server/plugin/command_webhook_test.go | 196 ++++++++++++++++++++++++++ 2 files changed, 246 insertions(+), 29 deletions(-) create mode 100644 server/plugin/command_webhook_test.go diff --git a/server/plugin/command.go b/server/plugin/command.go index b8cb89350..08e847c3d 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -409,47 +409,68 @@ func (p *Plugin) createPost(channelID, userID, message string) error { } func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClient *github.Client, userInfo *GitHubUserInfo, repo, owner string) (bool, error) { - found := false - opt := &github.ListOptions{ - PerPage: PerPageValue, - } siteURL, err := getSiteURL(p.client) if err != nil { return false, err } + // For an organization subscription only the organization-level webhooks are relevant. + if repo == "" { + return p.findWebhookForSiteURL(userInfo, siteURL, func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Organizations.ListHooks(ctx, owner, opt) + }) + } + + // For a repository subscription, check the repository-level webhooks first. + found, err := p.findWebhookForSiteURL(userInfo, siteURL, func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Repositories.ListHooks(ctx, owner, repo, opt) + }) + if err != nil || found { + return found, err + } + + // The webhook may instead be configured at the organization level, which still + // delivers events for this repository. Listing organization webhooks requires + // org-admin scope that a repository subscriber often lacks (yielding a 403/404), so + // treat this as a best-effort fallback: swallow its error and report "not found" + // rather than propagating, preserving the caller's existing behavior for such users. + found, orgErr := p.findWebhookForSiteURL(userInfo, siteURL, func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Organizations.ListHooks(ctx, owner, opt) + }) + if orgErr != nil { + p.client.Log.Debug("Unable to check organization webhooks as fallback for repository subscription", "Owner", owner, "Repo", repo, "error", orgErr.Error()) + return false, nil + } + + return found, nil +} + +// findWebhookForSiteURL pages through the webhooks returned by listHooks and reports +// whether any of them targets the configured site URL. listHooks fetches a single page of +// webhooks for the given list options; pagination is handled here. Any error from the +// underlying GitHub call is returned unwrapped so callers can string-match on it (e.g. +// "404 Not Found"). +func (p *Plugin) findWebhookForSiteURL(userInfo *GitHubUserInfo, siteURL string, listHooks func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error)) (bool, error) { + opt := &github.ListOptions{ + PerPage: PerPageValue, + } + for { var githubHooks []*github.Hook var githubResponse *github.Response - var err, cErr error - - if repo == "" { - cErr = p.useGitHubClient(userInfo, func(info *GitHubUserInfo, token *oauth2.Token) error { - githubHooks, githubResponse, err = githubClient.Organizations.ListHooks(ctx, owner, opt) - if err != nil { - return err - } - return nil - }) - } else { - cErr = p.useGitHubClient(userInfo, func(info *GitHubUserInfo, token *oauth2.Token) error { - githubHooks, githubResponse, err = githubClient.Repositories.ListHooks(ctx, owner, repo, opt) - if err != nil { - return err - } - return nil - }) - } + var err error + cErr := p.useGitHubClient(userInfo, func(info *GitHubUserInfo, token *oauth2.Token) error { + githubHooks, githubResponse, err = listHooks(opt) + return err + }) if cErr != nil { - p.client.Log.Warn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", err.Error()) - return found, err + return false, cErr } for _, hook := range githubHooks { - if strings.Contains(hook.Config["url"].(string), siteURL) { - found = true - break + if url, ok := hook.Config["url"].(string); ok && strings.Contains(url, siteURL) { + return true, nil } } @@ -459,7 +480,7 @@ func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClien opt.Page = githubResponse.NextPage } - return found, nil + return false, nil } func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string { diff --git a/server/plugin/command_webhook_test.go b/server/plugin/command_webhook_test.go new file mode 100644 index 000000000..aa43a747f --- /dev/null +++ b/server/plugin/command_webhook_test.go @@ -0,0 +1,196 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/google/go-github/v54/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "golang.org/x/oauth2" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" + "github.com/mattermost/mattermost/server/public/pluginapi" +) + +const ( + webhookTestSiteURL = "https://mattermost.example.com" + webhookTestOwner = "mockOrg" + webhookTestRepo = "mockRepo" + webhookTestMatchingURL = webhookTestSiteURL + "/plugins/github/webhook" + webhookTestOtherURL = "https://example.org/some-other-hook" +) + +// hooksResponse renders a GitHub "list hooks" API response whose webhooks target the given URLs. +func hooksResponse(urls ...string) string { + hooks := make([]string, 0, len(urls)) + for i, u := range urls { + hooks = append(hooks, fmt.Sprintf(`{"id":%d,"type":"Repository","name":"web","active":true,"config":{"url":%q,"content_type":"json"}}`, i+1, u)) + } + return "[" + strings.Join(hooks, ",") + "]" +} + +// jsonHandler responds with the given status code and body. +func jsonHandler(status int, body string) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = fmt.Fprint(w, body) + } +} + +// newHooksTestClient builds a github.Client pointed at a test server that serves the given +// handlers keyed by request path. Paths without a handler return 404. +func newHooksTestClient(t *testing.T, handlers map[string]http.HandlerFunc) (*github.Client, func()) { + t.Helper() + mux := http.NewServeMux() + for path, handler := range handlers { + mux.HandleFunc(path, handler) + } + server := httptest.NewServer(mux) + + client := github.NewClient(nil) + u, err := url.Parse(server.URL + "/") + if err != nil { + t.Fatalf("failed to parse test server URL: %v", err) + } + client.BaseURL = u + client.UploadURL = u + + return client, server.Close +} + +// setupWebhookCheckPlugin wires a Plugin with a mocked API that reports the configured site URL +// and swallows the warn/debug logs the webhook check may emit on fallback paths. +func setupWebhookCheckPlugin(t *testing.T) *Plugin { + t.Helper() + + config := &model.Config{} + config.ServiceSettings.SiteURL = model.NewPointer(webhookTestSiteURL) + + api := &plugintest.API{} + api.On("GetConfig").Return(config).Maybe() + // useGitHubClient logs at warn level on any GitHub error: message + "error" + value. + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything).Maybe() + // The org-level fallback logs at debug level: message + Owner/Repo/error key-value pairs. + api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + + p := NewPlugin() + p.setConfiguration(&Configuration{EncryptionKey: "dummyEncryptKey1"}) + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + + return p +} + +func repoHooksPath(owner, repo string) string { + return fmt.Sprintf("/repos/%s/%s/hooks", owner, repo) +} + +func orgHooksPath(owner string) string { + return fmt.Sprintf("/orgs/%s/hooks", owner) +} + +func TestCheckIfConfiguredWebhookExists(t *testing.T) { + userInfo := &GitHubUserInfo{ + UserID: "mockUserID", + Token: &oauth2.Token{AccessToken: "mockToken"}, + } + + tests := []struct { + name string + repo string + handlers map[string]http.HandlerFunc + expectFound bool + expectErr string // substring expected in the error; empty means no error + }{ + { + name: "repo subscription: repo-level webhook present", + repo: webhookTestRepo, + handlers: map[string]http.HandlerFunc{ + repoHooksPath(webhookTestOwner, webhookTestRepo): jsonHandler(http.StatusOK, hooksResponse(webhookTestOtherURL, webhookTestMatchingURL)), + }, + expectFound: true, + }, + { + name: "repo subscription: repo-level absent but org-level present", + repo: webhookTestRepo, + handlers: map[string]http.HandlerFunc{ + repoHooksPath(webhookTestOwner, webhookTestRepo): jsonHandler(http.StatusOK, hooksResponse(webhookTestOtherURL)), + orgHooksPath(webhookTestOwner): jsonHandler(http.StatusOK, hooksResponse(webhookTestMatchingURL)), + }, + expectFound: true, + }, + { + name: "repo subscription: webhook absent at both repo and org level", + repo: webhookTestRepo, + handlers: map[string]http.HandlerFunc{ + repoHooksPath(webhookTestOwner, webhookTestRepo): jsonHandler(http.StatusOK, hooksResponse(webhookTestOtherURL)), + orgHooksPath(webhookTestOwner): jsonHandler(http.StatusOK, hooksResponse()), + }, + expectFound: false, + }, + { + name: "repo subscription: org-level listing forbidden is swallowed", + repo: webhookTestRepo, + handlers: map[string]http.HandlerFunc{ + repoHooksPath(webhookTestOwner, webhookTestRepo): jsonHandler(http.StatusOK, hooksResponse(webhookTestOtherURL)), + orgHooksPath(webhookTestOwner): jsonHandler(http.StatusForbidden, `{"message":"Must have admin rights to Organization."}`), + }, + expectFound: false, + }, + { + name: "repo subscription: org-level listing 404 is swallowed", + repo: webhookTestRepo, + handlers: map[string]http.HandlerFunc{ + repoHooksPath(webhookTestOwner, webhookTestRepo): jsonHandler(http.StatusOK, hooksResponse(webhookTestOtherURL)), + orgHooksPath(webhookTestOwner): jsonHandler(http.StatusNotFound, `{"message":"Not Found"}`), + }, + expectFound: false, + }, + { + name: "org subscription: org-level webhook present", + repo: "", + handlers: map[string]http.HandlerFunc{ + orgHooksPath(webhookTestOwner): jsonHandler(http.StatusOK, hooksResponse(webhookTestMatchingURL)), + }, + expectFound: true, + }, + { + name: "org subscription: error is propagated for caller suppression", + repo: "", + handlers: map[string]http.HandlerFunc{ + orgHooksPath(webhookTestOwner): jsonHandler(http.StatusNotFound, `{"message":"Not Found"}`), + }, + expectFound: false, + expectErr: "404 Not Found", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + p := setupWebhookCheckPlugin(t) + client, closeServer := newHooksTestClient(t, tc.handlers) + defer closeServer() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), client, userInfo, tc.repo, webhookTestOwner) + + assert.Equal(t, tc.expectFound, found) + if tc.expectErr == "" { + assert.NoError(t, err) + } else { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.expectErr) + } + }) + } +}