diff --git a/backend/druks/core/apis/github.py b/backend/druks/core/apis/github.py index fa1afcd3..1a8cc24f 100644 --- a/backend/druks/core/apis/github.py +++ b/backend/druks/core/apis/github.py @@ -550,6 +550,15 @@ async def post_check_run( body["output"] = {**output, "annotations": list(annotations[start : start + limit])} await checks.async_update(owner, repo_name, check_run_id, data=body) + async def get_permission(self, repo: str, login: str) -> str: + """``login``'s base permission on ``repo``: admin, write, read, or none. GitHub + reports maintain as write and triage as read.""" + owner, name = repo.split("/", 1) + response = await self._github.rest.repos.async_get_collaborator_permission_level( + owner, name, login + ) + return response.parsed_data.permission + async def get_file_content( self, repo: str, diff --git a/backend/druks/core/webhooks/github.py b/backend/druks/core/webhooks/github.py index c69acb3b..4e7fbb5c 100644 --- a/backend/druks/core/webhooks/github.py +++ b/backend/druks/core/webhooks/github.py @@ -12,9 +12,11 @@ _REVIEW_ACTION = {"APPROVED": "approve", "CHANGES_REQUESTED": "request_changes"} -# GitHub's standing for the commenter on that repo. These three write to it; -# everyone else is a passer-by, and on a public repo that is the whole internet. +# GitHub's standing for the commenter on that repo. These three write to it. GitHub +# reports a private org member as a contributor, so anyone else's permission is read. _WRITERS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +# The repo permissions that write. GitHub reports maintain as write. +_WRITE_PERMISSIONS = frozenset({"admin", "write"}) class GitHubEvents(Webhook): @@ -71,7 +73,7 @@ async def on_pull_request_review_submitted(self) -> Response: "branch": pull_request["head"]["ref"], "action": action, "reviewer": sender["login"], - "author_can_write": review["author_association"] in _WRITERS, + "author_can_write": await self._can_write(sender, review["author_association"]), "body": review["body"] or "", # body is nullable on an approve "is_latest_commit": review["commit_id"] == pull_request["head"]["sha"], }, @@ -84,7 +86,9 @@ async def on_issue_comment_created(self) -> Response: if "pull_request" in issue: await self._publish_pull_request_comment(issue["number"]) else: - await publish("ticket.commented", payload={**self._ticket(issue), **self._comment()}) + await publish( + "ticket.commented", payload={**self._ticket(issue), **await self._comment()} + ) return _accepted() async def on_pull_request_review_comment_created(self) -> Response: @@ -105,13 +109,13 @@ async def _publish_pull_request_comment( repo=_repo_name(self.data), pr_number=pr_number, payload={ - **self._comment(), + **await self._comment(), "actor_id": str(self.data["sender"]["id"]), "review_thread_id": review_thread_id, }, ) - def _comment(self) -> dict[str, Any]: + async def _comment(self) -> dict[str, Any]: """The keys every comment event carries, for the comment in this delivery.""" sender, comment = self.data["sender"], self.data["comment"] return { @@ -119,9 +123,19 @@ def _comment(self) -> dict[str, Any]: "body": comment["body"], "author": sender["login"], "author_is_bot": sender["type"] == "Bot", - "author_can_write": comment["author_association"] in _WRITERS, + "author_can_write": await self._can_write(sender, comment["author_association"]), } + async def _can_write(self, user: dict[str, Any], association: str) -> bool: + """Whether ``user`` can write to the delivery's repo.""" + if association in _WRITERS: + return True + if user["type"] == "Bot": + return False + repo = _repo_name(self.data) + github = await Github.get_client(repo) + return await github.get_permission(repo, user["login"]) in _WRITE_PERMISSIONS + async def on_issues_labeled(self) -> Response: """Publish a label that a person or this App put on an issue. Another App's label acts for no one here.""" @@ -210,7 +224,9 @@ async def _publish_pull_request(self, fact: str) -> Response: "body": pull_request["body"] or "", "draft": pull_request["draft"], "author": pull_request["user"]["login"], - "author_can_write": pull_request["author_association"] in _WRITERS, + "author_can_write": await self._can_write( + pull_request["user"], pull_request["author_association"] + ), }, ) return _accepted() diff --git a/backend/tests/test_github_channel.py b/backend/tests/test_github_channel.py index 654c19d9..11d5ace2 100644 --- a/backend/tests/test_github_channel.py +++ b/backend/tests/test_github_channel.py @@ -69,12 +69,16 @@ async def create_reaction(client, repo, comment_id, content): async def create_review_comment_reaction(client, repo, comment_id, content): posts.append(("create_review_comment_reaction", repo, comment_id, content)) + async def get_permission(client, repo, login): + return "read" + monkeypatch.setattr(GitHubClient, "create_comment", create_comment) monkeypatch.setattr(GitHubClient, "reply_to_review_comment", reply_to_review_comment) monkeypatch.setattr(GitHubClient, "create_reaction", create_reaction) monkeypatch.setattr( GitHubClient, "create_review_comment_reaction", create_review_comment_reaction ) + monkeypatch.setattr(GitHubClient, "get_permission", get_permission) return posts @@ -139,7 +143,7 @@ async def receive(tmp_path, data, *, inline=False): await events.on_issue_comment_created() -async def test_the_webhook_publishes_each_comment_once(tmp_path, monkeypatch): +async def test_the_webhook_publishes_each_comment_once(card, github, tmp_path, monkeypatch): published = [] async def publish(name, **kwargs): diff --git a/backend/tests/test_webhooks_github.py b/backend/tests/test_webhooks_github.py index 93914b26..11ad9649 100644 --- a/backend/tests/test_webhooks_github.py +++ b/backend/tests/test_webhooks_github.py @@ -2,6 +2,7 @@ from typing import Any, cast import pytest +from druks.core.apis.github import GitHubClient from druks.core.webhooks import github as github_webhooks from druks.core.webhooks.github import GitHubEvents from druks.testing import make_settings @@ -48,7 +49,30 @@ async def _emit(event_type, **kwargs): return published -def _issue(*, action, is_pull_request=False, sender_type="User"): +class _Permissions(GitHubClient): + """GitHub's permission of each login on the repo. Anyone else reads.""" + + def __init__(self, **permissions: str): + super().__init__("token", app_id="1", slug="druks") + self.permissions = permissions + self.asked: list[str] = [] + + async def get_permission(self, repo, login): + self.asked.append(login) + return self.permissions.get(login, "read") + + +def _github(monkeypatch, **permissions: str) -> _Permissions: + github = _Permissions(**permissions) + + async def get_client(owner_or_repo): + return github + + monkeypatch.setattr(github_webhooks.Github, "get_client", get_client) + return github + + +def _issue(*, action, is_pull_request=False, sender_type="User", association="NONE"): issue = { "id": 4242, "number": 7, @@ -64,7 +88,7 @@ def _issue(*, action, is_pull_request=False, sender_type="User"): "repository": {"full_name": "acme/widget"}, "issue": issue, "sender": {"type": sender_type, "login": "passerby", "id": 99}, - "comment": {"id": 501, "body": "Still broken", "author_association": "NONE"}, + "comment": {"id": 501, "body": "Still broken", "author_association": association}, } @@ -109,6 +133,7 @@ async def test_an_issue_action_publishes_a_tracker_fact( async def test_an_issue_comment_publishes_ticket_commented(tmp_path, monkeypatch): published = _capture(monkeypatch) + _github(monkeypatch) await _events(tmp_path, payload=_issue(action="created")).on_issue_comment_created() @@ -117,6 +142,7 @@ async def test_an_issue_comment_publishes_ticket_commented(tmp_path, monkeypatch async def test_a_pull_request_comment_publishes_pr_commented(tmp_path, monkeypatch): published = _capture(monkeypatch) + _github(monkeypatch) payload = _issue(action="created", is_pull_request=True) await _events(tmp_path, payload=payload).on_issue_comment_created() @@ -214,3 +240,48 @@ async def test_a_review_says_whether_it_is_of_the_latest_commit( [(event_type, published_event)] = published assert event_type == "pr.review_submitted" assert published_event["payload"]["is_latest_commit"] is is_latest_commit + + +@pytest.mark.parametrize(("permission", "can_write"), [("write", True), ("read", False)]) +async def test_a_review_from_a_contributor_asks_github_whether_they_write( + tmp_path, monkeypatch, permission, can_write +): + # GitHub reports a private org member as a contributor. + published = _capture(monkeypatch) + _github(monkeypatch, alice=permission) + + payload = { + **_pull_request(action="submitted"), + "sender": {"login": "alice", "type": "User"}, + "review": { + "state": "approved", + "author_association": "CONTRIBUTOR", + "body": None, + "commit_id": "0f3a2c1d", + }, + } + + await _events(tmp_path, payload=payload).on_pull_request_review_submitted() + + assert published[0][1]["payload"]["author_can_write"] is can_write + + +async def test_a_comment_from_a_private_member_who_writes_can_write(tmp_path, monkeypatch): + published = _capture(monkeypatch) + _github(monkeypatch, passerby="admin") + payload = _issue(action="created", is_pull_request=True, association="CONTRIBUTOR") + + await _events(tmp_path, payload=payload).on_issue_comment_created() + + assert published[0][1]["payload"]["author_can_write"] is True + + +@pytest.mark.parametrize(("sender_type", "association"), [("User", "MEMBER"), ("Bot", "NONE")]) +async def test_a_writer_or_a_bot_is_not_looked_up(tmp_path, monkeypatch, sender_type, association): + _capture(monkeypatch) + github = _github(monkeypatch) + payload = _issue(action="created", sender_type=sender_type, association=association) + + await _events(tmp_path, payload=payload).on_issue_comment_created() + + assert github.asked == []