From 862c8ef7551be0d993bb0a78288da3f6bff4052d Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Fri, 29 May 2026 13:28:44 -0400 Subject: [PATCH] feat: update user permission protocol for github since github's and gitlab's endpoints for user permissions take in different IDs to discern the user github accepting the username, and gitlab accepting the user's internal ID, it was unclear what to do at the protocol level: should we make 2 separate protocols since the ID arg is "different" between the 2 providers? or should we make a single protocol and its implicit that gitlab and github use different values to fix this problem im proposing to have the protocol take in the Author type from within the SCM platform which contains both pieces of information either provider would need so we can have a single protocol that takes in the same value for either provider and the decision to use either the id or username is encapsulated i tested the gitlab and github endpoints locally using server and clients in bin/ --- bin/github-client | 6 ++- bin/gitlab-client | 17 ++++++ src/scm/actions.py | 4 +- src/scm/providers/github/provider.py | 14 ++--- src/scm/providers/gitlab/provider.py | 50 ++++++++++++++++++ src/scm/test_fixtures.py | 4 +- src/scm/types.py | 2 +- tests/unit/provider/test_github.py | 2 +- tests/unit/provider/test_gitlab.py | 79 ++++++++++++++++++++++++++++ tests/unit/test_rpc_integration.py | 2 +- 10 files changed, 165 insertions(+), 15 deletions(-) diff --git a/bin/github-client b/bin/github-client index 190b4f3..e42a40b 100755 --- a/bin/github-client +++ b/bin/github-client @@ -66,7 +66,7 @@ Commands: list-repositories get-repository-assignees list-repository-user-permissions - get-repository-user-permission + get-repository-user-permission get-repository-labels get-repository-topics get-app-installation @@ -80,6 +80,7 @@ import sys from scm.manager import SourceCodeManager from scm.types import ( + Author, CheckRunOutput, ChmodCommitAction, CollapsePullRequestCommentProtocol, @@ -385,6 +386,7 @@ def main(): sub.add_parser("get-repository-assignees") sub.add_parser("list-repository-user-permissions") p = sub.add_parser("get-repository-user-permission") + p.add_argument("id") p.add_argument("username") sub.add_parser("get-repository-labels") sub.add_parser("get-repository-topics") @@ -656,7 +658,7 @@ def main(): elif args.command == "get-repository-user-permission": assert isinstance(scm, GetRepositoryUserPermissionProtocol) - dump(scm.get_repository_user_permission(args.username)) + dump(scm.get_repository_user_permission(Author(id=args.id, username=args.username))) elif args.command == "get-repository-labels": assert isinstance(scm, GetRepositoryLabelsProtocol) diff --git a/bin/gitlab-client b/bin/gitlab-client index a7b24c6..d09f5a9 100755 --- a/bin/gitlab-client +++ b/bin/gitlab-client @@ -71,6 +71,8 @@ Commands: [--output-title --output-summary <summary> [--output-text <text>]] get-repository get-repository-assignees + list-repository-user-permissions + get-repository-user-permission <id> <username> get-repository-labels get-app-installation """ @@ -83,6 +85,7 @@ import sys from scm.manager import SourceCodeManager from scm.types import ( + Author, CheckRunOutput, ChmodCommitAction, CollapsePullRequestCommentProtocol, @@ -122,6 +125,8 @@ from scm.types import ( GetRepositoryAssigneesProtocol, GetRepositoryLabelsProtocol, GetRepositoryProtocol, + GetRepositoryUserPermissionProtocol, + ListRepositoryUserPermissionsProtocol, MoveCommitAction, PaginationParams, ReviewCommentInput, @@ -373,6 +378,10 @@ def main(): sub.add_parser("get-repository") sub.add_parser("get-repository-assignees") + sub.add_parser("list-repository-user-permissions") + p = sub.add_parser("get-repository-user-permission") + p.add_argument("id") + p.add_argument("username") sub.add_parser("get-repository-labels") sub.add_parser("get-app-installation") @@ -614,6 +623,14 @@ def main(): assert isinstance(scm, GetRepositoryAssigneesProtocol) dump(scm.get_repository_assignees()) + elif args.command == "list-repository-user-permissions": + assert isinstance(scm, ListRepositoryUserPermissionsProtocol) + dump(scm.list_repository_user_permissions()) + + elif args.command == "get-repository-user-permission": + assert isinstance(scm, GetRepositoryUserPermissionProtocol) + dump(scm.get_repository_user_permission(Author(id=args.id, username=args.username))) + elif args.command == "get-repository-labels": assert isinstance(scm, GetRepositoryLabelsProtocol) dump(scm.get_repository_labels()) diff --git a/src/scm/actions.py b/src/scm/actions.py index faf0162..c2ff289 100644 --- a/src/scm/actions.py +++ b/src/scm/actions.py @@ -164,11 +164,11 @@ def list_repository_user_permissions( def get_repository_user_permission( scm: GetRepositoryUserPermissionProtocol, - username: str, + author: Author, request_options: RequestOptions | None = None, ) -> ActionResult[UserPermissions]: """Get repository permissions for a single user.""" - return scm.get_repository_user_permission(username, request_options) + return scm.get_repository_user_permission(author, request_options) def get_repository_labels( diff --git a/src/scm/providers/github/provider.py b/src/scm/providers/github/provider.py index 4b6a0c7..2e23790 100644 --- a/src/scm/providers/github/provider.py +++ b/src/scm/providers/github/provider.py @@ -518,18 +518,20 @@ def list_repository_user_permissions( pagination=pagination, request_options=request_options, ) - return map_paginated_action(pagination, response, lambda r: [map_collaborator_user_perms(user) for user in r]) + return map_paginated_action( + pagination, response, lambda r: [map_collaborator_permissions_dict(user) for user in r] + ) def get_repository_user_permission( self, - username: str, + author: Author, request_options: RequestOptions | None = None, ) -> ActionResult[UserPermissions]: response = self.get( - f"/repos/{self.repository['name']}/collaborators/{username}/permission", + f"/repos/{self.repository['name']}/collaborators/{author['username']}/permission", request_options=request_options, ) - return map_action(response, map_collaborator_permission_user_perms) + return map_action(response, map_collaborator_permission) def get_repository_labels( self, @@ -1760,7 +1762,7 @@ def map_github_repository_permission(permissions: dict[str, bool]) -> Repository return "none" -def map_collaborator_user_perms(raw: dict[str, Any]) -> UserPermissions: +def map_collaborator_permissions_dict(raw: dict[str, Any]) -> UserPermissions: return UserPermissions( login=raw["login"], id=str(raw["id"]), @@ -1780,7 +1782,7 @@ def map_collaborator_permission_level(permission: str) -> RepositoryPermission: raise ValueError(f"unmappable repository permission: {permission!r}") -def map_collaborator_permission_user_perms(raw: dict[str, Any]) -> UserPermissions: +def map_collaborator_permission(raw: dict[str, Any]) -> UserPermissions: user = raw["user"] return UserPermissions( login=user["login"], diff --git a/src/scm/providers/gitlab/provider.py b/src/scm/providers/gitlab/provider.py index ea388a0..f3df9bf 100644 --- a/src/scm/providers/gitlab/provider.py +++ b/src/scm/providers/gitlab/provider.py @@ -64,6 +64,7 @@ ReactionResult, Referrer, Repository, + RepositoryPermission, RequestOptions, ResourceId, Review, @@ -74,6 +75,7 @@ ReviewThread, ReviewThreadComment, TreeEntry, + UserPermissions, WriteCommitAction, ) @@ -100,6 +102,8 @@ class GitLab: issue = "/projects/{project}/issues/{issue}" issues = "/projects/{project}/issues" project_users = "/projects/{project_id}/users" + project_members = "/projects/{project_id}/members/all" + project_member = "/projects/{project_id}/members/all/{user_id}" project_labels = "/projects/{project_id}/labels" issue_awards = "/projects/{project_id}/issues/{issue_id}/award_emoji" issue_award = "/projects/{project_id}/issues/{issue_id}/award_emoji/{award_id}" @@ -342,6 +346,29 @@ def get_repository_assignees( ) return make_paginated_result(map_author, response, response.json()) + def list_repository_user_permissions( + self, + pagination: PaginationParams | None = None, + request_options: RequestOptions | None = None, + ) -> PaginatedActionResult[list[UserPermissions]]: + response = self.get( + GitLab.project_members.format(project_id=self.project_id), + pagination=pagination, + request_options=request_options, + ) + return make_paginated_result(map_member_permissions, response, response.json()) + + def get_repository_user_permission( + self, + author: Author, + request_options: RequestOptions | None = None, + ) -> ActionResult[UserPermissions]: + response = self.get( + GitLab.project_member.format(project_id=self.project_id, user_id=author["id"]), + request_options=request_options, + ) + return make_result(map_member_permissions, response.json()) + def get_repository_labels( self, pagination: PaginationParams | None = None, @@ -1820,6 +1847,29 @@ def map_app_installation(raw: dict[str, Any]) -> AppInstallation: ) +def map_access_level(access_level: int) -> RepositoryPermission: + # GitLab default roles, keyed by numerical access level: + # https://docs.gitlab.com/user/permissions/#default-roles + # Maintainer (40) and Owner (50) can administer the project; Developer (30) + # can push; Reporter (20) and Guest (10) are read-only; anything lower has + # no access. + if access_level >= 40: # Maintainer, Owner + return "admin" + if access_level >= 30: # Developer + return "write" + if access_level >= 10: # Guest, Planner, Reporter + return "read" + return "none" + + +def map_member_permissions(raw: dict[str, Any]) -> UserPermissions: + return UserPermissions( + login=raw["username"], + id=str(raw["id"]), + perms=map_access_level(raw["access_level"]), + ) + + def map_repository(raw: dict[str, Any]) -> GitRepository: statistics = raw.get("statistics") repo_size = statistics.get("repository_size", 0) if statistics else 0 diff --git a/src/scm/test_fixtures.py b/src/scm/test_fixtures.py index dde92e2..cc6e978 100644 --- a/src/scm/test_fixtures.py +++ b/src/scm/test_fixtures.py @@ -712,11 +712,11 @@ def list_repository_user_permissions( def get_repository_user_permission( self, - username: str, + author: Author, request_options: RequestOptions | None = None, ) -> ActionResult[UserPermissions]: return ActionResult( - data=UserPermissions(login=username, id="123", perms="write"), + data=UserPermissions(login=author["username"], id=author["id"], perms="write"), type="github", raw={"headers": None, "data": None}, meta={}, diff --git a/src/scm/types.py b/src/scm/types.py index 9e0623c..8c88aeb 100644 --- a/src/scm/types.py +++ b/src/scm/types.py @@ -644,7 +644,7 @@ def list_repository_user_permissions( class GetRepositoryUserPermissionProtocol(Protocol): def get_repository_user_permission( self, - username: str, + author: Author, request_options: RequestOptions | None = None, ) -> ActionResult[UserPermissions]: ... diff --git a/tests/unit/provider/test_github.py b/tests/unit/provider/test_github.py index 08a80ac..541c9c4 100644 --- a/tests/unit/provider/test_github.py +++ b/tests/unit/provider/test_github.py @@ -722,7 +722,7 @@ def expected_check_run(raw: dict[str, Any]) -> dict[str, Any]: { "name": "get_repository_user_permission", "operation": "get", - "kwargs": {"username": "testuser"}, + "kwargs": {"author": {"id": "123", "username": "testuser"}}, "path": "/repos/test-org/test-repo/collaborators/testuser/permission", "raw": make_collaborator_permission(permission="write"), "expected_data": {"login": "testuser", "id": "123", "perms": "write"}, diff --git a/tests/unit/provider/test_gitlab.py b/tests/unit/provider/test_gitlab.py index 0ec99c5..89ef02e 100644 --- a/tests/unit/provider/test_gitlab.py +++ b/tests/unit/provider/test_gitlab.py @@ -23,6 +23,7 @@ GitLabProvider, _count_unified_diff_changes, _head_to_source_branch, + map_access_level, map_app_installation, map_pull_request_file, ) @@ -165,6 +166,68 @@ def _make_mock_response(json_data): "meta": {"next_cursor": None}, }, ), + ForwardToClientTest( + provider_method=GitLabProvider.list_repository_user_permissions, + provider_args={"pagination": None, "request_options": None}, + client_calls=[ + ClientForwardedCall( + method="GET", + path="/projects/79787061/members/all", + json_response=[ + {"id": 1, "username": "dev", "name": "Dev", "state": "active", "access_level": 30}, + {"id": 2, "username": "owner", "name": "Owner", "state": "active", "access_level": 50}, + ], + ), + ], + provider_return_value={ + "data": [ + {"login": "dev", "id": "1", "perms": "write"}, + {"login": "owner", "id": "2", "perms": "admin"}, + ], + "type": "gitlab", + "raw": { + "data": [ + {"id": 1, "username": "dev", "name": "Dev", "state": "active", "access_level": 30}, + {"id": 2, "username": "owner", "name": "Owner", "state": "active", "access_level": 50}, + ], + "headers": None, + }, + "meta": {"next_cursor": None}, + }, + ), + ForwardToClientTest( + provider_method=GitLabProvider.get_repository_user_permission, + # GitLab resolves the member by Author.id, not username. + provider_args={"author": {"id": "42", "username": "maintainer"}, "request_options": None}, + client_calls=[ + ClientForwardedCall( + method="GET", + path="/projects/79787061/members/all/42", + json_response={ + "id": 42, + "username": "maintainer", + "name": "Maintainer", + "state": "active", + "access_level": 40, + }, + ), + ], + provider_return_value={ + "data": {"login": "maintainer", "id": "42", "perms": "admin"}, + "type": "gitlab", + "raw": { + "data": { + "id": 42, + "username": "maintainer", + "name": "Maintainer", + "state": "active", + "access_level": 40, + }, + "headers": None, + }, + "meta": {}, + }, + ), ForwardToClientTest( provider_method=GitLabProvider.get_repository_topics, provider_args={"request_options": None}, @@ -14219,3 +14282,19 @@ def test_request_maps_status_code_to_error( assert exc_info.value.code == expected_code assert exc_info.value.detail == '{"message":"upstream said no"}' + + +@pytest.mark.parametrize( + ("access_level", "expected"), + [ + (0, "none"), # No access + (5, "none"), # Minimal access + (10, "read"), # Guest + (20, "read"), # Reporter + (30, "write"), # Developer + (40, "admin"), # Maintainer + (50, "admin"), # Owner + ], +) +def test_map_access_level(access_level: int, expected: str) -> None: + assert map_access_level(access_level) == expected diff --git a/tests/unit/test_rpc_integration.py b/tests/unit/test_rpc_integration.py index 3c76be8..dc339e5 100644 --- a/tests/unit/test_rpc_integration.py +++ b/tests/unit/test_rpc_integration.py @@ -187,7 +187,7 @@ def make_client_scm(organization_id, repository_id, server): ), ( "get_repository_user_permission", - lambda scm: actions.get_repository_user_permission(scm, "reader"), + lambda scm: actions.get_repository_user_permission(scm, {"id": "1", "username": "reader"}), {"permission": "read", "role_name": "read", "user": {"login": "reader", "id": 1}}, 200, None,