PM-17968: Create unique secret keys per user and handle decoding error #4696
Conversation
david-livefront
requested review from
brian-livefront,
dseverns-livefront,
ahaisting-livefront,
phil-livefront and
a team
as code owners
|
Great job, no security vulnerabilities found in this Pull Request |
david-livefront
force-pushed
the
PM-17968-biometrics-decoding-exception
branch
from
4d1b77f to
b63d0b9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4696 +/- ##
=======================================
Coverage 89.46% 89.46%
=======================================
Files 484 484
Lines 40563 40574 +11
Branches 5766 5768 +2
=======================================
+ Hits 36290 36300 +10
Misses 2273 2273
- Partials 2000 2001 +1 ☔ View full report in Codecov by Sentry. |
| @@ -1372,6 +1373,7 @@ class AuthRepositoryImpl( | |||
| // the notice needs to appear again | |||
| NewDeviceNoticeDisplayStatus.HAS_SEEN -> | |||
| newDeviceNoticeState.shouldDisplayNoticeIfSeen | |||
|
|
|||
There was a problem hiding this comment.
Is this new line intentional? I know with a -> { \n } branch it will add the space so if it does it for -> \n das cool too.
There was a problem hiding this comment.
This was the formatter, I could drop this change and make a separate PR for formatting the app?
| // Attempt to get the user scoped key. If that fails, we check to see if a legacy key | ||
| // is available. If neither succeeds, then we need to generate a new one. |
| isBiometricsValid = false, | ||
| dialog = VaultUnlockState.VaultUnlockDialog.Error( | ||
| title = R.string.an_error_has_occurred.asText(), | ||
| message = R.string.generic_error_message.asText(), |
There was a problem hiding this comment.
If I'm following correctly; the user will attempt biometrics, this error will occur, a generic error is displayed, and biometrics are no longer enabled. The user then has to log in with a PIN/Password and turn biometrics back on.
I feel like a more descriptive error is warranted here since we're invalidating their biometrics. Maybe something like, "Device biometrics unrecoverable for this account. Log in with your Master Password or PIN then re-enable biometric login in Settings."?
There was a problem hiding this comment.
Good point, I will get this updated
There was a problem hiding this comment.
I updated this but waiting on design to confirm the new copy.
david-livefront
force-pushed
the
PM-17968-biometrics-decoding-exception
branch
2 times, most recently
from
8a389a9 to
3cc5cd1
Compare
david-livefront
force-pushed
the
PM-17968-biometrics-decoding-exception
branch
from
3cc5cd1 to
62650e1
Compare
| @@ -497,6 +497,8 @@ Scanning will happen automatically.</string> | |||
| <string name="login_expired">Your login session has expired.</string> | |||
| <string name="biometrics_direction">Biometric verification</string> | |||
| <string name="biometrics">Biometrics</string> | |||
| <string name="biometrics_failed">Biometrics Failed</string> | |||
| <string name="biometrics_decoding_failure">Log in with your Master Password or PIN then re-enable biometric login in Settings.</string> | |||
|
Thanks @SaintPatrck |
🎟️ Tracking
PM-17968
PM-17812
Resolves #4659, #4683
📔 Objective
This PR updates the
BiometricsEncryptionManagerto migrate secret keys to be unique per user.Primary Changes:
Unfortunately, a user who is already in the failure state is not recoverable and they need to re-setup biometrics.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes