diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 64f87853e..31f06523a 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -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 @@ -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}" diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index e1357983a..9be28d2f5 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -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]" + 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") diff --git a/webhook_server/tests/test_github_repository_settings.py b/webhook_server/tests/test_github_repository_settings.py index 216c9fde7..b21872852 100644 --- a/webhook_server/tests/test_github_repository_settings.py +++ b/webhook_server/tests/test_github_repository_settings.py @@ -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 mock_file = Mock() mock_file.read.return_value = "test-private-key" @@ -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") @@ -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" @@ -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] @@ -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" @@ -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) @@ -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" @@ -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" diff --git a/webhook_server/utils/github_repository_settings.py b/webhook_server/utils/github_repository_settings.py index 5628c9fe1..e6d853666 100644 --- a/webhook_server/utils/github_repository_settings.py +++ b/webhook_server/utils/github_repository_settings.py @@ -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 @@ -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: @@ -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) + + +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("/") @@ -432,19 +447,42 @@ def get_repository_github_app_api(config_: Config, repository_name: str) -> Gith return None +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") + + 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: