Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 13 additions & 22 deletions webhook_server/libs/github_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
from webhook_server.utils.context import WebhookContext, get_context
from webhook_server.utils.github_repository_settings import (
DEFAULT_BRANCH_PROTECTION,
get_github_app_slug,
get_repository_github_app_api,
)
from webhook_server.utils.github_retry import github_api_call
Expand Down Expand Up @@ -546,28 +547,18 @@ async def process(self) -> Any:

# Initialize app bot login for bot-PR identification (async)
if not self.app_bot_login:
_github_app_api = await github_api_call(
get_repository_github_app_api,
config_=self.config,
repository_name=self.repository_full_name,
logger=self.logger,
log_prefix=self.log_prefix,
)
if _github_app_api:
try:
self.app_bot_login = await github_api_call(
lambda: _github_app_api.get_user().login,
logger=self.logger,
log_prefix=self.log_prefix,
)
except asyncio.CancelledError:
raise
except Exception:
self.logger.exception(
f"{self.log_prefix} Failed to get app bot login — bot-PR detection may not work"
)
else:
self.logger.debug(f"{self.log_prefix} No GitHub App API available — app_bot_login not set")
try:
_app_slug = await github_api_call(
get_github_app_slug,
self.config,
logger=self.logger,
log_prefix=self.log_prefix,
)
self.app_bot_login = f"{_app_slug}[bot]"
except asyncio.CancelledError:
raise
except Exception:
self.logger.exception(f"{self.log_prefix} Failed to get app bot login — bot-PR detection may not work")

event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}"

Expand Down
123 changes: 123 additions & 0 deletions webhook_server/tests/test_github_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,129 @@ async def test_process_unsupported_event(
# Should not raise an exception, just skip processing
await webhook.process()

@patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"})
@patch("webhook_server.libs.github_api.get_github_repo_api")
@patch("webhook_server.libs.github_api.get_repository_github_app_api")
@patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit")
@patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config")
@patch("webhook_server.libs.config.Config.repository_local_data")
@patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users")
@patch("webhook_server.libs.github_api.get_github_app_slug", return_value="manage-repositories-app")
async def test_process_initializes_app_bot_login_from_slug(
self,
mock_get_slug: Mock,
mock_auto_verified_prop: Mock,
mock_repo_local_data: Mock,
mock_get_apis: Mock,
mock_api_rate_limit: Mock,
mock_repo_github_app_api: Mock,
mock_repo_api: Mock,
pull_request_payload: dict[str, Any],
) -> None:
"""Test app_bot_login is initialized from get_github_app_slug."""
mock_api = Mock()
mock_api.rate_limiting = [100, 5000]
mock_user = Mock()
mock_user.login = "test-user"
mock_api.get_user.return_value = mock_user

mock_api_rate_limit.return_value = (mock_api, "TOKEN", "USER")
mock_repo_api.return_value = Mock()
mock_repo_github_app_api.return_value = Mock()
mock_get_apis.return_value = []
mock_repo_local_data.return_value = {}

headers = Headers({"X-GitHub-Event": "unsupported_event"})
webhook = GithubWebhook(hook_data=pull_request_payload, headers=headers, logger=Mock())
webhook.app_bot_login = ""

await webhook.process()
assert webhook.app_bot_login == "manage-repositories-app[bot]"
Comment thread
rnetser marked this conversation as resolved.
mock_api.get_user.assert_not_called()

@patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"})
@patch("webhook_server.libs.github_api.get_github_repo_api")
@patch("webhook_server.libs.github_api.get_repository_github_app_api")
@patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit")
@patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config")
@patch("webhook_server.libs.config.Config.repository_local_data")
@patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users")
@patch("webhook_server.libs.github_api.get_github_app_slug", return_value="other-app")
async def test_process_app_bot_login_skips_when_already_set(
self,
mock_get_slug: Mock,
mock_auto_verified_prop: Mock,
mock_repo_local_data: Mock,
mock_get_apis: Mock,
mock_api_rate_limit: Mock,
mock_repo_github_app_api: Mock,
mock_repo_api: Mock,
pull_request_payload: dict[str, Any],
) -> None:
"""Test app_bot_login is not overwritten when already set."""
mock_api = Mock()
mock_api.rate_limiting = [100, 5000]
mock_user = Mock()
mock_user.login = "test-user"
mock_api.get_user.return_value = mock_user

mock_api_rate_limit.return_value = (mock_api, "TOKEN", "USER")
mock_repo_api.return_value = Mock()
mock_repo_github_app_api.return_value = Mock()
mock_get_apis.return_value = []
mock_repo_local_data.return_value = {}

headers = Headers({"X-GitHub-Event": "unsupported_event"})
webhook = GithubWebhook(hook_data=pull_request_payload, headers=headers, logger=Mock())
webhook.app_bot_login = "existing-app[bot]"

await webhook.process()
assert webhook.app_bot_login == "existing-app[bot]"
mock_get_slug.assert_not_called()
mock_api.get_user.assert_not_called()

@patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"})
@patch("webhook_server.libs.github_api.get_github_repo_api")
@patch("webhook_server.libs.github_api.get_repository_github_app_api")
@patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit")
@patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config")
@patch("webhook_server.libs.config.Config.repository_local_data")
@patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users")
@patch("webhook_server.libs.github_api.get_github_app_slug", side_effect=Exception("PEM not found"))
async def test_process_app_bot_login_handles_slug_failure(
self,
mock_get_slug: Mock,
mock_auto_verified_prop: Mock,
mock_repo_local_data: Mock,
mock_get_apis: Mock,
mock_api_rate_limit: Mock,
mock_repo_github_app_api: Mock,
mock_repo_api: Mock,
pull_request_payload: dict[str, Any],
) -> None:
"""Test app_bot_login handles slug retrieval failure gracefully."""
mock_api = Mock()
mock_api.rate_limiting = [100, 5000]
mock_user = Mock()
mock_user.login = "test-user"
mock_api.get_user.return_value = mock_user

mock_api_rate_limit.return_value = (mock_api, "TOKEN", "USER")
mock_repo_api.return_value = Mock()
mock_repo_github_app_api.return_value = Mock()
mock_get_apis.return_value = []
mock_repo_local_data.return_value = {}

headers = Headers({"X-GitHub-Event": "unsupported_event"})
mock_logger = Mock()
webhook = GithubWebhook(hook_data=pull_request_payload, headers=headers, logger=mock_logger)
webhook.app_bot_login = ""

await webhook.process()
assert webhook.app_bot_login == ""
mock_logger.exception.assert_called_once()
mock_api.get_user.assert_not_called()

@patch("webhook_server.libs.github_api.get_repository_github_app_api")
@patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit")
@patch("webhook_server.libs.github_api.get_github_repo_api")
Expand Down
8 changes: 8 additions & 0 deletions webhook_server/tests/test_github_repository_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,7 @@ def test_get_repository_github_app_api_success(self, mock_logger: Mock, mock_ope
mock_config = Mock()
mock_config.data_dir = "/test/dir"
mock_config.root_data = {"github-app-id": 12345}
mock_config.get_value.return_value = 12345

Comment thread
rnetser marked this conversation as resolved.
mock_file = Mock()
mock_file.read.return_value = "test-private-key"
Expand All @@ -748,6 +749,7 @@ def test_get_repository_github_app_api_success(self, mock_logger: Mock, mock_ope
result = get_repository_github_app_api(mock_config, "owner/repo")

assert result == mock_github
mock_config.get_value.assert_called_with("github-app-id")
mock_auth.AppAuth.assert_called_once_with(app_id=12345, private_key="test-private-key")
mock_app_instance.get_repo_installation.assert_called_once_with(owner="owner", repo="repo")

Expand All @@ -758,6 +760,7 @@ def test_get_repository_github_app_api_exception(self, mock_logger: Mock, mock_o
mock_config = Mock()
mock_config.data_dir = "/test/dir"
mock_config.root_data = {"github-app-id": 12345}
mock_config.get_value.return_value = 12345

mock_file = Mock()
mock_file.read.return_value = "test-private-key"
Expand All @@ -776,6 +779,7 @@ def test_get_repository_github_app_api_exception(self, mock_logger: Mock, mock_o
result = get_repository_github_app_api(mock_config, "owner/repo")

assert result is None
mock_config.get_value.assert_called_with("github-app-id")
mock_logger.error.assert_called_once()
assert "Repository owner/repo not found by manage-repositories-app" in mock_logger.error.call_args[0][0]

Expand All @@ -786,6 +790,7 @@ def test_get_repository_github_app_token_success(self, _mock_logger: Mock, mock_
mock_config = Mock()
mock_config.data_dir = "/test/dir"
mock_config.root_data = {"github-app-id": 12345}
mock_config.get_value.return_value = 12345

mock_file = Mock()
mock_file.read.return_value = "test-private-key"
Expand All @@ -810,6 +815,7 @@ def test_get_repository_github_app_token_success(self, _mock_logger: Mock, mock_
result = get_repository_github_app_token(mock_config, "owner/repo")

assert result == "fake-installation-token" # pragma: allowlist secret
mock_config.get_value.assert_called_with("github-app-id")
mock_auth.AppAuth.assert_called_once_with(app_id=12345, private_key="test-private-key")
mock_app_instance.get_repo_installation.assert_called_once_with(owner="owner", repo="repo")
mock_app_instance.get_access_token.assert_called_once_with(67890)
Expand All @@ -821,6 +827,7 @@ def test_get_repository_github_app_token_failure(self, mock_logger: Mock, mock_o
mock_config = Mock()
mock_config.data_dir = "/test/dir"
mock_config.root_data = {"github-app-id": 12345}
mock_config.get_value.return_value = 12345

mock_file = Mock()
mock_file.read.return_value = "test-private-key"
Expand All @@ -838,6 +845,7 @@ def test_get_repository_github_app_token_failure(self, mock_logger: Mock, mock_o
result = get_repository_github_app_token(mock_config, "owner/repo")

assert result is None
mock_config.get_value.assert_called_with("github-app-id")
mock_logger.exception.assert_called_once()
assert (
"Failed to get GitHub App installation token for owner/repo"
Expand Down
60 changes: 49 additions & 11 deletions webhook_server/utils/github_repository_settings.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import copy
import os
import threading
from collections.abc import Callable
from concurrent.futures import Future, ThreadPoolExecutor, as_completed
from copy import deepcopy
Expand Down Expand Up @@ -43,6 +44,8 @@
}

LOGGER = get_logger_with_params()
_github_app_slug_cache: dict[int, str] = {}
_github_app_slug_lock = threading.Lock()


def _get_github_repo_api(github_api: github.Github, repository: int | str) -> Repository | None:
Expand Down Expand Up @@ -407,15 +410,27 @@ def _set_checkrun_queued(_api: Repository, _pull_request: PullRequest) -> None:
return True, f"[API user {api_user}] - {repository}: Set check run status to {QUEUED_STR} is done", LOGGER.debug


def get_repository_github_app_api(config_: Config, repository_name: str) -> Github | None:
LOGGER.debug("Getting repositories GitHub app API")
def _create_github_integration(config_: Config, github_app_id: int | None = None) -> GithubIntegration:
"""Create an authenticated GithubIntegration instance using App JWT.

Reads the private key and app ID from config to create an authenticated
GithubIntegration. This is the shared setup for all GitHub App API operations.
"""
with open(os.path.join(config_.data_dir, "webhook-server.private-key.pem")) as fd:
private_key = fd.read()

github_app_id: int = config_.root_data["github-app-id"]
if not github_app_id:
github_app_id = config_.get_value("github-app-id")
if not github_app_id:
raise ValueError("github-app-id not configured — required for GitHub App authentication")

auth: AppAuth = Auth.AppAuth(app_id=github_app_id, private_key=private_key)
app_instance: GithubIntegration = GithubIntegration(auth=auth)
return GithubIntegration(auth=auth)
Comment thread
rnetser marked this conversation as resolved.


def get_repository_github_app_api(config_: Config, repository_name: str) -> Github | None:
LOGGER.debug("Getting repositories GitHub app API")
app_instance = _create_github_integration(config_)
owner: str
repo: str
owner, repo = repository_name.split("/")
Expand All @@ -432,19 +447,42 @@ def get_repository_github_app_api(config_: Config, repository_name: str) -> Gith
return None


Comment thread
rnetser marked this conversation as resolved.
def get_github_app_slug(config_: Config) -> str:
"""Get the GitHub App slug using App JWT authentication.

Returns the app slug (e.g., 'manage-repositories-app').
Caches the result keyed by github-app-id since the slug is immutable per app.
Raises on failure so github_api_call() can apply retry/backoff.
"""
github_app_id: int | None = config_.get_value("github-app-id")
if not github_app_id:
raise ValueError("github-app-id not configured — required for GitHub App slug lookup")
Comment thread
rnetser marked this conversation as resolved.

if github_app_id in _github_app_slug_cache:
return _github_app_slug_cache[github_app_id]

# Perform network I/O outside the lock — concurrent calls may
# duplicate work but won't block each other.
LOGGER.debug("Getting GitHub App slug")
app_instance = _create_github_integration(config_, github_app_id=github_app_id)
slug = app_instance.get_app().slug
if not slug:
raise ValueError("GitHub App returned empty slug")

with _github_app_slug_lock:
# Another thread may have populated it while we were fetching
if github_app_id not in _github_app_slug_cache:
_github_app_slug_cache[github_app_id] = slug
return _github_app_slug_cache[github_app_id]


def get_repository_github_app_token(config_: Config, repository_name: str) -> str | None:
"""Get a raw GitHub App installation token string for use with CLI tools.

Returns the token string or None if the app is not configured/installed.
"""
LOGGER.debug(f"Getting GitHub App installation token for {repository_name}")

with open(os.path.join(config_.data_dir, "webhook-server.private-key.pem")) as fd:
private_key = fd.read()

github_app_id: int = config_.root_data["github-app-id"]
auth: AppAuth = Auth.AppAuth(app_id=github_app_id, private_key=private_key)
app_instance: GithubIntegration = GithubIntegration(auth=auth)
app_instance = _create_github_integration(config_)
owner, repo = repository_name.split("/", maxsplit=1)

try:
Expand Down