[fix] Authorize setter of 'approved' label (#229)

close #226
This commit prevents an author of a pull request to approve his/her own
pull request.

FYI) If a bot account makes pull requests, race can happen for
'approved' label. (label -> unlabel -> label -> ...)

cmd/controller/main.go

@@ -196,7 +196,7 @@ func main() {
 	// Add plugins for webhook
 	server.AddPlugin([]git.EventType{git.EventTypePullRequest, git.EventTypePush}, &dispatcher.Dispatcher{Client: mgr.GetClient()})
 	server.AddPlugin([]git.EventType{git.EventTypeIssueComment, git.EventTypePullRequestReview, git.EventTypePullRequestReviewComment}, co)
-	server.AddPlugin([]git.EventType{git.EventTypePullRequestReview}, approveHandler)
+	server.AddPlugin([]git.EventType{git.EventTypePullRequest, git.EventTypePullRequestReview}, approveHandler)
 	go srv.Start()
 
 	// Start API aggregation server

docs/quickstart.md

@@ -16,6 +16,7 @@ The contents are as follows.
 
 ## Create bot account and token
 1. Create account/token
+> DO NOT make pull requests using the bot account. Race can happen on labelling.
 - For GitHub  
   - Create a new bot account
   - Create an access token for the bot account  

pkg/chatops/plugins/approve/handlers_approve.go

@@ -25,13 +25,21 @@ type Handler struct {
 
 // Handle handles a raw webhook
 func (h *Handler) Handle(wh *git.Webhook, ic *cicdv1.IntegrationConfig) error {
-	// Exit if it's not an approve/cancel action
-	if wh.IssueComment == nil || wh.IssueComment.Issue.PullRequest.State != git.PullRequestStateOpen || wh.IssueComment.ReviewState == "" {
+	// Skip if token is empty
+	if ic.Spec.Git.Token == nil {
 		return nil
 	}
 
-	// Skip if token is empty
-	if ic.Spec.Git.Token == nil {
+	// Case 1) Approve / Cancel of a pull request (via github/gitlab feature)
+	isApproval := wh.EventType == git.EventTypeIssueComment && wh.IssueComment != nil &&
+		wh.IssueComment.Issue.PullRequest.State == git.PullRequestStateOpen && wh.IssueComment.ReviewState != ""
+
+	// Case 2) Label 'approved' is added/deleted
+	isLabeled := wh.EventType == git.EventTypePullRequest && wh.PullRequest != nil &&
+		(wh.PullRequest.Action == git.PullRequestActionLabeled || wh.PullRequest.Action == git.PullRequestActionUnlabeled)
+
+	// Exit if it's not an approve/cancel action or label action
+	if !isApproval && !isLabeled {
 		return nil
 	}
 
@@ -40,6 +48,12 @@ func (h *Handler) Handle(wh *git.Webhook, ic *cicdv1.IntegrationConfig) error {
 		return err
 	}
 
+	// For labeled/unlabeled event
+	if isLabeled {
+		return h.handleLabelEvent(wh, ic, gitCli)
+	}
+
+	// For approve/cancel event
 	switch wh.IssueComment.ReviewState {
 	case git.PullRequestReviewStateApproved:
 		return h.handleApproveCommand(wh.IssueComment, gitCli)
@@ -69,7 +83,7 @@ func (h *Handler) HandleChatOps(command chatops.Command, webhook *git.Webhook, c
 	}
 
 	// Authorize or exit
-	if err := h.authorize(config, &webhook.Sender, issueComment, gitCli); err != nil {
+	if err := h.authorize(config, webhook.Sender, issueComment.Issue.PullRequest.Author, gitCli); err != nil {
 		unAuthErr, ok := err.(*git.UnauthorizedError)
 		if !ok {
 			return err
@@ -99,6 +113,58 @@ func (h *Handler) HandleChatOps(command chatops.Command, webhook *git.Webhook, c
 	return nil
 }
 
+// handleLabelEvent handles labeled/unlabeled event for 'approved' label
+func (h *Handler) handleLabelEvent(wh *git.Webhook, ic *cicdv1.IntegrationConfig, gitCli git.Client) error {
+	pr := wh.PullRequest
+	// Check if 'approved' label is set/unset
+	isApprovedChanged := false
+	for _, l := range pr.LabelChanged {
+		if l.Name == approvedLabel {
+			isApprovedChanged = true
+			break
+		}
+	}
+	if !isApprovedChanged {
+		return nil
+	}
+
+	// Is it set or unset?
+	// Can't trust pr's action field (gitlab can set/unset labels at the same time)
+	isApprovedLabeled := false
+	for _, l := range pr.Labels {
+		if l.Name == approvedLabel {
+			isApprovedLabeled = true
+			break
+		}
+	}
+
+	// Authorize or exit
+	if err := h.authorize(ic, wh.Sender, pr.Author, gitCli); err != nil {
+		unAuthErr, ok := err.(*git.UnauthorizedError)
+		if !ok {
+			return err
+		}
+
+		// Set/Unset the label again
+		if isApprovedLabeled {
+			// Delete approved label
+			if err := gitCli.DeleteLabel(git.IssueTypePullRequest, pr.ID, approvedLabel); err != nil && !strings.Contains(err.Error(), "Label does not exist") {
+				return err
+			}
+		} else {
+			// Register approved label
+			if err := gitCli.SetLabel(git.IssueTypePullRequest, pr.ID, approvedLabel); err != nil {
+				return err
+			}
+		}
+		if err := gitCli.RegisterComment(git.IssueTypePullRequest, pr.ID, generateUserUnauthorizedComment(unAuthErr.User)); err != nil {
+			return err
+		}
+		return nil
+	}
+	return nil
+}
+
 // handleApproveCommand handles '/approve' command
 func (h *Handler) handleApproveCommand(issueComment *git.IssueComment, gitCli git.Client) error {
 	// Register approved label
@@ -128,14 +194,14 @@ func (h *Handler) handleApproveCancelCommand(issueComment *git.IssueComment, git
 }
 
 // authorize decides if the sender is authorized to approve the PR
-func (h *Handler) authorize(cfg *cicdv1.IntegrationConfig, sender *git.User, issueComment *git.IssueComment, gitCli git.Client) error {
+func (h *Handler) authorize(cfg *cicdv1.IntegrationConfig, sender git.User, author git.User, gitCli git.Client) error {
 	// Check if it's PR's author
-	if sender.ID == issueComment.Issue.PullRequest.Author.ID {
+	if sender.ID == author.ID {
 		return &git.UnauthorizedError{User: sender.Name, Repo: cfg.Spec.Git.Repository}
 	}
 
 	// Check if it's repo's maintainer
-	ok, err := gitCli.CanUserWriteToRepo(*sender)
+	ok, err := gitCli.CanUserWriteToRepo(sender)
 	if err != nil {
 		return err
 	} else if ok {

pkg/git/git_types.go

@@ -75,6 +75,9 @@ type PullRequest struct {
 	Head      Head
 	Labels    []IssueLabel
 	Mergeable bool
+
+	// LabelChanged
+	LabelChanged []IssueLabel
 }
 
 // IssueComment is a common structure for issue comment

pkg/git/github/parser.go

@@ -9,24 +9,29 @@ import (
 
 func (c *Client) parsePullRequestWebhook(jsonString []byte) (*git.Webhook, error) {
 	var data PullRequestWebhook
-
 	if err := json.Unmarshal(jsonString, &data); err != nil {
 		return nil, err
 	}
 
+	pullRequest := git.PullRequest{ID: data.Number, Title: data.PullRequest.Title, URL: data.Repo.URL, State: git.PullRequestState(data.PullRequest.State), Action: git.PullRequestAction(data.Action)}
+
 	// Get sender & author
 	sender, author := c.getSenderAuthor(data.Sender, data.PullRequest.User)
+	pullRequest.Author = *author
 
-	var labels []git.IssueLabel
 	for _, l := range data.PullRequest.Labels {
-		labels = append(labels, git.IssueLabel{Name: l.Name})
+		pullRequest.Labels = append(pullRequest.Labels, git.IssueLabel{Name: l.Name})
+	}
+
+	// Labeled/Unlabeled event
+	if data.Action == string(git.PullRequestActionLabeled) || data.Action == string(git.PullRequestActionUnlabeled) {
+		pullRequest.LabelChanged = append(pullRequest.LabelChanged, git.IssueLabel{Name: data.Label.Name})
 	}
 
-	base := git.Base{Ref: data.PullRequest.Base.Ref, Sha: data.PullRequest.Base.Sha}
-	head := git.Head{Ref: data.PullRequest.Head.Ref, Sha: data.PullRequest.Head.Sha}
+	pullRequest.Base = git.Base{Ref: data.PullRequest.Base.Ref, Sha: data.PullRequest.Base.Sha}
+	pullRequest.Head = git.Head{Ref: data.PullRequest.Head.Ref, Sha: data.PullRequest.Head.Sha}
 	repo := git.Repository{Name: data.Repo.Name, URL: data.Repo.URL}
-	pullRequest := git.PullRequest{ID: data.Number, Title: data.PullRequest.Title, Author: *author, URL: data.Repo.URL, Base: base, Head: head, State: git.PullRequestState(data.PullRequest.State), Action: git.PullRequestAction(data.Action), Labels: labels}
-	return &git.Webhook{EventType: git.EventTypePullRequest, Repo: repo, Sender: *sender, PullRequest: &pullRequest}, nil
+	return &git.Webhook{EventType: git.EventTypePullRequest, Repo: repo, PullRequest: &pullRequest, Sender: *sender}, nil
 }
 
 func (c *Client) parsePushWebhook(jsonString []byte) (*git.Webhook, error) {

pkg/git/github/webhook.go

@@ -11,6 +11,9 @@ type PullRequestWebhook struct {
 	PullRequest PullRequest `json:"pull_request"`
 
 	Repo Repo `json:"repository"`
+
+	// Changed label
+	Label LabelBody `json:"label"`
 }
 
 // PushWebhook is a github-specific push event webhook body

pkg/git/gitlab/parser.go

@@ -14,35 +14,34 @@ func (c *Client) parsePullRequestWebhook(jsonString []byte) (*git.Webhook, error
 		return nil, err
 	}
 
-	// Sender - Author might be different... in case of update
-	sender := git.User{Name: data.User.Name, Email: data.User.Email}
-	var author git.User
-	if data.User.ID == data.ObjectAttribute.AuthorID {
-		author = sender
-	} else {
-		user, err := c.GetUserInfo(strconv.Itoa(data.ObjectAttribute.AuthorID))
-		if err != nil {
-			return nil, err
-		}
-		author = *user
+	sender, author, err := c.getSenderAuthor(data.User, data.ObjectAttribute.AuthorID)
+	if err != nil {
+		return nil, err
 	}
 
-	base := git.Base{Ref: data.ObjectAttribute.BaseRef}
-	head := git.Head{Ref: data.ObjectAttribute.HeadRef, Sha: data.ObjectAttribute.LastCommit.Sha}
+	pullRequest := git.PullRequest{ID: data.ObjectAttribute.ID, Title: data.ObjectAttribute.Title, URL: data.Project.WebURL}
+	pullRequest.Author = *author
+	pullRequest.Base = git.Base{Ref: data.ObjectAttribute.BaseRef}
+	pullRequest.Head = git.Head{Ref: data.ObjectAttribute.HeadRef, Sha: data.ObjectAttribute.LastCommit.Sha}
 	repo := git.Repository{Name: data.Project.Name, URL: data.Project.WebURL}
-	action := git.PullRequestAction(data.ObjectAttribute.Action)
-	switch string(action) {
+	pullRequest.Action = git.PullRequestAction(data.ObjectAttribute.Action)
+	switch string(pullRequest.Action) {
 	case "close":
-		action = git.PullRequestActionClose
+		pullRequest.Action = git.PullRequestActionClose
 	case "open":
-		action = git.PullRequestActionOpen
+		pullRequest.Action = git.PullRequestActionOpen
 	case "reopen":
-		action = git.PullRequestActionReOpen
+		pullRequest.Action = git.PullRequestActionReOpen
 	case "update":
 		if data.ObjectAttribute.OldRev != "" {
-			action = git.PullRequestActionSynchronize
+			pullRequest.Action = git.PullRequestActionSynchronize
 		} else if data.Changes.Labels != nil {
-			action = git.PullRequestActionLabeled // Maybe unlabeled... but doesn't matter
+			var isUnlabeled bool
+			pullRequest.LabelChanged, isUnlabeled = diffLabels(data.Changes.Labels.Previous, data.Changes.Labels.Current)
+			pullRequest.Action = git.PullRequestActionLabeled
+			if isUnlabeled {
+				pullRequest.Action = git.PullRequestActionUnlabeled
+			}
 		}
 	case "approved", "unapproved":
 		return c.parsePullRequestReviewWebhook(data)
@@ -53,23 +52,21 @@ func (c *Client) parsePullRequestWebhook(jsonString []byte) (*git.Webhook, error
 	if err != nil {
 		return nil, err
 	}
-	base.Sha = baseBranch.CommitID
+	pullRequest.Base.Sha = baseBranch.CommitID
 
-	state := git.PullRequestState(data.ObjectAttribute.State)
-	switch string(state) {
+	pullRequest.State = git.PullRequestState(data.ObjectAttribute.State)
+	switch string(pullRequest.State) {
 	case "opened":
-		state = git.PullRequestStateOpen
+		pullRequest.State = git.PullRequestStateOpen
 	case "closed":
-		state = git.PullRequestStateClosed
+		pullRequest.State = git.PullRequestStateClosed
 	}
 
-	var labels []git.IssueLabel
 	for _, l := range data.Labels {
-		labels = append(labels, git.IssueLabel{Name: l.Title})
+		pullRequest.Labels = append(pullRequest.Labels, git.IssueLabel{Name: l.Title})
 	}
 
-	pullRequest := git.PullRequest{ID: data.ObjectAttribute.ID, Title: data.ObjectAttribute.Title, Author: author, URL: data.Project.WebURL, Base: base, Head: head, State: state, Action: action, Labels: labels}
-	return &git.Webhook{EventType: git.EventTypePullRequest, Repo: repo, Sender: sender, PullRequest: &pullRequest}, nil
+	return &git.Webhook{EventType: git.EventTypePullRequest, Repo: repo, PullRequest: &pullRequest, Sender: *sender}, nil
 }
 
 func (c *Client) parsePushWebhook(jsonString []byte) (*git.Webhook, error) {
@@ -114,20 +111,9 @@ func (c *Client) parseIssueComment(jsonString []byte) (*git.Webhook, error) {
 		return nil, nil
 	}
 
-	sender := git.User{
-		ID:    data.User.ID,
-		Name:  data.User.Name,
-		Email: data.User.Email,
-	}
-	var author git.User
-	if sender.ID == data.ObjectAttributes.AuthorID {
-		author = sender
-	} else {
-		user, err := c.GetUserInfo(strconv.Itoa(data.ObjectAttributes.AuthorID))
-		if err != nil {
-			return nil, err
-		}
-		author = *user
+	sender, author, err := c.getSenderAuthor(data.User, data.ObjectAttributes.AuthorID)
+	if err != nil {
+		return nil, err
 	}
 
 	// Get Merge Request user info
@@ -164,7 +150,7 @@ func (c *Client) parseIssueComment(jsonString []byte) (*git.Webhook, error) {
 		Name: data.Project.Name,
 		URL:  data.Project.WebURL,
 	},
-		Sender: sender,
+		Sender: *sender,
 		IssueComment: &git.IssueComment{
 			Comment: git.Comment{
 				Body:      data.ObjectAttributes.Note,
@@ -173,7 +159,7 @@ func (c *Client) parseIssueComment(jsonString []byte) (*git.Webhook, error) {
 			Issue: git.Issue{
 				PullRequest: pr,
 			},
-			Author: author,
+			Author: *author,
 		}}, nil
 }
 
@@ -236,3 +222,48 @@ func (c *Client) parsePullRequestReviewWebhook(data MergeRequestWebhook) (*git.W
 		},
 	}, nil
 }
+
+func (c *Client) getSenderAuthor(senderPre User, authorID int) (*git.User, *git.User, error) {
+	sender := git.User{ID: senderPre.ID, Name: senderPre.Name, Email: senderPre.Email}
+	var author git.User
+	if sender.ID == authorID {
+		author = sender
+	} else {
+		user, err := c.GetUserInfo(strconv.Itoa(authorID))
+		if err != nil {
+			return nil, nil, err
+		}
+		author = *user
+	}
+
+	return &sender, &author, nil
+}
+
+func diffLabels(prev, cur []Label) ([]git.IssueLabel, bool) {
+	var diff []git.IssueLabel
+	isUnlabeled := false
+
+	prevMap := map[string]Label{}
+	curMap := map[string]Label{}
+
+	for _, l := range prev {
+		prevMap[l.Title] = l
+	}
+	for _, l := range cur {
+		curMap[l.Title] = l
+	}
+
+	for _, l := range prev {
+		if _, exist := curMap[l.Title]; !exist {
+			diff = append(diff, git.IssueLabel{Name: l.Title})
+		}
+	}
+	for _, l := range cur {
+		if _, exist := prevMap[l.Title]; !exist {
+			diff = append(diff, git.IssueLabel{Name: l.Title})
+			isUnlabeled = true
+		}
+	}
+
+	return diff, isUnlabeled
+}