Skip to content

fix(artifact): restore collection-level permission fallback for child read/write ops - #1040

Open
nilsmechtel wants to merge 1 commit into
amun-ai:mainfrom
nilsmechtel:restore-collection-permission-fallback
Open

fix(artifact): restore collection-level permission fallback for child read/write ops#1040
nilsmechtel wants to merge 1 commit into
amun-ai:mainfrom
nilsmechtel:restore-collection-permission-fallback

Conversation

@nilsmechtel

Copy link
Copy Markdown
Contributor

A user holding a sufficient permission on a PARENT collection (e.g. a model-zoo reviewer with "rw+") was denied edit/commit/put_file on a CHILD artifact whose own config.permissions listed only its uploader (bioimage-io/tough-lion). Root cause: the parent-collection fallback in _get_artifact_with_permission was hardcoded to a read-only operation allow-list, so write ops skipped the parent check and fell through to the workspace-scope check only — which a collection-scoped reviewer does not satisfy. This regressed at 3cea1bd ("Support terminal worker"), which narrowed the previously-blanket parent fallback (16430e8) to read ops.

Fix: gate the parent fallback on the required permission LEVEL instead of a hardcoded op list — allow it for any non-admin operation (read AND read_write: edit, commit, put_file, remove_file, discard_changes, ...), including staged/versionless children. delete/reset_stats/publish/get_secret map to UserPermission.admin and are excluded, so delete stays owner/admin-only and cannot be granted through a collection-level permission. The parent check validates the parent's own ACL, so no cross-workspace leak.

Adds tests/test_artifact.py::test_collection_permission_fallback_for_children (real two-user integration test): reviewer with collection rw+ can edit a committed child + edit/put_file/commit a staged child; delete is denied; a read-only (r) reviewer is denied edit.

… read/write ops

A user holding a sufficient permission on a PARENT collection (e.g. a
model-zoo reviewer with "rw+") was denied edit/commit/put_file on a CHILD
artifact whose own config.permissions listed only its uploader
(bioimage-io/tough-lion). Root cause: the parent-collection fallback in
_get_artifact_with_permission was hardcoded to a read-only operation
allow-list, so write ops skipped the parent check and fell through to the
workspace-scope check only — which a collection-scoped reviewer does not
satisfy. This regressed at 3cea1bd ("Support terminal worker"), which
narrowed the previously-blanket parent fallback (16430e8) to read ops.

Fix: gate the parent fallback on the required permission LEVEL instead of a
hardcoded op list — allow it for any non-admin operation (read AND
read_write: edit, commit, put_file, remove_file, discard_changes, ...),
including staged/versionless children. delete/reset_stats/publish/get_secret
map to UserPermission.admin and are excluded, so delete stays
owner/admin-only and cannot be granted through a collection-level permission.
The parent check validates the parent's own ACL, so no cross-workspace leak.

Adds tests/test_artifact.py::test_collection_permission_fallback_for_children
(real two-user integration test): reviewer with collection rw+ can edit a
committed child + edit/put_file/commit a staged child; delete is denied; a
read-only (r) reviewer is denied edit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@oeway

oeway commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Thanks for this — I dug into the failing CI and the security model, and I want to lay out precisely what's going on so we can decide the right path together.

The CI failure is a real semantic conflict, not a flake

The red build fails at tests/test_artifact.py::test_permission_isolation_between_artifacts:

AssertionError: User 2 should NOT be able to edit User 1's artifact!

Your new test test_collection_permission_fallback_for_children passes — but it asserts the exact opposite invariant of an existing security test, so the two are mutually exclusive and cannot both be green.

Crucially, test_permission_isolation_between_artifacts (docstring: "users can only edit artifacts they own, even if parent grants permission") was introduced in commit 3cea1bd0 (PR #807) — the same commit that narrowed the parent fallback to read-only. So the read-only narrowing this PR frames as an accidental "regression" was a deliberate, test-backed security tightening, not a bug that slipped in.

The diff is safe on some axes but reverses a documented invariant on writes

Credit where due — the level-gate approach is coherent and safe on two axes I checked closely:

  • No cross-workspace hole: the fallback calls _check_permissions(parent_artifact, …), a genuine ACL check on the parent, which is always same-workspace as the child. No V10–V14 pattern.
  • Delete/admin correctly excluded: delete, reset_stats, publish, get_secret map to admin and are gated out, so the two-grant delete model (CLAUDE.md Bump s3fs from 2021.10.0 to 2021.10.1 #10) is untouched.

But it weakens the documented model on writes. Our permission model (CLAUDE.md, learned via #9) blesses only parent-read → child-read fallback ("child-read ≈ parent-read"). There is no documented parent-write → child-write fallback. This PR makes child-write ≈ parent-write for the whole collection, and it grants via plain rw too (edit ∈ rw), not only the rw+ "reviewer" role the description frames. Combined with the enumeration property (anyone with list sees every child), that means any user with rw/rw+ on a shared collection can edit/commit/put_file/remove_file on every contributor's child artifact regardless of the child's own ACL — exactly the intra-collection write-isolation guarantee #807 was written to protect.

What's needed to move forward

This is a genuine product/security decision, not something to silently flip:

  1. Decide the invariant: should collection rw/rw+ confer write on children owned by others? The two tests directly contradict; one has to win explicitly.
  2. If yes (reviewer-role semantics are desired): test_permission_isolation_between_artifacts and the CLAUDE.md Artifact Permission Model must be updated deliberately to document the new child-write ≈ parent-write rule and its blast radius, and Support terminal worker #807's rationale revisited. Ideally scope it to an explicit, opt-in "reviewer"/mutate grant rather than a blanket level gate that also catches plain rw.
  3. If no: grant child write only via the child's own ACL (or a narrower opt-in grant), leaving the isolation test intact.

I'm holding off on merging pending that call (cc @oeway for the product decision on the reviewer-write semantics). Happy to help implement whichever direction is chosen. The branch itself rebases cleanly onto main.

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.

2 participants