Fix GitOps Command Issue on Pushed Commit by Unautorized User

Issue: when an unautorized user sends GitOps comment on a pushed commit,
PAC is triggering CI since access check is done only for pull_request
event in verifyRepoAndUser func of controller.

Solution: added a check for push event and Ops comment event
type in verifyRepoAndUser func.

https://issues.redhat.com/browse/SRVKP-7110

Signed-off-by: Zaki Shaikh <[email protected]>

Author: zakisk

pkg/pipelineascode/match.go

@@ -132,11 +132,31 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
 		return repo, err
 	}
 
+	// Verify whether the sender of the GitOps command (e.g., /test) has the appropriate permissions to
+	// 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,
+		}
+		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
+			return nil, err
+		}
+	}
+
 	// Check if the submitter is allowed to run this.
 	// on push we don't need to check the policy since the user has pushed to the repo so it has access to it.
 	// 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() {
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, "via "+p.event.TriggerTarget.String()); !allowed {
+		status := provider.StatusOpts{
+			Status:     queuedStatus,
+			Title:      "Pending approval, waiting for an /ok-to-test",
+			Conclusion: pendingConclusion,
+			DetailsURL: p.event.URL,
+		}
+		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed {
 			return nil, err
 		}
 	}
@@ -245,7 +265,13 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 	// if the event is a comment event, but we don't have any match from the keys.OnComment then do the ACL checks again
 	// 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() {
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, "by gitops comment"); !allowed {
+		status := provider.StatusOpts{
+			Status:     queuedStatus,
+			Title:      "Pending approval, waiting for an /ok-to-test",
+			Conclusion: pendingConclusion,
+			DetailsURL: p.event.URL,
+		}
+		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
 			return nil, err
 		}
 	}
@@ -370,26 +396,20 @@ func (p *PacRun) checkNeedUpdate(_ string) (string, bool) {
 	return "", false
 }
 
-func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, viamsg string) (bool, error) {
+func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) {
 	allowed, err := p.vcx.IsAllowed(ctx, p.event)
 	if err != nil {
 		return false, err
 	}
 	if allowed {
 		return true, nil
 	}
-	msg := fmt.Sprintf("User %s is not allowed to trigger CI %s on this repo.", p.event.Sender, viamsg)
+	msg := fmt.Sprintf("User %s is not allowed to trigger CI %s in this repo.", p.event.Sender, viamsg)
 	if p.event.AccountID != "" {
-		msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s on this repo.", p.event.Sender, p.event.AccountID, viamsg)
+		msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s in this repo.", p.event.Sender, p.event.AccountID, viamsg)
 	}
 	p.eventEmitter.EmitMessage(repo, zap.InfoLevel, "RepositoryPermissionDenied", msg)
-	status := provider.StatusOpts{
-		Status:     queuedStatus,
-		Title:      "Pending approval, needs /ok-to-test",
-		Conclusion: pendingConclusion,
-		Text:       msg,
-		DetailsURL: p.event.URL,
-	}
+	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)
 	}

pkg/pipelineascode/pipelineascode_test.go

@@ -465,6 +465,24 @@ func TestRun(t *testing.T) {
 			finalStatusText:          "PipelineRun has no taskruns",
 			expectedNumberofCleanups: 10,
 		},
+		{
+			name: "Do not allow unauthorized user to run CI on pushed commit",
+			runevent: info.Event{
+				SHA:           "principale",
+				Organization:  "organizationes",
+				Repository:    "lagaffe",
+				URL:           "https://service/documentation",
+				HeadBranch:    "main",
+				BaseBranch:    "main",
+				Sender:        "fantasio",
+				EventType:     "test-all-comment",
+				TriggerTarget: "push",
+			},
+			tektondir:                    "testdata/push_branch",
+			finalStatus:                  "failure",
+			finalStatusText:              "User fantasio is not allowed to trigger CI by GitOps comment on push commit in this repo.",
+			skipReplyingOrgPublicMembers: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/provider/github/status.go

@@ -35,7 +35,7 @@ const taskStatusTemplate = `
 {{- end }}
 </table>`
 
-const pendingApproval = "Pending approval, needs /ok-to-test"
+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}

pkg/provider/github/status_test.go

@@ -414,7 +414,7 @@ func TestGithubProviderCreateStatus(t *testing.T) {
                                 "status": "queued",
                                 "conclusion": "pending", 
 								"output": {
-									"title": "Pending approval, needs /ok-to-test",
+									"title": "Pending approval, waiting for an /ok-to-test",
 									"summary": "My CI is waiting for approval"
 								}
 							}

test/gitea_access_control_test.go

@@ -142,7 +142,7 @@ func TestGiteaPolicyOkToTestRetest(t *testing.T) {
 	topts.GiteaCNX = normalUserCnx
 	tgitea.PostCommentOnPullRequest(t, topts, okToTestComment)
 	topts.CheckForStatus = "Skipped"
-	topts.Regexp = regexp.MustCompile(fmt.Sprintf(".*User %s is not allowed to trigger CI via pull_request on this repo.", normalUser.UserName))
+	topts.Regexp = regexp.MustCompile(fmt.Sprintf(".*User %s is not allowed to trigger CI via pull_request in this repo.", normalUser.UserName))
 	tgitea.WaitForPullRequestCommentMatch(t, topts)
 
 	topts.ParamsRun.Clients.Log.Infof("Sending a /retest comment as a user not belonging to an allowed team in Repo CR policy but part of the organization")

test/pkg/gitea/test.go

@@ -404,7 +404,7 @@ func WaitForStatus(t *testing.T, topts *TestOpts, ref, forcontext string, onlyla
 		}
 		for _, cstatus := range statuses {
 			if topts.CheckForStatus == "Skipped" {
-				if strings.HasSuffix(cstatus.Description, "Pending approval, needs /ok-to-test") {
+				if strings.HasSuffix(cstatus.Description, "Pending approval, waiting for an /ok-to-test") {
 					numstatus++
 					break
 				}