feat: add logic to handle overwriting existing pre-commit hooks (#410)

secureli-372

closes #372 

Handles overwriting existing pre-commit install in the user's repo on
new install

## Changes
* Adds logic to check if pre-commit hook exists and if so, creates a
backup of the current file and overwrites it with SeCureLI's pre-commit
content
* Print a warning to the user to let them know that the file is being
overwritten and that a backup is created

## Testing
Initialize SeCureLI on a git repo with an existing pre-commit file
<img width="677" alt="Screenshot 2024-01-30 at 2 01 42 PM"
src="https://github.com/slalombuild/secureli/assets/58826693/11240502-db9b-4b29-a3aa-87b400518638">

## Clean Code Checklist
<!-- This is here to support you. Some/most checkboxes may not apply to
your change -->
- [x] Meets acceptance criteria for issue
- [x] New logic is covered with automated tests
- [x] 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


<!--
Github-flavored markdown reference:
https://docs.github.com/en/get-started/writing-on-github
-->

---------

Co-authored-by: Tyler D <[email protected]>

secureli/abstractions/pre_commit.py

@@ -1,4 +1,6 @@
+import datetime
 from pathlib import Path
+import shutil
 
 # Note that this import is pulling from the pre-commit tool's internals.
 # A cleaner approach would be to update pre-commit
@@ -58,7 +60,7 @@ class InstallResult(pydantic.BaseModel):
     """
 
     successful: bool
-    # version_installed: str
+    backup_hook_path: Optional[str]
 
 
 class PreCommitAbstraction:
@@ -72,22 +74,40 @@ def __init__(
     ):
         self.command_timeout_seconds = command_timeout_seconds
 
-    def install(self, folder_path: Path):
+    def install(self, folder_path: Path) -> InstallResult:
         """
-        Creates the pre-commit hook file in the .git directory so that `secureli scan` is run on each commit
+        Creates a new  pre-commit hook file in .git/hooks with the SeCureLI pre-commit hook contents.
+        If a pre-commit hook file already exists, the existing file is copied as a back up and
+        overwritten with the SeCureLI hook contents.
         """
 
-        # Write pre-commit with invocation of `secureli scan`
-        pre_commit_hook = folder_path / ".git/hooks/pre-commit"
+        pre_commit_hook = Path(folder_path / ".git/hooks/pre-commit")
+        backup_hook_path: str = None
+
+        if pre_commit_hook.is_file():
+            # strip out certain chars from the ISO8601 timestamp for inclusion in file name
+            timestamp = re.sub(r"[.:-]+", "", datetime.datetime.now().isoformat())
+            backup_hook_path = f"{pre_commit_hook}.backup.{timestamp}"
+            shutil.copy2(
+                pre_commit_hook,
+                backup_hook_path,
+            )
+
         with open(pre_commit_hook, "w") as f:
-            f.write("#!/bin/sh\n")
             # if running scan as part of a commit (as opposed to a manual invocation of `secureli scan`),
             # then publish the results to the configured observability platform (e.g. New Relic)
-            f.write("secureli scan --publish-results=always\n")
+            pre_commit_hook_contents = """
+            ################## Auto generated by SeCureLI ##################
+            #!/bin/sh
+            secureli scan --publish-results=always
+            """
+            f.write(pre_commit_hook_contents)
 
         # Make pre-commit executable
         pre_commit_hook.chmod(pre_commit_hook.stat().st_mode | stat.S_IEXEC)
 
+        return InstallResult(successful=True, backup_hook_path=backup_hook_path)
+
     def execute_hooks(
         self, folder_path: Path, all_files: bool = False, hook_id: Optional[str] = None
     ) -> ExecuteResult:

secureli/actions/action.py

@@ -244,8 +244,18 @@ def _run_post_install_scan(
         """
 
         if new_install:
-            # Create seCureLI pre-commit hook with invocation of `secureli scan`
-            self.action_deps.updater.pre_commit.install(folder_path)
+            pre_commit_install_result = self.action_deps.updater.pre_commit.install(
+                folder_path
+            )
+
+            if pre_commit_install_result.backup_hook_path != None:
+                self.action_deps.echo.warning(
+                    (
+                        "An existing pre-commit hook file has been detected at /.git/hooks/pre-commit\n"
+                        "A backup file has been created and the existing file has been overwritten\n"
+                        f"Backup file: {pre_commit_install_result.backup_hook_path}"
+                    )
+                )
 
         if secret_test_id := metadata.security_hook_id:
             self.action_deps.echo.print(
@@ -261,7 +271,7 @@ def _run_post_install_scan(
 
         else:
             self.action_deps.echo.warning(
-                f"{config.languages} does not support secrets detection, skipping"
+                f"{format_sentence_list(config.languages)} does not support secrets detection, skipping"
             )
 
     def _detect_languages(self, folder_path: Path) -> list[str]:

tests/abstractions/test_pre_commit.py

@@ -1,3 +1,5 @@
+import datetime
+import shutil
 import unittest.mock as um
 from pathlib import Path
 from subprocess import CompletedProcess
@@ -7,6 +9,7 @@
 from pytest_mock import MockerFixture
 
 from secureli.abstractions.pre_commit import (
+    InstallResult,
     PreCommitAbstraction,
 )
 from secureli.repositories.settings import (
@@ -279,13 +282,45 @@ def test_that_pre_commit_install_creates_pre_commit_hook_for_secureli(
         um.patch.object(Path, "exists") as mock_exists,
         um.patch.object(Path, "chmod") as mock_chmod,
         um.patch.object(Path, "stat"),
+        um.patch.object(Path, "is_file") as mock_is_file,
+        um.patch.object(shutil, "copy2") as mock_copy,
     ):
         mock_exists.return_value = True
+        mock_is_file.return_value = False
 
-        pre_commit.install(test_folder_path)
+        mock_is_file.return_value = False
 
+        result = pre_commit.install(test_folder_path)
+
+        assert result == InstallResult(successful=True, backup_hook_path=None)
         mock_open.assert_called_once()
         mock_chmod.assert_called_once()
+        mock_copy.assert_not_called()
+
+
+def test_that_pre_commit_install_creates_backup_file_when_already_exists(
+    pre_commit: PreCommitAbstraction,
+):
+    mock_backup_datetime = datetime.datetime(2024, 1, 1, 6, 30, 45)
+    with (
+        um.patch("builtins.open", um.mock_open()),
+        um.patch.object(Path, "is_file") as mock_is_file,
+        um.patch.object(Path, "chmod"),
+        um.patch.object(Path, "stat"),
+        um.patch.object(shutil, "copy2") as mock_copy,
+        um.patch("datetime.datetime") as mock_dt,
+    ):
+        mock_is_file.return_value = True
+        mock_dt.now.return_value = mock_backup_datetime
+
+        result = pre_commit.install(test_folder_path)
+
+        assert result.successful == True
+        assert (
+            "/.git/hooks/pre-commit.backup.20240101T063045" in result.backup_hook_path
+        )
+        mock_is_file.assert_called_once()
+        mock_copy.assert_called_once()
 
 
 def test_pre_commit_config_file_is_deserialized_correctly(

tests/actions/test_action.py

@@ -2,6 +2,7 @@
 from unittest.mock import MagicMock
 
 import pytest
+from secureli.abstractions.pre_commit import InstallResult
 
 from secureli.actions.action import Action, ActionDependencies, VerifyOutcome
 from secureli.models.echo import Color
@@ -146,6 +147,7 @@ def test_that_initialize_repo_install_flow_warns_about_skipped_files(
     action: Action,
     mock_language_analyzer: MagicMock,
     mock_echo: MagicMock,
+    mock_updater: MagicMock,
 ):
     mock_language_analyzer.analyze.return_value = AnalyzeResult(
         language_proportions={
@@ -163,6 +165,10 @@ def test_that_initialize_repo_install_flow_warns_about_skipped_files(
         ],
     )
 
+    mock_updater.pre_commit.install.return_value = InstallResult(
+        successful=True, backup_hook_path=None
+    )
+
     action.verify_install(test_folder_path, reset=True, always_yes=True)
 
     assert (
@@ -247,7 +253,7 @@ def test_that_initialize_repo_updates_repo_config_if_old_schema(
     assert result.outcome == VerifyOutcome.UP_TO_DATE
 
 
-def test_that_initialize_repo_reports_errors_when_schema_upgdate_fails(
+def test_that_initialize_repo_reports_errors_when_schema_update_fails(
     action: Action,
     mock_secureli_config: MagicMock,
     mock_language_support: MagicMock,
@@ -300,6 +306,7 @@ def test_that_initialize_repo_prints_warnings_for_failed_linter_config_writes(
     action: Action,
     mock_language_support: MagicMock,
     mock_echo: MagicMock,
+    mock_updater: MagicMock,
 ):
     config_write_error = "Failed to write config file for RadLang"
 
@@ -309,6 +316,10 @@ def test_that_initialize_repo_prints_warnings_for_failed_linter_config_writes(
         linter_config_write_errors=[config_write_error],
     )
 
+    mock_updater.pre_commit.install.return_value = InstallResult(
+        successful=True, backup_hook_path=None
+    )
+
     action.verify_install(test_folder_path, reset=True, always_yes=True)
 
     mock_echo.warning.assert_called_once_with(config_write_error)
@@ -394,6 +405,38 @@ def test_that_verify_install_returns_success_result_newly_detected_language_inst
     assert verify_result.outcome == VerifyOutcome.INSTALL_SUCCEEDED
 
 
+def test_that_initialize_repo_install_flow_warns_about_overwriting_pre_commit_file(
+    action: Action,
+    mock_language_analyzer: MagicMock,
+    mock_echo: MagicMock,
+    mock_updater: MagicMock,
+):
+    mock_language_analyzer.analyze.return_value = AnalyzeResult(
+        language_proportions={
+            "RadLang": 0.75,
+        },
+        skipped_files=[],
+    )
+
+    install_result = InstallResult(
+        successful=True, backup_hook_path="pre-commit.backup"
+    )
+
+    mock_updater.pre_commit.install.return_value = install_result
+
+    action.verify_install(test_folder_path, reset=True, always_yes=True)
+
+    mock_echo.warning.assert_called_once_with(
+        (
+            (
+                "An existing pre-commit hook file has been detected at /.git/hooks/pre-commit\n"
+                "A backup file has been created and the existing file has been overwritten\n"
+                f"Backup file: {install_result.backup_hook_path}"
+            )
+        )
+    )
+
+
 def test_that_update_secureli_handles_declined_update(
     action: Action,
     mock_echo: MagicMock,
@@ -522,3 +565,53 @@ def test_that_prompt_to_install_does_not_prompt_if_always_yes(
 
     assert result == True
     mock_echo.confirm.not_called()
+
+
+def test_that_post_install_scan_creates_pre_commit_on_new_install(
+    action: Action, mock_updater: MagicMock
+):
+    action._run_post_install_scan(
+        "test/path", SecureliConfig(), LanguageMetadata(version="0.03"), True
+    )
+
+    mock_updater.pre_commit.install.assert_called_once()
+
+
+def test_that_post_install_scan_ignores_creating_pre_commit_on_existing_install(
+    action: Action, mock_updater: MagicMock
+):
+    action._run_post_install_scan(
+        "test/path", SecureliConfig(), LanguageMetadata(version="0.03"), False
+    )
+
+    mock_updater.pre_commit.install.assert_not_called()
+
+
+def test_that_post_install_scan_scans_repo(
+    action: Action, mock_scanner: MagicMock, mock_echo: MagicMock
+):
+    action._run_post_install_scan(
+        "test/path",
+        SecureliConfig(),
+        LanguageMetadata(version="0.03", security_hook_id="secrets-hook"),
+        False,
+    )
+
+    mock_scanner.scan_repo.assert_called_once()
+    mock_echo.warning.assert_not_called()
+
+
+def test_that_post_install_scan_does_not_scan_repo_when_no_security_hook_id(
+    action: Action, mock_scanner: MagicMock, mock_echo: MagicMock
+):
+    action._run_post_install_scan(
+        "test/path",
+        SecureliConfig(languages=["RadLang"]),
+        LanguageMetadata(version="0.03"),
+        False,
+    )
+
+    mock_scanner.scan_repo.assert_not_called()
+    mock_echo.warning.assert_called_once_with(
+        "RadLang does not support secrets detection, skipping"
+    )