refactor(auth): remove vestigial token-exchange code path
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) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
b134b2c539
commit
477f9a1ff7
@@ -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.
|
||||
@@ -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"
|
||||
)
|
||||
|
||||
return self._create_access_token(token, payload)
|
||||
|
||||
|
||||
@@ -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"))
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user