Fix PN metadata leak, PN navigation to chat, and unexpected PNs from other accounts #7268
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (92)
|
rasom
force-pushed
the
fix/#6772-pn-metadata-leak
branch
2 times, most recently
from
1df2523 to
ae1acf3
Compare
|
for some reason i still dont receive PNs on my ios device. I am using iPhone 7 with iOS 11.4.1. I think the problem may be in this |
|
@asemiankevich case when you turn off and on PN works for me as expected - so I can again receive PNs. |
|
@asemiankevich just checked on iphone X and 6s, both with 12.1.2, works as expected. From which device/app version do you send the messages? |
@rasom |
ok, and what's the app version on android? |
|
Tried to turn on/off notifications on ios, can receive PNs after turning notifications on. |
@rasom it is android 8, version of the app is latest current PR build |
|
can also reproduce no PN if Android 9, pixel XL sends new message in 1:1 chat to IPhone 7 plus, iOS 11.0.3.
|
|
probably it is related to iOS version, because old versions are killing the app on background faster, not sure. It is all fine with iOS 12. |
|
@asemiankevich @annadanchenko @pombeirp @mandrigin We can try to implement some handling on native side but not as part of this PR. I would suggest merging it as it is for now. |
|
agree |
|
moved back to testing, there are some other issues to be rechecked before merge |
rasom
force-pushed
the
fix/#6772-pn-metadata-leak
branch
from
ae1acf3 to
fe3d71c
Compare
|
@asemiankevich make sure you are testing build # 10, that builds table is broken a bit |
100% of end-end tests have passedPassed tests (58)Click to expand
|
100% of end-end tests have passedPassed tests (58)Click to expand
|
|
@churik You don't need to wait, all necessary changes are already here. Question to Pedro is rather about the coordination of necessary changes on go side. |
|
@rasom I'll build a PR on go side to add a new method side-by-side with |
|
@pombeirp ok |
|
1) IOS crash is not fixed yet Steps described in #6893 (comment):
testing is still WIP |
|
|
invertase/react-native-firebase#1576 (comment) might be related... |
rasom
force-pushed
the
fix/#6772-pn-metadata-leak
branch
from
d326f9c to
70361ea
Compare
|
@churik ios issue is fixed in # 22 |
|
@rasom can it affect all functionality or recheck only specific issue will be enough? |
|
this specific issue will be enough |
|
@rasom on latest build I started receive messages when app is closed (on IOS). IS it OK from the security perspective (taking into account #6893 (comment))? |
|
This may happen only if you send messages from another version of the application, which doesn't have current changes. |
|
Rechecked #7268 (comment) on build #22 - not reproducible Want to summarise:
Please tell me if some of those should be reported as separate issues and fixed. |
|
@rasom please squash and merge these changes 🎉 |
|
@mandrigin @pombeirp |
rasom
force-pushed
the
fix/#6772-pn-metadata-leak
branch
from
af38f61 to
5a69b41
Compare
|
ok, merged |
|
Thanks for all the help @rasom! This was a tough one! |
|
@churik thanks :) We'll call out the breaking change in the next release notes. I think we should capture the issues with iOS and discuss priority in next core meeting. |
The same PR as #6893. It has too many comments, quite hard to "navigate" even to find new builds.
fixes #6772, #3488 and #2984, depends on status-im/status-go#1297
Summary:
Pre-PR example payload:
Same payload post-PR (notice versioning):
This PR:
hash:${msg-v2/id}, creating a 1:1 link between Whisper messages and their respective PN on the receiver sidemsg-v2/from,msg-v2/to,msg-v2/id:push-notifications/stored)Only create PN if message with hash equal tomsg-v2/idis not marked as seenconfig/in-app-notifications-enabled?/IN_APP_NOTIFICATIONS_ENABLEDRemoves dependency on NotifyUsers status-go API by leveraging react-native-firebase APIthe API does not support some critical properties for Android and iOS, so we must continue with the implementation in status-go for the time being. Created a suggestion at https://boards.invertase.io/react-native-firebase/p/add-missing-properties-to-messagingremotemessage.Review notes (optional):
This PR paves the way to fix issues like #3488, #3487, #2984
Testing notes (optional):
Platforms
Check that new apps can receive PNs from both new and old clients. I've tested Desktop and Android, but not iOS.
See here for expected behavior from a technical perspective: https://github.com/invertase/react-native-firebase-docs/blob/master/docs/messaging/introduction.md#data-only-messages
Areas that maybe impacted
Functional
Steps to test:
Future steps
With this PR implemented, we can easily implement the following:
status: ready