Chris CoutinhoandClaude Opus 4.7
2ef4bfc4af
Addresses the two remaining 🟡 findings from the PR #758 follow-up review: 1. extract_user_id_from_token previously fell back to "default_user" when the verified access token had no sub claim. In a multi-tenant deployment a malformed IdP token could have bucketed every request under a single sentinel user, risking cross-tenant data exposure. The function now raises McpError on that branch; the BasicAuth no-token sentinel path is preserved. 2. oauth_callback_nextcloud (Flow 2) read the PKCE code_verifier from oauth_sessions but never deleted the row, leaving the verifier valid for the full 10-minute TTL. The row is now deleted eagerly inside the same branch, mirroring oauth_login_callback in browser_oauth_routes. Also wires TOKEN_ENCRYPTION_KEY through the docker-compose step in the CI test workflow so the integration matrix can boot — every job had been failing fast on the ${TOKEN_ENCRYPTION_KEY:?...} interpolation guard added in PR #758 finding 5. Tests pin both fixes (test_token_utils_user_id.py, test_oauth_callback_session_cleanup.py). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
84 lines
2.8 KiB
Python
84 lines
2.8 KiB
Python
"""Unit tests for ``extract_user_id_from_token`` (PR #758 follow-up review).
|
|
|
|
The function used to silently fall back to ``"default_user"`` whenever the
|
|
verified access token had no ``sub`` claim. In a multi-tenant deployment
|
|
that would let a malformed IdP token bucket every request under a single
|
|
sentinel user, risking cross-tenant data exposure. The fix is to keep the
|
|
no-token fallback (BasicAuth mode legitimately calls this without an
|
|
OAuth identity) but raise ``McpError`` whenever an access token is
|
|
present and ``resource`` is empty.
|
|
"""
|
|
|
|
import time
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
import pytest
|
|
from mcp.server.auth.provider import AccessToken
|
|
from mcp.shared.exceptions import McpError
|
|
|
|
from nextcloud_mcp_server.auth.token_utils import extract_user_id_from_token
|
|
|
|
pytestmark = pytest.mark.unit
|
|
|
|
|
|
def _token(resource: str | None = "alice") -> AccessToken:
|
|
return AccessToken(
|
|
token="t",
|
|
client_id="test-client",
|
|
scopes=["openid"],
|
|
expires_at=int(time.time() + 3600),
|
|
resource=resource,
|
|
)
|
|
|
|
|
|
async def test_returns_user_id_when_token_has_sub():
|
|
"""Happy path: verified access token with sub → returns the sub."""
|
|
with patch(
|
|
"nextcloud_mcp_server.auth.token_utils.get_access_token",
|
|
return_value=_token("alice"),
|
|
):
|
|
user_id = await extract_user_id_from_token(MagicMock())
|
|
|
|
assert user_id == "alice"
|
|
|
|
|
|
async def test_returns_default_user_when_no_access_token():
|
|
"""BasicAuth mode: get_access_token() returns None → sentinel.
|
|
|
|
BasicAuth deployments don't issue OAuth tokens; the sentinel lets
|
|
BasicAuth-aware callers branch on it. Removing this fallback would
|
|
break the BasicAuth path.
|
|
"""
|
|
with patch(
|
|
"nextcloud_mcp_server.auth.token_utils.get_access_token",
|
|
return_value=None,
|
|
):
|
|
user_id = await extract_user_id_from_token(MagicMock())
|
|
|
|
assert user_id == "default_user"
|
|
|
|
|
|
async def test_raises_when_token_present_but_resource_empty():
|
|
"""Token present but ``resource`` empty → fail closed with McpError.
|
|
|
|
Pins the PR #758 follow-up review fix: a malformed IdP token must
|
|
not silently funnel users into a shared ``"default_user"`` SQLite
|
|
bucket.
|
|
"""
|
|
with patch(
|
|
"nextcloud_mcp_server.auth.token_utils.get_access_token",
|
|
return_value=_token(""),
|
|
):
|
|
with pytest.raises(McpError, match="Cannot determine user identity"):
|
|
await extract_user_id_from_token(MagicMock())
|
|
|
|
|
|
async def test_raises_when_resource_is_none():
|
|
"""Same fail-closed behaviour when ``resource`` is None rather than ''."""
|
|
with patch(
|
|
"nextcloud_mcp_server.auth.token_utils.get_access_token",
|
|
return_value=_token(None),
|
|
):
|
|
with pytest.raises(McpError, match="Cannot determine user identity"):
|
|
await extract_user_id_from_token(MagicMock())
|