DRU-1130 -- druks: a build's clone fails with 503 when its project has many repos

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

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(

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)

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(

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
 ):

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()
 

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