Page MenuHomePhabricator

Temporary changes for staff release
ClosedPublic

Authored by atul on Oct 10 2022, 7:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Mon, Nov 25, 8:11 PM
Unknown Object (File)
Sun, Nov 24, 12:32 PM
Unknown Object (File)
Sun, Nov 24, 12:32 PM

Details

Summary

This differential introduces new temporary changes for staff release for C++/native langs. side after global db singleton has benn landed.

Test Plan

Build app, send noeifs and rescinds. Must work.

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D5332_1 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

@marcin, for each of these changes can you add a comment explaining why it is necessary? I was under the impression that the GlobalDBSingleton work would make these temporary changes unnecessary... not opposed to them, just want to understand why we still need some temporary changes

atul edited reviewers, added: marcin; removed: atul.

Commandeering to include additional changes

include changes to redux-setup

atul edited reviewers, added: atul; removed: marcin.

Passing back to @marcin to address following feedback

@marcin, for each of these changes can you add a comment explaining why it is necessary? I was under the impression that the GlobalDBSingleton work would make these temporary changes unnecessary... not opposed to them, just want to understand why we still need some temporary changes

atul requested changes to this revision.Oct 11 2022, 7:10 AM
This revision now requires changes to proceed.Oct 11 2022, 7:10 AM

@marcin, for each of these changes can you add a comment explaining why it is necessary? I was under the impression that the GlobalDBSingleton work would make these temporary changes unnecessary... not opposed to them, just want to understand why we still need some temporary changes

Those changes are not related to global db singleton. Some time ago we had theory that iOS 173 crash was caused be the fact that encryption key used for database encryption is available only WHEN_UNLOCKED since expo docs state that this is default value for accessibility option and we didn't specify any value for accessibility option. We created a release build with changes that explicitly set accessibility options to AFTER_FIRST_UNLOCK. Then I searched through expo's source code and found that there is a bug - even though docs said WHEN_UNLOCKED is default actually AFTER_FIRST_UNLOCK was the default. Changes above appeared to be no-op then, but we kept attaching them to each release build. The reason to keep them might be that I immediately informed expo team (at SWM) about the bug and they promised te resolve it quickly. I didn't check in their source code if they did. If they did we need those changes - if they didn't those changes are unnecessary now, but might be in the future once they resolve the bug.

ashoat requested changes to this revision.Oct 11 2022, 9:43 AM

For @marcin:

  1. Can you add inline comments explaining each change? I think your answer above explains most of the changes, but not all
  2. Regarding the SecureStore change, what's our plan for no longer needing to patch something in for releases? Is there something we can land permanently? Is there a task tracking this?
native/ios/Comm/AppDelegate.mm
174–175 ↗(On Diff #17460)

For instance, this doesn't seem related to your explanation, but I may be wrong

native/redux/redux-setup.js
323–356 ↗(On Diff #17460)

For @atul: I thought the plan in ENG-1822 was to remove these for everybody except yourself?

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
24 ↗(On Diff #17460)

We made secureStoreEncryptionKeyID public so that CommSecureStoreIOSWrapper could access it when migration accessibility settings for encryptionKey. Public getter was an alternative.

native/ios/Comm/AppDelegate.mm
73 ↗(On Diff #17460)

Here we migrate accessibility settings for encryptionKey from WHEN_UNLOCKED to AFTER_FIRST_UNLOCK. This one (and migration implementation) should stay if we want to be ready for expo fixing their bug.

174–175 ↗(On Diff #17460)

For this one you are right - it is not related and it is unnecessary change. This can be removed for release purpose.

native/ios/Comm/CommSecureStoreIOSWrapper.mm
70 ↗(On Diff #17460)

This is implementation for accessibility options migration for encryptionKey.

The context for encryptionKey accessibility migration code is in this differential we have probably forgotten about: https://phab.comm.dev/D4795. In this differential code was accepted as release-friendly but not landable. For landing I will need to introduce changes requested here but personally I wouldn't block release on this but rather create high priority Linear task for that.

ashoat edited reviewers, added: marcin; removed: ashoat.
ashoat foisted this revision upon atul.
ashoat edited reviewers, added: ashoat; removed: atul.
This revision now requires review to proceed.Oct 11 2022, 10:35 AM

For landing I will need to introduce changes requested here but personally I wouldn't block release on this but rather create high priority Linear task for that.

Thanks @marcin, can you create a task to follow-up on this?

Passing this back to @atul – one change requested inline and one question, and then we can move forward with the release

native/ios/Comm/AppDelegate.mm
174–175 ↗(On Diff #17460)

@atul, can you remove this?

native/redux/redux-setup.js
323–356 ↗(On Diff #17460)

@atul, see my question here

Created task: https://linear.app/comm/issue/ENG-2010/refactor-encryptionkey-accessibility-options-migration-code-so-that-it. I have most context on this so I am probably best choice if we want to land is ASAP, but I am open for other team members to work on this task since it is not that complex (and core logic is already implemented). Perhaps @bartek could be a good person since he is former expo member and this task deals with expo.

native/redux/redux-setup.js
323–356 ↗(On Diff #17460)

Will remove for everyone except for me before landing

Accepting pending two inline edits requested above

This revision is now accepted and ready to land.Oct 11 2022, 9:38 PM

remove changes to redux-setup