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