Skip to content

fix: Resolve dataset storage under its owner - #4850

Draft
dexters1 wants to merge 1 commit into
devfrom
fix/COG-issue-4829-dataset-database-owner-filter
Draft

fix: Resolve dataset storage under its owner#4850
dexters1 wants to merge 1 commit into
devfrom
fix/COG-issue-4829-dataset-database-owner-filter

Conversation

@dexters1

@dexters1 dexters1 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Description

Fixes #4829.

An ACL grantee calling forget() on a shared dataset crashed with UNIQUE constraint failed: dataset_database.dataset_id. Two defects, both fixed inside the dataset-resolution mechanism (single-file core change, no call-site churn):

  1. _existing_dataset_database now looks up by dataset_id alone. dataset_id is the table's sole primary key — one row per dataset by schema design since Dataset permissions #869 — so the owner_id filter could only ever turn a hit into a false negative, sending the function into a doomed duplicate INSERT (after already running the graph/vector handlers' create_dataset provisioning for the wrong user).

  2. apply_database_context_variables now derives the dataset's owner itself instead of trusting the caller-supplied user_id for storage. The registry row, physical database paths (databases/<owner>/…), and data root always belonged to the owner; previously they were built from whatever id the call site passed, so one wrong call site (forget passed the caller) redirected all of them — the crash above, and on the memory_only paths silently-wrong embedded-DB targets. Now no caller identity can redirect storage.

Deriving the owner reads the dataset row, so entering the context for a nonexistent dataset now fails fast with DatasetNotFoundError — previously it silently provisioned phantom registry rows on SQLite (FK pragma not enabled on that path) or crashed on FK after handler side effects on Postgres.

With storage no longer depending on it, user_id is now optional — it is consulted only by the new opt-in permission gate: pass permission_type="read"/"write"/"delete"/"share" to set_database_global_context_variables to have the caller's grant verified on entry (a permission_type without a user_id is refused). When permission_type is omitted (all current call sites), no permission check is performed here — authorization stays at the API layer exactly as today.

Verification

  • Live repro of the issue scenario (owner A, grantee B with ACL grants, B calls forget(data_id, dataset_id)): IntegrityError before → {"status": "success"} after, data record removed, single registry row still owned by A, no stray directories for B.
  • cognee/tests/test_delete_permission.py (e2e) extended: grantee forget() end-to-end, plus a direct assertion that get_or_create_dataset_database returns the owner's row to a non-owner.
  • New unit tests in test_context_global_variables.py: storage resolves under the owner for a non-owner caller (no permission check without permission_type), and a denied caller with permission_type set is rejected before any storage side effect.
  • Full affected unit suites pass (29/29); ruff/pre-commit clean; ty diagnostics unchanged except one more instance of the pre-existing SQLAlchemy Column typing noise.

Notes

DCO Affirmation

I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.

https://claude.ai/code/session_01QHTS3zLjcskWm1qXDPT1Lx

@dexters1
dexters1 force-pushed the fix/COG-issue-4829-dataset-database-owner-filter branch from 4d74249 to 758f713 Compare September 1, 2026 11:36
@dexters1 dexters1 changed the title fix: Return dataset database row to ACL grantees fix: Resolve dataset storage under its owner Sep 1, 2026
Description: The dataset_database existence check filtered by owner_id
even though dataset_id is the table's sole primary key, so an ACL
grantee calling forget() got a false negative and the fall-through
INSERT crashed on the primary key. The deeper cause: the database
context trusted the caller-supplied user_id to locate storage (registry
row, database paths, data root), so any call site passing a non-owner
redirected all of it.

Derive the owner from the dataset itself on context entry — a caller
identity can no longer redirect storage, and a nonexistent dataset now
fails fast instead of provisioning phantom registry rows. Look the
registry row up by dataset_id alone, matching the schema. user_id is now
optional and consulted only by the new opt-in permission gate: pass
permission_type to have the caller's grant verified on entry; when
omitted, authorization stays at the API layer as today.

Covered by a grantee forget scenario in the delete permission e2e test
and new context-manager unit tests.

Fixes #4829

Claude-Session: https://claude.ai/code/session_01QHTS3zLjcskWm1qXDPT1Lx
@dexters1
dexters1 force-pushed the fix/COG-issue-4829-dataset-database-owner-filter branch from 758f713 to b0cddec Compare September 1, 2026 11:47
Comment on lines 219 to +227

await dataset_queue().ensure_slot(dataset)

user = await get_user(user_id)
# Optional permission gate: checked only when the caller asked for it
# by passing a permission_type — callers that already authorized at the
# API layer pass nothing and no check is performed here.
if permission_type is not None:
if user_id is None:
raise CogneeValidationError(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor (correctness verified) — The permission check looks correct: it gates entry before storage is touched (line 91), and the owner resolution on line 227 ensures storage always derives from the dataset's owner, not the caller. Good fix for #4829.

One note: _get_dataset_owner_id has no authorization (intentionally, per its docstring), so any caller can learn who owns any dataset. This is probably acceptable since the permission check happens just above, but worth noting for the security model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant