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 && (