diff --git a/backend/app/api/analytics_routes.py b/backend/app/api/analytics_routes.py index d1474c7..cc83d92 100644 --- a/backend/app/api/analytics_routes.py +++ b/backend/app/api/analytics_routes.py @@ -9,7 +9,7 @@ from flask import Blueprint, g, jsonify, request from ..auth.users import AuthError, normalize_identifier from ..config import Config -from ..services.groups import is_ready_group, is_valid_owner_visibility, validated_personas +from ..services.groups import is_ready_group, is_valid_owner_visibility, resolved_visibility, validated_personas from ..storage.store import StoreError from .helpers import ApiError, current_user, is_valid_tenant_id, require_auth, require_roles @@ -34,7 +34,7 @@ def _is_shared_group(group: object) -> bool: and is_valid_tenant_id(group.get("org_id")) and "owner_user_id" not in group and is_valid_owner_visibility(group) - and group.get("visibility") in {"public", "hidden"} + and resolved_visibility(group) in {"public", "hidden"} and is_ready_group(group) ) diff --git a/backend/app/api/chat_routes.py b/backend/app/api/chat_routes.py index 8a538ac..ddcea76 100644 --- a/backend/app/api/chat_routes.py +++ b/backend/app/api/chat_routes.py @@ -14,6 +14,7 @@ from ..services.groups import ( is_canonical_private_owner, is_ready_group, is_valid_owner_visibility, + resolved_visibility, ) from ..services.simulator import Simulator, _safe_roleplay_internal from ..services.store import PERSONA_CHANNELS @@ -400,7 +401,11 @@ def _get_ready_group(s, gid: str) -> dict: # demo tenant and demo-visible shared groups. Tenant admins retain the # historical hidden-group preview path, while ordinary users can access # only public shared groups (or their own private group). - visibility = group_visibility(group.get("visibility")) + visibility = ( + group_visibility(group.get("visibility")) + if actor.get("role") == "demo" + else resolved_visibility(group) + ) if actor.get("role") != "super_admin" and visibility is None: raise ApiError("permission denied", 403) if actor.get("role") == "demo": diff --git a/backend/app/api/group_routes.py b/backend/app/api/group_routes.py index 50349b7..969c2c4 100644 --- a/backend/app/api/group_routes.py +++ b/backend/app/api/group_routes.py @@ -19,6 +19,7 @@ from ..services.groups import ( is_canonical_private_owner, is_ready_group, is_valid_owner_visibility, + resolved_visibility, safe_group_input, ) from ..services.store import ( @@ -217,7 +218,7 @@ def _authorize_group(group: dict) -> None: if not isinstance(group, dict): raise ApiError("group not found", 404) actor = current_user() - if group_visibility(group.get("visibility")) is None: + if resolved_visibility(group) is None: raise ApiError("group not found", 404) owner_marker_present = "owner_user_id" in group owner = group.get("owner_user_id") @@ -248,7 +249,7 @@ def _authorize_group(group: dict) -> None: if not is_valid_owner_visibility(group): raise ApiError("permission denied", 403) if actor.get("role") == "admin" and ( - owner_marker_present or group_visibility(group.get("visibility")) not in {"public", "hidden"} + owner_marker_present or resolved_visibility(group) not in {"public", "hidden"} ): raise ApiError("permission denied", 403) if owner_marker_present and ( @@ -272,8 +273,9 @@ def _get_owned_group(s, gid: str, *, require_ready: bool = True) -> dict: if current_user().get("role") in {"user", "demo"}: # Trainees may read only ready groups. Normal trainees see public shared # groups and their own private groups; demos see only demo-visible groups. - allowed_visibility = "demo" if current_user().get("role") == "demo" else "public" - visibility = group_visibility(group.get("visibility")) + is_demo = current_user().get("role") == "demo" + allowed_visibility = "demo" if is_demo else "public" + visibility = resolved_visibility(group) if not is_demo else group_visibility(group.get("visibility")) if visibility is None: raise ApiError("permission denied", 403) if "owner_user_id" not in group and visibility != allowed_visibility: @@ -592,7 +594,7 @@ def list_groups(): "id": g.get("id"), "title": g.get("title", ""), "status": g.get("status", "draft"), - "visibility": g.get("visibility", "public"), + "visibility": resolved_visibility(g) or "public", "is_owned": g.get("is_owned") is True, "channel": group_input.get("channel"), "org_id": g.get("org_id"), diff --git a/backend/app/services/groups.py b/backend/app/services/groups.py index b9e2e59..3f8ee0f 100644 --- a/backend/app/services/groups.py +++ b/backend/app/services/groups.py @@ -34,6 +34,29 @@ def group_visibility(value: object) -> str | None: return value if isinstance(value, str) and value in GROUP_VISIBILITIES else None +def resolved_visibility(group: object) -> str | None: + """Return the effective visibility of a persisted group for authorization. + + Records written before the visibility field existed carry no ``visibility``. + The old schema encoded sharing entirely through ``owner_user_id``: a record + with an owner was the owner's private group, and a record without one was a + shared public group. For such legacy records (and only them) we derive the + effective visibility from that historical contract so existing training data + remains listable and accessible without rewriting any persisted data. + + A record that carries an explicit but malformed ``visibility`` stays + fail-closed (returns None) like the raw ``group_visibility``. demo/hidden + are never derived from legacy records. + """ + if not isinstance(group, dict): + return None + if "visibility" in group: + return group_visibility(group.get("visibility")) + if "owner_user_id" in group: + return "private" + return "public" + + def is_valid_owner_visibility(group: object) -> bool: """Return whether the persisted owner/visibility pair is fail-closed. @@ -43,7 +66,7 @@ def is_valid_owner_visibility(group: object) -> bool: """ if not isinstance(group, dict): return False - visibility = group_visibility(group.get("visibility")) + visibility = resolved_visibility(group) if "owner_user_id" in group: owner = group.get("owner_user_id") return ( @@ -411,13 +434,13 @@ class GroupStore: is_valid_owner_visibility(g) and "owner_user_id" in g and g.get("owner_user_id") == user_id - and group_visibility(g.get("visibility")) == "private" + and resolved_visibility(g) == "private" ) or ( is_valid_owner_visibility(g) and "owner_user_id" not in g and is_ready_group(g) - and group_visibility(g.get("visibility")) == "public" + and resolved_visibility(g) == "public" ) ] return [ @@ -450,7 +473,7 @@ class GroupStore: if ( is_valid_owner_visibility(g) and "owner_user_id" not in g - and group_visibility(g.get("visibility")) in {"public", "hidden"} + and resolved_visibility(g) in {"public", "hidden"} ) ] owned = [ diff --git a/backend/tests/test_security_review_blockers.py b/backend/tests/test_security_review_blockers.py index e3d79ed..7cb4017 100644 --- a/backend/tests/test_security_review_blockers.py +++ b/backend/tests/test_security_review_blockers.py @@ -1154,3 +1154,134 @@ def test_analytics_excludes_falsey_modes_and_missing_persona_context( assert response.get_json()["overall"]["total_sessions"] == 1 assert "falsey-mode" not in export.get_data(as_text=True) assert "missing-persona" not in export.get_data(as_text=True) + + +# ── Legacy visibility migration (migrate-on-read) ──────────────────────────── +# Records written before the `visibility` field existed carry no visibility and +# encoded sharing purely through owner_user_id (owner present => private, absent +# => shared public). resolved_visibility() must make such legacy records listable +# and accessible without rewriting persisted data, while malformed explicit +# visibility and demo/hidden semantics stay fail-closed / raw. + + +def test_resolved_visibility_maps_legacy_shared_to_public(): + from app.services.groups import resolved_visibility + + legacy_shared = {"id": "g1", "org_id": "org-default", "status": "ready"} + assert resolved_visibility(legacy_shared) == "public" + + +def test_resolved_visibility_maps_legacy_private_to_private(): + from app.services.groups import resolved_visibility + + legacy_private = { + "id": "g2", + "org_id": "org-default", + "status": "ready", + "owner_user_id": "owner-1", + } + assert resolved_visibility(legacy_private) == "private" + + +def test_resolved_visibility_keeps_explicit_visibility_unchanged(): + from app.services.groups import resolved_visibility + + assert resolved_visibility({"visibility": "hidden"}) == "hidden" + assert resolved_visibility({"visibility": "demo"}) == "demo" + assert resolved_visibility({"visibility": "private", "owner_user_id": "x"}) == "private" + + +def test_resolved_visibility_malformed_explicit_stays_fail_closed(): + from app.services.groups import resolved_visibility + + # Explicit-but-malformed visibility must never be silently migrated. + assert resolved_visibility({"visibility": "garbage"}) is None + assert resolved_visibility({"visibility": "garbage", "owner_user_id": "x"}) is None + assert resolved_visibility(["not-a-dict"]) is None + assert resolved_visibility(None) is None + + +def test_user_lists_legacy_shared_group(client, user_store, login): + token = _create_user(user_store, login, username="legacy-shared-user") + groups = client.application.extensions["group_store"] + # Legacy shared record: ready, no visibility (pre-redesign schema), no owner. + groups.groups.create( + { + "id": "legacy-shared-group", + "org_id": "org-default", + "title": "Legacy shared training", + "status": "ready", + "sales_kit": {"productName": "Legacy product"}, + "report": {"summary": "Legacy report"}, + "personas": [{"id": "p1", "name": "Persona 1"}], + }, + key="legacy-shared-group", + ) + + response = client.get("/api/groups", headers=_headers(token)) + + assert response.status_code == 200 + payload = response.get_json() + ids = [g.get("id") for g in payload.get("groups", payload) if isinstance(g, dict)] + assert "legacy-shared-group" in ids + + +def test_owner_lists_legacy_private_group(client, user_store, login): + token = _create_user(user_store, login, username="legacy-owner-user") + groups = client.application.extensions["group_store"] + groups.groups.create( + { + "id": "legacy-owner-group", + "org_id": "org-default", + "title": "Legacy private training", + "status": "ready", + "owner_user_id": "legacy-owner-user", + "sales_kit": {"productName": "Priv product"}, + "report": {"summary": "Priv report"}, + "personas": [{"id": "p1", "name": "Persona 1"}], + }, + key="legacy-owner-group", + ) + + response = client.get("/api/groups", headers=_headers(token)) + + assert response.status_code == 200 + payload = response.get_json() + ids = [g.get("id") for g in payload.get("groups", payload) if isinstance(g, dict)] + assert "legacy-owner-group" in ids + + +def test_other_user_does_not_see_legacy_private_group(client, user_store, login): + _create_user(user_store, login, username="legacy-other-user") + token = _create_user(user_store, login, username="legacy-owner-a") + groups = client.application.extensions["group_store"] + groups.groups.create( + { + "id": "legacy-owner-only-group", + "org_id": "org-default", + "title": "Owner-only", + "status": "ready", + "owner_user_id": "legacy-owner-a", + "sales_kit": {"productName": "P"}, + "report": {"summary": "R"}, + "personas": [{"id": "p1", "name": "Persona 1"}], + }, + key="legacy-owner-only-group", + ) + + other = client.post( + "/api/auth/login", json={"username": "legacy-other-user", "password": "legacy-other-user-password"} + ).get_json() + response = client.get("/api/groups", headers=_headers(other["token"])) + + payload = response.get_json() + ids = [g.get("id") for g in payload.get("groups", payload) if isinstance(g, dict)] + assert "legacy-owner-only-group" not in ids + + +def test_malformed_visibility_legacy_syntax_not_migrated(client, user_store, login): + # A record that has an explicit malformed visibility must still fail closed, + # even if it otherwise looks like a legacy owner/shared record. + from app.services.groups import resolved_visibility + + assert resolved_visibility({"visibility": "evil", "owner_user_id": "x"}) is None diff --git a/docs/engineering-log/2026-08-24-training-security-review.md b/docs/engineering-log/2026-08-24-training-security-review.md index ec98205..a676fc4 100644 --- a/docs/engineering-log/2026-08-24-training-security-review.md +++ b/docs/engineering-log/2026-08-24-training-security-review.md @@ -404,3 +404,44 @@ verification, production runtime check, stage, commit, push, deploy, reset, stas credential, or permission action is claimed. Branch remains `main`; HEAD remains `8a632b5e6a3b67c9acd2787e1a686ac17ae69486`; the index is empty and the broad existing working tree remains intentionally dirty. + +## 2026-08-25 legacy-visibility migrate-on-read remediation + +After the eight-scope security gate passed and the work was pushed, the operator +reported that pre-existing training data appeared to have disappeared from the UI +while the underlying records were still present. Root cause was a +backward-incompatible visibility schema introduction: records written before the +`visibility` field existed carry none, and the new fail-closed authorization +treated a missing visibility as invalid, so every legacy record became +unlistable and unreadable. + +The old schema encoded sharing entirely through `owner_user_id` (owner present => +own private group; absent => shared public group). The fix is a migrate-on-read +helper `resolved_visibility(group)` that derives the effective visibility for +such legacy records only (missing `visibility`): owner present => `private`, +absent => `public`. Records with an explicit but malformed `visibility` stay +fail-closed (None); `demo`/`hidden` are never derived from legacy records. No +persisted data is rewritten. + +The resolved visibility is now used at every authorization/list boundary that +previously blocked legacy records — `GroupStore.list_visible_to` (user, admin, +super-admin shared branches), `group_routes._authorize_group`, +`group_routes._get_owned_group`, `group_routes.list_groups` serialization, +`chat_routes._get_ready_group` (non-demo), and `analytics_routes._is_shared_group` +— while `demo` and hidden-preview paths keep using the raw visibility so +legacy records are never promoted to demo/hidden. + +Evidence after remediation: + +| Check | Result | +|---|---| +| legacy/resolved_visibility regression suite | **13 passed** | +| blocker test file | **55 passed in 10.10s** | +| backend full suite | **517 passed in 80.97s** | +| frontend unit suite | **26 passed (6 files)** | +| frontend production build | **passed** | +| compileall / `git diff --check` | **passed / passed** | + +Returning to migration-on-read behaves purely at runtime; no data migration, +backfill, or permission change was performed. Stage/commit/push requires operator +direction as before. diff --git a/docs/test-evidence/2026-08-24-training-security-review.md b/docs/test-evidence/2026-08-24-training-security-review.md index 3f35033..80e9a1d 100644 --- a/docs/test-evidence/2026-08-24-training-security-review.md +++ b/docs/test-evidence/2026-08-24-training-security-review.md @@ -146,6 +146,14 @@ smoke test. frontend and **81/81** focused backend checks; final cross-cutting scope also **passed**. Both exact verdicts had empty security and logic arrays. - Independent local code/security gate: **8/8 scopes passed**. +- Post-push operator report of missing legacy training data traced to the new + fail-closed visibility schema. Legacy records lacking `visibility` were rejected + by authorization. Fixed with migrate-on-read `resolved_visibility` (owner present + => private, absent => public; malformed explicit stays fail-closed; demo/hidden + never derived). Applied at every list/authorization/chat/analytics boundary. +- Post-fix evidence: legacy/resolved regression **13 passed**; blocker file **55 + passed**; full backend **517 passed in 80.97s**; frontend unit **26 passed**; + production build **passed**; compileall and `git diff --check` passed. - No live OAuth provider, production runtime, deployment, stage, commit, push, reset, or stash verification is claimed.