ogr: add has_permission() for access-level checks#985
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new has_permission method to the GitProject abstract class and implements it across GitHub, GitLab, Pagure, and Forgejo services. This method allows checking if a user possesses at least a specified AccessLevel, handling forge-specific permission mappings and fallbacks. Review feedback suggests improving the GitLab implementation's compatibility with older python-gitlab versions by safely accessing the members manager. Additionally, an optimization for the Pagure implementation was recommended to avoid redundant API calls and inefficient group expansion during permission checks.
sergio-correia
force-pushed
the
triage
branch
2 times, most recently
from
0a82caf to
dba2e8e
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 19s |
There was a problem hiding this comment.
Thanks a lot for your contribution!
I have only one small change proposal that will hopefully fix the failing rawhide tests.
This is a nicely done first step toward addressing issue #984. However this alone would not fix it. A change in packit-service code is needed for that. Could you please remove "Fixes #984", from the PR description 🙏🏻.
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 18s |
|
Thanks for the review. I updated the PR as per the suggestion, and also updated the description to remove the |
| Note: ``has_permission()`` uses conservative fallbacks for forges | ||
| that lack a triage equivalent — triage maps to push/write level | ||
| on Pagure and Forgejo, not to ticket/read. |
There was a problem hiding this comment.
I'm confused about this comment. Why does the table list ticket/read then? Also, Shouldn't it be commit/write rather than push/write?
There was a problem hiding this comment.
Good catch, fixed. The note now says "requires commit on Pagure and write on Forgejo" instead of the confusing "push/write". The table shows the assignment mapping. The note explains where has_permission() diverges.
I believe they are related. You most likely need to regenerate some test data files, as the error message tells you to - by removing the outdated files, running tests locally and committing the newly generated ones. |
packit-service currently uses can_merge_pr() to decide who may trigger CI commands like /packit test. This requires write or admin access, but GitHub's triage role — which grants issue/PR management without code push rights — is a better fit for the CI trigger use case. There is no ogr method to check "does this user have at least triage-level access?" Add has_permission(username, access_level) to GitProject, implemented for all four forges. Each backend uses its own native permission hierarchy for the "at least" comparison, avoiding the IntEnum ordering mismatch between AccessLevel (maintain=5 > admin=4) and GitHub's actual hierarchy (admin > maintain). Forges that lack a triage equivalent (Pagure, Forgejo) conservatively fall back to the next higher level rather than degrading to read/ticket, which would bypass the gate on public repositories. Assisted-by: Claude Opus 4.6 Signed-off-by: Sergio Correia <scorreia@redhat.com>
The Forgejo instance (v10.next.forgejo.org) is down, returning 404 on all endpoints. Same failure on main, as far as I can see. Can't re-record until it's back. |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 16s |
If test data must be re-generated, I don't understand why only for rawhide and no for other distros. I will try to investigate more on this. |
4 participants
packit-service currently uses can_merge_pr() to decide who may trigger CI commands like /packit test. This requires write or admin access, but GitHub's triage role — which grants issue/PR management without code push rights — is a better fit for the CI trigger use case. There is no ogr method to check "does this user have at least triage-level access?"
Add has_permission(username, access_level) to GitProject, implemented for all four forges. Each backend uses its own native permission hierarchy for the "at least" comparison, avoiding the IntEnum ordering mismatch between AccessLevel (maintain=5 > admin=4) and GitHub's actual hierarchy (admin > maintain). Forges that lack a triage equivalent (Pagure, Forgejo) conservatively fall back to the next higher level rather than degrading to read/ticket, which would bypass the gate on public repositories.
TODO:
packit/packit.dev.Each forge uses its own native permission hierarchy for the "at least"
comparison rather than relying on the AccessLevel IntEnum ordering,
which doesn't match GitHub's hierarchy (admin > maintain, but
AccessLevel.maintain=5 > AccessLevel.admin=4).
Forges without a triage equivalent (Pagure, Forgejo) conservatively
fall back to write/commit level — not read/ticket — to avoid bypassing
the permission gate on public repos. This means
ACCESS_LEVEL_TO*_PERM intentionally diverges from the existing
access_dict for triage on those forges.
RELEASE NOTES BEGIN
ogr now provides has_permission(username, access_level) to check
whether a user has at least a given access level on a repository,
enabling finer-grained permission checks (e.g., triage) beyond
the existing can_merge_pr().
RELEASE NOTES END