diff --git a/src/github.com/matrix-org/go-neb/api.go b/src/github.com/matrix-org/go-neb/api.go index 57685f0..4ff7dca 100644 --- a/src/github.com/matrix-org/go-neb/api.go +++ b/src/github.com/matrix-org/go-neb/api.go @@ -270,6 +270,8 @@ func (s *configureServiceHandler) OnIncomingRequest(req *http.Request) (interfac return nil, &errors.HTTPError{err, "Error storing service", 500} } + service.PostRegister(old) + 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 f56beef..800108d 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 @@ -17,6 +17,7 @@ func (e *echoService) ServiceUserID() string func (e *echoService) ServiceID() string { return e.id } func (e *echoService) ServiceType() string { return "echo" } func (e *echoService) Register(oldService types.Service, client *matrix.Client) error { return nil } +func (e *echoService) PostRegister(oldService types.Service) {} func (e *echoService) Plugin(cli *matrix.Client, roomID string) plugin.Plugin { return plugin.Plugin{ Commands: []plugin.Command{ diff --git a/src/github.com/matrix-org/go-neb/services/giphy/giphy.go b/src/github.com/matrix-org/go-neb/services/giphy/giphy.go index b8b34c3..d66c13e 100644 --- a/src/github.com/matrix-org/go-neb/services/giphy/giphy.go +++ b/src/github.com/matrix-org/go-neb/services/giphy/giphy.go @@ -42,6 +42,7 @@ func (s *giphyService) ServiceType() string { return "giphy" } func (s *giphyService) OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *matrix.Client) { } func (s *giphyService) Register(oldService types.Service, client *matrix.Client) error { return nil } +func (s *giphyService) PostRegister(oldService types.Service) {} func (s *giphyService) Plugin(client *matrix.Client, roomID string) plugin.Plugin { return plugin.Plugin{ 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 c68d6aa..a572299 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 @@ -204,6 +204,8 @@ func (s *githubService) Register(oldService types.Service, client *matrix.Client return nil } +func (s *githubService) PostRegister(oldService types.Service) {} + // defaultRepo returns the default repo for the given room, or an empty string. func (s *githubService) defaultRepo(roomID string) string { logger := log.WithFields(log.Fields{ diff --git a/src/github.com/matrix-org/go-neb/services/github/github_webhook.go b/src/github.com/matrix-org/go-neb/services/github/github_webhook.go index 3a49d0e..4feeac2 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github_webhook.go +++ b/src/github.com/matrix-org/go-neb/services/github/github_webhook.go @@ -105,15 +105,10 @@ func (s *githubWebhookService) Register(oldService types.Service, client *matrix if s.RealmID == "" || s.ClientUserID == "" { return fmt.Errorf("RealmID and ClientUserID is required") } - // check realm exists - realm, err := database.GetServiceDB().LoadAuthRealm(s.RealmID) + realm, err := s.loadRealm() if err != nil { return err } - // make sure the realm is of the type we expect - if realm.Type() != "github" { - return fmt.Errorf("Realm is of type '%s', not 'github'", realm.Type()) - } // In order to register the GH service as a client, you must have authed with GH. cli := s.githubClientFor(s.ClientUserID, false) @@ -121,13 +116,8 @@ func (s *githubWebhookService) Register(oldService types.Service, client *matrix return fmt.Errorf( "User %s does not have a Github auth session with realm %s.", s.ClientUserID, realm.ID()) } - // Make sure they have specified some webhooks (it makes no sense otherwise) - reposForWebhooks := s.repoList() - if len(reposForWebhooks) == 0 { - return fmt.Errorf("No repos for webhooks specified") - } - // Fetch the old service list and work out the difference between the two. + // Fetch the old service list and work out the difference between the two services. var oldRepos []string if oldService != nil { old, ok := oldService.(*githubWebhookService) @@ -142,8 +132,16 @@ func (s *githubWebhookService) Register(oldService types.Service, client *matrix } } - // Add the repos in the new service but not the old service - newRepos, _ := util.Difference(reposForWebhooks, oldRepos) + reposForWebhooks := s.repoList() + + // Add hooks for the newly added repos but don't remove hooks for the removed repos: we'll clean those out later + newRepos, removedRepos := util.Difference(reposForWebhooks, oldRepos) + if len(reposForWebhooks) == 0 && len(removedRepos) == 0 { + // The user didn't specify any webhooks. This may be a bug or it may be + // a conscious decision to remove all webhooks for this service. Figure out + // which it is by checking if we'd be removing any webhooks. + return fmt.Errorf("No webhooks specified.") + } for _, r := range newRepos { logger := log.WithField("repo", r) err := s.createHook(cli, r) @@ -163,6 +161,39 @@ func (s *githubWebhookService) Register(oldService types.Service, client *matrix return nil } +func (s *githubWebhookService) PostRegister(oldService types.Service) { + // Clean up removed repositories from the old service by working out the delta between + // the old and new hooks. + + // Fetch the old service list + var oldRepos []string + if oldService != nil { + old, ok := oldService.(*githubWebhookService) + if !ok { + log.WithFields(log.Fields{ + "service_id": oldService.ServiceID(), + "service_type": oldService.ServiceType(), + }).Print("Cannot cast old github service to GithubWebhookService") + return + } + oldRepos = old.repoList() + } + + newRepos := s.repoList() + + // Register() handled adding the new repos, we just want to clean up after ourselves + _, removedRepos := util.Difference(newRepos, oldRepos) + for _, r := range removedRepos { + segs := strings.Split(r, "/") + if err := s.deleteHook(segs[0], segs[1]); err != nil { + log.WithFields(log.Fields{ + log.ErrorKey: err, + "repo": r, + }).Warn("Failed to remove webhook") + } + } +} + func (s *githubWebhookService) joinWebhookRooms(client *matrix.Client) error { for roomID := range s.Rooms { if _, err := client.JoinRoom(roomID, "", ""); err != nil { @@ -173,6 +204,7 @@ func (s *githubWebhookService) joinWebhookRooms(client *matrix.Client) error { return nil } +// Returns a list of "owner/repos" func (s *githubWebhookService) repoList() []string { var repos []string if s.Rooms == nil { @@ -328,6 +360,22 @@ func (s *githubWebhookService) githubClientFor(userID string, allowUnauth bool) } } +func (s *githubWebhookService) loadRealm() (types.AuthRealm, error) { + if s.RealmID == "" { + return nil, fmt.Errorf("Missing RealmID") + } + // check realm exists + realm, err := database.GetServiceDB().LoadAuthRealm(s.RealmID) + if err != nil { + return nil, err + } + // make sure the realm is of the type we expect + if realm.Type() != "github" { + return nil, fmt.Errorf("Realm is of type '%s', not 'github'", realm.Type()) + } + return realm, nil +} + func init() { types.RegisterService(func(serviceID, serviceUserID, webhookEndpointURL string) types.Service { return &githubWebhookService{ diff --git a/src/github.com/matrix-org/go-neb/services/jira/jira.go b/src/github.com/matrix-org/go-neb/services/jira/jira.go index d466664..97b2529 100644 --- a/src/github.com/matrix-org/go-neb/services/jira/jira.go +++ b/src/github.com/matrix-org/go-neb/services/jira/jira.go @@ -38,9 +38,10 @@ type jiraService struct { } } -func (s *jiraService) ServiceUserID() string { return s.serviceUserID } -func (s *jiraService) ServiceID() string { return s.id } -func (s *jiraService) ServiceType() string { return "jira" } +func (s *jiraService) ServiceUserID() string { return s.serviceUserID } +func (s *jiraService) ServiceID() string { return s.id } +func (s *jiraService) ServiceType() string { return "jira" } +func (s *jiraService) PostRegister(oldService types.Service) {} func (s *jiraService) Register(oldService types.Service, client *matrix.Client) error { // We only ever make 1 JIRA webhook which listens for all projects and then filter // on receive. So we simply need to know if we need to make a webhook or not. We 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 2583707..1011e45 100644 --- a/src/github.com/matrix-org/go-neb/types/types.go +++ b/src/github.com/matrix-org/go-neb/types/types.go @@ -47,7 +47,16 @@ type Service interface { ServiceType() string Plugin(cli *matrix.Client, roomID string) plugin.Plugin OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *matrix.Client) + // A lifecycle function which is invoked when the service is being registered. The old service, if one exists, is provided, + // along with a Client instance for ServiceUserID(). If this function returns an error, the service will not be registered + // or persisted to the database, and the user's request will fail. This can be useful if you depend on external factors + // such as registering webhooks. Register(oldService Service, client *matrix.Client) error + // A lifecycle function which is invoked after the service has been successfully registered and persisted to the database. + // This function is invoked within the critical section for configuring services, guaranteeing that there will not be + // concurrent modifications to this service whilst this function executes. This lifecycle hook should be used to clean + // up resources which are no longer needed (e.g. removing old webhooks). + PostRegister(oldService Service) } var baseURL = ""