Page MenuHomePhabricator

[Tunnelbroker] send push notifs to windows devices
ClosedPublic

Authored by varun on Jul 30 2024, 12:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 2:39 AM
Unknown Object (File)
Sat, Nov 2, 2:38 AM
Unknown Object (File)
Sat, Nov 2, 2:37 AM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Sat, Nov 2, 2:34 AM
Unknown Object (File)
Sat, Nov 2, 2:33 AM
Subscribers

Details

Summary

Make it possible to send push notifs to windows devices through tunnelbroker

Depends on D12934

Test Plan

https://gist.github.com/vdhanan/3921680cde51e92a8c7787771b36d19a

successfully sent push notifs to my windows desktop app

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun held this revision as a draft.
shared/tunnelbroker_messages/src/messages/notif.rs
40–41 ↗(On Diff #42928)

See https://linear.app/comm/issue/ENG-8901/fix-serde-parsing-incorrect-tunnelbroker-message-types
The same problem appears here and that's why Commtest CI fails

40–41 ↗(On Diff #42928)

should be TagAwareDeserialize, sorry

fix message serialization

finally got this working and tested end to end

varun published this revision for review.Aug 6 2024, 8:57 PM
varun retitled this revision from [WIP][Tunnelbroker] send push notifs to windows devices to [Tunnelbroker] send push notifs to windows devices.
varun edited the summary of this revision. (Show Details)
varun edited the test plan for this revision. (Show Details)

update revision

bartek added 1 blocking reviewer(s): kamil.

LGTM, letting @kamil take a look at the request format

services/tunnelbroker/src/notifs/wns/mod.rs
40 ↗(On Diff #43213)

nit, to disambiguate WNS token and device token

kamil added inline comments.
services/tunnelbroker/src/notifs/wns/mod.rs
59–69 ↗(On Diff #43213)

Could you create a specific error for status codes?

Similar to what I did in e.g. D12768. Most important are errors about invalid device token which you need for ENG-8913.

71–75 ↗(On Diff #43213)

I don't think this is needed - we don't that for other push services, for keyserver it was implemented because notif ID was needed

This revision is now accepted and ready to land.Aug 7 2024, 12:42 AM
This revision was landed with ongoing or failed builds.Aug 7 2024, 2:41 PM
This revision was automatically updated to reflect the committed changes.