From d8cce6367fcb1f70b54905da780e80132625221d Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 8 Aug 2016 11:47:16 +0100 Subject: [PATCH 1/5] Change github service config format to include owner/repo filtering on webhooks --- .../go-neb/services/github/github.go | 51 +++++++++++-------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/src/github.com/matrix-org/go-neb/services/github/github.go b/src/github.com/matrix-org/go-neb/services/github/github.go index f01ad9d..508c662 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github.go +++ b/src/github.com/matrix-org/go-neb/services/github/github.go @@ -26,7 +26,11 @@ type githubService struct { BotUserID string GithubUserID string RealmID string - WebhookRooms map[string][]string // room_id => ["push","issue","pull_request"] + Rooms map[string]struct { // room_id => {} + OwnerRepo map[string]struct { // owner/repo => { events: ["push","issue","pull_request"] } + Events []string + } + } } func (s *githubService) ServiceUserID() string { return s.BotUserID } @@ -34,7 +38,7 @@ func (s *githubService) ServiceID() string { return s.id } func (s *githubService) ServiceType() string { return "github" } func (s *githubService) RoomIDs() []string { var keys []string - for k := range s.WebhookRooms { + for k := range s.Rooms { keys = append(keys, k) } return keys @@ -130,25 +134,31 @@ func (s *githubService) OnReceiveWebhook(w http.ResponseWriter, req *http.Reques return } - for roomID, notif := range s.WebhookRooms { - notifyRoom := false - for _, notifyType := range notif { - if evType == notifyType { - notifyRoom = true - break + for roomID, roomConfig := range s.Rooms { + for ownerRepo, repoConfig := range roomConfig.OwnerRepo { + if *repo.FullName != ownerRepo { + continue } - } - if notifyRoom { - log.WithFields(log.Fields{ - "type": evType, - "msg": msg, - "repo": repo, - "room_id": roomID, - }).Print("Sending notification to room") - _, e := cli.SendMessageEvent(roomID, "m.room.message", msg) - if e != nil { - log.WithError(e).WithField("room_id", roomID).Print( - "Failed to send notification to room.") + + notifyRoom := false + for _, notifyType := range repoConfig.Events { + if evType == notifyType { + notifyRoom = true + break + } + } + if notifyRoom { + log.WithFields(log.Fields{ + "type": evType, + "msg": msg, + "repo": repo, + "room_id": roomID, + }).Print("Sending notification to room") + _, e := cli.SendMessageEvent(roomID, "m.room.message", msg) + if e != nil { + log.WithError(e).WithField("room_id", roomID).Print( + "Failed to send notification to room.") + } } } } @@ -167,6 +177,7 @@ func (s *githubService) Register() error { if realm.Type() != "github" { return fmt.Errorf("Realm is of type '%s', not 'github'", realm.Type()) } + return nil } From 33b5abb84bf03cdc35695d4f22e1138dcd30e5e1 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 8 Aug 2016 15:46:24 +0100 Subject: [PATCH 2/5] Add Service.PostRegister(oldService) and implement GH webhook creation - `PostRegister` is called after the new Service is stored in the database. It exists so Services can do post-creation actions like hit remote services using information from old services, if any. - The `GithubService` uses the `PostRegister()` function to remove old webhooks. --- src/github.com/matrix-org/go-neb/api.go | 2 + .../matrix-org/go-neb/services/echo/echo.go | 11 +- .../go-neb/services/github/github.go | 143 ++++++++++++++++-- .../go-neb/services/github/webhook/webhook.go | 4 + .../matrix-org/go-neb/types/types.go | 1 + 5 files changed, 145 insertions(+), 16 deletions(-) diff --git a/src/github.com/matrix-org/go-neb/api.go b/src/github.com/matrix-org/go-neb/api.go index 522d4c9..ea87876 100644 --- a/src/github.com/matrix-org/go-neb/api.go +++ b/src/github.com/matrix-org/go-neb/api.go @@ -213,6 +213,8 @@ func (s *configureServiceHandler) OnIncomingRequest(req *http.Request) (interfac return nil, &errors.HTTPError{err, "Error storing service", 500} } + service.PostRegister(oldService) + return &struct { ID string Type string diff --git a/src/github.com/matrix-org/go-neb/services/echo/echo.go b/src/github.com/matrix-org/go-neb/services/echo/echo.go index c0cde71..3631cce 100644 --- a/src/github.com/matrix-org/go-neb/services/echo/echo.go +++ b/src/github.com/matrix-org/go-neb/services/echo/echo.go @@ -14,11 +14,12 @@ type echoService struct { Rooms []string } -func (e *echoService) ServiceUserID() string { return e.UserID } -func (e *echoService) ServiceID() string { return e.id } -func (e *echoService) ServiceType() string { return "echo" } -func (e *echoService) RoomIDs() []string { return e.Rooms } -func (e *echoService) Register() error { return nil } +func (e *echoService) ServiceUserID() string { return e.UserID } +func (e *echoService) ServiceID() string { return e.id } +func (e *echoService) ServiceType() string { return "echo" } +func (e *echoService) RoomIDs() []string { return e.Rooms } +func (e *echoService) Register() error { return nil } +func (e *echoService) PostRegister(old types.Service) {} func (e *echoService) Plugin(roomID string) plugin.Plugin { return plugin.Plugin{ Commands: []plugin.Command{ diff --git a/src/github.com/matrix-org/go-neb/services/github/github.go b/src/github.com/matrix-org/go-neb/services/github/github.go index 508c662..86b4d18 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github.go +++ b/src/github.com/matrix-org/go-neb/services/github/github.go @@ -22,12 +22,14 @@ import ( var ownerRepoIssueRegex = regexp.MustCompile("([A-z0-9-_]+)/([A-z0-9-_]+)#([0-9]+)") type githubService struct { - id string - BotUserID string - GithubUserID string - RealmID string - Rooms map[string]struct { // room_id => {} - OwnerRepo map[string]struct { // owner/repo => { events: ["push","issue","pull_request"] } + id string + BotUserID string + ClientUserID string + RealmID string + SecretToken string + WebhookBaseURI string + Rooms map[string]struct { // room_id => {} + Repos map[string]struct { // owner/repo => { events: ["push","issue","pull_request"] } Events []string } } @@ -128,15 +130,15 @@ func (s *githubService) Plugin(roomID string) plugin.Plugin { } } func (s *githubService) OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *matrix.Client) { - evType, repo, msg, err := webhook.OnReceiveRequest(req, "") + evType, repo, msg, err := webhook.OnReceiveRequest(req, s.SecretToken) if err != nil { w.WriteHeader(err.Code) return } for roomID, roomConfig := range s.Rooms { - for ownerRepo, repoConfig := range roomConfig.OwnerRepo { - if *repo.FullName != ownerRepo { + for ownerRepo, repoConfig := range roomConfig.Repos { + if !strings.EqualFold(*repo.FullName, ownerRepo) { continue } @@ -165,8 +167,8 @@ func (s *githubService) OnReceiveWebhook(w http.ResponseWriter, req *http.Reques w.WriteHeader(200) } func (s *githubService) Register() error { - if s.RealmID == "" || s.BotUserID == "" { - return fmt.Errorf("RealmID and BotUserID are required") + if s.RealmID == "" || s.ClientUserID == "" || s.BotUserID == "" { + return fmt.Errorf("RealmID, BotUserID and ClientUserID are required") } // check realm exists realm, err := database.GetServiceDB().LoadAuthRealm(s.RealmID) @@ -178,9 +180,90 @@ func (s *githubService) Register() error { return fmt.Errorf("Realm is of type '%s', not 'github'", realm.Type()) } + // In order to register the GH service, you must have authed with GH. + cli := s.githubClientFor(s.ClientUserID, false) + if cli == nil { + return fmt.Errorf("User %s does not have a Github auth session.", s.ClientUserID) + } + return nil } +func (s *githubService) PostRegister(oldService types.Service) { + cli := s.githubClientFor(s.ClientUserID, false) + if cli == nil { + log.Errorf("PostRegister: %s does not have a github session", s.ClientUserID) + return + } + old, ok := oldService.(*githubService) + if !ok { + log.Error("PostRegister: Provided old service is not of type GithubService") + return + } + + // remove any existing webhooks this service created on the user's behalf + modifyWebhooks(old, cli, true) + + // make new webhooks according to service config + modifyWebhooks(s, cli, false) +} + +func modifyWebhooks(s *githubService, cli *github.Client, removeHooks bool) { + // TODO: This makes assumptions about how Go-NEB maps services to webhook endpoints. + // We should factor this out to a function called GetWebhookEndpoint(Service) or something. + trailingSlash := "" + if !strings.HasSuffix(s.WebhookBaseURI, "/") { + trailingSlash = "/" + } + webhookEndpointURL := s.WebhookBaseURI + trailingSlash + "services/hooks/" + s.id + + ownerRepoSet := make(map[string]bool) + for _, roomCfg := range s.Rooms { + for ownerRepo := range roomCfg.Repos { + // sanity check that it looks like 'owner/repo' as we'll split on / later + if strings.Count(ownerRepo, "/") != 1 { + log.WithField("owner_repo", ownerRepo).Print("Bad owner/repo value.") + continue + } + ownerRepoSet[ownerRepo] = true + } + } + + for ownerRepo := range ownerRepoSet { + o := strings.Split(ownerRepo, "/") + owner := o[0] + repo := o[1] + logger := log.WithFields(log.Fields{ + "owner": owner, + "repo": repo, + }) + if removeHooks { + removeHook(logger, cli, owner, repo, webhookEndpointURL) + + } else { + // make a hook for all GH events since we'll filter it when we receive webhook requests + name := "web" // https://developer.github.com/v3/repos/hooks/#create-a-hook + cfg := map[string]interface{}{ + "content_type": "json", + "url": webhookEndpointURL, + } + if s.SecretToken != "" { + cfg["secret"] = s.SecretToken + } + events := []string{"push", "pull_request", "issues", "issue_comment", "pull_request_review_comment"} + _, _, err := cli.Repositories.CreateHook(owner, repo, &github.Hook{ + Name: &name, + Config: cfg, + Events: events, + }) + if err != nil { + logger.WithError(err).Print("Failed to create webhook") + // continue as others may succeed + } + } + } +} + func (s *githubService) githubClientFor(userID string, allowUnauth bool) *github.Client { token, err := getTokenForUser(s.RealmID, userID) if err != nil { @@ -217,6 +300,9 @@ func getTokenForUser(realmID, userID string) (string, error) { if !ok { return "", fmt.Errorf("Session is not a github session: %s", session.ID()) } + if ghSession.AccessToken == "" { + return "", fmt.Errorf("Github auth session for %s has not been completed.", userID) + } return ghSession.AccessToken, nil } @@ -249,6 +335,41 @@ func ownerRepoNumberFromText(ownerRepoNumberText string) (string, string, int, e return groups[1], groups[2], num, nil } +func removeHook(logger *log.Entry, cli *github.Client, owner, repo, webhookEndpointURL string) { + // Get a list of webhooks for this owner/repo and find the one which has the + // same endpoint URL which is what github uses to determine equivalence. + hooks, _, err := cli.Repositories.ListHooks(owner, repo, nil) + if err != nil { + logger.WithError(err).Print("Failed to list hooks") + return + } + var hook *github.Hook + for _, h := range hooks { + if h.Config["url"] == nil { + logger.Print("Ignoring nil config.url") + continue + } + hookURL, ok := h.Config["url"].(string) + if !ok { + logger.Print("Ignoring non-string config.url") + continue + } + if hookURL == webhookEndpointURL { + hook = h + break + } + } + if hook == nil { + return // couldn't find it + } + + _, err = cli.Repositories.DeleteHook(owner, repo, *hook.ID) + if err != nil { + logger.WithError(err).Print("Failed to delete hook") + // continue as others may succeed + } +} + func init() { types.RegisterService(func(serviceID string) types.Service { return &githubService{id: serviceID} diff --git a/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go b/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go index 8e03d4c..5d4ac9a 100644 --- a/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go +++ b/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go @@ -53,6 +53,10 @@ func OnReceiveRequest(r *http.Request, secretToken string) (string, *github.Repo "signature": signatureSHA1, }).Print("Received Github event") + if eventType == "ping" { + return "", nil, nil, &errors.HTTPError{nil, "pong", 200} + } + htmlStr, repo, err := parseGithubEvent(eventType, content) if err != nil { log.WithError(err).Print("Failed to parse github event") diff --git a/src/github.com/matrix-org/go-neb/types/types.go b/src/github.com/matrix-org/go-neb/types/types.go index 942279f..5e22639 100644 --- a/src/github.com/matrix-org/go-neb/types/types.go +++ b/src/github.com/matrix-org/go-neb/types/types.go @@ -36,6 +36,7 @@ type Service interface { Plugin(roomID string) plugin.Plugin OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *matrix.Client) Register() error + PostRegister(oldService Service) } var servicesByType = map[string]func(string) Service{} From 28f26e61dd6e04aa7ce1db0392128ad1b3f46eac Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 8 Aug 2016 15:54:36 +0100 Subject: [PATCH 3/5] Formatting --- src/github.com/matrix-org/go-neb/services/github/github.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/github.com/matrix-org/go-neb/services/github/github.go b/src/github.com/matrix-org/go-neb/services/github/github.go index 86b4d18..22cdbe6 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github.go +++ b/src/github.com/matrix-org/go-neb/services/github/github.go @@ -239,7 +239,6 @@ func modifyWebhooks(s *githubService, cli *github.Client, removeHooks bool) { }) if removeHooks { removeHook(logger, cli, owner, repo, webhookEndpointURL) - } else { // make a hook for all GH events since we'll filter it when we receive webhook requests name := "web" // https://developer.github.com/v3/repos/hooks/#create-a-hook @@ -366,7 +365,6 @@ func removeHook(logger *log.Entry, cli *github.Client, owner, repo, webhookEndpo _, err = cli.Repositories.DeleteHook(owner, repo, *hook.ID) if err != nil { logger.WithError(err).Print("Failed to delete hook") - // continue as others may succeed } } From 4b41bfb4848b5a01f7069bf823b9111523a2c445 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 8 Aug 2016 16:22:25 +0100 Subject: [PATCH 4/5] Add TODO for order of operations --- src/github.com/matrix-org/go-neb/services/github/github.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/github.com/matrix-org/go-neb/services/github/github.go b/src/github.com/matrix-org/go-neb/services/github/github.go index 22cdbe6..b61ea91 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github.go +++ b/src/github.com/matrix-org/go-neb/services/github/github.go @@ -201,6 +201,12 @@ func (s *githubService) PostRegister(oldService types.Service) { return } + // TODO: We should be adding webhooks in Register() then removing old hooks in PostRegister() + // + // By doing both operations in PostRegister(), if some of the requests fail we can end up in + // an inconsistent state. It is a lot simpler and easy to reason about this way though, so + // for now it will do. + // remove any existing webhooks this service created on the user's behalf modifyWebhooks(old, cli, true) From cc0325f4ff0a4031f36b211408bdedbf9eb73076 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 8 Aug 2016 16:43:42 +0100 Subject: [PATCH 5/5] Explain what the ping event does --- .../matrix-org/go-neb/services/github/webhook/webhook.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go b/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go index 5d4ac9a..b70614e 100644 --- a/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go +++ b/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go @@ -54,6 +54,9 @@ func OnReceiveRequest(r *http.Request, secretToken string) (string, *github.Repo }).Print("Received Github event") if eventType == "ping" { + // Github will send a "ping" event when the webhook is first created. We need + // to return a 200 in order for the webhook to be marked as "up" (this doesn't + // affect delivery, just the tick/cross status flag). return "", nil, nil, &errors.HTTPError{nil, "pong", 200} }