diff --git a/hypha/artifact.py b/hypha/artifact.py index 1679eca61..ed6cc23db 100644 --- a/hypha/artifact.py +++ b/hypha/artifact.py @@ -2540,11 +2540,25 @@ async def _get_artifact_with_permission( if artifact_permissions: for perm in artifact_permissions: if not await self._check_permissions(artifact, user_info, perm): - # Try parent as fallback for read operations - if perm in ["read", "list", "get_file", "list_files", "get_vector", "list_vectors", "search_vectors"]: - if parent_artifact and await self._check_permissions(parent_artifact, user_info, perm): + # Fall back to the PARENT COLLECTION for read AND read_write + # operations. A user who holds a sufficient permission on the + # parent collection (e.g. a model-zoo reviewer with "rw+") is + # authorized for read/write operations (edit, commit, put_file, + # remove_file, discard_changes, ...) on its child artifacts, + # INCLUDING staged/versionless children whose own config only + # lists the uploader. Admin-level operations (delete, + # reset_stats, publish, get_secret) are deliberately EXCLUDED + # from this fallback so that deletion stays owner/admin-only and + # cannot be granted through a collection-level permission. + if ( + operation_map.get(perm, UserPermission.read_write) + != UserPermission.admin + ): + if parent_artifact and await self._check_permissions( + parent_artifact, user_info, perm + ): continue # Permission granted through parent - + # Check workspace-level permission as final fallback # Workspace owners always have highest permission if not user_info.check_permission(artifact.workspace, operation_map.get(perm, UserPermission.read_write)): diff --git a/tests/test_artifact.py b/tests/test_artifact.py index 1607d7523..fc067d465 100644 --- a/tests/test_artifact.py +++ b/tests/test_artifact.py @@ -1646,7 +1646,125 @@ async def test_workspace_based_permissions( # Clean up await artifact_manager1.delete(collection["id"]) - + + await api1.disconnect() + await api2.disconnect() + + +async def test_collection_permission_fallback_for_children( + minio_server, fastapi_server, test_user_token, test_user_token_2 +): + """A user holding a collection-level read_write permission (e.g. a model-zoo + reviewer with ``rw+``) must be authorized for read/write operations on the + collection's CHILD artifacts — including staged/versionless children whose own + config only lists the uploader — while delete stays owner/admin-only. + + Regression test for the collection-level permission fallback: previously the + parent-collection fallback in ``_get_artifact_with_permission`` only covered + read operations, so a collection-``rw+`` reviewer got a PermissionError on + ``edit``/``commit``/``put_file`` of a child model. + """ + # Owner / uploader + api1 = await connect_to_server( + {"name": "owner-client", "server_url": SERVER_URL, "token": test_user_token} + ) + artifact_manager1 = await api1.get_service("public/artifact-manager") + user1_info = api1.config["user"] + + # Reviewer (a different user) — will hold rw+ on the PARENT collection only + api2 = await connect_to_server( + {"name": "reviewer-client", "server_url": SERVER_URL, "token": test_user_token_2} + ) + artifact_manager2 = await api2.get_service("public/artifact-manager") + user2_info = api2.config["user"] + + # Collection: owner has full access, reviewer holds collection-level rw+. + collection = await artifact_manager1.create( + type="collection", + alias="reviewer-collection", + manifest={"name": "Reviewer Collection"}, + config={ + "permissions": { + user1_info["id"]: "*", + user2_info["id"]: "rw+", # collection-level reviewer + } + }, + ) + + # Committed child model — its OWN config lists only the uploader (user1). + child = await artifact_manager1.create( + parent_id=collection["id"], + type="model", + alias="committed-model", + manifest={"name": "Committed Model"}, + version="v0", + ) + # Sanity: the reviewer is NOT listed on the child's own permissions. + assert user2_info["id"] not in (child.get("config", {}) or {}).get( + "permissions", {} + ) + + # Reviewer can EDIT the committed child via the collection-level rw+ fallback. + await artifact_manager2.edit( + artifact_id=child["id"], + manifest={"name": "Reviewed committed model"}, + ) + read_child = await artifact_manager2.read(artifact_id=child["id"]) + assert read_child["manifest"]["name"] == "Reviewed committed model" + + # Staged / versionless child: reviewer must be able to edit + put_file + commit. + staged_child = await artifact_manager1.create( + parent_id=collection["id"], + type="model", + alias="staged-model", + manifest={"name": "Staged Model"}, + stage=True, + ) + # Reviewer edits the staged child (still uncommitted). + await artifact_manager2.edit( + artifact_id=staged_child["id"], + manifest={"name": "Reviewed staged model"}, + stage=True, + ) + # Reviewer uploads a file to the staged child. + put_url = await artifact_manager2.put_file( + artifact_id=staged_child["id"], file_path="weights.txt" + ) + async with httpx.AsyncClient(timeout=30) as client: + resp = await client.put(put_url, data=b"model-weights") + assert resp.status_code == 200 + # Reviewer commits the staged child. + await artifact_manager2.commit(artifact_id=staged_child["id"]) + committed = await artifact_manager2.read(artifact_id=staged_child["id"]) + assert committed["manifest"]["name"] == "Reviewed staged model" + + # Delete must NOT be granted through the collection-level fallback: rw+ on the + # parent does not include delete, and delete stays owner/admin-only. + with pytest.raises(Exception) as exc_info: + await artifact_manager2.delete(artifact_id=child["id"]) + assert "permission" in str(exc_info.value).lower() + + # A user with only READ (not rw+) on the parent collection must NOT be able to + # edit children — the fallback requires the WRITE permission on the parent. + await artifact_manager1.edit( + collection["id"], + config={ + "permissions": { + user1_info["id"]: "*", + user2_info["id"]: "r", # downgrade reviewer to read-only + } + }, + ) + with pytest.raises(Exception) as exc_info: + await artifact_manager2.edit( + artifact_id=child["id"], + manifest={"name": "Should be denied"}, + ) + assert "permission" in str(exc_info.value).lower() + + # Owner cleanup (owner still has full access via child "*" + parent). + await artifact_manager1.delete(collection["id"], recursive=True) + await api1.disconnect() await api2.disconnect()