From acb009844ece8c160131f77e6bfd7ad19b639a4e Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 25 Oct 2023 20:00:27 +0100 Subject: [PATCH] Simplify the code by using GitHub CLI Simplify the code by using GitHub CLIand the GitHub REST API. Also add tests. Fixes https://github.com/hypothesis/dependabot-alerts/issues/5 --- .cookiecutter/cookiecutter.json | 2 +- .cookiecutter/includes/README/head.md | 6 +- .../includes/setuptools/install_requires | 1 - .github/workflows/alert.yml | 2 +- README.md | 6 +- setup.cfg | 1 - src/dependabot_alerts/cli.py | 68 +++--- src/dependabot_alerts/core.py | 220 ++++-------------- tests/conftest.py | 40 ++++ tests/functional/sanity_test.py | 2 - tests/unit/dependabot_alerts/cli_test.py | 103 ++++++++ tests/unit/dependabot_alerts/core_test.py | 106 +++++++++ 12 files changed, 342 insertions(+), 215 deletions(-) delete mode 100644 .cookiecutter/includes/setuptools/install_requires create mode 100644 tests/conftest.py delete mode 100644 tests/functional/sanity_test.py create mode 100644 tests/unit/dependabot_alerts/core_test.py diff --git a/.cookiecutter/cookiecutter.json b/.cookiecutter/cookiecutter.json index 1abc1b8..f0f3b1e 100644 --- a/.cookiecutter/cookiecutter.json +++ b/.cookiecutter/cookiecutter.json @@ -2,7 +2,7 @@ "template": "https://github.com/hypothesis/cookiecutters", "checkout": null, "directory": "pypackage", - "ignore": ["tests/unit/dependabot_alerts/core_test.py"], + "ignore": ["tests/functional/sanity_test.py"], "extra_context": { "name": "Dependabot Alerts", "package_name": "dependabot_alerts", diff --git a/.cookiecutter/includes/README/head.md b/.cookiecutter/includes/README/head.md index 7da0881..5e34360 100644 --- a/.cookiecutter/includes/README/head.md +++ b/.cookiecutter/includes/README/head.md @@ -1,10 +1,12 @@ `dependabot-alerts` lists Dependabot security alerts for all repos of a GitHub -user or organization. You can run it from the command line: +organization. You can run it from the command line: ```terminal -$ dependabot-alerts +$ dependabot-alerts ``` +You'll need to have [GitHub CLI](https://cli.github.com/) installed and logged in. + There's also a [GitHub Actions workflow](.github/workflows/alert.yml) that runs automatically on a schedule and notifies us in Slack of any Dependabot alerts in the `hypothesis` GitHub organization. diff --git a/.cookiecutter/includes/setuptools/install_requires b/.cookiecutter/includes/setuptools/install_requires deleted file mode 100644 index f229360..0000000 --- a/.cookiecutter/includes/setuptools/install_requires +++ /dev/null @@ -1 +0,0 @@ -requests diff --git a/.github/workflows/alert.yml b/.github/workflows/alert.yml index 59d1722..02f6dd5 100644 --- a/.github/workflows/alert.yml +++ b/.github/workflows/alert.yml @@ -29,7 +29,7 @@ jobs: run: | { echo 'SLACK_MESSAGE<> "$GITHUB_OUTPUT" env: diff --git a/README.md b/README.md index acf29d0..ebabf3c 100644 --- a/README.md +++ b/README.md @@ -9,12 +9,14 @@ Notifications of Dependabot alerts across a GitHub organization. `dependabot-alerts` lists Dependabot security alerts for all repos of a GitHub -user or organization. You can run it from the command line: +organization. You can run it from the command line: ```terminal -$ dependabot-alerts +$ dependabot-alerts ``` +You'll need to have [GitHub CLI](https://cli.github.com/) installed and logged in. + There's also a [GitHub Actions workflow](.github/workflows/alert.yml) that runs automatically on a schedule and notifies us in Slack of any Dependabot alerts in the `hypothesis` GitHub organization. diff --git a/setup.cfg b/setup.cfg index f1872cd..ec7c6c0 100644 --- a/setup.cfg +++ b/setup.cfg @@ -18,7 +18,6 @@ package_dir = packages = find: python_requires = >=3.11 install_requires = - requests [options.packages.find] where = src diff --git a/src/dependabot_alerts/cli.py b/src/dependabot_alerts/cli.py index f245b93..522b753 100644 --- a/src/dependabot_alerts/cli.py +++ b/src/dependabot_alerts/cli.py @@ -1,49 +1,45 @@ +import subprocess from argparse import ArgumentParser from importlib.metadata import version -from dependabot_alerts.core import GitHubClient, Vulnerability, fetch_alerts +from dependabot_alerts.core import GitHub -def cli(_argv=None): # pragma: no cover +def cli(argv=None): parser = ArgumentParser() parser.add_argument( "-v", "--version", action="version", version=version("dependabot-alerts") ) - parser.add_argument("organization", help="GitHub user or organization") - - args = parser.parse_args(_argv) - - gh_client = GitHubClient.init() - vulns = fetch_alerts(gh_client, args.organization) - print(format_slack_message(args.organization, vulns)) - - return 0 - - -def format_slack_message( - organization: str, vulns: list[Vulnerability] -) -> str: # pragma: no cover - """ - Format a Slack status report from a list of vulnerabilities. - - Returns a message using Slack's "mrkdwn" format. See - https://api.slack.com/reference/surfaces/formatting. - """ - if not vulns: - return "Found no open vulnerabilities." + parser.add_argument( + "--slack", action="store_true", help="print output in Slack's 'mrkdwn' format" + ) + parser.add_argument("organization", help="GitHub organization") - n_repos = len(set(vuln.repo for vuln in vulns)) + args = parser.parse_args(argv) + organization = args.organization - msg_parts = [] - msg_parts.append(f"*Found {len(vulns)} vulnerabilities in {n_repos} repositories.*") + github = GitHub(subprocess.run) + repos = github.alerts(organization) - for vuln in vulns: - vuln_msg = [] - vuln_msg.append( - f"{organization}/{vuln.repo}: <{vuln.url}|{vuln.package_name} {vuln.severity} - {vuln.title}>" + if args.slack and not repos: + print( + f"There are no Dependabot security alerts in the {organization} GitHub organization." ) - if vuln.pr: - vuln_msg.append(f" Resolved by {vuln.pr}") - msg_parts.append("\n".join(vuln_msg)) - - return "\n\n".join(msg_parts) + elif args.slack and repos: + print( + f"*Found Dependabot security alerts in {len(repos)} repos in the {organization} GitHub organization:*" + ) + print() + for repo, packages in repos.items(): + for package, alerts in packages.items(): + print( + f"- : {package} ({len(alerts)} alerts)" + ) + print() + print( + "Message generated by the `alerts.yml` workflow " + ) + elif repos: + for repo, packages in repos.items(): + for package, alerts in packages.items(): + print(f"{repo}: {package} ({len(alerts)} alerts)") diff --git a/src/dependabot_alerts/core.py b/src/dependabot_alerts/core.py index c28f5f7..1e344f8 100644 --- a/src/dependabot_alerts/core.py +++ b/src/dependabot_alerts/core.py @@ -1,184 +1,66 @@ import json -import os +from collections import defaultdict from dataclasses import dataclass -from getpass import getpass -from typing import Optional +from subprocess import CalledProcessError -import requests +def safe_get(dict_, keys): + keys = list(reversed(keys)) + value = dict_ -class GitHubClient: # pragma: no cover - """ - Client for GitHub's GraphQL API. + while keys: + key = keys.pop() + try: + value = value[key] + except Exception: # pylint:disable=broad-exception-caught + return None - See https://docs.github.com/en/graphql. - """ - - def __init__(self, token): - self.token = token - self.endpoint = "https://api.github.com/graphql" - - def query(self, query, variables=None): - data = {"query": query, "variables": variables if variables is not None else {}} - result = requests.post( - url=self.endpoint, - headers={"Authorization": f"Bearer {self.token}"}, - data=json.dumps(data), - timeout=(30, 30), - ) - body = result.json() - result.raise_for_status() - if "errors" in body: - errors = body["errors"] - raise RuntimeError(f"Query failed: {json.dumps(errors)}") - return body["data"] - - @classmethod - def init(cls): - """ - Initialize an authenticated GitHubClient. - - This will read from the `GITHUB_TOKEN` env var if set, or prompt for - a token otherwise. - """ - access_token = os.environ.get("GITHUB_TOKEN") - if not access_token: - access_token = getpass("GitHub API token: ") - return GitHubClient(access_token) + return value @dataclass -class Vulnerability: # pylint:disable=too-many-instance-attributes +class Alert: repo: str - """Repository where this vulnerability was reported.""" - - created_at: str - """ISO date when this alert was created.""" - - package_name: str - """Name of the vulnerable package.""" - ecosystem: str - """Package ecosytem (eg. npm) that the package comes from.""" - - severity: str - """Vulnerability severity level.""" - - version_range: str - """Version ranges of package affected by vulnerability.""" - - number: str - """Number of this vulnerability report.""" - - url: str - """Link to the vulernability report on GitHub.""" - - pr: Optional[str] - """Link to the Dependabot update PR that resolves this vulnerability.""" + package: str - title: str - """Summary of what the vulnerability is.""" - - -def fetch_alerts( - gh: GitHubClient, organization: str -) -> list[Vulnerability]: # pragma: no cover - """ - Fetch details of all open vulnerability alerts in `organization`. - - To reduce the volume of noise, especially for repositories which include the - same dependency in multiple lockfiles, only one vulnerability is reported - per package per repository. - - Vulnerabilities are not reported from archived repositories. - """ - # pylint:disable=too-many-locals - - query = """ -query($organization: String!, $cursor: String) { - organization(login: $organization) { - repositories(first: 100, after: $cursor) { - pageInfo { - endCursor - hasNextPage - } - nodes { - name - vulnerabilityAlerts(first: 100, states:OPEN) { - nodes { - number - createdAt - dependabotUpdate { - pullRequest { - url - } - } - securityAdvisory { - summary - } - securityVulnerability { - package { - name - ecosystem - } - severity - vulnerableVersionRange - } - } - } - } - } - } -} -""" - - vulns = [] - cursor = None - has_next_page = True - - while has_next_page: - result = gh.query( - query=query, variables={"organization": organization, "cursor": cursor} + @classmethod + def make(cls, alert_dict): + return cls( + repo=safe_get(alert_dict, ["repository", "full_name"]), + ecosystem=safe_get(alert_dict, ["dependency", "package", "ecosystem"]), + package=safe_get(alert_dict, ["dependency", "package", "name"]), ) - page_info = result["organization"]["repositories"]["pageInfo"] - cursor = page_info["endCursor"] - has_next_page = page_info["hasNextPage"] - - for repo in result["organization"]["repositories"]["nodes"]: - alerts = repo["vulnerabilityAlerts"]["nodes"] - - if alerts: - repo_name = repo["name"] - vulnerable_packages = set() - - for alert in alerts: - sa = alert["securityAdvisory"] - sv = alert["securityVulnerability"] - number = alert["number"] - package_name = sv["package"]["name"] - - if package_name in vulnerable_packages: - continue - vulnerable_packages.add(package_name) - - pr = None - - dep_update = alert["dependabotUpdate"] - if dep_update and dep_update["pullRequest"]: - pr = dep_update["pullRequest"]["url"] - vuln = Vulnerability( - repo=repo_name, - created_at=alert["createdAt"], - ecosystem=sv["package"]["ecosystem"], - number=number, - package_name=sv["package"]["name"], - pr=pr, - severity=sv["severity"], - title=sa["summary"], - url=f"https://github.com/{organization}/{repo_name}/security/dependabot/{number}", - version_range=sv["vulnerableVersionRange"], - ) - vulns.append(vuln) - return vulns +class GitHub: + def __init__(self, run): + self._run = run + + def alerts(self, organization): + try: + result = self._run( + [ + "gh", + "api", + "--paginate", + f"/orgs/{organization}/dependabot/alerts?state=open", + ], + check=True, + capture_output=True, + text=True, + ) + except CalledProcessError as err: # pragma: no cover + print(err.stdout) + print(err.stderr) + raise + + alert_dicts = json.loads(result.stdout) + + alerts = defaultdict(lambda: defaultdict(list)) + + for alert_dict in alert_dicts: + alert = Alert.make(alert_dict) + alerts[alert.repo][(alert.ecosystem, alert.package)].append(alert) + + return alerts diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..6116173 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,40 @@ +import factory +from pytest_factoryboy import named_model, register + +from dependabot_alerts.core import Alert + + +@register +class AlertFactory(factory.Factory): + """Factory for Alert objects.""" + + class Meta: + model = Alert + exclude = ("organization",) + + organization = factory.Sequence(lambda n: f"organization-{n}") + repo = factory.LazyAttributeSequence(lambda o, n: f"{o.organization}/repo-{n}") + ecosystem = factory.Sequence(lambda n: f"ecosystem-{n}") + package = factory.Sequence(lambda n: f"package-{n}") + + +@register +class AlertDictFactory(AlertFactory): + """Factory for alert dicts as returned by the GitHub API.""" + + class Meta: + model = named_model(dict, "AlertDict") + + @factory.post_generation + def post(obj, *_args, **_kwargs): # pylint:disable=no-self-argument + repo = obj.pop("repo") # pylint:disable=no-member + ecosystem = obj.pop("ecosystem") # pylint:disable=no-member + package = obj.pop("package") # pylint:disable=no-member + + obj["repository"] = {"full_name": repo} + obj["dependency"] = { + "package": { + "ecosystem": ecosystem, + "name": package, + } + } diff --git a/tests/functional/sanity_test.py b/tests/functional/sanity_test.py deleted file mode 100644 index b76ddae..0000000 --- a/tests/functional/sanity_test.py +++ /dev/null @@ -1,2 +0,0 @@ -def test_it(): - assert True diff --git a/tests/unit/dependabot_alerts/cli_test.py b/tests/unit/dependabot_alerts/cli_test.py index e7816ae..3a038bf 100644 --- a/tests/unit/dependabot_alerts/cli_test.py +++ b/tests/unit/dependabot_alerts/cli_test.py @@ -3,6 +3,68 @@ import pytest from dependabot_alerts.cli import cli +from dependabot_alerts.core import Alert + + +def test_it(GitHub, github, subprocess, capsys): + cli(["test-organization"]) + + GitHub.assert_called_once_with(subprocess.run) + github.alerts.assert_called_once_with("test-organization") + captured = capsys.readouterr() + assert captured.out == "\n".join( + [ + "test-organization/test-repo-1: ('test-ecosystem', 'test-package-1') (2 alerts)", + "test-organization/test-repo-2: ('test-ecosystem', 'test-package-2') (1 alerts)", + "", + ] + ) + assert not captured.err + + +@pytest.mark.parametrize( + "repos,expected", + [ + ( + True, + "*Found Dependabot security alerts in 2 repos in the test-organization GitHub organization:", + ), + ( + False, + "There are no Dependabot security alerts in the test-organization GitHub organization.", + ), + ], +) +def test_it_can_format_output_for_Slack(capsys, github, repos, expected): + if not repos: + github.alerts.return_value = {} + + cli(["--slack", "test-organization"]) + + captured = capsys.readouterr() + assert captured.out.startswith(expected) + assert not captured.err + + +@pytest.mark.parametrize("slack", [True, False]) +def test_it_prints_nothing_if_there_are_no_alerts(github, capsys, slack): + github.alerts.return_value = {} + args = ["test-organization"] + if slack: + args.append("--slack") + + cli(["test-organization"]) + + captured = capsys.readouterr() + assert not captured.out + assert not captured.err + + +def test_it_crashes_if_no_organization_argument_is_given(): + with pytest.raises(SystemExit) as exc_info: + cli([]) + + assert exc_info.value.code == 2 def test_help(): @@ -18,3 +80,44 @@ def test_version(capsys): assert capsys.readouterr().out.strip() == version("dependabot-alerts") assert not exc_info.value.code + + +@pytest.fixture(autouse=True) +def GitHub(mocker): + return mocker.patch("dependabot_alerts.cli.GitHub") + + +@pytest.fixture(autouse=True) +def github(GitHub): + github = GitHub.return_value + github.alerts.return_value = { + "test-organization/test-repo-1": { + ("test-ecosystem", "test-package-1"): [ + Alert( + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-1", + ), + Alert( + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-1", + ), + ], + }, + "test-organization/test-repo-2": { + ("test-ecosystem", "test-package-2"): [ + Alert( + repo="test-organization/test-repo-2", + ecosystem="test-ecosystem", + package="test-package-2", + ), + ] + }, + } + return github + + +@pytest.fixture(autouse=True) +def subprocess(mocker): + return mocker.patch("dependabot_alerts.cli.subprocess") diff --git a/tests/unit/dependabot_alerts/core_test.py b/tests/unit/dependabot_alerts/core_test.py new file mode 100644 index 0000000..caed048 --- /dev/null +++ b/tests/unit/dependabot_alerts/core_test.py @@ -0,0 +1,106 @@ +import json +import subprocess +from unittest.mock import Mock, create_autospec + +import pytest + +from dependabot_alerts.core import Alert, GitHub, safe_get + + +@pytest.mark.parametrize( + "dict_,keys,result", + [ + ({}, ["foo", "bar"], None), + ({"foo": {}}, ["foo", "bar"], None), + ({"foo": {"bar": "gar"}}, ["foo", "bar"], "gar"), + ], +) +def test_safe_get(dict_, keys, result): + assert safe_get(dict_, keys) == result + + +class TestGitHub: + def test_alerts(self, github, run, alert_dict_factory): + run.return_value = Mock( + stdout=json.dumps( + [ + *alert_dict_factory.create_batch( + size=2, + organization="test-organization", + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-1", + ), + alert_dict_factory( + organization="test-organization", + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-2", + ), + alert_dict_factory( + organization="test-organization", + repo="test-organization/test-repo-2", + ecosystem="test-ecosystem", + package="test-package-1", + ), + ] + ) + ) + + repos = github.alerts("test-organization") + + run.assert_called_once_with( + [ + "gh", + "api", + "--paginate", + "/orgs/test-organization/dependabot/alerts?state=open", + ], + check=True, + capture_output=True, + text=True, + ) + assert repos == { + "test-organization/test-repo-1": { + ("test-ecosystem", "test-package-1"): [ + Alert( + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-1", + ), + Alert( + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-1", + ), + ], + ("test-ecosystem", "test-package-2"): [ + Alert( + repo="test-organization/test-repo-1", + ecosystem="test-ecosystem", + package="test-package-2", + ), + ], + }, + "test-organization/test-repo-2": { + ("test-ecosystem", "test-package-1"): [ + Alert( + repo="test-organization/test-repo-2", + ecosystem="test-ecosystem", + package="test-package-1", + ), + ] + }, + } + + def test_alerts_returns_an_empty_dict_if_there_are_no_alerts(self, github, run): + run.return_value = Mock(stdout=json.dumps([])) + assert github.alerts("test-organization") == {} + + @pytest.fixture + def run(self): + return create_autospec(subprocess.run) + + @pytest.fixture + def github(self, run): + return GitHub(run)