3ffdbbc521
CI & Build / Python lint (push) Successful in 3s
CI & Build / integration (push) Successful in 33s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Failing after 34s
CI & Build / Build & push image (push) Has been skipped
Option B, per the operator. Closes the last inconsistency from the ACL work.
The agent path had drifted into an indefensible position: delete_snippet honoured
editor shares (I made it share-aware so widening the read wouldn't let a VIEWER
trash things) while update_snippet still resolved through the owner-only
notes.get_note. So through an agent you could destroy a colleague's snippet but
not improve it — and the refusal claimed "not found" for a record you could
plainly open.
Now update_snippet, merge_snippets and update_process all resolve the read scope
and then require can_write_note, matching the REST routes and the sharing UI's
own promise that viewer / editor / admin are distinct grants. A viewer grant is
refused with the actual reason ("shared with you read-only — ask its owner for
edit access, or record your own version"), because not-found would send an agent
hunting for a missing id instead of recording its own copy.
Authorised writes are performed as the OWNER, since the underlying note update is
owner-scoped and a shared editor's own id would match nothing.
Merge additionally requires each source to share the TARGET'S owner and to be
writable by the caller — merging trashes the source, so read access isn't enough,
and cross-owner merge stays out of scope (#231). Sources failing either test are
skipped rather than half-merged.
A record the caller cannot read at all still returns not-found rather than
forbidden, so the error can't be used to confirm that an id exists.
Also fixed _fake_snippet's missing user_id proactively — the same
auto-MagicMock-reads-as-foreign trap that broke CI twice (see note 2109).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt
112 lines
4.9 KiB
Python
112 lines
4.9 KiB
Python
"""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]
|