Paths

Table of Contentst

[services] Tunnelbroker - Add `topic` to APNs
ClosedPublic
Actions

Authored by • max on Sep 8 2022, 3:25 AM.

Details

Reviewers

• jon
• karol
tomek

Commits

rCOMMd2aa8f6807a0: [services] Tunnelbroker - Add `topic` to APNs

Summary

This diff introduces changes to the A2 library wrapper to use topic for the APNs message (adding of topic argument to the wrapper function.).

When testing push notifications with the production certificate the topic for the message is mandatory. The topic is an iOS app bundle name. The topic is mandatory because some P12 certificates can have multiple app bundle names (one certificate for a few apps).

Related Linear task: ENG-1743

Test Plan

Successfully built app without errors.
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_body = argv[5];
const bool sandbox = true; // True if your certificate is for development.
sendNotifToAPNS(cert_path, cert_pass, topic, device_token, message_body, 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 created this revision.Sep 8 2022, 3:25 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 8 2022, 3:25 AM

Herald added subscribers: • abosh, • karol, atul and 2 others. · View Herald Transcript

Harbormaster completed remote builds in B11977: Diff 16432.Sep 8 2022, 3:34 AM

• max published this revision for review.Sep 8 2022, 6:16 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
47 ↗	(On Diff #16432)	CXX doesn't support Option<>, that's why we are checking for whether the topic is empty or not and returning the Option here.

• max added a parent revision: D5067: [services] Tunnelbroker - Changes to handle empty `message_id` inside the Rust `send_by_fcm_client` function.Sep 8 2022, 7:40 AM

• max added a child revision: D5083: [services] Tunnelbroker - Add payload to APNs message to make it `content-available`.

• jon accepted this revision.Sep 9 2022, 11:12 AM

• jon added inline comments.

services/tunnelbroker/rust-notifications/src/lib.rs
47 ↗	(On Diff #16432)	I would prefer a comment, so that when others stumble upon the code later, are able to find this relevant information easily.

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

tomek added inline comments.

services/tunnelbroker/rust-notifications/src/apns.rs
12 ↗	(On Diff #16432)	the topic for the message is mandatory It seems like there's no point in calling `send_by_a2_client` when topic is None. It should be a `&str` then
services/tunnelbroker/rust-notifications/src/lib.rs
15 ↗	(On Diff #16432)	Additional context: we have `getAPNsNotificationTopic` function that determines the topic. We should probably use the value from there, probably including older clients handling.

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

• max planned changes to this revision.Sep 15 2022, 8:16 AM

• max removed a parent revision: D5067: [services] Tunnelbroker - Changes to handle empty `message_id` inside the Rust `send_by_fcm_client` function.Sep 22 2022, 7:08 AM

Making topic not optional.

• max marked 3 inline comments as done.Sep 22 2022, 7:17 AM

• max added inline comments.

services/tunnelbroker/rust-notifications/src/apns.rs
12 ↗	(On Diff #16432)	the topic for the message is mandatory It seems like there's no point in calling `send_by_a2_client` when topic is None. It should be a `&str` then Makes sense. I've removed optional from `topic`.
services/tunnelbroker/rust-notifications/src/lib.rs
15 ↗	(On Diff #16432)	Additional context: we have `getAPNsNotificationTopic` function that determines the topic. We should probably use the value from there, probably including older clients handling. In a `getAPNsNotificationTopic` we are just hardcoded the topics: function getAPNsNotificationTopic(codeVersion: ?number): string { return codeVersion && codeVersion >= 87 ? 'app.comm' : 'org.squadcal.app'; } In the Tunnelbroker we are using the config file to provide a topic. Seems that this function is to support an old version of the app. Do we need to support the `org.squadcal.app` in the codebase of the services and hardcode it?
47 ↗	(On Diff #16432)	I would prefer a comment, so that when others stumble upon the code later, are able to find this relevant information easily. As the `topic` is no longer `Optional` we don't need to add a comment here. But it makes sense if it is. Thanks, @jon !

Harbormaster failed remote builds in B12382: Diff 16998!Sep 22 2022, 7:18 AM

tomek accepted this revision.Sep 22 2022, 7:26 AM

tomek added inline comments.

services/tunnelbroker/rust-notifications/src/lib.rs
15 ↗	(On Diff #16432)	It depends. If we're going to send notifications from tunnelbroker to older clients then yes. And I guess that we probably want that (but I'm not 100% sure)

This revision is now accepted and ready to land.Sep 22 2022, 7:26 AM

Harbormaster completed remote builds in B12382: Diff 16998.Sep 22 2022, 7:27 AM

• max marked 4 inline comments as done.Sep 22 2022, 7:31 AM

• max added inline comments.

services/tunnelbroker/rust-notifications/src/lib.rs
15 ↗	(On Diff #16432)	It depends. If we're going to send notifications from tunnelbroker to older clients then yes. And I guess that we probably want that (but I'm not 100% sure) I've created a possible follow-up task to discuss this.