From 22d47cd09ac70df934843ecea060086065056764 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 07:58:09 +0200 Subject: [PATCH 01/22] add config options for github_app_* --- charm/charmcraft.yaml | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/charm/charmcraft.yaml b/charm/charmcraft.yaml index 608f46df..65c21d37 100644 --- a/charm/charmcraft.yaml +++ b/charm/charmcraft.yaml @@ -48,9 +48,35 @@ config: write and higher permissions for the repository to run jobs from forks. type: boolean default: false + github_app_id: + description: >- + The app or client ID of the GitHub App to use for communication with GitHub. + If provided, the other github_app_* options must also be provided. + The Github App needs to have read permission for Administration. If private repositories + are checked, the Github App does also need read permission for Contents and Pull request. + Either this or the github_token must be provided. + type: string + github_app_installation_id: + description: >- + The installation ID of the GitHub App to use for communication with GitHub. + If provided, the other github_app_* options must also be provided. + The Github App needs to have read permission for Administration. If private repositories + are checked, the Github App does also need read permission for Contents and Pull request. + Either this or the github_token must be provided. + type: string + github_app_private_key: + # this will become a juju user secret once paas-app-charmer supports it + description: >- + The private key of the GitHub App to use for communication with GitHub. + If provided, the other github_app_* options must also be provided. + The Github App needs to have read permission for Administration. If private repositories + are checked, the Github App does also need read permission for Contents and Pull request. + Either this or the github_token must be provided. + type: string github_token: description: >- The token to use for communication with GitHub. This can be a PAT (with repo scope) or a fine-grained token with read permission for Administration. If private repositories are checked, the fine-grained token does also need read permission for Contents and Pull request. + Either this or the GitHub App configuration must be provided. From cde24756e431d9104dee8ace94b9bb6e60d658e0 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 10:56:43 +0200 Subject: [PATCH 02/22] add github app auth --- repo_policy_compliance/github_client.py | 144 ++++++++++++++++++++++-- src-docs/github_client.py.md | 18 ++- tests/app/unit/test_github_client.py | 68 ++++++++++- 3 files changed, 213 insertions(+), 17 deletions(-) diff --git a/repo_policy_compliance/github_client.py b/repo_policy_compliance/github_client.py index 3b62a4e9..98214379 100644 --- a/repo_policy_compliance/github_client.py +++ b/repo_policy_compliance/github_client.py @@ -10,7 +10,7 @@ from urllib import parse from github import BadCredentialsException, Github, GithubException, RateLimitExceededException -from github.Auth import Token +from github.Auth import AppAuth, AppInstallationAuth, Auth, Token from github.Branch import Branch from github.Repository import Repository from urllib3 import Retry @@ -27,6 +27,24 @@ # Bandit thinks this constant is the real Github token GITHUB_TOKEN_ENV_NAME = "GITHUB_TOKEN" # nosec +GITHUB_APP_ID_NAME = "GITHUB_APP_ID" +GITHUB_APP_INSTALLATION_ID_NAME = "GITHUB_APP_INSTALLATION_ID" +GITHUB_APP_PRIVATE_KEY_NAME = "GITHUB_APP_PRIVATE_KEY" + +MISSING_GITHUB_CONFIG_ERR_MSG = ( + f"Either the {GITHUB_TOKEN_ENV_NAME} or not all of {GITHUB_APP_ID_NAME}," + f" {GITHUB_APP_INSTALLATION_ID_NAME}, {GITHUB_APP_PRIVATE_KEY_NAME} " + f"environment variables were not provided or empty, " + "it is needed for interactions with GitHub, " +) +NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG = ( + f"Not all of {GITHUB_APP_ID_NAME}, {GITHUB_APP_INSTALLATION_ID_NAME}," + f" {GITHUB_APP_PRIVATE_KEY_NAME} environment variables were provided, " +) +# the following is no hardcoded password +PROVIDED_GITHUB_TOKEN_AND_APP_CONFIG_ERR_MSG = ( # nosec + "Provided github app config and github token, only one of them should be provided, " +) def get() -> Github: @@ -36,14 +54,10 @@ def get() -> Github: A GitHub client that is configured with a token from the environment. Raises: - ConfigurationError: If the GitHub token environment variable is not provided or empty. - """ - github_token = os.getenv(GITHUB_TOKEN_ENV_NAME) or os.getenv(f"FLASK_{GITHUB_TOKEN_ENV_NAME}") - if not github_token: - raise ConfigurationError( - f"The {GITHUB_TOKEN_ENV_NAME} environment variable was not provided or empty, " - f"it is needed for interactions with GitHub, got: {github_token!r}" - ) + ConfigurationError: If the GitHub auth config is not valid. + """ # noqa: DCO051 error raised is useful to know for the user of the public interface + auth = _get_auth() + # Only retry on 5xx and only retry once after 20 secs retry_config = Retry( total=1, @@ -53,7 +67,117 @@ def get() -> Github: raise_on_status=False, raise_on_redirect=False, ) - return Github(auth=Token(github_token), retry=retry_config) + return Github(auth=auth, retry=retry_config) + + +def _get_auth() -> Auth: + """Get a GitHub auth object. + + Returns: + A GitHub auth object that is configured with a token from the environment. + """ + github_token = os.getenv(GITHUB_TOKEN_ENV_NAME) or os.getenv(f"FLASK_{GITHUB_TOKEN_ENV_NAME}") + github_app_id = os.getenv(GITHUB_APP_ID_NAME) or os.getenv(f"FLASK_{GITHUB_APP_ID_NAME}") + github_app_installation_id_str = os.getenv(GITHUB_APP_INSTALLATION_ID_NAME) or os.getenv( + f"FLASK_{GITHUB_APP_INSTALLATION_ID_NAME}" + ) + github_app_private_key = os.getenv(GITHUB_APP_PRIVATE_KEY_NAME) or os.getenv( + f"FLASK_{GITHUB_APP_PRIVATE_KEY_NAME}" + ) + + _ensure_either_github_token_or_app_config( + github_token=github_token, + github_app_id=github_app_id, + github_app_installation_id_str=github_app_installation_id_str, + github_app_private_key=github_app_private_key, + ) + + auth: Auth + # we use asserts here to make mypy happy, we have already checked that the values are not None + # we know asserts are optimised away in production, so ignore bandit warnings + if github_app_id: + assert github_app_installation_id_str is not None # nosec + assert github_app_private_key is not None # nosec + auth = _get_github_app_installation_auth( + github_app_id=github_app_id, + github_app_installation_id_str=github_app_installation_id_str, + github_app_private_key=github_app_private_key, + ) + else: + assert github_token is not None # nosec + auth = Token(github_token) + + return auth + + +def _ensure_either_github_token_or_app_config( + github_token: str | None, + github_app_id: str | None, + github_app_installation_id_str: str | None, + github_app_private_key: str | None, +) -> None: + """Ensure that only one of github_token or github app config is provided and is valid. + + Args: + github_token: The GitHub token. + github_app_id: The GitHub App ID or Client ID. + github_app_installation_id_str: The GitHub App Installation ID as a string. + github_app_private_key: The GitHub App private key. + + Raises: + ConfigurationError: If both github_token and github app config are provided. + """ + if not github_token and not ( + github_app_id or github_app_installation_id_str or github_app_private_key + ): + raise ConfigurationError( + f"{MISSING_GITHUB_CONFIG_ERR_MSG}" + f"got: {github_token!r}, {github_app_id!r}," + f" {github_app_installation_id_str!r}, {github_app_private_key!r}" + ) + if github_token and ( + github_app_id or github_app_installation_id_str or github_app_private_key + ): + raise ConfigurationError( + f"{PROVIDED_GITHUB_TOKEN_AND_APP_CONFIG_ERR_MSG}" + f"got: {github_token!r}, {github_app_id!r}, {github_app_installation_id_str!r}," + f" {github_app_private_key!r}" + ) + + if github_app_id or github_app_installation_id_str or github_app_private_key: + if not (github_app_id and github_app_installation_id_str and github_app_private_key): + raise ConfigurationError( + f"{NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG}" + f"got: {github_app_id!r}, {github_app_installation_id_str!r}, " + f"{github_app_private_key!r}" + ) + + +def _get_github_app_installation_auth( + github_app_id: str, github_app_installation_id_str: str, github_app_private_key: str +) -> AppInstallationAuth: + """Get a GitHub App Installation Auth object. + + Args: + github_app_id: The GitHub App ID or Client ID. + github_app_installation_id_str: The GitHub App Installation ID as a string. + github_app_private_key: The GitHub App private key. + + Returns: + A GitHub App Installation Auth object. + + Raises: + ConfigurationError: If the GitHub App Installation Auth config is not valid. + """ + try: + github_app_installation_id = int(github_app_installation_id_str) + except ValueError as exc: + raise ConfigurationError( + f"Invalid github app installation id {github_app_installation_id_str!r}, " + f"it should be an integer." + ) from exc + app_auth = AppAuth(app_id=github_app_id, private_key=github_app_private_key) + return AppInstallationAuth(app_auth=app_auth, installation_id=github_app_installation_id) def inject(func: Callable[Concatenate[Github, P], R]) -> Callable[P, R]: diff --git a/src-docs/github_client.py.md b/src-docs/github_client.py.md index 09a5cb7a..28957794 100644 --- a/src-docs/github_client.py.md +++ b/src-docs/github_client.py.md @@ -8,10 +8,16 @@ Module for GitHub client. **Global Variables** --------------- - **GITHUB_TOKEN_ENV_NAME** +- **GITHUB_APP_ID_NAME** +- **GITHUB_APP_INSTALLATION_ID_NAME** +- **GITHUB_APP_PRIVATE_KEY_NAME** +- **MISSING_GITHUB_CONFIG_ERR_MSG** +- **NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG** +- **PROVIDED_GITHUB_TOKEN_AND_APP_CONFIG_ERR_MSG** --- - + ## function `get` @@ -30,12 +36,12 @@ Get a GitHub client. **Raises:** - - `ConfigurationError`: If the GitHub token environment variable is not provided or empty. + - `ConfigurationError`: If the GitHub auth config is not valid. --- - + ## function `inject` @@ -59,7 +65,7 @@ Injects a GitHub client as the first argument to a function. --- - + ## function `get_collaborators` @@ -89,7 +95,7 @@ Get collaborators with a given affiliation and permission. --- - + ## function `get_branch` @@ -119,7 +125,7 @@ Get the branch for the check. --- - + ## function `get_collaborator_permission` diff --git a/tests/app/unit/test_github_client.py b/tests/app/unit/test_github_client.py index 63207455..119466f0 100644 --- a/tests/app/unit/test_github_client.py +++ b/tests/app/unit/test_github_client.py @@ -11,7 +11,7 @@ import repo_policy_compliance.github_client from repo_policy_compliance.check import Result, target_branch_protection -from repo_policy_compliance.exceptions import GithubClientError +from repo_policy_compliance.exceptions import ConfigurationError, GithubClientError GITHUB_REPOSITORY_NAME = "test/repository" GITHUB_BRANCH_NAME = "arbitrary" @@ -74,3 +74,69 @@ def test_get_collaborator_permission_error(): mock_repository, "test_user" ) assert "Invalid collaborator permission" in str(error.value) + + +@pytest.mark.parametrize( + "github_app_id, github_app_installation_id, github_app_private_key, github_token, " + "expected_message", + [ + pytest.param( + "123", + "456", + "private", + "github_token", + repo_policy_compliance.github_client.PROVIDED_GITHUB_TOKEN_AND_APP_CONFIG_ERR_MSG, + id="github app config and github token", + ), + pytest.param( + None, + None, + None, + None, + repo_policy_compliance.github_client.MISSING_GITHUB_CONFIG_ERR_MSG, + id="no github app config or github token", + ), + pytest.param( + "eda", + "no int", + "private", + None, + "Invalid github app installation id", + id="invalid github app installation id", + ), + pytest.param( + "eda", + "123", + None, + None, + repo_policy_compliance.github_client.NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG, + id="not all github app config provided", + ), + ], +) # we use a lot of arguments but it seems not worth to introduce a capsulating object for this +def test_get_client_configuration_error( # pylint: disable=too-many-arguments + github_app_id: str, + github_app_installation_id: str, + github_app_private_key: str, + github_token: str, + expected_message: str, + monkeypatch: pytest.MonkeyPatch, +): + """ + arrange: Given a mocked get_client_configuration function that returns invalid value. + act: when get_client_configuration is called. + assert: GithubClientError is raised. + """ + if github_app_id: + monkeypatch.setenv("GITHUB_APP_ID", github_app_id) + if github_app_installation_id: + monkeypatch.setenv("GITHUB_APP_INSTALLATION_ID", github_app_installation_id) + if github_app_private_key: + monkeypatch.setenv("GITHUB_APP_PRIVATE_KEY", github_app_private_key) + if github_token: + monkeypatch.setenv("GITHUB_TOKEN", github_token) + + with pytest.raises(ConfigurationError) as error: + # The github_client is injected + repo_policy_compliance.github_client.get() + assert expected_message in str(error.value) From ec37ec231f41f9147d116cdee02cc07128ae827f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 12:54:07 +0200 Subject: [PATCH 03/22] add/adapt integration tests for github app auth --- .github/workflows/tests.yaml | 5 ++- README.md | 10 ++++- repo_policy_compliance/github_client.py | 26 ++++++----- src-docs/github_client.py.md | 14 +++--- tests/app/integration/conftest.py | 60 +++++++++++++++++++++---- 5 files changed, 86 insertions(+), 29 deletions(-) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 43ef75ff..b5583fe4 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -18,7 +18,10 @@ jobs: - name: Run tests (unit + integration) id: run-tests env: - GITHUB_TOKEN: ${{ secrets.PERSONAL_GITHUB_TOKEN }} + AUTH_GITHUB_TOKEN: ${{ secrets.PERSONAL_GITHUB_TOKEN }} + AUTH_GITHUB_APP_ID : ${{ secrets.TEST_GITHUB_APP_ID }} + AUTH_GITHUB_APP_INSTALLATION_ID : ${{ secrets.TEST_GITHUB_APP_INSTALLATION_ID }} + AUTH_GITHUB_APP_PRIVATE_KEY : ${{ secrets.TEST_GITHUB_APP_PRIVATE_KEY }} run: | # Ensure that stdout appears as normal and redirect to file and exit depends on exit code of first command STDOUT_LOG=$(mktemp --suffix=stdout.log) diff --git a/README.md b/README.md index de47bacd..5078c34c 100644 --- a/README.md +++ b/README.md @@ -46,8 +46,14 @@ failing check to be used for testing purposes. There are two types of test: the application test and the charm test. ### Application tests -To run the application tests, the `GITHUB_TOKEN` environment variable must be set. This +To run the application tests, the `AUTH_GITHUB_TOKEN` environment variable must be set. This should be a token of a user with full repo permissions for the test repository. +You can also pass in `AUTH_APP_ID`, `AUTH_INSTALLATION_ID`, and `AUTH_PRIVATE_KEY` +to test the authentication using Github App Auth. In that case, the tests will randomly select +either the token or the app auth to run the tests. Note that the Github app should be installed +in the test repository organisation/user namespace, with access granted to the test repository +and in the user namespace that owns the Github token with access to the forked repository. + The command `tox -e test` can be used to run all tests, which are primarily integration tests. You can also select the repository against which to run the tests by setting the `--repository` flag. The tests will fork the repository and create PRs against it. @@ -66,6 +72,8 @@ bot to test things like comments from a user with no write permissions or above. GitHub actions should have access to the GitHub token via a secret called `PERSONAL_GITHUB_TOKEN`. It is recommended to use either a fine-grained PAT or a token that is short-lived, e.g. 7 days. When it expires, a new token must be set. +For the GitHub App Auth, the `TEST_GITHUB_APP_ID`, `TEST_GIHUB_APP_INSTALLATION_ID`, and `TEST_GITHUB_APPP_RIVATE_KEY` +should be set as secrets. ### Charm tests diff --git a/repo_policy_compliance/github_client.py b/repo_policy_compliance/github_client.py index 98214379..8634127b 100644 --- a/repo_policy_compliance/github_client.py +++ b/repo_policy_compliance/github_client.py @@ -27,19 +27,19 @@ # Bandit thinks this constant is the real Github token GITHUB_TOKEN_ENV_NAME = "GITHUB_TOKEN" # nosec -GITHUB_APP_ID_NAME = "GITHUB_APP_ID" -GITHUB_APP_INSTALLATION_ID_NAME = "GITHUB_APP_INSTALLATION_ID" -GITHUB_APP_PRIVATE_KEY_NAME = "GITHUB_APP_PRIVATE_KEY" +GITHUB_APP_ID_ENV_NAME = "GITHUB_APP_ID" +GITHUB_APP_INSTALLATION_ID_ENV_NAME = "GITHUB_APP_INSTALLATION_ID" +GITHUB_APP_PRIVATE_KEY_ENV_NAME = "GITHUB_APP_PRIVATE_KEY" MISSING_GITHUB_CONFIG_ERR_MSG = ( - f"Either the {GITHUB_TOKEN_ENV_NAME} or not all of {GITHUB_APP_ID_NAME}," - f" {GITHUB_APP_INSTALLATION_ID_NAME}, {GITHUB_APP_PRIVATE_KEY_NAME} " + f"Either the {GITHUB_TOKEN_ENV_NAME} or not all of {GITHUB_APP_ID_ENV_NAME}," + f" {GITHUB_APP_INSTALLATION_ID_ENV_NAME}, {GITHUB_APP_PRIVATE_KEY_ENV_NAME} " f"environment variables were not provided or empty, " "it is needed for interactions with GitHub, " ) NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG = ( - f"Not all of {GITHUB_APP_ID_NAME}, {GITHUB_APP_INSTALLATION_ID_NAME}," - f" {GITHUB_APP_PRIVATE_KEY_NAME} environment variables were provided, " + f"Not all of {GITHUB_APP_ID_ENV_NAME}, {GITHUB_APP_INSTALLATION_ID_ENV_NAME}," + f" {GITHUB_APP_PRIVATE_KEY_ENV_NAME} environment variables were provided, " ) # the following is no hardcoded password PROVIDED_GITHUB_TOKEN_AND_APP_CONFIG_ERR_MSG = ( # nosec @@ -77,12 +77,14 @@ def _get_auth() -> Auth: A GitHub auth object that is configured with a token from the environment. """ github_token = os.getenv(GITHUB_TOKEN_ENV_NAME) or os.getenv(f"FLASK_{GITHUB_TOKEN_ENV_NAME}") - github_app_id = os.getenv(GITHUB_APP_ID_NAME) or os.getenv(f"FLASK_{GITHUB_APP_ID_NAME}") - github_app_installation_id_str = os.getenv(GITHUB_APP_INSTALLATION_ID_NAME) or os.getenv( - f"FLASK_{GITHUB_APP_INSTALLATION_ID_NAME}" + github_app_id = os.getenv(GITHUB_APP_ID_ENV_NAME) or os.getenv( + f"FLASK_{GITHUB_APP_ID_ENV_NAME}" ) - github_app_private_key = os.getenv(GITHUB_APP_PRIVATE_KEY_NAME) or os.getenv( - f"FLASK_{GITHUB_APP_PRIVATE_KEY_NAME}" + github_app_installation_id_str = os.getenv(GITHUB_APP_INSTALLATION_ID_ENV_NAME) or os.getenv( + f"FLASK_{GITHUB_APP_INSTALLATION_ID_ENV_NAME}" + ) + github_app_private_key = os.getenv(GITHUB_APP_PRIVATE_KEY_ENV_NAME) or os.getenv( + f"FLASK_{GITHUB_APP_PRIVATE_KEY_ENV_NAME}" ) _ensure_either_github_token_or_app_config( diff --git a/src-docs/github_client.py.md b/src-docs/github_client.py.md index 28957794..38066eb5 100644 --- a/src-docs/github_client.py.md +++ b/src-docs/github_client.py.md @@ -8,9 +8,9 @@ Module for GitHub client. **Global Variables** --------------- - **GITHUB_TOKEN_ENV_NAME** -- **GITHUB_APP_ID_NAME** -- **GITHUB_APP_INSTALLATION_ID_NAME** -- **GITHUB_APP_PRIVATE_KEY_NAME** +- **GITHUB_APP_ID_ENV_NAME** +- **GITHUB_APP_INSTALLATION_ID_ENV_NAME** +- **GITHUB_APP_PRIVATE_KEY_ENV_NAME** - **MISSING_GITHUB_CONFIG_ERR_MSG** - **NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG** - **PROVIDED_GITHUB_TOKEN_AND_APP_CONFIG_ERR_MSG** @@ -41,7 +41,7 @@ Get a GitHub client. --- - + ## function `inject` @@ -65,7 +65,7 @@ Injects a GitHub client as the first argument to a function. --- - + ## function `get_collaborators` @@ -95,7 +95,7 @@ Get collaborators with a given affiliation and permission. --- - + ## function `get_branch` @@ -125,7 +125,7 @@ Get the branch for the check. --- - + ## function `get_collaborator_permission` diff --git a/tests/app/integration/conftest.py b/tests/app/integration/conftest.py index e3a7396f..31ff0ad0 100644 --- a/tests/app/integration/conftest.py +++ b/tests/app/integration/conftest.py @@ -2,8 +2,9 @@ # See LICENSE file for licensing details. """Fixtures for integration tests.""" - +import logging import os +import random from typing import Iterator, cast import pytest @@ -16,13 +17,54 @@ from github.Repository import Repository import repo_policy_compliance -from repo_policy_compliance.github_client import GITHUB_TOKEN_ENV_NAME, get_collaborators -from repo_policy_compliance.github_client import inject as inject_github_client +from repo_policy_compliance.github_client import ( + GITHUB_APP_ID_ENV_NAME, + GITHUB_APP_INSTALLATION_ID_ENV_NAME, + GITHUB_APP_PRIVATE_KEY_ENV_NAME, + GITHUB_TOKEN_ENV_NAME, + get_collaborators, +) from ...conftest import REPOSITORY_ARGUMENT_NAME from . import branch_protection from .types_ import BranchWithProtection, RequestedCollaborator +logger = logging.getLogger(__name__) + +TEST_GITHUB_APP_ID_ENV_NAME = f"AUTH_{GITHUB_APP_ID_ENV_NAME}" +TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME = f"AUTH_{GITHUB_APP_INSTALLATION_ID_ENV_NAME}" +TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME = f"AUTH_{GITHUB_APP_PRIVATE_KEY_ENV_NAME}" +TEST_GITHUB_TOKEN_ENV_NAME = f"AUTH_{GITHUB_TOKEN_ENV_NAME}" + + +@pytest.fixture(scope="session", name="randomize_github_auth_method", autouse=True) +def fixture_randomize_github_auth_method(monkeypatch: pytest.MonkeyPatch) -> None: + """Randomize the GitHub authentication method. + + Either use Github Token auth or Github App auth if the latter is set. + This is achieved by monkeypatching the environment variables. + """ + app_id = os.getenv(TEST_GITHUB_APP_ID_ENV_NAME) + app_install_id = os.getenv(TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME) + app_private_key = os.getenv(TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME) + github_token = os.getenv(TEST_GITHUB_TOKEN_ENV_NAME) + + # random is not used for security purposes + if random.random() < 0.5 and app_id and app_install_id and app_private_key: # nosec + monkeypatch.delenv(GITHUB_TOKEN_ENV_NAME, raising=False) + monkeypatch.setenv(GITHUB_APP_ID_ENV_NAME, app_id) + monkeypatch.setenv(GITHUB_APP_INSTALLATION_ID_ENV_NAME, app_install_id) + monkeypatch.setenv(GITHUB_APP_PRIVATE_KEY_ENV_NAME, app_private_key) + logger.info("Using GitHub App authentication for this test.") + else: + assert ( + github_token + ), f"GitHub token must be set in the environment variable {TEST_GITHUB_TOKEN_ENV_NAME}" + monkeypatch.delenv(GITHUB_APP_ID_ENV_NAME, raising=False) + monkeypatch.delenv(GITHUB_APP_INSTALLATION_ID_ENV_NAME, raising=False) + monkeypatch.delenv(GITHUB_APP_PRIVATE_KEY_ENV_NAME, raising=False) + monkeypatch.setenv(GITHUB_TOKEN_ENV_NAME, github_token) + @pytest.fixture(scope="session", name="github_repository_name") def fixture_github_repository_name(pytestconfig: pytest.Config) -> str: @@ -31,10 +73,12 @@ def fixture_github_repository_name(pytestconfig: pytest.Config) -> str: @pytest.fixture(scope="session", name="github_token") -def fixutre_github_token() -> str: +def fixture_github_token() -> str: """Get the GitHub token from the environment.""" - github_token = os.getenv(GITHUB_TOKEN_ENV_NAME) - assert github_token, f"GitHub must be set in the environment variable {GITHUB_TOKEN_ENV_NAME}" + github_token = os.getenv(TEST_GITHUB_TOKEN_ENV_NAME) + assert ( + github_token + ), f"GitHub must be set in the environment variable {TEST_GITHUB_TOKEN_ENV_NAME}" return github_token @@ -64,9 +108,9 @@ def fixture_ci_github_repository( @pytest.fixture(scope="session", name="github_repository") -def fixture_github_repository(github_repository_name: str) -> Repository: +def fixture_github_repository(github_repository_name: str, github_token: str) -> Repository: """Returns client to the Github repository.""" - github_client = inject_github_client(lambda client: client)() + github_client = Github(auth=Token(github_token)) return github_client.get_repo(github_repository_name) From 04a6369b74d27360ab8d052e1e7fce2ae76041a3 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 12:57:07 +0200 Subject: [PATCH 04/22] fix scope of fixture --- tests/app/integration/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/integration/conftest.py b/tests/app/integration/conftest.py index 31ff0ad0..02756a68 100644 --- a/tests/app/integration/conftest.py +++ b/tests/app/integration/conftest.py @@ -37,7 +37,7 @@ TEST_GITHUB_TOKEN_ENV_NAME = f"AUTH_{GITHUB_TOKEN_ENV_NAME}" -@pytest.fixture(scope="session", name="randomize_github_auth_method", autouse=True) +@pytest.fixture(scope="function", name="randomize_github_auth_method", autouse=True) def fixture_randomize_github_auth_method(monkeypatch: pytest.MonkeyPatch) -> None: """Randomize the GitHub authentication method. From 17ef3e5cc4df85b3937fca5afe8d51670b96d6ed Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 13:34:39 +0200 Subject: [PATCH 05/22] pass env vars in tox --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 1ce10327..0a8f3b2b 100644 --- a/tox.ini +++ b/tox.ini @@ -71,7 +71,7 @@ commands = pylint {[vars]all_path} [testenv:test] -passenv = GITHUB_TOKEN, CI_GITHUB_TOKEN +passenv = AUTH_GITHUB_TOKEN, AUTH_GITHUB_APP_ID, AUTH_GITHUB_APP_INSTALLATION_ID, AUTH_GITHUB_APP_PRIVATE_KEY, CI_GITHUB_TOKEN description = Run tests deps = coverage[toml]>=6,<7 From db7d14c58980b8788291cf8721956c8297830aa8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 14:00:38 +0200 Subject: [PATCH 06/22] add unit test to ensure coverage --- tests/app/unit/test_github_client.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/tests/app/unit/test_github_client.py b/tests/app/unit/test_github_client.py index 119466f0..9f9cafa5 100644 --- a/tests/app/unit/test_github_client.py +++ b/tests/app/unit/test_github_client.py @@ -7,6 +7,7 @@ import pytest from github import BadCredentialsException, Github, GithubException, RateLimitExceededException +from github.Auth import AppInstallationAuth from github.Repository import Repository import repo_policy_compliance.github_client @@ -123,9 +124,9 @@ def test_get_client_configuration_error( # pylint: disable=too-many-arguments monkeypatch: pytest.MonkeyPatch, ): """ - arrange: Given a mocked get_client_configuration function that returns invalid value. - act: when get_client_configuration is called. - assert: GithubClientError is raised. + arrange: Given a mocked environment with invalid github auth configuration. + act: Call github_client.get. + assert: ConfigurationError is raised. """ if github_app_id: monkeypatch.setenv("GITHUB_APP_ID", github_app_id) @@ -140,3 +141,21 @@ def test_get_client_configuration_error( # pylint: disable=too-many-arguments # The github_client is injected repo_policy_compliance.github_client.get() assert expected_message in str(error.value) + + +def test_get_client_github_app_auth(monkeypatch: pytest.MonkeyPatch): + """ + arrange: Given a mocked environment with github app configuration and a mocked Github object. + act: Call github_client.get. + assert: The auth parameter of the Github object is an instance of AppInstallationAuth. + """ + monkeypatch.setenv("GITHUB_APP_ID", "123") + monkeypatch.setenv("GITHUB_APP_INSTALLATION_ID", "456") + monkeypatch.setenv("GITHUB_APP_PRIVATE_KEY", "private") + github_class_mock = MagicMock(spec=Github) + monkeypatch.setattr(repo_policy_compliance.github_client, "Github", github_class_mock) + + repo_policy_compliance.github_client.get() + github_class_mock.assert_called_once() + auth = github_class_mock.call_args[1]["auth"] + assert isinstance(auth, AppInstallationAuth) From 4836602e31a1572b29ee1820cae1d8b932bf8ba2 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 14:24:43 +0200 Subject: [PATCH 07/22] change tests to run additionally for github app auth --- README.md | 10 ++--- tests/app/integration/conftest.py | 72 ++++++++++++++++++++++--------- 2 files changed, 56 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 5078c34c..33aff04f 100644 --- a/README.md +++ b/README.md @@ -49,10 +49,10 @@ There are two types of test: the application test and the charm test. To run the application tests, the `AUTH_GITHUB_TOKEN` environment variable must be set. This should be a token of a user with full repo permissions for the test repository. You can also pass in `AUTH_APP_ID`, `AUTH_INSTALLATION_ID`, and `AUTH_PRIVATE_KEY` -to test the authentication using Github App Auth. In that case, the tests will randomly select -either the token or the app auth to run the tests. Note that the Github app should be installed +to test the authentication using GitHub App Auth. In that case, the tests will additionally +be executed using GitHub App auth. Note that the GitHub app should be installed in the test repository organisation/user namespace, with access granted to the test repository -and in the user namespace that owns the Github token with access to the forked repository. +and in the user namespace that owns the GitHub token with access to the forked repository. The command `tox -e test` can be used to run all tests, which are primarily integration tests. You can also select the repository against which to run the tests by setting @@ -72,8 +72,8 @@ bot to test things like comments from a user with no write permissions or above. GitHub actions should have access to the GitHub token via a secret called `PERSONAL_GITHUB_TOKEN`. It is recommended to use either a fine-grained PAT or a token that is short-lived, e.g. 7 days. When it expires, a new token must be set. -For the GitHub App Auth, the `TEST_GITHUB_APP_ID`, `TEST_GIHUB_APP_INSTALLATION_ID`, and `TEST_GITHUB_APPP_RIVATE_KEY` -should be set as secrets. +For the GitHub App Auth, the `TEST_GITHUB_APP_ID`, `TEST_GIHUB_APP_INSTALLATION_ID`, and +`TEST_GITHUB_APPP_RIVATE_KEY` should be set as secrets. ### Charm tests diff --git a/tests/app/integration/conftest.py b/tests/app/integration/conftest.py index 02756a68..d9eba3c2 100644 --- a/tests/app/integration/conftest.py +++ b/tests/app/integration/conftest.py @@ -4,7 +4,7 @@ """Fixtures for integration tests.""" import logging import os -import random +from collections import namedtuple from typing import Iterator, cast import pytest @@ -36,33 +36,63 @@ TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME = f"AUTH_{GITHUB_APP_PRIVATE_KEY_ENV_NAME}" TEST_GITHUB_TOKEN_ENV_NAME = f"AUTH_{GITHUB_TOKEN_ENV_NAME}" +AuthenticationMethod = namedtuple( + "AuthenticationMethod", ["app_id", "installation_id", "private_key", "github_token"] +) -@pytest.fixture(scope="function", name="randomize_github_auth_method", autouse=True) -def fixture_randomize_github_auth_method(monkeypatch: pytest.MonkeyPatch) -> None: - """Randomize the GitHub authentication method. - Either use Github Token auth or Github App auth if the latter is set. - This is achieved by monkeypatching the environment variables. +@pytest.fixture( + scope="function", + name="setup_github_auth_method", + autouse=True, + params=[ + AuthenticationMethod( + github_token=os.getenv(TEST_GITHUB_TOKEN_ENV_NAME), + app_id=None, + installation_id=None, + private_key=None, + ), + pytest.param( + AuthenticationMethod( + app_id=os.getenv(TEST_GITHUB_APP_ID_ENV_NAME), + installation_id=os.getenv(TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME), + private_key=os.getenv(TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME), + github_token=None, + ), + marks=pytest.mark.skipif( + not all( + [ + os.getenv(TEST_GITHUB_APP_ID_ENV_NAME), + os.getenv(TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME), + os.getenv(TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME), + ] + ) + ), + id="Using GitHub App authentication", + ), + ], +) +def fixture_setup_github_auth_method( + request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch +) -> None: + """Setup the GitHub authentication method. + + We want to test with GitHub Token authentication and optionally GitHub App authentication, + if the environment variables are set. + This is achieved by monkeypatching the respective environment variables. """ - app_id = os.getenv(TEST_GITHUB_APP_ID_ENV_NAME) - app_install_id = os.getenv(TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME) - app_private_key = os.getenv(TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME) - github_token = os.getenv(TEST_GITHUB_TOKEN_ENV_NAME) + app_id = request.param.app_id + app_install_id = request.param.installation_id + app_private_key = request.param.private_key + github_token = request.param.github_token - # random is not used for security purposes - if random.random() < 0.5 and app_id and app_install_id and app_private_key: # nosec - monkeypatch.delenv(GITHUB_TOKEN_ENV_NAME, raising=False) + if app_id: monkeypatch.setenv(GITHUB_APP_ID_ENV_NAME, app_id) + if app_install_id: monkeypatch.setenv(GITHUB_APP_INSTALLATION_ID_ENV_NAME, app_install_id) + if app_private_key: monkeypatch.setenv(GITHUB_APP_PRIVATE_KEY_ENV_NAME, app_private_key) - logger.info("Using GitHub App authentication for this test.") - else: - assert ( - github_token - ), f"GitHub token must be set in the environment variable {TEST_GITHUB_TOKEN_ENV_NAME}" - monkeypatch.delenv(GITHUB_APP_ID_ENV_NAME, raising=False) - monkeypatch.delenv(GITHUB_APP_INSTALLATION_ID_ENV_NAME, raising=False) - monkeypatch.delenv(GITHUB_APP_PRIVATE_KEY_ENV_NAME, raising=False) + if github_token: monkeypatch.setenv(GITHUB_TOKEN_ENV_NAME, github_token) From 5e1dc451cd211fd6c33cb403dfe3bfa0d2775e3c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 14:36:05 +0200 Subject: [PATCH 08/22] add reason to skipif --- tests/app/integration/conftest.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/app/integration/conftest.py b/tests/app/integration/conftest.py index d9eba3c2..70ae1458 100644 --- a/tests/app/integration/conftest.py +++ b/tests/app/integration/conftest.py @@ -46,11 +46,14 @@ name="setup_github_auth_method", autouse=True, params=[ - AuthenticationMethod( - github_token=os.getenv(TEST_GITHUB_TOKEN_ENV_NAME), - app_id=None, - installation_id=None, - private_key=None, + pytest.param( + AuthenticationMethod( + github_token=os.getenv(TEST_GITHUB_TOKEN_ENV_NAME), + app_id=None, + installation_id=None, + private_key=None, + ), + id="Using GitHub Token authentication", ), pytest.param( AuthenticationMethod( @@ -66,7 +69,8 @@ os.getenv(TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME), os.getenv(TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME), ] - ) + ), + reason="GitHub App Auth environment variables are not set", ), id="Using GitHub App authentication", ), From bccad203565a30cff104b4fd42856a227748270d Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 15:22:07 +0200 Subject: [PATCH 09/22] skip some tests for github app auth --- README.md | 3 +- tests/app/integration/conftest.py | 37 +++++++++++++++---- tests/app/integration/test_pull_request.py | 10 ++++- .../test_target_branch_protection.py | 11 +++++- 4 files changed, 50 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 33aff04f..88891d07 100644 --- a/README.md +++ b/README.md @@ -51,8 +51,7 @@ should be a token of a user with full repo permissions for the test repository. You can also pass in `AUTH_APP_ID`, `AUTH_INSTALLATION_ID`, and `AUTH_PRIVATE_KEY` to test the authentication using GitHub App Auth. In that case, the tests will additionally be executed using GitHub App auth. Note that the GitHub app should be installed -in the test repository organisation/user namespace, with access granted to the test repository -and in the user namespace that owns the GitHub token with access to the forked repository. +in the test repository organisation/user namespace, with access granted to the test repository. The command `tox -e test` can be used to run all tests, which are primarily integration tests. You can also select the repository against which to run the tests by setting diff --git a/tests/app/integration/conftest.py b/tests/app/integration/conftest.py index 70ae1458..2b619846 100644 --- a/tests/app/integration/conftest.py +++ b/tests/app/integration/conftest.py @@ -2,9 +2,11 @@ # See LICENSE file for licensing details. """Fixtures for integration tests.""" +import enum import logging import os from collections import namedtuple +from enum import Enum from typing import Iterator, cast import pytest @@ -36,18 +38,31 @@ TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME = f"AUTH_{GITHUB_APP_PRIVATE_KEY_ENV_NAME}" TEST_GITHUB_TOKEN_ENV_NAME = f"AUTH_{GITHUB_TOKEN_ENV_NAME}" -AuthenticationMethod = namedtuple( - "AuthenticationMethod", ["app_id", "installation_id", "private_key", "github_token"] + +class AuthenticationMethod(Enum): + """The authentication method to use. + + Attributes: + GITHUB_TOKEN: Use GitHub token authentication. + GITHUB_APP: Use GitHub App authentication. + """ + + GITHUB_TOKEN = enum.auto() + GITHUB_APP = enum.auto() + + +_AuthenticationMethodParams = namedtuple( + "_AuthenticationMethodParams", ["app_id", "installation_id", "private_key", "github_token"] ) @pytest.fixture( scope="function", - name="setup_github_auth_method", + name="github_auth", autouse=True, params=[ pytest.param( - AuthenticationMethod( + _AuthenticationMethodParams( github_token=os.getenv(TEST_GITHUB_TOKEN_ENV_NAME), app_id=None, installation_id=None, @@ -56,7 +71,7 @@ id="Using GitHub Token authentication", ), pytest.param( - AuthenticationMethod( + _AuthenticationMethodParams( app_id=os.getenv(TEST_GITHUB_APP_ID_ENV_NAME), installation_id=os.getenv(TEST_GITHUB_APP_INSTALLATION_ID_ENV_NAME), private_key=os.getenv(TEST_GITHUB_APP_PRIVATE_KEY_ENV_NAME), @@ -76,22 +91,28 @@ ), ], ) -def fixture_setup_github_auth_method( +def fixture_github_auth( request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch -) -> None: +) -> AuthenticationMethod: """Setup the GitHub authentication method. We want to test with GitHub Token authentication and optionally GitHub App authentication, if the environment variables are set. This is achieved by monkeypatching the respective environment variables. + + Returns: + The authentication method to use. """ app_id = request.param.app_id app_install_id = request.param.installation_id app_private_key = request.param.private_key github_token = request.param.github_token + auth_method = AuthenticationMethod.GITHUB_TOKEN + if app_id: monkeypatch.setenv(GITHUB_APP_ID_ENV_NAME, app_id) + auth_method = AuthenticationMethod.GITHUB_APP if app_install_id: monkeypatch.setenv(GITHUB_APP_INSTALLATION_ID_ENV_NAME, app_install_id) if app_private_key: @@ -99,6 +120,8 @@ def fixture_setup_github_auth_method( if github_token: monkeypatch.setenv(GITHUB_TOKEN_ENV_NAME, github_token) + return auth_method + @pytest.fixture(scope="session", name="github_repository_name") def fixture_github_repository_name(pytestconfig: pytest.Config) -> str: diff --git a/tests/app/integration/test_pull_request.py b/tests/app/integration/test_pull_request.py index 031d0006..8d7fc42c 100644 --- a/tests/app/integration/test_pull_request.py +++ b/tests/app/integration/test_pull_request.py @@ -2,7 +2,6 @@ # See LICENSE file for licensing details. """Tests for the pull_request function.""" - from uuid import uuid4 import pytest @@ -12,6 +11,7 @@ from repo_policy_compliance import PullRequestInput, UsedPolicy, policy, pull_request from repo_policy_compliance.check import Result +from .conftest import AuthenticationMethod from .types_ import BranchWithProtection, RequestedCollaborator @@ -234,6 +234,7 @@ def test_execute_job( # pylint: disable=too-many-arguments forked_github_repository: Repository, policy_enabled: bool, expected_result: Result, + github_auth: AuthenticationMethod, ): """ arrange: given a target and repository that is compliant and a source branch that is a fork and @@ -241,6 +242,13 @@ def test_execute_job( # pylint: disable=too-many-arguments act: when pull_request is called with the policy assert: then the expected report is returned. """ + # this test requires the github auth method to have access to the personal fork + # which would require a separate installation id for the app auth to be passed to the test, + # which is currently not supported (few tests which requires it so the overhead + # of adding it is not worth it) + if github_auth.GITHUB_APP: + pytest.skip("This test requires a personal fork to be accessible by the Github App Auth.") + policy_document = { policy.JobType.PULL_REQUEST: { policy.PullRequestProperty.EXECUTE_JOB: {policy.ENABLED_KEY: policy_enabled}, diff --git a/tests/app/integration/test_target_branch_protection.py b/tests/app/integration/test_target_branch_protection.py index 8f03c29e..f9637fb6 100644 --- a/tests/app/integration/test_target_branch_protection.py +++ b/tests/app/integration/test_target_branch_protection.py @@ -13,6 +13,7 @@ from repo_policy_compliance.check import Result, target_branch_protection from tests import assert_ +from .conftest import AuthenticationMethod from .types_ import BranchWithProtection @@ -118,13 +119,21 @@ def test_pass( def test_fail_default_branch( - forked_github_repository: Repository, caplog: pytest.LogCaptureFixture + forked_github_repository: Repository, + caplog: pytest.LogCaptureFixture, + github_auth: AuthenticationMethod, ): """ arrange: given a default branch branch that is not compliant. act: when target_branch_protection is called with the name of the branch. assert: then a fail report is returned. """ + # this test requires the github auth method to have access to the personal fork + # which would require a separate installation id for the app auth to be passed to the test, + # which is currently not supported (few tests which requires it so the overhead + # of adding it is not worth it) + if github_auth.GITHUB_APP: + pytest.skip("This test requires a personal fork to be accessible by the Github App Auth.") default_branch = forked_github_repository.get_branch(forked_github_repository.default_branch) default_branch.edit_protection() default_branch.remove_required_pull_request_reviews() From c77a57269abda73c7c54265102da4e8c212603f7 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 15:41:40 +0200 Subject: [PATCH 10/22] fix if condition --- tests/app/integration/test_pull_request.py | 2 +- tests/app/integration/test_target_branch_protection.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/integration/test_pull_request.py b/tests/app/integration/test_pull_request.py index 8d7fc42c..c803eb69 100644 --- a/tests/app/integration/test_pull_request.py +++ b/tests/app/integration/test_pull_request.py @@ -246,7 +246,7 @@ def test_execute_job( # pylint: disable=too-many-arguments # which would require a separate installation id for the app auth to be passed to the test, # which is currently not supported (few tests which requires it so the overhead # of adding it is not worth it) - if github_auth.GITHUB_APP: + if github_auth == AuthenticationMethod.GITHUB_APP: pytest.skip("This test requires a personal fork to be accessible by the Github App Auth.") policy_document = { diff --git a/tests/app/integration/test_target_branch_protection.py b/tests/app/integration/test_target_branch_protection.py index f9637fb6..6877a2a1 100644 --- a/tests/app/integration/test_target_branch_protection.py +++ b/tests/app/integration/test_target_branch_protection.py @@ -132,7 +132,7 @@ def test_fail_default_branch( # which would require a separate installation id for the app auth to be passed to the test, # which is currently not supported (few tests which requires it so the overhead # of adding it is not worth it) - if github_auth.GITHUB_APP: + if github_auth == AuthenticationMethod.GITHUB_APP: pytest.skip("This test requires a personal fork to be accessible by the Github App Auth.") default_branch = forked_github_repository.get_branch(forked_github_repository.default_branch) default_branch.edit_protection() From f5f619a589752ba455244ba4002e05b258926f0d Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 31 Jul 2024 15:50:23 +0200 Subject: [PATCH 11/22] bump minor version --- pyproject.toml | 2 +- rockcraft.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 78b21a12..130deca5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,7 +3,7 @@ [tool.poetry] name = "repo-policy-compliance" -version = "1.9.2" +version = "1.10.0" description = "Checks GitHub repository settings for compliance with policy" authors = ["Canonical IS DevOps "] license = "Apache 2.0" diff --git a/rockcraft.yaml b/rockcraft.yaml index d7790164..a5fa6d34 100644 --- a/rockcraft.yaml +++ b/rockcraft.yaml @@ -3,7 +3,7 @@ name: repo-policy-compliance base: ubuntu@22.04 -version: '1.9.2' +version: '1.10.0' summary: Check the repository setup for policy compliance description: | Used to check whether a GitHub repository complies with expected policies. From 2148cddf2a54d4fa94ca9d4d40df94588964045e Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 10:06:24 +0200 Subject: [PATCH 12/22] update docs --- charm/docs/reference/github-auth.md | 30 +++++++++++++++++++++++ charm/docs/reference/token-permissions.md | 20 --------------- 2 files changed, 30 insertions(+), 20 deletions(-) create mode 100644 charm/docs/reference/github-auth.md delete mode 100644 charm/docs/reference/token-permissions.md diff --git a/charm/docs/reference/github-auth.md b/charm/docs/reference/github-auth.md new file mode 100644 index 00000000..8f4f325c --- /dev/null +++ b/charm/docs/reference/github-auth.md @@ -0,0 +1,30 @@ +# GitHub Authentication + +This section describes the GitHub authentication options available for the charm. + +You can either choose to use + +- classic personal access tokens (PATs) +- fine-grained access tokens +- a GitHub app + +for authentication. The latter two options are recommended for better security and access control. +They require the fine-grained permissions as mentioned below. + + +## Personal access token scopes + +If you want to use classic PATS, you will need to select the `repo` scope. + +## Fine grained permissions + +For fine-grained access control, the following repository permissions are required: + +- Administration: read +- Contents: read (if you want to check private repositories) +- Pull requests: read (if you want to check private repositories) + + +**Note**: If you are using a fine-grained personal access token rather than a GitHub app, +the user who owns the token must have administrative access to the organisation or repository, +in addition to having a token with the necessary permissions. \ No newline at end of file diff --git a/charm/docs/reference/token-permissions.md b/charm/docs/reference/token-permissions.md deleted file mode 100644 index 239d6eb4..00000000 --- a/charm/docs/reference/token-permissions.md +++ /dev/null @@ -1,20 +0,0 @@ -# GitHub Token Permissions - -You can either choose to use a personal access token (PAT) or a fine-grained access token for the -`github_token` configuration. The token permissions/scopes are different for each type of token. - - -## Fine grained access token permissions - -**Note**: In addition to having a token with the necessary permissions, the user who owns the -token also must have admin access to the organisation or repository. - -For fine-grained access control, the following repository permissions are required: - -- Administration: read -- Contents: read (if you want to check private repositories) -- Pull requests: read (if you want to check private repositories) - -## Personal access token scopes - -If you want to use classic PATS, you will need to select the `repo` scope. \ No newline at end of file From a61108c63c0ab5836eb8de76fdd38f69aa7e482a Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 11:16:15 +0200 Subject: [PATCH 13/22] Kick off CI build From 5a9ce5104664df5760a6f42e223157b7ab744943 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 11:28:29 +0200 Subject: [PATCH 14/22] try out github app on cbartz-org/cbartz-repo-policy-compliance-tests --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 1c5e9284..719e9845 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,5 +26,5 @@ def pytest_addoption(parser: Parser) -> None: REPOSITORY_ARGUMENT_NAME, action="store", help="Name of the GitHub repository you want to run tests against.", - default="canonical/repo-policy-compliance-tests", + default="cbartz-org/cbartz-repo-policy-compliance-tests", # TODO: revert me ) From 2792feecf1bc7144a20eeadecc7f5857b472cbf7 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 12:05:28 +0200 Subject: [PATCH 15/22] update README on test repository requirements --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 88891d07..ce302fd6 100644 --- a/README.md +++ b/README.md @@ -57,9 +57,10 @@ The command `tox -e test` can be used to run all tests, which are primarily inte You can also select the repository against which to run the tests by setting the `--repository` flag. The tests will fork the repository and create PRs against it. Note that the tests are currently designed to work for specific Canonical repositories, -and may need to be for other repositories +and may need to be adapted for other repositories (e.g. `tests.app.integration.test_target_branch_protection.test_fail` -assumes that certain collaborators are in the `users_bypass_pull_request_allowances` list). +assumes that certain collaborators are in the `users_bypass_pull_request_allowances` list). +The test repository must also have a branch protection defined for the main branch. Also note that the forks are created in the personal space of the user whose token is being used, and that the forks are not deleted after the run. The reason for this is that it is only possible to create one fork of a repository, From 20dc596387dfed6c0e0cf3aedac746f26c793299 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 12:05:41 +0200 Subject: [PATCH 16/22] skip non-applicable tests --- tests/app/integration/test_blueprint.py | 12 +++++++++++- tests/app/integration/test_github_client.py | 4 ++++ tests/app/integration/test_pull_request.py | 12 ++++++++++-- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/app/integration/test_blueprint.py b/tests/app/integration/test_blueprint.py index 19bf7371..9771aceb 100644 --- a/tests/app/integration/test_blueprint.py +++ b/tests/app/integration/test_blueprint.py @@ -19,6 +19,7 @@ from repo_policy_compliance import blueprint, github_client, policy from tests import assert_ +from .conftest import AuthenticationMethod from .types_ import RequestedCollaborator EXPECTED_PULL_REQUEST_KEYS = ( @@ -786,13 +787,21 @@ def test_always_fail(client: FlaskClient, runner_token: str): @pytest.mark.parametrize( "invalid_token", [pytest.param("", id="empty"), pytest.param("invalid", id="invalid")] ) -def test_health_fail(client: FlaskClient, invalid_token: str, monkeypatch: pytest.MonkeyPatch): +def test_health_fail( + client: FlaskClient, + invalid_token: str, + monkeypatch: pytest.MonkeyPatch, + github_auth: AuthenticationMethod, +): """ arrange: given flask application with the blueprint registered and invalid token set in GITHUB_TOKEN environment variable act: when the health check endpoint is requested assert: then 500 is returned. """ + if github_auth == AuthenticationMethod.GITHUB_APP: + pytest.skip("This test is not applicable for GitHub App authentication.") + monkeypatch.setenv(github_client.GITHUB_TOKEN_ENV_NAME, invalid_token) response = client.get(blueprint.HEALTH_ENDPOINT) @@ -864,6 +873,7 @@ def test_internal_server_error( assert: 500 error is returned with reason. """ monkeypatch.setenv(github_client.GITHUB_TOKEN_ENV_NAME, "") + monkeypatch.setenv(github_client.GITHUB_APP_PRIVATE_KEY_ENV_NAME, "") main_branch = github_repository.get_branch(github_repository.default_branch) request_data = { "repository_name": github_repository.full_name, diff --git a/tests/app/integration/test_github_client.py b/tests/app/integration/test_github_client.py index 114c9ffc..93ecafaf 100644 --- a/tests/app/integration/test_github_client.py +++ b/tests/app/integration/test_github_client.py @@ -7,6 +7,7 @@ from repo_policy_compliance.check import Result, target_branch_protection from repo_policy_compliance.github_client import GITHUB_TOKEN_ENV_NAME +from tests.app.integration.conftest import AuthenticationMethod @pytest.mark.parametrize( @@ -34,12 +35,15 @@ def test_github_token( fail_reason: str, github_repository_name: str, monkeypatch: pytest.MonkeyPatch, + github_auth: AuthenticationMethod, ): """ arrange: A github repository name and a missing or invalid github token. act: when the github client is injected to target_branch_protection. assert: An expected error is raised with a specific error message. """ + if github_auth == AuthenticationMethod.GITHUB_APP: + pytest.skip("This test is not applicable for GitHub App authentication.") monkeypatch.setenv(GITHUB_TOKEN_ENV_NAME, str(github_token_value)) # The github_client is injected report = target_branch_protection( # pylint: disable=no-value-for-parameter diff --git a/tests/app/integration/test_pull_request.py b/tests/app/integration/test_pull_request.py index c803eb69..18b7a1f4 100644 --- a/tests/app/integration/test_pull_request.py +++ b/tests/app/integration/test_pull_request.py @@ -57,19 +57,27 @@ def test_invalid_policy(): ], indirect=["github_branch", "protected_github_branch"], ) -@pytest.mark.usefixtures("protected_github_branch") -def test_pull_request_disallow_fork( +@pytest.mark.usefixtures("protected_github_branch") # All the arguments are required for the test +def test_pull_request_disallow_fork( # pylint: disable=too-many-arguments github_branch: Branch, github_repository_name: str, forked_github_repository: Repository, policy_enabled: bool, expected_result: Result, + github_auth: AuthenticationMethod, ): """ arrange: given a forked repository and a disable_fork policy enabled/disabled. act: when pull_request is called assert: then a expected result is returned. """ + # this test requires the github auth method to have access to the personal fork + # which would require a separate installation id for the app auth to be passed to the test, + # which is currently not supported (few tests which requires it so the overhead + # of adding it is not worth it) + if github_auth == AuthenticationMethod.GITHUB_APP: + pytest.skip("This test requires a personal fork to be accessible by the Github App Auth.") + policy_document = { policy.JobType.PULL_REQUEST: { policy.PullRequestProperty.DISALLOW_FORK: {policy.ENABLED_KEY: policy_enabled}, From 3b547977b2bed58c1a3d5fe7d01fed9dc41a8df0 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 12:16:35 +0200 Subject: [PATCH 17/22] Revert "try out github app on cbartz-org/cbartz-repo-policy-compliance-tests" This reverts commit 5a9ce5104664df5760a6f42e223157b7ab744943. --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 719e9845..1c5e9284 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,5 +26,5 @@ def pytest_addoption(parser: Parser) -> None: REPOSITORY_ARGUMENT_NAME, action="store", help="Name of the GitHub repository you want to run tests against.", - default="cbartz-org/cbartz-repo-policy-compliance-tests", # TODO: revert me + default="canonical/repo-policy-compliance-tests", ) From 5938fd3220d0986d3be5b17b75d978d066bf1457 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 12:37:55 +0200 Subject: [PATCH 18/22] cleanup --- README.md | 4 ++-- repo_policy_compliance/github_client.py | 10 +++++----- src-docs/github_client.py.md | 2 +- tests/app/integration/conftest.py | 2 +- tests/app/integration/test_pull_request.py | 2 +- tests/app/unit/test_github_client.py | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ce302fd6..e0d125ff 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ To run the application tests, the `AUTH_GITHUB_TOKEN` environment variable must should be a token of a user with full repo permissions for the test repository. You can also pass in `AUTH_APP_ID`, `AUTH_INSTALLATION_ID`, and `AUTH_PRIVATE_KEY` to test the authentication using GitHub App Auth. In that case, the tests will additionally -be executed using GitHub App auth. Note that the GitHub app should be installed +be executed using GitHub app auth. Note that the GitHub app should be installed in the test repository organisation/user namespace, with access granted to the test repository. The command `tox -e test` can be used to run all tests, which are primarily integration tests. @@ -73,7 +73,7 @@ GitHub actions should have access to the GitHub token via a secret called `PERSONAL_GITHUB_TOKEN`. It is recommended to use either a fine-grained PAT or a token that is short-lived, e.g. 7 days. When it expires, a new token must be set. For the GitHub App Auth, the `TEST_GITHUB_APP_ID`, `TEST_GIHUB_APP_INSTALLATION_ID`, and -`TEST_GITHUB_APPP_RIVATE_KEY` should be set as secrets. +`TEST_GITHUB_APP_PRIVATE_KEY` should be set as secrets. ### Charm tests diff --git a/repo_policy_compliance/github_client.py b/repo_policy_compliance/github_client.py index 8634127b..7ff58470 100644 --- a/repo_policy_compliance/github_client.py +++ b/repo_policy_compliance/github_client.py @@ -34,8 +34,8 @@ MISSING_GITHUB_CONFIG_ERR_MSG = ( f"Either the {GITHUB_TOKEN_ENV_NAME} or not all of {GITHUB_APP_ID_ENV_NAME}," f" {GITHUB_APP_INSTALLATION_ID_ENV_NAME}, {GITHUB_APP_PRIVATE_KEY_ENV_NAME} " - f"environment variables were not provided or empty, " - "it is needed for interactions with GitHub, " + f"environment variables were provided or are empty, " + "the variables are needed for interactions with GitHub, " ) NOT_ALL_GITHUB_APP_CONFIG_ERR_MSG = ( f"Not all of {GITHUB_APP_ID_ENV_NAME}, {GITHUB_APP_INSTALLATION_ID_ENV_NAME}," @@ -51,7 +51,7 @@ def get() -> Github: """Get a GitHub client. Returns: - A GitHub client that is configured with a token from the environment. + A GitHub client that is configured with a token or GitHub app from the environment. Raises: ConfigurationError: If the GitHub auth config is not valid. @@ -118,7 +118,7 @@ def _ensure_either_github_token_or_app_config( github_app_installation_id_str: str | None, github_app_private_key: str | None, ) -> None: - """Ensure that only one of github_token or github app config is provided and is valid. + """Ensure that only one of github_token or GitHub App config is provided and is valid. Args: github_token: The GitHub token. @@ -127,7 +127,7 @@ def _ensure_either_github_token_or_app_config( github_app_private_key: The GitHub App private key. Raises: - ConfigurationError: If both github_token and github app config are provided. + ConfigurationError: If the configuration is not valid. """ if not github_token and not ( github_app_id or github_app_installation_id_str or github_app_private_key diff --git a/src-docs/github_client.py.md b/src-docs/github_client.py.md index 38066eb5..3b085d11 100644 --- a/src-docs/github_client.py.md +++ b/src-docs/github_client.py.md @@ -30,7 +30,7 @@ Get a GitHub client. **Returns:** - A GitHub client that is configured with a token from the environment. + A GitHub client that is configured with a token or GitHub app from the environment. diff --git a/tests/app/integration/conftest.py b/tests/app/integration/conftest.py index 2b619846..410d0c7a 100644 --- a/tests/app/integration/conftest.py +++ b/tests/app/integration/conftest.py @@ -135,7 +135,7 @@ def fixture_github_token() -> str: github_token = os.getenv(TEST_GITHUB_TOKEN_ENV_NAME) assert ( github_token - ), f"GitHub must be set in the environment variable {TEST_GITHUB_TOKEN_ENV_NAME}" + ), f"GitHub token must be set in the environment variable {TEST_GITHUB_TOKEN_ENV_NAME}" return github_token diff --git a/tests/app/integration/test_pull_request.py b/tests/app/integration/test_pull_request.py index 18b7a1f4..ba257a78 100644 --- a/tests/app/integration/test_pull_request.py +++ b/tests/app/integration/test_pull_request.py @@ -234,7 +234,7 @@ def test_collaborators( indirect=["github_branch", "protected_github_branch", "forked_github_branch"], ) @pytest.mark.usefixtures("protected_github_branch", "make_fork_from_non_collaborator") -# All the arguments are required for the test +# we use a lot of arguments, but it seems not worth to introduce a capsulating object for this def test_execute_job( # pylint: disable=too-many-arguments github_branch: Branch, github_repository_name: str, diff --git a/tests/app/unit/test_github_client.py b/tests/app/unit/test_github_client.py index 9f9cafa5..0b2ff57e 100644 --- a/tests/app/unit/test_github_client.py +++ b/tests/app/unit/test_github_client.py @@ -114,7 +114,7 @@ def test_get_collaborator_permission_error(): id="not all github app config provided", ), ], -) # we use a lot of arguments but it seems not worth to introduce a capsulating object for this +) # we use a lot of arguments, but it seems not worth to introduce a capsulating object for this def test_get_client_configuration_error( # pylint: disable=too-many-arguments github_app_id: str, github_app_installation_id: str, From d1d67427c92fa2318078966233412e23e20902e2 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 16:16:53 +0200 Subject: [PATCH 19/22] update docs --- charm/docs/reference/github-auth.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/charm/docs/reference/github-auth.md b/charm/docs/reference/github-auth.md index 8f4f325c..5df8c0e4 100644 --- a/charm/docs/reference/github-auth.md +++ b/charm/docs/reference/github-auth.md @@ -4,17 +4,22 @@ This section describes the GitHub authentication options available for the charm You can either choose to use -- classic personal access tokens (PATs) -- fine-grained access tokens +- classic personal access tokens +- fine-grained personal access tokens - a GitHub app for authentication. The latter two options are recommended for better security and access control. They require the fine-grained permissions as mentioned below. +**Note**: If you are using a personal access tokens rather than a GitHub app, +the user who owns the token must have administrative access to the organisation or repository, +in addition to having a token with the necessary permissions. + -## Personal access token scopes +## Classic personal access token scopes -If you want to use classic PATS, you will need to select the `repo` scope. +If you want to use classic personal access tokens, you will need to select the `repo` +scope when generating them. ## Fine grained permissions @@ -22,9 +27,4 @@ For fine-grained access control, the following repository permissions are requir - Administration: read - Contents: read (if you want to check private repositories) -- Pull requests: read (if you want to check private repositories) - - -**Note**: If you are using a fine-grained personal access token rather than a GitHub app, -the user who owns the token must have administrative access to the organisation or repository, -in addition to having a token with the necessary permissions. \ No newline at end of file +- Pull requests: read (if you want to check private repositories) \ No newline at end of file From a058f19ee0c9b528c90ccadf56506d57de576740 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 2 Aug 2024 11:49:03 +0200 Subject: [PATCH 20/22] use AuthMode enum and remove asserts --- repo_policy_compliance/github_client.py | 45 +++++++++++++++++-------- src-docs/github_client.py.md | 10 +++--- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/repo_policy_compliance/github_client.py b/repo_policy_compliance/github_client.py index 7ff58470..a4fdace3 100644 --- a/repo_policy_compliance/github_client.py +++ b/repo_policy_compliance/github_client.py @@ -2,10 +2,11 @@ # See LICENSE file for licensing details. """Module for GitHub client.""" - +import enum import functools import logging import os +from enum import Enum from typing import Callable, Concatenate, Literal, ParamSpec, TypeVar, cast from urllib import parse @@ -47,6 +48,18 @@ ) +class _AuthMode(Enum): + """Enum to represent the auth mode to use. + + Attributes: + TOKEN: Using GitHub token auth. + APP: Using GitHub App auth. + """ + + TOKEN = enum.auto() + APP = enum.auto() + + def get() -> Github: """Get a GitHub client. @@ -87,7 +100,7 @@ def _get_auth() -> Auth: f"FLASK_{GITHUB_APP_PRIVATE_KEY_ENV_NAME}" ) - _ensure_either_github_token_or_app_config( + auth_mode = _get_auth_mode( github_token=github_token, github_app_id=github_app_id, github_app_installation_id_str=github_app_installation_id_str, @@ -95,15 +108,11 @@ def _get_auth() -> Auth: ) auth: Auth - # we use asserts here to make mypy happy, we have already checked that the values are not None - # we know asserts are optimised away in production, so ignore bandit warnings - if github_app_id: - assert github_app_installation_id_str is not None # nosec - assert github_app_private_key is not None # nosec + if auth_mode == _AuthMode.APP: auth = _get_github_app_installation_auth( - github_app_id=github_app_id, - github_app_installation_id_str=github_app_installation_id_str, - github_app_private_key=github_app_private_key, + github_app_id=cast(str, github_app_id), + github_app_installation_id_str=cast(str, github_app_installation_id_str), + github_app_private_key=cast(str, github_app_private_key), ) else: assert github_token is not None # nosec @@ -112,13 +121,13 @@ def _get_auth() -> Auth: return auth -def _ensure_either_github_token_or_app_config( +def _get_auth_mode( github_token: str | None, github_app_id: str | None, github_app_installation_id_str: str | None, github_app_private_key: str | None, -) -> None: - """Ensure that only one of github_token or GitHub App config is provided and is valid. +) -> _AuthMode: + """Get the auth mode to use. Args: github_token: The GitHub token. @@ -127,7 +136,11 @@ def _ensure_either_github_token_or_app_config( github_app_private_key: The GitHub App private key. Raises: - ConfigurationError: If the configuration is not valid. + ConfigurationError: If the configuration is not valid, e.g. if both a token and app config + are provided. + + Returns: + The auth mode to use. """ if not github_token and not ( github_app_id or github_app_installation_id_str or github_app_private_key @@ -154,6 +167,10 @@ def _ensure_either_github_token_or_app_config( f"{github_app_private_key!r}" ) + if github_token: + return _AuthMode.TOKEN + return _AuthMode.APP + def _get_github_app_installation_auth( github_app_id: str, github_app_installation_id_str: str, github_app_private_key: str diff --git a/src-docs/github_client.py.md b/src-docs/github_client.py.md index 3b085d11..1ed8fff3 100644 --- a/src-docs/github_client.py.md +++ b/src-docs/github_client.py.md @@ -17,7 +17,7 @@ Module for GitHub client. --- - + ## function `get` @@ -41,7 +41,7 @@ Get a GitHub client. --- - + ## function `inject` @@ -65,7 +65,7 @@ Injects a GitHub client as the first argument to a function. --- - + ## function `get_collaborators` @@ -95,7 +95,7 @@ Get collaborators with a given affiliation and permission. --- - + ## function `get_branch` @@ -125,7 +125,7 @@ Get the branch for the check. --- - + ## function `get_collaborator_permission` From 6db8f175b09bc6c0117e91b2bd51bbeed649e92c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 1 Aug 2024 11:28:29 +0200 Subject: [PATCH 21/22] try out github app on cbartz-org/cbartz-repo-policy-compliance-tests --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 1c5e9284..719e9845 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,5 +26,5 @@ def pytest_addoption(parser: Parser) -> None: REPOSITORY_ARGUMENT_NAME, action="store", help="Name of the GitHub repository you want to run tests against.", - default="canonical/repo-policy-compliance-tests", + default="cbartz-org/cbartz-repo-policy-compliance-tests", # TODO: revert me ) From 90b735ad0c422e4c35e46c65be9f60c54e6d6342 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 2 Aug 2024 12:35:44 +0200 Subject: [PATCH 22/22] Revert "try out github app on cbartz-org/cbartz-repo-policy-compliance-tests" This reverts commit 6db8f175b09bc6c0117e91b2bd51bbeed649e92c. --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 719e9845..1c5e9284 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,5 +26,5 @@ def pytest_addoption(parser: Parser) -> None: REPOSITORY_ARGUMENT_NAME, action="store", help="Name of the GitHub repository you want to run tests against.", - default="cbartz-org/cbartz-repo-policy-compliance-tests", # TODO: revert me + default="canonical/repo-policy-compliance-tests", )