refactor: implement review feedback - simplify SSL verification config

Implement all review feedback from @agateau-gg:

- Remove allow_self_signed field from UserConfig, keep only insecure
- Convert allow_self_signed to insecure during config loading
- Use ui.display_warning() directly instead of deprecation_messages
- Shorten all deprecation warning messages
- Reorder --insecure help text (functionality before warning)
- Update v1 config converter to map allow_self_signed to insecure
- Simplify code by eliminating dual field state

This makes the code cleaner and prevents inconsistent state where
allow_self_signed and insecure could have different values.

Issue: SCRT-5952

Author: guischguardian

ggshield/__main__.py

@@ -81,7 +81,6 @@ def exit_code(ctx: click.Context, exit_code: int, **kwargs: Any) -> int:
 def cli(
     ctx: click.Context,
     *,
-    allow_self_signed: Optional[bool],
     insecure: Optional[bool],
     config_path: Optional[Path],
     **kwargs: Any,
@@ -99,14 +98,10 @@ def cli(
         ensure_level(ui.Level.VERBOSE)
 
     # Update SSL verification settings in the config
-    # Both --allow-self-signed and --insecure disable SSL verification
-    # If either is set, we enable both for backward compatibility
     # TODO: this should be reworked: if a command which writes the config is called with
-    # --allow-self-signed or --insecure, the config will contain the enabled flag.
-    if allow_self_signed or insecure:
-        # Set both flags to maintain consistency
-        user_config.allow_self_signed = True
-        user_config.insecure = True
+    # --insecure, the config will contain `insecure: true`.
+    if insecure:
+        user_config.insecure = insecure
 
     ctx_obj.config._dotenv_vars = load_dot_env()
 

ggshield/cmd/utils/common_options.py

@@ -141,26 +141,24 @@ def allow_self_signed_callback(
 ) -> Optional[bool]:
     if value:
         ui.display_warning(
-            "The --allow-self-signed option is deprecated and will be removed in a future version. "
-            "Use --insecure instead, which more accurately describes what this option does "
-            "(it disables all certificate validation, not just allows self-signed certificates)."
+            "The --allow-self-signed option is deprecated. Use --insecure instead."
         )
-    return create_config_callback("allow_self_signed")(ctx, param, value)
+    return create_config_callback("insecure")(ctx, param, value)
 
 
 _allow_self_signed_option = click.option(
     "--allow-self-signed",
     is_flag=True,
     default=None,
-    help="(Deprecated: use --insecure) Ignore ssl verification.",
+    help="Deprecated: use --insecure.",
     callback=allow_self_signed_callback,
 )
 
 _insecure_option = click.option(
     "--insecure",
     is_flag=True,
     default=None,
-    help="WARNING: using this option makes the transfer insecure, by skipping all certificate's validation checks.",
+    help="Skip all certificate verification checks. WARNING: this option makes the transfer insecure.",
     callback=create_config_callback("insecure"),
 )
 

ggshield/core/client.py

@@ -51,8 +51,7 @@ def create_client_from_config(config: Config) -> GGClient:
     return create_client(
         api_key,
         api_url,
-        allow_self_signed=config.user_config.allow_self_signed
-        or config.user_config.insecure,
+        allow_self_signed=config.user_config.insecure,
         callbacks=callbacks,
     )
 

ggshield/core/config/user_config.py

@@ -87,7 +87,6 @@ class UserConfig(FilteredConfig):
     instance: Optional[str] = None
     exit_zero: bool = False
     verbose: bool = False
-    allow_self_signed: bool = False
     insecure: bool = False
     max_commits_for_hook: int = 50
     secret: SecretConfig = field(default_factory=SecretConfig)
@@ -199,7 +198,7 @@ def _load_config_dict(
         if dash_keys:
             _warn_about_dash_keys(config_path, dash_keys)
         _fix_ignore_known_secrets(dct)
-        _check_allow_self_signed_deprecation(dct, config_path, deprecation_messages)
+        _fix_allow_self_signed(dct, config_path)
     elif config_version == 1:
         deprecation_messages.append(
             f"{config_path} uses a deprecated configuration file format."
@@ -228,16 +227,14 @@ def _fix_ignore_known_secrets(data: Dict[str, Any]) -> None:
     secret_dct[_IGNORE_KNOWN_SECRETS_KEY] = value
 
 
-def _check_allow_self_signed_deprecation(
-    data: Dict[str, Any], config_path: Path, deprecation_messages: List[str]
-) -> None:
-    """Check if allow_self_signed is used and add a deprecation warning."""
-    if data.get("allow_self_signed"):
-        deprecation_messages.append(
-            f"{config_path}: The 'allow_self_signed' option is deprecated and will be removed in a future version. "
-            "Use 'insecure: true' instead, which more accurately describes what this option does "
-            "(it disables all certificate validation, not just allows self-signed certificates)."
+def _fix_allow_self_signed(data: Dict[str, Any], config_path: Path) -> None:
+    """Convert allow_self_signed to insecure and display a deprecation warning."""
+    if insecure := data.pop("allow_self_signed", None):
+        ui.display_warning(
+            f"{config_path}: The 'allow_self_signed' option is deprecated. "
+            "Use 'insecure: true' instead."
         )
+        data["insecure"] = insecure
 
 
 def _warn_about_dash_keys(config_path: Path, dash_keys: Set[str]) -> None:

ggshield/core/config/v1_config.py

@@ -60,7 +60,7 @@ def copy_if_set(dct: Dict[str, Any], dst_key: str, src_key: Optional[str] = None
     copy_if_set(dct, "instance")
     copy_if_set(dct, "exit_zero")
     copy_if_set(dct, "verbose")
-    copy_if_set(dct, "allow_self_signed")
+    copy_if_set(dct, "insecure", "allow_self_signed")
     copy_if_set(dct, "max_commits_for_hook")
 
     return dct

tests/unit/cmd/scan/test_path.py

@@ -236,18 +236,6 @@ def test_instance_option(self, cli_fs_runner):
 
     @pytest.mark.parametrize("position", [0, 1, 2, 3, 4])
     def test_ssl_verify(self, cli_fs_runner, position):
-        self.create_files()
-
-        cmd = ["secret", "scan", "path", "file1"]
-        cmd.insert(position, "--allow-self-signed")
-
-        with patch("ggshield.core.client.GGClient") as client_mock:
-            cli_fs_runner.invoke(cli, cmd)
-            _, kwargs = client_mock.call_args
-            assert kwargs["session"].verify is False
-
-    @pytest.mark.parametrize("position", [0, 1, 2, 3, 4])
-    def test_ssl_verify_insecure(self, cli_fs_runner, position):
         """
         GIVEN the --insecure flag
         WHEN running the path scan command

tests/unit/cmd/test_status.py

@@ -122,16 +122,6 @@ def get_api_status(env, instance=None):
 
 @pytest.mark.parametrize("verify", [True, False])
 def test_ssl_verify(cli_fs_runner, verify):
-    cmd = ["api-status"] if verify else ["--allow-self-signed", "api-status"]
-
-    with mock.patch("ggshield.core.client.GGClient") as client_mock:
-        cli_fs_runner.invoke(cli, cmd)
-        _, kwargs = client_mock.call_args
-        assert kwargs["session"].verify == verify
-
-
-@pytest.mark.parametrize("verify", [True, False])
-def test_ssl_verify_insecure(cli_fs_runner, verify):
     """
     GIVEN the --insecure flag
     WHEN running the api-status command

tests/unit/core/config/test_user_config.py

@@ -294,11 +294,11 @@ def test_can_load_ignored_known_secrets_from_secret(self, local_config_path):
         config, _ = UserConfig.load(local_config_path)
         assert config.secret.ignore_known_secrets
 
-    def test_allow_self_signed_deprecation_warning(self, local_config_path):
+    def test_allow_self_signed_deprecation_warning(self, local_config_path, capsys):
         """
         GIVEN a config file containing allow_self_signed: true
         WHEN loading the config
-        THEN a deprecation warning is added to deprecation_messages
+        THEN it is converted to insecure and a deprecation warning is displayed
         """
         write_yaml(
             local_config_path,
@@ -308,11 +308,11 @@ def test_allow_self_signed_deprecation_warning(self, local_config_path):
             },
         )
         config, _ = UserConfig.load(local_config_path)
-        assert config.allow_self_signed is True
-        assert len(config.deprecation_messages) == 1
-        assert "allow_self_signed" in config.deprecation_messages[0]
-        assert "deprecated" in config.deprecation_messages[0]
-        assert "insecure" in config.deprecation_messages[0]
+        assert config.insecure is True
+        captured = capsys.readouterr()
+        assert "allow_self_signed" in captured.err
+        assert "deprecated" in captured.err
+        assert "insecure" in captured.err
 
     def test_bad_local_config(self, local_config_path, global_config_path):
         """