Merge pull request #1602 from billtarr-aws/fix/egress-configure-custom-provider-flags

fix(cli): forward the custom-OIDC fields so egress-configure --provider custom works

Author: omrishiv

api/registry_client.py

@@ -3883,6 +3883,11 @@ def configure_egress_auth(
         client_secret: str | None = None,
         scopes: list[str] | None = None,
         target_audience: str | None = None,
+        custom_authorize_url: str | None = None,
+        custom_token_url: str | None = None,
+        custom_scope_separator: str | None = None,
+        custom_token_auth_style: str | None = None,
+        custom_resource: str | None = None,
     ) -> dict[str, Any]:
         """Configure per-user egress auth on a server (admin only).
 
@@ -3898,6 +3903,18 @@ def configure_egress_auth(
             client_secret: OAuth client secret (oauth_user only, write-only).
             scopes: Optional list of OAuth scopes.
             target_audience: Target audience (obo_exchange only).
+            custom_authorize_url: Authorize endpoint. REQUIRED when
+                ``provider="custom"`` (server-side ``resolve_provider`` rejects
+                the config without it).
+            custom_token_url: Token endpoint. REQUIRED when
+                ``provider="custom"``.
+            custom_scope_separator: Scope delimiter when the provider does not
+                use a space (custom only).
+            custom_token_auth_style: Where the client secret goes on the token
+                request -- "post_body" (default) or "basic_header" (custom only).
+            custom_resource: RFC 8707 resource indicator, sent on both the
+                authorize and token requests to bind the token to one protected
+                resource (custom only).
 
         Returns:
             Non-secret egress config view dict.
@@ -3920,6 +3937,19 @@ def configure_egress_auth(
             body["scopes"] = scopes
         if target_audience is not None:
             body["target_audience"] = target_audience
+        # Custom-OIDC overrides. Only meaningful when provider == "custom"; the
+        # server ignores them for built-in providers, so they are forwarded
+        # unconditionally like the fields above rather than gated here.
+        if custom_authorize_url is not None:
+            body["custom_authorize_url"] = custom_authorize_url
+        if custom_token_url is not None:
+            body["custom_token_url"] = custom_token_url
+        if custom_scope_separator is not None:
+            body["custom_scope_separator"] = custom_scope_separator
+        if custom_token_auth_style is not None:
+            body["custom_token_auth_style"] = custom_token_auth_style
+        if custom_resource is not None:
+            body["custom_resource"] = custom_resource
 
         response = self._make_request(
             method="POST",

api/registry_management.py

@@ -2035,6 +2035,11 @@ def cmd_egress_configure(args: argparse.Namespace) -> int:
             client_secret=args.client_secret,
             scopes=_parse_scopes(args.scopes),
             target_audience=args.target_audience,
+            custom_authorize_url=args.custom_authorize_url,
+            custom_token_url=args.custom_token_url,
+            custom_scope_separator=args.custom_scope_separator,
+            custom_token_auth_style=args.custom_token_auth_style,
+            custom_resource=args.custom_resource,
         )
         print(json.dumps(response, indent=2, default=str))
         return 0
@@ -6730,6 +6735,30 @@ def main() -> int:
     egress_configure_parser.add_argument(
         "--target-audience", help="Target audience (obo_exchange only)"
     )
+    # Custom-OIDC provider overrides. --provider custom is unusable without the
+    # two URLs: the server's resolve_provider() rejects the config with
+    # "custom requires custom_authorize_url and custom_token_url".
+    egress_configure_parser.add_argument(
+        "--custom-authorize-url",
+        help="Authorize endpoint (REQUIRED with --provider custom)",
+    )
+    egress_configure_parser.add_argument(
+        "--custom-token-url",
+        help="Token endpoint (REQUIRED with --provider custom)",
+    )
+    egress_configure_parser.add_argument(
+        "--custom-scope-separator",
+        help="Scope delimiter when the provider does not use a space (custom only)",
+    )
+    egress_configure_parser.add_argument(
+        "--custom-token-auth-style",
+        choices=["post_body", "basic_header"],
+        help="Where the client secret goes on the token request (custom only, default post_body)",
+    )
+    egress_configure_parser.add_argument(
+        "--custom-resource",
+        help="RFC 8707 resource indicator, sent on authorize and token requests (custom only)",
+    )
 
     # Get egress config command
     egress_config_get_parser = subparsers.add_parser(

tests/unit/api/test_registry_client_egress_custom.py

@@ -0,0 +1,219 @@
+"""Unit tests for the custom-OIDC fields on egress-auth configuration.
+
+``POST /api/servers/{path}/egress-auth`` accepts five ``custom_*`` fields, and the
+server's ``resolve_provider`` REQUIRES two of them when ``provider="custom"``
+(rejecting the config with "custom requires custom_authorize_url and
+custom_token_url"). The frontend server-edit modal already exposes the URLs, but
+``RegistryClient.configure_egress_auth`` and the ``egress-configure`` CLI command
+did not forward any of them -- so ``--provider custom`` was accepted by argparse
+and then always failed server-side.
+
+These lock in that the fields reach the request body, and that the CLI parses the
+corresponding flags, so the CLI/UI parity gap cannot silently reopen.
+"""
+
+import sys
+from pathlib import Path
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from api.registry_client import RegistryClient
+
+# registry_management.py lives under api/ and imports sibling modules by name.
+_API_DIR = Path(__file__).resolve().parents[3] / "api"
+sys.path.insert(0, str(_API_DIR))
+
+SALESFORCE_AUTHORIZE = "https://example.my.salesforce.com/services/oauth2/authorize"
+# nosec B105 - test URL, not a credential
+SALESFORCE_TOKEN = "https://example.my.salesforce.com/services/oauth2/token"
+
+
+@pytest.fixture
+def client() -> RegistryClient:
+    """A RegistryClient pointed at a dummy URL with a dummy token."""
+    return RegistryClient(registry_url="http://localhost", token="dummy-token-1234567890")
+
+
+def _sent_body(mock_req: MagicMock) -> dict:
+    """The JSON body passed to the mocked _make_request."""
+    return mock_req.call_args.kwargs["data"]
+
+
+class TestConfigureEgressAuthCustomFields:
+    """Tests for configure_egress_auth custom-OIDC passthrough."""
+
+    def test_custom_urls_reach_the_request_body(
+        self,
+        client: RegistryClient,
+    ) -> None:
+        """The two REQUIRED custom URLs are forwarded for provider=custom.
+
+        Without these the server rejects every provider=custom config, which is
+        the bug this guards.
+        """
+        response = MagicMock()
+        response.json.return_value = {"path": "/sf", "egress_provider": "custom"}
+
+        with patch.object(client, "_make_request", return_value=response) as mock_req:
+            client.configure_egress_auth(
+                server_path="/sf",
+                mode="oauth_user",
+                provider="custom",
+                client_id="cid",
+                client_secret="csec",  # nosec B106 - dummy test value
+                scopes=["mcp_api", "refresh_token"],
+                custom_authorize_url=SALESFORCE_AUTHORIZE,
+                custom_token_url=SALESFORCE_TOKEN,
+            )
+
+        body = _sent_body(mock_req)
+        assert body["custom_authorize_url"] == SALESFORCE_AUTHORIZE
+        assert body["custom_token_url"] == SALESFORCE_TOKEN
+        # The pre-existing fields must still be sent.
+        assert body["egress_auth_mode"] == "oauth_user"
+        assert body["egress_provider"] == "custom"
+        assert body["scopes"] == ["mcp_api", "refresh_token"]
+
+    def test_all_five_custom_fields_are_forwarded(
+        self,
+        client: RegistryClient,
+    ) -> None:
+        """Every custom_* field the API accepts is passed through."""
+        response = MagicMock()
+        response.json.return_value = {}
+
+        with patch.object(client, "_make_request", return_value=response) as mock_req:
+            client.configure_egress_auth(
+                server_path="/sf",
+                mode="oauth_user",
+                provider="custom",
+                custom_authorize_url=SALESFORCE_AUTHORIZE,
+                custom_token_url=SALESFORCE_TOKEN,
+                custom_scope_separator=",",
+                custom_token_auth_style="basic_header",
+                custom_resource="https://api.example.com/mcp",
+            )
+
+        body = _sent_body(mock_req)
+        assert body["custom_scope_separator"] == ","
+        assert body["custom_token_auth_style"] == "basic_header"
+        assert body["custom_resource"] == "https://api.example.com/mcp"
+
+    def test_custom_fields_omitted_when_not_supplied(
+        self,
+        client: RegistryClient,
+    ) -> None:
+        """A built-in provider config carries no custom_* keys.
+
+        The fields follow the existing ``is not None`` convention, so omitting
+        them must not send nulls that could overwrite stored values on edit.
+        """
+        response = MagicMock()
+        response.json.return_value = {}
+
+        with patch.object(client, "_make_request", return_value=response) as mock_req:
+            client.configure_egress_auth(
+                server_path="/github",
+                mode="oauth_user",
+                provider="github",
+                client_id="cid",
+            )
+
+        body = _sent_body(mock_req)
+        assert not [k for k in body if k.startswith("custom_")]
+
+    def test_endpoint_and_method_unchanged(
+        self,
+        client: RegistryClient,
+    ) -> None:
+        """The custom fields do not alter the request target."""
+        response = MagicMock()
+        response.json.return_value = {}
+
+        with patch.object(client, "_make_request", return_value=response) as mock_req:
+            client.configure_egress_auth(
+                server_path="/sf",
+                mode="oauth_user",
+                provider="custom",
+                custom_authorize_url=SALESFORCE_AUTHORIZE,
+                custom_token_url=SALESFORCE_TOKEN,
+            )
+
+        kwargs = mock_req.call_args.kwargs
+        assert kwargs["method"] == "POST"
+        assert kwargs["endpoint"] == "/api/servers/sf/egress-auth"
+
+
+class TestEgressConfigureCommandWiring:
+    """Tests that cmd_egress_configure forwards the custom-OIDC args.
+
+    The parser is built inside ``main()``, so these exercise the command handler
+    directly with a SimpleNamespace (the convention used by
+    test_registry_management_m2m_secret.py). That covers the wiring this change
+    adds: args -> client kwargs.
+    """
+
+    @staticmethod
+    def _args(**overrides) -> SimpleNamespace:
+        """Args as argparse would produce them, with custom_* defaulting to None."""
+        base = {
+            "path": "/sf",
+            "mode": "oauth_user",
+            "provider": "custom",
+            "client_id": "cid",
+            "client_secret": "csec",
+            "scopes": "mcp_api,refresh_token",
+            "target_audience": None,
+            "custom_authorize_url": None,
+            "custom_token_url": None,
+            "custom_scope_separator": None,
+            "custom_token_auth_style": None,
+            "custom_resource": None,
+        }
+        base.update(overrides)
+        return SimpleNamespace(**base)
+
+    def test_command_forwards_custom_args_to_client(self) -> None:
+        """Every custom_* arg reaches configure_egress_auth as a kwarg."""
+        import registry_management
+
+        mock_client = MagicMock()
+        mock_client.configure_egress_auth.return_value = {"path": "/sf"}
+        args = self._args(
+            custom_authorize_url=SALESFORCE_AUTHORIZE,
+            custom_token_url=SALESFORCE_TOKEN,
+            custom_scope_separator=",",
+            custom_token_auth_style="basic_header",
+            custom_resource="https://api.example.com/mcp",
+        )
+
+        with patch.object(registry_management, "_create_client", return_value=mock_client):
+            rc = registry_management.cmd_egress_configure(args)
+
+        assert rc == 0
+        kwargs = mock_client.configure_egress_auth.call_args.kwargs
+        assert kwargs["custom_authorize_url"] == SALESFORCE_AUTHORIZE
+        assert kwargs["custom_token_url"] == SALESFORCE_TOKEN
+        assert kwargs["custom_scope_separator"] == ","
+        assert kwargs["custom_token_auth_style"] == "basic_header"
+        assert kwargs["custom_resource"] == "https://api.example.com/mcp"
+        # Pre-existing args must still be forwarded.
+        assert kwargs["mode"] == "oauth_user"
+        assert kwargs["provider"] == "custom"
+        assert kwargs["scopes"] == ["mcp_api", "refresh_token"]
+
+    def test_omitted_custom_args_forward_as_none(self) -> None:
+        """Unset flags pass None, which the client then skips entirely."""
+        import registry_management
+
+        mock_client = MagicMock()
+        mock_client.configure_egress_auth.return_value = {}
+
+        with patch.object(registry_management, "_create_client", return_value=mock_client):
+            rc = registry_management.cmd_egress_configure(self._args(provider="github"))
+
+        assert rc == 0
+        kwargs = mock_client.configure_egress_auth.call_args.kwargs
+        assert all(kwargs[k] is None for k in kwargs if k.startswith("custom_"))