Skip to content

Commit b6c3631

Browse files
KevLehmanKevLehman
authored
chore: Align cron job eviction process for abac (#40879)
1 parent ea1bdb6 commit b6c3631

4 files changed

Lines changed: 151 additions & 39 deletions

File tree

‎apps/meteor/tests/end-to-end/api/abac.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3167,10 +3167,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
31673167

31683168
await mockServerReset();
31693169
await seedDefaultMocks();
3170-
await seedGetDecisionBulk([
3171-
{ resourceDecisions: [{ decision: 'DECISION_PERMIT', ephemeralResourceId: room._id }] },
3172-
{ resourceDecisions: [{ decision: 'DECISION_PERMIT', ephemeralResourceId: room._id }] },
3173-
]);
3170+
await seedBulkDecisionByEntity([], 'DECISION_PERMIT');
31743171

31753172
await request
31763173
.post(`/api/v1/abac/rooms/${room._id}/attributes/${attrKey}`)

‎ee/packages/abac/src/pdp/VirtruPDP.spec.ts‎

Lines changed: 110 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { OnlyCompliantCanBeAddedToRoomError, PdpHealthCheckError } from '../errors';
2-
import { VirtruPDP } from './VirtruPDP';
2+
import { getDeniedSubjects, VirtruPDP } from './VirtruPDP';
3+
import type { Decision } from './types';
34

45
const serverFetchMock = jest.fn();
56
jest.mock('@rocket.chat/server-fetch', () => ({ serverFetch: (...a: unknown[]) => serverFetchMock(...a) }));
@@ -88,6 +89,35 @@ describe('VirtruPDP.isAvailable', () => {
8889
});
8990
});
9091

92+
describe('getDeniedSubjects', () => {
93+
const pair = (userId: string) => ({ user: { _id: userId }, room: { _id: 'r1' } });
94+
const [a, b, c] = [pair('a'), pair('b'), pair('c')];
95+
const resp = (...decisions: Decision[]) => ({
96+
resourceDecisions: decisions.map((decision) => ({ ephemeralResourceId: 'r1', decision })),
97+
});
98+
99+
it('collects subjects with an explicit DENY decision', () => {
100+
const responses = [resp('DECISION_DENY'), resp('DECISION_PERMIT'), resp('DECISION_DENY')];
101+
expect(getDeniedSubjects(responses, [a, b, c])).toEqual([a, c]);
102+
});
103+
104+
it('skips subjects when every decision is PERMIT', () => {
105+
expect(getDeniedSubjects([resp('DECISION_PERMIT')], [a])).toEqual([]);
106+
});
107+
108+
it('skips inconclusive subjects (UNSPECIFIED, empty decisions, missing response)', () => {
109+
expect(getDeniedSubjects([resp('DECISION_UNSPECIFIED'), resp(), undefined], [a, b, c])).toEqual([]);
110+
});
111+
112+
it('treats a DENY among PERMITs in one response as denied', () => {
113+
expect(getDeniedSubjects([resp('DECISION_PERMIT', 'DECISION_DENY')], [a])).toEqual([a]);
114+
});
115+
116+
it('ignores responses without a matching subject', () => {
117+
expect(getDeniedSubjects([resp('DECISION_DENY'), resp('DECISION_DENY')], [a])).toEqual([a]);
118+
});
119+
});
120+
91121
describe('VirtruPDP.canAccessObject', () => {
92122
const room = { _id: 'r1', abacAttributes: [{ key: 'clearance', values: ['secret'] }] };
93123

@@ -284,13 +314,39 @@ describe('VirtruPDP.onRoomAttributesChanged', () => {
284314
expect(result).toEqual([u2]);
285315
});
286316

287-
it('treats empty resourceDecisions as non-compliant', async () => {
317+
it('does not evict on empty resourceDecisions (inconclusive)', async () => {
288318
const u = user();
289319
usersFindActiveByRoomIds.mockReturnValue(asyncIterable([u]));
290320
const apiCall = jest.fn().mockResolvedValue({ decisionResponses: [{ resourceDecisions: [] }] });
291321
const pdp = new VirtruPDP(mkClient({ apiCall }));
292322
const result = await pdp.onRoomAttributesChanged(room, newAttrs);
293-
expect(result).toEqual([u]);
323+
expect(result).toEqual([]);
324+
});
325+
326+
it('does not evict on DECISION_UNSPECIFIED (inconclusive)', async () => {
327+
usersFindActiveByRoomIds.mockReturnValue(asyncIterable([user()]));
328+
const apiCall = jest.fn().mockResolvedValue({
329+
decisionResponses: [{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_UNSPECIFIED' }] }],
330+
});
331+
const pdp = new VirtruPDP(mkClient({ apiCall }));
332+
const result = await pdp.onRoomAttributesChanged(room, newAttrs);
333+
expect(result).toEqual([]);
334+
});
335+
336+
it('still evicts entity-keyless users alongside DENY-only filtering of decided users', async () => {
337+
const keyless = user({ _id: 'u0', emails: [] });
338+
const denied = user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com' }] });
339+
const unspecified = user({ _id: 'u3', username: 'carol', emails: [{ address: 'c@x.com' }] });
340+
usersFindActiveByRoomIds.mockReturnValue(asyncIterable([keyless, denied, unspecified]));
341+
const apiCall = jest.fn().mockResolvedValue({
342+
decisionResponses: [
343+
{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_DENY' }] },
344+
{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_UNSPECIFIED' }] },
345+
],
346+
});
347+
const pdp = new VirtruPDP(mkClient({ apiCall }));
348+
const result = await pdp.onRoomAttributesChanged(room, newAttrs);
349+
expect(result).toEqual([keyless, denied]);
294350
});
295351

296352
it('returns only entity-keyless users when ALL users lack entity keys (no API call)', async () => {
@@ -346,6 +402,17 @@ describe('VirtruPDP.onSubjectAttributesChanged', () => {
346402
expect(result).toEqual([rooms[1]]);
347403
});
348404

405+
it('still treats DECISION_UNSPECIFIED as non-compliant (LDAP-driven path cannot yield inconclusive decisions)', async () => {
406+
const rooms = [{ _id: 'r1', abacAttributes: [{ key: 'k', values: ['v'] }] }];
407+
roomsFindPrivateRoomsByIdsWithAbacAttributes.mockReturnValue(cursor(rooms));
408+
const apiCall = jest.fn().mockResolvedValue({
409+
decisionResponses: [{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_UNSPECIFIED' }] }],
410+
});
411+
const pdp = new VirtruPDP(mkClient({ apiCall }));
412+
const result = await pdp.onSubjectAttributesChanged(user({ __rooms: ['r1'] }) as any, []);
413+
expect(result).toEqual(rooms);
414+
});
415+
349416
it('splits >200 rooms into multiple decision batches', async () => {
350417
const total = 401;
351418
const rooms = Array.from({ length: total }, (_, i) => ({ _id: `r${i}`, abacAttributes: [{ key: 'k', values: ['v'] }] }));
@@ -414,13 +481,51 @@ describe('VirtruPDP.evaluateUserRooms', () => {
414481
expect(result).toEqual([{ user: u, room: rooms[1] }]);
415482
});
416483

417-
it('treats empty resourceDecisions as non-compliant', async () => {
484+
it('does not evict on empty resourceDecisions (inconclusive)', async () => {
418485
const u = user();
419486
const rooms = [{ _id: 'r1', abacAttributes: [{ key: 'k', values: ['v'] }] }];
420487
const apiCall = jest.fn().mockResolvedValue({ decisionResponses: [{ resourceDecisions: [] }] });
421488
const pdp = new VirtruPDP(mkClient({ apiCall }));
422489
const result = await pdp.evaluateUserRooms([{ user: u, rooms }]);
423-
expect(result).toEqual([{ user: u, room: rooms[0] }]);
490+
expect(result).toEqual([]);
491+
});
492+
493+
it('does not evict on DECISION_UNSPECIFIED (inconclusive)', async () => {
494+
const u = user();
495+
const rooms = [{ _id: 'r1', abacAttributes: [{ key: 'k', values: ['v'] }] }];
496+
const apiCall = jest.fn().mockResolvedValue({
497+
decisionResponses: [{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_UNSPECIFIED' }] }],
498+
});
499+
const pdp = new VirtruPDP(mkClient({ apiCall }));
500+
const result = await pdp.evaluateUserRooms([{ user: u, rooms }]);
501+
expect(result).toEqual([]);
502+
});
503+
504+
it('evicts only explicit DENY among mixed PERMIT/UNSPECIFIED/DENY decisions', async () => {
505+
const u = user();
506+
const rooms = [
507+
{ _id: 'rP', abacAttributes: [{ key: 'k', values: ['v'] }] },
508+
{ _id: 'rU', abacAttributes: [{ key: 'k', values: ['v'] }] },
509+
{ _id: 'rD', abacAttributes: [{ key: 'k', values: ['v'] }] },
510+
];
511+
const apiCall = jest.fn().mockResolvedValue({
512+
decisionResponses: [
513+
{ resourceDecisions: [{ ephemeralResourceId: 'rP', decision: 'DECISION_PERMIT' }] },
514+
{ resourceDecisions: [{ ephemeralResourceId: 'rU', decision: 'DECISION_UNSPECIFIED' }] },
515+
{ resourceDecisions: [{ ephemeralResourceId: 'rD', decision: 'DECISION_DENY' }] },
516+
],
517+
});
518+
const pdp = new VirtruPDP(mkClient({ apiCall }));
519+
const result = await pdp.evaluateUserRooms([{ user: u, rooms }]);
520+
expect(result).toEqual([{ user: u, room: rooms[2] }]);
521+
});
522+
523+
it('rejects when the decision call fails (no evictions, sweep retried next run)', async () => {
524+
const apiCall = jest.fn().mockRejectedValue(new Error('pdp down'));
525+
const pdp = new VirtruPDP(mkClient({ apiCall }));
526+
const rooms = [{ _id: 'r1', abacAttributes: [{ key: 'k', values: ['v'] }] }];
527+
await expect(pdp.evaluateUserRooms([{ user: user(), rooms }])).rejects.toThrow('pdp down');
528+
expect(apiCall).toHaveBeenCalled();
424529
});
425530
});
426531

@@ -460,14 +565,6 @@ describe('VirtruPDP — PDP unreachable (decision call rejects)', () => {
460565
await expect(pdp.onSubjectAttributesChanged(user({ __rooms: ['r1'] }) as any, [])).rejects.toThrow('pdp down');
461566
expect(apiCall).toHaveBeenCalled();
462567
});
463-
464-
it('evaluateUserRooms rejects when the decision call fails', async () => {
465-
const apiCall = pdpDown();
466-
const pdp = new VirtruPDP(mkClient({ apiCall }));
467-
const rooms = [{ _id: 'r1', abacAttributes: [{ key: 'k', values: ['v'] }] }];
468-
await expect(pdp.evaluateUserRooms([{ user: user(), rooms } as any])).rejects.toThrow('pdp down');
469-
expect(apiCall).toHaveBeenCalled();
470-
});
471568
});
472569

473570
describe('reevaluateUsers', () => {

‎ee/packages/abac/src/pdp/VirtruPDP.ts‎

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,36 @@ import pLimit from 'p-limit';
66

77
import { OnlyCompliantCanBeAddedToRoomError, PdpHealthCheckError } from '../errors';
88
import { logger } from '../logger';
9-
import type { IPolicyDecisionPoint, IGetDecisionBulkRequest, IGetDecisionBulkResponse, IResourceDecision, ReevaluationUser } from './types';
9+
import type {
10+
IPolicyDecisionPoint,
11+
IGetDecisionBulkRequest,
12+
IGetDecisionBulkResponse,
13+
IResourceDecision,
14+
NonCompliantPair,
15+
ReevaluationUser,
16+
} from './types';
1017
import { HEALTH_CHECK_TIMEOUT } from '../clients/virtru/VirtruClient';
1118
import type { VirtruClient } from '../clients/virtru/VirtruClient';
1219
import { buildEntityIdentifier, buildAttributeFqns, getUserEntityKey } from '../clients/virtru/identity';
1320

1421
const pdpLogger = logger.section('VirtruPDP');
1522

23+
export const getDeniedSubjects = <T extends { user: Pick<IUser, '_id'>; room: Pick<IRoom, '_id'> }>(
24+
responses: Array<{ resourceDecisions?: IResourceDecision[] } | undefined>,
25+
subjects: T[],
26+
): T[] => {
27+
return subjects.filter((subject, index) => {
28+
const decisions = responses[index]?.resourceDecisions ?? [];
29+
if (decisions.some((rd) => rd.decision === 'DECISION_DENY')) {
30+
return true;
31+
}
32+
if (!decisions.length || decisions.some((rd) => rd.decision !== 'DECISION_PERMIT')) {
33+
pdpLogger.warn({ msg: 'Inconclusive PDP decision, eviction skipped', rid: subject.room._id, userId: subject.user._id });
34+
}
35+
return false;
36+
});
37+
};
38+
1639
export class VirtruPDP implements IPolicyDecisionPoint {
1740
private client: VirtruClient;
1841

@@ -250,7 +273,7 @@ export class VirtruPDP implements IPolicyDecisionPoint {
250273
const config = this.client.getConfig();
251274
const nonCompliantUsers: IUser[] = [];
252275
const decisionRequests: IGetDecisionBulkRequest[] = [];
253-
const requestUserIndex: IUser[] = [];
276+
const requestIndex: Array<{ user: IUser; room: typeof room }> = [];
254277
const fqns = buildAttributeFqns(config.attributeNamespace, newAttributes);
255278

256279
for await (const user of users) {
@@ -261,7 +284,7 @@ export class VirtruPDP implements IPolicyDecisionPoint {
261284
continue;
262285
}
263286

264-
requestUserIndex.push(user);
287+
requestIndex.push({ user, room });
265288
decisionRequests.push({
266289
entityIdentifier: {
267290
entityChain: {
@@ -284,12 +307,7 @@ export class VirtruPDP implements IPolicyDecisionPoint {
284307

285308
const responses = await this.getDecisionBulk(decisionRequests);
286309

287-
responses.forEach((resp, index) => {
288-
const permitted = resp?.resourceDecisions?.length && resp.resourceDecisions.every((rd) => rd.decision === 'DECISION_PERMIT');
289-
if (!permitted && requestUserIndex[index]) {
290-
nonCompliantUsers.push(requestUserIndex[index]);
291-
}
292-
});
310+
nonCompliantUsers.push(...getDeniedSubjects(responses, requestIndex).map(({ user }) => user));
293311

294312
return nonCompliantUsers;
295313
}
@@ -299,19 +317,19 @@ export class VirtruPDP implements IPolicyDecisionPoint {
299317
user: Pick<IUser, '_id' | 'emails' | 'username'>;
300318
rooms: AtLeast<IRoom, '_id' | 'abacAttributes'>[];
301319
}>,
302-
): Promise<Array<{ user: Pick<IUser, '_id' | 'emails' | 'username'>; room: IRoom }>> {
303-
const requestIndex: Array<{ user: Pick<IUser, '_id' | 'emails' | 'username'>; room: AtLeast<IRoom, '_id' | 'abacAttributes'> }> = [];
320+
): Promise<NonCompliantPair[]> {
321+
const requestIndex: NonCompliantPair[] = [];
304322
const allRequests: IGetDecisionBulkRequest[] = [];
305323

306324
const config = this.client.getConfig();
307-
const nonCompliant: Array<{ user: Pick<IUser, '_id' | 'emails' | 'username'>; room: IRoom }> = [];
325+
const nonCompliant: NonCompliantPair[] = [];
308326

309327
for (const { user, rooms } of entries) {
310328
const entityKey = getUserEntityKey(config.defaultEntityKey, user);
311329
if (!entityKey) {
312330
pdpLogger.warn({ msg: 'User has no entity key for Virtru PDP evaluation, treating as non-compliant', userId: user._id });
313331
for (const room of rooms) {
314-
nonCompliant.push({ user, room: room as IRoom });
332+
nonCompliant.push({ user, room });
315333
}
316334
continue;
317335
}
@@ -341,17 +359,12 @@ export class VirtruPDP implements IPolicyDecisionPoint {
341359

342360
const responses = await this.getDecisionBulk(allRequests);
343361

344-
responses.forEach((resp, index) => {
345-
const permitted = resp?.resourceDecisions?.length && resp.resourceDecisions.every((rd) => rd.decision === 'DECISION_PERMIT');
346-
if (!permitted && requestIndex[index]) {
347-
nonCompliant.push({ user: requestIndex[index].user, room: requestIndex[index].room as IRoom });
348-
}
349-
});
362+
nonCompliant.push(...getDeniedSubjects(responses, requestIndex));
350363

351364
return nonCompliant;
352365
}
353366

354-
async reevaluateUsers(users: ReevaluationUser[]): Promise<Array<{ user: Pick<IUser, '_id' | 'emails' | 'username'>; room: IRoom }>> {
367+
async reevaluateUsers(users: ReevaluationUser[]): Promise<NonCompliantPair[]> {
355368
const roomIds = [...new Set(users.flatMap((u) => u.__rooms ?? []))];
356369
if (!roomIds.length) {
357370
return [];

‎ee/packages/abac/src/pdp/types.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ export interface IGetDecisionBulkResponse {
3030

3131
export type ReevaluationUser = Pick<IUser, '_id' | 'emails' | 'username' | '__rooms'>;
3232

33+
export type NonCompliantPair = {
34+
user: Pick<IUser, '_id' | 'emails' | 'username'>;
35+
room: AtLeast<IRoom, '_id' | 'abacAttributes'>;
36+
};
37+
3338
export interface IPolicyDecisionPoint {
3439
isAvailable(): Promise<boolean>;
3540

@@ -54,9 +59,9 @@ export interface IPolicyDecisionPoint {
5459
user: Pick<IUser, '_id' | 'emails' | 'username'>;
5560
rooms: AtLeast<IRoom, '_id' | 'abacAttributes'>[];
5661
}>,
57-
): Promise<Array<{ user: Pick<IUser, '_id' | 'emails' | 'username'>; room: IRoom }>>;
62+
): Promise<NonCompliantPair[]>;
5863

59-
reevaluateUsers(users: ReevaluationUser[]): Promise<void | Array<{ user: Pick<IUser, '_id' | 'emails' | 'username'>; room: IRoom }>>;
64+
reevaluateUsers(users: ReevaluationUser[]): Promise<void | NonCompliantPair[]>;
6065
}
6166

6267
export interface IVirtruPDPConfig {

0 commit comments

Comments
 (0)