Skip to content

Commit 50ea823

Browse files
KevLehmanKevLehman
authored
chore: Minor abac fixes (#41004)
1 parent 6ae500a commit 50ea823

6 files changed

Lines changed: 69 additions & 32 deletions

File tree

‎apps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,30 @@
11
import { Abac } from '@rocket.chat/core-services';
2+
import type { IRoom, IRoomAbacRedaction } from '@rocket.chat/core-typings';
23
import { License } from '@rocket.chat/license';
34
import { Users } from '@rocket.chat/models';
45

56
import { scopeAdminRoomsForAbac } from '../../../../app/api/server/lib/scopeAdminRoomsForAbac';
67
import { isABACManagedRoom } from '../../../../app/authorization/server/lib/isABACManagedRoom';
78

9+
const redact = <T extends Pick<IRoom, 't' | 'abacAttributes'>>(rooms: T[]): Array<T & IRoomAbacRedaction> =>
10+
rooms.map((room) => (isABACManagedRoom(room) ? { ...room, abacAttributes: [], abacAttributesRedacted: true } : room));
11+
812
scopeAdminRoomsForAbac.patch(async (next, rooms, uid) => {
913
const managed = License.hasModule('abac') ? rooms.filter(isABACManagedRoom) : [];
1014
if (!managed.length) {
1115
return next(rooms, uid);
1216
}
1317

14-
const user = await Users.findOneById(uid, { projection: { _id: 1, username: 1, name: 1 } });
15-
if (!user) {
16-
return rooms.map((room) => (isABACManagedRoom(room) ? { ...room, abacAttributes: [], abacAttributesRedacted: true } : room));
17-
}
18+
try {
19+
const user = await Users.findOneById(uid, { projection: { _id: 1, username: 1, name: 1 } });
20+
if (!user) {
21+
return redact(rooms);
22+
}
1823

19-
const scoped = await Abac.scopeRoomsForAdmin(managed, user);
20-
const scopedById = new Map(scoped.map((room) => [room._id, room]));
21-
return rooms.map((room) => scopedById.get(room._id) ?? room);
24+
const scoped = await Abac.scopeRoomsForAdmin(managed, user);
25+
const scopedById = new Map(scoped.map((room) => [room._id, room]));
26+
return rooms.map((room) => scopedById.get(room._id) ?? room);
27+
} catch (err) {
28+
return redact(rooms);
29+
}
2230
});

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

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3154,7 +3154,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
31543154
before(async function () {
31553155
this.timeout(15000);
31563156

3157-
user = await createUser();
3157+
user = await createUser({ verified: true });
31583158
userCreds = await login(user.username, password);
31593159

31603160
room = (await createRoom({ type: 'p', name: `extpdp-permit-${Date.now()}` })).body.group;
@@ -3219,7 +3219,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
32193219
before(async function () {
32203220
this.timeout(15000);
32213221

3222-
user = await createUser();
3222+
user = await createUser({ verified: true });
32233223
userCreds = await login(user.username, password);
32243224

32253225
room = (await createRoom({ type: 'p', name: `extpdp-access-${Date.now()}` })).body.group;
@@ -3284,8 +3284,8 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
32843284
before(async function () {
32853285
this.timeout(10000);
32863286

3287-
permitUser = await createUser();
3288-
denyUser = await createUser();
3287+
permitUser = await createUser({ verified: true });
3288+
denyUser = await createUser({ verified: true });
32893289

32903290
room = (await createRoom({ type: 'p', name: `extpdp-invite-${Date.now()}` })).body.group;
32913291
await mockServerReset();
@@ -3367,7 +3367,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
33673367
before(async function () {
33683368
this.timeout(10000);
33693369

3370-
user = await createUser();
3370+
user = await createUser({ verified: true });
33713371
userCredentials = await login(user.username, password);
33723372

33733373
room = (await createRoom({ type: 'p', name: `extpdp-failclose-${Date.now()}` })).body.group;
@@ -3415,7 +3415,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
34153415
await mockServerReset();
34163416
await mockServerSet('GET', '/healthz', { status: 'NOT_SERVING' });
34173417

3418-
const newUser = await createUser();
3418+
const newUser = await createUser({ verified: true });
34193419

34203420
await request
34213421
.post('/api/v1/groups.invite')
@@ -3452,7 +3452,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
34523452
before(async function () {
34533453
this.timeout(15000);
34543454

3455-
user = await createUser();
3455+
user = await createUser({ verified: true });
34563456
room = (await createRoom({ type: 'p', name: `extpdp-selective-${Date.now()}` })).body.group;
34573457
await request
34583458
.post('/api/v1/groups.invite')
@@ -3495,7 +3495,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
34953495
before(async function () {
34963496
this.timeout(15000);
34973497

3498-
user = await createUser();
3498+
user = await createUser({ verified: true });
34993499
room = (await createRoom({ type: 'p', name: `extpdp-tighten-${Date.now()}` })).body.group;
35003500
await request
35013501
.post('/api/v1/groups.invite')
@@ -3555,7 +3555,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
35553555
before(async function () {
35563556
this.timeout(15000);
35573557

3558-
user = await createUser({ username, email });
3558+
user = await createUser({ username, email, verified: true });
35593559
room = (await createRoom({ type: 'p', name: `extpdp-sync-deny-${Date.now()}` })).body.group;
35603560
await request
35613561
.post(api('groups.invite'))
@@ -3616,7 +3616,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
36163616
before(async function () {
36173617
this.timeout(15000);
36183618

3619-
user = await createUser({ username, email });
3619+
user = await createUser({ username, email, verified: true });
36203620
room = (await createRoom({ type: 'p', name: `extpdp-sync-permit-${Date.now()}` })).body.group;
36213621
await request
36223622
.post(api('groups.invite'))
@@ -3723,7 +3723,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
37233723
let userWithAttrs: IUser;
37243724

37253725
before(async () => {
3726-
userWithAttrs = await createUser();
3726+
userWithAttrs = await createUser({ verified: true });
37273727
await addAbacAttributesToUserDirectly(userWithAttrs._id, [{ key: attrKey, values: ['alpha'] }]);
37283728
});
37293729

@@ -3749,7 +3749,11 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
37493749
let storeConnection: MongoClient;
37503750

37513751
const makeAdmin = async (slug: string) => {
3752-
const u = await createUser({ roles: ['admin'], username: `vstore-${slug}-${Date.now()}-${Math.random().toString(36).slice(2, 7)}` });
3752+
const u = await createUser({
3753+
roles: ['admin'],
3754+
username: `vstore-${slug}-${Date.now()}-${Math.random().toString(36).slice(2, 7)}`,
3755+
verified: true,
3756+
});
37533757
const creds = await login(u.username, password);
37543758
return { user: u, creds };
37553759
};
@@ -4272,7 +4276,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
42724276
wipeRoom1 = (await createRoom({ type: 'p', name: `vstore-wipe-1-${Date.now()}` })).body.group;
42734277
wipeRoom2 = (await createRoom({ type: 'p', name: `vstore-wipe-2-${Date.now()}` })).body.group;
42744278

4275-
memberUser = await createUser();
4279+
memberUser = await createUser({ verified: true });
42764280
await request
42774281
.post(`${v1}/groups.invite`)
42784282
.set(credentials)

‎ee/packages/abac/src/clients/virtru/identity.spec.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,23 @@ describe('virtru/identity', () => {
1414
expect(getUserEntityKey('emailAddress', { _id: 'u', username: 'bob' })).toBeUndefined();
1515
});
1616

17+
it('getUserEntityKey never resolves to an unverified email address', () => {
18+
expect(
19+
getUserEntityKey('emailAddress', { _id: 'u', emails: [{ address: 'ceo@company.com', verified: false }], username: 'attacker' }),
20+
).toBeUndefined();
21+
expect(getUserEntityKey('emailAddress', { _id: 'u', emails: [{ address: 'ceo@company.com' }], username: 'attacker' })).toBeUndefined();
22+
expect(
23+
getUserEntityKey('emailAddress', {
24+
_id: 'u',
25+
emails: [
26+
{ address: 'ceo@company.com', verified: false },
27+
{ address: 'real@attacker.com', verified: true },
28+
],
29+
username: 'attacker',
30+
}),
31+
).toBe('real@attacker.com');
32+
});
33+
1734
it('buildAttributeFqns round-trips with parseAttributeFqns', () => {
1835
const attrs = [{ key: 'clearance', values: ['secret', 'topsecret'] }];
1936
const fqns = buildAttributeFqns('example.com', attrs);

‎ee/packages/abac/src/clients/virtru/identity.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ export function buildEntityIdentifier(defaultEntityKey: EntityKeyType, entityKey
1414
export function getUserEntityKey(defaultEntityKey: EntityKeyType, user: Pick<IUser, '_id' | 'emails' | 'username'>): string | undefined {
1515
switch (defaultEntityKey) {
1616
case 'emailAddress':
17-
return user.emails?.[0]?.address;
17+
return user.emails?.find((email) => email.verified)?.address;
1818
case 'oidcIdentifier':
1919
return user.username;
2020
}

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

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,12 @@ const asyncIterable = <T>(items: T[]) => ({
5050
},
5151
});
5252

53-
const user = (over: Partial<{ _id: string; username: string; emails: { address: string }[]; __rooms: string[] }> = {}) => ({
53+
const user = (
54+
over: Partial<{ _id: string; username: string; emails: { address: string; verified?: boolean }[]; __rooms: string[] }> = {},
55+
) => ({
5456
_id: 'u1',
5557
username: 'bob',
56-
emails: [{ address: 'bob@x.com' }],
58+
emails: [{ address: 'bob@x.com', verified: true }],
5759
...over,
5860
});
5961

@@ -236,7 +238,9 @@ describe('VirtruPDP.checkUsernamesMatchAttributes', () => {
236238
});
237239

238240
it('resolves when all users get DECISION_PERMIT', async () => {
239-
usersFindByUsernames.mockReturnValue(cursor([user(), user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com' }] })]));
241+
usersFindByUsernames.mockReturnValue(
242+
cursor([user(), user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com', verified: true }] })]),
243+
);
240244
const apiCall = jest.fn().mockResolvedValue({
241245
decisionResponses: [
242246
{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_PERMIT' }] },
@@ -248,7 +252,9 @@ describe('VirtruPDP.checkUsernamesMatchAttributes', () => {
248252
});
249253

250254
it('throws when any user is DENIED', async () => {
251-
usersFindByUsernames.mockReturnValue(cursor([user(), user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com' }] })]));
255+
usersFindByUsernames.mockReturnValue(
256+
cursor([user(), user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com', verified: true }] })]),
257+
);
252258
const apiCall = jest.fn().mockResolvedValue({
253259
decisionResponses: [
254260
{ resourceDecisions: [{ ephemeralResourceId: 'r1', decision: 'DECISION_PERMIT' }] },
@@ -301,7 +307,7 @@ describe('VirtruPDP.onRoomAttributesChanged', () => {
301307

302308
it('returns users that are DENIED', async () => {
303309
const u1 = user();
304-
const u2 = user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com' }] });
310+
const u2 = user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com', verified: true }] });
305311
usersFindActiveByRoomIds.mockReturnValue(asyncIterable([u1, u2]));
306312
const apiCall = jest.fn().mockResolvedValue({
307313
decisionResponses: [
@@ -335,8 +341,8 @@ describe('VirtruPDP.onRoomAttributesChanged', () => {
335341

336342
it('still evicts entity-keyless users alongside DENY-only filtering of decided users', async () => {
337343
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' }] });
344+
const denied = user({ _id: 'u2', username: 'alice', emails: [{ address: 'a@x.com', verified: true }] });
345+
const unspecified = user({ _id: 'u3', username: 'carol', emails: [{ address: 'c@x.com', verified: true }] });
340346
usersFindActiveByRoomIds.mockReturnValue(asyncIterable([keyless, denied, unspecified]));
341347
const apiCall = jest.fn().mockResolvedValue({
342348
decisionResponses: [

‎ee/packages/abac/src/store/VirtruAttributeStore.spec.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ const mkClient = (over: Partial<Record<'isAvailable' | 'apiCall' | 'getConfig',
2020
getConfig: over.getConfig ?? jest.fn().mockReturnValue(cfg),
2121
}) as any;
2222

23-
beforeEach(() => usersFindOneById.mockReset().mockResolvedValue({ _id: 'u1', emails: [{ address: 'bob@x.com' }], username: 'bob' }));
23+
beforeEach(() =>
24+
usersFindOneById.mockReset().mockResolvedValue({ _id: 'u1', emails: [{ address: 'bob@x.com', verified: true }], username: 'bob' }),
25+
);
2426

2527
describe('VirtruAttributeStore.entitlementsOf / list', () => {
2628
it('parses FQN map keys into grouped {key,values}', async () => {
@@ -87,7 +89,7 @@ describe('VirtruAttributeStore.entitlementsOf / list', () => {
8789
const apiCall = jest.fn().mockResolvedValue({ entitlements: [{ actionsPerAttributeValueFqn: {} }] });
8890
const store = new VirtruAttributeStore(mkClient({ apiCall }));
8991
await store.entitlementsOf(actor);
90-
usersFindOneById.mockResolvedValue({ _id: 'u2', emails: [{ address: 'alice@x.com' }], username: 'alice' });
92+
usersFindOneById.mockResolvedValue({ _id: 'u2', emails: [{ address: 'alice@x.com', verified: true }], username: 'alice' });
9193
await store.entitlementsOf({ _id: 'u2', username: 'alice', name: 'A' });
9294
expect(apiCall).toHaveBeenCalledTimes(2);
9395
});
@@ -172,11 +174,11 @@ describe('VirtruAttributeStore.entitlementsOf / list', () => {
172174
expect(apiCall).toHaveBeenCalledTimes(1);
173175

174176
apiCall.mockRejectedValueOnce(new Error('pdp blip'));
175-
usersFindOneById.mockResolvedValue({ _id: 'u2', emails: [{ address: 'alice@x.com' }], username: 'alice' });
177+
usersFindOneById.mockResolvedValue({ _id: 'u2', emails: [{ address: 'alice@x.com', verified: true }], username: 'alice' });
176178
await expect(store.entitlementsOf({ _id: 'u2', username: 'alice', name: 'A' })).rejects.toThrow('pdp blip');
177179
expect(apiCall).toHaveBeenCalledTimes(2);
178180

179-
usersFindOneById.mockResolvedValue({ _id: 'u1', emails: [{ address: 'bob@x.com' }], username: 'bob' });
181+
usersFindOneById.mockResolvedValue({ _id: 'u1', emails: [{ address: 'bob@x.com', verified: true }], username: 'bob' });
180182
const result = await store.entitlementsOf(actor);
181183
expect(result.get('clearance')).toEqual(new Set(['secret']));
182184
expect(apiCall).toHaveBeenCalledTimes(2);

0 commit comments

Comments
 (0)