MM-69266: Fall back to org-level webhooks for repo subscriptions

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 {

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)
+			}
+		})
+	}
+}