fix(acl): make the visibility predicates pure builders; repair CI
CI & Build / Python lint (push) Successful in 3s
CI & Build / integration (push) Successful in 21s
CI & Build / TypeScript typecheck (push) Successful in 49s
CI & Build / Python tests (push) Successful in 57s
CI & Build / Build & push image (push) Successful in 1m5s
CI & Build / Python lint (push) Successful in 3s
CI & Build / integration (push) Successful in 21s
CI & Build / TypeScript typecheck (push) Successful in 49s
CI & Build / Python tests (push) Successful in 57s
CI & Build / Build & push image (push) Successful in 1m5s
Run 2888 failed with 7 tests down, both causes mine. The real problem was a design flaw, not the tests: readable_notes_clause and browsable_notes_clause each opened their own DB session to fetch the caller's group ids. That made them unmockable at the call site, so every unrelated service test suddenly had to know they existed and stub them — four modules broke the moment a service started calling one, and one of my own stubs patched the wrong name (readable_* where the code had moved to browsable_*). Fixed at the root: group membership is now a SUBQUERY rather than a fetched list, so both clauses are synchronous pure functions with no session. One fewer round-trip per query, membership folded into the statement the caller was already running, and nothing for callers' tests to mock. The "no groups means no group arm" special case disappears too — an empty subquery simply matches nothing. Also: _fake_note in the process tool tests had no real user_id, so its auto-MagicMock attribute reached session.get(User, ...) through the new provenance check and SQLAlchemy rejected it. The fixture now takes a real user_id defaulting to the bound caller, which makes "is this shared?" meaningful, and gains a case asserting another user's process comes back flagged. Test assertions on compiled SQL are deliberately loose about formatting: the local env has no SQLAlchemy (rule #10), so they check that the arms exist rather than guessing at exact rendering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt
This commit is contained in:
@@ -182,7 +182,17 @@ async def can_write_note(user_id: int, note_id: int) -> bool:
|
||||
# you has not earned that standing, so it waits until you go looking for it.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
async def readable_notes_clause(user_id: int):
|
||||
def _my_group_ids(user_id: int):
|
||||
"""The caller's group ids as a SUBQUERY, not a fetched list.
|
||||
|
||||
Keeping it in SQL is what makes the clause builders below pure functions —
|
||||
no session, no await, nothing for a caller's unit test to mock — and it folds
|
||||
the membership lookup into the one statement the caller was already running.
|
||||
"""
|
||||
return select(GroupMembership.group_id).where(GroupMembership.user_id == user_id)
|
||||
|
||||
|
||||
def readable_notes_clause(user_id: int):
|
||||
"""A SQLAlchemy WHERE predicate matching every note this user may READ.
|
||||
|
||||
`get_note_permission` answers "may I read THIS note?" one row at a time,
|
||||
@@ -199,17 +209,20 @@ async def readable_notes_clause(user_id: int):
|
||||
too, or a note becomes findable but not openable (or the reverse — which is
|
||||
the bug this was written to fix).
|
||||
"""
|
||||
async with async_session() as session:
|
||||
group_ids = await _user_group_ids(session, user_id)
|
||||
groups = _my_group_ids(user_id)
|
||||
|
||||
note_share_conds = [NoteShare.shared_with_user_id == user_id]
|
||||
project_share_conds = [ProjectShare.shared_with_user_id == user_id]
|
||||
if group_ids:
|
||||
note_share_conds.append(NoteShare.shared_with_group_id.in_(group_ids))
|
||||
project_share_conds.append(ProjectShare.shared_with_group_id.in_(group_ids))
|
||||
|
||||
shared_note_ids = select(NoteShare.note_id).where(or_(*note_share_conds))
|
||||
shared_project_ids = select(ProjectShare.project_id).where(or_(*project_share_conds))
|
||||
shared_note_ids = select(NoteShare.note_id).where(
|
||||
or_(
|
||||
NoteShare.shared_with_user_id == user_id,
|
||||
NoteShare.shared_with_group_id.in_(groups),
|
||||
)
|
||||
)
|
||||
shared_project_ids = select(ProjectShare.project_id).where(
|
||||
or_(
|
||||
ProjectShare.shared_with_user_id == user_id,
|
||||
ProjectShare.shared_with_group_id.in_(groups),
|
||||
)
|
||||
)
|
||||
|
||||
return or_(
|
||||
Note.user_id == user_id,
|
||||
@@ -269,7 +282,7 @@ async def label_shared_items(user_id: int, items: list[dict]) -> list[dict]:
|
||||
return items
|
||||
|
||||
|
||||
async def browsable_notes_clause(user_id: int):
|
||||
def browsable_notes_clause(user_id: int):
|
||||
"""A WHERE predicate for PASSIVE surfaces: the caller's own notes, plus
|
||||
notes in a project they can reach (owned or shared).
|
||||
|
||||
@@ -283,14 +296,12 @@ async def browsable_notes_clause(user_id: int):
|
||||
project that you don't own therefore never matches — `NULL IN (...)` is not
|
||||
true — which is exactly the intent.
|
||||
"""
|
||||
async with async_session() as session:
|
||||
group_ids = await _user_group_ids(session, user_id)
|
||||
|
||||
project_share_conds = [ProjectShare.shared_with_user_id == user_id]
|
||||
if group_ids:
|
||||
project_share_conds.append(ProjectShare.shared_with_group_id.in_(group_ids))
|
||||
|
||||
shared_project_ids = select(ProjectShare.project_id).where(or_(*project_share_conds))
|
||||
shared_project_ids = select(ProjectShare.project_id).where(
|
||||
or_(
|
||||
ProjectShare.shared_with_user_id == user_id,
|
||||
ProjectShare.shared_with_group_id.in_(_my_group_ids(user_id)),
|
||||
)
|
||||
)
|
||||
owned_project_ids = select(Project.id).where(Project.user_id == user_id)
|
||||
|
||||
return or_(
|
||||
|
||||
@@ -94,7 +94,7 @@ async def query_knowledge(
|
||||
|
||||
# No query = browsing. Narrower scope: a record shared directly with the
|
||||
# caller is search-only and must not appear in an ambient list.
|
||||
visible = await browsable_notes_clause(user_id)
|
||||
visible = browsable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
base = select(Note).where(visible)
|
||||
|
||||
@@ -152,7 +152,7 @@ async def _semantic_knowledge_search(
|
||||
try:
|
||||
# A typed query is an explicit act, so it reaches the caller's full read
|
||||
# scope — including records shared directly with them.
|
||||
visible = await readable_notes_clause(user_id)
|
||||
visible = readable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
pattern = f"%{q}%"
|
||||
base = (
|
||||
@@ -231,7 +231,7 @@ async def get_knowledge_tags(user_id: int, note_type: str | None = None) -> list
|
||||
Follows the browse list rather than the read scope: a facet is itself a
|
||||
passive surface, and offering a tag that only a search-only record carries
|
||||
would filter the visible list down to nothing."""
|
||||
visible = await browsable_notes_clause(user_id)
|
||||
visible = browsable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
base = (
|
||||
select(func.unnest(Note.tags).label("tag"))
|
||||
@@ -247,7 +247,7 @@ async def get_knowledge_counts(user_id: int, tags: list[str] | None = None) -> d
|
||||
"""Per-type counts for the sidebar, over what this user can BROWSE — so the
|
||||
numbers match the list they sit beside rather than promising rows that only a
|
||||
search would surface."""
|
||||
visible = await browsable_notes_clause(user_id)
|
||||
visible = browsable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
# Count non-task types
|
||||
stmt = (
|
||||
@@ -316,7 +316,7 @@ async def query_knowledge_ids(
|
||||
return [item["id"] for item in items], total
|
||||
|
||||
# Browsing (see query_knowledge) — narrower scope.
|
||||
visible = await browsable_notes_clause(user_id)
|
||||
visible = browsable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
base = select(Note.id).where(visible)
|
||||
|
||||
@@ -347,7 +347,7 @@ async def get_knowledge_by_ids(user_id: int, ids: list[int]) -> list[dict]:
|
||||
return []
|
||||
# Fetching specific ids is explicit, so this takes the full read scope — the
|
||||
# ids came from either a browse or a search, and both must resolve.
|
||||
visible = await readable_notes_clause(user_id)
|
||||
visible = readable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
stmt = (
|
||||
select(Note)
|
||||
|
||||
@@ -425,7 +425,7 @@ async def resolve_process(user_id: int, name_or_id) -> tuple[Note | None, list[d
|
||||
matches.
|
||||
"""
|
||||
from scribe.services.access import readable_notes_clause
|
||||
visible = await readable_notes_clause(user_id)
|
||||
visible = readable_notes_clause(user_id)
|
||||
async with async_session() as session:
|
||||
base = select(Note).where(
|
||||
visible,
|
||||
|
||||
Reference in New Issue
Block a user