fix: address second round of PR review for scope prefix
- Use dynaconf (get_settings()) instead of os.getenv for OIDC_RESOURCE_SERVER_ID
- Re-add Settings field, _field_map entry, and settings.toml default
- Add trailing-slash guard (.rstrip("/")) to prevent double-slash in scopes
- Add double-prefixing guard: skip scopes already carrying the prefix
- Add @pytest.mark.unit to test module
- Add test for already-prefixed scopes
- Document OIDC_RESOURCE_SERVER_ID in docs/configuration.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
f67d4d1116
commit
cc6ba65993
@@ -123,6 +123,7 @@ NEXTCLOUD_PASSWORD=
|
|||||||
| `NEXTCLOUD_OIDC_CLIENT_ID` | ⚠️ Optional | - | OAuth client ID (auto-registers if empty) |
|
| `NEXTCLOUD_OIDC_CLIENT_ID` | ⚠️ Optional | - | OAuth client ID (auto-registers if empty) |
|
||||||
| `NEXTCLOUD_OIDC_CLIENT_SECRET` | ⚠️ Optional | - | OAuth client secret (auto-registers if empty) |
|
| `NEXTCLOUD_OIDC_CLIENT_SECRET` | ⚠️ Optional | - | OAuth client secret (auto-registers if empty) |
|
||||||
| `NEXTCLOUD_MCP_SERVER_URL` | ⚠️ Optional | `http://localhost:8000` | MCP server URL for OAuth callbacks |
|
| `NEXTCLOUD_MCP_SERVER_URL` | ⚠️ Optional | `http://localhost:8000` | MCP server URL for OAuth callbacks |
|
||||||
|
| `OIDC_RESOURCE_SERVER_ID` | ⚠️ Optional | - | Resource server identifier for IdPs that require prefixed scopes (e.g., AWS Cognito). When set, resource scopes are sent as `{id}/{scope}` |
|
||||||
| `NEXTCLOUD_USERNAME` | ❌ Must be empty | - | Leave empty to enable OAuth mode |
|
| `NEXTCLOUD_USERNAME` | ❌ Must be empty | - | Leave empty to enable OAuth mode |
|
||||||
| `NEXTCLOUD_PASSWORD` | ❌ Must be empty | - | Leave empty to enable OAuth mode |
|
| `NEXTCLOUD_PASSWORD` | ❌ Must be empty | - | Leave empty to enable OAuth mode |
|
||||||
|
|
||||||
|
|||||||
@@ -37,6 +37,7 @@ from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse
|
|||||||
from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback
|
from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback
|
||||||
from nextcloud_mcp_server.auth.client_registry import get_client_registry
|
from nextcloud_mcp_server.auth.client_registry import get_client_registry
|
||||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
||||||
|
from nextcloud_mcp_server.config import get_settings
|
||||||
|
|
||||||
from ..http import nextcloud_httpx_client
|
from ..http import nextcloud_httpx_client
|
||||||
|
|
||||||
@@ -125,8 +126,11 @@ def _transform_scopes_for_idp(scopes: str, resource_server_id: str) -> str:
|
|||||||
"""
|
"""
|
||||||
if not resource_server_id:
|
if not resource_server_id:
|
||||||
return scopes
|
return scopes
|
||||||
|
prefix = resource_server_id + "/"
|
||||||
return " ".join(
|
return " ".join(
|
||||||
f"{resource_server_id}/{s}" if s not in _OIDC_STANDARD_SCOPES else s
|
s
|
||||||
|
if s in _OIDC_STANDARD_SCOPES or s.startswith(prefix)
|
||||||
|
else f"{resource_server_id}/{s}"
|
||||||
for s in scopes.split()
|
for s in scopes.split()
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -369,7 +373,9 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
|
|||||||
|
|
||||||
# Prefix resource scopes with the resource server identifier if configured.
|
# Prefix resource scopes with the resource server identifier if configured.
|
||||||
# Required for IdPs like Cognito that use {identifier}/{scope} format.
|
# Required for IdPs like Cognito that use {identifier}/{scope} format.
|
||||||
resource_server_id = os.getenv("OIDC_RESOURCE_SERVER_ID", "").strip()
|
resource_server_id = (
|
||||||
|
(get_settings().oidc_resource_server_id or "").strip().rstrip("/")
|
||||||
|
)
|
||||||
idp_scope_str = _transform_scopes_for_idp(scopes, resource_server_id)
|
idp_scope_str = _transform_scopes_for_idp(scopes, resource_server_id)
|
||||||
if resource_server_id:
|
if resource_server_id:
|
||||||
logger.info(f" IdP scopes (prefixed): {idp_scope_str}")
|
logger.info(f" IdP scopes (prefixed): {idp_scope_str}")
|
||||||
|
|||||||
@@ -213,6 +213,7 @@ class Settings:
|
|||||||
oidc_client_id: str | None = None
|
oidc_client_id: str | None = None
|
||||||
oidc_client_secret: str | None = None
|
oidc_client_secret: str | None = None
|
||||||
oidc_issuer: str | None = None
|
oidc_issuer: str | None = None
|
||||||
|
oidc_resource_server_id: str | None = None
|
||||||
|
|
||||||
# Nextcloud settings
|
# Nextcloud settings
|
||||||
nextcloud_host: str | None = None
|
nextcloud_host: str | None = None
|
||||||
@@ -568,6 +569,7 @@ def get_settings() -> Settings:
|
|||||||
"oidc_client_id": "NEXTCLOUD_OIDC_CLIENT_ID",
|
"oidc_client_id": "NEXTCLOUD_OIDC_CLIENT_ID",
|
||||||
"oidc_client_secret": "NEXTCLOUD_OIDC_CLIENT_SECRET",
|
"oidc_client_secret": "NEXTCLOUD_OIDC_CLIENT_SECRET",
|
||||||
"oidc_issuer": "OIDC_ISSUER",
|
"oidc_issuer": "OIDC_ISSUER",
|
||||||
|
"oidc_resource_server_id": "OIDC_RESOURCE_SERVER_ID",
|
||||||
# Nextcloud settings
|
# Nextcloud settings
|
||||||
"nextcloud_host": "NEXTCLOUD_HOST",
|
"nextcloud_host": "NEXTCLOUD_HOST",
|
||||||
"nextcloud_username": "NEXTCLOUD_USERNAME",
|
"nextcloud_username": "NEXTCLOUD_USERNAME",
|
||||||
|
|||||||
@@ -37,6 +37,7 @@ oidc_issuer = "@none"
|
|||||||
jwks_uri = "@none"
|
jwks_uri = "@none"
|
||||||
introspection_uri = "@none"
|
introspection_uri = "@none"
|
||||||
userinfo_uri = "@none"
|
userinfo_uri = "@none"
|
||||||
|
oidc_resource_server_id = "@none"
|
||||||
|
|
||||||
# --- Mode flags ---
|
# --- Mode flags ---
|
||||||
enable_multi_user_basic_auth = false
|
enable_multi_user_basic_auth = false
|
||||||
|
|||||||
@@ -4,6 +4,8 @@ import pytest
|
|||||||
|
|
||||||
from nextcloud_mcp_server.auth.oauth_routes import _transform_scopes_for_idp
|
from nextcloud_mcp_server.auth.oauth_routes import _transform_scopes_for_idp
|
||||||
|
|
||||||
|
pytestmark = pytest.mark.unit
|
||||||
|
|
||||||
|
|
||||||
class TestTransformScopesForIdp:
|
class TestTransformScopesForIdp:
|
||||||
"""Test _transform_scopes_for_idp scope transformation."""
|
"""Test _transform_scopes_for_idp scope transformation."""
|
||||||
@@ -70,3 +72,13 @@ class TestTransformScopesForIdp:
|
|||||||
"""An empty scopes string returns empty."""
|
"""An empty scopes string returns empty."""
|
||||||
result = _transform_scopes_for_idp("", "https://api.example.com")
|
result = _transform_scopes_for_idp("", "https://api.example.com")
|
||||||
assert result == ""
|
assert result == ""
|
||||||
|
|
||||||
|
def test_already_prefixed_scopes_not_double_prefixed(self):
|
||||||
|
"""Scopes already carrying the resource server prefix are not prefixed again."""
|
||||||
|
result = _transform_scopes_for_idp(
|
||||||
|
"https://api.example.com/notes.read notes.write",
|
||||||
|
"https://api.example.com",
|
||||||
|
)
|
||||||
|
assert result == (
|
||||||
|
"https://api.example.com/notes.read https://api.example.com/notes.write"
|
||||||
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user