Introduce fallback mechanism for notifications decryption if MMKV initialization hasn't been executed yet.
ClosedPublic
Actions

Authored by marcin on Apr 3 2024, 6:40 AM.

Details

Reviewers

tomek
bartek
kamil

Commits

rCOMMd35aa5447d1a: Introduce fallback mechanism for notifications decryption if MMKV…

Summary

This differential fixes: https://linear.app/comm/issue/ENG-7309/nse-received-c-exception-nse-cant-initialize-mmkv-encryption-key. If decryption with session
stored in MMKV failes (since MMKV wasn't initialized yet) we try to use previous file-based session.

Test Plan

Revert this diff and the diff that migrates notifs session to MMKV. For the second reosolve the conflicts by making migration 38 a no-op.
Build the app and log in. This puts the app in legacy notifications state - notifications session is created in a flat file.
Reset reveerts, build the app again and send notifications.
Ensure notifications are working since the fallback mechanism uses old session.
Log out and log in again. Ensure that notifs are still working - new approach is working as well.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Apr 3 2024, 6:40 AM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 3 2024, 6:40 AM

marcin added inline comments.Apr 3 2024, 6:48 AM

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
87–148 ↗	(On Diff #38730)	This code is a copy-paste of a function that was introduced last year when encrypted notifications were introduced. No review is necessary here.
native/ios/NotificationService/NotificationService.mm
181 ↗	(On Diff #38730)	Originally this if statement would fail the notif the public user if they upgraded app version but didn't open the app after upgrading. This change will: make the NSE use badge count delivered with notif in case MMKV wasn't initialized (public user) display error message notification for staff user (previous behaviour).

Harbormaster completed remote builds in B27950: Diff 38730.Apr 3 2024, 6:56 AM

marcin requested review of this revision.Apr 3 2024, 6:56 AM

Please prioritize the review of this diff as it solves a long-standing urgent issue breaking notifs for all users

Looks good

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
416–422 ↗	(On Diff #38730)	I am wondering if this is the right approach, and shouldn't we e.g. modify `fetchNotificationsSession` to return `false`, `null`, or some special value? I am thinking about too wide range of exceptions, if this fails for a different reason than migration not executed yet, what should we do?
420 ↗	(On Diff #38730)	in this context - the name of this method is a bit confusing but I can't think of anything better
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h
55–56 ↗	(On Diff #38730)	How about making those protected without getters and setters?
103 ↗	(On Diff #38730)	nit: for me using this twice in one class decrease readabillity

This revision is now accepted and ready to land.Apr 4 2024, 5:11 AM

This issue needs to be fixed ASAP. It's disappointing to see this diff has been sitting in an accepted state for over 24 hours, while @marcin has prioritized other work. There is nothing else that matters more than resolving an urgent issue that breaks notifs for all users. I'm not sure how else to emphasize this.