Page MenuHomePhabricator

[services] Tunnelbroker - Add payload to APNs message to make it `content-available`
ClosedPublic

Authored by max on Sep 8 2022, 7:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 13, 5:23 PM
Unknown Object (File)
Mon, May 13, 2:58 PM
Unknown Object (File)
Mon, May 13, 2:58 PM
Unknown Object (File)
Thu, May 9, 5:23 PM
Unknown Object (File)
Thu, May 9, 5:23 PM
Unknown Object (File)
Thu, May 9, 5:23 PM
Unknown Object (File)
Thu, May 2, 3:36 PM
Unknown Object (File)
Thu, May 2, 3:36 PM

Details

Summary

This diff introduces changes to the Rust notification library to add an optional APNs push message payload.

To wake up the iOS app from the background we should send a message with the payload. In this case, a message will be with the content-available attribute, so we can do background information processing in the app.
Apple's related documentation, apple forum thread.

The message payload is optional in the library.

Related Linear task: ENG-1743

Test Plan
  1. Successfully built app without errors.
  2. Device testing:

To test the APNs notification physical iOS device is required.

  • Isolated testing can be done using the apns-ios-push-tester XCode project. To test it clone the apns-ios-push-tester and follow the instruction to set the certificate, run the app on the physical iOS device and get the device token.
  • Comm app testing can be done using the apns certificate provided in 1Password.

Send the apns message using the following library call from C++:

#include "cxxbridge_code/src/lib.rs.h"
#include "rust/cxx.h
...
const std::string cert_path = argv[1];
const std::string cert_pass = argv[2];
const std::string topic = argv[3];
const std::string device_token = argv[4];
const std::string message_text = argv[5];
const std::string message_payload = argv[6];
const bool sandbox = true; // True if your certificate is for development.
sendNotifToAPNS(cert_path, cert_pass, topic, device_token, message_text, message_payload, sandbox);

The expected result is push message delivery to the device if the credentials are correct. Errors if some of the credentials are wrong.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.
max published this revision for review.Sep 8 2022, 7:50 AM
max edited the summary of this revision. (Show Details)
max edited the test plan for this revision. (Show Details)
max added reviewers: karol, tomek.
max added inline comments.
services/tunnelbroker/rust-notifications/src/lib.rs
51–55 ↗(On Diff #16502)

CXX doesn't support Option<T>, that's why we are checking for whether the payload is empty or not and returning the Option here.

Send the apns message using the following library call from C++:

We should really get some CI around this, it's a lot of construction and the workflow seems highly aligned to being automated.

tomek requested changes to this revision.Sep 12 2022, 6:22 AM

This diff is confusing. You're mentioning that

To wake up the iOS app from the background we should send a message with the payload. In this case, a message will be with the content-available attribute, so we can do background information processing in the app.

But this diff doesn't set this flag anywhere.

services/tunnelbroker/rust-notifications/src/apns.rs
31–36 ↗(On Diff #16502)

When we have a match with only single meaningful arm, it is easier to use if let instead

33 ↗(On Diff #16502)

I don't think that adding a new generic field payload_data is a good idea. We should try to use the same message shape as we're using currently. Please check prepareIOSNotification function in comm/keyserver/src/push/send.js file.

This revision now requires changes to proceed.Sep 12 2022, 6:22 AM

Changes to change a content-available flag itself depsite adding of the payload_data.

In D5083#149217, @tomek wrote:

This diff is confusing. You're mentioning that

To wake up the iOS app from the background we should send a message with the payload. In this case, a message will be with the content-available attribute, so we can do background information processing in the app.

But this diff doesn't set this flag anywhere.

Now it is )

services/tunnelbroker/rust-notifications/src/apns.rs
31–36 ↗(On Diff #16502)

When we have a match with only single meaningful arm, it is easier to use if let instead

Good catch, but this part of the code was removed. Thanks anyway ;)

33 ↗(On Diff #16502)

I don't think that adding a new generic field payload_data is a good idea. We should try to use the same message shape as we're using currently. Please check prepareIOSNotification function in comm/keyserver/src/push/send.js file.

That makes sense. I've changed the code to change the content-available flag itself instead of adding an additional payload data. Btw in comm/keyserver/src/push/send.js seems we don't change content-available anywhere. Searching within the entire Comm repo didn't find any occasions except simulator.

max added inline comments.
services/tunnelbroker/rust-notifications/src/apns.rs
30 ↗(On Diff #16995)

Now, we are just changing a flag here.

Nice!

services/tunnelbroker/rust-notifications/src/apns.rs
30 ↗(On Diff #16995)

Is there a way to set this flag using builder?

33 ↗(On Diff #16502)

That's because on js side this flag is named contentAvailable

This revision is now accepted and ready to land.Sep 22 2022, 7:07 AM
max added inline comments.
services/tunnelbroker/rust-notifications/src/apns.rs
30 ↗(On Diff #16995)

Is there a way to set this flag using builder?

Unfortunately no, there is no such option.

max marked an inline comment as done.

Rebasing.

Fix merging with the latest changes.