From 3060087d0bb1d59b1ba6a640086daa0c8f60d62b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 30 May 2026 14:54:59 -0400 Subject: [PATCH] fix: Resolve global access role outside tenant context for synced skill grants Addresses Codex P2 (github.ts:1166): default access roles (incl. skill_viewer) are seeded globally with no tenantId under runAsSystem, but a tenant-scoped sync wraps ensurePublicViewer in the source's tenant context. The PermissionService grantPermission resolved the role via a tenant-isolated AccessRole query, so the global role did not match and tenant-scoped syncs failed with 'Role skill_viewer not found'. The sync adapter now resolves the role inside runAsSystem (matching the global seed) and writes the ACL entry in the active tenant context, so the AclEntry is tenant-scoped (visible to tenant users) while the role lookup still succeeds. Covered by service tests for the resolve-vs-write split and the missing-role failure. --- api/server/services/Skills/sync.js | 35 ++++++++++++- api/server/services/Skills/sync.test.js | 65 ++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index 6dbab2bebc..1f806c2109 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -4,9 +4,9 @@ const { createGitHubSkillSyncRunner, startGitHubSkillSyncScheduler, } = require('@librechat/api'); +const { runAsSystem } = require('@librechat/data-schemas'); const db = require('~/models'); const { getAppConfig } = require('~/server/services/Config'); -const { grantPermission } = require('~/server/services/PermissionService'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { getFileStrategy } = require('~/server/utils/getFileStrategy'); @@ -77,7 +77,38 @@ function createRunner() { upsertSkillFile: db.upsertSkillFile, deleteSkillFile: db.deleteSkillFile, deleteSkill: db.deleteSkill, - grantPermission, + grantPermission: async ({ + principalType, + principalId, + resourceType, + resourceId, + accessRoleId, + grantedBy, + }) => { + // Default access roles are seeded globally (no tenantId) under runAsSystem, + // but the runner may execute inside a source's tenant context. Resolve the + // role outside tenant isolation so the global role matches, then write the + // ACL entry in the active (tenant) context so tenant users can see it. + const role = await runAsSystem(() => db.findRoleByIdentifier(accessRoleId)); + if (!role) { + throw new Error(`Role ${accessRoleId} not found`); + } + if (role.resourceType !== resourceType) { + throw new Error( + `Role ${accessRoleId} is for ${role.resourceType} resources, not ${resourceType}`, + ); + } + return db.grantPermission( + principalType, + principalId, + resourceType, + resourceId, + role.permBits, + grantedBy, + undefined, + role._id, + ); + }, saveBuffer: async ({ userId, buffer, fileName, basePath, isImage, tenantId }) => { const storage = await resolveSkillStorage({ isImage }); const filepath = await storage.saveBuffer({ diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index da4991abfe..46901211d3 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -1,6 +1,8 @@ const mockGetAppConfig = jest.fn(); const mockGetStrategyFunctions = jest.fn(); const mockGetFileStrategy = jest.fn(); +const mockFindRoleByIdentifier = jest.fn(); +const mockGrantPermission = jest.fn(); let mockRunnerDeps; jest.mock('~/server/services/Config', () => ({ @@ -23,8 +25,10 @@ jest.mock('@librechat/data-schemas', () => ({ runAsSystem: jest.fn((fn) => fn()), })); -jest.mock('~/models', () => ({})); -jest.mock('~/server/services/PermissionService', () => ({ grantPermission: jest.fn() })); +jest.mock('~/models', () => ({ + findRoleByIdentifier: mockFindRoleByIdentifier, + grantPermission: mockGrantPermission, +})); jest.mock('~/server/services/Files/strategies', () => ({ getStrategyFunctions: mockGetStrategyFunctions, })); @@ -36,6 +40,8 @@ describe('GitHub skill sync service', () => { mockGetAppConfig.mockReset(); mockGetStrategyFunctions.mockReset(); mockGetFileStrategy.mockReset(); + mockFindRoleByIdentifier.mockReset(); + mockGrantPermission.mockReset(); mockRunnerDeps = undefined; }); @@ -133,4 +139,59 @@ describe('GitHub skill sync service', () => { expect(runAsSystem).not.toHaveBeenCalled(); }); + + it('resolves the access role outside tenant isolation but writes the ACL in context', async () => { + const { runAsSystem } = require('@librechat/data-schemas'); + mockGetAppConfig.mockResolvedValue({ skillSync: undefined, paths: {} }); + mockFindRoleByIdentifier.mockResolvedValue({ + _id: 'role-object-id', + resourceType: 'skill', + permBits: 1, + }); + mockGrantPermission.mockResolvedValue({ _id: 'acl-entry-id' }); + + const service = require('./sync'); + service.initializeGitHubSkillSync({ skillSync: undefined }); + await mockRunnerDeps.grantPermission({ + principalType: 'public', + principalId: null, + resourceType: 'skill', + resourceId: 'skill-id', + accessRoleId: 'skill_viewer', + grantedBy: 'system', + }); + + expect(runAsSystem).toHaveBeenCalledTimes(1); + expect(mockFindRoleByIdentifier).toHaveBeenCalledWith('skill_viewer'); + expect(mockGrantPermission).toHaveBeenCalledWith( + 'public', + null, + 'skill', + 'skill-id', + 1, + 'system', + undefined, + 'role-object-id', + ); + }); + + it('fails the grant when the access role does not exist', async () => { + mockGetAppConfig.mockResolvedValue({ skillSync: undefined, paths: {} }); + mockFindRoleByIdentifier.mockResolvedValue(null); + + const service = require('./sync'); + service.initializeGitHubSkillSync({ skillSync: undefined }); + + await expect( + mockRunnerDeps.grantPermission({ + principalType: 'public', + principalId: null, + resourceType: 'skill', + resourceId: 'skill-id', + accessRoleId: 'skill_viewer', + grantedBy: 'system', + }), + ).rejects.toThrow('Role skill_viewer not found'); + expect(mockGrantPermission).not.toHaveBeenCalled(); + }); });