Repository navigation
fix(events): scope event history reads to the caller's accessible agents - #3040
Merged
Merged
Conversation
…le agents (trinity-enterprise#713) Requirements EVT-001 and the agent-event-subscriptions flow describe the access scoping of GET /api/events before the handler change: roster filter pushed into SQL with limit applied after it, one uniform 403 on a targeted agent outside the caller's roster, admin unrestricted.
…in SQL (trinity-enterprise#713) EventSubscriptionOperations.list_events takes agent_names: None is unrestricted, an empty list returns [] before any query (never IN ()), and a non-empty list becomes source_agent IN (...) in the WHERE clause, so LIMIT applies after the filter. The facade passes it through; existing callers are unchanged. Tests run the production query on the tmp SQLite schema: limit after the filter, no engine call on an empty list, None unrestricted.
…(trinity-enterprise#713) Non-admin callers get the accessible roster (db.get_accessible_agent_names, the roster both event sockets use) passed to list_agent_events, so the filter and limit run in SQL; admins are unrestricted. A targeted source_agent goes through assert_agent_access and must also be in the roster, giving one uniform 403 for a nonexistent, inaccessible or soft-deleted-but-shared agent. An empty source_agent means no source filter; the roster still applies. Tests drive the real handler with real principals over users, ownership and sharing rows written by the real db writers.
…ne refs (trinity-enterprise#713) tests/registry.json gains the unit file; the agent-event-subscriptions flow points at the current lines of list_all_events, list_agent_events and list_events.
…inity-enterprise#713) GET /api/events resolved the roster with `email or ""`, so a non-admin principal with a blank email was matched against any other account whose email is also blank. Return an empty roster instead, as the /ws socket does for a falsy email.
… non-admin (trinity-enterprise#713)
AndriiPasternak31
marked this pull request as ready for review
September 28, 2026 10:57
Resolve tests/registry.json: keep both appended entries (ent#713 and ent#530).
dolho
approved these changes
Sep 29, 2026
dolho
left a comment
Contributor
There was a problem hiding this comment.
Approving the code. The merge hold for the preservation step is still yours to lift.
Verification. I ran test_ent713_events_access_scope.py, test_186_enumeration_uniformity.py and test_1310_auth_wiring.py in a worktree: 55 passed.
What holds up
- Read paths: all are covered.
/api/agents/{name}/eventswas already behindAuthorizedAgent. Subscription list usesAuthorizedAgentand get-by-id usesassert_agent_access. The WS broadcast carries identifiers only, andevent_busalready filtersagent_eventagainst the same roster. - Enumeration (Invariant #8): the access check runs first. A nonexistent agent, an inaccessible one and a shared-but-soft-deleted one all get the same 403.
- Connectors: blocked by the route allowlist, and
_enforce_connector_scoperuns again. - SQL: a bound
in_(), an early return on an empty roster (so noIN ()), and the limit is applied after the filter. Theidx_events_sourceindex backs the query on both SQLite and PG. - Tests: they use the real handler, real ownership and sharing rows, and the mounted route, with a mutation control. EVT-001 and the feature flow match the code.
Findings (minor or nits)
- Minor:
routers/event_subscriptions.py:468.current_user.role == "admin"is also true for an agent-scoped MCP key whose owner is an admin. On a default admin-owned install, any agent key can still read every agent's event history. The PR defers this to a follow-up. Please add a test that pins the current behaviour, or a TODO in code, so the deferral stays visible. - Minor:
tests/unit/test_ent713_events_access_scope.py:72. Every principal is built withagent_name=None. There is no case for an agent-scoped principal, which is the main programmatic caller. - Minor (behaviour change):
event_subscriptions.py:468-473. A non-admin no longer sees events they emitted themselves as a human (source_agent= their username). It is documented and consistent with/ws, but it needs a line in the release notes so nobody reads it as data loss. - Nit:
event_subscriptions.py:471. The full roster is loaded and bound asINparameters on every request. That's fine at realistic sizes, but a JOIN or subquery would avoid a growing parameter list and SQLite's 32766-variable ceiling. - Nit:
event_subscriptions.py:474-477.assert_agent_accessfollowed by the roster check is two lookups. The second is what catches a share of a soft-deleted agent, becausecan_user_access_agentignoresdeleted_at. A one-line comment saying why both are there would help.
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs trinity-enterprise#713
Summary
GET /api/eventsnow returns only events from agents the caller can access, the same roster the/wsand/ws/eventssockets already filteragent_eventwith.source_agentis in their accessible roster (owned + shared). The roster is applied in SQL (source_agent IN (...)), solimitcounts accessible events only.?source_agent=outside the caller's access (unshared, nonexistent, or shared-but-deleted) returns one uniform403 Access denied./ws.GET /api/agents/{name}/eventsis unchanged; it already goes throughAuthorizedAgent.Changes
db/event_subscriptions.py:list_events(agent_names=).Nonemeans unrestricted;[]returns[]without querying.database.py: facade pass-through.routers/event_subscriptions.py:list_all_eventsresolves the roster and checkssource_agent.requirements/roadmap.md(EVT-001),feature-flows/agent-event-subscriptions.md.Test plan
tests/unit/test_ent713_events_access_scope.py, 14 tests: the real handler over real user / ownership / sharing rows, plus one call through the mounted route as a non-admin. Green on 3 seeds together withtest_1310_auth_wiringandtest_186_enumeration_uniformity(55 passed).dev, the owned+shared list, emptysource_agent, uniform-403 and mounted-route tests go red./review+/cso --diff: one low finding (blank-email roster), fixed in this branch./verify-local: image build + import smoke, boot + health, and integration (70 passed, 13 skipped, 2 known webhook false-fails deselected) all PASS (--skip-unit --skip-agent). An earlier run's unit stage was red on 40 pre-existing, order-dependent failures in files this branch does not touch (test_ent477_*,test_ent666_objective_join,test_retention_floor). All of them run before this branch's test file in the fixed order, or pass right after it.?source_agent=shared narrows; unshared and nonexistent both return403 {"detail":"Access denied"}; admin sees all 6 and can target any agent. Control: withdev'srouters/event_subscriptions.pyswapped into the same container, the same check goes 3/6: the non-admin list returns all 6 events, and the unshared and nonexistent targets return 200.Merge
Ready for review, not for merge: don't merge until trinity-enterprise#713's pre-merge condition is recorded as done or waived. Close the issue by hand at the release cut (a cross-repo reference doesn't auto-close it).
🤖 Generated with Claude Code