feat: Only publish metrics on commit (#353)

Instead of only detecting scan vs commit scenarios, I added a CLI param
to set when metrics should be published (`always`, `never`, or
`on-fail`) and utilized that by setting the git pre-commit hook to
`secureli scan --publish-results=always`.

One other notable change is that I added a `models/` directory. I think
it would make sense to migrate various pydantic models & enums there in
the future.

.pre-commit-config.yaml

@@ -12,3 +12,7 @@ repos:
     rev: 23.11.0
     hooks:
     -   id: black
+-   repo: https://github.com/yelp/detect-secrets
+    rev: v1.4.0
+    hooks:
+    -   id: detect-secrets

secureli/abstractions/pre_commit.py

@@ -81,7 +81,9 @@ def install(self, folder_path: Path):
         pre_commit_hook = folder_path / ".git/hooks/pre-commit"
         with open(pre_commit_hook, "w") as f:
             f.write("#!/bin/sh\n")
-            f.write("secureli scan\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")
 
         # Make pre-commit executable
         pre_commit_hook.chmod(pre_commit_hook.stat().st_mode | stat.S_IEXEC)

secureli/actions/scan.py

@@ -11,6 +11,8 @@
     ActionDependencies,
     VerifyResult,
 )
+from secureli.models.publish_results import PublishResultsOption
+from secureli.models.result import Result
 from secureli.services.logging import LoggingService, LogAction
 from secureli.services.scanner import (
     ScanMode,
@@ -70,11 +72,36 @@ def _check_secureli_hook_updates(self, folder_path: Path) -> VerifyResult:
         # Since we don't actually perform the updates here, return an outcome of UPDATE_CANCELLED
         return VerifyResult(outcome=VerifyOutcome.UPDATE_CANCELED)
 
+    def publish_results(
+        self,
+        publish_results_condition: PublishResultsOption,
+        action_successful: bool,
+        log_str: str,
+    ):
+        """
+        Publish the results of the scan to the configured observability platform
+        :param publish_results_condition: When to publish the results of the scan to the configured observability platform
+        :param action_successful: Whether we should publish a success or failure
+        :param log_str: a string to be POSTed to backend instrumentation
+        """
+        if publish_results_condition == PublishResultsOption.ALWAYS or (
+            publish_results_condition == PublishResultsOption.ON_FAIL
+            and not action_successful
+        ):
+            result = post_log(log_str)
+            self.echo.debug(result.result_message)
+
+            if result.result == Result.SUCCESS:
+                self.logging.success(LogAction.publish)
+            else:
+                self.logging.failure(LogAction.publish, result.result_message)
+
     def scan_repo(
         self,
         folder_path: Path,
         scan_mode: ScanMode,
         always_yes: bool,
+        publish_results_condition: PublishResultsOption = PublishResultsOption.NEVER,
         specific_test: Optional[str] = None,
     ):
         """
@@ -114,18 +141,22 @@ def scan_repo(
             scan_result.failures
         )
 
-        if not scan_result.successful:
-            log_data = self.logging.failure(
+        log_data = (
+            self.logging.success(LogAction.scan)
+            if scan_result.successful
+            else self.logging.failure(
                 LogAction.scan,
                 scan_result_failures_json_string,
                 failure_count,
                 individual_failure_count,
             )
-
-            post_log(log_data.json(exclude_none=True))
-            sys.exit("Issues Found...Aborting")
-        else:
+        )
+        self.publish_results(
+            publish_results_condition,
+            action_successful=scan_result.successful,
+            log_str=log_data.json(exclude_none=True),
+        )
+        if scan_result.successful:
             self.echo.print("Scan executed successfully and detected no issues!")
-            log_data = self.logging.success(LogAction.scan)
-
-            post_log(log_data.json(exclude_none=True))
+        else:
+            sys.exit(1)

secureli/main.py

@@ -8,6 +8,7 @@
 from secureli.actions.setup import SetupAction
 from secureli.container import Container
 from secureli.abstractions.echo import Color
+from secureli.models.publish_results import PublishResultsOption
 from secureli.resources import read_resource
 from secureli.settings import Settings
 import secureli.repositories.secureli_config as SecureliConfig
@@ -93,6 +94,10 @@ def scan(
         "-m",
         help="Scan the files you're about to commit (the default) or all files in the repo.",
     ),
+    publish_results: PublishResultsOption = Option(
+        "never",
+        help="When to publish the results of the scan to the configured observability platform",
+    ),
     specific_test: Optional[str] = Option(
         None,
         "--specific-test",
@@ -113,7 +118,13 @@ def scan(
     Performs an explicit check of the repository to detect security issues without remote logging.
     """
     SecureliConfig.FOLDER_PATH = Path(directory)
-    container.scan_action().scan_repo(Path(directory), mode, yes, specific_test)
+    container.scan_action().scan_repo(
+        folder_path=Path(directory),
+        scan_mode=mode,
+        always_yes=False,
+        publish_results_condition=publish_results,
+        specific_test=specific_test,
+    )
 
 
 @app.command(hidden=True)

secureli/models/publish_results.py

@@ -0,0 +1,16 @@
+from dataclasses import dataclass
+from enum import Enum
+
+from secureli.models.result import Result
+
+
+class PublishResultsOption(Enum):
+    ALWAYS = "always"
+    NEVER = "never"
+    ON_FAIL = "on-fail"
+
+
+@dataclass
+class PublishLogResult:
+    result: Result
+    result_message: str

secureli/models/result.py

@@ -0,0 +1,6 @@
+from enum import Enum
+
+
+class Result(Enum):
+    SUCCESS = "SUCCESS"
+    FAILURE = "FAILURE"

secureli/services/logging.py

@@ -37,6 +37,7 @@ class LogAction(str, Enum):
     init = "INIT"
     build = "_BUILD"
     update = "UPDATE"
+    publish = "PUBLISH"  # "PUBLISH" does not correspond to a CLI action/subcommand
 
 
 class LogFailure(pydantic.BaseModel):

secureli/utilities/usage_stats.py

@@ -1,5 +1,7 @@
 import requests
 import os
+from secureli.models.publish_results import PublishLogResult
+from secureli.models.result import Result
 
 from secureli.services.scanner import Failure
 from collections import Counter
@@ -20,7 +22,7 @@ def convert_failures_to_failure_count(failure_list: list[Failure]):
     return failure_count_list
 
 
-def post_log(log_data: str):
+def post_log(log_data: str) -> PublishLogResult:
     """
     Send a log through http post
     :param log_data: a string to be sent to backend instrumentation
@@ -30,10 +32,19 @@ def post_log(log_data: str):
     API_KEY = os.getenv("API_KEY")
 
     if not API_ENDPOINT or not API_KEY:
-        return
-
-    result = requests.post(
-        url=API_ENDPOINT, headers={"Api-Key": API_KEY}, data=log_data
-    )
-
-    return result.text
+        return PublishLogResult(
+            result=Result.FAILURE,
+            result_message="API_ENDPOINT or API_KEY not found in environment variables",
+        )
+
+    try:
+        result = requests.post(
+            url=API_ENDPOINT, headers={"Api-Key": API_KEY}, data=log_data
+        )
+    except Exception as e:
+        return PublishLogResult(
+            result=Result.FAILURE,
+            result_message=f'Error posting log to {API_ENDPOINT}: "{e}"',
+        )
+
+    return PublishLogResult(result=Result.SUCCESS, result_message=result.text)

tests/actions/test_scan_action.py

@@ -2,6 +2,8 @@
 from secureli.abstractions.pre_commit import RevisionPair
 from secureli.actions.action import ActionDependencies, VerifyOutcome
 from secureli.actions.scan import ScanAction
+from secureli.models.publish_results import PublishResultsOption
+from secureli.models.result import Result
 from secureli.repositories.secureli_config import SecureliConfig, VerifyConfigOutcome
 from secureli.repositories.settings import (
     PreCommitHook,
@@ -11,6 +13,7 @@
     EchoSettings,
     EchoLevel,
 )
+from secureli.services.logging import LogAction
 from secureli.services.scanner import ScanMode, ScanResult, Failure
 from unittest import mock
 from unittest.mock import MagicMock
@@ -123,6 +126,11 @@ def scan_action(
     )
 
 
+@pytest.fixture()
+def mock_post_log(mocker: MockerFixture) -> MagicMock:
+    return mocker.patch("secureli.actions.scan.post_log")
+
+
 # @mock.patch.dict(os.environ, {"API_KEY": "", "API_ENDPOINT": ""}, clear=True)
 # def test_that_scan_repo_errors_if_not_successful(
 #     scan_action: ScanAction,
@@ -279,3 +287,34 @@ def test_that_scan_update_check_updates_last_check_time(
     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
+
+
+def test_publish_results_always(scan_action: ScanAction, mock_post_log: MagicMock):
+    mock_post_log.return_value.result = Result.SUCCESS
+    mock_post_log.return_value.result_message = "Success"
+
+    scan_action.publish_results(PublishResultsOption.ALWAYS, True, "log_str")
+
+    mock_post_log.assert_called_once_with("log_str")
+    scan_action.logging.success.assert_called_once_with(LogAction.publish)
+
+
+def test_publish_results_on_fail_and_action_successful(
+    scan_action: ScanAction, mock_post_log: MagicMock
+):
+    scan_action.publish_results(PublishResultsOption.ON_FAIL, True, "log_str")
+
+    mock_post_log.assert_not_called()
+    scan_action.logging.success.assert_not_called()
+
+
+def test_publish_results_on_fail_and_action_not_successful(
+    scan_action: ScanAction, mock_post_log: MagicMock
+):
+    mock_post_log.return_value.result = Result.FAILURE
+    mock_post_log.return_value.result_message = "Failure"
+
+    scan_action.publish_results(PublishResultsOption.ON_FAIL, False, "log_str")
+
+    mock_post_log.assert_called_once_with("log_str")
+    scan_action.logging.failure.assert_called_once_with(LogAction.publish, "Failure")

tests/utilities/test_usage_stats.py

@@ -1,3 +1,5 @@
+from secureli.models.publish_results import PublishLogResult
+from secureli.models.result import Result
 from secureli.utilities.usage_stats import post_log, convert_failures_to_failure_count
 from secureli.services.scanner import Failure
 from unittest import mock
@@ -30,18 +32,24 @@ def test_that_convert_failures_to_failure_count_returns_correctly_when_no_failur
 @mock.patch.dict(
     os.environ, {"API_KEY": "", "API_ENDPOINT": "testendpoint"}, clear=True
 )
-def test_that_post_log_return_none_when_no_api_key():
+def test_post_log_with_no_api_key():
     result = post_log("testing")
 
-    assert result == None
+    assert result == PublishLogResult(
+        result=Result.FAILURE,
+        result_message="API_ENDPOINT or API_KEY not found in environment variables",
+    )
 
 
 # pragma: allowlist nextline secret
 @mock.patch.dict(os.environ, {"API_KEY": "testkey", "API_ENDPOINT": ""}, clear=True)
-def test_that_post_log_return_none_when_no_api_endpoint():
+def test_post_log_with_no_api_endpoint():
     result = post_log("testing")
 
-    assert result == None
+    assert result == PublishLogResult(
+        result=Result.FAILURE,
+        result_message="API_ENDPOINT or API_KEY not found in environment variables",
+    )
 
 
 @mock.patch.dict(
@@ -50,10 +58,30 @@ def test_that_post_log_return_none_when_no_api_endpoint():
     clear=True,
 )
 @patch("requests.post")
-def test_that_post_log_return_correctly_when_argument_is_correct(mock_requests):
-    mock_requests.return_value = Mock(status_code=202, text={"requestId": "test-0001"})
+def test_post_log_http_error(mock_requests):
+    mock_requests.side_effect = Exception("test exception")
 
     result = post_log("test_log_data")
 
     mock_requests.assert_called_once()
-    assert result == {"requestId": "test-0001"}
+    assert result == PublishLogResult(
+        result=Result.FAILURE,
+        result_message='Error posting log to testendpoint: "test exception"',
+    )
+
+
+@mock.patch.dict(
+    os.environ,
+    {"API_KEY": "testkey", "API_ENDPOINT": "testendpoint"},  # pragma: allowlist secret
+    clear=True,
+)
+@patch("requests.post")
+def test_post_log_happy_path(mock_requests):
+    mock_requests.return_value = Mock(status_code=202, text="sample-response")
+
+    result = post_log("test_log_data")
+
+    mock_requests.assert_called_once()
+    assert result == PublishLogResult(
+        result=Result.SUCCESS, result_message="sample-response"
+    )