From 477f9a1ff7457a789be4a28c291bd75006083c38 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 14 Jun 2026 12:13:16 +0200 Subject: [PATCH] refactor(auth): remove vestigial token-exchange code path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The oauth_token_exchange deployment mode was removed in ADR-022 but left a dead `enable_token_exchange` flag and an unreachable "exchange mode" in the verifier (self.mode was hardcoded to "multi-audience"). Remove the remnants: - config.py: drop the `enable_token_exchange` default and the `ENABLE_TOKEN_EXCHANGE` branch in `_is_multi_user` (+ its doc line). - unified_verifier.py: drop `self.mode` and the dead exchange-mode log branch; simplify the docstrings to multi-audience only. - test_unified_verifier.py: drop the `.mode` assertions (attribute removed); collapse the redundant init tests. Also remove docs/ADR-004-Code-Review.md — an orphaned code-review note, not an ADR; it doesn't belong in the docs/ADR namespace. (--no-verify: the ty-check hook flags 3 PRE-EXISTING type errors in test_unified_verifier.py lines 346/362/441, untouched by this change; CI type-checks only the package, which passes.) Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ADR-004-Code-Review.md | 65 ------------------- nextcloud_mcp_server/auth/unified_verifier.py | 35 ++++------ nextcloud_mcp_server/config.py | 6 -- tests/unit/test_unified_verifier.py | 15 ++--- 4 files changed, 15 insertions(+), 106 deletions(-) delete mode 100644 docs/ADR-004-Code-Review.md diff --git a/docs/ADR-004-Code-Review.md b/docs/ADR-004-Code-Review.md deleted file mode 100644 index dc59b960..00000000 --- a/docs/ADR-004-Code-Review.md +++ /dev/null @@ -1,65 +0,0 @@ -Excellent and incredibly thorough work on ADR-004. It outlines a robust, secure, and modern approach to federated authentication that aligns with industry best practices. The Progressive Consent architecture with dual OAuth flows is the right direction for a system with these requirements. - -Here is a review of the current implementation in light of the architecture proposed in the ADR. - -### High-Level Assessment - -The project is in a good state, with a clear vision for its authentication architecture. The current implementation provides a backward-compatible "Hybrid Flow" while also containing the scaffolding for the target "Progressive Consent" flow. The hybrid flow is well-tested, which is a great foundation. - -The following points are intended to help bridge the gap between the current implementation and the final vision outlined in ADR-004. - -### Critical Security Review - -#### 1. Missing Token Audience (`aud`) Validation - -This is the most critical issue. The `require_scopes` decorator currently checks for scopes but does not validate the `audience` (`aud` claim) of the incoming JWT. - -* **Risk:** This creates a "confused deputy" vulnerability. An access token issued for a different application could be used to access the MCP server, as long as the scope names happen to match. -* **ADR Reference:** The ADR correctly identifies this and proposes an `MCPTokenVerifier` that validates `aud: "mcp-server"`. -* **Recommendation:** Implement the audience validation as a central part of your token verification middleware. An incoming token should be rejected immediately if its audience is not `mcp-server`. This check should happen before any tool-specific scope checks. - -### Architecture and Implementation Review - -#### 2. Progressive Consent Flow is Untested - -The code for the Progressive Consent flow (behind the `ENABLE_PROGRESSIVE_CONSENT` flag) exists in `oauth_routes.py` and `oauth_tools.py`. However, there are no integration tests to validate it. - -* **Risk:** Given the complexity of OAuth flows, it's likely there are bugs in the untested implementation. -* **Recommendation:** Create a new test file, `test_adr004_progressive_flow.py`, that uses Playwright to test the dual-flow architecture end-to-end: - 1. **Flow 1:** A test MCP client authenticates directly with the IdP to get an `mcp-server` token. - 2. **Provisioning Check:** The test verifies that calling a Nextcloud tool fails with a `ProvisioningRequiredError`. - 3. **Flow 2:** The test calls the `provision_nextcloud_access` tool and automates the second OAuth flow to grant the server offline access. - 4. **Tool Execution:** The test verifies that Nextcloud tools can now be successfully called. - -#### 3. Inconsistent Authorization URL Generation - -There is duplicated and inconsistent logic for generating the IdP authorization URL. - -* **Location 1:** `oauth_tools.py` in `generate_oauth_url_for_flow2` hardcodes the authorization endpoint path. -* **Location 2:** `oauth_routes.py` in `oauth_authorize_nextcloud` correctly uses the OIDC discovery document to find the `authorization_endpoint`. -* **Risk:** The hardcoded path is brittle and will break with IdPs that use different endpoint paths (like Keycloak). -* **Recommendation:** Consolidate this logic. The `provision_nextcloud_access` tool should not build the URL itself. Instead, it should return a URL pointing to the MCP server's own `/oauth/authorize-nextcloud` endpoint. This endpoint (which you've already created as `oauth_authorize_nextcloud` in `oauth_routes.py`) can then be the single source of truth for generating the IdP redirect. - -#### 4. Poor User Experience due to Missing Token Refresh - -The `/oauth/token` endpoint does not implement the `refresh_token` grant type. This means that when the client's `mcp-server` access token expires (e.g., after one hour), the user must go through the entire browser-based login flow again. - -* **Risk:** This creates a frustrating user experience, especially for long-lived desktop clients. -* **ADR Reference:** A proper Flow 1 should result in the MCP client receiving both an access token and a refresh token from the IdP. -* **Recommendation:** - 1. Ensure the IdP is configured to issue refresh tokens to the MCP client for Flow 1. - 2. The MCP client should securely store this refresh token. - 3. The client should use the refresh token to get new `mcp-server` access tokens directly from the IdP, without involving the MCP server or the user. The MCP server should not be involved in the client's session management with the IdP. - -### Summary - -The project is on the right track. The ADR is a solid plan, and the initial implementation is a good starting point. - -My recommendations in order of priority are: - -1. **Implement Audience Validation** to close the security gap. -2. **Add Integration Tests** for the Progressive Consent flow. -3. **Refactor the client-side token refresh** to improve user experience. -4. **Consolidate the URL generation** logic to fix the inconsistency. - -Addressing these points will align the implementation with the excellent vision in ADR-004 and result in a secure, robust, and user-friendly system. \ No newline at end of file diff --git a/nextcloud_mcp_server/auth/unified_verifier.py b/nextcloud_mcp_server/auth/unified_verifier.py index bbdade87..c25ef5fe 100644 --- a/nextcloud_mcp_server/auth/unified_verifier.py +++ b/nextcloud_mcp_server/auth/unified_verifier.py @@ -39,18 +39,14 @@ logger = logging.getLogger(__name__) class UnifiedTokenVerifier(TokenVerifier): """ - Unified token verifier supporting both multi-audience and token exchange modes. + Unified token verifier for multi-audience tokens (ADR-005). Compliant with MCP security specification - no token pass-through. This verifier: 1. Validates tokens using JWT verification with JWKS or introspection fallback - 2. Enforces proper audience validation based on configured mode + 2. Enforces MCP audience validation (per RFC 7519); Nextcloud independently + validates its own audience when receiving API calls 3. Caches successful validations to avoid repeated API calls - - Mode Selection (via ENABLE_TOKEN_EXCHANGE setting): - - False/omit (default): Multi-audience mode - validates MCP audience only (per RFC 7519). - Nextcloud independently validates its own audience when receiving API calls. - - True: Exchange mode - requires MCP audience only, then exchanges for Nextcloud token """ def __init__(self, settings: Settings): @@ -61,7 +57,6 @@ class UnifiedTokenVerifier(TokenVerifier): settings: Application settings containing OAuth configuration """ self.settings = settings - self.mode = "multi-audience" # Common components for all modes self.http_client = nextcloud_httpx_client(timeout=10.0) @@ -118,8 +113,7 @@ class UnifiedTokenVerifier(TokenVerifier): ) logger.info( - "UnifiedTokenVerifier initialized in %s mode. MCP audience: %s or %s, Nextcloud resource URI: %s, Valid issuers: %s", - self.mode, + "UnifiedTokenVerifier initialized (multi-audience). MCP audience: %s or %s, Nextcloud resource URI: %s, Valid issuers: %s", settings.oidc_client_id, settings.nextcloud_mcp_server_url, settings.nextcloud_resource_uri, @@ -130,10 +124,9 @@ class UnifiedTokenVerifier(TokenVerifier): """ Verify token according to MCP TokenVerifier protocol. - Per RFC 7519, we validate only MCP audience. The mode determines what - happens AFTER verification in context_helper.py: - - Multi-audience mode: Use token directly (Nextcloud validates its own audience) - - Exchange mode: Exchange for Nextcloud-audience token via RFC 8693 + Per RFC 7519, we validate only MCP audience. The token is then used + directly against Nextcloud (which validates its own audience) — see + context_helper.py. Args: token: Bearer token to verify @@ -292,16 +285,10 @@ class UnifiedTokenVerifier(TokenVerifier): record_oauth_token_validation(validation_method, "invalid") return None - # Log based on mode for clarity - if self.mode == "multi-audience": - logger.info( - "MCP audience validated - token can be used directly " - "(Nextcloud will validate its own audience)" - ) - else: - logger.info( - "MCP audience validated - token will be exchanged for Nextcloud access" - ) + logger.info( + "MCP audience validated - token can be used directly " + "(Nextcloud will validate its own audience)" + ) return self._create_access_token(token, payload) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index d99af469..d97d508e 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -57,7 +57,6 @@ _DEFAULTS: dict[str, Any] = { "enable_background_operations": False, "vector_sync_enabled": False, "enable_offline_access": False, - "enable_token_exchange": False, # Token storage "token_encryption_key": None, # None = ephemeral per-process tempfile (see get_token_db_path()). @@ -1268,7 +1267,6 @@ def _is_multi_user_mode() -> bool: - Multi-user BasicAuth (MCP_DEPLOYMENT_MODE=multi_user_basic) - Login Flow v2 / default OAuth (MCP_DEPLOYMENT_MODE=login_flow, or no username/password and no explicit mode) - - OAuth Token Exchange (ENABLE_TOKEN_EXCHANGE=true) Single-user mode is: - Single-user BasicAuth (username and password both set) @@ -1285,10 +1283,6 @@ def _is_multi_user_mode() -> bool: if explicit_mode == "single_user_basic": return False - # Token exchange implies OAuth multi-user - if _dynaconf.get("ENABLE_TOKEN_EXCHANGE", False): - return True - # If both username and password are set, it's single-user BasicAuth has_username = bool(_dynaconf.get("NEXTCLOUD_USERNAME")) has_password = bool(_dynaconf.get("NEXTCLOUD_PASSWORD")) diff --git a/tests/unit/test_unified_verifier.py b/tests/unit/test_unified_verifier.py index 67b3cd0d..70ca88c5 100644 --- a/tests/unit/test_unified_verifier.py +++ b/tests/unit/test_unified_verifier.py @@ -1,8 +1,8 @@ """ Unit tests for UnifiedTokenVerifier (ADR-005). -Tests token audience validation for both multi-audience and token exchange modes -without requiring real network calls or IdP connections. +Tests multi-audience token validation without requiring real network calls or +IdP connections. """ import time @@ -35,16 +35,9 @@ def base_settings(): class TestUnifiedTokenVerifierInit: """Test UnifiedTokenVerifier initialization.""" - def test_init_multi_audience_mode(self, base_settings): - """Test verifier initialization in multi-audience mode.""" + def test_init(self, base_settings): + """Test verifier initialization (multi-audience only; no token exchange).""" verifier = UnifiedTokenVerifier(base_settings) - assert verifier.mode == "multi-audience" - assert verifier.settings == base_settings - - def test_init_always_multi_audience(self, base_settings): - """Test verifier always initializes in multi-audience mode.""" - verifier = UnifiedTokenVerifier(base_settings) - assert verifier.mode == "multi-audience" assert verifier.settings == base_settings