Skip to content

Commit 92ab36d

Browse files
ogr: add has_permission() for access-level checks
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>
1 parent b41306d commit 92ab36d

11 files changed

Lines changed: 547 additions & 9 deletions

File tree

COMPATIBILITY.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ In case you find any error, please [create a new issue](https://github.com/packi
6363
| `which_groups_can_merge_pr` |||||
6464
| `get_pr_files_diff` |||||
6565
| `get_users_with_given_access` |||||
66+
| `has_permission` |||||
6667

6768
## User
6869

ogr/abstract/access_level.py

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,17 @@ class AccessLevel(IntEnum):
88
"""
99
Enumeration representing an access level to the repository.
1010
11-
| Value from enumeration | GitHub | GitLab | Pagure |
12-
| ---------------------- | -------- | ----------------------- | ------ |
13-
| `AccessLevel.pull` | pull | guest | ticket |
14-
| `AccessLevel.triage` | triage | reporter | ticket |
15-
| `AccessLevel.push` | push | developer | commit |
16-
| `AccessLevel.admin` | admin | maintainer | commit |
17-
| `AccessLevel.maintain` | maintain | owner (only for groups) | admin |
11+
| Value from enumeration | GitHub | GitLab | Pagure | Forgejo |
12+
| ---------------------- | -------- | ----------------------- | ------ | ------- |
13+
| `AccessLevel.pull` | pull | guest | ticket | read |
14+
| `AccessLevel.triage` | triage | reporter | ticket | read |
15+
| `AccessLevel.push` | push | developer | commit | write |
16+
| `AccessLevel.admin` | admin | maintainer | commit | admin |
17+
| `AccessLevel.maintain` | maintain | owner (only for groups) | admin | owner |
18+
19+
Note: ``has_permission()`` uses conservative fallbacks for forges
20+
that lack a triage equivalent — triage maps to push/write level
21+
on Pagure and Forgejo, not to ticket/read.
1822
"""
1923

2024
pull = 1

ogr/abstract/git_project.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,30 @@ def can_merge_pr(self, username: str) -> bool:
186186
"""
187187
raise NotImplementedError()
188188

189+
def has_permission(self, username: str, access_level: AccessLevel) -> bool:
190+
"""
191+
Check if user has at least the given access level.
192+
193+
Not all forges support all access levels. When a forge lacks an
194+
equivalent for a given level, the check falls back to the nearest
195+
higher level (e.g., triage falls back to write/push on forges
196+
without a triage concept). See the ``AccessLevel`` docstring for
197+
the per-forge mapping table.
198+
199+
Implementations must not propagate forge-specific exceptions for
200+
unknown or non-existent users — return ``False`` instead.
201+
202+
Args:
203+
username: Username.
204+
access_level: Minimum required access level.
205+
206+
Returns:
207+
``True`` if user has at least the given access level,
208+
``False`` otherwise. Returns ``False`` if the user does not
209+
exist or is not a collaborator.
210+
"""
211+
raise NotImplementedError()
212+
189213
def get_users_with_given_access(self, access_levels: list[AccessLevel]) -> set[str]:
190214
"""
191215
Args:

ogr/services/forgejo/project.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,25 @@ class ForgejoProject(BaseGitProject):
4949
None: "",
5050
}
5151

52+
_FORGEJO_PERM_RANK: ClassVar[dict[str, int]] = {
53+
"none": -1,
54+
"read": 0,
55+
"write": 1,
56+
"admin": 2,
57+
"owner": 3,
58+
}
59+
60+
# Intentionally differs from access_dict: triage maps to "write"
61+
# (not "read") because "read" would bypass the permission gate on
62+
# public repos.
63+
_ACCESS_LEVEL_TO_FORGEJO_PERM: ClassVar[dict[AccessLevel, str]] = {
64+
AccessLevel.pull: "read",
65+
AccessLevel.triage: "write",
66+
AccessLevel.push: "write",
67+
AccessLevel.admin: "admin",
68+
AccessLevel.maintain: "owner",
69+
}
70+
5271
def __init__(
5372
self,
5473
repo: str,
@@ -288,6 +307,28 @@ def can_merge_pr(self, username: str) -> bool:
288307
collaborator=username,
289308
).permission in ("owner", "admin", "write")
290309

310+
def has_permission(self, username: str, access_level: AccessLevel) -> bool:
311+
try:
312+
perm = self.api.repo_get_repo_permissions(
313+
owner=self.namespace,
314+
repo=self.repo,
315+
collaborator=username,
316+
).permission
317+
except NotFoundError:
318+
return False
319+
perm = perm.lower() if perm else "none"
320+
if perm not in self._FORGEJO_PERM_RANK:
321+
logger.warning(
322+
"Unknown Forgejo permission %r for user %s on %s/%s; denying",
323+
perm,
324+
username,
325+
self.namespace,
326+
self.repo,
327+
)
328+
return False
329+
required = self._ACCESS_LEVEL_TO_FORGEJO_PERM[access_level]
330+
return self._FORGEJO_PERM_RANK[perm] >= self._FORGEJO_PERM_RANK[required]
331+
291332
def get_users_with_given_access(self, access_levels: list[AccessLevel]) -> set[str]:
292333
access_levels_forgejo = [
293334
self.access_dict[access_level] for access_level in access_levels

ogr/services/github/project.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,23 @@ class GithubProject(BaseGitProject):
4949
# Permission levels that can merge PRs
5050
CAN_MERGE_PERMS: ClassVar[set[str]] = {"admin", "write"}
5151

52+
_GITHUB_PERM_RANK: ClassVar[dict[str, int]] = {
53+
"none": -1,
54+
"read": 0,
55+
"triage": 1,
56+
"write": 2,
57+
"maintain": 3,
58+
"admin": 4,
59+
}
60+
61+
_ACCESS_LEVEL_TO_GITHUB_PERM: ClassVar[dict[AccessLevel, str]] = {
62+
AccessLevel.pull: "read",
63+
AccessLevel.triage: "triage",
64+
AccessLevel.push: "write",
65+
AccessLevel.admin: "admin",
66+
AccessLevel.maintain: "maintain",
67+
}
68+
5269
def __init__(
5370
self,
5471
repo: str,
@@ -251,6 +268,24 @@ def can_merge_pr(self, username) -> bool:
251268
in self.CAN_MERGE_PERMS
252269
)
253270

271+
def has_permission(self, username: str, access_level: AccessLevel) -> bool:
272+
try:
273+
perm = self.github_repo.get_collaborator_permission(username)
274+
except UnknownObjectException:
275+
return False
276+
perm = perm.lower() if perm else "none"
277+
if perm not in self._GITHUB_PERM_RANK:
278+
logger.warning(
279+
"Unknown GitHub permission %r for user %s on %s/%s; denying",
280+
perm,
281+
username,
282+
self.namespace,
283+
self.repo,
284+
)
285+
return False
286+
required = self._ACCESS_LEVEL_TO_GITHUB_PERM[access_level]
287+
return self._GITHUB_PERM_RANK[perm] >= self._GITHUB_PERM_RANK[required]
288+
254289
def _get_collaborators_with_permission(self) -> dict:
255290
"""
256291
Get all project collaborators in dictionary with permission association.

ogr/services/gitlab/project.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import logging
55
import os
6-
from typing import Any, Optional, Union
6+
from typing import Any, ClassVar, Optional, Union
77

88
import gitlab
99
from gitlab.exceptions import GitlabGetError
@@ -38,6 +38,14 @@
3838
class GitlabProject(BaseGitProject):
3939
service: "ogr_gitlab.GitlabService"
4040

41+
_ACCESS_LEVEL_TO_GITLAB: ClassVar[dict[AccessLevel, int]] = {
42+
AccessLevel.pull: gitlab.const.GUEST_ACCESS,
43+
AccessLevel.triage: gitlab.const.REPORTER_ACCESS,
44+
AccessLevel.push: gitlab.const.DEVELOPER_ACCESS,
45+
AccessLevel.admin: gitlab.const.MAINTAINER_ACCESS,
46+
AccessLevel.maintain: gitlab.const.OWNER_ACCESS,
47+
}
48+
4149
def __init__(
4250
self,
4351
repo: str,
@@ -193,6 +201,34 @@ def who_can_merge_pr(self) -> set[str]:
193201
def can_merge_pr(self, username) -> bool:
194202
return username in self.who_can_merge_pr()
195203

204+
def has_permission(self, username: str, access_level: AccessLevel) -> bool:
205+
required_access = self._ACCESS_LEVEL_TO_GITLAB[access_level]
206+
try:
207+
users = self.service.gitlab_instance.users.list(username=username)
208+
user = next(
209+
(u for u in users if u.username.lower() == username.lower()),
210+
None,
211+
)
212+
if user is None:
213+
return False
214+
member_manager = getattr(
215+
self.gitlab_repo,
216+
"members_all",
217+
self.gitlab_repo.members,
218+
)
219+
member = member_manager.get(user.id)
220+
member_access = (
221+
member["access_level"]
222+
if isinstance(member, dict)
223+
else member.access_level
224+
)
225+
return member_access >= required_access
226+
except (
227+
gitlab.exceptions.GitlabGetError,
228+
gitlab.exceptions.GitlabOperationError,
229+
):
230+
return False
231+
196232
def delete(self) -> None:
197233
self.gitlab_repo.delete()
198234

ogr/services/pagure/project.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,25 @@ class PagureProject(BaseGitProject):
4747
None: "",
4848
}
4949

50+
_PAGURE_PERM_RANK: ClassVar[dict[str, int]] = {
51+
"ticket": 0,
52+
"commit": 1,
53+
"admin": 2,
54+
"owner": 3,
55+
}
56+
57+
# Intentionally differs from access_dict: triage maps to "commit"
58+
# (not "ticket") and admin maps to "admin" (not "commit") because
59+
# has_permission uses "at least" threshold semantics rather than
60+
# the assignment semantics of access_dict.
61+
_ACCESS_LEVEL_TO_PAGURE_PERM: ClassVar[dict[AccessLevel, str]] = {
62+
AccessLevel.pull: "ticket",
63+
AccessLevel.triage: "commit",
64+
AccessLevel.push: "commit",
65+
AccessLevel.admin: "admin",
66+
AccessLevel.maintain: "admin",
67+
}
68+
5069
def __init__(
5170
self,
5271
repo: str,
@@ -255,6 +274,34 @@ def can_merge_pr(self, username) -> bool:
255274
)
256275
return username in accounts_that_can_merge_pr
257276

277+
def has_permission(self, username: str, access_level: AccessLevel) -> bool:
278+
required = self._ACCESS_LEVEL_TO_PAGURE_PERM[access_level]
279+
required_rank = self._PAGURE_PERM_RANK[required]
280+
qualifying = [
281+
perm
282+
for perm, rank in self._PAGURE_PERM_RANK.items()
283+
if rank >= required_rank
284+
]
285+
try:
286+
# Check direct user access first to avoid O(G) group expansion
287+
project = self.get_project_info()
288+
access_users = project.get("access_users", {})
289+
for perm in qualifying:
290+
if username in access_users.get(perm, []):
291+
return True
292+
293+
# Fall back to group expansion
294+
access_groups = project.get("access_groups", {})
295+
for perm in qualifying:
296+
if perm == "owner":
297+
continue
298+
for group in access_groups.get(perm, []):
299+
if username in self.service.get_group(group).members:
300+
return True
301+
return False
302+
except PagureAPIException:
303+
return False
304+
258305
def request_access(self):
259306
raise OperationNotSupported("Not possible on Pagure")
260307

tests/unit/test_forgejo.py

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
# Copyright Contributors to the Packit project.
2+
# SPDX-License-Identifier: MIT
3+
4+
import pytest
5+
from flexmock import flexmock
6+
from pyforgejo import NotFoundError
7+
8+
from ogr.abstract import AccessLevel
9+
from ogr.services.forgejo.project import ForgejoProject
10+
11+
12+
class TestForgejoHasPermission:
13+
@staticmethod
14+
def _make_project():
15+
service = flexmock()
16+
project = ForgejoProject(
17+
repo="test_repo",
18+
service=service,
19+
namespace="test_namespace",
20+
)
21+
mock_api = flexmock()
22+
flexmock(project).should_receive("api").and_return(mock_api)
23+
return project, mock_api
24+
25+
@pytest.mark.parametrize(
26+
("perm", "access_level", "expected"),
27+
[
28+
("write", AccessLevel.triage, True),
29+
("write", AccessLevel.push, True),
30+
("read", AccessLevel.triage, False),
31+
("read", AccessLevel.pull, True),
32+
("admin", AccessLevel.admin, True),
33+
("admin", AccessLevel.maintain, False),
34+
("owner", AccessLevel.maintain, True),
35+
("none", AccessLevel.pull, False),
36+
],
37+
)
38+
def test_has_permission(self, perm, access_level, expected):
39+
project, mock_api = self._make_project()
40+
41+
mock_api.should_receive("repo_get_repo_permissions").with_args(
42+
owner="test_namespace",
43+
repo="test_repo",
44+
collaborator="testuser",
45+
).and_return(flexmock(permission=perm))
46+
47+
assert project.has_permission("testuser", access_level) is expected
48+
49+
def test_has_permission_nonexistent_user(self):
50+
project, mock_api = self._make_project()
51+
52+
mock_api.should_receive("repo_get_repo_permissions").with_args(
53+
owner="test_namespace",
54+
repo="test_repo",
55+
collaborator="ghost",
56+
).and_raise(NotFoundError("Not Found"))
57+
58+
assert project.has_permission("ghost", AccessLevel.pull) is False
59+
60+
def test_has_permission_none_permission(self):
61+
project, mock_api = self._make_project()
62+
63+
mock_api.should_receive("repo_get_repo_permissions").with_args(
64+
owner="test_namespace",
65+
repo="test_repo",
66+
collaborator="testuser",
67+
).and_return(flexmock(permission=None))
68+
69+
assert project.has_permission("testuser", AccessLevel.pull) is False
70+
71+
def test_has_permission_unknown_string(self):
72+
project, mock_api = self._make_project()
73+
74+
mock_api.should_receive("repo_get_repo_permissions").with_args(
75+
owner="test_namespace",
76+
repo="test_repo",
77+
collaborator="testuser",
78+
).and_return(flexmock(permission="custom_role"))
79+
80+
assert project.has_permission("testuser", AccessLevel.pull) is False

0 commit comments

Comments
 (0)