From 98398816fa7956ed3912f4c6328aeb19689c13b3 Mon Sep 17 00:00:00 2001 From: Tyler D Date: Thu, 4 Jan 2024 16:23:41 -0800 Subject: [PATCH 1/2] feat: Periodically check for hook updates on scan (#336) # Description This PR adds the feature to check for updates to hooks when running a scan. Since the update check doesn't need to be done on every scan, we only check at most once per week. This will prevent the added latency of calling out to GitHub (or wherever hooks are hosted) on every scan. [pre-commit](https://pre-commit.com/) does not expose the functionality of checking for hook updates without actually performing the update, so for now this PR imports functions directly from the pre-commit tool (since it is also written in python). This is not a best practice, but is probably the cleanest option we have for now. We can look into implementing this functionality in `pre-commit` itself in the future. Note that there is currently no way to specify whether to include the `--bleeding-edge` flag (implemented internally with a `tags_only` boolean). The implication is that if someone updates their version of a hook repository beyond the latest release, we will detect it as being out-of-date, instead of ahead. In practice, this feels like a day-2 kind of feature that won't be important to many users. This PR closes #176 . # Feature Work * Updates internal `.pre-commit-config.yaml` file to add hooks. Not sure why this repo did not have defined hooks previously? * Updates internal secureli config file to set the log level to DEBUG (we should see all output while developing secureli) * Instead of deserializing the `.pre-commit-config.yaml` file to a dictionary, this adds a proper pydantic model # Cleanup work Unrelated to the ticket, I also performed some fixes/cleanup: * Fixed bug in existing unit test causing `.pre-commit-config.yaml` to get overwritten * Cleaned up errors in type hints across numerous files * Cleaned up some comments & output for clarity/correctness * Minor cleanup refactoring * Fixed an error from `pytest` warning of use of the deprecated package `pkg_resources` (by using the recommended alternative) * Rename test function with duplicate name * Remove duplicate test fixture --- .github/workflows/publish.yml | 2 +- .pre-commit-config.yaml | 15 ++- .secureli.yaml | 2 +- secureli/abstractions/pre_commit.py | 82 ++++++++++++- secureli/actions/action.py | 13 +- secureli/actions/scan.py | 46 ++++++- secureli/main.py | 12 +- secureli/repositories/secureli_config.py | 14 ++- secureli/repositories/settings.py | 17 ++- secureli/services/language_support.py | 8 +- secureli/services/scanner.py | 36 +++--- secureli/services/updater.py | 4 +- secureli/utilities/secureli_meta.py | 4 +- tests/abstractions/test_pre_commit.py | 125 ++++++++++++++++++- tests/actions/test_scan_action.py | 146 ++++++++++++++++++----- tests/services/test_language_config.py | 2 +- tests/services/test_language_support.py | 1 + tests/services/test_logging_service.py | 9 -- 18 files changed, 432 insertions(+), 106 deletions(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 8f63dcff..a4eb7d69 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -146,7 +146,7 @@ jobs: name: Integration Testing needs: [ build-test, secureli-release, secureli-publish, deploy ] if: | - always() && + always() && (needs.secureli-publish.result == 'success' || needs.secureli-publish.result == 'skipped') && (needs.deploy.result == 'success' || needs.deploy.result == 'skipped') uses: ./.github/workflows/integration_testing.yml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4e6a92c4..69149ddd 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1 +1,14 @@ -repos: [] +# See https://pre-commit.com for more information +# See https://pre-commit.com/hooks.html for more hooks +repos: +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.5.0 + hooks: + - id: trailing-whitespace + - id: end-of-file-fixer + - id: check-yaml + - id: check-added-large-files +- repo: https://github.com/psf/black-pre-commit-mirror + rev: 23.11.0 + hooks: + - id: black diff --git a/.secureli.yaml b/.secureli.yaml index 41456942..ee482100 100644 --- a/.secureli.yaml +++ b/.secureli.yaml @@ -1,5 +1,5 @@ echo: - level: ERROR + level: DEBUG repo_files: exclude_file_patterns: - .idea/ diff --git a/secureli/abstractions/pre_commit.py b/secureli/abstractions/pre_commit.py index 7f4f6f24..71677e24 100644 --- a/secureli/abstractions/pre_commit.py +++ b/secureli/abstractions/pre_commit.py @@ -1,10 +1,18 @@ -import stat -import subprocess from pathlib import Path -from typing import Optional +# Note that this import is pulling from the pre-commit tool's internals. +# A cleaner approach would be to update pre-commit +# by implementing a dry-run option for the `autoupdate` command +from pre_commit.commands.autoupdate import RevInfo as HookRepoRevInfo +from typing import Any, Optional import pydantic +import re +import stat +import subprocess +import yaml + +from secureli.repositories.settings import PreCommitSettings class InstallFailedError(Exception): @@ -34,6 +42,16 @@ class ExecuteResult(pydantic.BaseModel): output: str +class RevisionPair(pydantic.BaseModel): + """ + Used for updating hooks. + This could alternatively be implemented as named tuple, but those can't subclass pydantic's BaseModel + """ + + oldRev: str + newRev: str + + class InstallResult(pydantic.BaseModel): """ The results of calling install @@ -105,6 +123,48 @@ def execute_hooks( else: return ExecuteResult(successful=True, output=output) + def check_for_hook_updates( + self, + config: PreCommitSettings, + tags_only: bool = True, + freeze: Optional[bool] = None, + ) -> dict[str, RevisionPair]: + """ + Call's pre-commit's undocumented/internal functions to check for updates to repositories containing hooks + :param config: A model representing the contents of the .pre-commit-config.yaml file. + See :meth:`~get_pre_commit_config` to deserialize the config file into a model. + :param tags_only: Represents whether we should check for the latest git tag or the latest git commit. + This defaults to true since anyone who cares enough to be on the "bleeding edge" (tags_only=False) can manually + update with `secureli update`. + :param freeze: This indicates whether tags names should be converted to the corresponding commit hash, + in case a tag is updated to point to a different commit in the future. If not specified, we check + the existing revision in the .pre-commit-config.yaml file to see if it looks like a commit (40-character hex string), + and infer that we should replace the commit hash with another commit hash ("freezing" the tag ref). + :returns: A dictionary of outdated with repositories, where the key is the repository URL and the RevisionPair value + indicates the old and new revisions. If the result is empty/falsy, then no updates were found. + """ + + git_commit_sha_pattern = re.compile(r"^[a-f0-9]{40}$") + + repos_to_update: dict[str, RevisionPair] = {} + for repo_config in config.repos: + repo_config_dict = repo_config.__dict__ | { + "repo": repo_config.url + } # PreCommitSettings uses "url" instead of "repo", so we need to copy that value over + old_rev_info = HookRepoRevInfo.from_config(repo_config_dict) + # if the revision currently specified in .pre-commit-config.yaml looks like a full git SHA + # (40-character hex string), then set freeze to True + freeze = ( + bool(git_commit_sha_pattern.fullmatch(repo_config.rev)) + if freeze is None + else freeze + ) + new_rev_info = old_rev_info.update(tags_only=tags_only, freeze=freeze) + revisions = RevisionPair(oldRev=old_rev_info.rev, newRev=new_rev_info.rev) + if revisions.oldRev != revisions.newRev: + repos_to_update[old_rev_info.repo] = revisions + return repos_to_update + def autoupdate_hooks( self, folder_path: Path, @@ -113,7 +173,7 @@ def autoupdate_hooks( repos: Optional[list] = None, ) -> ExecuteResult: """ - Updates the precommit hooks but executing precommit's autoupdate command. Additional info at + Updates the precommit hooks by executing precommit's autoupdate command. Additional info at https://pre-commit.com/#pre-commit-autoupdate :param folder_path: Indicates the git folder against which you run secureli :param bleeding_edge: True if updating to the bleeding edge of the default branch instead of @@ -199,3 +259,17 @@ def remove_unused_hooks(self, folder_path: Path) -> ExecuteResult: return ExecuteResult(successful=False, output=output) else: return ExecuteResult(successful=True, output=output) + + def get_pre_commit_config(self, folder_path: Path): + """ + Gets the contents of the .pre-commit-config file and returns it as a dictionary + :return: Dictionary containing the contents of the .pre-commit-config.yaml file + """ + path_to_config = folder_path / ".pre-commit-config.yaml" + with open(path_to_config, "r") as f: + # For some reason, the mocking causes an infinite loop when we try to use yaml.safe_load() + # directly on the file-like object f. Reading the contents of the file into a string as a workaround. + # return PreCommitSettings(**yaml.safe_load(f)) # TODO figure out why this isn't working + contents = f.read() + yaml_values = yaml.safe_load(contents) + return PreCommitSettings(**yaml_values) diff --git a/secureli/actions/action.py b/secureli/actions/action.py index 2b4a4d8e..7735260f 100644 --- a/secureli/actions/action.py +++ b/secureli/actions/action.py @@ -2,9 +2,6 @@ from enum import Enum from pathlib import Path from typing import Optional - -import pydantic - from secureli.abstractions.echo import EchoAbstraction, Color from secureli.abstractions.pre_commit import ( InstallFailedError, @@ -21,14 +18,13 @@ from secureli.services.scanner import ScannerService, ScanMode from secureli.services.updater import UpdaterService +import pydantic + class VerifyOutcome(str, Enum): INSTALL_CANCELED = "install-canceled" INSTALL_FAILED = "install-failed" INSTALL_SUCCEEDED = "install-succeeded" - UPGRADE_CANCELED = "upgrade-canceled" - UPGRADE_SUCCEEDED = "upgrade-succeeded" - UPGRADE_FAILED = "upgrade-failed" UPDATE_CANCELED = "update-canceled" UPDATE_SUCCEEDED = "update-succeeded" UPDATE_FAILED = "update-failed" @@ -249,7 +245,8 @@ def _update_secureli(self, always_yes: bool): update_result = self.action_deps.updater.update() details = update_result.output - self.action_deps.echo.print(details) + if details: + self.action_deps.echo.print(details) if update_result.successful: return VerifyResult(outcome=VerifyOutcome.UPDATE_SUCCEEDED) @@ -259,7 +256,7 @@ def _update_secureli(self, always_yes: bool): def _update_secureli_config_only(self, always_yes: bool) -> VerifyResult: self.action_deps.echo.print("seCureLI is using an out-of-date config.") response = always_yes or self.action_deps.echo.confirm( - "Update config only now?", + "Update configuration now?", default_response=True, ) if not response: diff --git a/secureli/actions/scan.py b/secureli/actions/scan.py index 90d46923..839cb9cf 100644 --- a/secureli/actions/scan.py +++ b/secureli/actions/scan.py @@ -1,10 +1,16 @@ import json import sys from pathlib import Path +from time import time from typing import Optional from secureli.abstractions.echo import EchoAbstraction -from secureli.actions.action import VerifyOutcome, Action, ActionDependencies +from secureli.actions.action import ( + VerifyOutcome, + Action, + ActionDependencies, + VerifyResult, +) from secureli.services.logging import LoggingService, LogAction from secureli.services.scanner import ( ScanMode, @@ -12,6 +18,8 @@ ) from secureli.utilities.usage_stats import post_log, convert_failures_to_failure_count +ONE_WEEK_IN_SECONDS: int = 7 * 24 * 60 * 60 + class ScanAction(Action): """The action for the secureli `scan` command, orchestrating services and outputs results""" @@ -34,6 +42,34 @@ def __init__( self.echo = echo self.logging = logging + def _check_secureli_hook_updates(self, folder_path: Path) -> VerifyResult: + """ + Queries repositories referenced by pre-commit hooks to check + if we have the latest revisions listed in the .pre-commit-config.yaml file + :param folder_path: The folder path containing the .pre-commit-config.yaml file + """ + + self.action_deps.echo.info("Checking for pre-commit hook updates...") + pre_commit_config = self.scanner.pre_commit.get_pre_commit_config(folder_path) + + repos_to_update = self.scanner.pre_commit.check_for_hook_updates( + pre_commit_config + ) + + if not repos_to_update: + self.action_deps.echo.info("No hooks to update") + return VerifyResult(outcome=VerifyOutcome.UP_TO_DATE) + + for repo, revs in repos_to_update.items(): + self.action_deps.echo.debug( + f"Found update for {repo}: {revs.oldRev} -> {revs.newRev}" + ) + self.action_deps.echo.warning( + "You have out-of-date pre-commit hooks. Run `secureli update` to update them." + ) + # Since we don't actually perform the updates here, return an outcome of UPDATE_CANCELLED + return VerifyResult(outcome=VerifyOutcome.UPDATE_CANCELED) + def scan_repo( self, folder_path: Path, @@ -53,6 +89,14 @@ def scan_repo( """ verify_result = self.verify_install(folder_path, False, always_yes) + # Check if pre-commit hooks are up-to-date + secureli_config = self.action_deps.secureli_config.load() + now: int = int(time()) + if (secureli_config.last_hook_update_check or 0) + ONE_WEEK_IN_SECONDS < now: + self._check_secureli_hook_updates(folder_path) + secureli_config.last_hook_update_check = now + self.action_deps.secureli_config.save(secureli_config) + if verify_result.outcome in self.halting_outcomes: return diff --git a/secureli/main.py b/secureli/main.py index 4ea1942d..a91d31f5 100644 --- a/secureli/main.py +++ b/secureli/main.py @@ -62,14 +62,14 @@ def init( help="Say 'yes' to every prompt automatically without input", ), directory: Annotated[ - Optional[Path], + Path, Option( ".", "--directory", "-d", help="Run secureli against a specific directory", ), - ] = ".", + ] = Path("."), ): """ Detect languages and initialize pre-commit hooks and linters for the project @@ -100,14 +100,14 @@ def scan( help="Limit the scan to a specific hook ID from your pre-commit config", ), directory: Annotated[ - Optional[Path], + Path, Option( ".", "--directory", "-d", help="Run secureli against a specific directory", ), - ] = ".", + ] = Path("."), ): """ Performs an explicit check of the repository to detect security issues without remote logging. @@ -133,14 +133,14 @@ def update( help="Update the installed pre-commit hooks to their latest versions", ), directory: Annotated[ - Optional[Path], + Path, Option( ".", "--directory", "-d", help="Run secureli against a specific directory", ), - ] = ".", + ] = Path("."), ): """ Update linters, configuration, and all else needed to maintain a secure repository. diff --git a/secureli/repositories/secureli_config.py b/secureli/repositories/secureli_config.py index fb721223..8e7351c4 100644 --- a/secureli/repositories/secureli_config.py +++ b/secureli/repositories/secureli_config.py @@ -9,9 +9,10 @@ class SecureliConfig(BaseModel): - languages: Optional[list[str]] - lint_languages: Optional[list[str]] - version_installed: Optional[str] + languages: Optional[list[str]] = None + lint_languages: Optional[list[str]] = None + version_installed: Optional[str] = None + last_hook_update_check: Optional[int] = 0 class DeprecatedSecureliConfig(BaseModel): @@ -94,10 +95,13 @@ def update(self) -> SecureliConfig: with open(secureli_config_path, "r") as f: data = yaml.safe_load(f) old_config = DeprecatedSecureliConfig.parse_obj(data) + languages: list[str] | None = ( + [old_config.overall_language] if old_config.overall_language else None + ) return SecureliConfig( - languages=[old_config.overall_language], - lint_languages=[old_config.overall_language], + languages=languages, + lint_languages=languages, version_installed=old_config.version_installed, ) diff --git a/secureli/repositories/settings.py b/secureli/repositories/settings.py index 8aeee9d6..80d3f37d 100644 --- a/secureli/repositories/settings.py +++ b/secureli/repositories/settings.py @@ -85,7 +85,7 @@ class LanguageSupportSettings(BaseSettings): command_timeout_seconds: int = Field(default=300) -class PreCommitHook(BaseSettings): +class PreCommitHook(BaseModel): """ Hook settings for pre-commit. """ @@ -96,19 +96,24 @@ class PreCommitHook(BaseSettings): exclude_file_patterns: Optional[list[str]] = Field(default=[]) -class PreCommitRepo(BaseSettings): +class PreCommitRepo(BaseModel): """ Repo settings for pre-commit. """ - url: str + url: str = Field(alias="repo") + rev: str hooks: list[PreCommitHook] = Field(default=[]) suppressed_hook_ids: list[str] = Field(default=[]) -class PreCommitSettings(BaseSettings): +class PreCommitSettings(BaseModel): """ Various adjustments that affect how seCureLI configures the pre-commit system. + + Extends schema for .pre-commit-config.yaml file. + See for details: https://pre-commit.com/#pre-commit-configyaml---top-level + """ repos: list[PreCommitRepo] = Field(default=[]) @@ -120,8 +125,8 @@ class SecureliFile(BaseModel): Represents the contents of the .secureli.yaml file """ - repo_files: Optional[RepoFilesSettings] - echo: Optional[EchoSettings] + repo_files: Optional[RepoFilesSettings] = None + echo: Optional[EchoSettings] = None language_support: Optional[LanguageSupportSettings] = Field(default=None) diff --git a/secureli/services/language_support.py b/secureli/services/language_support.py index 18fb3ed8..8476041a 100644 --- a/secureli/services/language_support.py +++ b/secureli/services/language_support.py @@ -1,5 +1,5 @@ from pathlib import Path -from typing import Callable, Optional, Any +from typing import Callable, Iterable, Optional, Any import pydantic import yaml @@ -105,9 +105,9 @@ def apply_support( self, languages: list[str], language_config_result: BuildConfigResult ) -> LanguageMetadata: """ - Applies Secure Build support for the provided language + Applies Secure Build support for the provided languages :param languages: list of languages to provide support for - :raises LanguageNotSupportedError if support for the language is not provided + :raises LanguageNotSupportedError if support for any language is not provided :return: Metadata including version of the language configuration that was just installed as well as a secret-detection hook ID, if present. """ @@ -193,7 +193,7 @@ def create_repo(raw_repo: dict) -> Repo: return HookConfiguration(repos=repos) def _build_pre_commit_config( - self, languages: list[str], lint_languages: list[str] + self, languages: list[str], lint_languages: Iterable[str] ) -> BuildConfigResult: """ Builds the final .pre-commit-config.yaml from all supported repo languages. Also returns any and all diff --git a/secureli/services/scanner.py b/secureli/services/scanner.py index 9f65d5e2..2ef4db09 100644 --- a/secureli/services/scanner.py +++ b/secureli/services/scanner.py @@ -4,9 +4,9 @@ import pydantic import re -import yaml from secureli.abstractions.pre_commit import PreCommitAbstraction +from secureli.repositories.settings import PreCommitSettings class ScanMode(str, Enum): @@ -94,12 +94,15 @@ def _parse_scan_ouput(self, folder_path: Path, output: str = "") -> ScanOuput: """ Parses the output from a scan and returns a list of Failure objects representing any hook rule failures during a scan. + :param folder_path: folder containing .pre-commit-config.yaml, usually repository root :param output: Raw output from a scan. :return: ScanOuput object representing a list of hook rule Failure objects. """ failures = [] failure_indexes = [] - config_data = self._get_config(folder_path) + pre_commit_config: PreCommitSettings = self.pre_commit.get_pre_commit_config( + folder_path + ) # Split the output up by each line and record the index of each failure output_by_line = output.split("\n") @@ -114,7 +117,7 @@ def _parse_scan_ouput(self, folder_path: Path, output: str = "") -> ScanOuput: id = self._remove_ansi_from_string(id_with_encoding) # Retrieve repo url for failure - repo = self._find_repo_from_id(hook_id=id, config=config_data) + repo = self._find_repo_from_id(hook_id=id, config=pre_commit_config) # Capture all output lines for this failure failure_output_list = self._get_single_failure_output( @@ -143,7 +146,7 @@ def _get_single_failure_output( return failure_lines - def _find_file_names(self, failure_output_list: list[str]) -> str: + def _find_file_names(self, failure_output_list: list[str]) -> list[str]: """ Finds the file name for a hook rule failure :param failure_index: The index of the initial failure in failure_output_list @@ -174,31 +177,20 @@ def _remove_ansi_from_string(self, string: str) -> str: return clean_string - def _find_repo_from_id(self, hook_id: str, config: dict): + def _find_repo_from_id(self, hook_id: str, config: PreCommitSettings): """ Retrieves the repo URL that a hook ID belongs to and returns it :param linter_id: The hook id we want to retrieve the repo url for - :config: A dict containing the contents of the .pre-commit-config.yaml file + :config: A model of the YAML data in .pre-commit-config.yaml file :return: The repo url our hook id belongs to """ - repos = config.get("repos") - for repo in repos: - hooks = repo["hooks"] - repo = repo["repo"] + for repo in config.repos: + hooks = repo.hooks + repo_str = repo.url for hook in hooks: - if hook["id"] == hook_id: - return repo + if hook.id == hook_id: + return repo_str return OutputParseErrors.REPO_NOT_FOUND - - def _get_config(self, folder_path: Path): - """ - Gets the contents of the .pre-commit-config file and returns it as a dict - :return: Dict containing the contents of the .pre-commit-config.yaml file - """ - path_to_config = folder_path / ".pre-commit-config.yaml" - with open(path_to_config, "r") as f: - data = yaml.safe_load(f) - return data diff --git a/secureli/services/updater.py b/secureli/services/updater.py index 33649f38..347b7df6 100644 --- a/secureli/services/updater.py +++ b/secureli/services/updater.py @@ -53,13 +53,13 @@ def update_hooks( if update_result.successful and not output: output = "No changes necessary.\n" - if update_result.successful and update_result.output: + if update_result.successful and output: prune_result = self.pre_commit.remove_unused_hooks(folder_path) output = output + "\nRemoving unused environments:\n" + prune_result.output return UpdateResult(successful=update_result.successful, output=output) - def update(self, folder_path: Path): + def update(self, folder_path: Path = Path(".")): """ Updates secureli with the latest local configuration. :param folder_path: Indicates the git folder against which you run secureli diff --git a/secureli/utilities/secureli_meta.py b/secureli/utilities/secureli_meta.py index 15804961..6478683e 100644 --- a/secureli/utilities/secureli_meta.py +++ b/secureli/utilities/secureli_meta.py @@ -1,6 +1,6 @@ -import pkg_resources +from importlib.metadata import version def secureli_version() -> str: """Leverage package resources to determine the current version of secureli""" - return pkg_resources.get_distribution("secureli").version + return version("secureli") diff --git a/tests/abstractions/test_pre_commit.py b/tests/abstractions/test_pre_commit.py index 1a901be5..44406167 100644 --- a/tests/abstractions/test_pre_commit.py +++ b/tests/abstractions/test_pre_commit.py @@ -16,6 +16,7 @@ ) test_folder_path = Path("does-not-matter") +example_git_sha = "a" * 40 @pytest.fixture() @@ -23,7 +24,8 @@ def settings_dict() -> dict: return PreCommitSettings( repos=[ PreCommitRepo( - url="http://example-repo.com/", + repo="http://example-repo.com/", + rev="master", hooks=[ PreCommitHook( id="hook-id", @@ -209,7 +211,7 @@ def test_that_pre_commit_autoupdate_hooks_ignores_repos_when_repos_is_a_dict( ): test_repos = {} mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) + execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) # type: ignore assert execute_result.successful assert "--repo {}" not in mock_subprocess.run.call_args_list[0].args[0] @@ -221,7 +223,7 @@ def test_that_pre_commit_autoupdate_hooks_converts_repos_when_repos_is_a_string( ): test_repos = "string" mock_subprocess.run.return_value = CompletedProcess(args=[], returncode=0) - execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) + execute_result = pre_commit.autoupdate_hooks(test_folder_path, repos=test_repos) # type: ignore assert execute_result.successful assert "--repo string" in mock_subprocess.run.call_args_list[0].args[0] @@ -284,3 +286,120 @@ def test_that_pre_commit_install_creates_pre_commit_hook_for_secureli( mock_open.assert_called_once() mock_chmod.assert_called_once() + + +def test_pre_commit_config_file_is_deserialized_correctly( + pre_commit: PreCommitAbstraction, +): + with um.patch("builtins.open", um.mock_open()) as mock_open: + mock_open.return_value.read.return_value = ( + "repos:\n" + " - repo: my-repo\n" + " rev: tag1\n" + " hooks:\n" + " - id: detect-secrets\n" + " args: ['--foo', '--bar']\n" + ) + pre_commit_config = pre_commit.get_pre_commit_config(test_folder_path) + assert pre_commit_config.repos[0].url == "my-repo" + assert pre_commit_config.repos[0].rev == "tag1" + assert pre_commit_config.repos[0].hooks[0].id == "detect-secrets" + + +@pytest.mark.parametrize( + argnames=["rev", "rev_is_sha"], argvalues=[("tag1", False), (example_git_sha, True)] +) +def test_check_for_hook_updates_infers_freeze_param_when_not_provided( + pre_commit: PreCommitAbstraction, + rev: str, + rev_is_sha: bool, +): + with um.patch( + "secureli.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info: + pre_commit_config_repo = PreCommitRepo( + repo="http://example-repo.com/", + rev=rev, + hooks=[PreCommitHook(id="hook-id")], + ) + pre_commit_config = PreCommitSettings(repos=[pre_commit_config_repo]) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date + pre_commit.check_for_hook_updates(pre_commit_config) + rev_info_mock.update.assert_called_with(tags_only=True, freeze=rev_is_sha) + + +def test_check_for_hook_updates_respects_freeze_param_when_false( + pre_commit: PreCommitAbstraction, +): + """ + When freeze is explicitly provided, the rev_info.update() method respect that value + regardless of whether the existing rev is a tag or a commit hash. + """ + with um.patch( + "secureli.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info: + pre_commit_config_repo = PreCommitRepo( + repo="http://example-repo.com/", + rev=example_git_sha, + hooks=[PreCommitHook(id="hook-id")], + ) + pre_commit_config = PreCommitSettings(repos=[pre_commit_config_repo]) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date + pre_commit.check_for_hook_updates(pre_commit_config, freeze=False) + rev_info_mock.update.assert_called_with(tags_only=True, freeze=False) + + +def test_check_for_hook_updates_respects_freeze_param_when_true( + pre_commit: PreCommitAbstraction, +): + with um.patch( + "secureli.abstractions.pre_commit.HookRepoRevInfo.from_config" + ) as mock_hook_repo_rev_info: + pre_commit_config_repo = PreCommitRepo( + repo="http://example-repo.com/", + rev="tag1", + hooks=[PreCommitHook(id="hook-id")], + ) + pre_commit_config = PreCommitSettings(repos=[pre_commit_config_repo]) + rev_info_mock = MagicMock(rev=pre_commit_config_repo.rev) + mock_hook_repo_rev_info.return_value = rev_info_mock + rev_info_mock.update.return_value = rev_info_mock # Returning the same revision info on update means the hook will be considered up to date + pre_commit.check_for_hook_updates(pre_commit_config, freeze=True) + rev_info_mock.update.assert_called_with(tags_only=True, freeze=True) + + +def test_check_for_hook_updates_returns_repos_with_new_revs( + pre_commit: PreCommitAbstraction, +): + with um.patch( + "secureli.abstractions.pre_commit.HookRepoRevInfo" + ) as mock_hook_repo_rev_info: + repo_urls = ["http://example-repo.com/", "http://example-repo-2.com/"] + old_rev = "tag1" + repo_1_new_rev = "tag2" + pre_commit_config = PreCommitSettings( + repos=[ + PreCommitRepo( + repo=repo_url, rev=old_rev, hooks=[PreCommitHook(id="hook-id")] + ) + for repo_url in repo_urls + ] + ) + repo_1_old_rev_mock = MagicMock(rev=old_rev, repo=repo_urls[0]) + repo_1_new_rev_mock = MagicMock(rev=repo_1_new_rev, repo=repo_urls[0]) + repo_2_old_rev_mock = MagicMock(rev=old_rev, repo=repo_urls[1]) + mock_hook_repo_rev_info.from_config = MagicMock( + side_effect=[repo_1_old_rev_mock, repo_2_old_rev_mock] + ) + repo_1_old_rev_mock.update.return_value = repo_1_new_rev_mock + repo_2_old_rev_mock.update.return_value = ( + repo_2_old_rev_mock # this update should return the same rev info + ) + updated_repos = pre_commit.check_for_hook_updates(pre_commit_config) + assert len(updated_repos) == 1 # only the first repo should be returned + assert updated_repos[repo_urls[0]].oldRev == "tag1" + assert updated_repos[repo_urls[0]].newRev == "tag2" diff --git a/tests/actions/test_scan_action.py b/tests/actions/test_scan_action.py index 77e710b8..ba58dd39 100644 --- a/tests/actions/test_scan_action.py +++ b/tests/actions/test_scan_action.py @@ -1,30 +1,57 @@ -import os from pathlib import Path -from unittest import mock -from unittest.mock import MagicMock - -import pytest - -from secureli.actions.action import ActionDependencies +from secureli.abstractions.pre_commit import RevisionPair +from secureli.actions.action import ActionDependencies, VerifyOutcome from secureli.actions.scan import ScanAction -from secureli.repositories.secureli_config import SecureliConfig +from secureli.repositories.secureli_config import SecureliConfig, VerifyConfigOutcome from secureli.repositories.settings import ( + PreCommitHook, + PreCommitRepo, + PreCommitSettings, SecureliFile, EchoSettings, EchoLevel, ) from secureli.services.scanner import ScanMode, ScanResult, Failure +from unittest import mock +from unittest.mock import MagicMock +from pytest_mock import MockerFixture + +import os +import pytest test_folder_path = Path("does-not-matter") @pytest.fixture() -def mock_scanner() -> MagicMock: +def mock_scanner(mock_pre_commit) -> MagicMock: mock_scanner = MagicMock() mock_scanner.scan_repo.return_value = ScanResult(successful=True, failures=[]) + mock_scanner.pre_commit = mock_pre_commit return mock_scanner +@pytest.fixture() +def mock_pre_commit() -> MagicMock: + mock_pre_commit = MagicMock() + mock_pre_commit.get_pre_commit_config.return_value = PreCommitSettings( + repos=[ + PreCommitRepo( + repo="http://example-repo.com/", + rev="master", + hooks=[ + PreCommitHook( + id="hook-id", + arguments=None, + additional_args=None, + ) + ], + ) + ] + ) + mock_pre_commit.check_for_hook_updates.return_value = {} + return mock_pre_commit + + @pytest.fixture() def mock_updater() -> MagicMock: mock_updater = MagicMock() @@ -32,33 +59,21 @@ def mock_updater() -> MagicMock: @pytest.fixture() -def mock_default_settings(mock_settings_repository: MagicMock) -> MagicMock: - mock_echo_settings = EchoSettings(EchoLevel.info) - mock_settings_file = SecureliFile(echo=mock_echo_settings) - mock_settings_repository.load.return_value = mock_settings_file - - return mock_settings_repository +def mock_get_time_near_epoch(mocker: MockerFixture) -> MagicMock: + return mocker.patch( + "secureli.actions.scan.time", return_value=1.0 + ) # 1 second after epoch @pytest.fixture() -def mock_default_settings_populated(mock_settings_repository: MagicMock) -> MagicMock: - mock_echo_settings = EchoSettings(EchoLevel.info) - mock_settings_file = SecureliFile( - echo=mock_echo_settings, - ) - mock_settings_repository.load.return_value = mock_settings_file - - return mock_settings_repository +def mock_get_time_far_from_epoch(mocker: MockerFixture) -> MagicMock: + return mocker.patch("secureli.actions.scan.time", return_value=1e6) @pytest.fixture() -def mock_default_settings_ignore_already_exists( - mock_settings_repository: MagicMock, -) -> MagicMock: - mock_echo_settings = EchoSettings(EchoLevel.info) - mock_settings_file = SecureliFile( - echo=mock_echo_settings, - ) +def mock_default_settings(mock_settings_repository: MagicMock) -> MagicMock: + mock_echo_settings = EchoSettings(level=EchoLevel.info) + mock_settings_file = SecureliFile(echo=mock_echo_settings) mock_settings_repository.load.return_value = mock_settings_file return mock_settings_repository @@ -193,3 +208,74 @@ def test_that_scan_repo_does_not_add_ignore_if_always_yes_is_true( scan_action.scan_repo(test_folder_path, ScanMode.STAGED_ONLY, True) mock_settings_repository.save.assert_not_called() + + +def test_that_scan_checks_for_updates( + scan_action: ScanAction, + mock_scanner: MagicMock, + mock_secureli_config: MagicMock, + mock_pass_install_verification: MagicMock, +): + scan_action.scan_repo(test_folder_path, ScanMode.STAGED_ONLY, always_yes=True) + mock_scanner.pre_commit.check_for_hook_updates.assert_called_once() + + +def test_that_scan_only_checks_for_updates_periodically( + scan_action: ScanAction, + mock_scanner: MagicMock, + mock_get_time_near_epoch: MagicMock, + mock_secureli_config: MagicMock, +): + mock_secureli_config.load.return_value = SecureliConfig() + + scan_action.scan_repo(test_folder_path, ScanMode.STAGED_ONLY, always_yes=True) + mock_scanner.pre_commit.check_for_hook_updates.assert_not_called() + + +def test_that_scan_update_check_uses_pre_commit_config( + scan_action: ScanAction, + mock_scanner: MagicMock, + mock_secureli_config: MagicMock, +): + mock_secureli_config.load.return_value = SecureliConfig() + scan_action.scan_repo(test_folder_path, ScanMode.STAGED_ONLY, always_yes=True) + mock_scanner.pre_commit.get_pre_commit_config.assert_called_once() + + +# Test that _check_secureli_hook_updates returns UP_TO_DATE if no hooks need updating +def test_scan_update_check_return_value_when_up_to_date( + scan_action: ScanAction, + mock_scanner: MagicMock, + mock_secureli_config: MagicMock, +): + mock_secureli_config.load.return_value = SecureliConfig() + result = scan_action._check_secureli_hook_updates(test_folder_path) + assert result.outcome == VerifyOutcome.UP_TO_DATE + + +# Test that _check_secureli_hook_updates returns UPDATE_CANCELED if hooks need updating +def test_scan_update_check_return_value_when_not_up_to_date( + scan_action: ScanAction, + mock_scanner: MagicMock, + mock_secureli_config: MagicMock, +): + mock_secureli_config.load.return_value = SecureliConfig() + mock_scanner.pre_commit.check_for_hook_updates.return_value = { + "http://example-repo.com/": RevisionPair(oldRev="old-rev", newRev="new-rev") + } + result = scan_action._check_secureli_hook_updates(test_folder_path) + assert result.outcome == VerifyOutcome.UPDATE_CANCELED + + +# Validate that scan_repo persists changes to the .secureli.yaml file after checking for hook updates +def test_that_scan_update_check_updates_last_check_time( + scan_action: ScanAction, + mock_scanner: MagicMock, + mock_get_time_far_from_epoch: MagicMock, + mock_secureli_config: MagicMock, + mock_pass_install_verification: MagicMock, +): + mock_secureli_config.verify.return_value = VerifyConfigOutcome.UP_TO_DATE + scan_action.scan_repo(test_folder_path, ScanMode.STAGED_ONLY, always_yes=True) + mock_secureli_config.save.assert_called_once() + assert mock_secureli_config.save.call_args.args[0].last_hook_update_check == 1e6 diff --git a/tests/services/test_language_config.py b/tests/services/test_language_config.py index f7ac5718..4464af75 100644 --- a/tests/services/test_language_config.py +++ b/tests/services/test_language_config.py @@ -174,7 +174,7 @@ def test_that_calculate_combined_configuration_ignores_lint_config( assert mock_data_loader.call_count == 1 -def test_that_calculate_combined_configuration_ignores_lint_config( +def test_that_calculate_combined_configuration_returns_valid_config_if_config_file_is_empty( language_config_service: LanguageConfigService, mock_data_loader: MagicMock, ): diff --git a/tests/services/test_language_support.py b/tests/services/test_language_support.py index ff2f0176..031da6ec 100644 --- a/tests/services/test_language_support.py +++ b/tests/services/test_language_support.py @@ -293,6 +293,7 @@ def test_that_language_support_throws_exception_when_language_config_file_cannot def test_that_language_support_handles_invalid_language_config( language_support_service: LanguageSupportService, mock_language_config_service: MagicMock, + mock_open: MagicMock, ): mock_language_config_service.get_language_config.return_value = ( LanguagePreCommitResult( diff --git a/tests/services/test_logging_service.py b/tests/services/test_logging_service.py index 6ca832e1..a873ddda 100644 --- a/tests/services/test_logging_service.py +++ b/tests/services/test_logging_service.py @@ -32,15 +32,6 @@ def mock_open(mocker: MockerFixture) -> MagicMock: return mock_open -@pytest.fixture() -def mock_open(mocker: MockerFixture) -> MagicMock: - mock_open = mocker.mock_open() - - mocker.patch("builtins.open", mock_open) - - return mock_open - - @pytest.fixture() def mock_language_support() -> MagicMock: mock_language_support = MagicMock() From f70429f6d3749f88a925ee41783adc90f9eeea5e Mon Sep 17 00:00:00 2001 From: github-actions Date: Fri, 5 Jan 2024 00:26:21 +0000 Subject: [PATCH 2/2] chore(release): Tag v0.19.0 [skip ci] --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 42809050..cc38fb15 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "secureli" -version = "0.18.0" +version = "0.19.0" description = "Secure Project Manager" authors = ["Caleb Tonn "] license = "Apache-2.0"