mirror of
https://github.com/samanhappy/mcphub.git
synced 2025-12-24 02:39:19 -05:00
Fix group creation not persisting - updated mergeSettings implementations
Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
@@ -22,7 +22,19 @@ export class DataServiceImpl implements DataService {
|
||||
}
|
||||
|
||||
mergeSettings(all: McpSettings, newSettings: McpSettings, _user?: IUser): McpSettings {
|
||||
return newSettings;
|
||||
// Merge all fields from newSettings into all, preserving fields not present in newSettings
|
||||
return {
|
||||
...all,
|
||||
...newSettings,
|
||||
// Ensure arrays and objects are properly handled
|
||||
users: newSettings.users !== undefined ? newSettings.users : all.users,
|
||||
mcpServers: newSettings.mcpServers !== undefined ? newSettings.mcpServers : all.mcpServers,
|
||||
groups: newSettings.groups !== undefined ? newSettings.groups : all.groups,
|
||||
systemConfig:
|
||||
newSettings.systemConfig !== undefined ? newSettings.systemConfig : all.systemConfig,
|
||||
userConfigs:
|
||||
newSettings.userConfigs !== undefined ? newSettings.userConfigs : all.userConfigs,
|
||||
};
|
||||
}
|
||||
|
||||
getPermissions(_user: IUser): string[] {
|
||||
|
||||
@@ -36,11 +36,17 @@ export class DataServicex implements DataService {
|
||||
// Use passed user parameter if available, otherwise fall back to context
|
||||
const currentUser = user || UserContextService.getInstance().getCurrentUser();
|
||||
if (!currentUser || currentUser.isAdmin) {
|
||||
// Admin users can modify all settings
|
||||
const result = { ...all };
|
||||
result.users = newSettings.users;
|
||||
result.systemConfig = newSettings.systemConfig;
|
||||
// Merge all fields, using newSettings values when present
|
||||
if (newSettings.users !== undefined) result.users = newSettings.users;
|
||||
if (newSettings.mcpServers !== undefined) result.mcpServers = newSettings.mcpServers;
|
||||
if (newSettings.groups !== undefined) result.groups = newSettings.groups;
|
||||
if (newSettings.systemConfig !== undefined) result.systemConfig = newSettings.systemConfig;
|
||||
if (newSettings.userConfigs !== undefined) result.userConfigs = newSettings.userConfigs;
|
||||
return result;
|
||||
} else {
|
||||
// Non-admin users can only modify their own userConfig
|
||||
const result = JSON.parse(JSON.stringify(all));
|
||||
if (!result.userConfigs) {
|
||||
result.userConfigs = {};
|
||||
|
||||
225
tests/services/dataServiceMerge.test.ts
Normal file
225
tests/services/dataServiceMerge.test.ts
Normal file
@@ -0,0 +1,225 @@
|
||||
import { DataServiceImpl } from '../../src/services/dataService.js';
|
||||
import { DataServicex } from '../../src/services/dataServicex.js';
|
||||
import { McpSettings, IUser } from '../../src/types/index.js';
|
||||
|
||||
describe('DataService mergeSettings', () => {
|
||||
describe('DataServiceImpl', () => {
|
||||
let service: DataServiceImpl;
|
||||
|
||||
beforeEach(() => {
|
||||
service = new DataServiceImpl();
|
||||
});
|
||||
|
||||
it('should merge all fields from newSettings into existing settings', () => {
|
||||
const all: McpSettings = {
|
||||
users: [
|
||||
{ username: 'admin', password: 'hash1', isAdmin: true },
|
||||
{ username: 'user1', password: 'hash2', isAdmin: false },
|
||||
],
|
||||
mcpServers: {
|
||||
'server1': { command: 'cmd1', args: [] },
|
||||
'server2': { command: 'cmd2', args: [] },
|
||||
},
|
||||
groups: [
|
||||
{ id: '1', name: 'group1', servers: [], owner: 'admin' },
|
||||
],
|
||||
systemConfig: {
|
||||
routing: { enableGlobalRoute: true, enableGroupNameRoute: true },
|
||||
},
|
||||
userConfigs: {
|
||||
user1: { routing: { enableGlobalRoute: false, enableGroupNameRoute: false } },
|
||||
},
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [
|
||||
{ id: '1', name: 'group1', servers: [], owner: 'admin' },
|
||||
{ id: '2', name: 'group2', servers: [], owner: 'admin' },
|
||||
],
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings);
|
||||
|
||||
// New groups should be present
|
||||
expect(result.groups).toHaveLength(2);
|
||||
expect(result.groups).toEqual(newSettings.groups);
|
||||
|
||||
// Other fields from 'all' should be preserved when not in newSettings
|
||||
expect(result.users).toEqual(all.users);
|
||||
expect(result.systemConfig).toEqual(all.systemConfig);
|
||||
expect(result.userConfigs).toEqual(all.userConfigs);
|
||||
});
|
||||
|
||||
it('should preserve fields not present in newSettings', () => {
|
||||
const all: McpSettings = {
|
||||
users: [{ username: 'admin', password: 'hash', isAdmin: true }],
|
||||
mcpServers: {
|
||||
'server1': { command: 'cmd1', args: [] },
|
||||
},
|
||||
groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
|
||||
systemConfig: { routing: { enableGlobalRoute: true, enableGroupNameRoute: true } },
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [
|
||||
{ id: '1', name: 'group1', servers: [], owner: 'admin' },
|
||||
{ id: '2', name: 'group2', servers: [], owner: 'admin' },
|
||||
],
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings);
|
||||
|
||||
// Groups from newSettings should be present
|
||||
expect(result.groups).toEqual(newSettings.groups);
|
||||
|
||||
// Other fields should be preserved from 'all'
|
||||
expect(result.users).toEqual(all.users);
|
||||
expect(result.systemConfig).toEqual(all.systemConfig);
|
||||
});
|
||||
|
||||
it('should handle undefined fields in newSettings', () => {
|
||||
const all: McpSettings = {
|
||||
users: [{ username: 'admin', password: 'hash', isAdmin: true }],
|
||||
mcpServers: { 'server1': { command: 'cmd1', args: [] } },
|
||||
groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
// groups is undefined
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings);
|
||||
|
||||
// Groups from 'all' should be preserved since newSettings.groups is undefined
|
||||
expect(result.groups).toEqual(all.groups);
|
||||
expect(result.users).toEqual(all.users);
|
||||
});
|
||||
});
|
||||
|
||||
describe('DataServicex', () => {
|
||||
let service: DataServicex;
|
||||
|
||||
beforeEach(() => {
|
||||
service = new DataServicex();
|
||||
});
|
||||
|
||||
it('should merge all fields for admin users', () => {
|
||||
const adminUser: IUser = { username: 'admin', password: 'hash', isAdmin: true };
|
||||
|
||||
const all: McpSettings = {
|
||||
users: [adminUser],
|
||||
mcpServers: {
|
||||
'server1': { command: 'cmd1', args: [] },
|
||||
},
|
||||
groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
|
||||
systemConfig: { routing: { enableGlobalRoute: true, enableGroupNameRoute: true } },
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [
|
||||
{ id: '1', name: 'group1', servers: [], owner: 'admin' },
|
||||
{ id: '2', name: 'group2', servers: [], owner: 'admin' },
|
||||
],
|
||||
systemConfig: { routing: { enableGlobalRoute: false, enableGroupNameRoute: false } },
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings, adminUser);
|
||||
|
||||
// All fields from newSettings should be merged
|
||||
expect(result.groups).toEqual(newSettings.groups);
|
||||
expect(result.systemConfig).toEqual(newSettings.systemConfig);
|
||||
|
||||
// Users should be preserved from 'all' since not in newSettings
|
||||
expect(result.users).toEqual(all.users);
|
||||
});
|
||||
|
||||
it('should preserve groups for admin users when adding new groups', () => {
|
||||
const adminUser: IUser = { username: 'admin', password: 'hash', isAdmin: true };
|
||||
|
||||
const all: McpSettings = {
|
||||
users: [adminUser],
|
||||
mcpServers: { 'server1': { command: 'cmd1', args: [] } },
|
||||
groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [
|
||||
{ id: '1', name: 'group1', servers: [], owner: 'admin' },
|
||||
{ id: '2', name: 'group2', servers: [], owner: 'admin' },
|
||||
],
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings, adminUser);
|
||||
|
||||
// New groups should be present
|
||||
expect(result.groups).toHaveLength(2);
|
||||
expect(result.groups).toEqual(newSettings.groups);
|
||||
});
|
||||
|
||||
it('should handle non-admin users correctly', () => {
|
||||
const regularUser: IUser = { username: 'user1', password: 'hash', isAdmin: false };
|
||||
|
||||
const all: McpSettings = {
|
||||
users: [regularUser],
|
||||
mcpServers: { 'server1': { command: 'cmd1', args: [] } },
|
||||
groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
|
||||
userConfigs: {},
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
systemConfig: {
|
||||
routing: {
|
||||
enableGlobalRoute: false,
|
||||
enableGroupNameRoute: false,
|
||||
enableBearerAuth: true,
|
||||
bearerAuthKey: 'test-key',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings, regularUser);
|
||||
|
||||
// For non-admin users, groups should not change
|
||||
expect(result.groups).toEqual(all.groups);
|
||||
|
||||
// User config should be updated
|
||||
expect(result.userConfigs).toBeDefined();
|
||||
expect(result.userConfigs?.['user1']).toBeDefined();
|
||||
expect(result.userConfigs?.['user1'].routing).toEqual(newSettings.systemConfig?.routing);
|
||||
});
|
||||
|
||||
it('should preserve all fields from original when only updating systemConfig', () => {
|
||||
const adminUser: IUser = { username: 'admin', password: 'hash', isAdmin: true };
|
||||
|
||||
const all: McpSettings = {
|
||||
users: [adminUser],
|
||||
mcpServers: { 'server1': { command: 'cmd1', args: [] } },
|
||||
groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
|
||||
systemConfig: { routing: { enableGlobalRoute: true, enableGroupNameRoute: true } },
|
||||
};
|
||||
|
||||
const newSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
systemConfig: { routing: { enableGlobalRoute: false, enableGroupNameRoute: false } },
|
||||
};
|
||||
|
||||
const result = service.mergeSettings(all, newSettings, adminUser);
|
||||
|
||||
// Groups should be preserved from 'all' since not in newSettings
|
||||
expect(result.groups).toEqual(all.groups);
|
||||
// SystemConfig should be updated from newSettings
|
||||
expect(result.systemConfig).toEqual(newSettings.systemConfig);
|
||||
// Users should be preserved from 'all' since not in newSettings
|
||||
expect(result.users).toEqual(all.users);
|
||||
// mcpServers should be updated from newSettings (empty in this case)
|
||||
// This is expected behavior - when mcpServers is explicitly provided, it replaces the old value
|
||||
expect(result.mcpServers).toEqual(newSettings.mcpServers);
|
||||
});
|
||||
});
|
||||
});
|
||||
262
tests/services/groupService.test.ts
Normal file
262
tests/services/groupService.test.ts
Normal file
@@ -0,0 +1,262 @@
|
||||
import { createGroup, getAllGroups, deleteGroup } from '../../src/services/groupService.js';
|
||||
import * as config from '../../src/config/index.js';
|
||||
import { McpSettings } from '../../src/types/index.js';
|
||||
|
||||
// Mock the config module
|
||||
jest.mock('../../src/config/index.js', () => {
|
||||
let mockSettings: McpSettings = {
|
||||
mcpServers: {
|
||||
'test-server': {
|
||||
command: 'test',
|
||||
args: [],
|
||||
},
|
||||
},
|
||||
groups: [],
|
||||
users: [],
|
||||
};
|
||||
|
||||
return {
|
||||
loadSettings: jest.fn(() => mockSettings),
|
||||
saveSettings: jest.fn((settings: McpSettings) => {
|
||||
mockSettings = settings;
|
||||
return true;
|
||||
}),
|
||||
clearSettingsCache: jest.fn(),
|
||||
};
|
||||
});
|
||||
|
||||
// Mock the mcpService
|
||||
jest.mock('../../src/services/mcpService.js', () => ({
|
||||
notifyToolChanged: jest.fn(),
|
||||
}));
|
||||
|
||||
// Mock the dataService
|
||||
jest.mock('../../src/services/services.js', () => ({
|
||||
getDataService: jest.fn(() => ({
|
||||
filterData: (data: any[]) => data,
|
||||
filterSettings: (settings: any) => settings,
|
||||
mergeSettings: (all: any, newSettings: any) => newSettings,
|
||||
getPermissions: () => ['*'],
|
||||
})),
|
||||
}));
|
||||
|
||||
describe('Group Service', () => {
|
||||
beforeEach(() => {
|
||||
// Reset mocks before each test
|
||||
jest.clearAllMocks();
|
||||
|
||||
// Reset the mock settings to initial state
|
||||
const mockSettings: McpSettings = {
|
||||
mcpServers: {
|
||||
'test-server': {
|
||||
command: 'test',
|
||||
args: [],
|
||||
},
|
||||
'test-server-2': {
|
||||
command: 'test2',
|
||||
args: [],
|
||||
},
|
||||
},
|
||||
groups: [],
|
||||
users: [],
|
||||
};
|
||||
|
||||
(config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
|
||||
(config.saveSettings as jest.Mock).mockImplementation((settings: McpSettings) => {
|
||||
mockSettings.groups = settings.groups;
|
||||
return true;
|
||||
});
|
||||
});
|
||||
|
||||
describe('createGroup', () => {
|
||||
it('should create a new group and persist it', () => {
|
||||
const groupName = 'test-group';
|
||||
const description = 'Test group description';
|
||||
const servers = ['test-server'];
|
||||
|
||||
const newGroup = createGroup(groupName, description, servers);
|
||||
|
||||
expect(newGroup).not.toBeNull();
|
||||
expect(newGroup?.name).toBe(groupName);
|
||||
expect(newGroup?.description).toBe(description);
|
||||
expect(newGroup?.servers).toHaveLength(1);
|
||||
expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
|
||||
|
||||
// Verify saveSettings was called
|
||||
expect(config.saveSettings).toHaveBeenCalled();
|
||||
|
||||
// Verify the settings passed to saveSettings include the new group
|
||||
const savedSettings = (config.saveSettings as jest.Mock).mock.calls[0][0];
|
||||
expect(savedSettings.groups).toHaveLength(1);
|
||||
expect(savedSettings.groups[0].name).toBe(groupName);
|
||||
});
|
||||
|
||||
it('should create a group with multiple servers', () => {
|
||||
const groupName = 'multi-server-group';
|
||||
const servers = ['test-server', 'test-server-2'];
|
||||
|
||||
const newGroup = createGroup(groupName, undefined, servers);
|
||||
|
||||
expect(newGroup).not.toBeNull();
|
||||
expect(newGroup?.servers).toHaveLength(2);
|
||||
expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
|
||||
expect(newGroup?.servers[1]).toEqual({ name: 'test-server-2', tools: 'all' });
|
||||
});
|
||||
|
||||
it('should create a group with server configuration objects', () => {
|
||||
const groupName = 'config-group';
|
||||
const servers = [
|
||||
{ name: 'test-server', tools: 'all' },
|
||||
{ name: 'test-server-2', tools: ['tool1', 'tool2'] },
|
||||
];
|
||||
|
||||
const newGroup = createGroup(groupName, undefined, servers);
|
||||
|
||||
expect(newGroup).not.toBeNull();
|
||||
expect(newGroup?.servers).toHaveLength(2);
|
||||
expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
|
||||
expect(newGroup?.servers[1]).toEqual({ name: 'test-server-2', tools: ['tool1', 'tool2'] });
|
||||
});
|
||||
|
||||
it('should filter out non-existent servers', () => {
|
||||
const groupName = 'filtered-group';
|
||||
const servers = ['test-server', 'non-existent-server'];
|
||||
|
||||
const newGroup = createGroup(groupName, undefined, servers);
|
||||
|
||||
expect(newGroup).not.toBeNull();
|
||||
expect(newGroup?.servers).toHaveLength(1);
|
||||
expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
|
||||
});
|
||||
|
||||
it('should not create a group with duplicate name', () => {
|
||||
const groupName = 'duplicate-group';
|
||||
|
||||
// Create first group
|
||||
const firstGroup = createGroup(groupName, 'First group');
|
||||
expect(firstGroup).not.toBeNull();
|
||||
|
||||
// Update the mock to include the first group
|
||||
const mockSettings: McpSettings = {
|
||||
mcpServers: {
|
||||
'test-server': {
|
||||
command: 'test',
|
||||
args: [],
|
||||
},
|
||||
},
|
||||
groups: [firstGroup!],
|
||||
users: [],
|
||||
};
|
||||
(config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
|
||||
|
||||
// Try to create second group with same name
|
||||
const secondGroup = createGroup(groupName, 'Second group');
|
||||
expect(secondGroup).toBeNull();
|
||||
});
|
||||
|
||||
it('should set owner to admin by default', () => {
|
||||
const groupName = 'owned-group';
|
||||
|
||||
const newGroup = createGroup(groupName);
|
||||
|
||||
expect(newGroup).not.toBeNull();
|
||||
expect(newGroup?.owner).toBe('admin');
|
||||
});
|
||||
|
||||
it('should set custom owner when provided', () => {
|
||||
const groupName = 'custom-owned-group';
|
||||
const owner = 'testuser';
|
||||
|
||||
const newGroup = createGroup(groupName, undefined, [], owner);
|
||||
|
||||
expect(newGroup).not.toBeNull();
|
||||
expect(newGroup?.owner).toBe(owner);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getAllGroups', () => {
|
||||
it('should return all groups', () => {
|
||||
const mockSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [
|
||||
{
|
||||
id: '1',
|
||||
name: 'group1',
|
||||
servers: [],
|
||||
owner: 'admin',
|
||||
},
|
||||
{
|
||||
id: '2',
|
||||
name: 'group2',
|
||||
servers: [],
|
||||
owner: 'admin',
|
||||
},
|
||||
],
|
||||
users: [],
|
||||
};
|
||||
(config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
|
||||
|
||||
const groups = getAllGroups();
|
||||
|
||||
expect(groups).toHaveLength(2);
|
||||
expect(groups[0].name).toBe('group1');
|
||||
expect(groups[1].name).toBe('group2');
|
||||
});
|
||||
|
||||
it('should return empty array when no groups exist', () => {
|
||||
const mockSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
users: [],
|
||||
};
|
||||
(config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
|
||||
|
||||
const groups = getAllGroups();
|
||||
|
||||
expect(groups).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('deleteGroup', () => {
|
||||
it('should delete a group by id', () => {
|
||||
const mockSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [
|
||||
{
|
||||
id: 'group-to-delete',
|
||||
name: 'Delete Me',
|
||||
servers: [],
|
||||
owner: 'admin',
|
||||
},
|
||||
],
|
||||
users: [],
|
||||
};
|
||||
(config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
|
||||
(config.saveSettings as jest.Mock).mockImplementation((settings: McpSettings) => {
|
||||
mockSettings.groups = settings.groups;
|
||||
return true;
|
||||
});
|
||||
|
||||
const result = deleteGroup('group-to-delete');
|
||||
|
||||
expect(result).toBe(true);
|
||||
expect(config.saveSettings).toHaveBeenCalled();
|
||||
|
||||
// Verify the settings passed to saveSettings have the group removed
|
||||
const savedSettings = (config.saveSettings as jest.Mock).mock.calls[0][0];
|
||||
expect(savedSettings.groups).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('should return false when group does not exist', () => {
|
||||
const mockSettings: McpSettings = {
|
||||
mcpServers: {},
|
||||
groups: [],
|
||||
users: [],
|
||||
};
|
||||
(config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
|
||||
|
||||
const result = deleteGroup('non-existent-id');
|
||||
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user