diff --git a/application/tests/user_resources_api_test.py b/application/tests/user_resources_api_test.py new file mode 100644 index 000000000..2d51f865e --- /dev/null +++ b/application/tests/user_resources_api_test.py @@ -0,0 +1,344 @@ +"""Tests for the per-user resource-selection API (issue #586, PR2). + +GET/PUT /rest/v1/user/resources, gated by login_required + is_login_enabled. +Flag-off returns a safe default; authenticated users read/write their selection. +""" + +import json +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 TestUserResourcesApi(unittest.TestCase): + def setUp(self) -> None: + # SQL-only surface; skip the Neo4j graph load and allow http in tests. + 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() + self.collection = db.Node_collection() + + def tearDown(self) -> None: + sqla.session.remove() + sqla.drop_all() + self.app_context.pop() + # Restore the prior value rather than unconditionally deleting it, so a + # value set by the test runner survives for later tests. + 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 + + def _login(self, client: Any, google_sub: str = "sub-1", name: str = "U") -> None: + with client.session_transaction() as sess: + sess["google_id"] = google_sub + sess["name"] = name + + # --- flag off -> safe default, no auth required, no writes --- + def test_get_returns_default_when_login_disabled(self) -> None: + with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): + os.environ.pop("CRE_ENABLE_LOGIN", None) + with self.app.test_client() as client: + resp = client.get("/rest/v1/user/resources") + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": []}) + + def test_put_noops_when_login_disabled(self) -> None: + with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): + os.environ.pop("CRE_ENABLE_LOGIN", None) + with self.app.test_client() as client: + resp = client.put( + "/rest/v1/user/resources", json={"selected": ["ASVS"]} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": []}) + self.assertEqual(sqla.session.query(db.UserResourceSelection).count(), 0) + + # --- flag on, anonymous -> 401 --- + def test_get_401_when_anonymous(self) -> None: + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/user/resources") + self.assertEqual(resp.status_code, 401) + + def test_put_401_when_anonymous(self) -> None: + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + resp = client.put( + "/rest/v1/user/resources", json={"selected": ["ASVS"]} + ) + self.assertEqual(resp.status_code, 401) + + # --- flag on, authenticated --- + def test_get_returns_saved_selection(self) -> None: + user = self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + self.collection.set_user_resource_selection(user.id, ["ASVS", "CWE"]) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + resp = client.get("/rest/v1/user/resources") + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": ["ASVS", "CWE"]}) + + def test_get_returns_empty_for_new_user(self) -> None: + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-new", "U") + resp = client.get("/rest/v1/user/resources") + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": []}) + + def test_put_persists_and_returns_selection(self) -> None: + self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + resp = client.put( + "/rest/v1/user/resources", json={"selected": ["CWE", "ASVS"]} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual( + sorted(json.loads(resp.data)["selected"]), ["ASVS", "CWE"] + ) + get = client.get("/rest/v1/user/resources") + self.assertEqual( + sorted(json.loads(get.data)["selected"]), ["ASVS", "CWE"] + ) + + def test_put_replaces_previous_selection(self) -> None: + user = self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + self.collection.set_user_resource_selection(user.id, ["ASVS", "CWE"]) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + client.put("/rest/v1/user/resources", json={"selected": ["SAMM"]}) + get = client.get("/rest/v1/user/resources") + self.assertEqual(json.loads(get.data)["selected"], ["SAMM"]) + + def test_put_dedupes_input(self) -> None: + self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + resp = client.put( + "/rest/v1/user/resources", + json={"selected": ["ASVS", "ASVS", "CWE"]}, + ) + self.assertEqual( + sorted(json.loads(resp.data)["selected"]), ["ASVS", "CWE"] + ) + + def test_put_trims_and_dedupes_whitespace_variants(self) -> None: + # " ASVS " and "ASVS" must normalize to a single stored entry, otherwise + # they'd persist as distinct rows and defeat the dedupe. + self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + resp = client.put( + "/rest/v1/user/resources", + json={"selected": [" ASVS ", "ASVS", "CWE "]}, + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual( + sorted(json.loads(resp.data)["selected"]), ["ASVS", "CWE"] + ) + self.assertEqual(sqla.session.query(db.UserResourceSelection).count(), 2) + + def test_put_400_on_invalid_body(self) -> None: + self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + self.assertEqual( + client.put( + "/rest/v1/user/resources", json={"foo": "bar"} + ).status_code, + 400, + ) + self.assertEqual( + client.put( + "/rest/v1/user/resources", json={"selected": "ASVS"} + ).status_code, + 400, + ) + self.assertEqual( + client.put( + "/rest/v1/user/resources", json={"selected": [1, 2]} + ).status_code, + 400, + ) + + # --- login on but myopencre off -> safe default, no writes --- + def test_get_returns_default_when_myopencre_disabled(self) -> None: + # Seed a real, non-empty selection. With myopencre off the endpoint must + # return the safe default [] instead of it, proving the gate short-circuits + # BEFORE reading the DB (an empty-user default would pass for the wrong + # reason). If the gate were bypassed, this would return ["ASVS", "CWE"]. + user = self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + self.collection.set_user_resource_selection(user.id, ["ASVS", "CWE"]) + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + os.environ.pop("CRE_ENABLE_MYOPENCRE", None) + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + resp = client.get("/rest/v1/user/resources") + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": []}) + + def test_put_noops_when_myopencre_disabled(self) -> None: + self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + os.environ.pop("CRE_ENABLE_MYOPENCRE", None) + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + resp = client.put( + "/rest/v1/user/resources", json={"selected": ["ASVS"]} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": []}) + self.assertEqual(sqla.session.query(db.UserResourceSelection).count(), 0) + + def test_responses_are_no_store(self) -> None: + # Per-user data must not be shared-cached: the global after_request sets + # max-age=300, so assert no-store wins for both GET and PUT and the + # max-age directive is gone. + self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + self._login(client, "sub-1", "U") + get = client.get("/rest/v1/user/resources") + self.assertTrue(get.cache_control.no_store) + self.assertIsNone(get.cache_control.max_age) + put = client.put("/rest/v1/user/resources", json={"selected": ["ASVS"]}) + self.assertTrue(put.cache_control.no_store) + self.assertIsNone(put.cache_control.max_age) + + def test_resolves_user_by_session_user_id_over_sub(self) -> None: + # session['user_id'] takes precedence over the OIDC sub. google_id is set + # to a DIFFERENT sub (login_required needs it present): if the endpoint + # resolved by sub it would create that other user and return [], so + # returning sub-1's selection proves user_id wins. + user = self.collection.upsert_user( + google_sub="sub-1", email="a@x.com", display_name="U" + ) + self.collection.set_user_resource_selection(user.id, ["ASVS", "CWE"]) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "CRE_ENABLE_MYOPENCRE": "1", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = user.id + sess["google_id"] = "sub-different" + sess["name"] = "U" + resp = client.get("/rest/v1/user/resources") + self.assertEqual(resp.status_code, 200) + self.assertEqual(json.loads(resp.data), {"selected": ["ASVS", "CWE"]}) + + +if __name__ == "__main__": + unittest.main() diff --git a/application/web/openapi_registry.py b/application/web/openapi_registry.py index 34ec2cc7c..438111f91 100644 --- a/application/web/openapi_registry.py +++ b/application/web/openapi_registry.py @@ -58,6 +58,7 @@ class PathSpec: "not_found", "extra_responses", "response_override", + "request_body", ) def __init__( @@ -74,6 +75,7 @@ def __init__( not_found: bool = True, extra_responses: Optional[Dict[str, Any]] = None, response_override: Optional[Dict[str, Any]] = None, + request_body: Optional[Dict[str, Any]] = None, ) -> None: self.path = path self.method = method.lower() @@ -86,6 +88,7 @@ def __init__( self.not_found = not_found self.extra_responses = extra_responses or {} self.response_override = response_override + self.request_body = request_body OPENAPI_PATHS: List[PathSpec] = [ @@ -301,6 +304,107 @@ def __init__( response_schema=schemas.ConfigResponseSchema, not_found=False, ), + PathSpec( + "/rest/v1/user/resources", + "get_user_resources", + tags=["User"], + summary="Get the current user's selected standards", + description=( + "Requires login (CRE_ENABLE_LOGIN) and the MyOpenCRE feature " + "(CRE_ENABLE_MYOPENCRE). When either flag is disabled the endpoint " + "does not authenticate and returns an empty selection. When both are " + "enabled, anonymous requests receive 401." + ), + not_found=False, + extra_responses={ + "401": { + "description": ( + "Not authenticated (both feature flags enabled and no active session)" + ) + } + }, + response_override={ + "200": { + "description": "The user's selected standards", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": ["selected"], + "properties": { + "selected": { + "type": "array", + "items": {"type": "string"}, + } + }, + } + } + }, + } + }, + ), + PathSpec( + "/rest/v1/user/resources", + "put_user_resources", + method="put", + tags=["User"], + summary="Replace the current user's selected standards", + description=( + "Requires login (CRE_ENABLE_LOGIN) and the MyOpenCRE feature " + "(CRE_ENABLE_MYOPENCRE). When either flag is disabled the request is a " + "no-op and returns an empty selection. When both are enabled, anonymous " + "requests receive 401." + ), + not_found=False, + extra_responses={ + "400": {"description": "Invalid selection body"}, + "401": { + "description": ( + "Not authenticated (both feature flags enabled and no active session)" + ) + }, + }, + request_body={ + "required": True, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": ["selected"], + "properties": { + "selected": { + "type": "array", + "items": {"type": "string", "minLength": 1}, + "description": ( + "Standard names to select. Non-empty strings; " + "values are trimmed and deduplicated." + ), + } + }, + } + } + }, + }, + response_override={ + "200": { + "description": "The stored selection", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": ["selected"], + "properties": { + "selected": { + "type": "array", + "items": {"type": "string"}, + } + }, + } + } + }, + } + }, + ), ] @@ -393,6 +497,9 @@ def _operation_from_path( if parameters: operation["parameters"] = parameters + if path_spec.request_body is not None: + operation["requestBody"] = path_spec.request_body + if path_spec.response_override is not None: responses = dict(path_spec.response_override) else: diff --git a/application/web/web_main.py b/application/web/web_main.py index bfa8d7930..dbac65eb1 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -866,6 +866,11 @@ def before_request(): @app.after_request def add_header(response): + # Per-user endpoints must never be shared-cached; no-store wins over the + # default max-age for them (this hook runs after the view). + if request.path == "/rest/v1/user/resources": + response.cache_control.no_store = True + return response response.cache_control.max_age = 300 return response @@ -887,6 +892,52 @@ def login_r(*args, **kwargs): return login_r +def feature_enabled_or_default(is_enabled: Any, default_factory: Any) -> Any: + """Gate a view on a feature predicate. + + When ``is_enabled()`` is false the wrapped view is skipped and + ``default_factory()`` is returned, so callers receive a safe default instead + of an auth error. When true the view runs (typically behind ``login_required``). + """ + + def decorator(f): + @wraps(f) + def wrapper(*args, **kwargs): + if not is_enabled(): + return default_factory() + return f(*args, **kwargs) + + return wrapper + + return decorator + + +def _resolve_current_user(database): + """Return the persisted User for the current session, creating it if absent. + + Prefers ``session['user_id']`` (recorded by the login flow) and only falls + back to resolving/creating by the OIDC subject when it is absent — e.g. a + session established before the id was recorded. Returns None when there is no + authenticated subject. + """ + user_id = session.get("user_id") + if user_id: + user = database.session.query(db.User).filter(db.User.id == user_id).first() + if user is not None: + return user + google_sub = session.get("google_id") + if not google_sub: + return None + user = database.get_user_by_sub(google_sub) + if user is None: + user = database.upsert_user( + google_sub=google_sub, + email=session.get("email") or "", + display_name=session.get("name"), + ) + return user + + def admin_imports_enabled_required(f): @wraps(f) def enabled_r(*args, **kwargs): @@ -1267,6 +1318,48 @@ def logout(): return redirect("/") +@openapi_documented("get_user_resources") +@app.route("/rest/v1/user/resources", methods=["GET"]) +@feature_enabled_or_default( + lambda: is_login_enabled() and is_myopencre_enabled(), + lambda: jsonify({"selected": []}), +) +@login_required +def get_user_resources() -> Any: + """Return the standard names the current user has selected.""" + database = db.Node_collection() + user = _resolve_current_user(database) + if user is None: + abort(401, description="Not authenticated") + return jsonify({"selected": database.get_user_resource_selection(user.id)}) + + +@openapi_documented("put_user_resources") +@app.route("/rest/v1/user/resources", methods=["PUT"]) +@feature_enabled_or_default( + lambda: is_login_enabled() and is_myopencre_enabled(), + lambda: jsonify({"selected": []}), +) +@login_required +def put_user_resources() -> Any: + """Replace the current user's selected standards.""" + body = request.get_json(silent=True) + if not isinstance(body, dict) or not isinstance(body.get("selected"), list): + abort(400, description="Body must be a JSON object with a 'selected' list") + raw_selected = body["selected"] + if not all(isinstance(name, str) and name.strip() for name in raw_selected): + abort(400, description="'selected' must be a list of non-empty strings") + # Normalize before storing: otherwise " ASVS " and "ASVS" both validate but + # persist as distinct rows, defeating the dedupe. + selected = [name.strip() for name in raw_selected] + database = db.Node_collection() + user = _resolve_current_user(database) + if user is None: + abort(401, description="Not authenticated") + stored = database.set_user_resource_selection(user.id, selected) + return jsonify({"selected": stored}) + + @openapi_documented("all_cres") @app.route("/rest/v1/all_cres", methods=["GET"]) def all_cres() -> Any: diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 08931d824..4bf657765 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -494,6 +494,73 @@ paths: description: Not found '400': description: Missing text parameter + /rest/v1/user/resources: + get: + tags: + - User + summary: Get the current user's selected standards + description: Requires login (CRE_ENABLE_LOGIN) and the MyOpenCRE feature (CRE_ENABLE_MYOPENCRE). + When either flag is disabled the endpoint does not authenticate and returns + an empty selection. When both are enabled, anonymous requests receive 401. + responses: + '200': + description: The user's selected standards + content: + application/json: + schema: + type: object + required: + - selected + properties: + selected: + type: array + items: + type: string + '401': + description: Not authenticated (both feature flags enabled and no active + session) + put: + tags: + - User + summary: Replace the current user's selected standards + description: Requires login (CRE_ENABLE_LOGIN) and the MyOpenCRE feature (CRE_ENABLE_MYOPENCRE). + When either flag is disabled the request is a no-op and returns an empty selection. + When both are enabled, anonymous requests receive 401. + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - selected + properties: + selected: + type: array + items: + type: string + minLength: 1 + description: Standard names to select. Non-empty strings; values + are trimmed and deduplicated. + responses: + '200': + description: The stored selection + content: + application/json: + schema: + type: object + required: + - selected + properties: + selected: + type: array + items: + type: string + '400': + description: Invalid selection body + '401': + description: Not authenticated (both feature flags enabled and no active + session) /rest/v1/{ntype}/{name}: get: tags: