fix(auth): address PR #758 round-7 medium/minor review
- Gate browser session creation on a successful refresh token. When the IdP returns no refresh token, SessionAuthBackend would silently reject every subsequent request and bounce the user back to /oauth/login in a loop. The callback now bails with a 400 + correlation ID + actionable hint about offline_access *before* writing browser_sessions or setting the cookie. Pinned by a new end-to-end unit test. - Evict orphaned browser_sessions rows in SessionAuthBackend when the associated refresh token is gone, instead of letting them accumulate until TTL cleanup. Best-effort; deletion errors stay non-fatal. - Demote identity-bearing logs in the Flow 2 OAuth callback (user_id, scopes, audience, expires_at) from INFO to DEBUG so they don't leak into multi-tenant log aggregation on every provision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
27fcf05d3a
commit
b875eaf069
@@ -568,30 +568,47 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
|
||||
token_data.get("scope", "").split() if token_data.get("scope") else None
|
||||
)
|
||||
|
||||
# Store refresh token (for background jobs ONLY)
|
||||
if refresh_token:
|
||||
logger.debug(
|
||||
"Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s",
|
||||
# Store refresh token (for background jobs ONLY). The browser session
|
||||
# itself is gated on this — without a refresh token, ``SessionAuthBackend``
|
||||
# would reject every subsequent request and silently bounce the user back
|
||||
# to ``/oauth/login`` (PR #758 round-7 medium 1).
|
||||
if not refresh_token:
|
||||
correlation_id = secrets.token_urlsafe(8)
|
||||
logger.error(
|
||||
"No refresh token in token response — cannot establish browser "
|
||||
"session (correlation_id=%s, user_id=%s)",
|
||||
correlation_id,
|
||||
user_id,
|
||||
state[:16],
|
||||
granted_scopes,
|
||||
refresh_expires_at,
|
||||
)
|
||||
await storage.store_refresh_token(
|
||||
user_id=user_id,
|
||||
refresh_token=refresh_token,
|
||||
expires_at=refresh_expires_at,
|
||||
flow_type="browser", # Browser-based login flow
|
||||
provisioning_client_id=state, # Store state for unified session lookup
|
||||
scopes=granted_scopes,
|
||||
return HTMLResponse(
|
||||
f"<h1>Login Failed</h1>"
|
||||
f"<p>The identity provider did not return a refresh token, so a "
|
||||
f"persistent session could not be established. Make sure "
|
||||
f"<code>offline_access</code> is granted in the IdP configuration.</p>"
|
||||
f"<p>Correlation ID: <code>{html_escape(correlation_id)}</code></p>",
|
||||
status_code=400,
|
||||
)
|
||||
logger.info(
|
||||
"Refresh token stored for user %s (lookup key: %s...)",
|
||||
user_id,
|
||||
state[:16],
|
||||
)
|
||||
else:
|
||||
logger.warning("No refresh token in token response - cannot store session")
|
||||
|
||||
logger.debug(
|
||||
"Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s",
|
||||
user_id,
|
||||
state[:16],
|
||||
granted_scopes,
|
||||
refresh_expires_at,
|
||||
)
|
||||
await storage.store_refresh_token(
|
||||
user_id=user_id,
|
||||
refresh_token=refresh_token,
|
||||
expires_at=refresh_expires_at,
|
||||
flow_type="browser", # Browser-based login flow
|
||||
provisioning_client_id=state, # Store state for unified session lookup
|
||||
scopes=granted_scopes,
|
||||
)
|
||||
logger.info(
|
||||
"Refresh token stored for user %s (lookup key: %s...)",
|
||||
user_id,
|
||||
state[:16],
|
||||
)
|
||||
|
||||
# Query and cache user profile (for browser UI display)
|
||||
access_token = token_data.get("access_token")
|
||||
|
||||
@@ -702,16 +702,19 @@ async def oauth_callback_nextcloud(request: Request):
|
||||
# Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON
|
||||
# string rather than an int; coerce to be safe.
|
||||
refresh_expires_at = int(time.time()) + int(refresh_expires_in)
|
||||
logger.info(" refresh_expires_in: %ss", refresh_expires_in)
|
||||
logger.info(" refresh_expires_at: %s", refresh_expires_at)
|
||||
logger.debug(" refresh_expires_in: %ss", refresh_expires_in)
|
||||
logger.debug(" refresh_expires_at: %s", refresh_expires_at)
|
||||
|
||||
logger.info("Storing refresh token:")
|
||||
logger.info(" user_id: %s", user_id)
|
||||
logger.info(" flow_type: flow2")
|
||||
logger.info(" token_audience: nextcloud")
|
||||
logger.info(" provisioning_client_id: %s...", state[:16])
|
||||
logger.info(" scopes: %s", granted_scopes)
|
||||
logger.info(" expires_at: %s", refresh_expires_at)
|
||||
# Identity-bearing fields stay at DEBUG so they don't reach
|
||||
# multi-tenant log aggregation on every Flow 2 provision (PR #758
|
||||
# round-7 minor).
|
||||
logger.debug("Storing refresh token:")
|
||||
logger.debug(" user_id: %s", user_id)
|
||||
logger.debug(" flow_type: flow2")
|
||||
logger.debug(" token_audience: nextcloud")
|
||||
logger.debug(" provisioning_client_id: %s...", state[:16])
|
||||
logger.debug(" scopes: %s", granted_scopes)
|
||||
logger.debug(" expires_at: %s", refresh_expires_at)
|
||||
|
||||
await storage.store_refresh_token(
|
||||
user_id=user_id,
|
||||
@@ -722,8 +725,8 @@ async def oauth_callback_nextcloud(request: Request):
|
||||
scopes=granted_scopes,
|
||||
expires_at=refresh_expires_at,
|
||||
)
|
||||
logger.info("✓ Stored Flow 2 master refresh token for user %s", user_id)
|
||||
logger.info("=" * 60)
|
||||
logger.debug("✓ Stored Flow 2 master refresh token for user %s", user_id)
|
||||
logger.debug("=" * 60)
|
||||
|
||||
# Return success HTML page
|
||||
success_html = """
|
||||
|
||||
@@ -101,6 +101,17 @@ class SessionAuthBackend(AuthenticationBackend):
|
||||
session_id[:8],
|
||||
user_id,
|
||||
)
|
||||
# Proactively evict the orphan so the table doesn't accumulate
|
||||
# rows that the auth check will keep rejecting until TTL
|
||||
# cleanup (PR #758 round-7 minor).
|
||||
try:
|
||||
await storage.delete_browser_session(session_id)
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"Failed to delete orphaned browser session %s…: %s",
|
||||
session_id[:8],
|
||||
e,
|
||||
)
|
||||
return None
|
||||
|
||||
return AuthCredentials(["authenticated"]), SimpleUser(user_id)
|
||||
|
||||
Reference in New Issue
Block a user