chore(deps): react-native-keychain upgrade + migrate to @interaxyz fork
#6470
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6470 +/- ##
========================================
Coverage 89.02% 89.02%
========================================
Files 729 729
Lines 31899 31899
Branches 5829 6151 +322
========================================
Hits 28397 28397
+ Misses 3454 3303 -151
- Partials 48 199 +151
... and 63 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -27,7 +27,6 @@ export async function storeItem({ key, value, options = {} }: SecureStorage) { | |||
| const result = await Keychain.setGenericPassword('CELO', value, { | |||
| service: key, | |||
| accessible: Keychain.ACCESSIBLE.WHEN_UNLOCKED_THIS_DEVICE_ONLY, | |||
| rules: Keychain.SECURITY_RULES.NONE, | |||
There was a problem hiding this comment.
this one is moved from the setter to the getter.
seems reasonable as this rule is steering the upgrade process which happens only at requesting the secret
| @@ -85,7 +85,7 @@ export async function removeStoredItem(key: string) { | |||
|
|
|||
| export async function listStoredItems() { | |||
| try { | |||
| return Keychain.getAllGenericPasswordServices() | |||
| return Keychain.getAllGenericPasswordServices({ skipUIAuth: true }) | |||
There was a problem hiding this comment.
| @@ -191,7 +191,6 @@ function storePinWithBiometry(pin: string) { | |||
| options: { | |||
| accessControl: Keychain.ACCESS_CONTROL.BIOMETRY_CURRENT_SET, | |||
| accessible: Keychain.ACCESSIBLE.WHEN_UNLOCKED_THIS_DEVICE_ONLY, | |||
| authenticationType: Keychain.AUTHENTICATION_TYPE.BIOMETRICS, | |||
There was a problem hiding this comment.
this property is gone in 9.2.2
probably not a big deal since we have accessControl property above which seems more elaborated
see also:
There was a problem hiding this comment.
Awesome! 🚀
I guess we'll probably need to do some additional testing before the next release (at bugapalooza?) to ensure the upgrade works as expected with existing accounts.
@kathaypacific you might want take a look at this one too since you initially added biometrics.
|
@kathaypacific the main thing to ensure is that users will not lose access to their PINs after the upgrade, because the library migrated to a different storage type on Android. I guess this can be mostly done by testing upgrades on different devices/Android versions. probably additional manual testing of a nightly build on different Android versions on the emulator could help mitigate this risk. I can do that. if we can also ask folks to check the nightly builds on their phones, that would be also helpful. not sure about bugapalooza, since we are worrying mostly about the update which happens automatically, not as a result of some unexpected user interaction. perhaps we could ask folks first to install the current nightly version, and then check it the next day after we merge the library update. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Migrate
react-native-keychainto the@interaxyzfork in order to get rid of the patch.The patch changes are implemented in the fork:
oblador/react-native-keychain@master...mobilestack-xyz:react-native-keychain:intera-fork
Since the current version (8.2.0) is outdated, it was also upgraded to the latest one (9.2.2), which required some refactoring.
Test plan
Manual test plan:
The library migrates the secrets storage in Android from
SharedPreferencestoDataStorein version 9.2.0. While the migration is expected to be seamless, perhaps we should do as much manual testing on devices as we can to ensure there is no major issue.Related issues
Backwards compatibility
Y
Network scalability
NA