Fix GitOps Command Issue on Pushed Commit by Unautorized User #1922
Conversation
zakisk
force-pushed
the
fix-gitops-command-issue-for-external-user
branch
from
2668911 to
9956fdd
Compare
|
/test |
|
After some thought, I think we really need to try to do the e2e test like done here: https://github.com/chmouel/pipelines-as-code/blob/main/test/github_pullrequest_oktotest_test.go#L79-L88. It shouldn't be too hard using that same technique, and since this is sensitive, we want to cover it. |
zakisk
force-pushed
the
fix-gitops-command-issue-for-external-user
branch
from
9956fdd to
ee59992
Compare
|
seems like it fails on --- FAIL: TestGiteaPolicyPullRequest (266.04s) |
zakisk
force-pushed
the
fix-gitops-command-issue-for-external-user
branch
from
8c343bc to
4c7784e
Compare
should work on 🚀 |
zakisk
force-pushed
the
fix-gitops-command-issue-for-external-user
branch
from
4c7784e to
82e1020
Compare
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]>
zakisk
force-pushed
the
fix-gitops-command-issue-for-external-user
branch
from
82e1020 to
69ade58
Compare
|
/test |
|
/retest |
|
ci error will be fixed by #1933 |
|
/lable fix |
didn't work for me 😕 |
|
/rebase |
|
✅ Rebased the PR branch on the base branch. |
|
/rebase |
|
✅ Rebased the PR branch on the base branch. |
|
/merge |
❌ Insufficient Approvals
Please obtain additional approvals before merging. |
|
/lgtm |
|
/merge |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Pull Request Approved
Approval Status:
- Required Approvals: 1
- Current Approvals: 1
👥 Approved By:
| Reviewer | Permission | Status |
|---|---|---|
| @chmouel | admin |
✅ |
📝 Next Steps
- All required checks must pass
- Branch protection rules apply
- Get a maintainer to use the
/mergecommand to merge the PR
Thank you for your contributions! 🎉
pipelines-as-code
bot
merged commit d51c912
into
openshift-pipelines:main
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
Changes
Submitter Checklist
📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).
♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.
✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).
📖 Document any user-facing features or changes in behavior.
🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.
🎁 If feasible, add an end-to-end test. See README for details.
🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).
If adding a provider feature, fill in the following details:
(update the documentation accordingly)