nextcloud-mcp-server/docs/ADR-004-Code-Review.md

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.