mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-18 11:56:01 +02:00
fix(auth): API keys with Manage Library can curate library files (#1832)
require_ownership_permission gates API keys on `all_perm` only — the
comment at auth.py:1659 says OWN and ALL "both map to the same scope
flag" for queue / archives / etc., so checking `all_perm` is the
correct gate. Library deliberately broke that: LIBRARY_UPDATE_OWN /
LIBRARY_DELETE_OWN mapped to can_manage_library, but the ALL variants
were in _APIKEY_DENIED_PERMISSIONS. Result — every API-key request to
DELETE /library/files/{id}, PUT /library/files/{id} (rename), or
POST /library/files/move hit "administrative operations" 403, even
for keys with can_manage_library=True. Only slice worked, because it
doesn't go through require_ownership_permission.
The "ALL stays admin-only because it crosses the user boundary"
intent was internally inconsistent. API keys have no per-row
ownership identity (user=None), so the route's
`file.created_by_id != user.id` ownership check would AttributeError
on a key acting under OWN anyway — the only working path is
can_modify_all=True, which `all_perm` denial blocked outright.
Fix folds LIBRARY_UPDATE_ALL and LIBRARY_DELETE_ALL into
_APIKEY_SCOPE_BY_PERMISSION under can_manage_library, matching the
can_queue precedent (QUEUE_UPDATE_OWN and QUEUE_UPDATE_ALL both
map to can_queue for the same per-key-identity reason). Both removed
from _APIKEY_DENIED_PERMISSIONS. LIBRARY_PURGE stays denied — it
bypasses the soft-delete window and is genuinely destructive.
This commit is contained in:
File diff suppressed because one or more lines are too long
@@ -112,13 +112,21 @@ _APIKEY_SCOPE_BY_PERMISSION: dict[Permission, str] = {
|
||||
Permission.PRINTERS_AMS_RFID: "can_control_printer",
|
||||
Permission.PRINTERS_CLEAR_PLATE: "can_control_printer",
|
||||
Permission.SMART_PLUGS_CONTROL: "can_control_printer",
|
||||
# can_manage_library — file-manager scope (upload/rename/delete OWN library
|
||||
# can_manage_library — file-manager scope (upload/rename/delete library
|
||||
# entries + MakerWorld import which downloads files into the library).
|
||||
# Bulk/ALL-ownership library ops (UPDATE_ALL / DELETE_ALL / PURGE) stay
|
||||
# admin-only because they cross the user boundary.
|
||||
# OWN and ALL ownership variants map to the same scope so the
|
||||
# `require_ownership_permission` checker (which gates on `all_perm`)
|
||||
# passes the API key through. This matches `can_queue` and the
|
||||
# archives/inventory scopes — API keys have no per-row ownership identity
|
||||
# (line 1663), so splitting OWN/ALL across allowlist/denylist made the
|
||||
# whole library curation surface unreachable for API keys (#1832).
|
||||
# LIBRARY_PURGE stays admin-only as a genuinely destructive op that
|
||||
# bypasses the soft-delete window.
|
||||
Permission.LIBRARY_UPLOAD: "can_manage_library",
|
||||
Permission.LIBRARY_UPDATE_OWN: "can_manage_library",
|
||||
Permission.LIBRARY_UPDATE_ALL: "can_manage_library",
|
||||
Permission.LIBRARY_DELETE_OWN: "can_manage_library",
|
||||
Permission.LIBRARY_DELETE_ALL: "can_manage_library",
|
||||
Permission.MAKERWORLD_IMPORT: "can_manage_library",
|
||||
# can_manage_inventory — inventory write scope. Covers the documented
|
||||
# spool/catalog/forecast write surface AND the SpoolBuddy kiosk endpoints
|
||||
@@ -183,8 +191,11 @@ _APIKEY_DENIED_PERMISSIONS: frozenset[Permission] = frozenset(
|
||||
Permission.ARCHIVES_DELETE_OWN,
|
||||
Permission.ARCHIVES_DELETE_ALL,
|
||||
Permission.ARCHIVES_PURGE,
|
||||
Permission.LIBRARY_UPDATE_ALL,
|
||||
Permission.LIBRARY_DELETE_ALL,
|
||||
# LIBRARY_UPDATE_ALL / LIBRARY_DELETE_ALL moved to the allowlist
|
||||
# under `can_manage_library` (#1832) — split between allow/deny made
|
||||
# the whole library curation surface unreachable for API keys via
|
||||
# `require_ownership_permission`. Purge stays denied as a genuinely
|
||||
# destructive op.
|
||||
Permission.LIBRARY_PURGE,
|
||||
Permission.PROJECTS_CREATE,
|
||||
Permission.PROJECTS_UPDATE,
|
||||
|
||||
@@ -300,9 +300,15 @@ class TestCheckApiKeyPermissionsMatrix:
|
||||
("PRINTERS_CONTROL", "can_control_printer", "start/stop print"),
|
||||
("PRINTERS_FILES", "can_control_printer", "send file to printer"),
|
||||
("SMART_PLUGS_CONTROL", "can_control_printer", "smart plug on/off"),
|
||||
# can_manage_library
|
||||
# can_manage_library — OWN and ALL ownership variants both fold into
|
||||
# the same scope (#1832): API keys have no per-row ownership identity,
|
||||
# so splitting OWN/ALL across allowlist/denylist made the curation
|
||||
# surface unreachable. PURGE stays admin-only.
|
||||
("LIBRARY_UPLOAD", "can_manage_library", "upload library file"),
|
||||
("LIBRARY_UPDATE_OWN", "can_manage_library", "rename own library file"),
|
||||
("LIBRARY_UPDATE_ALL", "can_manage_library", "rename any library file"),
|
||||
("LIBRARY_DELETE_OWN", "can_manage_library", "delete own library file"),
|
||||
("LIBRARY_DELETE_ALL", "can_manage_library", "delete any library file"),
|
||||
("MAKERWORLD_IMPORT", "can_manage_library", "import from MakerWorld"),
|
||||
# can_manage_inventory
|
||||
("INVENTORY_CREATE", "can_manage_inventory", "create spool record"),
|
||||
@@ -321,7 +327,8 @@ class TestCheckApiKeyPermissionsMatrix:
|
||||
"FIRMWARE_UPDATE",
|
||||
# Unmapped administrative (allowlist fail-closed catches these too)
|
||||
"PRINTERS_CREATE",
|
||||
"LIBRARY_DELETE_ALL",
|
||||
# LIBRARY_DELETE_ALL / LIBRARY_UPDATE_ALL moved to can_manage_library
|
||||
# under #1832 — covered by the _SCOPE_CASES matrix above.
|
||||
"LIBRARY_PURGE",
|
||||
"DISCOVERY_SCAN",
|
||||
]
|
||||
|
||||
@@ -1349,6 +1349,140 @@ class TestLibraryPermissions:
|
||||
# Viewers don't have delete_own or delete_all permissions
|
||||
assert response.status_code == 403
|
||||
|
||||
# ---------- #1832: API-key curation under can_manage_library ----------
|
||||
#
|
||||
# require_ownership_permission gates API keys on `all_perm`, but the
|
||||
# library deliberately split UPDATE_OWN/DELETE_OWN (allowed under
|
||||
# can_manage_library) from UPDATE_ALL/DELETE_ALL (previously denied).
|
||||
# That made the entire curation surface (DELETE, PUT rename, POST move)
|
||||
# unreachable for API keys, including for files the key's owner uploaded.
|
||||
# The fix folds UPDATE_ALL/DELETE_ALL into can_manage_library so the
|
||||
# checker passes; LIBRARY_PURGE stays admin-only.
|
||||
|
||||
@pytest.fixture
|
||||
async def manage_library_key(self, db_session, auth_setup):
|
||||
"""Mint an API key owned by the admin user with can_manage_library."""
|
||||
from backend.app.core.auth import generate_api_key
|
||||
from backend.app.models.api_key import APIKey
|
||||
|
||||
admin = auth_setup["admin_user"]
|
||||
full_key, key_hash, key_prefix = generate_api_key()
|
||||
row = APIKey(
|
||||
name="lib-curation",
|
||||
key_hash=key_hash,
|
||||
key_prefix=key_prefix,
|
||||
user_id=admin.id,
|
||||
can_manage_library=True,
|
||||
)
|
||||
db_session.add(row)
|
||||
await db_session.commit()
|
||||
return full_key
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_apikey_with_manage_library_can_delete_file(
|
||||
self, async_client: AsyncClient, db_session, auth_setup, test_file, manage_library_key
|
||||
):
|
||||
"""Pre-#1832 this 403'd with "administrative operations" because
|
||||
LIBRARY_DELETE_ALL wasn't in _APIKEY_SCOPE_BY_PERMISSION."""
|
||||
from pathlib import Path
|
||||
|
||||
from backend.app.core.config import settings as app_settings
|
||||
|
||||
# Materialise the file on disk so the delete handler doesn't 500 on
|
||||
# the path it tries to unlink.
|
||||
file_path = Path(app_settings.base_dir) / test_file.file_path
|
||||
file_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
file_path.write_text("test content")
|
||||
|
||||
response = await async_client.delete(
|
||||
f"/api/v1/library/files/{test_file.id}",
|
||||
headers={"X-API-Key": manage_library_key},
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_apikey_with_manage_library_can_rename_file(
|
||||
self, async_client: AsyncClient, db_session, auth_setup, test_file, manage_library_key
|
||||
):
|
||||
"""PUT /library/files/{id} is gated on LIBRARY_UPDATE_ALL/OWN. Same
|
||||
#1832 path as delete."""
|
||||
response = await async_client.put(
|
||||
f"/api/v1/library/files/{test_file.id}",
|
||||
headers={"X-API-Key": manage_library_key},
|
||||
json={"filename": "renamed.txt"},
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
assert response.json()["filename"] == "renamed.txt"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_apikey_with_manage_library_can_move_file(
|
||||
self, async_client: AsyncClient, db_session, auth_setup, test_file, manage_library_key
|
||||
):
|
||||
"""POST /library/files/move (bulk) is gated on LIBRARY_UPDATE_ALL/OWN
|
||||
— same checker, same #1832 path."""
|
||||
# Create a target folder the move can land in.
|
||||
from backend.app.models.library import LibraryFolder
|
||||
|
||||
folder = LibraryFolder(name="target")
|
||||
db_session.add(folder)
|
||||
await db_session.commit()
|
||||
await db_session.refresh(folder)
|
||||
|
||||
response = await async_client.post(
|
||||
"/api/v1/library/files/move",
|
||||
headers={"X-API-Key": manage_library_key},
|
||||
json={"file_ids": [test_file.id], "folder_id": folder.id},
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_apikey_without_manage_library_still_blocked(
|
||||
self, async_client: AsyncClient, db_session, auth_setup, test_file
|
||||
):
|
||||
"""Regression guard: a key WITHOUT can_manage_library must still get
|
||||
403 — the fix widens the allowed-permission set, it doesn't bypass
|
||||
the per-key scope check."""
|
||||
from backend.app.core.auth import generate_api_key
|
||||
from backend.app.models.api_key import APIKey
|
||||
|
||||
admin = auth_setup["admin_user"]
|
||||
full_key, key_hash, key_prefix = generate_api_key()
|
||||
row = APIKey(
|
||||
name="read-only",
|
||||
key_hash=key_hash,
|
||||
key_prefix=key_prefix,
|
||||
user_id=admin.id,
|
||||
can_read_status=True,
|
||||
can_manage_library=False,
|
||||
)
|
||||
db_session.add(row)
|
||||
await db_session.commit()
|
||||
|
||||
response = await async_client.delete(
|
||||
f"/api/v1/library/files/{test_file.id}",
|
||||
headers={"X-API-Key": full_key},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_apikey_with_manage_library_still_cannot_purge(
|
||||
self, async_client: AsyncClient, db_session, auth_setup, manage_library_key
|
||||
):
|
||||
"""LIBRARY_PURGE deliberately stays in _APIKEY_DENIED_PERMISSIONS as
|
||||
a genuinely destructive op that bypasses the soft-delete window.
|
||||
can_manage_library does NOT grant it."""
|
||||
response = await async_client.post(
|
||||
"/api/v1/library/purge",
|
||||
headers={"X-API-Key": manage_library_key},
|
||||
json={"days_in_trash": 30},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
|
||||
class TestPrintFileUploadValidation:
|
||||
"""#1401: pre-flight rejection of unprintable uploads at the library +
|
||||
|
||||
Reference in New Issue
Block a user