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

Jump to bottom

Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

Merged
merged 7 commits into from
Mar 2, 2024
Merged

Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
054b988
Desktop: Fixes #9985: Filter Sync Target Info Logs
criticic Feb 27, 2024
b319fdd
Only Filter if values are present
criticic Feb 27, 2024
0e79f4f
Disable Spellcheck for test
criticic Feb 27, 2024
441b85e
Compare with the complete object
criticic Feb 27, 2024
612b705
Remove unnecessary check
criticic Feb 27, 2024
320a8cc
Update publicKey truncation & change test to use static data
criticic Feb 27, 2024
32d6dfa
Make the filter consistent
criticic Feb 27, 2024
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
5 changes: 2 additions & 3 deletions packages/lib/Synchronizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ export default class Synchronizer {

try {
let remoteInfo = await fetchSyncInfo(this.api());
logger.info('Sync target remote info:', remoteInfo);
logger.info('Sync target remote info:', remoteInfo.filterSyncInfo());
eventManager.emit(EventName.SessionEstablished);

let syncTargetIsNew = false;
Expand All @@ -471,8 +471,7 @@ export default class Synchronizer {
if (appVersion !== 'unknown') checkIfCanSync(remoteInfo, appVersion);

let localInfo = await localSyncInfo();

logger.info('Sync target local info:', localInfo);
logger.info('Sync target local info:', localInfo.filterSyncInfo());

localInfo = await this.setPpkIfNotExist(localInfo, remoteInfo);

Expand Down
49 changes: 49 additions & 0 deletions packages/lib/services/synchronizer/syncInfoUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,55 @@ describe('syncInfoUtils', () => {
logger.enabled = true;
});

// cSpell:disable
it('should filter unnecessary sync info', async () => {
const syncInfo = new SyncInfo();
syncInfo.masterKeys = [{
id: '1',
content: 'longstringverylongstringlongstringverylongstringlongstringverylongstring',
checksum: 'longstringverylongstringlongstringverylongstringlongstringverylongstring',
}];
syncInfo.ppk = {
id: '1',
publicKey: 'longstringverylongstringlongstringverylongstringlongstringverylongstring',
privateKey: {
encryptionMethod: 1,
ciphertext: 'longstringverylongstringlongstringverylongstringlongstringverylongstring',
},
createdTime: 0,
keySize: 0,
Copy link
Owner

Choose a reason for hiding this comment

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

Could you try the app and use more realistic data? Checking that the time or size is 0 is not a good test - it might be 0 because of a bug.

};
const filteredSyncInfo = syncInfo.filterSyncInfo();
const originalSyncInfo = syncInfo.toObject();

expect(filteredSyncInfo).toEqual({
activeMasterKeyId_: originalSyncInfo.activeMasterKeyId,
appMinVersion_: originalSyncInfo.appMinVersion,
e2ee_: originalSyncInfo.e2ee,
version_: originalSyncInfo.version,
Copy link
Owner

Choose a reason for hiding this comment

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

When you create a test you should not use dynamic data, especially not data that comes from the data you're supposed to check. So all these should be static strings/numbers.

masterKeys_: [
// Content & Checksum are removed
{ id: '1' },
],
ppk_: {
value: {
// Public Key is truncated to 40 characters
publicKey: 'longstringverylongstringlongstringverylo',
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind adding "..." to this string too to show it's truncated?

Copy link
Owner

Choose a reason for hiding this comment

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

And please remove all the comments. They are not necessary and are likely to become outdated.

privateKey: {
// Private Key is truncated to first and last 20 characters
ciphertext: 'longstringverylongst...stringverylongstring',
encryptionMethod: 1,
},
id: '1',
createdTime: 0,
keySize: 0,
},
updatedTime: originalSyncInfo.ppk.updatedTime,
Copy link
Owner

Choose a reason for hiding this comment

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

Same here

},
});
});
laurent22 marked this conversation as resolved.
Show resolved Hide resolved
// cSpell:enable

test.each([
['1.0.0', '1.0.4', true],
['1.0.0', '0.0.5', false],
Expand Down
20 changes: 20 additions & 0 deletions packages/lib/services/synchronizer/syncInfoUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,26 @@ export class SyncInfo {
};
}

public filterSyncInfo() {
const filtered = JSON.parse(JSON.stringify(this));

// Filter content and checksum properties from master keys
if (filtered.masterKeys_) {
filtered.masterKeys_ = filtered.masterKeys_.map((mk: MasterKeyEntity) => {
delete mk.content;
delete mk.checksum;
return mk;
});
}

// Truncate the private key and public key
if (filtered.ppk_.value) {
filtered.ppk_.value.privateKey.ciphertext = `${filtered.ppk_.value.privateKey.ciphertext.substr(0, 20)}...${filtered.ppk_.value.privateKey.ciphertext.substr(-20)}`;
filtered.ppk_.value.publicKey = filtered.ppk_.value.publicKey.substr(0, 40);
}
return filtered;
}

public serialize(): string {
return JSON.stringify(this.toObject(), null, '\t');
}
Expand Down
Loading