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

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 3 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
28 changes: 28 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,34 @@ 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();

expect(filteredSyncInfo.masterKeys_[0].content).toBeUndefined();
expect(filteredSyncInfo.masterKeys_[0].checksum).toBeUndefined();

expect(filteredSyncInfo.ppk_.value.publicKey).toBe('longstringverylongstringlongstringverylo');
expect(filteredSyncInfo.ppk_.value.privateKey.ciphertext).toBe('longstringverylongst...stringverylongstring');
laurent22 marked this conversation as resolved.
Show resolved Hide resolved
});
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) => {
if (mk.content) delete mk.content;
if (mk.checksum) delete mk.checksum;
Copy link
Owner

Choose a reason for hiding this comment

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

Why this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember some tests failing due to this, though they run fine now without it also. Not sure what caused the issue then.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok because I think with real data mk.checksum is an empty string so you wouldn't delete it in this case. And this check is unnecessary anyway

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