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. |
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. |
|
Forgejo test failing on fedora rawhide should be fixed by: #986 |
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>
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 19s |
mfocko
left a comment
mfocko
left a comment
There was a problem hiding this comment.
Hi, sorry for the late response.
We've discussed this PR with @majamassarini off GitHub, since there were some issue that I anticipated coming up :/
Overall I kind of understand the point of not exposing the maintainers that have higher privileges, at least from the security perspective.
I have tested locally what you can and cannot do with your implementation:
- GitHub PAT (personal access token) — you can check “permissions”¹ of repos that you own (potentially admin, I did not inspect it with such granularity), otherwise you get
403² - GitHub App — generally seems to be working, yields 403 for suspended repos and a separate exception for non-enabled repos (where the App has not been installed)
- GitLab — seems OK
- Forgejo — allows admins (of instance; based on my understanding) to query all permissions, repo admins to query on repo and finally contributors to query only their own
- Pagure — not investigated; IIRC the use-case for this are permission checks on the upstream forges, we do not support Pagure as an upstream forge
The limitations should be clearly documented IMO, especially since they are different for each forge and also have their own nuances, e.g., PAT vs GitHub App.
As for the testing, I'm, as always, leaning towards consistent tests here, therefore integration tests and trying to avoid mocking as much as possible. OTOH in this case, I'm afraid, it doesn't really help anyways, because we generally test on repos that we own (easier to perform actions, no PITA with the privileges), but in this case… it kinda hides the issues I have listed above.
It would be for the best to have integration tests, so that we are sure the functionality works as intended at least. I am aware the testing experience with requre ain't the best, so after we're OK with the implementation/docs, we can possibly take it over to speed it up and alleviate the pain on your end :)
Overall, apart from tests and the design issues of the forges, LGTM :) Thanks a lot for your contribution.
¹ NB with GitHub the permissions are quite complicated… GitHub supports (original) branch protections which are quite complex (you can allow specific people, specific CI checks, bypassing, etc.) and additionally they have moved on to “rulesets” which work in a similar way. In general I doubt it's possible to replicate the behaviour that GitHub allows (I don't think it's safe (for GitHub) to expose branch protections / rulesets outside of the repo owners)
² That 403 is quite annoying, because of another GitHub's decision to use 403 also for rate-limiting which we specifically try to handle. I'm not even going to start the rant on the fact that during my testing when I got 403 I also got the rate-limit header to suggest a retry after 50 minutes or so. I am not sure what the “engineers” at GitHub are smoking, but it's pretty good stuff. (And it also justifies projects moving away from GitHub…)
| collaborator=username, | ||
| ).permission in ("owner", "admin", "write") | ||
|
|
||
| def has_permission(self, username: str, access_level: AccessLevel) -> bool: |
There was a problem hiding this comment.
This will very likely be an issue. Trying it out locally I have managed to get the following error message out of it:
status_code: 403
message: >
Only admins can query all permissions, repo admins can query all repo
permissions, collaborators can query only their own
Unfortunately the related issue (to this PR) of service is not linked, but in my perspective this will be a big blocker for supporting Forgejo as an upstream forge in Packit Service, as this also goes against our least privilege “rule” for repos where we provide CI, e.g., on dist-git we do not possess any privileges.
| in self.CAN_MERGE_PERMS | ||
| ) | ||
|
|
||
| def has_permission(self, username: str, access_level: AccessLevel) -> bool: |
There was a problem hiding this comment.
Also, based on the local testing, similar issue as with the Forgejo.
Packit Service is OK here, since it is a GitHub App. However for general usage, with a classic PAT (personal access token) this endpoint yields 403.
The 403 status is even bigger issue, because of the utterly poor design choices done by GitHub to return 403 even for rate-limiting which classifies this Forbidden status code as to be retried in ogr, basically resulting in an infinite loop…
This should be definitely reflected in docs, if not checked manually and handled with a proper exception (IIUC only repo admins, same as with Forgejo, are allowed to query this endpoint).
| users = self.service.gitlab_instance.users.list(username=username) | ||
| user = next( | ||
| (u for u in users if u.username.lower() == username.lower()), | ||
| None, | ||
| ) |
There was a problem hiding this comment.
| users = self.service.gitlab_instance.users.list(username=username) | |
| user = next( | |
| (u for u in users if u.username.lower() == username.lower()), | |
| None, | |
| ) | |
| users = self.service.gitlab_instance.users.list(username=username, iterator=True) | |
| user = next(users, None) |
Also, just out of interest, why is there conversion from a list to a generator including a filtering? I would've expected the .list(username=username) to handle the filtering?
| member = member_manager.get(user.id) | ||
| member_access = ( | ||
| member["access_level"] | ||
| if isinstance(member, dict) |
| | `which_groups_can_merge_pr` | ✘ | ✘ | ✔ | ✘ | | ||
| | `get_pr_files_diff` | ✘ | ✘ | ✔ | ✘ | | ||
| | `get_users_with_given_access` | ✘ | ✘ | ✔ | ✔ | | ||
| | `has_permission` | ✔ | ✔ | ✔ | ✔ | |
There was a problem hiding this comment.
No need to add this, the table is used only when there are features that are missing in some forges, i.e., at least one forge doesn't support the feature.
| _PAGURE_PERM_RANK: ClassVar[dict[str, int]] = { | ||
| "ticket": 0, | ||
| "commit": 1, | ||
| "admin": 2, | ||
| "owner": 3, | ||
| } | ||
|
|
||
| # Intentionally differs from access_dict: triage maps to "commit" | ||
| # (not "ticket") and admin maps to "admin" (not "commit") because | ||
| # has_permission uses "at least" threshold semantics rather than | ||
| # the assignment semantics of access_dict. | ||
| _ACCESS_LEVEL_TO_PAGURE_PERM: ClassVar[dict[AccessLevel, str]] = { | ||
| AccessLevel.pull: "ticket", | ||
| AccessLevel.triage: "commit", | ||
| AccessLevel.push: "commit", | ||
| AccessLevel.admin: "admin", | ||
| AccessLevel.maintain: "admin", | ||
| } | ||
|
|
There was a problem hiding this comment.
IMO it would probably make sense to make a Pagure-only dataclass… oh i see… ew… never mind
5 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