Skip to content

Commit c3c08dd

Browse files
chmouelchmouel
committed
Merge branch 'main' into issue-2664-same-repo-shortcut-comment-sender
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
2 parents 34cb0e8 + de6de63 commit c3c08dd

File tree

7 files changed

+138
-62
lines changed

7 files changed

+138
-62
lines changed

‎pkg/provider/github/acl.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,11 @@ func (v *Provider) aclCheckAll(ctx context.Context, rev *info.Event) (bool, erro
211211
if isFromSameRepo {
212212
return true, nil
213213
}
214+
} else if rev.PullRequestNumber != 0 && v.Logger != nil {
215+
v.Logger.Debugf(
216+
"Skipping same-repo pull request shortcut for untrusted event %T on %s/%s#%d from sender %s",
217+
rev.Event, rev.Organization, rev.Repository, rev.PullRequestNumber, rev.Sender,
218+
)
214219
}
215220

216221
// If the user who has submitted the pr is a owner on the repo then allows

‎pkg/provider/github/acl_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -816,3 +816,35 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
816816
})
817817
}
818818
}
819+
820+
func TestACLCheckAllIssueCommentLogsShortcutSkip(t *testing.T) {
821+
fakeclient, _, _, teardown := ghtesthelper.SetupGH()
822+
defer teardown()
823+
824+
ctx, _ := rtesting.SetupFakeContext(t)
825+
repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{
826+
Settings: &v1alpha1.Settings{},
827+
}}
828+
observer, logs := zapobserver.New(zap.DebugLevel)
829+
830+
gprovider := Provider{
831+
ghClient: fakeclient,
832+
repo: repo,
833+
Logger: zap.New(observer).Sugar(),
834+
}
835+
836+
allowed, err := gprovider.aclCheckAll(ctx, &info.Event{
837+
Organization: "owner",
838+
Sender: "nonowner",
839+
Repository: "repo",
840+
PullRequestNumber: 1,
841+
HeadURL: "http://org.com/owner/repo",
842+
BaseURL: "http://org.com/owner/repo",
843+
HeadBranch: "main",
844+
BaseBranch: "dependabot",
845+
Event: &github.IssueCommentEvent{},
846+
})
847+
assert.NilError(t, err)
848+
assert.Equal(t, false, allowed)
849+
assert.Assert(t, logs.FilterMessageSnippet("Skipping same-repo pull request shortcut for untrusted event").Len() == 1)
850+
}

‎pkg/provider/github/detect.go‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,11 @@ func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any
7676
}
7777
return "", fmt.Sprintf("pull_request: unsupported action \"%s\"", event.GetAction())
7878
case *github.IssueCommentEvent:
79-
if event.GetAction() == "created" &&
80-
event.GetIssue().IsPullRequest() &&
79+
if event.GetAction() != "created" {
80+
return "", fmt.Sprintf("issue_comment: unsupported action \"%s\"", event.GetAction())
81+
}
82+
83+
if event.GetIssue().IsPullRequest() &&
8184
event.GetIssue().GetState() == "open" {
8285
if opscomments.IsTestRetestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) {
8386
return triggertype.Retest, ""
@@ -101,18 +104,20 @@ func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any
101104
}
102105
return "", fmt.Sprintf("check_run: unsupported action \"%s\"", event.GetAction())
103106
case *github.CommitCommentEvent:
104-
if event.GetAction() == "created" {
105-
if opscomments.IsTestRetestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) {
106-
return triggertype.Retest, ""
107-
}
108-
if opscomments.IsCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) {
109-
return triggertype.Cancel, ""
110-
}
111-
// Here, the `/ok-to-test` command is ignored because it is intended for pull requests.
112-
// For unauthorized users, it has no relevance to pushed commits, as only authorized users
113-
// are allowed to run CI on pushed commits. Therefore, the `ok-to-test` command holds no significance in this context.
114-
// However, it is left to be processed by the `on-comment` annotation rather than returning an error.
107+
if event.GetAction() != "created" {
108+
return "", fmt.Sprintf("commit_comment: unsupported action \"%s\"", event.GetAction())
109+
}
110+
111+
if opscomments.IsTestRetestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) {
112+
return triggertype.Retest, ""
113+
}
114+
if opscomments.IsCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) {
115+
return triggertype.Cancel, ""
115116
}
117+
// Here, the `/ok-to-test` command is ignored because it is intended for pull requests.
118+
// For unauthorized users, it has no relevance to pushed commits, as only authorized users
119+
// are allowed to run CI on pushed commits. Therefore, the `ok-to-test` command holds no significance in this context.
120+
// However, it is left to be processed by the `on-comment` annotation rather than returning an error.
116121
return triggertype.Comment, ""
117122
}
118123
return "", fmt.Sprintf("github: event \"%v\" is not supported", ghEventType)

‎pkg/provider/github/detect_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,10 @@ func TestProviderDetect(t *testing.T) {
7474
event: github.CommitCommentEvent{
7575
Action: github.Ptr("something"),
7676
},
77+
wantReason: "commit_comment: unsupported action \"something\"",
7778
eventType: "commit_comment",
7879
isGH: true,
79-
processReq: true,
80+
processReq: false,
8081
},
8182
{
8283
name: "invalid check run Event",
@@ -92,9 +93,10 @@ func TestProviderDetect(t *testing.T) {
9293
event: github.IssueCommentEvent{
9394
Action: github.Ptr("deleted"),
9495
},
96+
wantReason: "issue_comment: unsupported action \"deleted\"",
9597
eventType: "issue_comment",
9698
isGH: true,
97-
processReq: true,
99+
processReq: false,
98100
},
99101
{
100102
name: "issue comment Event with no valid comment",

‎pkg/provider/github/parse_payload.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,9 +353,6 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt
353353
return nil, fmt.Errorf("no github client has been initialized, " +
354354
"exiting... (hint: did you forget setting a secret on your repo?)")
355355
}
356-
if gitEvent.GetAction() != "created" {
357-
return nil, fmt.Errorf("only newly created comment is supported, received: %s", gitEvent.GetAction())
358-
}
359356
processedEvent, err = v.handleIssueCommentEvent(ctx, gitEvent)
360357
if err != nil {
361358
return nil, err

‎pkg/provider/github/parse_payload_test.go‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -744,14 +744,6 @@ func TestParsePayLoad(t *testing.T) {
744744
},
745745
},
746746
},
747-
{
748-
name: "bad/issue_comment_not_from_created",
749-
wantErrString: "only newly created comment is supported, received: deleted",
750-
payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("deleted")},
751-
eventType: "issue_comment",
752-
triggerTarget: "pull_request",
753-
githubClient: true,
754-
},
755747
{
756748
name: "good/issue comment",
757749
eventType: "issue_comment",

‎test/github_pullrequest_oktotest_test.go‎

Lines changed: 79 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,15 @@ import (
77
"fmt"
88
"net/http"
99
"os"
10+
"regexp"
1011
"strconv"
1112
"testing"
13+
"time"
1214

1315
"github.com/google/go-github/v84/github"
16+
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
1417
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
18+
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx"
1519
tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
1620
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
1721
twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
@@ -41,60 +45,99 @@ func TestGithubGHEPullRequestOkToTest(t *testing.T) {
4145
Organization: g.Options.Organization,
4246
Repository: g.Options.Repo,
4347
URL: repoinfo.GetHTMLURL(),
44-
Sender: g.Options.Organization,
4548
}
4649

50+
repo, err := g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
51+
assert.NilError(t, err)
52+
initialStatusCount := len(repo.Status)
53+
54+
pruns, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{
55+
LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA),
56+
})
57+
assert.NilError(t, err)
58+
initialPipelineRunCount := len(pruns.Items)
59+
4760
installID, err := strconv.ParseInt(os.Getenv("TEST_GITHUB_SECOND_APPLICATION_ID"), 10, 64)
4861
assert.NilError(t, err)
49-
event := github.IssueCommentEvent{
50-
Comment: &github.IssueComment{
51-
Body: github.Ptr(`/ok-to-test`),
52-
},
53-
Installation: &github.Installation{
54-
ID: &installID,
55-
},
56-
Action: github.Ptr("created"),
57-
Issue: &github.Issue{
58-
State: github.Ptr("open"),
59-
PullRequestLinks: &github.PullRequestLinks{
60-
HTMLURL: github.Ptr(fmt.Sprintf("%s/pull/%d", runevent.URL, g.PRNumber)),
62+
63+
sendIssueComment := func(t *testing.T, sender string) {
64+
t.Helper()
65+
66+
event := github.IssueCommentEvent{
67+
Comment: &github.IssueComment{
68+
Body: github.Ptr(`/ok-to-test`),
69+
},
70+
Installation: &github.Installation{
71+
ID: &installID,
72+
},
73+
Action: github.Ptr("created"),
74+
Issue: &github.Issue{
75+
State: github.Ptr("open"),
76+
PullRequestLinks: &github.PullRequestLinks{
77+
HTMLURL: github.Ptr(fmt.Sprintf("%s/pull/%d", runevent.URL, g.PRNumber)),
78+
},
79+
},
80+
Repo: &github.Repository{
81+
DefaultBranch: &runevent.DefaultBranch,
82+
HTMLURL: &runevent.URL,
83+
Name: &runevent.Repository,
84+
Owner: &github.User{Login: &runevent.Organization},
85+
},
86+
Sender: &github.User{
87+
Login: github.Ptr(sender),
6188
},
62-
},
63-
Repo: &github.Repository{
64-
DefaultBranch: &runevent.DefaultBranch,
65-
HTMLURL: &runevent.URL,
66-
Name: &runevent.Repository,
67-
Owner: &github.User{Login: &runevent.Organization},
68-
},
69-
Sender: &github.User{
70-
Login: &runevent.Sender,
71-
},
89+
}
90+
91+
err = payload.Send(ctx,
92+
g.Cnx,
93+
os.Getenv("TEST_GITHUB_SECOND_EL_URL"),
94+
os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SECRET"),
95+
os.Getenv("TEST_GITHUB_SECOND_API_URL"),
96+
os.Getenv("TEST_GITHUB_SECOND_APPLICATION_ID"),
97+
event,
98+
"issue_comment",
99+
)
100+
assert.NilError(t, err)
72101
}
73102

74-
err = payload.Send(ctx,
75-
g.Cnx,
76-
os.Getenv("TEST_GITHUB_SECOND_EL_URL"),
77-
os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SECRET"),
78-
os.Getenv("TEST_GITHUB_SECOND_API_URL"),
79-
os.Getenv("TEST_GITHUB_SECOND_APPLICATION_ID"),
80-
event,
81-
"issue_comment",
82-
)
103+
g.Cnx.Clients.Log.Infof("Sending /ok-to-test from untrusted sender on same-repo pull request")
104+
sendIssueComment(t, "nonowner")
105+
106+
time.Sleep(10 * time.Second)
107+
108+
pruns, err = g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{
109+
LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA),
110+
})
111+
assert.NilError(t, err)
112+
assert.Equal(t, initialPipelineRunCount, len(pruns.Items), "untrusted issue_comment must not create a new PipelineRun")
113+
114+
repo, err = g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
83115
assert.NilError(t, err)
116+
assert.Equal(t, initialStatusCount, len(repo.Status), "untrusted issue_comment must not add a new Repository status")
84117

85-
g.Cnx.Clients.Log.Infof("Wait for the second repository update to be updated")
118+
ctx, err = cctx.GetControllerCtxInfo(ctx, g.Cnx)
119+
assert.NilError(t, err)
120+
numLines := int64(1000)
121+
logRegex := regexp.MustCompile(`Skipping same-repo pull request shortcut for untrusted event \*github\.IssueCommentEvent`)
122+
err = twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *logRegex, 10, "ghe-controller", &numLines, nil)
123+
assert.NilError(t, err)
124+
125+
g.Cnx.Clients.Log.Infof("Sending /ok-to-test from trusted sender")
126+
sendIssueComment(t, g.Options.Organization)
127+
128+
g.Cnx.Clients.Log.Infof("Wait for the repository update triggered by trusted /ok-to-test")
86129
waitOpts := twait.Opts{
87130
RepoName: g.TargetNamespace,
88131
Namespace: g.TargetNamespace,
89-
MinNumberStatus: 1,
132+
MinNumberStatus: initialStatusCount + 1,
90133
PollTimeout: twait.DefaultTimeout,
91134
TargetSHA: g.SHA,
92135
}
93136
_, err = twait.UntilRepositoryUpdated(ctx, g.Cnx.Clients, waitOpts)
94137
assert.NilError(t, err)
95138

96-
g.Cnx.Clients.Log.Infof("Check if we have the repository set as succeeded")
97-
repo, err := g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
139+
g.Cnx.Clients.Log.Infof("Check if we have the repository set as succeeded after trusted /ok-to-test")
140+
repo, err = g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
98141
assert.NilError(t, err)
99142
assert.Assert(t, repo.Status[len(repo.Status)-1].Conditions[0].Status == corev1.ConditionTrue)
100143
}

0 commit comments

Comments
 (0)