PM-13104: Remove biometric integrity checks #995
Conversation
…icsServiceError.getAuthKeyFailed
matt-livefront
requested review from
phil-livefront,
victor-livefront,
brant-livefront and
a team
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #995 +/- ##
==========================================
- Coverage 89.10% 89.08% -0.03%
==========================================
Files 657 657
Lines 41317 41211 -106
==========================================
- Hits 36814 36711 -103
+ Misses 4503 4500 -3 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
| if !hasMasterPassword { | ||
| let biometricUnlockStatus = try await services.biometricsRepository.getBiometricUnlockStatus() | ||
| if case .available(_, true, false) = biometricUnlockStatus { | ||
| return .enterpriseSingleSignOn(email: activeAccount.profile.email) |
There was a problem hiding this comment.
🤔 We're good with not navigating to the enterprise SSO screen here because if the biometrics fails (because the user changed their settings), it will drop to the login screen, which will give them an option to re-login with SSO?
There was a problem hiding this comment.
Correct. That's a good point though, in the case of a biometrics failure (and if you don't have a MP or PIN), I think you would still remain on the vault unlock screen. We don't have logic that would log out you in that case. Should I add that?
There was a problem hiding this comment.
Can't we just show the unlock with biometrics button in the screen when the user doesn't have MP and has biometrics configured? So they can try again and if they fail 5 times then it'd go through the logic of automatic logout because of reiterative failed attempts.
There was a problem hiding this comment.
The problem is that it'd never succeed, because it'd fail here for the user having changed their biometrics settings, and we need to re-authorize the user in order to save a new key.
I think forcing a logout at that point is probably the most reasonable solution? If a TDE user is in this state, the only thing that can re-authorize them is SSO (which they'd do from the login screen), since biometrics won't work until they do.
At least, that's my understanding of the situation?
There was a problem hiding this comment.
Can't we just show the unlock with biometrics button in the screen when the user doesn't have MP and has biometrics configured? So they can try again and if they fail 5 times then it'd go through the logic of automatic logout because of reiterative failed attempts.
Once biometrics fails because the key doesn't exist, we disable biometrics and remove the biometrics button. iOS also doesn't prompt you for Face ID in this case since the key has been removed. So even if we keep the biometrics button visible, you would tap it and it would look like nothing happened. Until you eventually get logged out, so we might as well just logout from the start.
There was a problem hiding this comment.
I added a change to log the user out if biometrics fails because the key doesn't exist, and the user doesn't have a MP or PIN. 4698b86
…oesn't have MP or PIN
| let hasMasterPassword = try? await services.authRepository.hasMasterPassword() | ||
| let isPinEnabled = try? await services.authRepository.isPinUnlockAvailable() | ||
| if hasMasterPassword == false, isPinEnabled == false { | ||
| // If biometrics is enabled, but the auth key doesn't exist and the user doesn't | ||
| // have a master password or PIN, log the user out. | ||
| await logoutUser(userInitiated: false) | ||
| } else { | ||
| // Otherwise, refresh the data to remove biometrics as an unlock method. | ||
| await loadData() | ||
| } |
There was a problem hiding this comment.
🤔 If hasMasterPassword is nil because of throwing shouldn't we logout the user as well? Given that either there are no accounts or there are no active accounts; so I guess on these cases it makes sense to logout as well.
There was a problem hiding this comment.
I think it's pretty unlikely that you would get here in that case, something before this would fail, but I can make that change.
…whether the user has a master password fails
matt-livefront
deleted the
matt/PM-13104-remove-biometrics-integrity-checks
branch
🎟️ Tracking
PM-13104
📔 Objective
Removes biometric integrity checks. The downside of these checks is that they are unique per-process. So even if biometrics were enabled in the app, opening the autofill extension would require the user to enter their MP or PIN before biometrics would be enabled in that extension. The biometrics auth key already uses biometryCurrentSet which will invalid the keychain item if a finger or face is added for Touch/Face ID. Without these checks, enabling biometrics in the app will cause it to work in any of the extensions immediately vs after the first successful unlock with MP or PIN.
⏰ 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 confirmed issue 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