From 40651fa57a50b525b361ccd9776b35e0bb3fa846 Mon Sep 17 00:00:00 2001 From: Isaac Heist Date: Fri, 26 Apr 2024 12:38:44 -0700 Subject: [PATCH] feat: Update template file hook before running initial scan (#521) secureli-433 Update template file hook before running initial scan, not after ## Changes * Update template file hook before running initial scan * Added logging to action dependencies * Cleaned up update progress output (`pre-commit gc` not accepting `--config` flag) * Cleaned up container dependency injection * Updated test to get rid of warnings in terminal * Updated hooks versions ## Testing * Added required unit test * Updated existing unit tests and all passing ## Clean Code Checklist - [x] Meets acceptance criteria for issue - [x] New logic is covered with automated tests - [ ] Appropriate exception handling added - [x] Thoughtful logging included - [ ] Documentation is updated - [ ] Follow-up work is documented in TODOs - [ ] TODOs have a ticket associated with them - [x] No commented-out code included --- .secureli/.pre-commit-config.yaml | 36 +++++++++---------- secureli/actions/action.py | 30 ++++++++++++++++ secureli/actions/initializer.py | 6 ++-- secureli/actions/scan.py | 22 ++++++------ secureli/actions/update.py | 36 +++++++++---------- secureli/container.py | 6 +--- secureli/main.py | 1 - .../modules/shared/abstractions/pre_commit.py | 2 -- tests/actions/test_action.py | 18 ++++++++++ tests/actions/test_initializer_action.py | 14 ++++---- tests/actions/test_scan_action.py | 13 +++---- tests/actions/test_update_action.py | 5 ++- tests/application/test_main.py | 13 ++++--- 13 files changed, 117 insertions(+), 85 deletions(-) diff --git a/.secureli/.pre-commit-config.yaml b/.secureli/.pre-commit-config.yaml index 25af6f94..3fa2556f 100644 --- a/.secureli/.pre-commit-config.yaml +++ b/.secureli/.pre-commit-config.yaml @@ -1,18 +1,18 @@ -# 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.6.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: 24.4.1 - hooks: - - id: black -- repo: https://github.com/yelp/detect-secrets - rev: v1.4.0 - hooks: - - id: detect-secrets +# 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.6.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: 24.4.2 + hooks: + - id: black + - repo: https://github.com/yelp/detect-secrets + rev: v1.4.0 + hooks: + - id: detect-secrets diff --git a/secureli/actions/action.py b/secureli/actions/action.py index 3951916b..2895b671 100644 --- a/secureli/actions/action.py +++ b/secureli/actions/action.py @@ -1,10 +1,14 @@ from abc import ABC from pathlib import Path +from rich.progress import Progress + +from secureli.modules.observability.observability_services.logging import LoggingService from secureli.modules.shared.abstractions.echo import EchoAbstraction from secureli.modules.observability.consts.logging import TELEMETRY_DEFAULT_ENDPOINT from secureli.modules.shared.models.echo import Color from secureli.modules.shared.models.install import VerifyOutcome, VerifyResult from secureli.modules.shared.models import language +from secureli.modules.shared.models.logging import LogAction from secureli.modules.shared.models.scan import ScanMode from secureli.modules.language_analyzer import language_analyzer, language_support from secureli.modules.core.core_services.scanner import HooksScannerService @@ -33,6 +37,7 @@ def __init__( secureli_config: secureli_config.SecureliConfigRepository, settings: SecureliRepository, updater: UpdaterService, + logging: LoggingService, ): self.echo = echo self.language_analyzer = language_analyzer @@ -41,6 +46,7 @@ def __init__( self.secureli_config = secureli_config self.settings = settings self.updater = updater + self.logging = logging class Action(ABC): @@ -296,6 +302,9 @@ def _run_post_install_scan( ) ) + # Updated hooks prior to initial scan + self._initial_hooks_update(folder_path) + if secret_test_id := metadata.security_hook_id: self.action_deps.echo.print( f"The following language(s) support secrets detection: {format_sentence_list(config.languages)}" @@ -459,3 +468,24 @@ def _update_secureli_pre_commit_config_location( return VerifyResult( outcome=VerifyOutcome.UPDATE_CANCELED, file_path=deprecated_location ) + + def _initial_hooks_update(self, folder_path: Path): + with Progress(transient=True) as progress: + self.action_deps.echo.print( + "Updating hooks to the latest version prior to initial scan..." + ) + progress.add_task("Updating...", start=False, total=None) + update_result = self.action_deps.updater.update_hooks(folder_path) + details = ( + update_result.output + or "Unknown output while updating hooks to latest version" + ) + progress.stop() + self.action_deps.echo.print(details) + if not update_result.successful: + self.action_deps.logging.failure(LogAction.update, details) + else: + self.action_deps.echo.print( + "Hooks successfully updated to latest version" + ) + self.action_deps.logging.success(LogAction.update) diff --git a/secureli/actions/initializer.py b/secureli/actions/initializer.py index a07b8bb3..9ac5c094 100644 --- a/secureli/actions/initializer.py +++ b/secureli/actions/initializer.py @@ -13,10 +13,8 @@ class InitializerAction(Action): def __init__( self, action_deps: ActionDependencies, - logging: LoggingService, ): super().__init__(action_deps) - self.logging = logging def initialize_repo( self, folder_path: Path, reset: bool, always_yes: bool @@ -29,8 +27,8 @@ def initialize_repo( """ verify_result = self.verify_install(folder_path, reset, always_yes, files=None) if verify_result.outcome in ScanAction.halting_outcomes: - self.logging.failure(LogAction.init, verify_result.outcome) + self.action_deps.logging.failure(LogAction.init, verify_result.outcome) else: - self.logging.success(LogAction.init) + self.action_deps.logging.success(LogAction.init) return verify_result diff --git a/secureli/actions/scan.py b/secureli/actions/scan.py index e0766375..f5e6efae 100644 --- a/secureli/actions/scan.py +++ b/secureli/actions/scan.py @@ -36,8 +36,6 @@ class ScanAction(action.Action): def __init__( self, action_deps: action.ActionDependencies, - echo: EchoAbstraction, - logging: LoggingService, hooks_scanner: HooksScannerService, pii_scanner: PiiScannerService, git_repo: GitRepo, @@ -45,8 +43,6 @@ def __init__( super().__init__(action_deps) self.hooks_scanner = hooks_scanner self.pii_scanner = pii_scanner - self.echo = echo - self.logging = logging self.git_repo = git_repo def publish_results( @@ -66,12 +62,14 @@ def publish_results( and not action_successful ): result = utilities.post_log(log_str, Settings()) - self.echo.debug(result.result_message) + self.action_deps.echo.debug(result.result_message) if result.result == Result.SUCCESS: - self.logging.success(LogAction.publish) + self.action_deps.logging.success(LogAction.publish) else: - self.logging.failure(LogAction.publish, result.result_message) + self.action_deps.logging.failure( + LogAction.publish, result.result_message + ) def scan_repo( self, @@ -129,7 +127,7 @@ def scan_repo( scan_result = utilities.merge_scan_results([pii_scan_result, hooks_scan_result]) details = scan_result.output or "Unknown output during scan" - self.echo.print(details) + self.action_deps.echo.print(details) failure_count = len(scan_result.failures) scan_result_failures_json_string = json.dumps( @@ -141,9 +139,9 @@ def scan_repo( ) log_data = ( - self.logging.success(LogAction.scan) + self.action_deps.logging.success(LogAction.scan) if scan_result.successful - else self.logging.failure( + else self.action_deps.logging.failure( LogAction.scan, scan_result_failures_json_string, failure_count, @@ -156,7 +154,9 @@ def scan_repo( log_str=log_data.json(exclude_none=True), ) if scan_result.successful: - self.echo.print("Scan executed successfully and detected no issues!") + self.action_deps.echo.print( + "Scan executed successfully and detected no issues!" + ) else: sys.exit(ExitCode.SCAN_ISSUES_DETECTED.value) diff --git a/secureli/actions/update.py b/secureli/actions/update.py index 7498bc5b..0e27e5e1 100644 --- a/secureli/actions/update.py +++ b/secureli/actions/update.py @@ -13,13 +13,9 @@ class UpdateAction(Action): def __init__( self, action_deps: ActionDependencies, - echo: EchoAbstraction, - logging: LoggingService, updater: UpdaterService, ): super().__init__(action_deps) - self.echo = echo - self.logging = logging self.updater = updater def update_hooks(self, folder_path: Path, latest: Optional[bool] = False): @@ -32,35 +28,35 @@ def update_hooks(self, folder_path: Path, latest: Optional[bool] = False): """ with Progress(transient=True) as progress: if latest: - self.echo.print("Updating hooks to the latest version...") + self.action_deps.echo.print("Updating hooks to the latest version...") progress.add_task("Updating...", start=False, total=None) update_result = self.updater.update_hooks(folder_path) details = ( update_result.output or "Unknown output while updating hooks to latest version" ) - self.echo.print(details) + progress.stop() + self.action_deps.echo.print(details) if not update_result.successful: - self.echo.print(details) - progress.stop() - self.logging.failure(LogAction.update, details) + self.action_deps.echo.print(details) + self.action_deps.logging.failure(LogAction.update, details) else: - self.echo.print("Hooks successfully updated to latest version") - progress.stop() - self.logging.success(LogAction.update) + self.action_deps.echo.print( + "Hooks successfully updated to latest version" + ) + self.action_deps.logging.success(LogAction.update) else: - self.echo.print("Beginning update...") + self.action_deps.echo.print("Beginning update...") progress.add_task("Updating...", start=False, total=None) install_result = self.updater.update(folder_path) details = ( install_result.output or "Unknown output during hook installation" ) - self.echo.print(details) + progress.stop() + self.action_deps.echo.print(details) if not install_result.successful: - self.echo.print(details) - progress.stop() - self.logging.failure(LogAction.update, details) + self.action_deps.echo.print(details) + self.action_deps.logging.failure(LogAction.update, details) else: - self.echo.print("Update executed successfully.") - progress.stop() - self.logging.success(LogAction.update) + self.action_deps.echo.print("Update executed successfully.") + self.action_deps.logging.success(LogAction.update) diff --git a/secureli/container.py b/secureli/container.py index 38d5b1da..3fca156c 100644 --- a/secureli/container.py +++ b/secureli/container.py @@ -160,6 +160,7 @@ class Container(containers.DeclarativeContainer): secureli_config=secureli_config_repository, settings=settings_repository, updater=updater_service, + logging=logging_service, ) """The Build Action, used to render the build_data using the echo""" @@ -174,15 +175,12 @@ class Container(containers.DeclarativeContainer): initializer_action = providers.Factory( InitializerAction, action_deps=action_deps, - logging=logging_service, ) """Scan Action, representing what happens when the scan command is invoked""" scan_action = providers.Factory( ScanAction, action_deps=action_deps, - echo=echo, - logging=logging_service, hooks_scanner=hooks_scanner_service, pii_scanner=pii_scanner_service, git_repo=git_repo, @@ -192,7 +190,5 @@ class Container(containers.DeclarativeContainer): update_action = providers.Factory( UpdateAction, action_deps=action_deps, - echo=echo, - logging=logging_service, updater=updater_service, ) diff --git a/secureli/main.py b/secureli/main.py index 07b24e62..148ba6e6 100644 --- a/secureli/main.py +++ b/secureli/main.py @@ -101,7 +101,6 @@ def init( Path(directory), reset, yes ) if init_result.outcome in [ - VerifyOutcome.INSTALL_SUCCEEDED, VerifyOutcome.UP_TO_DATE, VerifyOutcome.UPDATE_SUCCEEDED, ]: diff --git a/secureli/modules/shared/abstractions/pre_commit.py b/secureli/modules/shared/abstractions/pre_commit.py index be2a333d..c91fc60e 100644 --- a/secureli/modules/shared/abstractions/pre_commit.py +++ b/secureli/modules/shared/abstractions/pre_commit.py @@ -296,8 +296,6 @@ def remove_unused_hooks(self, folder_path: Path) -> ExecuteResult: subprocess_args = [ "pre-commit", "gc", - "--config", - self.get_pre_commit_config_path(folder_path), "--color", "always", ] diff --git a/tests/actions/test_action.py b/tests/actions/test_action.py index af5217b0..c0127ed2 100644 --- a/tests/actions/test_action.py +++ b/tests/actions/test_action.py @@ -9,6 +9,7 @@ from secureli.modules.shared.models.echo import Color from secureli.modules.shared.models.install import VerifyOutcome from secureli.modules.shared.models import language +from secureli.modules.shared.models.logging import LogAction import secureli.modules.shared.models.repository as RepositoryModels import secureli.modules.shared.models.config as ConfigModels from secureli.modules.shared.models.scan import ScanFailure, ScanResult @@ -38,6 +39,7 @@ def action_deps( mock_secureli_config: MagicMock, mock_updater: MagicMock, mock_settings: MagicMock, + mock_logging_service: MagicMock, ) -> ActionDependencies: return ActionDependencies( mock_echo, @@ -47,6 +49,7 @@ def action_deps( mock_secureli_config, mock_settings, mock_updater, + mock_logging_service, ) @@ -768,3 +771,18 @@ def test_that_prompt_get_telemetry_api_url_returns_prompt_response( result = action._prompt_get_telemetry_api_url(False) assert result is mock_api_endpoint + + +def test_that_unsuccessful_initial_hooks_update_logs_details( + action: Action, action_deps: ActionDependencies +): + action_deps.updater.update_hooks.return_value = UpdateResult( + successful=False, output="Update failed" + ) + + action._initial_hooks_update(Path("test/path")) + + logging_result = action_deps.logging + + assert logging_result.failure + logging_result.failure.assert_called_once_with(LogAction.update, "Update failed") diff --git a/tests/actions/test_initializer_action.py b/tests/actions/test_initializer_action.py index a26fff90..97b298d3 100644 --- a/tests/actions/test_initializer_action.py +++ b/tests/actions/test_initializer_action.py @@ -32,6 +32,7 @@ def action_deps( mock_secureli_config: MagicMock, mock_settings: MagicMock, mock_updater: MagicMock, + mock_logging_service: MagicMock, ) -> ActionDependencies: return ActionDependencies( mock_echo, @@ -41,41 +42,38 @@ def action_deps( mock_secureli_config, mock_settings, mock_updater, + mock_logging_service, ) @pytest.fixture() def initializer_action( action_deps: ActionDependencies, - mock_logging_service: MagicMock, ) -> InitializerAction: return InitializerAction( action_deps=action_deps, - logging=mock_logging_service, ) def test_that_initialize_repo_does_not_load_config_when_resetting( initializer_action: InitializerAction, mock_secureli_config: MagicMock, - mock_echo: MagicMock, - mock_logging_service: MagicMock, ): initializer_action.initialize_repo(test_folder_path, True, True) mock_secureli_config.load.assert_not_called() - mock_logging_service.success.assert_called_once_with(LogAction.init) + initializer_action.action_deps.logging.success.assert_called_with(LogAction.init) def test_that_initialize_repo_logs_failure_when_failing_to_verify( initializer_action: InitializerAction, - mock_secureli_config: MagicMock, mock_language_analyzer: MagicMock, - mock_logging_service: MagicMock, ): mock_language_analyzer.analyze.side_effect = LanguageNotSupportedError initializer_action.initialize_repo(test_folder_path, True, True) - mock_logging_service.failure.assert_called_once_with(LogAction.init, ANY) + initializer_action.action_deps.logging.failure.assert_called_once_with( + LogAction.init, ANY + ) diff --git a/tests/actions/test_scan_action.py b/tests/actions/test_scan_action.py index adad3723..578b6741 100644 --- a/tests/actions/test_scan_action.py +++ b/tests/actions/test_scan_action.py @@ -115,6 +115,7 @@ def action_deps( mock_secureli_config: MagicMock, mock_settings_repository: MagicMock, mock_updater: MagicMock, + mock_logging_service: MagicMock, ) -> ActionDependencies: return ActionDependencies( mock_echo, @@ -124,20 +125,18 @@ def action_deps( mock_secureli_config, mock_settings_repository, mock_updater, + mock_logging_service, ) @pytest.fixture() def scan_action( action_deps: ActionDependencies, - mock_logging_service: MagicMock, mock_pii_scanner: MagicMock, mock_git_repo: MagicMock, ) -> ScanAction: return ScanAction( action_deps=action_deps, - echo=action_deps.echo, - logging=mock_logging_service, hooks_scanner=action_deps.hooks_scanner, pii_scanner=mock_pii_scanner, git_repo=mock_git_repo, @@ -371,7 +370,7 @@ def test_publish_results_always(scan_action: ScanAction, mock_post_log: MagicMoc ) mock_post_log.assert_called_once_with("log_str", Settings()) - scan_action.logging.success.assert_called_once_with(LogAction.publish) + scan_action.action_deps.logging.success.assert_called_once_with(LogAction.publish) def test_publish_results_on_fail_and_action_successful( @@ -384,7 +383,7 @@ def test_publish_results_on_fail_and_action_successful( ) mock_post_log.assert_not_called() - scan_action.logging.success.assert_not_called() + scan_action.action_deps.logging.success.assert_not_called() def test_publish_results_on_fail_and_action_not_successful( @@ -400,7 +399,9 @@ def test_publish_results_on_fail_and_action_not_successful( ) mock_post_log.assert_called_once_with("log_str", Settings()) - scan_action.logging.failure.assert_called_once_with(LogAction.publish, "Failure") + scan_action.action_deps.logging.failure.assert_called_once_with( + LogAction.publish, "Failure" + ) def test_verify_install_is_called_with_commted_files( diff --git a/tests/actions/test_update_action.py b/tests/actions/test_update_action.py index 4ca8fc66..ac7760e2 100644 --- a/tests/actions/test_update_action.py +++ b/tests/actions/test_update_action.py @@ -32,6 +32,7 @@ def action_deps( mock_secureli_config: MagicMock, mock_settings: MagicMock, mock_updater: MagicMock, + mock_logging_service: MagicMock, ) -> ActionDependencies: return ActionDependencies( mock_echo, @@ -41,19 +42,17 @@ def action_deps( mock_secureli_config, mock_settings, mock_updater, + mock_logging_service, ) @pytest.fixture() def update_action( action_deps: ActionDependencies, - mock_logging_service: MagicMock, mock_updater: MagicMock, ) -> UpdateAction: return UpdateAction( action_deps=action_deps, - echo=action_deps.echo, - logging=mock_logging_service, updater=mock_updater, ) diff --git a/tests/application/test_main.py b/tests/application/test_main.py index a16d7c0e..f16a3902 100644 --- a/tests/application/test_main.py +++ b/tests/application/test_main.py @@ -57,7 +57,7 @@ def test_that_app_implements_version_option( result = CliRunner().invoke(secureli.main.app, [test_input]) mock_container = request.getfixturevalue("mock_container") - assert result.exit_code is 0 + assert result.exit_code == 0 assert secureli_version() in result.stdout mock_container.init_resources.assert_not_called() mock_container.wire.assert_not_called() @@ -70,7 +70,7 @@ def test_that_version_callback_does_not_return_hook_versions_if_no_config( with patch.object(Path, "exists", return_value=False): result = CliRunner().invoke(secureli.main.app, [test_input]) - assert result.exit_code is 0 + assert result.exit_code == 0 assert secureli_version() in result.stdout assert "\nHook Versions:" not in result.stdout @@ -82,7 +82,7 @@ def test_that_version_callback_returns_hook_versions_if_config( with patch.object(Path, "exists", return_value=True): result = CliRunner().invoke(secureli.main.app, [test_input]) - assert result.exit_code is 0 + assert result.exit_code == 0 assert secureli_version() in result.stdout assert "\nHook Versions:" in result.stdout assert "--------------" in result.stdout @@ -91,7 +91,7 @@ def test_that_version_callback_returns_hook_versions_if_config( def test_that_app_ignores_version_callback(mock_container: MagicMock): result = CliRunner().invoke(secureli.main.app, ["scan"]) - assert result.exit_code is 0 + assert result.exit_code == 0 assert secureli_version() not in result.stdout mock_container.init_resources.assert_called_once() mock_container.wire.assert_called_once() @@ -100,7 +100,6 @@ def test_that_app_ignores_version_callback(mock_container: MagicMock): @pytest.mark.parametrize( "test_input", [ - VerifyOutcome.INSTALL_SUCCEEDED, VerifyOutcome.UPDATE_SUCCEEDED, VerifyOutcome.UP_TO_DATE, ], @@ -138,7 +137,7 @@ def test_that_unsuccessful_init_does_not_run_update( def test_that_scan_implements_file_arg(mock_container: MagicMock): result = CliRunner().invoke(secureli.main.app, ["scan", "--file", "test.py"]) - assert result.exit_code is 0 + assert result.exit_code == 0 assert result.stdout == "" mock_container.init_resources.assert_called_once() mock_container.scan_action.return_value.scan_repo.assert_called_once_with( @@ -155,7 +154,7 @@ def test_that_scan_implements_multiple_file_args(mock_container: MagicMock): result = CliRunner().invoke( secureli.main.app, ["scan", "--file", "test.py", "--file", "test2.py"] ) - assert result.exit_code is 0 + assert result.exit_code == 0 assert result.stdout == "" mock_container.init_resources.assert_called_once() mock_container.scan_action.return_value.scan_repo.assert_called_once_with(