PAC requests /ok-to-test even if PLR shouldn't run

[MartinBasti]

For PLR with configured CEL expression that shouldn't run for given PR, PAC is requesting approval from maintainers.

Steps to reproduce:

1. Have a user without permissions to run PLRs (PR must be approved by /ok-to-test)
2. Have a PLR definition with CEL expressions, that should skip opened PR
3. User opens PR with changes that doesn't match CEL expressions
4. PAC reports that PR has to /ok-to-test labeled
5. Owner adds /ok-to-test
6. Nothing happens; PAC still shows the same - queued, we cannot get rid of that test status from github

With a user that has permissions, PAC don't report anything.

It seems that RBAC is evaluated before CEL, and asks for permissions even if they aren't needed.

[MartinBasti]

From

pipelines-as-code/pkg/pipelineascode/match.go

Line 23 in a10162c

func (p *PacRun) matchRepoPR(ctx context.Context) ([]matcher.Match, *v1alpha1.Repository, error) {

first runs verifyRepoAndUser that calls checkAccessOrErrror

pipelines-as-code/pkg/pipelineascode/match.go

Line 138 in a10162c

if allowed, err := p.checkAccessOrErrror(ctx, repo, "via "+p.event.TriggerTarget.String()); !allowed {

, responsible for checking access.
Later getPipelineRunsFromRepo runs that calls MatchPipelineRunByAnnotation, responsible for CEL expressions

pipelines-as-code/pkg/matcher/annotation_matcher.go

Line 205 in a10162c

if celExpr, ok := prun.GetObjectMeta().GetAnnotations()[keys.OnCelExpression]; ok {

[tnevrlka]

From a quick look, this might be relevant.
Here, the MatchPipelinerunByAnnotation responsible for CEL expressions is ran before checkAccessOrErrror

pipelines-as-code/pkg/pipelineascode/match.go

Lines 229 to 239 in a10162c

	if matchedPRs, err = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, pipelineRuns, p.run, p.event, p.vcx); err != nil {
	// Don't fail when you don't have a match between pipeline and annotations
	p.eventEmitter.EmitMessage(nil, zap.WarnLevel, "RepositoryNoMatch", err.Error())
	return nil, nil
	}
	}
	// 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 {

Could this be the reason why adding /ok-to-test doesn't do anything?

[zakisk]

fix is covered in #1899 @MartinBasti @tnevrlka please see and let me know if this is what you were looking. Thanks!

[MartinBasti]

it hopefully address the case

[zakisk]

@MartinBasti thank you for confirmation, closing this as done.