fix(auth): address PR #757 round-3 review feedback

Three review items from the third-round review on PR #757:

- scope_authorization: split the combined logger.warning(error_msg) in
  the require_scopes decorator's missing-app-password branch into two
  lazy %-style logger calls (one per branch), keeping the f-string
  error_msg for the exception only. The else branch also logs the
  elicit_result for diagnostics. Bypassing lazy %-interpolation in
  security-sensitive code formatted the message regardless of log level
  and matched the repo-wide lazy-logging preference; the new code now
  conforms.
- config + browser_oauth_routes: wire COOKIE_SECURE through Settings
  (cookie_secure: bool | None = None) so _should_use_secure_cookies()
  reads it via get_settings() rather than os.getenv. Completes the
  consolidation pass that touched this file in commit 7464340 and
  removes the last raw os.getenv from browser_oauth_routes.py
  (import os dropped). Dynaconf auto-coerces "true"/"false" → bool;
  "1"/"0" arrive as int and are normalised by an explicit bool() at
  the consumer.
- elicitation: clarify the _astrolabe_settings_url docstring to call
  out that the empty-string case is also a None-return path (matches
  the existing `if not base:` guard).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/auth/browser_oauth_routes.py

@@ -6,7 +6,6 @@ for accessing admin UI endpoints like /app.
 
 import hashlib
 import logging
-import os
 import secrets
 import time
 from base64 import urlsafe_b64encode
@@ -30,23 +29,21 @@ logger = logging.getLogger(__name__)
 
 
 def _should_use_secure_cookies() -> bool:
-    """Determine if cookies should have secure flag.
+    """Determine if cookies should have the Secure flag.
 
-    Checks COOKIE_SECURE env var first, then auto-detects from NEXTCLOUD_HOST.
+    Reads ``settings.cookie_secure`` first (set via the ``COOKIE_SECURE``
+    env var). Falls back to auto-detect from the ``nextcloud_host`` scheme
+    when unset.
 
     Returns:
         True if cookies should be secure (HTTPS), False otherwise
     """
-    # Explicit configuration takes precedence
-    explicit = os.getenv("COOKIE_SECURE", "").lower()
-    if explicit == "true":
-        return True
-    if explicit == "false":
-        return False
-
-    # Auto-detect from NEXTCLOUD_HOST protocol (read via Settings for
-    # consistency with the rest of this file).
-    nextcloud_host = get_settings().nextcloud_host or ""
+    settings = get_settings()
+    if settings.cookie_secure is not None:
+        # Dynaconf auto-coerces "true"/"false" → bool but "1"/"0" → int;
+        # bool() normalises both.
+        return bool(settings.cookie_secure)
+    nextcloud_host = settings.nextcloud_host or ""
     return nextcloud_host.startswith("https://")
 
 

nextcloud_mcp_server/auth/elicitation.py

@@ -46,9 +46,10 @@ def _astrolabe_settings_url() -> str | None:
 
     Prefers ``nextcloud_public_issuer_url`` (the browser-reachable public URL)
     over ``nextcloud_host`` (which may be an internal hostname in Docker
-    deployments). Returns None if neither is set, or if the configured base
-    URL is missing an http:// or https:// scheme — in the latter case the
-    caller renders the tool-only fallback message instead of a broken link.
+    deployments). Returns None if neither is set (or set to the empty
+    string), or if the configured base URL is missing an http:// or
+    https:// scheme — in the latter case the caller renders the tool-only
+    fallback message instead of a broken link.
     """
     settings = get_settings()
     base = (

nextcloud_mcp_server/auth/scope_authorization.py

@@ -179,6 +179,11 @@ def require_scopes(*required_scopes: str):
                             # time the next retry can still hit a not-yet-
                             # populated entry — hence the "wait a moment"
                             # qualifier below.
+                            logger.warning(
+                                "Access denied to %s: app password missing "
+                                "after user accepted elicitation; advising retry",
+                                func_name,
+                            )
                             error_msg = (
                                 f"Access denied to {func_name}: Nextcloud "
                                 f"access was not provisioned at the time of "
@@ -188,12 +193,18 @@ def require_scopes(*required_scopes: str):
                                 f"completing; wait a moment and try again."
                             )
                         else:
+                            logger.warning(
+                                "Access denied to %s: app password missing; "
+                                "advising nc_auth_provision_access "
+                                "(elicit_result=%s)",
+                                func_name,
+                                elicit_result,
+                            )
                             error_msg = (
                                 f"Access denied to {func_name}: "
                                 f"Nextcloud access not provisioned. "
                                 f"Please call 'nc_auth_provision_access' first."
                             )
-                        logger.warning(error_msg)
                         raise ProvisioningRequiredError(error_msg)
 
                     if stored_scopes == "all":

nextcloud_mcp_server/config.py

@@ -32,6 +32,7 @@ _DEFAULTS: dict[str, Any] = {
     "nextcloud_mcp_server_url": None,
     "nextcloud_resource_uri": None,
     "nextcloud_public_issuer_url": None,
+    "cookie_secure": None,
     # OAuth/OIDC
     "oidc_discovery_url": None,
     "nextcloud_oidc_client_id": None,
@@ -412,6 +413,11 @@ class Settings:
     # nextcloud_host when unset.
     nextcloud_public_issuer_url: str | None = None
 
+    # Browser cookie Secure flag. None = auto-detect from nextcloud_host
+    # scheme (https → True, else False). Set COOKIE_SECURE=true/false to
+    # override.
+    cookie_secure: bool | None = None
+
     # Nextcloud SSL/TLS settings
     nextcloud_verify_ssl: bool = True
     nextcloud_ca_bundle: str | None = None
@@ -784,6 +790,7 @@ def get_settings() -> Settings:
         "nextcloud_password": "NEXTCLOUD_PASSWORD",
         "nextcloud_app_password": "NEXTCLOUD_APP_PASSWORD",
         "nextcloud_public_issuer_url": "NEXTCLOUD_PUBLIC_ISSUER_URL",
+        "cookie_secure": "COOKIE_SECURE",
         # Nextcloud SSL/TLS settings
         "nextcloud_verify_ssl": "NEXTCLOUD_VERIFY_SSL",
         "nextcloud_ca_bundle": "NEXTCLOUD_CA_BUNDLE",