fix: address PR review issues for Login Flow v2
- Fix circular dependency in scope_authorization: auth tools requiring only identity scopes (openid/profile/email) now bypass the login flow provisioning check, so unprovisioned users can call provisioning tools - Fix no-op detection in nc_auth_update_scopes: NULL scopes (legacy "all") now correctly map to ALL_SUPPORTED_SCOPES instead of empty set - Fix get_app_password_with_scopes swallowing exceptions: re-raise instead of returning None, matching sibling methods - Add missing audit logging to update_app_password_scopes, delete_login_flow_session, and delete_expired_login_flow_sessions - Pin setup-uv to v7.3.1 in CI unit-test job (was v7.3.0) - Add FastMCP type annotation to register_auth_tools parameter - Log warning when user accepts elicitation without checking acknowledged box Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -25,7 +25,7 @@ jobs:
|
||||
steps:
|
||||
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
|
||||
- name: Install the latest version of uv
|
||||
uses: astral-sh/setup-uv@eac588ad8def6316056a12d4907a9d4d84ff7a3b # v7.3.0
|
||||
uses: astral-sh/setup-uv@5a095e7a2014a4212f075830d4f7277575a9d098 # v7.3.1
|
||||
- name: Run unit tests
|
||||
run: uv run pytest -v -m unit -o "addopts=-p no:asyncio"
|
||||
|
||||
|
||||
@@ -56,6 +56,11 @@ async def present_login_url(
|
||||
)
|
||||
|
||||
if result.action == "accept":
|
||||
if hasattr(result, "data") and not result.data.acknowledged: # type: ignore[union-attr]
|
||||
logger.warning(
|
||||
"User accepted login flow without checking the acknowledged box — "
|
||||
"login completion will be verified via polling"
|
||||
)
|
||||
logger.info("User acknowledged login flow completion")
|
||||
return "accepted"
|
||||
elif result.action == "decline":
|
||||
|
||||
@@ -14,6 +14,13 @@ from nextcloud_mcp_server.config import get_settings
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Scopes that only assert identity (OIDC standard claims).
|
||||
# Tools requiring *only* these scopes (e.g. auth provisioning tools) must
|
||||
# bypass the Login Flow v2 "is the user provisioned?" check — otherwise the
|
||||
# very tools that *create* app passwords would be blocked for unprovisioned
|
||||
# users, creating a circular dependency.
|
||||
IDENTITY_ONLY_SCOPES: frozenset[str] = frozenset({"openid", "profile", "email"})
|
||||
|
||||
|
||||
class ScopeAuthorizationError(Exception):
|
||||
"""Raised when a request lacks required scopes."""
|
||||
@@ -141,7 +148,9 @@ 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 get_settings().enable_login_flow:
|
||||
if get_settings().enable_login_flow and not set(required_scopes).issubset(
|
||||
IDENTITY_ONLY_SCOPES
|
||||
):
|
||||
from nextcloud_mcp_server.server.oauth_tools import ( # noqa: PLC0415
|
||||
extract_user_id_from_token,
|
||||
)
|
||||
|
||||
@@ -1606,13 +1606,10 @@ class RefreshTokenStorage:
|
||||
"updated_at": updated_at,
|
||||
}
|
||||
|
||||
except Exception as e:
|
||||
except Exception:
|
||||
duration = time.time() - start_time
|
||||
record_db_operation("sqlite", "select", duration, "error")
|
||||
logger.error(
|
||||
f"Failed to retrieve scoped app password for user {user_id}: {e}"
|
||||
)
|
||||
return None
|
||||
raise
|
||||
|
||||
async def update_app_password_scopes(self, user_id: str, scopes: list[str]) -> bool:
|
||||
"""Update only the scopes for an existing app password (no decrypt/re-encrypt).
|
||||
@@ -1641,6 +1638,14 @@ class RefreshTokenStorage:
|
||||
|
||||
duration = time.time() - start_time
|
||||
record_db_operation("sqlite", "update", duration, "success")
|
||||
|
||||
if updated:
|
||||
await self._audit_log(
|
||||
event="update_app_password_scopes",
|
||||
user_id=user_id,
|
||||
auth_method="app_password",
|
||||
)
|
||||
|
||||
return updated
|
||||
|
||||
except Exception:
|
||||
@@ -1803,6 +1808,11 @@ class RefreshTokenStorage:
|
||||
|
||||
if deleted:
|
||||
logger.info(f"Deleted login flow session for user {user_id}")
|
||||
await self._audit_log(
|
||||
event="delete_login_flow_session",
|
||||
user_id=user_id,
|
||||
auth_method="login_flow",
|
||||
)
|
||||
|
||||
return deleted
|
||||
|
||||
@@ -1836,6 +1846,11 @@ class RefreshTokenStorage:
|
||||
|
||||
if count > 0:
|
||||
logger.info(f"Cleaned up {count} expired login flow sessions")
|
||||
await self._audit_log(
|
||||
event="delete_expired_login_flow_sessions",
|
||||
user_id="system",
|
||||
auth_method="login_flow",
|
||||
)
|
||||
|
||||
return count
|
||||
|
||||
|
||||
@@ -9,7 +9,7 @@ tools during the migration period.
|
||||
|
||||
import logging
|
||||
|
||||
from mcp.server.fastmcp import Context
|
||||
from mcp.server.fastmcp import Context, FastMCP
|
||||
from mcp.types import ToolAnnotations
|
||||
|
||||
from nextcloud_mcp_server.auth.elicitation import present_login_url
|
||||
@@ -28,7 +28,7 @@ from nextcloud_mcp_server.server.oauth_tools import extract_user_id_from_token
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def register_auth_tools(mcp) -> None:
|
||||
def register_auth_tools(mcp: FastMCP) -> None:
|
||||
"""Register Login Flow v2 auth tools with the MCP server."""
|
||||
|
||||
@mcp.tool(
|
||||
@@ -381,7 +381,9 @@ def register_auth_tools(mcp) -> None:
|
||||
)
|
||||
|
||||
# No-op detection: skip Login Flow if scopes are unchanged
|
||||
previous_scopes_set = set(previous_scopes) if previous_scopes else set()
|
||||
previous_scopes_set = (
|
||||
set(previous_scopes) if previous_scopes else set(ALL_SUPPORTED_SCOPES)
|
||||
)
|
||||
if set(new_scopes) == previous_scopes_set:
|
||||
return UpdateScopesResponse(
|
||||
status="unchanged",
|
||||
|
||||
Reference in New Issue
Block a user