Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(azure-devops): add check for test webhook URL #2809

Merged
merged 20 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions server/controllers/events/events_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ const bitbucketCloudRequestIDHeader = "X-Request-UUID"
const bitbucketServerRequestIDHeader = "X-Request-ID"
const bitbucketServerSignatureHeader = "X-Hub-Signature"

// The URL used for Azure DevOps test webhooks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this PR been tested in your atlantis deployment? Were you able to verify that this change worked as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll build it and run through some tests - hold off until I've confirmed that's sorted

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Once you confirm then we can merge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SSKLCP friendly ping

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SSKLCP friendly ping

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SSKLCP friendly ping

const azuredevopsTestURL = "https://fabrikam.visualstudio.com/DefaultCollection/_apis/git/repositories/4bc14d40-c903-45e2-872e-0462c7748079"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this url has to be hardcoded?

Shouldn't this value be in the test instead?

Who has access to use this url for testing purposes?

Couldn't this be mocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is to detect and ignore the test webhooks that come from Azure DevOps. The test requests for the 3 webhooks that Atlantis uses are in the PR description. (The tests aren't configurable, they just send the same requests every time)

The URL is a common property that exists in all three requests so we're using it to say: "If the request says it's coming from this URL: Ignore the request"

No access is needed to this URL, we're simply using it to identify these test requests.

Does that sound okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we'll ever have to configure these urls?

Would you add some additional comments in the code to add this explaination in case people have this question in the future?


// VCSEventsController handles all webhook requests which signify 'events' in the
// VCS host, ex. GitHub.
type VCSEventsController struct {
Expand Down Expand Up @@ -639,6 +642,11 @@ func (e *VCSEventsController) HandleAzureDevopsPullRequestCommentedEvent(w http.
return
}

if isAzureDevOpsTestRepoURL(resource.PullRequest.GetRepository()) {
e.respond(w, logging.Debug, http.StatusOK, "Ignoring Azure DevOps Test Event with Repo URL: %v %s", resource.PullRequest.Repository.URL, azuredevopsReqID)
return
}

createdBy := resource.PullRequest.GetCreatedBy()
user := models.User{Username: createdBy.GetUniqueName()}
baseRepo, err := e.Parser.ParseAzureDevopsRepo(resource.PullRequest.GetRepository())
Expand Down Expand Up @@ -681,6 +689,16 @@ func (e *VCSEventsController) HandleAzureDevopsPullRequestEvent(w http.ResponseW
}
}

resource, ok := event.Resource.(*azuredevops.GitPullRequest)
if !ok || event.PayloadType != azuredevops.PullRequestEvent {
e.respond(w, logging.Error, http.StatusBadRequest, "Event.Resource is nil or received bad event type %v; %s", event.Resource, azuredevopsReqID)
return
}
if isAzureDevOpsTestRepoURL(resource.GetRepository()) {
e.respond(w, logging.Debug, http.StatusOK, "Ignoring Azure DevOps Test Event with Repo URL: %v %s", resource.Repository.URL, azuredevopsReqID)
return
}

pull, pullEventType, baseRepo, headRepo, user, err := e.Parser.ParseAzureDevopsPullEvent(*event)
if err != nil {
e.respond(w, logging.Error, http.StatusBadRequest, "Error parsing pull data: %s %s", err, azuredevopsReqID)
Expand Down Expand Up @@ -730,3 +748,10 @@ func (e *VCSEventsController) commentNotAllowlisted(baseRepo models.Repo, pullNu
e.Logger.Err("unable to comment on pull request: %s", err)
}
}

func isAzureDevOpsTestRepoURL(repository *azuredevops.GitRepository) bool {
if repository == nil {
return false
}
return repository.GetURL() == azuredevopsTestURL
}
95 changes: 95 additions & 0 deletions server/controllers/events/events_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,101 @@ func TestPost_AzureDevopsPullRequestDeletedCommentIgnoreEvent(t *testing.T) {
})
}

func TestPost_AzureDevopsPullRequestCommentWebhookTestIgnoreEvent(t *testing.T) {
t.Log("when the event is an azure devops webhook test we ignore it")
e, _, _, ado, _, _, _, _, _ := setup(t)

event := `{
"subscriptionId": "11111111-1111-1111-1111-111111111111",
"notificationId": 1,
"id": "22222222-2222-2222-2222-222222222222",
"eventType": "%s",
"publisherId": "tfs",
"message": {
"text": "%s"
},
"resource": {
"pullRequest": {
"repository":{
"url": "https://fabrikam.visualstudio.com/DefaultCollection/_apis/git/repositories/4bc14d40-c903-45e2-872e-0462c7748079"
}
},
"comment": {
"content": "This is my comment."
}
}}`

cases := []struct {
eventType string
message string
}{
{
"ms.vss-code.git-pullrequest-comment-event",
"Jamal Hartnett has edited a pull request comment",
},
}

for _, c := range cases {
t.Run(c.message, func(t *testing.T) {
payload := fmt.Sprintf(event, c.eventType, c.message)
req, _ := http.NewRequest("GET", "", strings.NewReader(payload))
req.Header.Set(azuredevopsHeader, "reqID")
When(ado.Validate(req, user, secret)).ThenReturn([]byte(payload), nil)
w := httptest.NewRecorder()
e.Parser = &events.EventParser{}
e.Post(w, req)
ResponseContains(t, w, http.StatusOK, "Ignoring Azure DevOps Test Event with Repo URL")
})
}
}

func TestPost_AzureDevopsPullRequestWebhookTestIgnoreEvent(t *testing.T) {
t.Log("when the event is an azure devops webhook tests we ignore it")
e, _, _, ado, _, _, _, _, _ := setup(t)

event := `{
"subscriptionId": "11111111-1111-1111-1111-111111111111",
"notificationId": 1,
"id": "22222222-2222-2222-2222-222222222222",
"eventType": "%s",
"publisherId": "tfs",
"message": {
"text": "%s"
},
"resource": {
"repository":{
"url": "https://fabrikam.visualstudio.com/DefaultCollection/_apis/git/repositories/4bc14d40-c903-45e2-872e-0462c7748079"
}
}}`

cases := []struct {
eventType string
message string
}{
{
"git.pullrequest.created",
"Jamal Hartnett created a new pull request",
},
{
"git.pullrequest.updated",
"Jamal Hartnett marked the pull request as completed",
},
}

for _, c := range cases {
t.Run(c.message, func(t *testing.T) {
payload := fmt.Sprintf(event, c.eventType, c.message)
req, _ := http.NewRequest("GET", "", strings.NewReader(payload))
req.Header.Set(azuredevopsHeader, "reqID")
When(ado.Validate(req, user, secret)).ThenReturn([]byte(payload), nil)
w := httptest.NewRecorder()
e.Parser = &events.EventParser{}
e.Post(w, req)
ResponseContains(t, w, http.StatusOK, "Ignoring Azure DevOps Test Event with Repo URL")
})
}
}

func TestPost_AzureDevopsPullRequestCommentPassingIgnores(t *testing.T) {
t.Log("when the event should not be ignored it should pass through all ignore statements without error")
e, _, _, ado, _, _, _, _, _ := setup(t)
Expand Down