diff --git a/gramps_webapi/api/resources/oidc.py b/gramps_webapi/api/resources/oidc.py index 52fd0415..1520bed0 100644 --- a/gramps_webapi/api/resources/oidc.py +++ b/gramps_webapi/api/resources/oidc.py @@ -20,17 +20,17 @@ """OIDC authentication resources.""" import logging +import secrets from gettext import gettext as _ -from urllib.parse import urlencode, urlparse +from urllib.parse import urlencode from flask import ( current_app, - jsonify, redirect, render_template, - request, session, ) +from itsdangerous import BadSignature, SignatureExpired, URLSafeTimedSerializer from marshmallow import EXCLUDE, Schema from webargs import fields @@ -43,10 +43,16 @@ from ...auth.oidc_helpers import is_oidc_enabled from ...const import TREE_MULTI from ..blueprint import api_blueprint +from ..cache import persistent_cache from ..ratelimiter import limiter from ..util import abort_with_message, get_config, tree_exists from . import Resource -from .schemas import OIDCConfigSchema, OIDCLogoutSchema, OIDCTokensSchema +from .schemas import ( + OIDCConfigSchema, + OIDCLogoutSchema, + OIDCTokenExchangeSchema, + OIDCTokensSchema, +) from .token import get_tokens, get_tree_id_and_permissions logger = logging.getLogger(__name__) @@ -56,6 +62,17 @@ # the redirect URI to match the registered one exactly. SESSION_TREE_KEY = "oidc_tree" +# Prefix for the server-side entry holding the tokens of a pending exchange. +OIDC_CODE_PREFIX = "oidc_code:" + +# Seconds a code stays redeemable, i.e. how long the frontend has to load. +OIDC_CODE_TIMEOUT = 120 + +# The cached entries outlive the code itself, so that an expired code is always +# reported as expired rather than as a missing entry, and so that a redeemed one +# is still known to have been redeemed. +OIDC_CODE_CACHE_TIMEOUT = OIDC_CODE_TIMEOUT + 60 + def _get_oidc_client(provider_id: str | None) -> tuple[object, dict]: """Return the authlib client and configuration for a validated provider. @@ -118,21 +135,60 @@ def _validate_tree_id(tree_id: str | None) -> str | None: return tree_id -def _use_secure_cookies() -> bool: - """Whether to set the Secure flag on the temporary OIDC cookies. +def _code_serializer() -> URLSafeTimedSerializer: + """Serializer signing the exchange code, so its origin and age are provable.""" + return URLSafeTimedSerializer( + current_app.config["SECRET_KEY"], salt="oidc-exchange-code" + ) - Driven by the scheme actually in use rather than by hostname guessing: a - Secure cookie is silently dropped by the browser over plain HTTP, while - omitting the flag over HTTPS would expose the tokens. Both the configured - frontend URL and the incoming request are consulted, because either one on - its own can be misleading - `BASE_URL` is often left at its localhost - default, and `request.is_secure` is false behind a TLS-terminating proxy - unless forwarding headers are honoured. - """ - frontend_url = get_config("FRONTEND_URL") or get_config("BASE_URL") - if urlparse(frontend_url or "").scheme == "https": - return True - return request.is_secure + +def _store_exchange_code(tokens: dict) -> str: + """Park the tokens server-side and return the code that redeems them.""" + code_id = secrets.token_urlsafe(32) + persistent_cache.set( + f"{OIDC_CODE_PREFIX}{code_id}", tokens, timeout=OIDC_CODE_CACHE_TIMEOUT + ) + return _code_serializer().dumps(code_id) + + +def _redeem_exchange_code(code: str) -> dict: + """Return the tokens for a code, consuming it so it works only once.""" + try: + code_id = _code_serializer().loads(code, max_age=OIDC_CODE_TIMEOUT) + except SignatureExpired: + abort_with_message( + 400, "The OIDC exchange code has expired, please log in again" + ) + except BadSignature: + abort_with_message(400, "Invalid OIDC exchange code") + + key = f"{OIDC_CODE_PREFIX}{code_id}" + claim_key = f"{key}:claimed" + entry = persistent_cache.get(key) + + # Claiming with add() rather than checking and then writing, so that two + # simultaneous requests cannot both redeem the same code. + if not persistent_cache.add(claim_key, "1", timeout=OIDC_CODE_CACHE_TIMEOUT): + abort_with_message(400, "The OIDC exchange code has already been used") + + if entry is None: + # The signature proves this server issued the code and that it is not + # yet expired, so the entry it names should still be in the cache. + persistent_cache.delete(claim_key) + logger.error( + "The persistent cache did not retain a valid OIDC exchange code." + " OIDC login requires a cache shared by every worker and replica," + " such as Redis; a per-process or disabled cache cannot work." + ) + abort_with_message( + 500, + "Login could not be completed because the server did not retain the" + " pending tokens. The persistent cache is most likely disabled, or" + " not shared between workers.", + ) + + persistent_cache.delete(key) + return entry class OIDCLoginQueryArgs(Schema): @@ -318,47 +374,22 @@ def get(self, args, provider_id=None): oidc_provider=provider_id, ) - # Redirect to frontend with secure HTTP-only cookies - frontend_url = get_config("FRONTEND_URL") or get_config("BASE_URL") - response = redirect(f"{frontend_url.rstrip('/')}/oidc/complete") - - secure = _use_secure_cookies() - - response.set_cookie( - "oidc_access_token", - tokens["access_token"], - max_age=300, # 5 minutes - httponly=True, - secure=secure, - samesite="Lax", - path="/", - ) - response.set_cookie( - "oidc_refresh_token", - tokens["refresh_token"], - max_age=300, # 5 minutes - httponly=True, - secure=secure, - samesite="Lax", - path="/", - ) - - # Store id_token if available (needed for OIDC logout) + exchange_tokens = { + "access_token": tokens["access_token"], + "refresh_token": tokens["refresh_token"], + } + # Keep the id_token around, it is needed as id_token_hint on logout. if token.get("id_token"): - response.set_cookie( - "oidc_id_token", - token["id_token"], - max_age=300, # 5 minutes - httponly=True, - secure=secure, - samesite="Lax", - path="/", - ) + exchange_tokens["id_token"] = token["id_token"] - logger.debug( - f"Set OIDC cookies, redirecting to {frontend_url}/oidc/complete" - ) - return response + code = _store_exchange_code(exchange_tokens) + + # In the fragment, which is not sent to the frontend's web server. + frontend_url = get_config("FRONTEND_URL") or get_config("BASE_URL") + complete_url = f"{frontend_url.rstrip('/')}/oidc/complete#code={code}" + + logger.debug(f"Redirecting to {frontend_url}/oidc/complete with code") + return redirect(complete_url) except ValueError as e: logger.exception( @@ -368,52 +399,27 @@ def get(self, args, provider_id=None): class OIDCTokenExchangeResource(Resource): - """Resource for securely exchanging OIDC tokens from cookies.""" + """Resource for exchanging a single-use OIDC code for tokens.""" @api_blueprint.response(200, OIDCTokensSchema()) + @api_blueprint.arguments(OIDCTokenExchangeSchema, location="json") @limiter.limit("10/minute") - def get(self): - """Exchange HTTP-only cookies for tokens that can be stored in localStorage.""" - # Get tokens from HTTP-only cookies - access_token = request.cookies.get("oidc_access_token") - refresh_token = request.cookies.get("oidc_refresh_token") - id_token = request.cookies.get("oidc_id_token") - - if not access_token or not refresh_token: - logger.warning( - "OIDC token exchange with no tokens in cookies; " - f"cookies present: {sorted(request.cookies)}" - ) - abort_with_message(400, "No OIDC tokens found in cookies") + def post(self, args): + """Exchange the code from the login redirect for tokens.""" + tokens = _redeem_exchange_code(args["code"]) - # Return tokens and clear cookies response_data = { - "access_token": access_token, - "refresh_token": refresh_token, + "access_token": tokens["access_token"], + "refresh_token": tokens["refresh_token"], "token_type": "Bearer", } # Include id_token if available (needed for OIDC logout) - if id_token: - response_data["id_token"] = id_token - - response = jsonify(response_data) - - # Clear the temporary cookies with same settings as when they were set - secure = _use_secure_cookies() - for name in ("oidc_access_token", "oidc_refresh_token", "oidc_id_token"): - response.set_cookie( - name, - "", - expires=0, - httponly=True, - secure=secure, - samesite="Lax", - path="/", - ) + if tokens.get("id_token"): + response_data["id_token"] = tokens["id_token"] - logger.debug("OIDC token exchange successful, cookies cleared") - return response + logger.debug("OIDC token exchange successful, code consumed") + return response_data class OIDCConfigResource(Resource): diff --git a/gramps_webapi/api/resources/schemas.py b/gramps_webapi/api/resources/schemas.py index a1b15a81..9e476cea 100644 --- a/gramps_webapi/api/resources/schemas.py +++ b/gramps_webapi/api/resources/schemas.py @@ -1741,8 +1741,20 @@ class OIDCConfigSchema(_Base): ) +class OIDCTokenExchangeSchema(_Base): + """Body of a POST to the /oidc/tokens/ exchange endpoint.""" + + code = fields.Str( + required=True, + metadata={ + "description": "Single-use code from the fragment of the OIDC login" + " redirect." + }, + ) + + class OIDCTokensSchema(_Base): - """Tokens returned by the /oidc/tokens/ cookie exchange endpoint.""" + """Tokens returned by the /oidc/tokens/ code exchange endpoint.""" access_token = fields.Str( metadata={"description": "JWT access token."}, diff --git a/gramps_webapi/app.py b/gramps_webapi/app.py index 41cd7fa3..3a0f11e0 100644 --- a/gramps_webapi/app.py +++ b/gramps_webapi/app.py @@ -214,7 +214,7 @@ def create_app(config: Optional[Dict[str, Any]] = None, config_from_env: bool = resources={ f"{API_PREFIX}/*": { "origins": app.config["CORS_ORIGINS"], - "supports_credentials": True, + "supports_credentials": False, } }, ) diff --git a/tests/test_endpoints/test_oidc.py b/tests/test_endpoints/test_oidc.py index eb2dc302..152e9b65 100644 --- a/tests/test_endpoints/test_oidc.py +++ b/tests/test_endpoints/test_oidc.py @@ -24,6 +24,8 @@ from flask import redirect +from gramps_webapi.api.cache import persistent_cache + from . import BASE_URL, get_single_tree_test_client, get_test_client @@ -332,32 +334,18 @@ def test_oidc_callback_success( mock_create_user.assert_called_once_with(mock_userinfo, None, "custom") mock_get_tokens.assert_called_once() - # Verify redirect location + # Verify redirect location carries an exchange code self.assertIn("/oidc/complete", rv.location) - - # Verify cookies are set - set_cookie_headers = rv.headers.getlist("Set-Cookie") - self.assertTrue( - any("oidc_access_token" in cookie for cookie in set_cookie_headers) - ) - self.assertTrue( - any("oidc_refresh_token" in cookie for cookie in set_cookie_headers) + self.assertIn("#code=", rv.location) + + # The tokens themselves must not travel back to the browser. + # The session cookie may well be set, it carries no tokens. + cookies = rv.headers.getlist("Set-Cookie") + self.assertFalse([c for c in cookies if c.startswith("oidc_")]) + self.assertNotIn( + mock_get_tokens.return_value["refresh_token"], rv.location ) - # Verify HttpOnly flag is set - access_token_cookie = next( - cookie - for cookie in set_cookie_headers - if "oidc_access_token" in cookie - ) - refresh_token_cookie = next( - cookie - for cookie in set_cookie_headers - if "oidc_refresh_token" in cookie - ) - self.assertIn("HttpOnly", access_token_cookie) - self.assertIn("HttpOnly", refresh_token_cookie) - @patch("gramps_webapi.api.resources.oidc.is_oidc_enabled", return_value=True) @patch( "gramps_webapi.api.resources.oidc.get_available_oidc_providers", @@ -786,6 +774,10 @@ def test_tree_survives_the_round_trip( self.assertIn("/oidc/complete", rv.location) # the tree from the login request was handed to user creation self.assertEqual(mock_create_user.call_args[0][1], "the_tree") + # popping the tree dirties the session, so a session cookie is set here + # - but still never a token cookie + cookies = rv.headers.getlist("Set-Cookie") + self.assertFalse([c for c in cookies if c.startswith("oidc_")]) @patch("gramps_webapi.api.resources.oidc.is_oidc_enabled", return_value=True) @patch( @@ -943,20 +935,23 @@ def test_login_with_another_tree_is_refused( self.assertEqual(rv.status_code, 422) -class TestOIDCCookies(unittest.TestCase): - """Test cases for the temporary token cookies and their exchange.""" +class TestOIDCCodeExchange(unittest.TestCase): + """Test cases for the single-use exchange code and its redemption.""" @classmethod def setUpClass(cls): """Test class setup.""" cls.client = get_test_client() - def _run_callback(self, frontend_url): - """Run a successful callback and return the Set-Cookie headers.""" + def _run_callback(self, frontend_url, id_token=None): + """Run a successful callback and return the exchange code.""" mock_oauth = MagicMock() mock_oidc_client = MagicMock() mock_oauth.gramps_custom = mock_oidc_client - mock_oidc_client.authorize_access_token.return_value = {"access_token": "t"} + provider_token = {"access_token": "t"} + if id_token: + provider_token["id_token"] = id_token + mock_oidc_client.authorize_access_token.return_value = provider_token mock_oidc_client.userinfo.return_value = {"sub": "user123"} with ( @@ -995,46 +990,60 @@ def _run_callback(self, frontend_url): BASE_URL + "/oidc/callback/custom?code=auth_code&tree=t" ) self.assertEqual(rv.status_code, 302) - return rv.headers.getlist("Set-Cookie") - - def test_cookies_are_secure_over_https(self): - """Tokens must never be sent without the Secure flag over HTTPS.""" - cookies = self._run_callback("https://app.example.com") - token_cookies = [c for c in cookies if c.startswith("oidc_")] - self.assertTrue(token_cookies) - for cookie in token_cookies: - self.assertIn("Secure", cookie) - self.assertIn("HttpOnly", cookie) - - def test_cookies_are_not_secure_over_plain_http(self): - """A Secure cookie would be dropped by the browser over plain HTTP.""" - cookies = self._run_callback("http://localhost:5000") - token_cookies = [c for c in cookies if c.startswith("oidc_")] - self.assertTrue(token_cookies) - for cookie in token_cookies: - self.assertNotIn("Secure", cookie) - - def test_token_exchange_returns_and_clears_cookies(self): - """The frontend trades the HttpOnly cookies for tokens exactly once.""" - self.client.set_cookie("oidc_access_token", "access-1", domain="localhost") - self.client.set_cookie("oidc_refresh_token", "refresh-1", domain="localhost") - - rv = self.client.get(BASE_URL + "/oidc/tokens/") + return rv.location.partition("#code=")[2] + + def test_code_is_redeemed_exactly_once(self): + """A replayed code must not yield a second set of tokens.""" + code = self._run_callback("https://app.example.com") + + rv = self.client.post(BASE_URL + "/oidc/tokens/", json={"code": code}) self.assertEqual(rv.status_code, 200) data = rv.get_json() - self.assertEqual(data["access_token"], "access-1") - self.assertEqual(data["refresh_token"], "refresh-1") + self.assertEqual(data["access_token"], "a") + self.assertEqual(data["refresh_token"], "r") self.assertEqual(data["token_type"], "Bearer") - # the cookies are cleared, so a replay finds nothing - rv = self.client.get(BASE_URL + "/oidc/tokens/") + rv = self.client.post(BASE_URL + "/oidc/tokens/", json={"code": code}) self.assertEqual(rv.status_code, 400) + self.assertIn("already been used", rv.get_json()["error"]["message"]) + + def test_id_token_survives_the_exchange(self): + """The id_token is needed later as id_token_hint on logout.""" + code = self._run_callback("https://app.example.com", id_token="id-1") + rv = self.client.post(BASE_URL + "/oidc/tokens/", json={"code": code}) + self.assertEqual(rv.get_json()["id_token"], "id-1") + + def test_unsigned_code_is_rejected(self): + """A guessed code carries no signature and cannot name a cache entry.""" + rv = self.client.post( + BASE_URL + "/oidc/tokens/", json={"code": "not-a-real-code"} + ) + self.assertEqual(rv.status_code, 400) + self.assertIn("Invalid", rv.get_json()["error"]["message"]) - def test_token_exchange_without_cookies(self): - """Exchange with no cookies present is a client error, not a crash.""" - rv = self.client.get(BASE_URL + "/oidc/tokens/") + def test_expired_code_is_reported_as_expired(self): + """Age is read from the signature, not from whether the entry survived.""" + code = self._run_callback("https://app.example.com") + with patch( + "gramps_webapi.api.resources.oidc.OIDC_CODE_TIMEOUT", -1 + ): + rv = self.client.post(BASE_URL + "/oidc/tokens/", json={"code": code}) self.assertEqual(rv.status_code, 400) - self.assertIn("No OIDC tokens found", rv.get_json()["error"]["message"]) + self.assertIn("expired", rv.get_json()["error"]["message"]) + + def test_lost_entry_names_the_unshared_cache(self): + """A valid, unexpired code with no entry means the cache is not shared. + + This is what a per-worker cache looks like from the worker that did not + handle the callback, and it must not be reported as an expired code. + """ + code = self._run_callback("https://app.example.com") + with self.client.application.app_context(): + persistent_cache.clear() + + rv = self.client.post(BASE_URL + "/oidc/tokens/", json={"code": code}) + self.assertEqual(rv.status_code, 500) + self.assertIn("not shared", rv.get_json()["error"]["message"]) class TestOIDCLogoutEndpoint(unittest.TestCase):