ef1dbdfc86
CI & Build / Python lint (push) Successful in 3s
CI & Build / Python tests (push) Failing after 31s
CI & Build / TypeScript typecheck (push) Successful in 34s
CI & Build / integration (push) Successful in 34s
CI & Build / Build & push image (push) Has been skipped
Closes #2092 and the Knowledge-browse provenance gap. The two halves of a hybrid search disagreed: the keyword half honoured shares while the semantic half was pinned to NoteEmbedding.user_id, so a shared record was findable by wording and invisible by meaning — the case a semantic search exists to serve. semantic_search_notes now scopes on Note via a `scope` parameter, and each of its five callers declares which kind of act it is: mcp/tools/search.py read the agent asked routes/search.py read the user typed it knowledge.py (semantic) read matches the keyword half beside it plugin_context.py browse nobody asked; never a one-to-one share dedup.py own a verdict that blocks a write must not hinge on another person's notes That last one is the reason this isn't a single global widening: the dedup gate returns "update the existing one instead", so matching a stranger's record would refuse a legitimate create and point at something the caller can't edit. Scope defaults to "own" so a caller that forgets is wrong in the safe direction, and an unknown scope raises rather than falling back — a typo there would be a data-exposure bug. Auto-inject keeps the browse scope, which still admits a collaborator's note via a shared project. Its menu line is the only provenance an agent sees, so a foreign hit now reads: #12 "Title" (0.71) - shared by alex, treat as a suggestion. MCP and REST search results carry shared/owner too. Knowledge browse: the feed hydrates cards from /api/knowledge/batch rather than the list route, so both paths label rows now, and KnowledgeView shows "by <owner>" on records the viewer doesn't own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt
135 lines
5.3 KiB
Python
135 lines
5.3 KiB
Python
"""The two ACL predicates behind list queries.
|
|
|
|
`get_note_permission` answers "may I read THIS note?" one row at a time, which a
|
|
list query can't use. These express the same resolution as set membership. They
|
|
are pure SQL builders — group membership is a subquery rather than a fetched
|
|
list, so they need no session and callers' unit tests need not know they exist.
|
|
|
|
Two scopes, deliberately different (decision note 2094):
|
|
readable_* — everything the ACL permits, for explicit acts (a typed search, a
|
|
fetch by id).
|
|
browsable_* — owner + project access only, for passive surfaces (browse lists,
|
|
facet counts, the process→skill manifest).
|
|
|
|
Assertions compile each clause to SQL and inspect its shape.
|
|
"""
|
|
import pytest
|
|
|
|
from scribe.services.access import (
|
|
browsable_notes_clause,
|
|
notes_visibility_clause,
|
|
readable_notes_clause,
|
|
)
|
|
|
|
|
|
def _sql(clause) -> str:
|
|
return str(clause.compile(compile_kwargs={"literal_binds": True}))
|
|
|
|
|
|
def _read(user_id: int = 7) -> str:
|
|
return _sql(readable_notes_clause(user_id))
|
|
|
|
|
|
def _browse(user_id: int = 7) -> str:
|
|
return _sql(browsable_notes_clause(user_id))
|
|
|
|
|
|
# --- read scope --------------------------------------------------------------
|
|
|
|
def test_read_scope_covers_ownership_and_every_share_path():
|
|
sql = _read()
|
|
assert "notes.user_id = 7" in sql # 1. ownership
|
|
assert "note_shares" in sql # 2/3. direct or group note share
|
|
assert "notes.project_id IN" in sql # 4. inherited from a shared project
|
|
assert "project_shares" in sql
|
|
|
|
|
|
def test_read_scope_resolves_group_membership_in_sql():
|
|
"""Group ids are a subquery, not a pre-fetched list — that's what keeps this
|
|
a pure function with no session of its own."""
|
|
sql = _read()
|
|
assert "group_memberships.user_id = 7" in sql
|
|
# Both the note-level and project-level share lookups consult it. Count FROM
|
|
# clauses rather than bare occurrences — each rendered subquery names the
|
|
# table in SELECT, FROM and WHERE — and compare loosely, since the assertion
|
|
# is about the arms existing, not about SQLAlchemy's formatting.
|
|
assert sql.count("FROM group_memberships") >= 2
|
|
assert _browse().count("FROM group_memberships") >= 1
|
|
|
|
|
|
def test_read_scope_is_never_the_whole_table():
|
|
"""Guard against the predicate degrading to always-true, which would expose
|
|
every user's notes to every other user."""
|
|
sql = _read().lower()
|
|
assert " true" not in sql
|
|
assert "1 = 1" not in sql
|
|
|
|
|
|
# --- browse scope: the trust boundary ---------------------------------------
|
|
|
|
def test_browse_scope_excludes_direct_note_shares():
|
|
"""The whole point of the narrower scope. If `note_shares` leaks in here, a
|
|
record someone shared one-to-one with the operator lands in their own browse
|
|
list, facet counts and skill manifest as though they had recorded it."""
|
|
assert "note_shares" not in _browse()
|
|
|
|
|
|
def test_browse_scope_keeps_ownership_and_project_access():
|
|
sql = _browse()
|
|
assert "notes.user_id = 7" in sql # your own records
|
|
assert "project_shares" in sql # a project shared with you
|
|
assert "projects" in sql # a project you own
|
|
|
|
|
|
def test_browse_scope_is_strictly_narrower_than_read_scope():
|
|
"""Browse must never surface something read scope wouldn't also allow, or a
|
|
list could show a record the caller cannot then open."""
|
|
browse, read = _browse(), _read()
|
|
assert "note_shares" in read and "note_shares" not in browse
|
|
for arm in ("notes.user_id = 7", "project_shares"):
|
|
assert arm in browse and arm in read
|
|
|
|
|
|
def test_browse_scope_is_never_the_whole_table():
|
|
sql = _browse().lower()
|
|
assert " true" not in sql
|
|
assert "1 = 1" not in sql
|
|
|
|
|
|
# --- the scope resolver ------------------------------------------------------
|
|
|
|
def test_own_scope_is_ownership_alone():
|
|
"""The near-duplicate gate depends on this: its verdict must not turn on
|
|
another person's records, or it would refuse a write and point the caller at
|
|
something they may not be able to edit."""
|
|
sql = _sql(notes_visibility_clause(7, "own"))
|
|
assert sql == "notes.user_id = 7"
|
|
|
|
|
|
def test_scope_resolver_maps_to_the_right_clauses():
|
|
assert _sql(notes_visibility_clause(7, "browse")) == _browse()
|
|
assert _sql(notes_visibility_clause(7, "read")) == _read()
|
|
|
|
|
|
def test_scope_defaults_to_the_narrowest():
|
|
"""A caller that forgets to choose must be wrong in the safe direction."""
|
|
assert _sql(notes_visibility_clause(7)) == _sql(notes_visibility_clause(7, "own"))
|
|
|
|
|
|
def test_unknown_scope_is_rejected_loudly():
|
|
"""Silently falling back would turn a typo into a data-exposure bug."""
|
|
with pytest.raises(ValueError):
|
|
notes_visibility_clause(7, "everything")
|
|
|
|
|
|
@pytest.mark.parametrize("clause_fn", [readable_notes_clause, browsable_notes_clause])
|
|
def test_clauses_are_pure_builders(clause_fn):
|
|
"""Synchronous and side-effect free — no coroutine, no session of their own.
|
|
|
|
This is the property that keeps them usable: when they opened their own
|
|
session, every unrelated service test had to know they existed and stub them,
|
|
and four test modules broke the moment a service started calling one."""
|
|
import inspect
|
|
assert not inspect.iscoroutinefunction(clause_fn)
|
|
assert _sql(clause_fn(7)) # builds without touching a database
|