diff --git a/server/plugin/command.go b/server/plugin/command.go index b8cb89350..601cb268b 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -409,57 +409,92 @@ 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 { - 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 - }) + listOrgHooks := func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Organizations.ListHooks(ctx, owner, opt) + } + + if repo == "" { + return p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, listOrgHooks) + } + + found, err := p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Repositories.ListHooks(ctx, owner, repo, opt) + }) + if err != nil { + return false, err + } + if found { + return true, nil + } + + isOrg, err := p.isOrganization(ctx, githubClient, owner) + if err != nil { + return false, err + } + if !isOrg { + return false, nil + } + + found, err = p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, listOrgHooks) + if err != nil { + if isWebhookListAccessError(err) { + return true, nil } + return false, err + } + return found, nil +} +func (p *Plugin) isOrganization(ctx context.Context, githubClient *github.Client, owner string) (bool, error) { + _, resp, err := githubClient.Organizations.Get(ctx, owner) + if err != nil { + if resp != nil && resp.StatusCode == http.StatusNotFound { + return false, nil + } + return false, err + } + return true, nil +} + +func (p *Plugin) anyHookMatchesSiteURL(userInfo *GitHubUserInfo, owner, repo, siteURL string, list func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error)) (bool, error) { + opt := &github.ListOptions{PerPage: PerPageValue} + for { + var hooks []*github.Hook + var resp *github.Response + var listErr error + cErr := p.useGitHubClient(userInfo, func(_ *GitHubUserInfo, _ *oauth2.Token) error { + hooks, resp, listErr = list(opt) + return listErr + }) 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 + p.client.Log.Warn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", cErr.Error()) + return false, cErr } - for _, hook := range githubHooks { - if strings.Contains(hook.Config["url"].(string), siteURL) { - found = true - break + for _, hook := range hooks { + if url, ok := hook.Config["url"].(string); ok && strings.Contains(url, siteURL) { + return true, nil } } - if githubResponse.NextPage == 0 { - break + if resp == nil || resp.NextPage == 0 { + return false, nil } - opt.Page = githubResponse.NextPage + opt.Page = resp.NextPage } +} - return found, nil +func isWebhookListAccessError(err error) bool { + if err == nil { + return false + } + msg := err.Error() + return strings.Contains(msg, "404 Not Found") || strings.Contains(msg, "403 Forbidden") } func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string { diff --git a/server/plugin/command_test.go b/server/plugin/command_test.go index 21add9942..f64f76b36 100644 --- a/server/plugin/command_test.go +++ b/server/plugin/command_test.go @@ -4,12 +4,18 @@ package plugin import ( + "context" "fmt" + "net/http" + "net/http/httptest" + "net/url" "os" "path/filepath" + "sync/atomic" "testing" "github.com/golang/mock/gomock" + "github.com/google/go-github/v54/github" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin" "github.com/mattermost/mattermost/server/public/plugin/plugintest" @@ -1784,6 +1790,131 @@ func TestFormattedString(t *testing.T) { } } +func TestCheckIfConfiguredWebhookExists(t *testing.T) { + const ( + siteURL = "https://example.com" + owner = "test-owner" + repo = "test-repo" + ) + + newGitHubClient := func(t *testing.T, server *httptest.Server) *github.Client { + t.Helper() + client := github.NewClient(nil) + base, err := url.Parse(server.URL + "/") + require.NoError(t, err) + client.BaseURL = base + client.UploadURL = base + return client + } + + configureSiteURL := func(api *plugintest.API) { + siteURLCopy := siteURL + api.On("GetConfig").Return(&model.Config{ + ServiceSettings: model.ServiceSettings{SiteURL: &siteURLCopy}, + }) + } + + t.Run("user-owned repo skips org-hooks fallback and returns false", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + + var orgHooksCalls int32 + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("[]")) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s", owner), func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/hooks", owner), func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&orgHooksCalls, 1) + http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.False(t, found) + assert.Zero(t, atomic.LoadInt32(&orgHooksCalls), "org-hooks endpoint must not be called for user-owned repos") + }) + + t.Run("repo-level webhook matches site URL", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `[{"config":{"url":"%s/plugins/github/webhook"}}]`, siteURL) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.True(t, found) + }) + + t.Run("org-owned repo falls back to org-hooks and finds a match", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("[]")) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s", owner), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"login":"%s","type":"Organization"}`, owner) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/hooks", owner), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `[{"config":{"url":"%s/plugins/github/webhook"}}]`, siteURL) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.True(t, found) + }) + + t.Run("org-owned repo treats org-hooks access error as configured", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + mockAPI.On("LogWarn", "Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", mock.Anything).Maybe() + mockAPI.On("LogWarn", "Error occurred while using the Github client", "error", mock.Anything).Maybe() + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("[]")) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s", owner), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"login":"%s","type":"Organization"}`, owner) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/hooks", owner), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Forbidden"}`)) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.True(t, found, "403 from org-hooks should be treated as access error and assumed configured") + }) +} + func TestToSlice(t *testing.T) { tests := []struct { name string