bvandeusen
04b58ce01e
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 28s
CI & Build / integration (push) Successful in 35s
CI & Build / Python tests (push) Failing after 41s
CI & Build / Build & push image (push) Has been skipped
Narrows theb7d6fc7widening per the operator's call, and fixes a regression it introduced. Decision recorded as note 2094. Two scopes now, deliberately different: readable_notes_clause — everything the ACL permits, including records reached only via a direct/group note share. For EXPLICIT acts: a search the caller typed, a fetch by id. browsable_notes_clause — the caller's own records plus anything in a project they can reach. For PASSIVE surfaces: browse lists, facet counts, the process->skill manifest. The split is a trust boundary. Anything appearing unasked — in your own list, your own counts, or as a skill installed on your machine — reads as material you endorsed. A one-off someone shared with you hasn't earned that standing, so it waits until you go looking. This also dissolves the shared-Process problem by construction rather than by special case: the manifest is a passive surface, so a directly-shared Process is never installed as an auto-surfacing skill. Regression fix (#2093):b7d6fc7widened the list queries but left the fetch path owner-only, so on the MCP path a record could be listed and then not opened — get_snippet raised not-found, get_process couldn't resolve, and the manifest emitted stubs whose get_process call would fail. snippets.get_snippet and notes.resolve_process now resolve the read scope. delete_snippet gained an explicit can_write_note guard, since being able to SEE a shared snippet must not imply being able to bin it. Provenance, so nothing arrives looking like the operator's own work: - access.describe_provenance / label_shared_items add shared/owner/permission; labelling costs no query when everything is the caller's own. - MCP: get_snippet, list_snippets, get_process and list_processes carry it, and get_process now says outright NOT to follow a shared process verbatim — its follow-as-written contract was the sharpest instance of the problem. - The skill stub for a shared Process names its author and asks for a go-ahead, instead of describing it as "the operator's saved Scribe process". - Policy stated once in the MCP _INSTRUCTIONS and the reusing-code skill: a shared record is that person's suggestion, weigh it, attribute it, ask before adopting it. - UI: shared snippets show "by <owner>" in the list and a notice above the code in the detail view, reusing SharedWithMeView's vocabulary. Plugin 0.1.15 -> 0.1.16 (skill text changed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt
111 lines
4.5 KiB
Python
111 lines
4.5 KiB
Python
"""`readable_notes_clause` — the set-based ACL predicate behind list queries.
|
|
|
|
It exists because `get_note_permission` answers "may I read THIS note?" one row
|
|
at a time, which a list query can't use. The two must stay in step: anything
|
|
readable one way has to be findable the other, or a record shared with you is
|
|
openable by id but invisible in every list — the bug this predicate fixed.
|
|
|
|
No DB: the group lookup is stubbed and the resulting clause is compiled to SQL
|
|
so the shape can be asserted directly.
|
|
"""
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
|
|
def _session_returning_groups(group_ids: list[int]):
|
|
"""A stub async session whose only query — the caller's group ids — returns
|
|
`group_ids`."""
|
|
result = MagicMock()
|
|
result.scalars.return_value.all.return_value = group_ids
|
|
session = AsyncMock()
|
|
session.__aenter__ = AsyncMock(return_value=session)
|
|
session.__aexit__ = AsyncMock(return_value=False)
|
|
session.execute = AsyncMock(return_value=result)
|
|
return session
|
|
|
|
|
|
async def _compiled_clause(group_ids: list[int]) -> str:
|
|
from scribe.services.access import readable_notes_clause
|
|
with patch("scribe.services.access.async_session") as cls:
|
|
cls.return_value = _session_returning_groups(group_ids)
|
|
clause = await readable_notes_clause(user_id=7)
|
|
return str(clause.compile(compile_kwargs={"literal_binds": True}))
|
|
|
|
|
|
async def _compiled_browse_clause(group_ids: list[int]) -> str:
|
|
from scribe.services.access import browsable_notes_clause
|
|
with patch("scribe.services.access.async_session") as cls:
|
|
cls.return_value = _session_returning_groups(group_ids)
|
|
clause = await browsable_notes_clause(user_id=7)
|
|
return str(clause.compile(compile_kwargs={"literal_binds": True}))
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_clause_covers_ownership_and_both_share_paths():
|
|
sql = await _compiled_clause(group_ids=[])
|
|
# 1. ownership
|
|
assert "notes.user_id = 7" in sql
|
|
# 2/3. a direct or group share on the note itself
|
|
assert "note_shares" in sql
|
|
assert "shared_with_user_id = 7" in sql
|
|
# 4. inheritance from a shared project
|
|
assert "project_shares" in sql
|
|
assert "notes.project_id IN" in sql
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_group_membership_widens_both_share_lookups():
|
|
sql = await _compiled_clause(group_ids=[3, 9])
|
|
assert "shared_with_group_id IN (3, 9)" in sql
|
|
# Both the note-level and project-level share lookups gain the group arm.
|
|
assert sql.count("shared_with_group_id IN (3, 9)") == 2
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_no_groups_means_no_group_arm():
|
|
"""A user in no groups must not produce an empty `IN ()`, which would be
|
|
invalid SQL on some backends and always-false on others."""
|
|
sql = await _compiled_clause(group_ids=[])
|
|
assert "shared_with_group_id" not in sql
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_owner_only_result_is_never_the_whole_table():
|
|
"""Guard against the predicate degrading to something always-true — that
|
|
would expose every user's notes to every other user."""
|
|
sql = await _compiled_clause(group_ids=[])
|
|
assert "true" not in sql.lower()
|
|
assert "1 = 1" not in sql
|
|
|
|
|
|
# --- browse scope: the trust boundary ---------------------------------------
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_browse_scope_excludes_direct_note_shares():
|
|
"""The whole point of the narrower scope (decision note 2094): a record
|
|
shared directly with you must NOT appear in a passive list. If `note_shares`
|
|
leaks in here, someone else's one-off lands in the operator's own browse
|
|
list, facet counts, and skill manifest as though they had recorded it."""
|
|
sql = await _compiled_browse_clause(group_ids=[3])
|
|
assert "note_shares" not in sql
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_browse_scope_keeps_ownership_and_project_access():
|
|
sql = await _compiled_browse_clause(group_ids=[])
|
|
assert "notes.user_id = 7" in sql # your own
|
|
assert "project_shares" in sql # a project shared with you
|
|
assert "projects" in sql # a project you own
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_browse_scope_is_strictly_narrower_than_read_scope():
|
|
"""Browse must never surface something read scope wouldn't also allow —
|
|
otherwise a list could show a record the caller can't open."""
|
|
browse = await _compiled_browse_clause(group_ids=[3])
|
|
read = await _compiled_clause(group_ids=[3])
|
|
assert "note_shares" in read and "note_shares" not in browse
|
|
for shared_arm in ("notes.user_id = 7", "project_shares"):
|
|
assert shared_arm in browse and shared_arm in read
|