From ce06ceeddea095491112e1932f14dab2c611961b Mon Sep 17 00:00:00 2001 From: "druks-ai[bot]" <339005418+druks-ai[bot]@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:19:44 +0000 Subject: [PATCH] DRU-1130 -- Look up an owner's installation once when it covers all its repositories issue_token looks up the first repo's installation one time. When that installation covers all repositories, the other repos of that owner go into the token with no more lookups. An installation on selected repositories keeps the lookup for each repo. A repo of another owner gets no lookup and stays out of the token. Owner names match without regard to letter case. GitHubAppClient.get_installation replaces get_installation_id and returns the installation that GitHub answers. The client keeps no state. A renamed or deleted repo of an all-repositories owner goes into the token request, and GitHub refuses the whole token with a 422. issue_token turns that 422 into a MissingSecretError. Its message names the repos of the request and the fix: remove the renamed or deleted repository from the project. --- backend/druks/core/apis/github.py | 4 +- backend/druks/core/services.py | 47 +++++++++++++------ backend/tests/test_github.py | 6 +-- backend/tests/test_github_channel.py | 6 +-- backend/tests/test_managed_github.py | 59 +++++++++++++++++++++++- backend/tests/test_sandbox_identities.py | 4 +- backend/tests/test_workspaces.py | 12 ++--- 7 files changed, 106 insertions(+), 32 deletions(-) diff --git a/backend/druks/core/apis/github.py b/backend/druks/core/apis/github.py index 9e0b2fe8..fa1afcd3 100644 --- a/backend/druks/core/apis/github.py +++ b/backend/druks/core/apis/github.py @@ -66,7 +66,7 @@ def __init__( auto_retry=RETRY_SERVER_ERROR, ) - async def get_installation_id(self, owner_or_repo: str) -> int: + async def get_installation(self, owner_or_repo: str): """The installation that covers a repository ``owner/name``, or an account.""" owner, _, name = owner_or_repo.partition("/") apps = self._app.rest.apps @@ -80,7 +80,7 @@ async def get_installation_id(self, owner_or_repo: str) -> int: ] for lookup in lookups: try: - return (await lookup()).parsed_data.id + return (await lookup()).parsed_data except RequestFailed as error: if error.response.status_code != 404: raise diff --git a/backend/druks/core/services.py b/backend/druks/core/services.py index 317b7c41..4fd764af 100644 --- a/backend/druks/core/services.py +++ b/backend/druks/core/services.py @@ -11,7 +11,7 @@ import httpx from githubkit import GitHub -from githubkit.exception import RequestError +from githubkit.exception import RequestError, RequestFailed from githubkit.retry import RETRY_SERVER_ERROR from pydantic import BaseModel, Field, SecretStr from slack_sdk.errors import SlackApiError @@ -95,21 +95,40 @@ async def issue_token( app = cls.service._app_client(row) first, *others = resources try: - installation_id = await app.get_installation_id(first) + installation = await app.get_installation(first) except exceptions.GitHubAppNotInstalledError as error: raise MissingSecretError(str(error)) from error - names = [first.split("/", 1)[1]] + owner, repo_name = first.split("/", 1) + names = [repo_name] for repo in others: - # GitHub refuses the whole token when one named repository is out of reach. - with suppress(exceptions.GitHubAppNotInstalledError): - if await app.get_installation_id(repo) == installation_id: - names.append(repo.split("/", 1)[1]) + repo_owner, repo_name = repo.split("/", 1) + # An installation is on one account, so a repo of another owner is never in it. + if repo_owner.lower() != owner.lower(): + continue + if installation.repository_selection == "selected": + try: + await app.get_installation(repo) + except exceptions.GitHubAppNotInstalledError: + continue + names.append(repo_name) names.sort() - return await VaultSecret.get_or_mint_token( - row.session, - Audience.resource(cls.service.slug, "installation", installation_id, *names), - on_miss=functools.partial(app.create_installation_token, installation_id, names), - ) + try: + return await VaultSecret.get_or_mint_token( + row.session, + Audience.resource(cls.service.slug, "installation", installation.id, *names), + on_miss=functools.partial( + app.create_installation_token, installation.id, names + ), + ) + except RequestFailed as error: + if error.response.status_code == 422: + raise MissingSecretError( + "GitHub refused a token for " + f"{', '.join(f'{owner}/{repo_name}' for repo_name in names)}. GitHub has " + "no repository under one of these names. Remove the renamed or deleted " + "repository from the project." + ) from error + raise @classmethod def get_manifest(cls, *, endpoint: str, webhook_base: str) -> dict[str, Any]: @@ -165,7 +184,7 @@ async def get_client(cls, owner_or_repo: str) -> github.GitHubClient: an App reinstall mints for the new one.""" row = await cls.get() app = cls._app_client(row) - installation_id = await app.get_installation_id(owner_or_repo) + installation_id = (await app.get_installation(owner_or_repo)).id token, _ = await VaultSecret.get_or_mint_token( row.session, Audience.resource(cls.slug, "installation", installation_id), @@ -179,7 +198,7 @@ async def revoke_tokens(cls, owner_or_repo: str) -> None: repository or an account, so the next fetch mints a new one. GitHub can refuse a token before its expiry, and the vault would serve it until then.""" row = await cls.get() - installation_id = await cls._app_client(row).get_installation_id(owner_or_repo) + installation_id = (await cls._app_client(row).get_installation(owner_or_repo)).id # The issuer reads the vault in its own session, so this commits on its own. async with get_session(row.session.bind) as own: await VaultSecret.revoke_tokens( diff --git a/backend/tests/test_github.py b/backend/tests/test_github.py index ead380ab..84f1a8f9 100644 --- a/backend/tests/test_github.py +++ b/backend/tests/test_github.py @@ -258,7 +258,7 @@ async def test_an_account_installation_falls_back_to_the_user_endpoint() -> None ), ) - assert await _TestGitHubAppClient(apps).get_installation_id("czpython") == 42 + assert (await _TestGitHubAppClient(apps).get_installation("czpython")).id == 42 apps.async_get_org_installation.assert_awaited_once_with("czpython") apps.async_get_user_installation.assert_awaited_once_with("czpython") @@ -271,7 +271,7 @@ async def test_an_org_installation_does_not_consult_the_user_endpoint() -> None: async_get_user_installation=AsyncMock(), ) - assert await _TestGitHubAppClient(apps).get_installation_id("clawhaven") == 41 + assert (await _TestGitHubAppClient(apps).get_installation("clawhaven")).id == 41 apps.async_get_user_installation.assert_not_awaited() @@ -285,7 +285,7 @@ async def test_installation_lookup_404_names_the_repo() -> None: ) with pytest.raises(GitHubAppNotInstalledError) as raised: - await _TestGitHubAppClient(apps).get_installation_id("ClawHaven/acme-app") + await _TestGitHubAppClient(apps).get_installation("ClawHaven/acme-app") assert "The GitHub App example-app has no access to ClawHaven/acme-app" in str(raised.value) assert "install the app" in str(raised.value) diff --git a/backend/tests/test_github_channel.py b/backend/tests/test_github_channel.py index a2043676..ad48d999 100644 --- a/backend/tests/test_github_channel.py +++ b/backend/tests/test_github_channel.py @@ -32,13 +32,13 @@ @pytest.fixture async def card(druks_db, monkeypatch): - async def get_installation_id(self, owner_or_repo): - return 1 + async def get_installation(self, owner_or_repo): + return SimpleNamespace(id=1) async def create_installation_token(self, installation_id, repositories=()): return "ghs_test", datetime.now(UTC) + timedelta(hours=1) - monkeypatch.setattr(GitHubAppClient, "get_installation_id", get_installation_id) + monkeypatch.setattr(GitHubAppClient, "get_installation", get_installation) monkeypatch.setattr(GitHubAppClient, "create_installation_token", create_installation_token) bind_ambient_session(druks_db) card = await connect_service( diff --git a/backend/tests/test_managed_github.py b/backend/tests/test_managed_github.py index 8867f665..b430bc82 100644 --- a/backend/tests/test_managed_github.py +++ b/backend/tests/test_managed_github.py @@ -19,6 +19,7 @@ from druks.redis import get_client from druks.secrets.datastructures import Audience from druks.secrets.enums import SecretKind +from druks.secrets.exceptions import MissingSecretError from druks.secrets.models import VaultSecret from druks.services import Connection, Service from druks.settings import configure_settings @@ -48,6 +49,8 @@ """ # This Druks's installations, by account login. INSTALLATIONS = {"acme": 11, "paulo": 22} +SELECTED_REPOSITORIES = {"paulo": ("site", "blog")} +DELETED_REPOSITORIES = ("gone",) EVENTS = "/_external/github/events/" _INSTANCE_PEM = ( @@ -93,13 +96,18 @@ def __call__( async def _installation_of(self, owner: str, name: str) -> SimpleNamespace: self.calls.append(f"GET repos/{owner}/{name}/installation") - if owner not in INSTALLATIONS: + selected = SELECTED_REPOSITORIES.get(owner) + if owner not in INSTALLATIONS or (selected and name not in selected): raise _refused(404) - return SimpleNamespace(parsed_data=SimpleNamespace(id=INSTALLATIONS[owner])) + selection = "selected" if selected else "all" + installation = SimpleNamespace(id=INSTALLATIONS[owner], repository_selection=selection) + return SimpleNamespace(parsed_data=installation) async def _mint(self, installation_id: int, repositories: object) -> SimpleNamespace: call = f"POST app/installations/{installation_id}/access_tokens" self.calls.append(f"{call} {repositories}" if repositories else call) + if set(DELETED_REPOSITORIES) & set(repositories or ()): + raise _refused(422) self.minted += 1 expires_at = (datetime.now(UTC) + timedelta(hours=1)).strftime("%Y-%m-%dT%H:%M:%SZ") token = SimpleNamespace(token=f"ghs_{installation_id}_{self.minted}", expires_at=expires_at) @@ -295,6 +303,53 @@ async def test_a_box_token_names_the_repos_its_first_repos_installation_covers( ] +async def test_an_installation_on_all_of_an_owners_repos_is_looked_up_once_per_token( + managed_github, githubkit, monkeypatch +): + repos = ("acme/web", "acme/api", "acme/docs") + await Github.issue_token(repos) + monkeypatch.setitem(INSTALLATIONS, "acme", 33) + await Github.issue_token(repos) + + assert githubkit.calls == [ + "GET repos/acme/web/installation", + "POST app/installations/11/access_tokens ['api', 'docs', 'web']", + "GET repos/acme/web/installation", + "POST app/installations/33/access_tokens ['api', 'docs', 'web']", + ] + + +async def test_an_installation_on_selected_repos_is_looked_up_for_each_repo( + managed_github, githubkit +): + await Github.issue_token(("paulo/site", "acme/web", "paulo/blog", "acme/api", "paulo/private")) + + assert githubkit.calls == [ + "GET repos/paulo/site/installation", + "GET repos/paulo/blog/installation", + "GET repos/paulo/private/installation", + "POST app/installations/22/access_tokens ['blog', 'site']", + ] + + +async def test_a_stale_repo_name_stops_the_token_with_the_fix(managed_github, githubkit): + with pytest.raises(MissingSecretError) as error: + await Github.issue_token(("acme/web", "acme/gone")) + + assert "acme/gone" in str(error.value) + assert "Remove the renamed or deleted repository from the project." in str(error.value) + assert githubkit.calls == [ + "GET repos/acme/web/installation", + "POST app/installations/11/access_tokens ['gone', 'web']", + ] + + +async def test_an_owner_in_another_letter_case_is_the_same_owner(managed_github, githubkit): + await Github.issue_token(("acme/web", "Acme/api")) + + assert githubkit.calls[-1] == "POST app/installations/11/access_tokens ['api', 'web']" + + async def test_install_binds_at_the_manager_and_links_the_installer( managed_github, githubkit, manager, github_identity ): diff --git a/backend/tests/test_sandbox_identities.py b/backend/tests/test_sandbox_identities.py index 04d351b4..f103c4ab 100644 --- a/backend/tests/test_sandbox_identities.py +++ b/backend/tests/test_sandbox_identities.py @@ -444,10 +444,10 @@ async def issue_token(resources, *, name): async def test_the_issuer_answers_422_with_the_reason_when_the_app_is_not_on_the_repo( druks_db, tmp_path, monkeypatch ): - async def get_installation_id(self, owner_or_repo: str) -> int: + async def get_installation(self, owner_or_repo: str): raise GitHubAppNotInstalledError(owner_or_repo, app="druks-reviewer") - monkeypatch.setattr(GitHubAppClient, "get_installation_id", get_installation_id) + monkeypatch.setattr(GitHubAppClient, "get_installation", get_installation) await _connect_github() identity, bearer = await _github_identity() diff --git a/backend/tests/test_workspaces.py b/backend/tests/test_workspaces.py index b09c477c..785c77e3 100644 --- a/backend/tests/test_workspaces.py +++ b/backend/tests/test_workspaces.py @@ -171,14 +171,14 @@ async def test_the_github_service_issues_its_installation_token( expiry = datetime.now(UTC) + timedelta(hours=1) asked: list[tuple] = [] - async def get_installation_id(self, owner_or_repo: str) -> int: - return 7 + async def get_installation(self, owner_or_repo: str) -> SimpleNamespace: + return SimpleNamespace(id=7) async def create_installation_token(self, installation_id, repositories=()): asked.append((self._app_id, installation_id, *repositories)) return "ghs_operator", expiry - monkeypatch.setattr(GitHubAppClient, "get_installation_id", get_installation_id) + monkeypatch.setattr(GitHubAppClient, "get_installation", get_installation) monkeypatch.setattr(GitHubAppClient, "create_installation_token", create_installation_token) await connect_service( "github", @@ -193,14 +193,14 @@ async def create_installation_token(self, installation_id, repositories=()): def _mint_tokens(monkeypatch: pytest.MonkeyPatch) -> list[str]: minted: list[str] = [] - async def get_installation_id(self, owner_or_repo: str) -> int: - return 7 + async def get_installation(self, owner_or_repo: str) -> SimpleNamespace: + return SimpleNamespace(id=7) async def create_installation_token(self, installation_id, repositories=()): minted.append(f"ghs_{len(minted)}") return minted[-1], datetime.now(UTC) + timedelta(hours=1) - monkeypatch.setattr(GitHubAppClient, "get_installation_id", get_installation_id) + monkeypatch.setattr(GitHubAppClient, "get_installation", get_installation) monkeypatch.setattr(GitHubAppClient, "create_installation_token", create_installation_token) return minted