mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-10 08:13:46 +00:00
🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema (#12613)
* 🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema Adds Skills as a new first-class resource modeled on Anthropic's Agent Skills, reusing the existing Prompt ACL stack for sharing. Lays the groundwork for multi-file skills (SkillFile schema + metadata routes) without wiring upload processing — single-file skills (inline SKILL.md body) work end-to-end, multi-file uploads are stubbed for phase 2. * 🔬 fix: Wire Skill Cleanup, AccessRole Enum, and Express 5 Path Params CI surfaced four follow-ups from the initial Skills scaffolding commit that local builds missed: - AccessRole's resourceType field had a hardcoded enum that didn't include `'skill'`, blocking SKILL_OWNER/EDITOR/VIEWER role creation in every test that hit the AccessRole model. - The seedDefaultRoles assertion in accessRole.spec.ts hard-listed the expected role IDs and needed the new SKILL_* entries. - deleteUserController had no cleanup for skills, and the deleteUserResourceCoverage guard test enforces every ResourceType has a documented handler — wired in db.deleteUserSkills(user._id) and added the entry to HANDLED_RESOURCE_TYPES. - Express 5's path-to-regexp v6 rejects the legacy `(*)` named-group glob syntax. The two skill file routes now use a plain `:relativePath` param; the client already encodeURIComponents the path, so a single param is sufficient and decoded server-side. * 🪡 fix: Make Skill Name Uniqueness Application-Level Resolve three more CI failures from the Skills scaffolding PR: - Mongoose creates indexes asynchronously and mongodb-memory-server tests can race ahead of the unique (name, author, tenantId) index being built, so the duplicate-name uniqueness test was flaky. Added an explicit findOne pre-check inside createSkill that throws with code 11000 (mimicking the index violation), giving deterministic behavior. The unique index stays as the persistent guarantee. - The deleteUser.spec.js and UserController.spec.js suites mock the ~/models module directly and were missing deleteUserSkills, causing deleteUserController to throw and return 500 instead of 200. - Removed two doc-comment claims that the SKILL_NAME_MAX_LENGTH and SKILL_DESCRIPTION_MAX_LENGTH constants "match Anthropic's API". The values themselves are reasonable but the comments were misleading about who enforces them. * 🪢 fix: Address Code Review Findings on Skills Scaffolding Resolve all 15 findings from the comprehensive PR review: Critical: - Rollback the created skill when grantPermission throws so a transient ACL failure cannot leave an orphaned, inaccessible skill in the DB. - Fix infinite query cache corruption in useUpdateSkillMutation helpers. setQueriesData([QueryKeys.skills]) matches useSkillsInfiniteQuery's InfiniteData cache entries, which have { pages, pageParams } shape — spreading data.skills on those would throw. Added an isInfiniteSkillData guard and per-page transform so both flat and infinite caches update correctly. Major: - Fix TUpdateSkillContext type: the public type declared previousListData but onMutate actually returns previousListSnapshots (a [key, value] tuple array). Updated the type + added TSkillCacheEntry as a shared export from data-provider. - Add cancelQueries calls before optimistic update in onMutate so in-flight refetches cannot clobber the optimistic state. - Parallelize deleteUserSkills ACL removal via Promise.allSettled instead of a sequential await loop — O(1) round-trip vs O(n). - Stub mockDeleteUserSkills in stubDeletionMocks() and assert it's called with user.id in the deleteUser.spec.js happy-path test. - Add idResolver: getSkillById to the SKILL branch in accessPermissions.js so GET /api/permissions/skill/<missing-id> returns 404 instead of 403. Minor: - Reuse resolved skill from req.resourceAccess.resourceInfo in getHandler to eliminate a redundant getSkillById call per GET /api/skills/:id. - Reject PATCH /api/skills/:id requests whose body contains only expectedVersion — previously they silently bumped version with no changes, triggering spurious 409s for collaborators. - Make TSkill.frontmatter optional (wire type) and add serializeFrontmatter / serializeSourceMetadata helpers that return undefined for empty objects instead of casting incomplete data to SkillFrontmatter. - Standardize deleteUserSkills to accept string | ObjectId and convert internally, matching deleteUserPrompts's signature; UserController now passes user.id consistently. - Replace bumpSkillVersionAndRecount (read-then-write, racy) with bumpSkillVersionAndAdjustFileCount using atomic $inc. upsertSkillFile pre-checks existence to distinguish insert (+1) from replace (0). - Add DELETE /api/skills/:id/files/:relativePath integration tests covering success, 404, and 403 paths. Nits: - Drop trivial resolveSkillId wrapper — pass getSkillById directly. - Remove dead staleTime: 1000 * 10 from useListSkillsQuery since all refetch triggers are already disabled. * 🧭 fix: Resolve Second Skills Review Pass — Cache, Gate, TOCTOU Address 13 of 14 findings from the second code review; reject #13 as misread of the AGENTS.md import-order rule (package types correctly precede local types regardless of length). Major: - Fix addSkillToCachedLists closure bug: a hoisted `prepended` flag was shared across every cache entry matched by setQueriesData, so concurrent flat + infinite caches would silently drop the prepend on whichever was processed second. Replaced the shared helper with three per-entry inline updaters that handle InfiniteData at the page level (page 0 only for prepend, all pages for replace/remove). - Tighten patchHandler's expectedVersion validation: NaN passes `typeof === 'number'` and would previously leak current skill state via a misleading 409. Now requires finite positive integer and returns 400 otherwise. - Guard decodeURIComponent in deleteFileHandler with try/catch — malformed percent encoding now returns 400 instead of 500. - Add PermissionTypes.SKILLS + skillPermissionsSchema + TSkillPermissions in data-provider; seed default SKILLS permissions for ADMIN (all true) and USER (use + create only); wire checkSkillAccess / checkSkillCreate via generateCheckAccess onto the skills router mirroring the prompts pattern. Skills route now enforces role-based capability gates alongside per-resource ACLs. Test suite adds a mocked getRoleByName returning permissive SKILLS. - Fix upsertSkillFile TOCTOU: replaced the pre-check + upsert pair with a single `findOneAndUpdate({ new: false, upsert: true })` call that atomically returns the pre-update doc (null ⇒ insert) so fileCount delta can't double-count on concurrent same-path uploads. Minor: - Add `sourceMetadata` to listSkillsByAccess .select() so summaries no longer silently drop the field for GitHub/Notion-synced skills. - Include `cursor` in useListSkillsQuery's query key so manual pagination doesn't alias across pages. - Clean up TSkillSummary to `Omit<TSkill, 'body' | 'frontmatter'>` matching what serializeSkillSummary actually emits; drop the Omit-then-re-add noise. - Skip getPublicSkillIdSet in createHandler; a newly-created skill cannot have a PUBLIC ACL entry, so pass an empty set directly instead of paying a DB round-trip. - Trim SkillMethods public surface: drop internal helpers countSkillFiles / deleteSkillFilesBySkillId / getSkillFile from the return object; inline the file cascade into deleteSkill. - Use TSkillConflictResponse at the PATCH 409 call site instead of an inline ad-hoc object literal. - Drop the now-unused EXPECTED_VERSION_ERROR module constant. * 🧩 fix: Extend Role Schema + Types with SKILLS PermissionType CI type-check and unit test failures from the PermissionTypes.SKILLS addition surfaced three unrelated places that all hardcode the permission-type set: - IRole.permissions in data-schemas/types/role.ts enumerates every PermissionTypes key as an optional field. Adding SKILLS to the enum without updating the interface caused TS7053 'expression of type PermissionTypes can't be used to index type' errors in role.methods.spec.ts (lines 407-408, 477-478) because Object.values(PermissionTypes) now yielded a value the interface didn't cover. - schema/role.ts rolePermissionsSchema mirrors the interface at the Mongoose layer; also needed SKILLS added so the persisted role document can actually store skill permissions. - data-provider/roles.spec.ts has a guard test that every permission type carrying CREATE/SHARE/SHARE_PUBLIC must be explicitly "tracked" either in RESOURCE_PERMISSION_TYPES or in the PROMPTS/AGENTS/MEMORIES exemption list. Added SKILLS to the exemption list since skills follow the same default model as prompts/agents (USE + CREATE on for USER, SHARE / SHARE_PUBLIC off). All three are additive pass-throughs with no behavior change. * 🏷️ refactor: Introduce ISkillSummary for Narrow List Projection Follow-up NITs from the second review pass on the Skills PR: - Define ISkillSummary = Omit<ISkill, 'body' | 'frontmatter'> and use it as the element type in ListSkillsByAccessResult. The list query's .select() intentionally omits body and frontmatter for payload size, but the previous type claimed both fields were present — a type lie that would mislead future readers even though serializeSkillSummary never touches those fields at runtime. handlers.ts's signature for serializeSkillSummary now accepts ISkillSummary too. - Document the intentional second-round-trip `findOne` in upsertSkillFile. Switching to `findOneAndUpdate({ new: false })` was required for TOCTOU-safe insert-vs-replace detection, which means the handler needs a follow-up query to return the post-upsert document. A comment now explains the tradeoff so future readers don't silently "optimize" it away. No behavior change. * 🌐 fix: Wire SKILL into SHARE_PUBLIC Resource Maps Address codex comment #1 — making a skill public was blocked on two hardcoded resource→permission-type maps that didn't know about SKILL: - api/server/middleware/checkSharePublicAccess.js's resourceToPermissionType map was missing ResourceType.SKILL, so PUT /api/permissions/skill/:id with { public: true } would fall through to the 400 "Unsupported resource type for public sharing" path even though PermissionTypes.SKILLS exists and ADMIN has SHARE_PUBLIC configured. Added the mapping. - client/src/hooks/Sharing/useCanSharePublic.ts has an identical client-side map used to gate the "Make Public" UI toggle. Without the SKILL mapping the hook returned false for everyone, so the toggle wouldn't render for skills once the sharing UI lands in phase 2. Added the mapping. Codex comment #2 (create/update cache writes inject skills into unrelated filtered lists) is invalid — it flags a pattern that mirrors useUpdatePromptGroup (which the PR description explicitly cites as the model) and is a deliberate optimistic-update tradeoff. Trying to match each cache key's embedded filter would couple the mutation callback to query-key internals, which is exactly what setQueriesData is designed to avoid. No change there. * 🧪 feat: Frontmatter Validation, Reserved-Name Fixes, Coaching Warnings Address the follow-up review notes on the Skills PR. This commit closes the gap between the wire-type promise and what the backend actually enforces, tightens the reserved-name rules, and adds a non-blocking coaching tier for validators. Frontmatter validation (new): - Add `validateSkillFrontmatter` in data-schemas/methods/skill.ts with strict mode — unknown keys are rejected so expanding the allowed set is an intentional code change. Known keys are type-checked against a `FrontmatterKind` table derived from Anthropic's Agent Skills spec (name, description, when-to-use, allowed-tools, arguments, argument-hint, user-invocable, disable-model-invocation, model, effort, context, agent, paths, shell, hooks, version, metadata). - `hooks` and `metadata` get a shallow JSON-safety check (max depth 4, max string 2000, max array 100) instead of a full schema, since their full shapes live outside this module. - Wired into BOTH createSkill AND updateSkill so the PATCH path can't smuggle invalid frontmatter past the validator. Validation warning tier (new): - Add optional `severity: 'error' | 'warning'` to `ValidationIssue` (defaults to error). `partitionIssues` splits an issue list into blocking errors and non-blocking warnings. - `createSkill` / `updateSkill` filter on errors for the throw check and return warnings in a new `warnings: ValidationIssue[]` field on their result objects (`CreateSkillResult` / `UpdateSkillResult`). - `validateSkillDescription` now emits a `TOO_SHORT` warning for descriptions under 20 chars — the primary triggering field, so a little coaching goes a long way. - `createHandler` / `patchHandler` in packages/api surface the warnings via a new `attachWarnings` helper that decorates the serialized response with a `warnings?: TSkillWarning[]` field. - `TSkill` gains an optional `warnings?: TSkillWarning[]` field documented as "present on POST/PATCH, never on GET". Reserved-name filter (tightened): - Replace the substring match (`.includes('anthropic')`) with prefix matching on `anthropic-` and `claude-` plus exact-match rejection of CLI slash-command collisions (help, clear, compact, model, exit, quit, settings, plus the bare `anthropic` / `claude` words). Both the pure validator (`methods/skill.ts`) and the Mongoose schema validator (`schema/skill.ts`) updated in lockstep; comments on each reference the other to prevent drift. - `research-anthropic-helper` and `about-claude` are now allowed; `anthropic-helper`, `claude-bot`, and `settings` are still rejected. Documentation: - Add docstrings on `ISkill`, `schema/skill.ts`, and `TSkill` explaining the semantics of `name` (Claude-visible identifier, kebab-case, stable), `displayTitle` (UI-only cosmetic label, NOT sent to Claude), `description` (highest-leverage trigger field), and `source` / `sourceMetadata` (reserved for phase 2+ external sync). - Add a detailed consistency comment on `bumpSkillVersionAndAdjustFileCount` explaining that it runs as a separate MongoDB operation from upsertSkillFile/deleteSkillFile, so `fileCount` can drift if the second op fails — options listed, tradeoff documented, phase 1 risk window noted as closed because upload is still stubbed. Tests: - data-schemas skill.spec.ts: destructure `{ skill, warnings }` from createSkill at every call site; add a TOO_SHORT warning test, a frontmatter strict-mode test, reserved-prefix tests (including positive cases for substring names that should pass), CLI reserved word tests, and a full `validateSkillFrontmatter` describe block covering unknown keys, type mismatches, and deep-nesting rejection. - api/server/routes/skills.test.js: bump default test description above the 20-char threshold, add a warning-emission test, add reserved-prefix + reserved-CLI-word tests, add an unknown-frontmatter- key test asserting the 400 response carries `issues` with `UNKNOWN_KEY`. * 📦 fix: Export CreateSkillResult from data-schemas Methods Index `CreateSkillResult` was defined in `methods/skill.ts` and consumed by `packages/api/src/skills/handlers.ts` but never re-exported from the methods barrel, so the type-check job failed with TS2724 "'@librechat/data-schemas' has no exported member named 'CreateSkillResult'". Rollup's bundle-mode build picked up the type via its internal resolver, but the standalone `tsc --noEmit` type-check ran against the package's public entrypoint and couldn't see it. Added the type import + export alongside the existing `UpdateSkillResult` export, which fixes the CI type-check without any runtime change.
This commit is contained in:
Danny Avila
parent
738003b220
commit
963068b112
47 changed files with 3965 additions and 8 deletions
|
|
@ -333,6 +333,7 @@ const deleteUserController = async (req, res) => {
|
|||
await db.deleteConversationTags({ user: user.id });
|
||||
await db.deleteAllUserMemories(user.id);
|
||||
await db.deleteUserPrompts(user.id);
|
||||
await db.deleteUserSkills(user.id);
|
||||
await deleteUserMcpServers(user.id);
|
||||
await db.deleteActions({ user: user.id });
|
||||
await db.deleteTokens({ userId: user.id });
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ jest.mock('~/models', () => {
|
|||
deleteAssistants: jest.fn().mockResolvedValue(undefined),
|
||||
deleteUserById: jest.fn().mockResolvedValue(undefined),
|
||||
deleteUserPrompts: jest.fn().mockResolvedValue(undefined),
|
||||
deleteUserSkills: jest.fn().mockResolvedValue(undefined),
|
||||
deleteMessages: jest.fn().mockResolvedValue(undefined),
|
||||
deleteBalances: jest.fn().mockResolvedValue(undefined),
|
||||
deleteActions: jest.fn().mockResolvedValue(undefined),
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ const mockProcessDeleteRequest = jest.fn();
|
|||
const mockDeleteToolCalls = jest.fn();
|
||||
const mockDeleteUserAgents = jest.fn();
|
||||
const mockDeleteUserPrompts = jest.fn();
|
||||
const mockDeleteUserSkills = jest.fn();
|
||||
|
||||
jest.mock('@librechat/data-schemas', () => ({
|
||||
logger: { error: jest.fn(), info: jest.fn() },
|
||||
|
|
@ -56,6 +57,7 @@ jest.mock('~/models', () => ({
|
|||
deleteToolCalls: (...args) => mockDeleteToolCalls(...args),
|
||||
deleteUserAgents: (...args) => mockDeleteUserAgents(...args),
|
||||
deleteUserPrompts: (...args) => mockDeleteUserPrompts(...args),
|
||||
deleteUserSkills: (...args) => mockDeleteUserSkills(...args),
|
||||
deleteTransactions: jest.fn(),
|
||||
deleteBalances: jest.fn(),
|
||||
deleteAllAgentApiKeys: jest.fn(),
|
||||
|
|
@ -130,6 +132,7 @@ function stubDeletionMocks() {
|
|||
mockDeleteToolCalls.mockResolvedValue();
|
||||
mockDeleteUserAgents.mockResolvedValue();
|
||||
mockDeleteUserPrompts.mockResolvedValue();
|
||||
mockDeleteUserSkills.mockResolvedValue(0);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
|
|
@ -148,6 +151,9 @@ describe('deleteUserController - 2FA enforcement', () => {
|
|||
expect(res.status).toHaveBeenCalledWith(200);
|
||||
expect(res.send).toHaveBeenCalledWith({ message: 'User deleted' });
|
||||
expect(mockDeleteMessages).toHaveBeenCalled();
|
||||
expect(mockDeleteUserAgents).toHaveBeenCalledWith('user1');
|
||||
expect(mockDeleteUserPrompts).toHaveBeenCalledWith('user1');
|
||||
expect(mockDeleteUserSkills).toHaveBeenCalledWith('user1');
|
||||
expect(mockVerifyOTPOrBackupCode).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ const HANDLED_RESOURCE_TYPES = {
|
|||
[ResourceType.REMOTE_AGENT]: 'deleteUserAgents',
|
||||
[ResourceType.PROMPTGROUP]: 'deleteUserPrompts',
|
||||
[ResourceType.MCPSERVER]: 'deleteUserMcpServers',
|
||||
[ResourceType.SKILL]: 'deleteUserSkills',
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -310,6 +310,7 @@ if (cluster.isMaster) {
|
|||
app.use('/api/convos', routes.convos);
|
||||
app.use('/api/presets', routes.presets);
|
||||
app.use('/api/prompts', routes.prompts);
|
||||
app.use('/api/skills', routes.skills);
|
||||
app.use('/api/categories', routes.categories);
|
||||
app.use('/api/endpoints', routes.endpoints);
|
||||
app.use('/api/balance', routes.balance);
|
||||
|
|
|
|||
|
|
@ -168,6 +168,7 @@ const startServer = async () => {
|
|||
app.use('/api/convos', routes.convos);
|
||||
app.use('/api/presets', routes.presets);
|
||||
app.use('/api/prompts', routes.prompts);
|
||||
app.use('/api/skills', routes.skills);
|
||||
app.use('/api/categories', routes.categories);
|
||||
app.use('/api/endpoints', routes.endpoints);
|
||||
app.use('/api/balance', routes.balance);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,32 @@
|
|||
const { ResourceType } = require('librechat-data-provider');
|
||||
const { canAccessResource } = require('./canAccessResource');
|
||||
const { getSkillById } = require('~/models');
|
||||
|
||||
/**
|
||||
* Skill-specific middleware factory that checks skill access permissions.
|
||||
* Wraps the generic `canAccessResource` with the SKILL resource type and
|
||||
* `getSkillById` as the ID resolver.
|
||||
*
|
||||
* @param {Object} options
|
||||
* @param {number} options.requiredPermission - Permission bit required (1=view, 2=edit, 4=delete, 8=share)
|
||||
* @param {string} [options.resourceIdParam='id'] - Route parameter name holding the skill id
|
||||
* @returns {Function} Express middleware
|
||||
*/
|
||||
const canAccessSkillResource = (options) => {
|
||||
const { requiredPermission, resourceIdParam = 'id' } = options || {};
|
||||
|
||||
if (!requiredPermission || typeof requiredPermission !== 'number') {
|
||||
throw new Error('canAccessSkillResource: requiredPermission is required and must be a number');
|
||||
}
|
||||
|
||||
return canAccessResource({
|
||||
resourceType: ResourceType.SKILL,
|
||||
requiredPermission,
|
||||
resourceIdParam,
|
||||
idResolver: getSkillById,
|
||||
});
|
||||
};
|
||||
|
||||
module.exports = {
|
||||
canAccessSkillResource,
|
||||
};
|
||||
|
|
@ -4,6 +4,7 @@ const { canAccessAgentFromBody } = require('./canAccessAgentFromBody');
|
|||
const { canAccessPromptViaGroup } = require('./canAccessPromptViaGroup');
|
||||
const { canAccessPromptGroupResource } = require('./canAccessPromptGroupResource');
|
||||
const { canAccessMCPServerResource } = require('./canAccessMCPServerResource');
|
||||
const { canAccessSkillResource } = require('./canAccessSkillResource');
|
||||
|
||||
module.exports = {
|
||||
canAccessResource,
|
||||
|
|
@ -12,4 +13,5 @@ module.exports = {
|
|||
canAccessPromptViaGroup,
|
||||
canAccessPromptGroupResource,
|
||||
canAccessMCPServerResource,
|
||||
canAccessSkillResource,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ const resourceToPermissionType = {
|
|||
[ResourceType.PROMPTGROUP]: PermissionTypes.PROMPTS,
|
||||
[ResourceType.MCPSERVER]: PermissionTypes.MCP_SERVERS,
|
||||
[ResourceType.REMOTE_AGENT]: PermissionTypes.REMOTE_AGENTS,
|
||||
[ResourceType.SKILL]: PermissionTypes.SKILLS,
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ const {
|
|||
const { requireJwtAuth, checkBan, uaParser, canAccessResource } = require('~/server/middleware');
|
||||
const { checkPeoplePickerAccess } = require('~/server/middleware/checkPeoplePickerAccess');
|
||||
const { checkSharePublicAccess } = require('~/server/middleware/checkSharePublicAccess');
|
||||
const { findMCPServerByObjectId } = require('~/models');
|
||||
const { findMCPServerByObjectId, getSkillById } = require('~/models');
|
||||
|
||||
const router = express.Router();
|
||||
|
||||
|
|
@ -72,6 +72,13 @@ const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) =
|
|||
resourceIdParam: 'resourceId',
|
||||
idResolver: findMCPServerByObjectId,
|
||||
});
|
||||
} else if (resourceType === ResourceType.SKILL) {
|
||||
middleware = canAccessResource({
|
||||
resourceType: ResourceType.SKILL,
|
||||
requiredPermission,
|
||||
resourceIdParam: 'resourceId',
|
||||
idResolver: getSkillById,
|
||||
});
|
||||
} else {
|
||||
return res.status(400).json({
|
||||
error: 'Bad Request',
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ const messages = require('./messages');
|
|||
const memories = require('./memories');
|
||||
const presets = require('./presets');
|
||||
const prompts = require('./prompts');
|
||||
const skills = require('./skills');
|
||||
const balance = require('./balance');
|
||||
const actions = require('./actions');
|
||||
const apiKeys = require('./apiKeys');
|
||||
|
|
@ -56,6 +57,7 @@ module.exports = {
|
|||
config,
|
||||
models,
|
||||
prompts,
|
||||
skills,
|
||||
actions,
|
||||
presets,
|
||||
balance,
|
||||
|
|
|
|||
102
api/server/routes/skills.js
Normal file
102
api/server/routes/skills.js
Normal file
|
|
@ -0,0 +1,102 @@
|
|||
const express = require('express');
|
||||
const { createSkillsHandlers, generateCheckAccess } = require('@librechat/api');
|
||||
const { isValidObjectIdString } = require('@librechat/data-schemas');
|
||||
const { PermissionBits, PermissionTypes, Permissions } = require('librechat-data-provider');
|
||||
const {
|
||||
createSkill,
|
||||
getSkillById,
|
||||
listSkillsByAccess,
|
||||
updateSkill,
|
||||
deleteSkill,
|
||||
listSkillFiles,
|
||||
deleteSkillFile,
|
||||
getRoleByName,
|
||||
} = require('~/models');
|
||||
const { requireJwtAuth, canAccessSkillResource } = require('~/server/middleware');
|
||||
const {
|
||||
findAccessibleResources,
|
||||
findPubliclyAccessibleResources,
|
||||
grantPermission,
|
||||
} = require('~/server/services/PermissionService');
|
||||
|
||||
const router = express.Router();
|
||||
|
||||
// Role-based capability gates. Mirrors prompts.js — the ACL middleware on each
|
||||
// route handles per-resource permissions; these check that the caller's role
|
||||
// is even allowed to use / create skills at all.
|
||||
const checkSkillAccess = generateCheckAccess({
|
||||
permissionType: PermissionTypes.SKILLS,
|
||||
permissions: [Permissions.USE],
|
||||
getRoleByName,
|
||||
});
|
||||
const checkSkillCreate = generateCheckAccess({
|
||||
permissionType: PermissionTypes.SKILLS,
|
||||
permissions: [Permissions.USE, Permissions.CREATE],
|
||||
getRoleByName,
|
||||
});
|
||||
|
||||
router.use(requireJwtAuth);
|
||||
router.use(checkSkillAccess);
|
||||
|
||||
const handlers = createSkillsHandlers({
|
||||
createSkill,
|
||||
getSkillById,
|
||||
listSkillsByAccess,
|
||||
updateSkill,
|
||||
deleteSkill,
|
||||
listSkillFiles,
|
||||
deleteSkillFile,
|
||||
findAccessibleResources,
|
||||
findPubliclyAccessibleResources,
|
||||
grantPermission,
|
||||
isValidObjectIdString,
|
||||
});
|
||||
|
||||
router.get('/', handlers.list);
|
||||
router.post('/', checkSkillCreate, handlers.create);
|
||||
|
||||
router.get(
|
||||
'/:id',
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }),
|
||||
handlers.get,
|
||||
);
|
||||
|
||||
router.patch(
|
||||
'/:id',
|
||||
checkSkillCreate,
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }),
|
||||
handlers.patch,
|
||||
);
|
||||
|
||||
router.delete(
|
||||
'/:id',
|
||||
checkSkillCreate,
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.DELETE }),
|
||||
handlers.delete,
|
||||
);
|
||||
|
||||
router.get(
|
||||
'/:id/files',
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }),
|
||||
handlers.listFiles,
|
||||
);
|
||||
|
||||
router.post(
|
||||
'/:id/files',
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }),
|
||||
handlers.uploadFileStub,
|
||||
);
|
||||
|
||||
router.get(
|
||||
'/:id/files/:relativePath',
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }),
|
||||
handlers.downloadFileStub,
|
||||
);
|
||||
|
||||
router.delete(
|
||||
'/:id/files/:relativePath',
|
||||
canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }),
|
||||
handlers.deleteFile,
|
||||
);
|
||||
|
||||
module.exports = router;
|
||||
480
api/server/routes/skills.test.js
Normal file
480
api/server/routes/skills.test.js
Normal file
|
|
@ -0,0 +1,480 @@
|
|||
const express = require('express');
|
||||
const request = require('supertest');
|
||||
const mongoose = require('mongoose');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
const {
|
||||
SystemRoles,
|
||||
ResourceType,
|
||||
AccessRoleIds,
|
||||
PrincipalType,
|
||||
PermissionBits,
|
||||
} = require('librechat-data-provider');
|
||||
|
||||
jest.mock('~/server/services/Config', () => ({
|
||||
getCachedTools: jest.fn().mockResolvedValue({}),
|
||||
}));
|
||||
|
||||
jest.mock('~/models', () => {
|
||||
const mongoose = require('mongoose');
|
||||
const { createMethods } = require('@librechat/data-schemas');
|
||||
const methods = createMethods(mongoose, {
|
||||
removeAllPermissions: async ({ resourceType, resourceId }) => {
|
||||
const AclEntry = mongoose.models.AclEntry;
|
||||
if (AclEntry) {
|
||||
await AclEntry.deleteMany({ resourceType, resourceId });
|
||||
}
|
||||
},
|
||||
});
|
||||
// Override getRoleByName to return a permissive SKILLS capability block for all
|
||||
// test users. The real role seeding relies on `initializeRoles` which this
|
||||
// suite intentionally skips to keep setup minimal.
|
||||
return {
|
||||
...methods,
|
||||
getRoleByName: jest.fn(),
|
||||
};
|
||||
});
|
||||
|
||||
jest.mock('~/server/middleware', () => ({
|
||||
requireJwtAuth: (req, res, next) => next(),
|
||||
canAccessSkillResource: jest.requireActual('~/server/middleware').canAccessSkillResource,
|
||||
}));
|
||||
|
||||
let app;
|
||||
let mongoServer;
|
||||
let skillRoutes;
|
||||
let Skill;
|
||||
let SkillFile;
|
||||
let AclEntry;
|
||||
let AccessRole;
|
||||
let User;
|
||||
let testUsers;
|
||||
let testRoles;
|
||||
let grantPermission;
|
||||
let currentTestUser;
|
||||
|
||||
function setTestUser(user) {
|
||||
currentTestUser = user;
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
mongoServer = await MongoMemoryServer.create();
|
||||
await mongoose.connect(mongoServer.getUri());
|
||||
|
||||
const dbModels = require('~/db/models');
|
||||
Skill = dbModels.Skill;
|
||||
SkillFile = dbModels.SkillFile;
|
||||
AclEntry = dbModels.AclEntry;
|
||||
AccessRole = dbModels.AccessRole;
|
||||
User = dbModels.User;
|
||||
|
||||
const permissionService = require('~/server/services/PermissionService');
|
||||
grantPermission = permissionService.grantPermission;
|
||||
|
||||
await setupTestData();
|
||||
|
||||
app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, res, next) => {
|
||||
if (currentTestUser) {
|
||||
req.user = {
|
||||
...(currentTestUser.toObject ? currentTestUser.toObject() : currentTestUser),
|
||||
id: currentTestUser._id.toString(),
|
||||
_id: currentTestUser._id,
|
||||
name: currentTestUser.name,
|
||||
role: currentTestUser.role,
|
||||
};
|
||||
}
|
||||
next();
|
||||
});
|
||||
|
||||
currentTestUser = testUsers.owner;
|
||||
skillRoutes = require('./skills');
|
||||
app.use('/api/skills', skillRoutes);
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await Skill.deleteMany({});
|
||||
await SkillFile.deleteMany({});
|
||||
await AclEntry.deleteMany({});
|
||||
currentTestUser = testUsers.owner;
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await mongoose.disconnect();
|
||||
await mongoServer.stop();
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
async function setupTestData() {
|
||||
testRoles = {
|
||||
viewer: await AccessRole.create({
|
||||
accessRoleId: AccessRoleIds.SKILL_VIEWER,
|
||||
name: 'Viewer',
|
||||
resourceType: ResourceType.SKILL,
|
||||
permBits: PermissionBits.VIEW,
|
||||
}),
|
||||
editor: await AccessRole.create({
|
||||
accessRoleId: AccessRoleIds.SKILL_EDITOR,
|
||||
name: 'Editor',
|
||||
resourceType: ResourceType.SKILL,
|
||||
permBits: PermissionBits.VIEW | PermissionBits.EDIT,
|
||||
}),
|
||||
owner: await AccessRole.create({
|
||||
accessRoleId: AccessRoleIds.SKILL_OWNER,
|
||||
name: 'Owner',
|
||||
resourceType: ResourceType.SKILL,
|
||||
permBits:
|
||||
PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE | PermissionBits.SHARE,
|
||||
}),
|
||||
};
|
||||
|
||||
testUsers = {
|
||||
owner: await User.create({
|
||||
name: 'Skill Owner',
|
||||
email: 'skill-owner@test.com',
|
||||
role: SystemRoles.USER,
|
||||
}),
|
||||
editor: await User.create({
|
||||
name: 'Skill Editor',
|
||||
email: 'skill-editor@test.com',
|
||||
role: SystemRoles.USER,
|
||||
}),
|
||||
noAccess: await User.create({
|
||||
name: 'No Access',
|
||||
email: 'no-access@test.com',
|
||||
role: SystemRoles.USER,
|
||||
}),
|
||||
};
|
||||
|
||||
const { getRoleByName } = require('~/models');
|
||||
getRoleByName.mockImplementation((roleName) => {
|
||||
if (roleName === SystemRoles.USER || roleName === SystemRoles.ADMIN) {
|
||||
return {
|
||||
permissions: {
|
||||
SKILLS: {
|
||||
USE: true,
|
||||
CREATE: true,
|
||||
SHARE: true,
|
||||
SHARE_PUBLIC: true,
|
||||
},
|
||||
},
|
||||
};
|
||||
}
|
||||
return null;
|
||||
});
|
||||
}
|
||||
|
||||
async function createSkillAsOwner(overrides = {}) {
|
||||
// Description is deliberately kept above the 20-char short-description
|
||||
// warning threshold so existing tests don't trip the coaching warning.
|
||||
const res = await request(app)
|
||||
.post('/api/skills')
|
||||
.send({
|
||||
name: 'demo-skill',
|
||||
description: 'A small demo skill used in routing integration tests.',
|
||||
body: '# Demo',
|
||||
...overrides,
|
||||
});
|
||||
return res;
|
||||
}
|
||||
|
||||
describe('Skill routes', () => {
|
||||
let errSpy;
|
||||
beforeEach(() => {
|
||||
errSpy = jest.spyOn(console, 'error').mockImplementation();
|
||||
});
|
||||
afterEach(() => errSpy.mockRestore());
|
||||
|
||||
describe('POST /api/skills', () => {
|
||||
it('creates a skill and grants SKILL_OWNER ACL', async () => {
|
||||
const res = await createSkillAsOwner();
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body._id).toBeDefined();
|
||||
expect(res.body.version).toBe(1);
|
||||
expect(res.body.name).toBe('demo-skill');
|
||||
// No warnings on a description that comfortably clears the threshold.
|
||||
expect(res.body.warnings).toBeUndefined();
|
||||
|
||||
const acl = await AclEntry.findOne({
|
||||
resourceType: ResourceType.SKILL,
|
||||
resourceId: res.body._id,
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUsers.owner._id,
|
||||
});
|
||||
expect(acl).toBeTruthy();
|
||||
expect(acl.roleId.toString()).toBe(testRoles.owner._id.toString());
|
||||
});
|
||||
|
||||
it('attaches a TOO_SHORT warning on create when description is under 20 chars', async () => {
|
||||
const res = await createSkillAsOwner({
|
||||
name: 'short-desc-skill',
|
||||
description: 'Too short.',
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body._id).toBeDefined();
|
||||
expect(Array.isArray(res.body.warnings)).toBe(true);
|
||||
expect(res.body.warnings).toEqual([
|
||||
expect.objectContaining({
|
||||
field: 'description',
|
||||
code: 'TOO_SHORT',
|
||||
severity: 'warning',
|
||||
}),
|
||||
]);
|
||||
});
|
||||
|
||||
it('rejects names starting with reserved brand prefixes', async () => {
|
||||
const anthropic = await createSkillAsOwner({ name: 'anthropic-helper' });
|
||||
expect(anthropic.status).toBe(400);
|
||||
const claude = await createSkillAsOwner({ name: 'claude-helper' });
|
||||
expect(claude.status).toBe(400);
|
||||
});
|
||||
|
||||
it('allows names that merely contain reserved brand words as substrings', async () => {
|
||||
const res = await createSkillAsOwner({ name: 'research-anthropic-helper' });
|
||||
expect(res.status).toBe(201);
|
||||
});
|
||||
|
||||
it('rejects reserved CLI command names', async () => {
|
||||
const res = await createSkillAsOwner({ name: 'settings' });
|
||||
expect(res.status).toBe(400);
|
||||
});
|
||||
|
||||
it('rejects frontmatter with unknown keys', async () => {
|
||||
const res = await createSkillAsOwner({
|
||||
name: 'bad-frontmatter-skill',
|
||||
frontmatter: { 'not-a-real-key': 'value' },
|
||||
});
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.issues).toEqual(
|
||||
expect.arrayContaining([expect.objectContaining({ code: 'UNKNOWN_KEY' })]),
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects missing description with 400', async () => {
|
||||
const res = await request(app).post('/api/skills').send({ name: 'x-skill', body: '' });
|
||||
expect(res.status).toBe(400);
|
||||
});
|
||||
|
||||
it('rejects invalid name with 400 validation failure', async () => {
|
||||
const res = await createSkillAsOwner({ name: 'BAD NAME' });
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.issues).toBeDefined();
|
||||
});
|
||||
|
||||
it('rejects duplicate names with 409', async () => {
|
||||
const a = await createSkillAsOwner();
|
||||
expect(a.status).toBe(201);
|
||||
const b = await createSkillAsOwner();
|
||||
expect(b.status).toBe(409);
|
||||
});
|
||||
});
|
||||
|
||||
describe('GET /api/skills', () => {
|
||||
it('returns only skills the caller can access', async () => {
|
||||
const mine = await createSkillAsOwner({ name: 'mine-skill' });
|
||||
expect(mine.status).toBe(201);
|
||||
|
||||
setTestUser(testUsers.noAccess);
|
||||
const other = await createSkillAsOwner({ name: 'other-skill' });
|
||||
expect(other.status).toBe(201);
|
||||
// Note: the user middleware grants owner perms to whichever user created, so both
|
||||
// users see their own skill only.
|
||||
|
||||
setTestUser(testUsers.owner);
|
||||
const res = await request(app).get('/api/skills');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.skills.length).toBe(1);
|
||||
expect(res.body.skills[0].name).toBe('mine-skill');
|
||||
});
|
||||
});
|
||||
|
||||
describe('GET /api/skills/:id', () => {
|
||||
it('returns 403 when the user has no access', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
expect(created.status).toBe(201);
|
||||
setTestUser(testUsers.noAccess);
|
||||
const res = await request(app).get(`/api/skills/${created.body._id}`);
|
||||
expect(res.status).toBe(403);
|
||||
});
|
||||
|
||||
it('returns the skill to the owner with isPublic flag', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app).get(`/api/skills/${created.body._id}`);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.name).toBe('demo-skill');
|
||||
expect(res.body.isPublic).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('PATCH /api/skills/:id (optimistic concurrency)', () => {
|
||||
it('updates with correct expectedVersion and bumps version', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app)
|
||||
.patch(`/api/skills/${created.body._id}`)
|
||||
.send({ expectedVersion: 1, description: 'Updated description' });
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.version).toBe(2);
|
||||
expect(res.body.description).toBe('Updated description');
|
||||
});
|
||||
|
||||
it('returns 409 on stale expectedVersion', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const first = await request(app)
|
||||
.patch(`/api/skills/${created.body._id}`)
|
||||
.send({ expectedVersion: 1, description: 'First' });
|
||||
expect(first.status).toBe(200);
|
||||
|
||||
const stale = await request(app)
|
||||
.patch(`/api/skills/${created.body._id}`)
|
||||
.send({ expectedVersion: 1, description: 'Stale' });
|
||||
expect(stale.status).toBe(409);
|
||||
expect(stale.body.error).toBe('skill_version_conflict');
|
||||
expect(stale.body.current.version).toBe(2);
|
||||
});
|
||||
|
||||
it('rejects updates without expectedVersion', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app)
|
||||
.patch(`/api/skills/${created.body._id}`)
|
||||
.send({ description: 'no version' });
|
||||
expect(res.status).toBe(400);
|
||||
});
|
||||
|
||||
it('returns 403 for a user without EDIT permission', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
setTestUser(testUsers.noAccess);
|
||||
const res = await request(app)
|
||||
.patch(`/api/skills/${created.body._id}`)
|
||||
.send({ expectedVersion: 1, description: 'nope' });
|
||||
expect(res.status).toBe(403);
|
||||
});
|
||||
});
|
||||
|
||||
describe('DELETE /api/skills/:id', () => {
|
||||
it('deletes and cascades ACL entries', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app).delete(`/api/skills/${created.body._id}`);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.deleted).toBe(true);
|
||||
|
||||
const remainingAcl = await AclEntry.countDocuments({
|
||||
resourceType: ResourceType.SKILL,
|
||||
resourceId: created.body._id,
|
||||
});
|
||||
expect(remainingAcl).toBe(0);
|
||||
});
|
||||
|
||||
it('returns 403 for a non-owner', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
setTestUser(testUsers.noAccess);
|
||||
const res = await request(app).delete(`/api/skills/${created.body._id}`);
|
||||
expect(res.status).toBe(403);
|
||||
});
|
||||
});
|
||||
|
||||
describe('GET /api/skills/:id/files', () => {
|
||||
it('returns an empty list for a skill with no files', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app).get(`/api/skills/${created.body._id}/files`);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.files).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('POST /api/skills/:id/files (stub)', () => {
|
||||
it('returns 501 with phase marker', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app).post(`/api/skills/${created.body._id}/files`);
|
||||
expect(res.status).toBe(501);
|
||||
expect(res.body.phase).toBe(2);
|
||||
});
|
||||
});
|
||||
|
||||
describe('GET /api/skills/:id/files/:relativePath (stub)', () => {
|
||||
it('returns 501 stub', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app).get(
|
||||
`/api/skills/${created.body._id}/files/scripts%2Fparse.sh`,
|
||||
);
|
||||
expect(res.status).toBe(501);
|
||||
});
|
||||
});
|
||||
|
||||
describe('DELETE /api/skills/:id/files/:relativePath', () => {
|
||||
const { upsertSkillFile } = require('~/models');
|
||||
|
||||
it('deletes an existing skill file, bumps skill version, and returns 200', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
await upsertSkillFile({
|
||||
skillId: created.body._id,
|
||||
relativePath: 'scripts/parse.sh',
|
||||
file_id: 'file-1',
|
||||
filename: 'parse.sh',
|
||||
filepath: '/tmp/parse.sh',
|
||||
source: 'local',
|
||||
mimeType: 'text/x-shellscript',
|
||||
bytes: 42,
|
||||
author: testUsers.owner._id,
|
||||
});
|
||||
|
||||
const beforeSkill = await request(app).get(`/api/skills/${created.body._id}`);
|
||||
expect(beforeSkill.body.fileCount).toBe(1);
|
||||
expect(beforeSkill.body.version).toBe(2);
|
||||
|
||||
const res = await request(app).delete(
|
||||
`/api/skills/${created.body._id}/files/scripts%2Fparse.sh`,
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({
|
||||
skillId: created.body._id,
|
||||
relativePath: 'scripts/parse.sh',
|
||||
deleted: true,
|
||||
});
|
||||
|
||||
const afterSkill = await request(app).get(`/api/skills/${created.body._id}`);
|
||||
expect(afterSkill.body.fileCount).toBe(0);
|
||||
expect(afterSkill.body.version).toBe(3);
|
||||
});
|
||||
|
||||
it('returns 404 when the file does not exist', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
const res = await request(app).delete(
|
||||
`/api/skills/${created.body._id}/files/scripts%2Fmissing.sh`,
|
||||
);
|
||||
expect(res.status).toBe(404);
|
||||
});
|
||||
|
||||
it('returns 403 for a non-owner', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
setTestUser(testUsers.noAccess);
|
||||
const res = await request(app).delete(
|
||||
`/api/skills/${created.body._id}/files/scripts%2Fparse.sh`,
|
||||
);
|
||||
expect(res.status).toBe(403);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Sharing via ACL (editor grant)', () => {
|
||||
it('allows an editor to patch a shared skill', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
await grantPermission({
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUsers.editor._id,
|
||||
resourceType: ResourceType.SKILL,
|
||||
resourceId: created.body._id,
|
||||
accessRoleId: AccessRoleIds.SKILL_EDITOR,
|
||||
grantedBy: testUsers.owner._id,
|
||||
});
|
||||
|
||||
setTestUser(testUsers.editor);
|
||||
const res = await request(app)
|
||||
.patch(`/api/skills/${created.body._id}`)
|
||||
.send({ expectedVersion: 1, description: 'Edited by editor' });
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
// Editor should NOT be able to delete
|
||||
const del = await request(app).delete(`/api/skills/${created.body._id}`);
|
||||
expect(del.status).toBe(403);
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue