[authz][P2] Wave 11: harden Visu boundaries and grant lifecycle - #995
#995[authz][P2] Wave 11: harden Visu boundaries and grant lifecycle#995Micsi wants to merge 170 commits intoabeggled:mainabeggled/openbridgeserver:mainfrom Micsi:issue-583-p2-wave11-integrationMicsi/openbridgeserver:issue-583-p2-wave11-integrationCopy head branch name to clipboard
Conversation
Three authz correctness gaps in the user lifecycle: 1. create_user: INSERT + audit write now run in db.transaction() so an audit failure cannot leave the user row pending on the shared connection for a subsequent request to commit accidentally. 2. delete_user: central grants for the deleted user's API keys (principal_type='api_key') are now deleted before the api_keys rows are removed, preventing orphan grant rows that could become effective if a key ID is reused. 3. delete_user: owner grants for transferred visu pages and logic graphs are now always materialised for the successor, regardless of the successor's current admin bit; a later demotion would otherwise leave them without any central owner grant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8e30f78 to
d4a2150
Compare
|
@codex review Please review the current cumulative Wave 11 head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 272a3d5e3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review current cumulative Wave 11 head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e2c39d146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A principal with datapoint READ but no adapter_instance grant could
enumerate adapter_instance_id, instance_name, and binding config via
GET /datapoints/{dp_id}/bindings, bypassing the instance boundary that
all other adapter-instance endpoints enforce via _ensure_instance_read.
_filter_bindings_by_instance_read now loads instance grants once and
filters out any binding whose adapter_instance_id the principal cannot
read (min_role=guest), consistent with the gate in adapters.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review-Runde 1 — 1 Findings (1 fix, 0 resolve, 0 reject, 0 escalate)Gates: ruff_check=pass ruff_format=pass tests=pass
Finding FIX umgesetzt: list_bindings filtert Bindings jetzt nach Adapter-Instanz-READ-Scope (min_role=guest), konsistent mit allen anderen Adapter-Instanz-Endpunkten. Commit ac755e5, alle 4318 Tests grün. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac755e5730
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
authorize_visu_page READ grants intentionally propagate upward for tree navigation, but _check_user_access gates full content reads. Add a strict=True flag to authorize_visu_page that pre-filters grants to the target's own path (node itself + ancestors), preventing a grant on a descendant page from allowing reads of the ancestor's page config and widgets. _can_discover_node retains strict=False (default) so breadcrumb/tree discovery via descendant grants is unaffected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review-Runde 2 — 1 Findings (1 fix, 0 resolve, 0 reject, 0 escalate)Gates: ruff_check=pass ruff_format=pass tests=pass
_Finding FIX umgesetzt: check_user_access nutzt jetzt authorize_visu_page mit strict=True und verhindert damit, dass ein Descendant-Grant Zugriff auf Vorfahren-Seiteninhalte gewährt. 3 neue Tests, 195 bestehende Tests weiterhin grün, Ruff sauber. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Stack
This is Wave 11 of #583 and remains cumulative by commit ancestry.
3ba64e9e7f34850d17a6a434e03459e9bea854590e2c39d146e097762d526ab1b819f09b466ee9e5Verification
BASE_URL=http://127.0.0.1:18091): pass, 230/230 without retryThe populated-only retry is reported separately as test-isolation/timing evidence; this PR does not claim a fully flake-free E2E state.
Deliberately deferred
DataPoint
control_classdefaults/mapping and additional rights-editor switches remain outside this wave because their product contract is not yet decided. No speculative authorization semantics were introduced.Closes #618
Closes #628
Refs #583
Refs #629
Review-Umsetzung (Codex-Loop)
obs/api/auth.py— create_user führte INSERT und audit_writer.write(commit=False) auf der geteilten db._conn durch und rief danach db.commit() auf. Bei einem Fehler in audit_writer.write wurde weder committed noch explizit rollback aufgerufen; die geteilte SQLite-Verbindung akkumuliert die uncommitteten Änderungen und ein späterer Request-Commit kann den Benutzer-INSERT unkontrolliert persistieren. Fix: INSERT und audit.write in db.transaction() eingeschlossen (analog zu delete_user), explizites db.commit() entfernt. (83cd620)obs/api/auth.py— delete_user löschte api_keys-Zeilen und authz_node_roles-Zeilen für principal_type='user', aber nicht authz_node_roles für principal_type='api_key' mit den gelöschten Key-IDs als principal_id. Diese Waisen-Grants blieben im DB und können reaktiviert werden, falls eine Key-ID wiederverwendet wird. Fix: DELETE FROM authz_node_roles WHERE principal_type='api_key' AND principal_id IN (SELECT id FROM api_keys WHERE owner=?) wird vor dem api_keys-DELETE in derselben Transaktion ausgeführt. (83cd620)obs/api/v1/config.py— Der Rebase übernimmt die vollständige, geschlossene Zielvalidierung aus dem finalen Wave-10-Head: importierte Grants werden über_require_grant_targetsgegen alle konkreten Ressourcentabellen und die Logic-Capability-Allowlist geprüft; unbekannte oder fehlende Ziele werden nicht persistiert. (e86b8a6)obs/api/auth.py— delete_user übersprang die Materialisierung von Owner-Grants für visu_page- und logic_graph-Transfers, wenn der Nachfolger aktuell Admin war (if not successor_row['is_admin']). Bei späterer Demotierung des Nachfolgers existieren keine zentralen Owner-Grants mehr für die übertragenen Artefakte. Fix: Guard entfernt; transferred_grants wird immer gebaut und bei nicht-leerer Liste via executemany eingefügt. (83cd620)obs/api/v1/visu.py— Subtree-Export nutzt für effektivuser-gescopte Pages dieselben Principal- und Datapoint-READ-Prüfungen wie Page/WidgetRef; API-Key-Principals und Nutzer mit widerrufenem Datapoint-READ erhalten keine Config. (272a3d5)obs/admin_cli.py—owner-recoveryvalidiert bekannte Node-Typen, konkrete Ressourcen-IDs und Logic-Capabilities vor jeder Promotion oder Grant-Mutation; unbekannte Ziele bleiben atomar ohne Seiteneffekt. (272a3d5)obs/api/v1/visu.py— User-gescopte Discovery lehnt API-Key-Principals unabhängig von zentralen Visu-Grants ab; Tree, Node, Children, Breadcrumb und Export sind regressionsgetestet. (0e2c39d)obs/api/v1/visu.py— Rekursive Subtree-Grant-Bereinigung dedupliziert besuchte IDs und terminiert auch bei zyklischen Parent-Daten. (0e2c39d)obs/api/v1/logic.py— Der Datapoint-READ-Fallback gibt Side-Effect- oder unbekannte Flows nicht ohne explizitenlogic_graph-Grant preis. (0e2c39d)obs/api/v1/bindings.py— Valid P2 in-scope finding. list_bindings (line 288) prüfte nur den Datapoint-READ-Scope, gab aber alle Bindings inkl. adapter_instance_id, instance_name und config zurück – ohne zu prüfen, ob der Principal auch READ-Zugriff auf die jeweilige Adapter-Instanz hat. Alle anderen Adapter-Instanz-Endpunkte nutzen _ensure_instance_read (min_role=guest). Fix: neue Funktion _filter_bindings_by_instance_read lädt Instanz-Grants einmalig und filtert Bindings, deren adapter_instance_id der Principal nicht lesen darf. TDD: Roter Test test_list_bindings_filters_bindings_by_instance_read_access zuerst geschrieben, dann Fix, dann grün. Bestehender Test test_non_admin_list_bindings_requires_read_scope an neue Semantik angepasst (instance grant hinzugefügt). Gates: ruff check/format pass, 4318 Tests grün. (ac755e5)obs/api/v1/visu.py— Bug bestätigt: _check_user_access rief authorize_visu_page mit READ auf, das upward-discovery aktiviert (grant.node_id in target.path ODER target.node_id in grant.path). Damit konnte ein User mit Grant nur auf einem Kind-Knoten auch den Eltern-Knoten lesen. Fix: authorize_visu_page erhält strict=True-Parameter, der Grants vor dem authorize-Aufruf auf target.path filtert (nur Grants auf dem Knoten selbst oder seinen Vorfahren). _can_discover_node nutzt weiterhin strict=False, sodass Breadcrumb/Tree-Navigation unverändert funktioniert. Test test_check_user_access_does_not_grant_ancestor_via_descendant_grant war vor dem Fix rot, nach dem Fix grün. (2672244)