fix: address PR review for OIDC scope prefix feature
Add offline_access to OIDC standard scopes exclusion list to prevent it from being incorrectly prefixed, which would break Cognito refresh token flows. Extract scope transformation into testable _transform_scopes_for_idp() helper, add debug logging for prefixed scopes, remove unused Settings field (oauth_routes.py consistently uses os.getenv), and add unit tests. 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
e21ddd91b9
commit
f67d4d1116
@@ -109,6 +109,28 @@ _DCR_RATE_LIMIT_MAX = 10 # max requests
|
||||
_DCR_RATE_LIMIT_WINDOW = 60 # per 60 seconds
|
||||
|
||||
|
||||
# OIDC standard scopes that must never be prefixed with a resource server identifier.
|
||||
_OIDC_STANDARD_SCOPES = {"openid", "profile", "email", "offline_access"}
|
||||
|
||||
|
||||
def _transform_scopes_for_idp(scopes: str, resource_server_id: str) -> str:
|
||||
"""Prefix resource scopes with an IdP resource server identifier.
|
||||
|
||||
IdPs like AWS Cognito require resource scopes in ``{identifier}/{scope}``
|
||||
format. Standard OIDC scopes (openid, profile, email, offline_access) are
|
||||
forwarded unchanged.
|
||||
|
||||
When *resource_server_id* is empty the original scope string is returned
|
||||
as-is.
|
||||
"""
|
||||
if not resource_server_id:
|
||||
return scopes
|
||||
return " ".join(
|
||||
f"{resource_server_id}/{s}" if s not in _OIDC_STANDARD_SCOPES else s
|
||||
for s in scopes.split()
|
||||
)
|
||||
|
||||
|
||||
async def _get_cached_discovery(url: str) -> dict[str, Any]:
|
||||
"""Fetch OIDC discovery document with caching (5-minute TTL)."""
|
||||
now = time.time()
|
||||
@@ -347,17 +369,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
|
||||
|
||||
# Prefix resource scopes with the resource server identifier if configured.
|
||||
# Required for IdPs like Cognito that use {identifier}/{scope} format.
|
||||
# OIDC standard scopes are forwarded as-is.
|
||||
oidc_scopes = {"openid", "profile", "email"}
|
||||
resource_server_id = os.getenv("OIDC_RESOURCE_SERVER_ID", "")
|
||||
resource_server_id = os.getenv("OIDC_RESOURCE_SERVER_ID", "").strip()
|
||||
idp_scope_str = _transform_scopes_for_idp(scopes, resource_server_id)
|
||||
if resource_server_id:
|
||||
idp_scope_list = [
|
||||
f"{resource_server_id}/{s}" if s not in oidc_scopes else s
|
||||
for s in scopes.split()
|
||||
]
|
||||
idp_scope_str = " ".join(idp_scope_list)
|
||||
else:
|
||||
idp_scope_str = scopes
|
||||
logger.info(f" IdP scopes (prefixed): {idp_scope_str}")
|
||||
|
||||
# Redirect to Nextcloud with MCP server's own client_id (no PKCE — confidential client)
|
||||
idp_params = {
|
||||
|
||||
@@ -213,7 +213,6 @@ class Settings:
|
||||
oidc_client_id: str | None = None
|
||||
oidc_client_secret: str | None = None
|
||||
oidc_issuer: str | None = None
|
||||
oidc_resource_server_id: str | None = None
|
||||
|
||||
# Nextcloud settings
|
||||
nextcloud_host: str | None = None
|
||||
@@ -569,7 +568,6 @@ def get_settings() -> Settings:
|
||||
"oidc_client_id": "NEXTCLOUD_OIDC_CLIENT_ID",
|
||||
"oidc_client_secret": "NEXTCLOUD_OIDC_CLIENT_SECRET",
|
||||
"oidc_issuer": "OIDC_ISSUER",
|
||||
"oidc_resource_server_id": "OIDC_RESOURCE_SERVER_ID",
|
||||
# Nextcloud settings
|
||||
"nextcloud_host": "NEXTCLOUD_HOST",
|
||||
"nextcloud_username": "NEXTCLOUD_USERNAME",
|
||||
|
||||
Reference in New Issue
Block a user