fix(acl): align MCP writes with the share model — editor edits, viewer doesn't
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
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
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user