diff --git a/application/frontend/src/hooks/useResourceSelection.test.ts b/application/frontend/src/hooks/useResourceSelection.test.ts index a7b39ee32..26d353ffd 100644 --- a/application/frontend/src/hooks/useResourceSelection.test.ts +++ b/application/frontend/src/hooks/useResourceSelection.test.ts @@ -44,7 +44,10 @@ describe('useResourceSelection', () => { const { getByTestId } = render(React.createElement(Probe)); await waitFor(() => expect(getByTestId('loading').textContent).toBe('false')); - expect(fetchMock).toHaveBeenCalledWith('/rest/v1/user/resources', { method: 'GET' }); + expect(fetchMock).toHaveBeenCalledWith('/rest/v1/user/resources', { + method: 'GET', + headers: { Accept: 'application/json' }, + }); expect(getByTestId('selected').textContent).toBe('ASVS,CWE'); }); diff --git a/application/frontend/src/hooks/useResourceSelection.ts b/application/frontend/src/hooks/useResourceSelection.ts index 0f6a90d47..77d5919c9 100644 --- a/application/frontend/src/hooks/useResourceSelection.ts +++ b/application/frontend/src/hooks/useResourceSelection.ts @@ -29,7 +29,10 @@ export const useResourceSelection = (): ResourceSelectionState => { const load = async () => { try { - const res = await fetch(`${apiUrl}/user/resources`, { method: 'GET' }); + const res = await fetch(`${apiUrl}/user/resources`, { + method: 'GET', + headers: { Accept: 'application/json' }, + }); if (res.status === 401) { return; // anonymous / feature not available — not an error } diff --git a/application/frontend/src/hooks/useUser.test.ts b/application/frontend/src/hooks/useUser.test.ts new file mode 100644 index 000000000..3f1245a70 --- /dev/null +++ b/application/frontend/src/hooks/useUser.test.ts @@ -0,0 +1,66 @@ +import { act, render, waitFor } from '@testing-library/react'; +import React from 'react'; + +import { useUser } from './useUser'; + +jest.mock('./useEnvironment', () => ({ + useEnvironment: () => ({ name: 'test', apiUrl: '/rest/v1' }), +})); + +// react-testing-library v11 has no renderHook; drive the hook via a probe. +type Captured = ReturnType; +let captured: Captured; + +function Probe(): React.ReactElement { + captured = useUser(); + return React.createElement('span', { 'data-testid': 'loading' }, String(captured.loading)); +} + +function jsonResponse(body: unknown, status = 200): Response { + return { + status, + ok: status >= 200 && status < 300, + json: () => Promise.resolve(body), + text: () => Promise.resolve(typeof body === 'string' ? body : JSON.stringify(body)), + } as unknown as Response; +} + +describe('useUser (auth route migration #963)', () => { + const originalLocation = window.location; + + beforeEach(() => { + delete (window as any).location; + (window as any).location = { href: '' }; + }); + + afterEach(() => { + (window as any).location = originalLocation; + jest.resetAllMocks(); + }); + + it('GETs /rest/v1/auth/user with Accept: application/json (so anon gets 401, not a Google redirect)', async () => { + const fetchMock = jest.fn().mockResolvedValueOnce(jsonResponse(null, 401)); + (global as any).fetch = fetchMock; + + const { getByTestId } = render(React.createElement(Probe)); + await waitFor(() => expect(getByTestId('loading').textContent).toBe('false')); + + expect(fetchMock).toHaveBeenCalledWith('/rest/v1/auth/user', { + method: 'GET', + headers: { Accept: 'application/json' }, + }); + }); + + it('login() navigates to /auth/login and logout() to /auth/logout', async () => { + (global as any).fetch = jest.fn().mockResolvedValueOnce(jsonResponse(null, 401)); + + const { getByTestId } = render(React.createElement(Probe)); + await waitFor(() => expect(getByTestId('loading').textContent).toBe('false')); + + act(() => captured.login()); + expect((window as any).location.href).toBe('/rest/v1/auth/login'); + + act(() => captured.logout()); + expect((window as any).location.href).toBe('/rest/v1/auth/logout'); + }); +}); diff --git a/application/frontend/src/hooks/useUser.ts b/application/frontend/src/hooks/useUser.ts index 0575fe365..91a967e55 100644 --- a/application/frontend/src/hooks/useUser.ts +++ b/application/frontend/src/hooks/useUser.ts @@ -15,7 +15,7 @@ export const useUser = () => { useEffect(() => { let active = true; - fetch(`${apiUrl}/user`, { method: 'GET' }) + fetch(`${apiUrl}/auth/user`, { method: 'GET', headers: { Accept: 'application/json' } }) .then((res) => { if (res.status === 200) { return res.text(); @@ -51,11 +51,11 @@ export const useUser = () => { }, [apiUrl]); const login = () => { - window.location.href = `${apiUrl}/login`; + window.location.href = `${apiUrl}/auth/login`; }; const logout = () => { - window.location.href = `${apiUrl}/logout`; + window.location.href = `${apiUrl}/auth/logout`; }; return { user, isLoggedIn: user !== null, loading, login, logout }; diff --git a/application/frontend/src/pages/chatbot/chatbot.tsx b/application/frontend/src/pages/chatbot/chatbot.tsx index 698ae0128..79a175632 100644 --- a/application/frontend/src/pages/chatbot/chatbot.tsx +++ b/application/frontend/src/pages/chatbot/chatbot.tsx @@ -88,12 +88,12 @@ export const Chatbot = () => { }, [chatMessages]); function login() { - fetch(`${apiUrl}/user`, { method: 'GET' }) + fetch(`${apiUrl}/auth/user`, { method: 'GET', headers: { Accept: 'application/json' } }) .then((response) => { if (response.status === 200) { response.text().then((user) => setUser(user)); } else { - window.location.href = `${apiUrl}/login`; + window.location.href = `${apiUrl}/auth/login`; } }) .catch((error) => { @@ -156,7 +156,7 @@ export const Chatbot = () => { fetch(`${apiUrl}/completion`, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, body: JSON.stringify({ prompt: currentTerm }), }) .then(async (response) => { diff --git a/application/tests/admin_imports_api_test.py b/application/tests/admin_imports_api_test.py index 718982bba..26d9cbc69 100644 --- a/application/tests/admin_imports_api_test.py +++ b/application/tests/admin_imports_api_test.py @@ -94,6 +94,28 @@ def test_admin_imports_disabled_flag_returns_404(self) -> None: os.environ, {"CRE_ALLOW_IMPORT": "1", "INSECURE_REQUESTS": "1"}, clear=True ) def test_admin_imports_requires_login(self) -> None: + with self.app.test_client() as c: + # API client (Accept: application/json) gets a 401; browsers (Accept: + # text/html) get a 302 to the login flow (login_required content + # negotiation, #963 — default is now 401). + r = c.get("/admin/imports/runs", headers={"Accept": "application/json"}) + self.assertEqual(r.status_code, 401) + + @patch.dict( + os.environ, {"CRE_ALLOW_IMPORT": "1", "INSECURE_REQUESTS": "1"}, clear=True + ) + def test_admin_imports_star_accept_returns_401(self) -> None: + # /admin/* tooling with curl's default Accept "*/*" must get a clean 401, + # not a 302 into login HTML (the case Spyros called out). + with self.app.test_client() as c: + r = c.get("/admin/imports/runs", headers={"Accept": "*/*"}) + self.assertEqual(r.status_code, 401) + + @patch.dict( + os.environ, {"CRE_ALLOW_IMPORT": "1", "INSECURE_REQUESTS": "1"}, clear=True + ) + def test_admin_imports_no_accept_header_returns_401(self) -> None: + # /admin/* tooling with no Accept header at all -> 401, not 302. with self.app.test_client() as c: r = c.get("/admin/imports/runs") self.assertEqual(r.status_code, 401) diff --git a/application/tests/auth_routes_test.py b/application/tests/auth_routes_test.py new file mode 100644 index 000000000..9d6f8f059 --- /dev/null +++ b/application/tests/auth_routes_test.py @@ -0,0 +1,353 @@ +"""Tests for the /rest/v1/auth/* migration (issue #963, RFC #876 TODO 1). + +Canonical auth routes, deprecated aliases (header-only), login_required content +negotiation (browser 302 vs JSON 401), the user_id-keyed session predicate, and +the NO_LOGIN dev bypass. OpenAPI documentation is intentionally out of scope here. +""" + +import os +import unittest +from typing import Any +from unittest.mock import patch + +from application import create_app, sqla +from application.database import db + + +class TestAuthRoutes(unittest.TestCase): + def setUp(self) -> None: + self._prev_no_load_graph = os.environ.get("NO_LOAD_GRAPH_DB") + os.environ["NO_LOAD_GRAPH_DB"] = "1" + self.app = create_app(mode="test") + self.app.secret_key = "test-secret" + self.app_context = self.app.app_context() + self.app_context.push() + sqla.create_all() + + def tearDown(self) -> None: + sqla.session.remove() + sqla.drop_all() + self.app_context.pop() + if self._prev_no_load_graph is None: + os.environ.pop("NO_LOAD_GRAPH_DB", None) + else: + os.environ["NO_LOAD_GRAPH_DB"] = self._prev_no_load_graph + + # --- canonical routes --- + def test_auth_logout_clears_session_and_redirects(self) -> None: + with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = "uid" + resp = client.get("/rest/v1/auth/logout") + self.assertEqual(resp.status_code, 302) + self.assertTrue(resp.headers["Location"].endswith("/")) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + + def test_auth_login_dev_bypass_sets_session(self) -> None: + with patch.dict( + os.environ, + {"NO_LOGIN": "1", "CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"}, + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/login") + self.assertEqual(resp.status_code, 302) + self.assertTrue(resp.headers["Location"].endswith("/chatbot")) + with client.session_transaction() as sess: + self.assertIn("user_id", sess) + + def test_auth_user_returns_email_when_logged_in(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = "uid" + sess["email"] = "e@x.com" + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data.decode(), "e@x.com") + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_auth_callback_sets_user_id( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + id_token_mock.verify_oauth2_token.return_value = { + "sub": "sub-xyz", + "name": "Test User", + "email": "test@example.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + client.get("/rest/v1/auth/callback?state=xyz") + with client.session_transaction() as sess: + self.assertIn("user_id", sess) + self.assertEqual(sqla.session.query(db.User).count(), 1) + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_auth_callback_state_mismatch_returns_without_continuing( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + # Regression for #1021: missing ``return`` on the state-mismatch redirect + # let the handler continue into token verification / session writes. + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "expected-state" + resp = client.get("/rest/v1/auth/callback?state=wrong-state") + self.assertEqual(resp.status_code, 302) + self.assertIn("/rest/v1/auth/login", resp.headers.get("Location", "")) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + self.assertNotIn("google_id", sess) + id_token_mock.verify_oauth2_token.assert_not_called() + self.assertEqual(sqla.session.query(db.User).count(), 0) + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + @patch("application.web.web_main.db.Node_collection") + def test_auth_callback_persistence_failure_returns_503( + self, node_collection_mock: Any, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + # If upsert_user fails, we must NOT redirect as if logged in (that leaves + # a broken session that fails every login_required call). Surface a + # retryable 503 and leave user_id unset. + from sqlalchemy.exc import SQLAlchemyError + + id_token_mock.verify_oauth2_token.return_value = { + "sub": "sub-boom", + "name": "T", + "email": "t@example.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + node_collection_mock.return_value.upsert_user.side_effect = SQLAlchemyError( + "db down" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + resp = client.get("/rest/v1/auth/callback?state=xyz") + self.assertEqual(resp.status_code, 503) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_auth_callback_missing_sub_returns_401( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + # No 'sub' claim -> identity can't be established -> explicit 401, not a + # silently broken session. + id_token_mock.verify_oauth2_token.return_value = { + "sub": None, + "name": "T", + "email": "t@example.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + resp = client.get("/rest/v1/auth/callback?state=xyz") + self.assertEqual(resp.status_code, 401) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + + # --- deprecated aliases: header-only --- + def test_logout_alias_carries_deprecation_header(self) -> None: + with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): + with self.app.test_client() as client: + resp = client.get("/rest/v1/logout") + self.assertEqual(resp.status_code, 302) + self.assertEqual(resp.headers.get("Deprecation"), "true") + self.assertIn("/rest/v1/auth/logout", resp.headers.get("Link", "")) + self.assertIn("successor-version", resp.headers.get("Link", "")) + + def test_user_alias_carries_deprecation_header(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = "uid" + sess["email"] = "e@x.com" + resp = client.get( + "/rest/v1/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.headers.get("Deprecation"), "true") + self.assertIn("/rest/v1/auth/user", resp.headers.get("Link", "")) + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_callback_alias_carries_deprecation_header( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + id_token_mock.verify_oauth2_token.return_value = { + "sub": "sub-abc", + "name": "T", + "email": "t@x.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + resp = client.get("/rest/v1/callback?state=xyz") + # Header-only: not a redirect to the canonical path (still runs + # the OAuth flow and lands on /chatbot). + self.assertEqual(resp.headers.get("Deprecation"), "true") + self.assertIn("/rest/v1/auth/callback", resp.headers.get("Link", "")) + + # --- login_required content negotiation --- + def test_login_required_json_returns_401(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 401) + + def test_login_required_browser_redirects_to_auth_login(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/user", headers={"Accept": "text/html"}) + self.assertEqual(resp.status_code, 302) + # Redirect to the constant login route -- no ?next (auth_login does + # not consume a return target; callback always lands on /chatbot). + self.assertTrue( + resp.headers["Location"].endswith("/rest/v1/auth/login") + ) + self.assertNotIn("next=", resp.headers["Location"]) + + def test_login_required_browser_multivalue_accept_redirects(self) -> None: + # A real browser sends a multi-value Accept; "text/html" is present as a + # substring, so the browser still gets the 302 (not an exact-match check). + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get( + "/rest/v1/auth/user", + headers={ + "Accept": ( + "text/html,application/xhtml+xml," + "application/xml;q=0.9,*/*;q=0.8" + ) + }, + ) + self.assertEqual(resp.status_code, 302) + self.assertTrue( + resp.headers["Location"].endswith("/rest/v1/auth/login") + ) + + def test_login_required_star_accept_returns_401(self) -> None: + # curl's default Accept is "*/*": tooling must get a clean 401, NOT a 302 + # into login HTML. This is the key assertion of the inverted default. + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/user", headers={"Accept": "*/*"}) + self.assertEqual(resp.status_code, 401) + + def test_login_required_no_accept_header_returns_401(self) -> None: + # No Accept header at all (scripts/requests without one) -> 401, not 302. + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/user") + self.assertEqual(resp.status_code, 401) + + # --- the re-baselined predicate: keyed on user_id, not google_id+name --- + def test_session_with_google_id_but_no_user_id_is_anonymous(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["google_id"] = "sub-1" + sess["name"] = "U" # but NO user_id + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 401) + + def test_completion_anonymous_json_returns_401(self) -> None: + # /rest/v1/completion is login_required; an anonymous API client + # (Accept: application/json) must get a clean 401, not the browser 302 + # toward Google (which the chatbot's fetch would fail to follow). + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.post( + "/rest/v1/completion", + json={"prompt": "x"}, + headers={"Accept": "application/json"}, + ) + self.assertEqual(resp.status_code, 401) + + # --- NO_LOGIN dev bypass preserved --- + def test_no_login_bypasses_login_required(self) -> None: + with patch.dict(os.environ, {"NO_LOGIN": "1", "INSECURE_REQUESTS": "1"}): + with self.app.test_client() as client: + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data.decode(), "foobar") + + +if __name__ == "__main__": + unittest.main() diff --git a/application/tests/user_resources_api_test.py b/application/tests/user_resources_api_test.py index f8eb019cf..a1ccc1a0b 100644 --- a/application/tests/user_resources_api_test.py +++ b/application/tests/user_resources_api_test.py @@ -39,7 +39,13 @@ def tearDown(self) -> None: os.environ["NO_LOAD_GRAPH_DB"] = self._prev_no_load_graph def _login(self, client: Any, google_sub: str = "sub-1", name: str = "U") -> None: + # login_required keys off session['user_id'] (post-#980, #963), so a test + # session must carry it. Resolve/create the user and record its id. + user = self.collection.upsert_user( + google_sub=google_sub, email="", display_name=name + ) with client.session_transaction() as sess: + sess["user_id"] = user.id sess["google_id"] = google_sub sess["name"] = name @@ -74,7 +80,10 @@ def test_get_401_when_anonymous(self) -> None: }, ): with self.app.test_client() as client: - resp = client.get("/rest/v1/user/resources") + resp = client.get( + "/rest/v1/user/resources", + headers={"Accept": "application/json"}, + ) self.assertEqual(resp.status_code, 401) def test_put_401_when_anonymous(self) -> None: @@ -88,7 +97,9 @@ def test_put_401_when_anonymous(self) -> None: ): with self.app.test_client() as client: resp = client.put( - "/rest/v1/user/resources", json={"selected": ["ASVS"]} + "/rest/v1/user/resources", + json={"selected": ["ASVS"]}, + headers={"Accept": "application/json"}, ) self.assertEqual(resp.status_code, 401) diff --git a/application/web/openapi_registry.py b/application/web/openapi_registry.py index 7a65b0239..4d2677e44 100644 --- a/application/web/openapi_registry.py +++ b/application/web/openapi_registry.py @@ -21,6 +21,13 @@ OPENAPI_GUARDRAIL_EXEMPT_RULES: Set[str] = { "/rest/v1/openapi.yaml", + # Canonical OAuth/auth routes (#963). Auth/redirect endpoints, not part of + # the documented read-only public API -- exempt rather than documented. + "/rest/v1/auth/login", + "/rest/v1/auth/callback", + "/rest/v1/auth/logout", + "/rest/v1/auth/user", + # Deprecated pre-#963 aliases of the above (header-only successor links). "/rest/v1/login", "/rest/v1/callback", "/rest/v1/logout", diff --git a/application/web/web_main.py b/application/web/web_main.py index 604e75e9b..a40b5fa7d 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -35,6 +35,7 @@ Blueprint, abort, jsonify, + make_response, redirect, request, send_from_directory, @@ -882,19 +883,49 @@ def add_header(response): return response +def _is_logged_in() -> bool: + """Single source of truth for session presence. + + Keyed on ``session['user_id']`` (recorded by the login flow since #980) — not + ``google_id``/``name``. Both ``login_required`` and its content-negotiation + branch route through here, so the predicate lives in exactly one place. + """ + return "user_id" in session + + +def _auth_challenge(): + """Response for an unauthenticated request, negotiated by Accept. + + Default is a clean 401 so tooling (curl/requests/scripts, a bare ``*/*`` or + a missing Accept header) and ``/admin/*`` tooling get a machine-readable + challenge instead of being 302'd into login HTML. Only real browsers -- which + advertise ``text/html`` in Accept (e.g. + ``text/html,application/xhtml+xml,...;q=0.9,*/*;q=0.8``) -- get the 302 to the + login flow. Matched as a substring, so a bare ``*/*`` does not qualify. + + The redirect target is the constant login route with no ``?next`` param: + ``auth_login`` does not consume a return target and ``auth_callback`` always + lands on ``/chatbot``, so forwarding the request path here would be a dead + (and taint-flagged) value. Return-to-page is a separate, future feature that + would store a validated target in the session at login time. + """ + if "text/html" in request.headers.get("Accept", ""): + return redirect("/rest/v1/auth/login") + allowed_domains = os.environ.get("LOGIN_ALLOWED_DOMAINS") + abort( + 401, + description=f"You need an account with one of the following providers to access this functionality {allowed_domains}", + ) + + def login_required(f): @wraps(f) def login_r(*args, **kwargs): if os.environ.get("NO_LOGIN") == "1": return f(*args, **kwargs) - if "google_id" not in session or "name" not in session: - allowed_domains = os.environ.get("LOGIN_ALLOWED_DOMAINS") - abort( - 401, - description=f"You need an account with one of the following providers to access this functionality {allowed_domains}", - ) - else: - return f(*args, **kwargs) + if not _is_logged_in(): + return _auth_challenge() + return f(*args, **kwargs) return login_r @@ -1232,7 +1263,7 @@ def instance(cls): "openid", ], redirect_uri=( - request.root_url.rstrip("/") + url_for("web.callback") + request.root_url.rstrip("/") + url_for("web.auth_callback") ).replace("http://", "https://"), ) return cls.__instance @@ -1241,8 +1272,8 @@ def __init__(sel): raise ValueError("class is a singleton, please call instance() instead") -@app.route("/rest/v1/login") -def login(): +@app.route("/rest/v1/auth/login") +def auth_login(): if os.environ.get("NO_LOGIN") == "1": session["state"] = {"state": True} session["google_id"] = "some dev id" @@ -1269,16 +1300,16 @@ def login(): return redirect(authorization_url) -@app.route("/rest/v1/user") +@app.route("/rest/v1/auth/user") @login_required -def logged_in_user(): +def auth_user(): if os.environ.get("NO_LOGIN") == "1": return "foobar" return session.get("email") -@app.route("/rest/v1/callback") -def callback(): +@app.route("/rest/v1/auth/callback") +def auth_callback(): flow_instance = CREFlow.instance() try: flow_instance.flow.fetch_token( @@ -1287,7 +1318,7 @@ def callback(): except oauthlib.oauth2.rfc6749.errors.MismatchingStateError as mse: return redirect("/chatbot") if not session.get("state") or session.get("state") != request.args["state"]: - redirect(url_for("web.login")) # State does not match! + return redirect(url_for("web.auth_login")) # State does not match! credentials = flow_instance.flow.credentials token_request = google.auth.transport.requests.Request() id_info = id_token.verify_oauth2_token( @@ -1317,37 +1348,76 @@ def callback(): description=f"You need an account with one of the following providers to access this functionality {allowed_domains}", ) - # Persist the account when login is enabled; the session keeps working - # unchanged if this no-ops (flag off) or fails. + # Persist the account when login is enabled. ``session['user_id']`` is what + # ``_is_logged_in`` checks, so if persistence cannot establish it we must NOT + # redirect as if login succeeded: that would leave a "logged-in-looking" + # session that fails every ``login_required`` call and bounces the user back + # into the login flow. Fail explicitly (retryable) instead. if is_login_enabled(): google_sub = id_info.get("sub") if not google_sub: logger.error( - "OIDC callback returned no 'sub' claim; skipping user persistence" + "OIDC callback returned no 'sub' claim; cannot establish session" + ) + abort( + 401, description="Login failed: identity provider returned no subject" + ) + try: + user = db.Node_collection().upsert_user( + google_sub=google_sub, + email=id_info.get("email") or "", + display_name=id_info.get("name"), + ) + session["user_id"] = user.id + except SQLAlchemyError as e: + # Log only the exception class: the message can carry SQL parameters + # such as the user's email or OIDC subject. Surface a retryable 503 + # rather than a silently broken session. + logger.error("failed to persist user on login: %s", type(e).__name__) + abort( + 503, + description="Login temporarily unavailable, please try again", ) - else: - try: - user = db.Node_collection().upsert_user( - google_sub=google_sub, - email=id_info.get("email") or "", - display_name=id_info.get("name"), - ) - session["user_id"] = user.id - except SQLAlchemyError as e: - # Keep DB failures soft so persistence can never block login, but - # let unexpected (non-DB) bugs surface instead of being swallowed. - # Log only the exception class: the message can carry SQL - # parameters such as the user's email or OIDC subject. - logger.error("failed to persist user on login: %s", type(e).__name__) return redirect("/chatbot") -@app.route("/rest/v1/logout") -def logout(): +@app.route("/rest/v1/auth/logout") +def auth_logout(): session.clear() return redirect("/") +# --- Deprecated auth aliases (RFC #876 TODO 1 migration) -------------------- +# Keep the old paths working during the migration but signal deprecation. These +# are header-only (no redirect to the canonical path), so /callback's OAuth +# code/state flow is left untouched. +def _deprecated_auth(response, canonical): + resp = make_response(response) + resp.headers["Deprecation"] = "true" + resp.headers["Link"] = f'<{canonical}>; rel="successor-version"' + return resp + + +@app.route("/rest/v1/login") +def login(): + return _deprecated_auth(auth_login(), "/rest/v1/auth/login") + + +@app.route("/rest/v1/user") +def logged_in_user(): + return _deprecated_auth(auth_user(), "/rest/v1/auth/user") + + +@app.route("/rest/v1/callback") +def callback(): + return _deprecated_auth(auth_callback(), "/rest/v1/auth/callback") + + +@app.route("/rest/v1/logout") +def logout(): + return _deprecated_auth(auth_logout(), "/rest/v1/auth/logout") + + @openapi_documented("get_user_resources") @app.route("/rest/v1/user/resources", methods=["GET"]) @feature_enabled_or_default(