Page MenuHomePhabricator
Differential D6775

Implement method to deserialize olm account from a file
ClosedPublic
Actions

Authored by marcin on Feb 20 2023, 4:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 4, 4:22 AM
Unknown Object (File)
Thu, Apr 3, 8:28 AM
Unknown Object (File)
Wed, Apr 2, 4:05 PM
Unknown Object (File)
Wed, Apr 2, 4:05 PM
Unknown Object (File)
Wed, Apr 2, 4:05 PM
Unknown Object (File)
Wed, Apr 2, 4:04 PM
Unknown Object (File)
Wed, Apr 2, 4:04 PM
Unknown Object (File)
Wed, Apr 2, 4:04 PM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
bartek
kamil
Commits
rCOMM382293dcee61: Implement method to deserialize olm account from a file
Summary

This differential implements a method in NotificationsCryptoModule that can deserialize flat file containing JSON-fied pickled olm account into in-memory CryptoModule onject that is capable to perform end2end encrypted messaging sessions.

Test Plan

Temporarily make this method public. Partially modify this method so that it does not read from a file but takes JSONI-ified pickled olm account directly and use it to create CryptoModule instance in CommCoreModule where we read pickled public olm account from the database. Use XCode debugger or logging to examine object
content. JSON-ified pickled olm account can be created in CommCoreModule.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Feb 20 2023, 4:18 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 20 2023, 4:18 AM
marcin added a parent revision: D6774: Start NotificationsCryptoModule API definition and implementation.Feb 20 2023, 4:19 AM
marcin removed a reviewer: atul.Feb 20 2023, 4:39 AM
marcin added a child revision: D6776: Implement method in NotificationsCryptoModule to serialize CryptoModule instance and atomically store it in a file..
marcin edited the test plan for this revision. (Show Details)
marcin published this revision for review.Feb 20 2023, 4:43 AM
Harbormaster failed remote builds in B16605: Diff 22711!Feb 20 2023, 7:29 AM
marcin updated this revision to Diff 22879.Feb 21 2023, 10:48 AM

Rebase

Harbormaster failed remote builds in B16724: Diff 22879!Feb 21 2023, 10:55 AM
Harbormaster completed remote builds in B16724: Diff 22879.Feb 21 2023, 11:03 AM
bartek added inline comments.Feb 22 2023, 12:25 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
26 ↗(On Diff #22879)

I think ifstream::in is the default, but can be left for clarity.

46 ↗(On Diff #22879)

What if JSON is malformed and account doesn't exist?

marcin added inline comments.Feb 22 2023, 2:30 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
46 ↗(On Diff #22879)
  1. The counterpart method to this one - serializeAndFlushCryptoModule - introduced in child differential ensures that either entire JSON (containing "account" key) is written or the method fails.
  2. Both this and serializeAndFlushCryptoModule are private and will be called in public methods only with olm notifications account path.

That said if serializeAndFlushCryptoModule fails on the very first write (first after last deletion of this file) to olm notifications account path, we will fail on the fist if in deserializeCryptoModule (the file does not exist). If it failed on n-th write, we will read correct content after last successful write to this file. So I think we can add this if statement and throw, but my understanding is that it is not necessary.

bartek accepted this revision.Feb 23 2023, 12:28 AM
bartek added inline comments.
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
46 ↗(On Diff #22879)

Yes, I understand that, my concern was the same as in D6777.
But you are right, if this file is only written by serializeAndFlushCryptoModule - this statement is unnecessary.

This revision is now accepted and ready to land.Feb 23 2023, 12:28 AM
marcin updated this revision to Diff 23253.Feb 28 2023, 10:06 AM

Rebase before landing

Harbormaster failed remote builds in B16999: Diff 23253!Feb 28 2023, 10:15 AM
Harbormaster completed remote builds in B16999: Diff 23253.Feb 28 2023, 1:44 PM
marcin updated this revision to Diff 23307.Mar 1 2023, 1:20 AM

Rebase before landing

Harbormaster completed remote builds in B17024: Diff 23307.Mar 1 2023, 1:35 AM
Closed by commit rCOMM382293dcee61: Implement method to deserialize olm account from a file (authored by marcin). · Explain WhyMar 1 2023, 2:04 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM382293dcee61: Implement method to deserialize olm account from a file.

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
Notifications/
BackgroundDataStorage/
64 lines
ios/
Comm.xcodeproj/
4 lines

Diff 23307

View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/ios/Comm.xcodeproj/project.pbxproj

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines