Details

Reviewers

bartek

Commits

rCOMMfa56357e2fff: Implement method to query identity service from the NSE

Summary

This differential enables the NSE to call idenity service to query for device identity keys

Test Plan

Apply patch from D12673 test plan.
Apply this patch: https://gist.github.com/marcinwasowicz/2ff63cc0989b19990cbc8924113891c2
Set NSE debugger to stop at the line that checks for error from identity request
Send notification from web client
Examine content of the response from identity service

Diff Detail

Repository

rCOMM Comm

Branch

marcin/eng-8237

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Jul 5 2024, 11:24 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJul 5 2024, 11:24 AM

marcin added a parent revision: D12676: Migrate notifications crypto account to MMKV on native.Jul 5 2024, 11:27 AM

marcin edited the test plan for this revision. (Show Details)Jul 5 2024, 11:29 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 5 2024, 11:36 AM

Harbormaster failed remote builds in B30172: Diff 42083!

Harbormaster completed remote builds in B30172: Diff 42083.Jul 8 2024, 2:31 AM

marcin requested review of this revision.Jul 8 2024, 2:31 AM

marcin added a child revision: D12692: Migrate notification account to IndexedDB on web.Jul 8 2024, 6:52 AM

ashoat added inline comments.Jul 8 2024, 12:27 PM

native/ios/Comm/CommIOSServices/CommIOSServicesClient.mm
7–15	I don't love that these are configured separately from the rest of the app. We went through a lot of effort to make sure that the Rust code in `native_rust_library` respects the same configs as the rest of the JS app (eg. `native/facts/identity_service_config.json`)

marcin removed a reviewer: tomek. marcin added 1 blocking reviewer(s): bartek.Jul 15 2024, 8:09 AM

ashoat added inline comments.Jul 15 2024, 8:45 AM

native/ios/Comm/CommIOSServices/CommIOSServicesClient.mm
14	Missing semicolon here

The HTTP request-response logic looks good; it's analogous to the existing Blob GET request logic.

Accepting but please remember to fix that semicolon. Otherwise, release builds will fail.

native/ios/Comm/CommIOSServices/CommIOSServicesClient.mm
7–15	Good point. The proper way of doing this would be to create a script that reads the `native/facts/identity_service_config.json` file (and others similiar) and generates a header file named e.g. `CommServices.h` that is `#import`ed here. The script should be run as a Xcode build phase (similar to what we do to build native-rust-lib). This is out of this diff's scope but we should create a follow-up task.

This revision is now accepted and ready to land.Jul 16 2024, 4:41 AM

marcin added a child revision: D12836: Patch localforage to support transactional multiple items retrieval/persistence.Jul 22 2024, 7:13 AM

marcin removed a child revision: D12692: Migrate notification account to IndexedDB on web.

In D12677#361397, @bartek wrote:

The HTTP request-response logic looks good; it's analogous to the existing Blob GET request logic.

Accepting but please remember to fix that semicolon. Otherwise, release builds will fail.

Created follow up task to track

Address review

Harbormaster failed remote builds in B30636: Diff 42732!Jul 24 2024, 6:21 AM

Harbormaster completed remote builds in B30636: Diff 42732.Jul 24 2024, 9:13 AM

Update phab

Harbormaster completed remote builds in B30796: Diff 42941.Jul 30 2024, 4:37 AM

Rebase before landing

Harbormaster completed remote builds in B30898: Diff 43078.Aug 5 2024, 5:30 AM

Closed by commit rCOMMfa56357e2fff: Implement method to query identity service from the NSE (authored by marcin, committed by Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local>). · Explain WhyAug 5 2024, 6:39 AM

This revision was automatically updated to reflect the committed changes.

Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local> added a commit: rCOMMfa56357e2fff: Implement method to query identity service from the NSE.