diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 52921be..dd76214 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -236,7 +236,9 @@ adopting it or acting on it. This matters most for a shared Process — do not r one as written; describe what it would do and get the operator's go-ahead. Records shared directly with the operator are also deliberately search-only: they surface when you look for them (pass a query), not in plain lists, so -nobody else's material arrives unasked. +nobody else's material arrives unasked. Editing another user's record needs an +editor or admin share from them; a read-only share is refused, and the right +answer is usually to record the operator's own version rather than to push. When developing Scribe itself, honor its multi-user sharing ACL: scope every read and mutation of user data by owner + shares — never assume a single diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index a02d841..c54e00e 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -88,11 +88,21 @@ async def get_process(name_or_id: str) -> dict: async def update_process(process_id: int, title: str = "", body: str = "", tags: list[str] | None = None) -> dict: """Update a stored process. Only provided fields change — empty title/body - leave that field unchanged; pass tags to replace the tag set.""" + leave that field unchanged; pass tags to replace the tag set. + + Editing another user's process requires an editor or admin share from them; a + read-only share is not enough and says so rather than claiming not-found. + """ uid = current_user_id() - note = await notes_svc.get_note(uid, process_id) - if note is None or note.note_type != "process": + loaded = await notes_svc.get_note_for_user(uid, process_id) + note = loaded[0] if loaded else None + if note is None or note.note_type != "process" or note.deleted_at is not None: raise ValueError(f"process {process_id} not found") + if not await access_svc.can_write_note(uid, process_id): + raise ValueError( + f"process {process_id} is shared with you read-only — ask its owner " + f"for edit access, or save your own copy with create_process" + ) fields: dict = {} if title.strip(): fields["title"] = title.strip() @@ -100,8 +110,13 @@ async def update_process(process_id: int, title: str = "", body: str = "", fields["body"] = body if tags is not None: fields["tags"] = tags - updated = await notes_svc.update_note(uid, process_id, **fields) - return updated.to_dict() + # As the owner — update_note is owner-scoped and the write is authorised above. + updated = await notes_svc.update_note(note.user_id, process_id, **fields) + if updated is None: + raise ValueError(f"process {process_id} not found") + out = updated.to_dict() + out.update(await access_svc.describe_provenance(uid, updated)) + return out def register(mcp) -> None: diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index 0f83f98..f2a93fe 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -180,6 +180,10 @@ async def update_snippet( tags: Replaces the extra-tag set (language + "snippet" are re-derived). project_id: 0 leaves it unchanged, -1 detaches it from its project, a positive id moves it. + + Editing someone else's snippet requires an editor or admin share from them. + A read-only share is refused with a message saying so — record your own + version instead of trying to force it. """ uid = current_user_id() if project_id == 0: @@ -189,21 +193,27 @@ async def update_snippet( else: project = project_id - note = await snippets_svc.update_snippet( - uid, snippet_id, - name=name, code=code, language=language, - signature=signature, when_to_use=when_to_use, - repo=repo, path=path, symbol=symbol, - locations=locations, tags=tags, project_id=project, - ) + try: + note = await snippets_svc.update_snippet( + uid, snippet_id, + name=name, code=code, language=language, + signature=signature, when_to_use=when_to_use, + repo=repo, path=path, symbol=symbol, + locations=locations, tags=tags, project_id=project, + ) + except PermissionError as exc: + # Readable but not writable — surface the real reason, not "not found". + raise ValueError(str(exc)) from exc if note is None: raise ValueError(f"snippet {snippet_id} not found") if system_ids is not None: - await systems_svc.set_record_systems(uid, snippet_id, system_ids) + await systems_svc.set_record_systems(note.user_id, snippet_id, system_ids) data = snippets_svc.snippet_to_dict(note) + data.update(await access_svc.describe_provenance(uid, note)) if system_ids is not None: data["systems"] = [ - s.to_dict() for s in await systems_svc.list_record_systems(uid, snippet_id) + s.to_dict() + for s in await systems_svc.list_record_systems(note.user_id, snippet_id) ] return data @@ -245,7 +255,10 @@ async def merge_snippets(target_id: int, source_ids: list[int]) -> dict: ids = [s for s in (source_ids or []) if s != target_id] if not ids: raise ValueError("merge_snippets requires at least one other source_id") - result = await snippets_svc.merge_snippets(uid, target_id, ids) + try: + result = await snippets_svc.merge_snippets(uid, target_id, ids) + except PermissionError as exc: + raise ValueError(str(exc)) from exc if result is None: raise ValueError(f"snippet {target_id} not found") note, merged_ids = result diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 268a08f..0c4636e 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -354,7 +354,14 @@ async def update_snippet( ): """Partial update: only fields passed (not None) change. Re-serializes the merged field set back into title/body/tags. Returns the Note, or None if the - id isn't a snippet. + id isn't a snippet the caller can see. + + Share-aware (rule #47/#78): resolves the read scope, then requires WRITE — + so an editor/admin grant lets the holder edit, and a viewer grant does not. + Raises PermissionError when the caller can read but not write, because "not + found" would be a lie about a record they can plainly open. The write itself + is performed as the OWNER, mirroring routes/snippets.py, since the underlying + note update is owner-scoped. ``project_id``: omit to leave unchanged, pass None to detach from its project, pass an id to move it. @@ -362,9 +369,15 @@ async def update_snippet( Locations: ``locations`` replaces the whole set; else a legacy single ``repo``/``path``/``symbol`` overlays onto the first existing location; else the existing locations are kept.""" - note = await notes_svc.get_note(user_id, snippet_id) - if note is None or note.note_type != SNIPPET_NOTE_TYPE: + note = await get_snippet(user_id, snippet_id) + if note is None: return None + from scribe.services.access import can_write_note + if not await can_write_note(user_id, snippet_id): + raise PermissionError( + f"snippet {snippet_id} is shared with you read-only — ask its owner " + f"for edit access, or record your own version" + ) cur = parse_snippet_fields(note.title, note.body, note.tags) overlay = { @@ -404,7 +417,9 @@ async def update_snippet( if project_id is not UNSET: fields["project_id"] = project_id - updated = await notes_svc.update_note(user_id, snippet_id, **fields) + # As the OWNER: update_note is owner-scoped, so a shared editor's own id + # would find nothing. The write was authorised by can_write_note above. + updated = await notes_svc.update_note(note.user_id, snippet_id, **fields) if updated is not None: # Title/body changed → refresh the embedding so recall reflects the edit. _embed_snippet(updated) @@ -460,23 +475,41 @@ def merge_snippet_fields( async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): - """Unify source snippets INTO the target (all owned by user_id): union their - locations + tags onto the canonical target, keep the target's scalar fields, - trash the sources (recoverable), re-embed the survivor. + """Unify source snippets INTO the target: union their locations + tags onto + the canonical target, keep the target's scalar fields, trash the sources + (recoverable), re-embed the survivor. + + Share-aware and write-gated, like update: an editor/admin grant is enough, a + viewer grant is not. Raises PermissionError when the caller can read the + target but not write it. Every source must share the TARGET'S OWNER — + cross-owner merge stays out of scope (#231) — and must itself be writable; + sources failing either test are skipped rather than silently half-merged. Returns (merged_target_note, merged_source_ids), or None if the target isn't - the user's snippet. Source ids that aren't the user's snippets are skipped.""" - target = await notes_svc.get_note(user_id, target_id) - if target is None or target.note_type != SNIPPET_NOTE_TYPE: + a snippet the caller can see.""" + from scribe.services.access import can_write_note + target = await get_snippet(user_id, target_id) + if target is None: return None + if not await can_write_note(user_id, target_id): + raise PermissionError( + f"snippet {target_id} is shared with you read-only — you can't merge " + f"into a record you can't edit" + ) + owner_id = target.user_id sources = [] for sid in source_ids: if sid == target_id: continue - s = await notes_svc.get_note(user_id, sid) - if s is not None and s.note_type == SNIPPET_NOTE_TYPE: - sources.append(s) + s = await get_snippet(user_id, sid) + # Same owner as the target, and writable by this caller. Merging trashes + # the source, so read access is not enough. + if s is None or s.user_id != owner_id: + continue + if not await can_write_note(user_id, sid): + continue + sources.append(s) tgt_fields = parse_snippet_fields(target.title, target.body, target.tags) parsed_sources = [ @@ -484,8 +517,9 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): ] locations, extra_tags = merge_snippet_fields(tgt_fields, target.tags, parsed_sources) + # Owner-scoped write, authorised above — same reason as update_snippet. updated = await notes_svc.update_note( - user_id, target_id, + owner_id, target_id, body=compose_body( code=tgt_fields["code"], language=tgt_fields["language"], signature=tgt_fields["signature"], when_to_use=tgt_fields["when_to_use"], @@ -500,7 +534,7 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): from scribe.services.trash import delete as trash_delete merged_ids: list[int] = [] for s in sources: - batch = await trash_delete(user_id, "note", s.id) + batch = await trash_delete(owner_id, "note", s.id) if batch is not None: merged_ids.append(s.id) diff --git a/tests/test_mcp_tool_snippets.py b/tests/test_mcp_tool_snippets.py index a9fb7db..adadc80 100644 --- a/tests/test_mcp_tool_snippets.py +++ b/tests/test_mcp_tool_snippets.py @@ -13,13 +13,17 @@ def _bind_user(): _user_id_ctx.reset(token) -def _fake_snippet(): +def _fake_snippet(user_id: int = 7): n = MagicMock() n.id = 1 n.title = "debounce — rate-limit a callback" n.body = "```js\nreturn 1\n```\n" n.tags = ["js", "snippet"] n.note_type = "snippet" + # Real int, matching the bound caller by default. The tools compare it to + # decide whether to attach a shared/owner marker; an auto-MagicMock would read + # as another user's record and send them off to look up a username. + n.user_id = user_id n.to_dict.return_value = { "id": 1, "title": n.title, "note_type": "snippet", "tags": n.tags, } @@ -128,6 +132,31 @@ async def test_create_and_update_pass_locations_through(): assert mock_update.await_args.kwargs["locations"] == locs +@pytest.mark.asyncio +async def test_update_snippet_read_only_share_says_why(): + """A viewer grant must be refused with the REAL reason. "Not found" would be + a lie about a record the caller can plainly open, and would send an agent + hunting for a missing id instead of recording its own version.""" + with patch("scribe.services.snippets.update_snippet", + AsyncMock(side_effect=PermissionError( + "snippet 1 is shared with you read-only — ask its owner for " + "edit access, or record your own version"))): + from scribe.mcp.tools.snippets import update_snippet + with pytest.raises(ValueError, match="read-only"): + await update_snippet(1, name="x") + + +@pytest.mark.asyncio +async def test_merge_snippets_read_only_target_says_why(): + with patch("scribe.services.snippets.merge_snippets", + AsyncMock(side_effect=PermissionError( + "snippet 1 is shared with you read-only — you can't merge " + "into a record you can't edit"))): + from scribe.mcp.tools.snippets import merge_snippets + with pytest.raises(ValueError, match="read-only"): + await merge_snippets(target_id=1, source_ids=[2]) + + @pytest.mark.asyncio async def test_delete_snippet_retires_or_raises(): from scribe.mcp.tools.snippets import delete_snippet diff --git a/tests/test_shared_write_access.py b/tests/test_shared_write_access.py new file mode 100644 index 0000000..c68abb7 --- /dev/null +++ b/tests/test_shared_write_access.py @@ -0,0 +1,111 @@ +"""Editing a shared record: an editor grant is enough, a viewer grant is not. + +The agent and web paths used to disagree — and worse, within the agent path +`delete_snippet` honoured editor shares while `update_snippet` did not, so a +colleague's snippet could be destroyed through an agent but not improved. Both +now resolve the read scope and then require WRITE, which is what the sharing UI +already promises: viewer / editor / admin are distinct grants. + +A record readable-but-not-writable raises PermissionError rather than reporting +"not found" — the caller can plainly open it, so not-found would be a lie that +sends them hunting for a missing id. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + + +def _snippet(id=1, owner=9): + n = MagicMock() + n.id = id + n.user_id = owner + n.title = "formatDuration — humanize a millisecond count" + n.body = "```ts\nexport const f = 1\n```\n" + n.tags = ["ts", "snippet"] + n.note_type = "snippet" + n.deleted_at = None + return n + + +@pytest.mark.asyncio +async def test_editor_share_can_update_and_writes_as_the_owner(): + """The underlying note update is owner-scoped, so a shared editor's own id + would match nothing — the authorised write has to be performed as the owner.""" + from scribe.services import snippets as svc + note = _snippet(owner=9) + updated = _snippet(owner=9) + with patch.object(svc, "get_snippet", AsyncMock(return_value=note)), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ + patch.object(svc.notes_svc, "update_note", + AsyncMock(return_value=updated)) as mock_update, \ + patch.object(svc, "_embed_snippet", MagicMock()): + got = await svc.update_snippet(7, 1, name="formatDuration") + + assert got is updated + # Positional user_id is the OWNER (9), not the caller (7). + assert mock_update.await_args.args[0] == 9 + + +@pytest.mark.asyncio +async def test_viewer_share_cannot_update(): + from scribe.services import snippets as svc + with patch.object(svc, "get_snippet", AsyncMock(return_value=_snippet())), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=False)), \ + patch.object(svc.notes_svc, "update_note", AsyncMock()) as mock_update: + with pytest.raises(PermissionError, match="read-only"): + await svc.update_snippet(7, 1, name="nope") + mock_update.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_viewer_share_cannot_delete(): + """delete_snippet returns False rather than raising — its contract is boolean + — but a viewer must not be able to bin someone else's record.""" + from scribe.services import snippets as svc + with patch.object(svc, "get_snippet", AsyncMock(return_value=_snippet())), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=False)), \ + patch("scribe.services.trash.delete", AsyncMock()) as mock_trash: + assert await svc.delete_snippet(7, 1) is False + mock_trash.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_unreadable_record_is_not_found_not_forbidden(): + """A record the caller can't see at all must NOT be distinguishable from one + that doesn't exist — otherwise the error itself confirms it exists.""" + from scribe.services import snippets as svc + with patch.object(svc, "get_snippet", AsyncMock(return_value=None)): + assert await svc.update_snippet(7, 404, name="x") is None + assert await svc.delete_snippet(7, 404) is False + + +@pytest.mark.asyncio +async def test_merge_requires_write_on_target(): + from scribe.services import snippets as svc + with patch.object(svc, "get_snippet", AsyncMock(return_value=_snippet())), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=False)): + with pytest.raises(PermissionError, match="read-only"): + await svc.merge_snippets(7, 1, [2]) + + +@pytest.mark.asyncio +async def test_merge_skips_sources_owned_by_someone_else(): + """Cross-owner merge stays out of scope (#231): a source belonging to a + different owner than the target is skipped, not silently folded in and + trashed.""" + from scribe.services import snippets as svc + target = _snippet(id=1, owner=9) + same_owner = _snippet(id=2, owner=9) + other_owner = _snippet(id=3, owner=42) + + async def fake_get(_uid, sid): + return {1: target, 2: same_owner, 3: other_owner}.get(sid) + + with patch.object(svc, "get_snippet", AsyncMock(side_effect=fake_get)), \ + patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ + patch.object(svc.notes_svc, "update_note", AsyncMock(return_value=target)), \ + patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \ + patch.object(svc, "_embed_snippet", MagicMock()): + _note, merged_ids = await svc.merge_snippets(7, 1, [2, 3]) + + assert merged_ids == [2]