fix: address PR #589 review feedback for Login Flow v2
- Fix data loss in nc_auth_update_scopes: remove premature delete_app_password call; old password stays valid until upsert replaces it on successful re-provisioning - Replace assert with proper error return in nc_auth_check_status - Add lazy singleton for RefreshTokenStorage in auth_tools, scope_authorization, and context to avoid per-call re-initialization - Centralize _is_login_flow_mode() to get_settings().enable_login_flow and remove duplicate definitions and per-call os.getenv reads - Add dev-only comment to TOKEN_ENCRYPTION_KEY in docker-compose.yml - Gate OIDC build steps in CI behind matrix.needs-playwright - Add diagnostic step reporting Playwright skip count in CI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
@@ -1,7 +1,6 @@
|
||||
"""Scope-based authorization for MCP tools."""
|
||||
|
||||
import logging
|
||||
import os
|
||||
from functools import wraps
|
||||
from typing import Any, Callable
|
||||
|
||||
@@ -10,6 +9,7 @@ from mcp.server.auth.provider import AccessToken
|
||||
from mcp.server.fastmcp import Context
|
||||
from mcp.server.fastmcp.utilities.context_injection import find_context_parameter
|
||||
|
||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
||||
from nextcloud_mcp_server.config import get_settings
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -123,9 +123,8 @@ def require_scopes(*required_scopes: str):
|
||||
if access_token is None:
|
||||
# Check if single-user BasicAuth mode (env var app password)
|
||||
# If NEXTCLOUD_APP_PASSWORD or NEXTCLOUD_PASSWORD is set, bypass scope checks
|
||||
if os.getenv("NEXTCLOUD_APP_PASSWORD") or os.getenv(
|
||||
"NEXTCLOUD_PASSWORD"
|
||||
):
|
||||
settings = get_settings()
|
||||
if settings.nextcloud_app_password or settings.nextcloud_password:
|
||||
logger.debug(
|
||||
f"No access token for {func_name} - allowing (env var app password)"
|
||||
)
|
||||
@@ -142,7 +141,7 @@ def require_scopes(*required_scopes: str):
|
||||
# In Login Flow v2 multi-user mode, OAuth tokens provide MCP session
|
||||
# identity only. Nextcloud API access uses stored app passwords.
|
||||
# Check if the user has a stored app password with appropriate scopes.
|
||||
if _is_login_flow_mode():
|
||||
if get_settings().enable_login_flow:
|
||||
from nextcloud_mcp_server.server.oauth_tools import ( # noqa: PLC0415
|
||||
extract_user_id_from_token,
|
||||
)
|
||||
@@ -479,19 +478,16 @@ def discover_all_scopes(mcp) -> list[str]:
|
||||
# ── Login Flow v2 helpers ────────────────────────────────────────────────
|
||||
|
||||
|
||||
def _is_login_flow_mode() -> bool:
|
||||
"""Check if server is configured for Login Flow v2 multi-user mode.
|
||||
_scope_storage_instance = None
|
||||
|
||||
Login Flow v2 mode is active when:
|
||||
- ENABLE_LOGIN_FLOW=true is set, OR
|
||||
- Multi-user BasicAuth with offline access (uses stored app passwords)
|
||||
|
||||
Returns:
|
||||
True if Login Flow v2 enforcement should be active
|
||||
"""
|
||||
if os.getenv("ENABLE_LOGIN_FLOW", "false").lower() == "true":
|
||||
return True
|
||||
return False
|
||||
async def _get_scope_storage():
|
||||
"""Get initialized storage instance for scope checks (lazy singleton)."""
|
||||
global _scope_storage_instance
|
||||
if _scope_storage_instance is None:
|
||||
_scope_storage_instance = RefreshTokenStorage.from_env()
|
||||
await _scope_storage_instance.initialize()
|
||||
return _scope_storage_instance
|
||||
|
||||
|
||||
async def _get_stored_scopes(user_id: str) -> list[str] | str | None:
|
||||
@@ -502,11 +498,8 @@ async def _get_stored_scopes(user_id: str) -> list[str] | str | None:
|
||||
- "all": NULL scopes in DB (legacy = all allowed)
|
||||
- None: No stored app password (provisioning required)
|
||||
"""
|
||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage # noqa: PLC0415
|
||||
|
||||
try:
|
||||
storage = RefreshTokenStorage.from_env()
|
||||
await storage.initialize()
|
||||
storage = await _get_scope_storage()
|
||||
|
||||
data = await storage.get_app_password_with_scopes(user_id)
|
||||
if data is None:
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
"""Helper functions for accessing context in MCP tools."""
|
||||
|
||||
import logging
|
||||
import os
|
||||
|
||||
from httpx import BasicAuth
|
||||
from mcp.server.fastmcp import Context
|
||||
@@ -11,6 +10,7 @@ from nextcloud_mcp_server.auth.context_helper import (
|
||||
get_session_client_from_context,
|
||||
)
|
||||
from nextcloud_mcp_server.auth.scope_authorization import ProvisioningRequiredError
|
||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
||||
from nextcloud_mcp_server.client import NextcloudClient
|
||||
from nextcloud_mcp_server.config import (
|
||||
DeploymentMode,
|
||||
@@ -82,7 +82,7 @@ async def get_client(ctx: Context) -> NextcloudClient:
|
||||
|
||||
# Login Flow v2 multi-user mode: app password is REQUIRED for NC API access
|
||||
# OAuth token is only used for MCP session identity, not NC API calls
|
||||
if hasattr(lifespan_ctx, "nextcloud_host") and _is_login_flow_mode():
|
||||
if hasattr(lifespan_ctx, "nextcloud_host") and settings.enable_login_flow:
|
||||
return await _get_client_from_login_flow(ctx, lifespan_ctx.nextcloud_host)
|
||||
|
||||
# BasicAuth mode - use shared client (no token exchange)
|
||||
@@ -254,9 +254,16 @@ def _get_client_from_basic_auth(ctx: Context) -> NextcloudClient:
|
||||
)
|
||||
|
||||
|
||||
def _is_login_flow_mode() -> bool:
|
||||
"""Check if Login Flow v2 multi-user mode is active."""
|
||||
return os.getenv("ENABLE_LOGIN_FLOW", "false").lower() == "true"
|
||||
_login_flow_storage_instance = None
|
||||
|
||||
|
||||
async def _get_login_flow_storage():
|
||||
"""Get initialized storage instance for login flow (lazy singleton)."""
|
||||
global _login_flow_storage_instance
|
||||
if _login_flow_storage_instance is None:
|
||||
_login_flow_storage_instance = RefreshTokenStorage.from_env()
|
||||
await _login_flow_storage_instance.initialize()
|
||||
return _login_flow_storage_instance
|
||||
|
||||
|
||||
async def _get_client_from_login_flow(
|
||||
@@ -277,7 +284,6 @@ async def _get_client_from_login_flow(
|
||||
Raises:
|
||||
ProvisioningRequiredError: If no stored app password exists
|
||||
"""
|
||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage # noqa: PLC0415
|
||||
from nextcloud_mcp_server.server.oauth_tools import ( # noqa: PLC0415
|
||||
extract_user_id_from_token,
|
||||
)
|
||||
@@ -288,8 +294,7 @@ async def _get_client_from_login_flow(
|
||||
"Cannot determine user identity from MCP token."
|
||||
)
|
||||
|
||||
storage = RefreshTokenStorage.from_env()
|
||||
await storage.initialize()
|
||||
storage = await _get_login_flow_storage()
|
||||
|
||||
app_data = await storage.get_app_password_with_scopes(user_id)
|
||||
if not app_data:
|
||||
|
||||
@@ -28,11 +28,16 @@ from nextcloud_mcp_server.server.oauth_tools import extract_user_id_from_token
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
_storage_instance: RefreshTokenStorage | None = None
|
||||
|
||||
|
||||
async def _get_storage() -> RefreshTokenStorage:
|
||||
"""Get initialized storage instance."""
|
||||
storage = RefreshTokenStorage.from_env()
|
||||
await storage.initialize()
|
||||
return storage
|
||||
"""Get initialized storage instance (lazy singleton)."""
|
||||
global _storage_instance
|
||||
if _storage_instance is None:
|
||||
_storage_instance = RefreshTokenStorage.from_env()
|
||||
await _storage_instance.initialize()
|
||||
return _storage_instance
|
||||
|
||||
|
||||
def register_auth_tools(mcp) -> None:
|
||||
@@ -241,7 +246,12 @@ def register_auth_tools(mcp) -> None:
|
||||
|
||||
if poll_result.status == "completed":
|
||||
# Store the app password with scopes
|
||||
assert poll_result.app_password is not None
|
||||
if poll_result.app_password is None:
|
||||
return ProvisionStatusResponse(
|
||||
status="error",
|
||||
message="Login Flow completed but no app password was returned.",
|
||||
success=False,
|
||||
)
|
||||
await storage.store_app_password_with_scopes(
|
||||
user_id=user_id,
|
||||
app_password=poll_result.app_password,
|
||||
@@ -287,8 +297,8 @@ def register_auth_tools(mcp) -> None:
|
||||
title="Update Nextcloud Access Scopes",
|
||||
description=(
|
||||
"Update the scopes for your Nextcloud access. "
|
||||
"This revokes the current app password and starts a new Login Flow "
|
||||
"with the combined scope set."
|
||||
"This starts a new Login Flow with the combined scope set. "
|
||||
"The current app password remains valid until the new one is obtained."
|
||||
),
|
||||
annotations=ToolAnnotations(
|
||||
idempotentHint=False,
|
||||
@@ -357,11 +367,10 @@ def register_auth_tools(mcp) -> None:
|
||||
success=False,
|
||||
)
|
||||
|
||||
# Delete existing app password from storage (user must revoke in NC Security settings)
|
||||
if existing:
|
||||
await storage.delete_app_password(user_id)
|
||||
|
||||
# Initiate new Login Flow v2
|
||||
# Note: existing app password stays valid until the new flow completes.
|
||||
# store_app_password_with_scopes() does an upsert, so the old password
|
||||
# is replaced atomically when nc_auth_check_status stores the new one.
|
||||
settings = get_settings()
|
||||
nextcloud_host = settings.nextcloud_host
|
||||
if not nextcloud_host:
|
||||
|
||||
Reference in New Issue
Block a user