From 6826dcaba4a36918e6029024eaba6b51f3722a13 Mon Sep 17 00:00:00 2001 From: Atlas Date: Wed, 25 Mar 2026 13:35:42 +0800 Subject: [PATCH] Let tenant admins manage tenant skill folders without hiding delete actions Issue #180 mixed two failures: enterprise skill management was still platform-admin only, and the shared FileBrowser hid delete actions for directories, which made agent skill folders look undeletable even when the backend delete path worked. This change scopes skill CRUD and browse writes/deletes to the current tenant for org admins, keeps builtin and cross-tenant skills read-only for them, and avoids iterating lazy files on freshly created skills. The FileBrowser now shows delete actions for directories so skill folders expose the same operation the backend already supports. Constraint: org_admin access must stay tenant-bounded and must not allow editing builtin platform skills Rejected: Grant org_admin full platform_admin-equivalent skill access | would let tenant admins modify builtin or other-tenant skills Rejected: Fix only backend permissions | leaves skill folders with no visible delete affordance in the agent UI Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep skill write/delete rules aligned between direct CRUD and browse endpoints; changing one without the other will reintroduce inconsistent UI behavior Tested: backend/.venv/bin/python -m pytest backend/tests/test_skills_api.py -q; backend/.venv/bin/python -m ruff check backend/app/api/skills.py backend/tests/test_skills_api.py; cd frontend && npm run build; manual API verification for orgadmin browse-write/browse-delete/direct-delete Not-tested: full end-to-end browser deletion flow after page reload under every role combination --- backend/app/api/skills.py | 85 ++++++---- backend/tests/test_skills_api.py | 208 ++++++++++++++++++++++++ frontend/src/components/FileBrowser.tsx | 2 +- 3 files changed, 259 insertions(+), 36 deletions(-) create mode 100644 backend/tests/test_skills_api.py diff --git a/backend/app/api/skills.py b/backend/app/api/skills.py index 12c8b613..61c4d9bd 100644 --- a/backend/app/api/skills.py +++ b/backend/app/api/skills.py @@ -13,7 +13,7 @@ from app.database import async_session from app.models.skill import Skill, SkillFile -from app.core.security import require_role, get_current_user +from app.core.security import get_current_admin, get_current_user, require_role from app.models.user import User logger = logging.getLogger(__name__) @@ -138,6 +138,23 @@ def _parse_github_url(url: str) -> dict | None: return None +def _apply_skill_scope(query, current_user: User): + """Scope skill queries for tenant admins while leaving platform admins unrestricted.""" + from sqlalchemy import or_ as _or + + if current_user.role == "platform_admin" or not current_user.tenant_id: + return query + return query.where(_or(Skill.tenant_id.is_(None), Skill.tenant_id == current_user.tenant_id)) + + +def _ensure_skill_write_access(skill: Skill, current_user: User): + """Allow platform admins to edit everything; tenant admins can edit only tenant-owned custom skills.""" + if current_user.role == "platform_admin": + return + if not current_user.tenant_id or skill.tenant_id != current_user.tenant_id: + raise HTTPException(403, "Cannot modify builtin or other-tenant skills") + + async def _fetch_github_directory( owner: str, repo: str, path: str, branch: str = "main", token: str = "", @@ -227,7 +244,7 @@ async def _save_skill_to_db( if tenant_id: conflict_q = conflict_q.where(Skill.tenant_id == _uuid.UUID(tenant_id)) else: - conflict_q = conflict_q.where(Skill.tenant_id == None) + conflict_q = conflict_q.where(Skill.tenant_id.is_(None)) existing = await db.execute(conflict_q) if existing.scalar_one_or_none(): raise HTTPException( @@ -492,7 +509,7 @@ async def list_skills(current_user: User = Depends(get_current_user)): query = select(Skill).order_by(Skill.name) # Scope by tenant: show builtin (tenant_id is NULL) + tenant-specific skills if tenant_id: - query = query.where(_or(Skill.tenant_id == None, Skill.tenant_id == _uuid.UUID(tenant_id))) + query = query.where(_or(Skill.tenant_id.is_(None), Skill.tenant_id == _uuid.UUID(tenant_id))) result = await db.execute(query) skills = result.scalars().all() return [ @@ -512,12 +529,11 @@ async def list_skills(current_user: User = Depends(get_current_user)): @router.get("/{skill_id}") -async def get_skill(skill_id: str): +async def get_skill(skill_id: str, current_user: User = Depends(get_current_user)): """Get a skill with its files.""" async with async_session() as db: - result = await db.execute( - select(Skill).where(Skill.id == skill_id).options(selectinload(Skill.files)) - ) + query = select(Skill).where(Skill.id == skill_id).options(selectinload(Skill.files)) + result = await db.execute(_apply_skill_scope(query, current_user)) skill = result.scalar_one_or_none() if not skill: raise HTTPException(404, "Skill not found") @@ -537,7 +553,7 @@ async def get_skill(skill_id: str): @router.post("/") -async def create_skill(body: SkillCreateIn, _=Depends(require_role("platform_admin"))): +async def create_skill(body: SkillCreateIn, current_user: User = Depends(get_current_admin)): """Create a custom skill.""" async with async_session() as db: skill = Skill( @@ -547,6 +563,7 @@ async def create_skill(body: SkillCreateIn, _=Depends(require_role("platform_adm icon=body.icon, folder_name=body.folder_name, is_builtin=False, + tenant_id=current_user.tenant_id, ) db.add(skill) await db.flush() @@ -575,15 +592,15 @@ class SkillUpdateIn(BaseModel): @router.put("/{skill_id}") -async def update_skill(skill_id: str, body: SkillUpdateIn, _=Depends(require_role("platform_admin"))): +async def update_skill(skill_id: str, body: SkillUpdateIn, current_user: User = Depends(get_current_admin)): """Update a skill's metadata and/or files.""" async with async_session() as db: - result = await db.execute( - select(Skill).where(Skill.id == skill_id).options(selectinload(Skill.files)) - ) + query = select(Skill).where(Skill.id == skill_id).options(selectinload(Skill.files)) + result = await db.execute(_apply_skill_scope(query, current_user)) skill = result.scalar_one_or_none() if not skill: raise HTTPException(404, "Skill not found") + _ensure_skill_write_access(skill, current_user) if body.name is not None: skill.name = body.name @@ -607,15 +624,17 @@ async def update_skill(skill_id: str, body: SkillUpdateIn, _=Depends(require_rol @router.delete("/{skill_id}") -async def delete_skill(skill_id: str, _=Depends(require_role("platform_admin"))): +async def delete_skill(skill_id: str, current_user: User = Depends(get_current_admin)): """Delete a skill (not builtin).""" async with async_session() as db: - result = await db.execute(select(Skill).where(Skill.id == skill_id)) + query = select(Skill).where(Skill.id == skill_id) + result = await db.execute(_apply_skill_scope(query, current_user)) skill = result.scalar_one_or_none() if not skill: raise HTTPException(404, "Skill not found") if skill.is_builtin: raise HTTPException(400, "Cannot delete builtin skill") + _ensure_skill_write_access(skill, current_user) await db.delete(skill) await db.commit() return {"ok": True} @@ -704,7 +723,7 @@ async def browse_list(path: str = "", current_user: User = Depends(get_current_u # Root: list all skill folders (scoped by tenant) query = select(Skill).order_by(Skill.name) if tenant_id: - query = query.where(_or(Skill.tenant_id == None, Skill.tenant_id == _uuid.UUID(tenant_id))) + query = query.where(_or(Skill.tenant_id.is_(None), Skill.tenant_id == _uuid.UUID(tenant_id))) result = await db.execute(query) skills = result.scalars().all() return [ @@ -718,7 +737,7 @@ async def browse_list(path: str = "", current_user: User = Depends(get_current_u # Resolve skill folder scoped by tenant skill_q = select(Skill).where(Skill.folder_name == folder).options(selectinload(Skill.files)) if tenant_id: - skill_q = skill_q.where(_or(Skill.tenant_id == None, Skill.tenant_id == _uuid.UUID(tenant_id))) + skill_q = skill_q.where(_or(Skill.tenant_id.is_(None), Skill.tenant_id == _uuid.UUID(tenant_id))) result = await db.execute(skill_q) skill = result.scalar_one_or_none() if not skill: @@ -766,7 +785,7 @@ async def browse_read(path: str, current_user: User = Depends(get_current_user)) async with async_session() as db: skill_q = select(Skill).where(Skill.folder_name == folder).options(selectinload(Skill.files)) if tenant_id: - skill_q = skill_q.where(_or(Skill.tenant_id == None, Skill.tenant_id == _uuid.UUID(tenant_id))) + skill_q = skill_q.where(_or(Skill.tenant_id.is_(None), Skill.tenant_id == _uuid.UUID(tenant_id))) result = await db.execute(skill_q) skill = result.scalar_one_or_none() if not skill: @@ -783,21 +802,17 @@ class BrowseWriteIn(BaseModel): @router.put("/browse/write") -async def browse_write(body: BrowseWriteIn, current_user: User = Depends(require_role("platform_admin"))): +async def browse_write(body: BrowseWriteIn, current_user: User = Depends(get_current_admin)): """Write a file in a skill folder. Creates the skill if the folder doesn't exist.""" - import uuid as _uuid - from sqlalchemy import or_ as _or - tenant_id = str(current_user.tenant_id) if current_user.tenant_id else None parts = body.path.strip("/").split("/", 1) if len(parts) < 2: raise HTTPException(400, "Path must include folder and file") folder, file_path = parts async with async_session() as db: skill_q = select(Skill).where(Skill.folder_name == folder).options(selectinload(Skill.files)) - if tenant_id: - skill_q = skill_q.where(_or(Skill.tenant_id == None, Skill.tenant_id == _uuid.UUID(tenant_id))) - result = await db.execute(skill_q) + result = await db.execute(_apply_skill_scope(skill_q, current_user)) skill = result.scalar_one_or_none() + created_new_skill = False if not skill: # Auto-create skill from folder name, scoped to tenant skill = Skill( @@ -811,13 +826,17 @@ async def browse_write(body: BrowseWriteIn, current_user: User = Depends(require ) db.add(skill) await db.flush() + created_new_skill = True + else: + _ensure_skill_write_access(skill, current_user) # Upsert file existing = None - for f in skill.files: - if f.path == file_path: - existing = f - break + if not created_new_skill: + for f in skill.files: + if f.path == file_path: + existing = f + break if existing: existing.content = body.content else: @@ -827,23 +846,19 @@ async def browse_write(body: BrowseWriteIn, current_user: User = Depends(require @router.delete("/browse/delete") -async def browse_delete(path: str, current_user: User = Depends(require_role("platform_admin"))): +async def browse_delete(path: str, current_user: User = Depends(get_current_admin)): """Delete a file or an entire skill folder.""" - import uuid as _uuid - from sqlalchemy import or_ as _or - tenant_id = str(current_user.tenant_id) if current_user.tenant_id else None parts = path.strip("/").split("/", 1) folder = parts[0] async with async_session() as db: skill_q = select(Skill).where(Skill.folder_name == folder).options(selectinload(Skill.files)) - if tenant_id: - skill_q = skill_q.where(_or(Skill.tenant_id == None, Skill.tenant_id == _uuid.UUID(tenant_id))) - result = await db.execute(skill_q) + result = await db.execute(_apply_skill_scope(skill_q, current_user)) skill = result.scalar_one_or_none() if not skill: raise HTTPException(404, "Skill not found") if skill.is_builtin and len(parts) == 1: raise HTTPException(400, "Cannot delete builtin skill") + _ensure_skill_write_access(skill, current_user) if len(parts) == 1: # Delete entire skill diff --git a/backend/tests/test_skills_api.py b/backend/tests/test_skills_api.py new file mode 100644 index 00000000..2ca5f8dc --- /dev/null +++ b/backend/tests/test_skills_api.py @@ -0,0 +1,208 @@ +import uuid +from types import SimpleNamespace + +import httpx +import pytest + +from app.api import skills as skills_api +from app.core.security import get_current_user +from app.main import app + + +class FakeScalarResult: + def __init__(self, value): + self._value = value + + def scalar_one_or_none(self): + return self._value + + +class TrapList(list): + def __iter__(self): + raise AssertionError("newly created skills should not iterate over lazy files") + + +class FakeSession: + def __init__(self, *, skill=None): + self.skill = skill + self.added = [] + self.deleted = [] + self.committed = False + + async def execute(self, _query): + return FakeScalarResult(self.skill) + + def add(self, value): + self.added.append(value) + + async def flush(self): + return None + + async def delete(self, value): + self.deleted.append(value) + + async def commit(self): + self.committed = True + + +class FakeAsyncSessionFactory: + def __init__(self, session): + self.session = session + + def __call__(self): + return self + + async def __aenter__(self): + return self.session + + async def __aexit__(self, exc_type, exc, tb): + return False + + +class FakeQuery: + def where(self, *_args, **_kwargs): + return self + + def options(self, *_args, **_kwargs): + return self + + def order_by(self, *_args, **_kwargs): + return self + + +class RaiseOnInstanceAccess: + def __get__(self, instance, owner): + if instance is None: + return self + raise AssertionError("newly created skills should not iterate over lazy files") + + +class QueryField: + def is_(self, _value): + return self + + def __eq__(self, _other): + return self + + +class FakeSkill: + folder_name = QueryField() + tenant_id = QueryField() + files = RaiseOnInstanceAccess() + + def __init__(self, **kwargs): + self.id = uuid.uuid4() + for key, value in kwargs.items(): + setattr(self, key, value) + + +@pytest.fixture +def org_admin_user(): + return SimpleNamespace( + id=uuid.uuid4(), + role="org_admin", + tenant_id=uuid.uuid4(), + is_active=True, + department_id=None, + ) + + +@pytest.fixture +def platform_admin_user(): + return SimpleNamespace( + id=uuid.uuid4(), + role="platform_admin", + tenant_id=uuid.uuid4(), + is_active=True, + department_id=None, + ) + + +@pytest.fixture +def client(): + transport = httpx.ASGITransport(app=app) + + async def _build(): + return httpx.AsyncClient(transport=transport, base_url="http://test") + + return _build + + +@pytest.mark.asyncio +async def test_org_admin_can_delete_custom_skill_via_browse(monkeypatch, client, org_admin_user): + skill = SimpleNamespace( + id=uuid.uuid4(), + folder_name="tenant-skill", + tenant_id=org_admin_user.tenant_id, + is_builtin=False, + files=[], + ) + session = FakeSession(skill=skill) + + monkeypatch.setattr(skills_api, "async_session", FakeAsyncSessionFactory(session)) + app.dependency_overrides[get_current_user] = lambda: org_admin_user + + async with await client() as ac: + response = await ac.delete("/api/skills/browse/delete", params={"path": "tenant-skill"}) + + app.dependency_overrides.clear() + + assert response.status_code == 200 + assert response.json() == {"ok": True} + assert session.deleted == [skill] + assert session.committed is True + + +@pytest.mark.asyncio +async def test_org_admin_can_delete_custom_skill_directly(monkeypatch, client, org_admin_user): + skill = SimpleNamespace( + id=uuid.uuid4(), + folder_name="tenant-skill", + tenant_id=org_admin_user.tenant_id, + is_builtin=False, + ) + session = FakeSession(skill=skill) + + monkeypatch.setattr(skills_api, "async_session", FakeAsyncSessionFactory(session)) + app.dependency_overrides[get_current_user] = lambda: org_admin_user + + async with await client() as ac: + response = await ac.delete(f"/api/skills/{skill.id}") + + app.dependency_overrides.clear() + + assert response.status_code == 200 + assert response.json() == {"ok": True} + assert session.deleted == [skill] + assert session.committed is True + + +@pytest.mark.asyncio +async def test_browse_write_creates_tenant_skill_without_iterating_lazy_files( + monkeypatch, client, platform_admin_user +): + session = FakeSession(skill=None) + + monkeypatch.setattr(skills_api, "async_session", FakeAsyncSessionFactory(session)) + monkeypatch.setattr(skills_api, "select", lambda *_args, **_kwargs: FakeQuery()) + monkeypatch.setattr(skills_api, "selectinload", lambda *_args, **_kwargs: None) + monkeypatch.setattr(skills_api, "Skill", FakeSkill) + app.dependency_overrides[get_current_user] = lambda: platform_admin_user + + async with await client() as ac: + response = await ac.put( + "/api/skills/browse/write", + json={"path": "tenant-skill/SKILL.md", "content": "# test"}, + ) + + app.dependency_overrides.clear() + + assert response.status_code == 200 + assert response.json() == {"ok": True} + created_skill = next(value for value in session.added if isinstance(value, FakeSkill)) + created_file = next(value for value in session.added if isinstance(value, skills_api.SkillFile)) + assert created_skill.folder_name == "tenant-skill" + assert created_skill.tenant_id == platform_admin_user.tenant_id + assert created_file.path == "SKILL.md" + assert created_file.content == "# test" + assert session.committed is True diff --git a/frontend/src/components/FileBrowser.tsx b/frontend/src/components/FileBrowser.tsx index 92169128..72c0566c 100644 --- a/frontend/src/components/FileBrowser.tsx +++ b/frontend/src/components/FileBrowser.tsx @@ -539,7 +539,7 @@ export default function FileBrowser({ ⬇ )} - {canDelete && !f.is_dir && ( + {canDelete && (