Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: MLSStateHandler updates clientEntities directly #16008

Merged
merged 6 commits into from
Oct 16, 2023
Merged

feat: MLSStateHandler updates clientEntities directly #16008

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6446b2a
feat: MLSStateHandler updates clientEntities directly
aweiss-dev Oct 16, 2023
ac7dac3
Update package.json
aweiss-dev Oct 16, 2023
afd9a56
fix: yarn.lock
aweiss-dev Oct 16, 2023
9ca69fa
fix: push identity only if it is verified
aweiss-dev Oct 16, 2023
3f9be0c
fix: add mlsConversation check
aweiss-dev Oct 16, 2023
4345b18
Merge branch 'dev' into feat/update_devices
aweiss-dev Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"@peculiar/x509": "1.9.5",
"@wireapp/avs": "9.4.18",
"@wireapp/commons": "5.2.1",
"@wireapp/core": "42.13.0",
"@wireapp/core": "^42.14.0",
"@wireapp/lru-cache": "3.8.1",
"@wireapp/react-ui-kit": "9.9.10",
"@wireapp/store-engine-dexie": "2.1.6",
Expand Down
38 changes: 34 additions & 4 deletions src/script/conversation/ConversationVerificationStateHandler/MLS/MLSStateHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import {ConversationProtocol} from '@wireapp/api-client/lib/conversation/NewConversation';

import {ClientEntity} from 'src/script/client';
import {Conversation} from 'src/script/entity/Conversation';
import {Core} from 'src/script/service/CoreSingleton';
import {createUuid} from 'Util/uuid';
Expand All @@ -36,12 +37,20 @@ describe('MLSConversationVerificationStateHandler', () => {
let mockOnConversationVerificationStateChange: jest.Mock;
let mockConversationState: jest.Mocked<ConversationState>;
let mockCore: jest.Mocked<Core>;
const conversation: Conversation = new Conversation(uuid, '', ConversationProtocol.MLS);
const groupId = 'groupIdXYZ';
const clientEntityId = 'clientIdXYZ';
const conversation: Conversation = new Conversation(uuid, '', ConversationProtocol.MLS);
const clientEntity: ClientEntity = new ClientEntity(false, '', clientEntityId);

beforeEach(() => {
conversation.groupId = groupId;

conversation.getAllUserEntities = jest.fn().mockReturnValue([
{
devices: () => [clientEntity],
},
]);

mockOnConversationVerificationStateChange = jest.fn();
mockConversationState = {
filteredConversations: () => [conversation],
Expand All @@ -51,7 +60,14 @@ describe('MLSConversationVerificationStateHandler', () => {
mls: {
on: jest.fn(),
},
e2eIdentity: {},
e2eIdentity: {
getUserDeviceEntities: jest.fn().mockResolvedValue([
{
certificate: 'mockCertificate',
clientId: clientEntityId,
},
]),
},
},
} as unknown as jest.Mocked<Core>;

Expand Down Expand Up @@ -110,7 +126,7 @@ describe('MLSConversationVerificationStateHandler', () => {
epoch: 12345,
};

jest.spyOn(handler as any, 'getAllUserEntitiesInConversation').mockResolvedValue({
jest.spyOn(handler as any, 'updateUserDevices').mockResolvedValue({
isResultComplete: false,
identities: [],
qualifiedIds: [],
Expand All @@ -129,7 +145,7 @@ describe('MLSConversationVerificationStateHandler', () => {
epoch: 12345,
};

jest.spyOn(handler as any, 'getAllUserEntitiesInConversation').mockResolvedValue({
jest.spyOn(handler as any, 'updateUserDevices').mockResolvedValue({
isResultComplete: true,
identities: [
{
Expand All @@ -145,5 +161,19 @@ describe('MLSConversationVerificationStateHandler', () => {

expect((handler as any).verifyConversation).toHaveBeenCalled();
});

it('should update ClientEntity isMLSVerified observable', async () => {
const mockData = {
groupId,
epoch: 12345,
};

jest.spyOn(handler as any, 'isCertificateActiveAndValid').mockReturnValue(true);
jest.spyOn(handler as any, 'verifyConversation').mockImplementation(() => null);

await (handler as any).checkEpoch(mockData); // Calling the private method

expect(clientEntity.meta.isMLSVerified?.()).toBe(true);
});
});
});
57 changes: 34 additions & 23 deletions src/script/conversation/ConversationVerificationStateHandler/MLS/MLSStateHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@

import {X509Certificate} from '@peculiar/x509';
import {QualifiedId} from '@wireapp/api-client/lib/user';
import {WireIdentity} from '@wireapp/core/lib/messagingProtocols/mls';
import {container} from 'tsyringe';

import {ClientEntity} from 'src/script/client';
import {Conversation} from 'src/script/entity/Conversation';
import {VerificationMessageType} from 'src/script/message/VerificationMessageType';
import {Core} from 'src/script/service/CoreSingleton';
Expand Down Expand Up @@ -95,31 +97,41 @@ export class MLSConversationVerificationStateHandler {
* This function returns the WireIdentity of all userDeviceEntities in a conversation, as long as they have a certificate.
* If the conversation has userDeviceEntities without a certificate, it will not be included in the returned array
*
* It also updates the isMLSVerified observable of all the devices in the conversation
*/
private getAllUserEntitiesInConversation = async (conversation: Conversation) => {
if (!conversation.groupId) {
this.logger.error('Conversation has no groupId', conversation.name);
throw new Error('Conversation has no groupId');
}

private updateUserDevices = async (conversation: Conversation) => {
const userEntities = conversation.getAllUserEntities();

const deviceUserPairs = userEntities
.flatMap(userEntity => {
return userEntity.devices().map(device => ({[device.id]: userEntity.qualifiedId}));
})
.reduce((acc, current) => {
return {...acc, ...current};
}, {});

const identities = await this.core.service!.e2eIdentity!.getUserDeviceEntities(
conversation.groupId,
deviceUserPairs,
);
const allClients: ClientEntity[] = [];
const allIdentities: WireIdentity[] = [];
userEntities.forEach(async userEntity => {
const devices = userEntity.devices();
const deviceUserPairs = devices
.map(device => ({
[device.id]: userEntity.qualifiedId,
}))
.reduce((acc, current) => {
return {...acc, ...current};
}, {});
const identities = await this.core.service!.e2eIdentity!.getUserDeviceEntities(
conversation.groupId!,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a isMLSConversation that will also make sure that this groupId is present ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have a look at the ConversationSelector.ts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. Changed the types and added the guard

deviceUserPairs,
);
identities.forEach(identity => {
if (identity.certificate) {
const device = devices.find(device => device.id === identity.clientId);
/**
* ToDo: Change the current implementation of isMLSVerified to be stored in Zustand instead of ko.observable
*/
device?.meta.isMLSVerified?.(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This meta is saved to the DB, right?

Copy link
Member Author

@aweiss-dev aweiss-dev Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should. I added all the entries for that.
The DB entries get initialized with it, and it is also added in the devices toJSON method.

}
});
allIdentities.push(...identities);
allClients.push(...devices);
});

return {
identities,
isResultComplete: Object.keys(deviceUserPairs).length === identities.length,
identities: allIdentities,
isResultComplete: allClients.length === allIdentities.length,
qualifiedIds: userEntities.map(userEntity => userEntity.qualifiedId),
};
};
Expand All @@ -140,8 +152,7 @@ export class MLSConversationVerificationStateHandler {
return;
}

const {isResultComplete, identities, qualifiedIds} =
await this.getAllUserEntitiesInConversation(conversationEntity);
const {isResultComplete, identities, qualifiedIds} = await this.updateUserDevices(conversationEntity);

// If the number of userDevicePairs is not equal to the number of identities, our Conversation is not secure
if (!isResultComplete) {
Expand Down
20 changes: 10 additions & 10 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5401,9 +5401,9 @@ __metadata:
languageName: node
linkType: hard

"@wireapp/api-client@npm:^26.3.1":
version: 26.3.1
resolution: "@wireapp/api-client@npm:26.3.1"
"@wireapp/api-client@npm:^26.3.2":
version: 26.3.2
resolution: "@wireapp/api-client@npm:26.3.2"
dependencies:
"@wireapp/commons": ^5.2.1
"@wireapp/priority-queue": ^2.1.4
Expand All @@ -5418,7 +5418,7 @@ __metadata:
tough-cookie: 4.1.3
ws: 8.14.2
zod: 3.22.4
checksum: 33783d4b7e48cb66121be0ef8c5ace81fdc4f698af1c47ed5b91c27896a30724fecefc34ce66b27620ecb2e54a556782904b9d42a1f64c9592cf594f8c3f100e
checksum: 311225a5927062c59becbd10a89e10d1bb04fcf12d862d1ad7e8c5fc171d8d5cb5e7139212a959af771f044812bd30873068684f92a15e4e9e1504fd67f32135
languageName: node
linkType: hard

Expand Down Expand Up @@ -5471,11 +5471,11 @@ __metadata:
languageName: node
linkType: hard

"@wireapp/core@npm:42.13.0":
version: 42.13.0
resolution: "@wireapp/core@npm:42.13.0"
"@wireapp/core@npm:^42.14.0":
version: 42.14.0
resolution: "@wireapp/core@npm:42.14.0"
dependencies:
"@wireapp/api-client": ^26.3.1
"@wireapp/api-client": ^26.3.2
"@wireapp/commons": ^5.2.1
"@wireapp/core-crypto": 1.0.0-rc.13
"@wireapp/cryptobox": 12.8.0
Expand All @@ -5493,7 +5493,7 @@ __metadata:
long: ^5.2.0
uuidjs: 4.2.13
zod: 3.22.4
checksum: 8874e5f14b022adf6896598b7d470a81e5699448e4c9bf2f36666e4b1701062a2cb67b1f99edf6dca336d1bb99dd639a80091bb9784703d7928dfdc18ed68407
checksum: 8019a35e62c859f20c309066c9386b02cea5481bc78080c9aadad715a6739397ab23275e657b742655bb66e964dca6ad134b5992131c6d58a4db2bcae0f11c7e
languageName: node
linkType: hard

Expand Down Expand Up @@ -18571,7 +18571,7 @@ __metadata:
"@wireapp/avs": 9.4.18
"@wireapp/commons": 5.2.1
"@wireapp/copy-config": 2.1.9
"@wireapp/core": 42.13.0
"@wireapp/core": ^42.14.0
"@wireapp/eslint-config": 3.0.4
"@wireapp/lru-cache": 3.8.1
"@wireapp/prettier-config": 0.6.3
Expand Down