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: ignore bot users for blocking pull request when non authorized #1982

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
27 changes: 17 additions & 10 deletions docs/content/docs/guide/running.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,40 +52,47 @@ run a PipelineRun on CI:
owns the repository.
- The author of the pull request has permissions to push to branches inside the
repository.

- The author of the pull request is listed in the `OWNERS` file located in the main
directory of the default branch on GitHub or your other service provider.
(see below for the OWNERS file format).

If the author of the pull request does not have the necessary permissions to run a
PipelineRun, another user who does have the permissions can comment
`/ok-to-test` on the pull request to trigger the PipelineRuns.
If an unauthorized user attempts to trigger a PipelineRun through the creation
of a Pull Request or by any other means, Pipelines-as-Code will block the
execution and post a `'Pending'` status check. This check will inform the user
that they lack the necessary permissions. Only authorized users can initiate the
PipelineRun by commenting `/ok-to-test` on the pull request.

GitHub bot users, identified through the GitHub API, are generally exempt from
the `Pending` status check that would otherwise block a pull request. This
means the status check is silently ignored for bots unless they have been
explicitly authorized (using [OWNERS](#owners-file) file,
[Policy]({{< relref "/docs/guide/policy" >}}) or other means).

## OWNERS file

The `OWNERS` file follows a specific format similar to the Prow `OWNERS` file
format (detailed at <https://www.kubernetes.dev/docs/guide/owners/>). We
support a basic `OWNERS` configuration with `approvers` and `reviewers` lists,
both of which have equal permissions for executing a `PipelineRun`.
both of which have equal permissions for executing a `PipelineRun`.

If the `OWNERS` file uses `filters` instead of a simple configuration, we only
consider the `.*` filter and extract the `approvers` and `reviewers` lists from
it. Any other filters targeting specific files or directories are ignored.
it. Any other filters targeting specific files or directories are ignored.

Additionally, `OWNERS_ALIASES` is supported and allows mapping alias names to
lists of usernames.
lists of usernames.

Including contributors in the `approvers` or `reviewers` lists within your
`OWNERS` file grants them the ability to execute a `PipelineRun` via
Pipelines-as-Code.
Pipelines-as-Code.

For example, if your repository’s `main` or `master` branch contains the
following `approvers` section:
following `approvers` section:

```yaml
approvers:
- approved
```
```

The user with the username `"approved"` will have the necessary
permissions.
Expand Down
28 changes: 16 additions & 12 deletions pkg/pipelineascode/match.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,11 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
// trigger CI on the repository, as any user is able to comment on a pushed commit in open-source repositories.
if p.event.TriggerTarget == triggertype.Push && opscomments.IsAnyOpsEventType(p.event.EventType) {
status := provider.StatusOpts{
Status: CompletedStatus,
Title: "Permission denied",
Conclusion: failureConclusion,
DetailsURL: p.event.URL,
Status: CompletedStatus,
Title: "Permission denied",
Conclusion: failureConclusion,
DetailsURL: p.event.URL,
AccessDenied: true,
}
if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
return nil, err
Expand All @@ -151,10 +152,11 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
// on comment we skip it for now, we are going to check later on
if p.event.TriggerTarget != triggertype.Push && p.event.EventType != opscomments.NoOpsCommentEventType.String() {
status := provider.StatusOpts{
Status: queuedStatus,
Title: "Pending approval, waiting for an /ok-to-test",
Conclusion: pendingConclusion,
DetailsURL: p.event.URL,
Status: queuedStatus,
Title: "Pending approval, waiting for an /ok-to-test",
Conclusion: pendingConclusion,
DetailsURL: p.event.URL,
AccessDenied: true,
}
if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed {
return nil, err
Expand Down Expand Up @@ -266,10 +268,11 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
// we skipped previously so we can get the match from the event to the pipelineruns
if p.event.EventType == opscomments.NoOpsCommentEventType.String() || p.event.EventType == opscomments.OnCommentEventType.String() {
status := provider.StatusOpts{
Status: queuedStatus,
Title: "Pending approval, waiting for an /ok-to-test",
Conclusion: pendingConclusion,
DetailsURL: p.event.URL,
Status: queuedStatus,
Title: "Pending approval, waiting for an /ok-to-test",
Conclusion: pendingConclusion,
DetailsURL: p.event.URL,
AccessDenied: true,
}
if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
return nil, err
Expand Down Expand Up @@ -410,6 +413,7 @@ func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Reposit
}
p.eventEmitter.EmitMessage(repo, zap.InfoLevel, "RepositoryPermissionDenied", msg)
status.Text = msg

if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil {
return false, fmt.Errorf("failed to run create status, user is not allowed to run the CI:: %w", err)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/provider/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type Provider struct {
repo *v1alpha1.Repository
eventEmitter *events.EventEmitter
PaginedNumber int
userType string // The type of user i.e bot or not
skippedRun
}

Expand Down
6 changes: 6 additions & 0 deletions pkg/provider/github/parse_payload.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt
processedEvent.HeadBranch = processedEvent.BaseBranch // in push events Head Branch is the same as Basebranch
processedEvent.BaseURL = gitEvent.GetRepo().GetHTMLURL()
processedEvent.HeadURL = processedEvent.BaseURL // in push events Head URL is the same as BaseURL
v.userType = gitEvent.GetSender().GetType()
case *github.PullRequestEvent:
processedEvent.Repository = gitEvent.GetRepo().GetName()
if gitEvent.GetRepo() == nil {
Expand All @@ -291,6 +292,7 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt
processedEvent.HeadURL = gitEvent.GetPullRequest().Head.GetRepo().GetHTMLURL()
processedEvent.Sender = gitEvent.GetPullRequest().GetUser().GetLogin()
processedEvent.EventType = event.EventType
v.userType = gitEvent.GetPullRequest().GetUser().GetType()

if gitEvent.Action != nil && provider.Valid(*gitEvent.Action, pullRequestLabelEvent) {
processedEvent.EventType = string(triggertype.LabelUpdate)
Expand Down Expand Up @@ -343,6 +345,7 @@ func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.Check
// we allow the rerequest user here, not the push user, i guess it's
// fine because you can't do a rereq without being a github owner?
runevent.Sender = event.GetSender().GetLogin()
v.userType = event.GetSender().GetType()
return runevent, nil
}
runevent.PullRequestNumber = event.GetCheckRun().GetCheckSuite().PullRequests[0].GetNumber()
Expand Down Expand Up @@ -373,6 +376,7 @@ func (v *Provider) handleCheckSuites(ctx context.Context, event *github.CheckSui
// we allow the rerequest user here, not the push user, i guess it's
// fine because you can't do a rereq without being a github owner?
runevent.Sender = event.GetSender().GetLogin()
v.userType = event.GetSender().GetType()
return runevent, nil
// return nil, fmt.Errorf("check suite event is not supported for push events")
}
Expand Down Expand Up @@ -401,6 +405,7 @@ func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.Is
if !event.GetIssue().IsPullRequest() {
return info.NewEvent(), fmt.Errorf("issue comment is not coming from a pull_request")
}
v.userType = event.GetSender().GetType()
opscomments.SetEventTypeAndTargetPR(runevent, event.GetComment().GetBody())
// We are getting the full URL so we have to get the last part to get the PR number,
// we don't have to care about URL query string/hash and other stuff because
Expand All @@ -424,6 +429,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C
runevent.Organization = event.GetRepo().GetOwner().GetLogin()
runevent.Repository = event.GetRepo().GetName()
runevent.Sender = event.GetSender().GetLogin()
v.userType = event.GetSender().GetType()
runevent.URL = event.GetRepo().GetHTMLURL()
runevent.SHA = event.GetComment().GetCommitID()
runevent.HeadURL = runevent.URL
Expand Down
13 changes: 10 additions & 3 deletions pkg/provider/github/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ import (
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
)

const (
botType = "Bot"
pendingApproval = "Pending approval, waiting for an /ok-to-test"
)

const taskStatusTemplate = `
<table>
<tr><th>Status</th><th>Duration</th><th>Name</th></tr>
Expand All @@ -35,8 +40,6 @@ const taskStatusTemplate = `
{{- end }}
</table>`

const pendingApproval = "Pending approval, waiting for an /ok-to-test"

func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Event, status provider.StatusOpts) (*int64, error) {
opt := github.ListOptions{PerPage: v.PaginedNumber}
for {
Expand Down Expand Up @@ -350,6 +353,11 @@ func (v *Provider) CreateStatus(ctx context.Context, runevent *info.Event, statu
return fmt.Errorf("cannot set status on github no token or url set")
}

// If the request comes from a bot user, skip setting the status and just log the event silently
if statusOpts.AccessDenied && v.userType == botType {
return nil
}

switch statusOpts.Conclusion {
case "success":
statusOpts.Title = "Success"
Expand Down Expand Up @@ -386,7 +394,6 @@ func (v *Provider) CreateStatus(ctx context.Context, runevent *info.Event, statu
onPr = "/" + statusOpts.OriginalPipelineRunName
}
statusOpts.Summary = fmt.Sprintf("%s%s %s", v.pacInfo.ApplicationName, onPr, statusOpts.Summary)

// If we have an installationID which mean we have a github apps and we can use the checkRun API
if runevent.InstallationID > 0 {
return v.getOrUpdateCheckRunStatus(ctx, runevent, statusOpts)
Expand Down
48 changes: 47 additions & 1 deletion pkg/provider/github/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ func TestGithubProviderCreateStatus(t *testing.T) {
titleSubstr string
nilCompletedAtDate bool
githubApps bool
accessDenied bool
isBot bool
}
tests := []struct {
name string
Expand Down Expand Up @@ -334,6 +336,36 @@ func TestGithubProviderCreateStatus(t *testing.T) {
want: &github.CheckRun{ID: &resultid},
wantErr: false,
},
{
name: "failure from bot",
args: args{
runevent: runEvent,
status: "completed",
conclusion: "failure",
text: "Nay",
detailsURL: "https://cireport.com",
titleSubstr: "Failed",
githubApps: true,
accessDenied: true,
isBot: true,
},
wantErr: false,
},
{
name: "success from bot",
args: args{
runevent: runEvent,
status: "completed",
conclusion: "failure",
text: "Nay",
detailsURL: "https://cireport.com",
titleSubstr: "Failed",
githubApps: true,
isBot: true,
},
wantErr: false,
want: &github.CheckRun{ID: &resultid},
},
{
name: "skipped",
args: args{
Expand Down Expand Up @@ -378,14 +410,18 @@ func TestGithubProviderCreateStatus(t *testing.T) {
gcvs.Client = fakeclient
gcvs.Logger, _ = logger.GetLogger()
gcvs.Run = params.New()
if tt.args.isBot {
gcvs.userType = "Bot"
}

checkRunCreated := false
mux.HandleFunc("/repos/check/run/statuses/sha", func(_ http.ResponseWriter, _ *http.Request) {})
mux.HandleFunc(fmt.Sprintf("/repos/check/run/check-runs/%d", checkrunid), func(rw http.ResponseWriter, r *http.Request) {
bit, _ := io.ReadAll(r.Body)
checkRun := &github.CheckRun{}
err := json.Unmarshal(bit, checkRun)
assert.NilError(t, err)

checkRunCreated = true
if tt.args.nilCompletedAtDate {
// I guess that's the way you check for an undefined year,
// or maybe i don't understand fully how go works😅
Expand All @@ -402,6 +438,7 @@ func TestGithubProviderCreateStatus(t *testing.T) {
_, err = fmt.Fprintf(rw, `{"id": %d}`, resultid)
assert.NilError(t, err)
})

if tt.addExistingCheckruns {
tt.args.runevent.SHA = "sha"
mux.HandleFunc(fmt.Sprintf("/repos/%v/%v/commits/%v/check-runs", tt.args.runevent.Organization, tt.args.runevent.Repository, tt.args.runevent.SHA), func(w http.ResponseWriter, _ *http.Request) {
Expand Down Expand Up @@ -430,6 +467,7 @@ func TestGithubProviderCreateStatus(t *testing.T) {
Conclusion: tt.args.conclusion,
Text: tt.args.text,
DetailsURL: tt.args.detailsURL,
AccessDenied: tt.args.accessDenied,
}
if tt.pr != nil {
status.PipelineRun = tt.pr
Expand Down Expand Up @@ -469,6 +507,14 @@ func TestGithubProviderCreateStatus(t *testing.T) {
t.Errorf("GithubProvider.CreateStatus() error = %v, wantErr %v", err, tt.wantErr)
return
}
if tt.want == nil && checkRunCreated {
t.Errorf("Check run should have not be created for this test")
return
}
if tt.want != nil && !checkRunCreated {
t.Errorf("Check run should have been created for this test")
return
}
})
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/provider/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type StatusOpts struct {
Summary string
Title string
InstanceCountForCheckRun int
AccessDenied bool
}

type Interface interface {
Expand Down
Loading