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 https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/pipelineascode/match.go#L23 first runs verifyRepoAndUser that calls checkAccessOrErrror https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/pipelineascode/match.go#L138, responsible for checking access. Later getPipelineRunsFromRepo runs that calls MatchPipelineRunByAnnotation, responsible for CEL expressions https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/matcher/annotation_matcher.go#L205

[tnevrlka]

From a quick look, this might be relevant. Here, the MatchPipelinerunByAnnotation responsible for CEL expressions is ran before checkAccessOrErrror https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/pipelineascode/match.go#L229-L239

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.