fix(systems): System tagging works from whichever door wrote the record, as whoever wrote it (#4249) #178
@@ -190,10 +190,12 @@ async def update_process(process_id: int, title: str = "", body: str = "",
|
||||
fields["tags"] = tags
|
||||
# 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)
|
||||
# Written as the OWNER, matching the update above: an editor-shared process
|
||||
# keeps its owner's associations rather than sprouting a second set.
|
||||
# The CALLER, not the owner. `update_note` above is owner-scoped because
|
||||
# the service demands it; `set_record_systems` is not — it runs its own
|
||||
# share-aware check and links only Systems the acting user can read, so
|
||||
# passing the owner would bypass the check and borrow their reach (#47).
|
||||
if system_ids is not None:
|
||||
await systems_svc.set_record_systems(note.user_id, process_id, system_ids)
|
||||
await systems_svc.set_record_systems(uid, process_id, system_ids)
|
||||
if updated is None:
|
||||
raise ValueError(f"process {process_id} not found")
|
||||
out = updated.to_dict()
|
||||
|
||||
@@ -483,7 +483,10 @@ async def update_snippet(
|
||||
if note is None:
|
||||
raise ValueError(f"snippet {snippet_id} not found")
|
||||
if system_ids is not None:
|
||||
await systems_svc.set_record_systems(note.user_id, snippet_id, system_ids)
|
||||
# The CALLER, not the owner (#4249). `update_snippet` above already
|
||||
# raised PermissionError if this user may not write, so the tagging
|
||||
# runs with the actor's own System visibility rather than the owner's.
|
||||
await systems_svc.set_record_systems(uid, snippet_id, system_ids)
|
||||
data = snippets_svc.snippet_to_dict(note)
|
||||
data.update(await access_svc.describe_provenance(uid, note))
|
||||
await systems_tools.attach_systems(
|
||||
|
||||
@@ -240,9 +240,13 @@ async def update_lesson_route(lesson_id: int):
|
||||
if updated is None:
|
||||
return not_found("Lesson")
|
||||
if data.get("system_ids") is not None:
|
||||
await systems_svc.set_record_systems(
|
||||
owner_uid, lesson_id, data["system_ids"]
|
||||
)
|
||||
# The CALLER, not owner_uid (#4249). `set_record_systems` runs its own
|
||||
# `can_write_note` and links only Systems the acting user can read;
|
||||
# handing it the owner makes that check trivially pass and filters by
|
||||
# the owner's visibility instead. The caller's write permission is
|
||||
# already established above, so this neither loosens nor tightens who
|
||||
# may edit — it decides WHOSE reach the tagging uses (#47).
|
||||
await systems_svc.set_record_systems(uid, lesson_id, data["system_ids"])
|
||||
out = lessons_svc.lesson_to_dict(updated)
|
||||
out["systems"] = [
|
||||
s.to_dict()
|
||||
|
||||
@@ -208,7 +208,8 @@ async def update_snippet_route(snippet_id: int):
|
||||
if updated is None:
|
||||
return not_found("Snippet")
|
||||
if data.get("system_ids") is not None:
|
||||
await systems_svc.set_record_systems(owner_uid, snippet_id, data["system_ids"])
|
||||
# The CALLER, not owner_uid — see routes/lessons.py for the why (#4249).
|
||||
await systems_svc.set_record_systems(uid, snippet_id, data["system_ids"])
|
||||
out = snippets_svc.snippet_to_dict(updated)
|
||||
out["systems"] = [
|
||||
s.to_dict() for s in await systems_svc.list_record_systems(owner_uid, snippet_id)
|
||||
|
||||
@@ -178,6 +178,60 @@ def test_the_registry_names_functions_that_exist():
|
||||
assert not missing, f"registry names functions that no longer exist: {missing}"
|
||||
|
||||
|
||||
def test_every_tagging_write_acts_as_the_caller():
|
||||
"""`set_record_systems` is handed the ACTING user, never the record's owner.
|
||||
|
||||
THE ARGUMENT. `set_record_systems` is not a dumb setter — it runs its own
|
||||
`can_write_note` and then links only Systems the given user can READ.
|
||||
Handing it `note.user_id` therefore does two things at once, both quiet:
|
||||
the access check becomes trivially true (an owner can always write their
|
||||
own record), and the System filter uses the owner's visibility instead of
|
||||
the actor's. On a single-user install those are invisible. With a share
|
||||
they are an editor acting with the owner's reach — the shape #47 exists to
|
||||
prevent, and the same reasoning routes/notes.py already applies to
|
||||
`set_supersedes`.
|
||||
|
||||
It is NOT a permission loosening either way: every call site establishes
|
||||
the caller's write access first (a route's `can_write_note`, or a service
|
||||
that raises PermissionError). What this decides is WHOSE reach the tagging
|
||||
runs with, which is exactly the kind of difference that survives review
|
||||
because each call site reads fine on its own.
|
||||
|
||||
Four of twenty sites passed the owner before #4249 — two routes, two MCP
|
||||
tools, one of them written earlier in the very session that unified them.
|
||||
That is the tell that this needs a guard rather than care.
|
||||
"""
|
||||
offenders = []
|
||||
for path in ROOT.rglob("*.py"):
|
||||
src = path.read_text()
|
||||
if "set_record_systems" not in src:
|
||||
continue
|
||||
tree = ast.parse(src)
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
fn = node.func
|
||||
name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None)
|
||||
if name != "set_record_systems" or not node.args:
|
||||
continue
|
||||
first = node.args[0]
|
||||
# The acting user arrives as a bare local — `uid` in the tools and
|
||||
# routes, `user_id` in the service's own signature. An attribute
|
||||
# access (`note.user_id`, `target.user_id`) is a record's owner,
|
||||
# and `owner_uid` is the same thing already unpacked.
|
||||
ok = isinstance(first, ast.Name) and first.id in {"uid", "user_id"}
|
||||
if not ok:
|
||||
shown = ast.unparse(first) if hasattr(ast, "unparse") else "?"
|
||||
rel = path.relative_to(ROOT.parent.parent)
|
||||
offenders.append(f"{rel}:{node.lineno} passes {shown!r}")
|
||||
assert not offenders, (
|
||||
"set_record_systems must be called with the acting user, not the "
|
||||
"record's owner — passing the owner bypasses its own access check and "
|
||||
"borrows the owner's System visibility (#47, #4249):\n "
|
||||
+ "\n ".join(offenders)
|
||||
)
|
||||
|
||||
|
||||
def test_milestones_are_absent_on_purpose():
|
||||
"""The one 'gap' that is not a gap, pinned so it is not re-opened.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user