diff --git a/README.rst b/README.rst index ccc8cfb..d16ba22 100644 --- a/README.rst +++ b/README.rst @@ -407,11 +407,9 @@ If the incremental argument is used, per-resource checkpoints are only advanced It's therefore recommended to only use the incremental argument if the output/result is being actively monitored, or complimented with periodic full non-incremental runs, to avoid unexpected missing data in a regular backup runs. -**Starred public repo hooks blocking** +**Hooks need admin access** -Since the ``--all`` argument includes ``--hooks``, if you use ``--all`` and ``--all-starred`` together to clone a users starred public repositories, the backup will likely error and block the backup continuing. - -This is due to needing the correct permission for ``--hooks`` on public repos. +Reading a repository's hooks requires admin access to it, and fine-grained tokens also need the Webhooks read permission. Since the ``--all`` argument includes ``--hooks``, repositories you don't administer, such as starred repositories with ``--all-starred``, are skipped for hooks and the rest of the backup continues. "bare" is actually "mirror" diff --git a/github_backup/github_backup.py b/github_backup/github_backup.py index a8297c9..9fbfacf 100644 --- a/github_backup/github_backup.py +++ b/github_backup/github_backup.py @@ -3155,6 +3155,15 @@ def backup_hooks(args, repo_cwd, repository, repos_template): if not auth: logger.info("Skipping hooks since no authentication provided") return + # Reading hooks needs admin access, and GitHub returns 404 without it. + # Skip the request when the repository listing says we aren't an admin. + if repository.get("permissions", {}).get("admin") is False: + logger.info( + "Skipping hooks for {0} since admin access is required".format( + repository["full_name"] + ) + ) + return hook_cwd = os.path.join(repo_cwd, "hooks") output_file = "{0}/hooks.json".format(hook_cwd) template = "{0}/{1}/hooks".format(repos_template, repository["full_name"]) @@ -3162,7 +3171,12 @@ def backup_hooks(args, repo_cwd, repository, repos_template): _backup_data(args, "hooks", template, output_file, hook_cwd) except HTTPError as e: if e.code == 404: - logger.info("Unable to read hooks, skipping") + logger.info( + "Unable to read hooks for {0}, skipping (requires admin access, " + "or Webhooks read permission for fine-grained tokens)".format( + repository["full_name"] + ) + ) else: raise e diff --git a/tests/test_hooks.py b/tests/test_hooks.py new file mode 100644 index 0000000..4869a7f --- /dev/null +++ b/tests/test_hooks.py @@ -0,0 +1,96 @@ +"""Tests for hooks backup permissions (GitHub issue #532). + +Reading ``/repos/{owner}/{repo}/hooks`` needs admin access to the repository. +Without it GitHub responds with HTTP 404 rather than 403, which used to log an +API error for every repository the user doesn't administer. +""" + +import logging +from urllib.error import HTTPError + +import pytest + +from github_backup import github_backup + +REPOS_TEMPLATE = "https://api.github.com/repos" + + +@pytest.fixture +def hook_args(create_args): + return create_args(include_hooks=True, token_classic="ghp_token") + + +def _repository(**fields): + return {"full_name": "owner/repo", **fields} + + +def test_hooks_skipped_without_request_when_not_admin( + hook_args, tmp_path, monkeypatch, caplog +): + def fail_retrieve_data(*args, **kwargs): + raise AssertionError("hooks should not be requested without admin access") + + monkeypatch.setattr(github_backup, "retrieve_data", fail_retrieve_data) + + with caplog.at_level(logging.INFO, logger=github_backup.logger.name): + github_backup.backup_hooks( + hook_args, + tmp_path, + _repository(permissions={"admin": False, "push": True, "pull": True}), + REPOS_TEMPLATE, + ) + + assert "Skipping hooks for owner/repo since admin access is required" in caplog.text + assert not (tmp_path / "hooks").exists() + + +@pytest.mark.parametrize( + "repository", + [ + _repository(permissions={"admin": True, "push": True, "pull": True}), + _repository(), + ], + ids=["admin", "no-permissions-field"], +) +def test_hooks_requested_when_admin_or_permissions_unknown( + hook_args, tmp_path, monkeypatch, repository +): + requested = [] + + def fake_retrieve_data(passed_args, template, *args, **kwargs): + requested.append(template) + return [{"id": 1, "name": "web"}] + + monkeypatch.setattr(github_backup, "retrieve_data", fake_retrieve_data) + + github_backup.backup_hooks(hook_args, tmp_path, repository, REPOS_TEMPLATE) + + assert requested == ["https://api.github.com/repos/owner/repo/hooks"] + assert (tmp_path / "hooks" / "hooks.json").exists() + + +def test_hooks_404_is_skipped_with_reason(hook_args, tmp_path, monkeypatch, caplog): + def fake_retrieve_data(passed_args, template, *args, **kwargs): + raise HTTPError(template, 404, "Not Found", hdrs=None, fp=None) + + monkeypatch.setattr(github_backup, "retrieve_data", fake_retrieve_data) + + with caplog.at_level(logging.INFO, logger=github_backup.logger.name): + github_backup.backup_hooks( + hook_args, + tmp_path, + _repository(permissions={"admin": True}), + REPOS_TEMPLATE, + ) + + assert "Unable to read hooks for owner/repo, skipping" in caplog.text + + +def test_hooks_non_404_error_propagates(hook_args, tmp_path, monkeypatch): + def fake_retrieve_data(passed_args, template, *args, **kwargs): + raise HTTPError(template, 500, "Server Error", hdrs=None, fp=None) + + monkeypatch.setattr(github_backup, "retrieve_data", fake_retrieve_data) + + with pytest.raises(HTTPError): + github_backup.backup_hooks(hook_args, tmp_path, _repository(), REPOS_TEMPLATE)