Merge pull request #57 from matrix-org/kegan/gh-explicit-unregister-webhooks

Allow github webhooks to be explicitly removed

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

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{

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{

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{

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{

src/github.com/matrix-org/go-neb/services/jira/jira.go

@@ -41,6 +41,7 @@ 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) 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

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 = ""