feat: signed in over there, signed in here
Opening one PedsHub app while already signed in at the provider for the other one should not produce a sign-in page. It asks the provider once, with prompt=none — "do you already know this person?" — and if the answer is yes the round trip finishes with no screen and no click. The refusal is the interesting half. login_required, interaction_required, consent_required and account_selection_required are the provider saying nobody is signed in, which is an answer rather than a failure: the visitor lands on the page they asked for, with no message and no sign of having been anywhere. Anything else still goes to /login?error=sso_failed, and a silent attempt that throws is swallowed too — nobody should be interrupted by a request they did not make. The whole risk in this is a loop between two sites, so: at most one attempt per browser session, never after somebody has signed themselves out, and never inside a native shell where there is no third-party cookie to carry the provider's session. Signing out sets a marker that outlives the tab; pressing any sign-in control clears it, because that is a person saying they have changed their mind. A deep link survives the trip. The intended path rides in the server session rather than the URL, and is validated on the way back — a scheme, a host or a protocol-relative //evil all collapse to "/", because a sign-in round trip is exactly where an open redirect would live. Verified against the live provider: /api/auth/sso/login?prompt=none answers 302 to Authentik carrying prompt=none, state and nonce, and a visitor with no session anywhere lands on the landing page with the attempt marked spent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN
This commit is contained in:
parent
d896239690
commit
b2a75b9e08
8 changed files with 222 additions and 7 deletions
|
|
@ -1,5 +1,6 @@
|
||||||
import logging
|
import logging
|
||||||
import secrets
|
import secrets
|
||||||
|
from urllib.parse import quote
|
||||||
from datetime import datetime, timedelta
|
from datetime import datetime, timedelta
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks, status, Request
|
from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks, status, Request
|
||||||
|
|
@ -432,7 +433,40 @@ async def sso_login(request: Request):
|
||||||
)
|
)
|
||||||
|
|
||||||
redirect_uri = f"{cfg.APP_URL}/api/auth/sso/callback"
|
redirect_uri = f"{cfg.APP_URL}/api/auth/sso/callback"
|
||||||
return await oauth.oidc.authorize_redirect(request, redirect_uri)
|
|
||||||
|
# Where to come back to, and whether this attempt is allowed to show a
|
||||||
|
# login screen. Both live in the session rather than the URL: the callback
|
||||||
|
# needs them and neither is the provider's business.
|
||||||
|
#
|
||||||
|
# `prompt=none` is the whole of "sign me in if you already know me".
|
||||||
|
# Somebody who signed in at the provider for the other app is signed in
|
||||||
|
# here too, without a screen, without a click — and somebody who is not
|
||||||
|
# gets a quiet refusal we can act on rather than a login page they did not
|
||||||
|
# ask for.
|
||||||
|
request.session["sso_next"] = _safe_next(request.query_params.get("next"))
|
||||||
|
silent = request.query_params.get("prompt") == "none"
|
||||||
|
request.session["sso_silent"] = silent
|
||||||
|
extra = {"prompt": "none"} if silent else {}
|
||||||
|
return await oauth.oidc.authorize_redirect(request, redirect_uri, **extra)
|
||||||
|
|
||||||
|
|
||||||
|
#: The provider's way of saying "nobody is signed in here" to a silent
|
||||||
|
#: attempt. None of the three is a failure; they are the answer.
|
||||||
|
QUIET_REFUSALS = {"login_required", "interaction_required", "consent_required",
|
||||||
|
"account_selection_required"}
|
||||||
|
|
||||||
|
|
||||||
|
def _safe_next(raw: str | None) -> str:
|
||||||
|
"""A path within this app, or the front door.
|
||||||
|
|
||||||
|
Anything else — a scheme, a host, a protocol-relative `//evil` — is an
|
||||||
|
open redirect, which is exactly the thing a sign-in round trip must not
|
||||||
|
become.
|
||||||
|
"""
|
||||||
|
path = (raw or "").strip()
|
||||||
|
if not path.startswith("/") or path.startswith("//"):
|
||||||
|
return "/"
|
||||||
|
return path[:500]
|
||||||
|
|
||||||
|
|
||||||
@router.get("/sso/callback")
|
@router.get("/sso/callback")
|
||||||
|
|
@ -455,9 +489,25 @@ async def sso_callback(request: Request, db: Session = Depends(get_db)):
|
||||||
client_kwargs={"scope": cfg.OIDC_SCOPES},
|
client_kwargs={"scope": cfg.OIDC_SCOPES},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
silent = bool(request.session.pop("sso_silent", False))
|
||||||
|
landing = _safe_next(request.session.pop("sso_next", None))
|
||||||
|
|
||||||
|
# A silent attempt that finds nobody signed in is not a failure. The
|
||||||
|
# provider says so in the query string, and the visitor should land on the
|
||||||
|
# page they asked for with no message and no sign of having been anywhere.
|
||||||
|
refusal = request.query_params.get("error")
|
||||||
|
if refusal:
|
||||||
|
if silent and refusal in QUIET_REFUSALS:
|
||||||
|
return RedirectResponse(url=f"{cfg.APP_URL}{landing}")
|
||||||
|
return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed")
|
||||||
|
|
||||||
try:
|
try:
|
||||||
token = await oauth.oidc.authorize_access_token(request)
|
token = await oauth.oidc.authorize_access_token(request)
|
||||||
except Exception:
|
except Exception:
|
||||||
|
if silent:
|
||||||
|
# Anything at all going wrong on an attempt nobody asked for is
|
||||||
|
# still not something to interrupt them with.
|
||||||
|
return RedirectResponse(url=f"{cfg.APP_URL}{landing}")
|
||||||
return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed")
|
return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed")
|
||||||
|
|
||||||
userinfo = token.get("userinfo") or {}
|
userinfo = token.get("userinfo") or {}
|
||||||
|
|
@ -521,4 +571,7 @@ async def sso_callback(request: Request, db: Session = Depends(get_db)):
|
||||||
if code is None:
|
if code is None:
|
||||||
logger.error("SSO succeeded but the exchange store is unreachable")
|
logger.error("SSO succeeded but the exchange store is unreachable")
|
||||||
return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed")
|
return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed")
|
||||||
return RedirectResponse(url=f"{cfg.APP_URL}/sso-callback?code={code}")
|
# Back to whatever was being opened when the round trip started, so a link
|
||||||
|
# to one article lands on that article rather than the dashboard.
|
||||||
|
where = f"&next={quote(landing, safe='')}" if landing and landing != "/" else ""
|
||||||
|
return RedirectResponse(url=f"{cfg.APP_URL}/sso-callback?code={code}{where}")
|
||||||
|
|
|
||||||
|
|
@ -112,3 +112,33 @@ class OneTimeCodeTests(unittest.TestCase):
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|
||||||
|
|
||||||
|
class SilentSignInTests(unittest.TestCase):
|
||||||
|
"""Signed in at the provider for the other app means signed in here.
|
||||||
|
|
||||||
|
`prompt=none` asks "do you already know this person?". If the provider
|
||||||
|
does, the round trip completes with no screen and no click. If it does
|
||||||
|
not, it refuses in the query string — and that refusal is the answer, not
|
||||||
|
a failure to report.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def test_a_quiet_refusal_is_not_an_error(self):
|
||||||
|
from app.routers.auth import QUIET_REFUSALS
|
||||||
|
for said in ("login_required", "interaction_required",
|
||||||
|
"consent_required", "account_selection_required"):
|
||||||
|
self.assertIn(said, QUIET_REFUSALS)
|
||||||
|
# A real failure still is one.
|
||||||
|
self.assertNotIn("invalid_client", QUIET_REFUSALS)
|
||||||
|
self.assertNotIn("server_error", QUIET_REFUSALS)
|
||||||
|
|
||||||
|
def test_the_return_path_cannot_leave_the_site(self):
|
||||||
|
"""A sign-in round trip is exactly where an open redirect would live."""
|
||||||
|
from app.routers.auth import _safe_next
|
||||||
|
self.assertEqual(_safe_next("/articles/95?section=abc"), "/articles/95?section=abc")
|
||||||
|
self.assertEqual(_safe_next("//evil.example"), "/")
|
||||||
|
self.assertEqual(_safe_next("https://evil.example/x"), "/")
|
||||||
|
self.assertEqual(_safe_next("javascript:alert(1)"), "/")
|
||||||
|
self.assertEqual(_safe_next(None), "/")
|
||||||
|
self.assertEqual(_safe_next(""), "/")
|
||||||
|
self.assertEqual(len(_safe_next("/" + "a" * 900)), 500)
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
import { createContext, useContext, useState, useEffect } from 'react'
|
import { createContext, useContext, useState, useEffect } from 'react'
|
||||||
import api from '../api/client'
|
import api from '../api/client'
|
||||||
import { setToken } from '../utils/token'
|
import { setToken } from '../utils/token'
|
||||||
|
import { markSignedOut, shouldTrySilently, trySilently } from '../utils/silentSso'
|
||||||
|
|
||||||
const AuthContext = createContext(null)
|
const AuthContext = createContext(null)
|
||||||
|
|
||||||
|
|
@ -16,7 +17,19 @@ export function AuthProvider({ children }) {
|
||||||
.catch(() => { if (localStorage.getItem('token') === token) setToken(null) })
|
.catch(() => { if (localStorage.getItem('token') === token) setToken(null) })
|
||||||
.finally(() => setLoading(false))
|
.finally(() => setLoading(false))
|
||||||
} else {
|
} else {
|
||||||
setLoading(false)
|
// Nobody signed in here — but they may be signed in at the provider for
|
||||||
|
// the other app, in which case they should not be shown a door. One
|
||||||
|
// silent attempt, then the page as normal. `loading` stays true while
|
||||||
|
// the browser is on its way out, so nothing paints and swaps.
|
||||||
|
api.get('/auth/sso/config')
|
||||||
|
.then(res => {
|
||||||
|
if (shouldTrySilently({ hasToken: false, ssoEnabled: res.data?.sso_enabled === true })) {
|
||||||
|
trySilently()
|
||||||
|
return
|
||||||
|
}
|
||||||
|
setLoading(false)
|
||||||
|
})
|
||||||
|
.catch(() => setLoading(false))
|
||||||
}
|
}
|
||||||
}, [])
|
}, [])
|
||||||
|
|
||||||
|
|
@ -38,6 +51,10 @@ export function AuthProvider({ children }) {
|
||||||
const logout = () => {
|
const logout = () => {
|
||||||
setToken(null)
|
setToken(null)
|
||||||
setUser(null)
|
setUser(null)
|
||||||
|
// Somebody who signs out has said they want to be signed out. Without
|
||||||
|
// this the next page load would sign them straight back in, which is not
|
||||||
|
// a bug anybody would report — it just looks like the button is broken.
|
||||||
|
markSignedOut()
|
||||||
}
|
}
|
||||||
|
|
||||||
return (
|
return (
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
import { useEffect, useRef, useState } from 'react'
|
import { useEffect, useRef, useState } from 'react'
|
||||||
import { Link, useNavigate } from 'react-router-dom'
|
import { Link, useNavigate } from 'react-router-dom'
|
||||||
import api from '../api/client'
|
import api from '../api/client'
|
||||||
|
import { clearSignedOut } from '../utils/silentSso'
|
||||||
import { useAuth } from '../context/AuthContext'
|
import { useAuth } from '../context/AuthContext'
|
||||||
import useMediaQuery from '../hooks/useMediaQuery'
|
import useMediaQuery from '../hooks/useMediaQuery'
|
||||||
import Navbar from '../components/Navbar'
|
import Navbar from '../components/Navbar'
|
||||||
|
|
@ -292,7 +293,7 @@ function AuthModal({ onClose }) {
|
||||||
{/* The provider, first, and on an SSO-only site the only thing here. */}
|
{/* The provider, first, and on an SSO-only site the only thing here. */}
|
||||||
{ssoEnabled === true && (
|
{ssoEnabled === true && (
|
||||||
<>
|
<>
|
||||||
<a href="/api/auth/sso/login" className="btn btn-primary btn-block"
|
<a href="/api/auth/sso/login" onClick={clearSignedOut} className="btn btn-primary btn-block"
|
||||||
style={{ display: 'block', textAlign: 'center', textDecoration: 'none' }}>
|
style={{ display: 'block', textAlign: 'center', textDecoration: 'none' }}>
|
||||||
Sign in with {providerName}
|
Sign in with {providerName}
|
||||||
</a>
|
</a>
|
||||||
|
|
@ -588,7 +589,7 @@ export default function LandingPage() {
|
||||||
return () => { live = false }
|
return () => { live = false }
|
||||||
}, [])
|
}, [])
|
||||||
const signIn = () => {
|
const signIn = () => {
|
||||||
if (ssoOnly) window.location.href = '/api/auth/sso/login'
|
if (ssoOnly) { clearSignedOut(); window.location.href = '/api/auth/sso/login' }
|
||||||
else setAuthOpen(true)
|
else setAuthOpen(true)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@ import { useState, useEffect } from 'react'
|
||||||
import { useNavigate, Link } from 'react-router-dom'
|
import { useNavigate, Link } from 'react-router-dom'
|
||||||
import { useAuth } from '../context/AuthContext'
|
import { useAuth } from '../context/AuthContext'
|
||||||
import api from '../api/client'
|
import api from '../api/client'
|
||||||
|
import { clearSignedOut } from '../utils/silentSso'
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Signing in.
|
* Signing in.
|
||||||
|
|
@ -61,7 +62,7 @@ export default function LoginPage() {
|
||||||
|
|
||||||
{ssoConfig?.sso_enabled && (
|
{ssoConfig?.sso_enabled && (
|
||||||
<>
|
<>
|
||||||
<a href="/api/auth/sso/login" className="btn btn-primary" style={{ width: '100%', display: 'block', textAlign: 'center', textDecoration: 'none', marginBottom: 16 }}>
|
<a href="/api/auth/sso/login" onClick={clearSignedOut} className="btn btn-primary" style={{ width: '100%', display: 'block', textAlign: 'center', textDecoration: 'none', marginBottom: 16 }}>
|
||||||
Sign in with {ssoConfig.provider_name}
|
Sign in with {ssoConfig.provider_name}
|
||||||
</a>
|
</a>
|
||||||
{!ssoConfig.sso_only && <div style={{ textAlign: 'center', color: 'var(--text-muted)', fontSize: '0.82rem', margin: '12px 0' }}>or sign in with email</div>}
|
{!ssoConfig.sso_only && <div style={{ textAlign: 'center', color: 'var(--text-muted)', fontSize: '0.82rem', margin: '12px 0' }}>or sign in with email</div>}
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@ import { useEffect, useRef } from 'react'
|
||||||
import { useSearchParams, useNavigate } from 'react-router-dom'
|
import { useSearchParams, useNavigate } from 'react-router-dom'
|
||||||
import api from '../api/client'
|
import api from '../api/client'
|
||||||
import { useAuth } from '../context/AuthContext'
|
import { useAuth } from '../context/AuthContext'
|
||||||
|
import { clearSignedOut } from '../utils/silentSso'
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Where the provider sends you back to, holding a code rather than a token.
|
* Where the provider sends you back to, holding a code rather than a token.
|
||||||
|
|
@ -29,9 +30,14 @@ export default function SsoCallbackPage() {
|
||||||
spent.current = true
|
spent.current = true
|
||||||
const code = searchParams.get('code')
|
const code = searchParams.get('code')
|
||||||
if (!code) { navigate('/login?error=sso_failed'); return }
|
if (!code) { navigate('/login?error=sso_failed'); return }
|
||||||
|
// Back to what was being opened when the round trip started — a study
|
||||||
|
// session, an article — rather than the dashboard. The server hands the
|
||||||
|
// path back because it is the only side that survived the redirect.
|
||||||
|
const next = searchParams.get('next')
|
||||||
|
const where = next && next.startsWith('/') && !next.startsWith('//') ? next : '/'
|
||||||
api.post('/auth/sso/exchange', { code })
|
api.post('/auth/sso/exchange', { code })
|
||||||
.then(res => loginWithToken(res.data.access_token))
|
.then(res => loginWithToken(res.data.access_token))
|
||||||
.then(() => navigate('/'))
|
.then(() => { clearSignedOut(); navigate(where, { replace: true }) })
|
||||||
.catch(() => navigate('/login?error=sso_failed'))
|
.catch(() => navigate('/login?error=sso_failed'))
|
||||||
}, [])
|
}, [])
|
||||||
|
|
||||||
|
|
|
||||||
60
frontend/src/utils/silentSso.js
Normal file
60
frontend/src/utils/silentSso.js
Normal file
|
|
@ -0,0 +1,60 @@
|
||||||
|
/**
|
||||||
|
* Signed in over there, signed in here.
|
||||||
|
*
|
||||||
|
* Somebody who has just signed in at the identity provider for the other app
|
||||||
|
* should not be shown a sign-in page by this one. `prompt=none` asks the
|
||||||
|
* provider "do you already know this person?" — if it does, the round trip
|
||||||
|
* completes without a screen and without a click; if it does not, it refuses
|
||||||
|
* quietly and the visitor lands where they were going, none the wiser.
|
||||||
|
*
|
||||||
|
* The whole risk here is a loop: a silent attempt that fails, retries, and
|
||||||
|
* bounces somebody between two sites for ever. So it happens at most once per
|
||||||
|
* browser session, never after somebody has signed themselves out, and never
|
||||||
|
* inside a native shell where the round trip has nowhere to land.
|
||||||
|
*/
|
||||||
|
const TRIED = 'sso_silent_tried'
|
||||||
|
const SIGNED_OUT = 'sso_signed_out'
|
||||||
|
|
||||||
|
//: A webview has no address bar and often no third-party cookie to carry the
|
||||||
|
//: provider's session, so the attempt would fail and cost a page load.
|
||||||
|
const inNativeShell = () => (
|
||||||
|
typeof navigator !== 'undefined'
|
||||||
|
&& /(wv|; ?wv\)|Median|Capacitor|Cordova)/i.test(navigator.userAgent || '')
|
||||||
|
)
|
||||||
|
|
||||||
|
const store = (which) => {
|
||||||
|
try { return which === 'local' ? window.localStorage : window.sessionStorage }
|
||||||
|
catch { return null }
|
||||||
|
}
|
||||||
|
|
||||||
|
const get = (which, key) => { try { return store(which)?.getItem(key) } catch { return null } }
|
||||||
|
const put = (which, key, value) => { try { store(which)?.setItem(key, value) } catch { /* private mode */ } }
|
||||||
|
const drop = (which, key) => { try { store(which)?.removeItem(key) } catch { /* private mode */ } }
|
||||||
|
|
||||||
|
/** Remember that this person chose to leave, so nothing signs them back in. */
|
||||||
|
export function markSignedOut() {
|
||||||
|
put('local', SIGNED_OUT, '1')
|
||||||
|
put('session', TRIED, '1')
|
||||||
|
}
|
||||||
|
|
||||||
|
/** They asked to sign in, so the refusal no longer stands. */
|
||||||
|
export function clearSignedOut() {
|
||||||
|
drop('local', SIGNED_OUT)
|
||||||
|
drop('session', TRIED)
|
||||||
|
}
|
||||||
|
|
||||||
|
export function shouldTrySilently({ hasToken, ssoEnabled }) {
|
||||||
|
if (hasToken || !ssoEnabled) return false
|
||||||
|
if (get('local', SIGNED_OUT)) return false
|
||||||
|
if (get('session', TRIED)) return false
|
||||||
|
if (inNativeShell()) return false
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
/** One attempt, carrying where we were going so it can be restored after. */
|
||||||
|
export function trySilently() {
|
||||||
|
put('session', TRIED, '1')
|
||||||
|
const here = window.location.pathname + window.location.search + window.location.hash
|
||||||
|
const next = here && here !== '/' ? `&next=${encodeURIComponent(here)}` : ''
|
||||||
|
window.location.replace(`/api/auth/sso/login?prompt=none${next}`)
|
||||||
|
}
|
||||||
47
frontend/src/utils/silentSso.test.js
Normal file
47
frontend/src/utils/silentSso.test.js
Normal file
|
|
@ -0,0 +1,47 @@
|
||||||
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||||
|
import { clearSignedOut, markSignedOut, shouldTrySilently, trySilently } from './silentSso'
|
||||||
|
|
||||||
|
describe('signing in silently when the provider already knows you', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
localStorage.clear()
|
||||||
|
sessionStorage.clear()
|
||||||
|
vi.unstubAllGlobals()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('tries once, and only once, per browser session', () => {
|
||||||
|
expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(true)
|
||||||
|
const replace = vi.fn()
|
||||||
|
vi.stubGlobal('location', { pathname: '/articles/95', search: '', hash: '', replace })
|
||||||
|
trySilently()
|
||||||
|
expect(replace).toHaveBeenCalledWith(
|
||||||
|
'/api/auth/sso/login?prompt=none&next=%2Farticles%2F95')
|
||||||
|
// The whole risk is a loop between two sites; the marker is what stops it.
|
||||||
|
expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('never runs for somebody already signed in, or where there is no provider', () => {
|
||||||
|
expect(shouldTrySilently({ hasToken: true, ssoEnabled: true })).toBe(false)
|
||||||
|
expect(shouldTrySilently({ hasToken: false, ssoEnabled: false })).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('never signs somebody back in after they signed themselves out', () => {
|
||||||
|
markSignedOut()
|
||||||
|
expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(false)
|
||||||
|
// Until they ask, which is what pressing a sign-in control means. The
|
||||||
|
// marker outlives the tab, so a sign-out holds tomorrow as well.
|
||||||
|
clearSignedOut()
|
||||||
|
expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('stays out of a native shell, where the round trip has nowhere to land', () => {
|
||||||
|
vi.stubGlobal('navigator', { userAgent: 'Mozilla/5.0 (Linux; Android 14; wv) Median' })
|
||||||
|
expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('does not throw where storage is refused', () => {
|
||||||
|
const boom = () => { throw new Error('private mode') }
|
||||||
|
vi.stubGlobal('sessionStorage', { getItem: boom, setItem: boom, removeItem: boom })
|
||||||
|
expect(() => shouldTrySilently({ hasToken: false, ssoEnabled: true })).not.toThrow()
|
||||||
|
expect(() => markSignedOut()).not.toThrow()
|
||||||
|
})
|
||||||
|
})
|
||||||
Loading…
Reference in a new issue