regression: Department & Unit limits not correctly applied to agents

apps/meteor/ee/app/livechat-enterprise/server/lib/restrictQuery.ts

@@ -31,16 +31,22 @@ export const restrictQuery = async ({
 		// IF user is trying to filter by a unit he doens't have access to, apply empty filter (no matches)
 		userUnits = [...userUnit.intersection(filteredUnits)];
 	}
-	// TODO: units is meant to include units and departments, however, here were only using them as units
-	// We have to change the filter to something like { $or: [{ ancestors: {$in: units }}, {_id: {$in: units}}] }
-	const departments = await LivechatDepartment.find({ ancestors: { $in: userUnits } }, { projection: { _id: 1 } }).toArray();
+
+	const departments = await LivechatDepartment.find(
+		{ $or: [{ ancestors: { $in: userUnits } }, { _id: { $in: userUnits } }] },
+		{ projection: { _id: 1 } },
+	).toArray();
 
 	const expressions = query.$and || [];
 	const condition = {
-		$or: [{ departmentAncestors: { $in: userUnits } }, { departmentId: { $in: departments.map(({ _id }) => _id) } }],
+		$or: [
+			{ departmentAncestors: { $in: userUnits } },
+			{ departmentId: { $in: departments.map(({ _id }) => _id) } },
+			{ departmentId: { $exists: false } },
+		],
 	};
 	query.$and = [condition, ...expressions];
 
-	cbLogger.debug({ msg: 'Applying room query restrictions', userUnits });
+	cbLogger.debug({ msg: 'Applying room query restrictions', userUnits, query });
 	return query;
 };

apps/meteor/ee/tests/unit/apps/livechat-enterprise/lib/restrictQuery.tests.ts

@@ -0,0 +1,301 @@
+import { expect } from 'chai';
+import proxyquire from 'proxyquire';
+import sinon from 'sinon';
+
+type RestrictQueryParams = {
+	originalQuery?: Record<string, any>;
+	unitsFilter?: string[];
+	userId?: string;
+};
+
+describe('restrictQuery', () => {
+	const modulePath = '../../../../../app/livechat-enterprise/server/lib/restrictQuery';
+
+	// Helper to require the SUT with injected stubs
+	const loadSut = ({
+		getUnitsFromUserResult,
+		departmentsToReturn = [],
+		captureFindArgs = false,
+	}: {
+		getUnitsFromUserResult: any;
+		departmentsToReturn?: Array<{ _id: string }>;
+		captureFindArgs?: boolean;
+	}) => {
+		const getUnitsFromUserStub = sinon.stub().callsFake(async () => getUnitsFromUserResult);
+
+		const findArgs: any[] = [];
+		const findStub = sinon.stub().callsFake((query: any, projection: any) => {
+			if (captureFindArgs) {
+				findArgs.push({ query, projection });
+			}
+			return {
+				toArray: async () => departmentsToReturn,
+			};
+		});
+
+		const debugStub = sinon.stub();
+
+		const { restrictQuery } = proxyquire.noCallThru().load(modulePath, {
+			'@rocket.chat/models': {
+				LivechatDepartment: { find: findStub },
+			},
+			'../methods/getUnitsFromUserRoles': {
+				getUnitsFromUser: getUnitsFromUserStub,
+			},
+			'./logger': {
+				cbLogger: { debug: debugStub },
+			},
+		});
+
+		return { restrictQuery, getUnitsFromUserStub, findStub, debugStub, findArgs };
+	};
+
+	afterEach(() => {
+		sinon.restore();
+	});
+
+	it('returns the original query untouched when user has no units and no unitsFilter is provided', async () => {
+		const { restrictQuery, getUnitsFromUserStub, findStub, debugStub } = loadSut({
+			getUnitsFromUserResult: null,
+		});
+
+		const originalQuery = { status: 'open' };
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery,
+			userId: 'user-id',
+		});
+
+		expect(getUnitsFromUserStub.calledOnce).to.equal(true);
+		expect(getUnitsFromUserStub.firstCall.args).to.deep.equal(['user-id']);
+		expect(findStub.called).to.equal(false);
+		expect(debugStub.called).to.equal(false);
+
+		// It must be a shallow copy but equivalent content
+		expect(result).to.deep.equal(originalQuery);
+		// Ensure original wasn't mutated
+		expect(originalQuery).to.not.have.property('$and');
+	});
+
+	it('returns the original query untouched when user has no units and no unitsFilter is provided', async () => {
+		const { restrictQuery, getUnitsFromUserStub, findStub, debugStub } = loadSut({
+			getUnitsFromUserResult: null,
+		});
+
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery: undefined,
+			userId: 'user-id',
+		});
+
+		expect(getUnitsFromUserStub.calledOnce).to.equal(true);
+		expect(getUnitsFromUserStub.firstCall.args).to.deep.equal(['user-id']);
+		expect(findStub.called).to.equal(false);
+		expect(debugStub.called).to.equal(false);
+
+		// It must be a shallow copy but equivalent content
+		expect(result).to.deep.equal({});
+	});
+
+	it('applies departmentAncestors filter directly when user has no units but a unitsFilter is provided', async () => {
+		const { restrictQuery, getUnitsFromUserStub, findStub, debugStub } = loadSut({
+			getUnitsFromUserResult: undefined,
+		});
+
+		const originalQuery = { status: { $ne: 'closed' } };
+		const unitsFilter = ['unit-a', 'unit-b'];
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery,
+			unitsFilter,
+		});
+
+		expect(getUnitsFromUserStub.calledOnce).to.equal(true);
+		expect(findStub.called).to.equal(false);
+		expect(debugStub.called).to.equal(false);
+
+		expect(result).to.deep.equal({
+			status: { $ne: 'closed' },
+			departmentAncestors: { $in: unitsFilter },
+		});
+		// Ensure original wasn't mutated
+		expect(originalQuery).to.not.have.property('departmentAncestors');
+	});
+
+	it('adds $and restriction when user has units and no unitsFilter is provided', async () => {
+		const { restrictQuery, findArgs, debugStub } = loadSut({
+			getUnitsFromUserResult: ['unit-1', 'unit-2'],
+			departmentsToReturn: [{ _id: 'dep-1' }, { _id: 'dep-2' }],
+			captureFindArgs: true,
+		});
+
+		const originalQuery = { status: 'open' };
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery,
+			userId: 'u',
+		});
+
+		// Verify LivechatDepartment.find was called with the proper query and projection
+		expect(findArgs).to.have.length(1);
+		expect(findArgs[0].query).to.deep.equal({
+			$or: [{ ancestors: { $in: ['unit-1', 'unit-2'] } }, { _id: { $in: ['unit-1', 'unit-2'] } }],
+		});
+		expect(findArgs[0].projection).to.deep.equal({ projection: { _id: 1 } });
+
+		// Verify the resulting query
+		expect(result).to.have.property('$and').that.is.an('array').with.lengthOf(1);
+		expect(result.$and[0]).to.deep.equal({
+			$or: [
+				{ departmentAncestors: { $in: ['unit-1', 'unit-2'] } },
+				{ departmentId: { $in: ['dep-1', 'dep-2'] } },
+				{ departmentId: { $exists: false } },
+			],
+		});
+		expect(result.status).to.equal('open');
+
+		// Logger should be called with the final query and computed userUnits
+		expect(debugStub.callCount).to.equal(1);
+		const debugArg = debugStub.firstCall.args[0];
+		expect(debugArg).to.include({ msg: 'Applying room query restrictions' });
+		expect(debugArg.userUnits).to.deep.equal(['unit-1', 'unit-2']);
+		expect(debugArg.query).to.deep.equal(result);
+	});
+
+	it('intersects user units with unitsFilter and applies restrictions accordingly', async () => {
+		const { restrictQuery, findArgs, debugStub } = loadSut({
+			getUnitsFromUserResult: ['unit-1', 'unit-2'],
+			departmentsToReturn: [{ _id: 'dep-3' }],
+			captureFindArgs: true,
+		});
+
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery: { priority: 'high' },
+			unitsFilter: ['unit-2', 'unit-3'], // intersection => ['unit-2']
+			userId: 'u',
+		});
+
+		// find must be called using the intersected set ['unit-2']
+		expect(findArgs).to.have.length(1);
+		expect(findArgs[0].query).to.deep.equal({
+			$or: [{ ancestors: { $in: ['unit-2'] } }, { _id: { $in: ['unit-2'] } }],
+		});
+
+		// Resulting $and must use the intersected units as well
+		expect(result.$and).to.have.length(1);
+		expect(result.$and[0]).to.deep.equal({
+			$or: [{ departmentAncestors: { $in: ['unit-2'] } }, { departmentId: { $in: ['dep-3'] } }, { departmentId: { $exists: false } }],
+		});
+		expect(result.priority).to.equal('high');
+
+		expect(debugStub.callCount).to.equal(1);
+		const debugArg = debugStub.firstCall.args[0];
+		expect(debugArg.userUnits).to.deep.equal(['unit-2']);
+	});
+
+	it('places the new restriction before existing $and expressions and preserves other fields', async () => {
+		const { restrictQuery } = loadSut({
+			getUnitsFromUserResult: ['u-a'],
+			departmentsToReturn: [{ _id: 'dep-a' }],
+		});
+
+		const originalQuery = { $and: [{ foo: 1 }], bar: 2 };
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery,
+		});
+
+		expect(result).to.have.property('bar', 2);
+		expect(result).to.have.property('$and').that.is.an('array').with.lengthOf(2);
+
+		// First element is the newly added condition
+		expect(result.$and[0]).to.deep.equal({
+			$or: [{ departmentAncestors: { $in: ['u-a'] } }, { departmentId: { $in: ['dep-a'] } }, { departmentId: { $exists: false } }],
+		});
+		// Then the existing ones
+		expect(result.$and[1]).to.deep.equal({ foo: 1 });
+
+		// Ensure original $and was not mutated by reference insertion (since we recompose it)
+		expect(originalQuery.$and).to.deep.equal([{ foo: 1 }]);
+	});
+
+	it('handles empty intersections by producing restrictive filters while still allowing rooms without department', async () => {
+		const { restrictQuery, findArgs } = loadSut({
+			getUnitsFromUserResult: ['unit-x'],
+			departmentsToReturn: [], // none will match
+			captureFindArgs: true,
+		});
+
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery: {},
+			unitsFilter: ['unit-y'], // intersection => []
+		});
+
+		// find called with empty $in arrays
+		expect(findArgs).to.have.length(1);
+		expect(findArgs[0].query).to.deep.equal({
+			$or: [{ ancestors: { $in: [] } }, { _id: { $in: [] } }],
+		});
+
+		// Resulting condition must reflect empty unit and department lists,
+		// but still allow rooms where departmentId does not exist
+		expect(result.$and).to.have.length(1);
+		expect(result.$and[0]).to.deep.equal({
+			$or: [{ departmentAncestors: { $in: [] } }, { departmentId: { $in: [] } }, { departmentId: { $exists: false } }],
+		});
+	});
+
+	// New test: Validate the exact parameters passed to LivechatDepartment.find
+	it('calls LivechatDepartment.find with correct query and projection parameters', async () => {
+		const userUnits = ['unit-10', 'unit-20', 'unit-30'];
+		const { restrictQuery, findArgs, findStub } = loadSut({
+			getUnitsFromUserResult: userUnits,
+			departmentsToReturn: [{ _id: 'dep-10' }],
+			captureFindArgs: true,
+		});
+
+		const originalQuery = { $and: [{ state: { $ne: 'archived' } }], custom: 'x' };
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery,
+			userId: 'abc',
+		});
+
+		// Ensure find has been called exactly once
+		expect(findStub.callCount).to.equal(1);
+		expect(findArgs).to.have.length(1);
+
+		// Validate query argument
+		expect(findArgs[0].query).to.deep.equal({
+			$or: [{ ancestors: { $in: userUnits } }, { _id: { $in: userUnits } }],
+		});
+
+		// Validate projection argument is exactly as expected
+		expect(findArgs[0].projection).to.deep.equal({ projection: { _id: 1 } });
+
+		// And the resulting query contains our original bits
+		expect(result).to.have.property('custom', 'x');
+		expect(result.$and[1]).to.deep.equal({ state: { $ne: 'archived' } });
+	});
+
+	// New test: Ensure departments returned by find are added to the query's departmentId $in clause
+	it('uses departments returned by find to build departmentId $in condition', async () => {
+		const userUnits = ['unit-a'];
+		const departments = [{ _id: 'dep-a' }, { _id: 'dep-b' }, { _id: 'dep-c' }];
+		const { restrictQuery } = loadSut({
+			getUnitsFromUserResult: userUnits,
+			departmentsToReturn: departments,
+			captureFindArgs: true,
+		});
+
+		const result = await (restrictQuery as (p: RestrictQueryParams) => Promise<any>)({
+			originalQuery: { tag: 'vip' },
+			userId: 'some-user',
+		});
+
+		// Validate that all department ids are present in the $in list
+		expect(result).to.have.property('$and');
+		expect(result.$and).to.be.an('array').with.lengthOf(1);
+		const orCondition = result.$and[0].$or;
+		expect(orCondition).to.be.an('array').with.lengthOf(3);
+
+		// Extract the departmentId condition
+		const deptIdCond = orCondition.find((c: any) => Object.keys(c)[0] === 'departmentId');
+		expect(deptIdCond).to.deep.equal({ departmentId: { $in: ['dep-a', 'dep-b', 'dep-c'] } });
+		expect(result.tag).to.equal('vip');
+	});
+});

apps/meteor/tests/data/livechat/department.ts

@@ -97,7 +97,7 @@ export const createDepartmentWithMethod = ({
 			});
 	});
 
-type OnlineAgent = {
+export type OnlineAgent = {
 	user: WithRequiredProperty<IUser, 'username'>;
 	credentials: Credentials;
 };