PM-14585: Removed lock account option from Profile switcher if account has no master password #1155
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1155 +/- ##
==========================================
- Coverage 89.38% 89.38% -0.01%
==========================================
Files 688 688
Lines 43734 43760 +26
==========================================
+ Hits 39091 39114 +23
- Misses 4643 4646 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
No New Or Fixed Issues Found |
| let hasMasterPassword: Bool = await ( | ||
| try? stateService.getUserHasMasterPassword(userId: account.profile.userId) | ||
| ) ?? false | ||
| let isUnlockWithPinOn: Bool = await ( | ||
| try? stateService.pinProtectedUserKey(userId: account.profile.userId) | ||
| ) != nil | ||
| let isUnlockWithBiometricOn: Bool = await ( | ||
| try? biometricsRepository.getBiometricUnlockStatus().isEnabled | ||
| ) ?? false | ||
| let canBeLocked = hasMasterPassword || isUnlockWithPinOn || isUnlockWithBiometricOn |
There was a problem hiding this comment.
🤔 Does it make sense to extract this logic into a func in this repository? So it's easier to be reused if needed elsewhere. It can be private for the time being if you want. What do you think?
🤔 Additionally, I see these functions already being used which is alike the logic you have here but without the user id:
isPinUnlockAvailable()canVerifyMasterPassword()
Would it make sense to improve these functions passinguserId: String?and having an extension method with no parameter that passesuserId: nilso we reuse them in this logic as well?
I feel like by reusing these functions, then if the state service changes somehow it would be less code to maintain/update.
There was a problem hiding this comment.
I think that is good improvement, I am sure we would need this canBeLocked check somewhere else. I will add it.
BitwardenShared/UI/Auth/ProfileSwitcher/ProfileSwitcherRow.swift
Outdated
Show resolved
Hide resolved
| let hasMasterPassword: Bool = await ( | ||
| try? canVerifyMasterPassword(userId: userId) | ||
| ) ?? false | ||
| let isUnlockWithPinOn: Bool = await ( | ||
| try? isPinUnlockAvailable(userId: userId) | ||
| ) ?? false | ||
| let isUnlockWithBiometricOn: Bool = await ( | ||
| try? biometricsRepository.getBiometricUnlockStatus().isEnabled | ||
| ) ?? false | ||
| return hasMasterPassword || isUnlockWithPinOn || isUnlockWithBiometricOn |
There was a problem hiding this comment.
♻️ @ezimet-livefront @matt-livefront IMO all the try? should not ignore errors. If errors are thrown here they should be inside a do...catch and logged into Crashlytics so we actually know something went wrong. If any of the errors should be ignored then we should have specific catch clauses for them.
There was a problem hiding this comment.
I will try to add this in my future PR.
🎟️ Tracking
PM-14585
📔 Objective
canBeLockedproperty forProfileSwitcherItem.canBeLockedinAuthRepository, An account can now be locked only if it has a master password, biometric authentication, or a PIN.📸 Screenshots
⏰ 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