PM-16062 Prevent account locks for ongoing autofill requests#4498
Conversation
…ly for autofill and update restart timeout to look at app created events
dseverns-livefront
requested review from
a team,
ahaisting-livefront,
brian-livefront,
david-livefront and
phil-livefront
as code owners
|
No New Or Fixed Issues Found |
| * @param createdForAutofill Whether the app was created for autofill. | ||
| */ | ||
| CREATED, | ||
| data class Created(val createdForAutofill: Boolean) : AppCreationState() |
There was a problem hiding this comment.
Can we make this isCreatedForAutofill or wasCreatedForAutofill? I think that might fit boolean patterns in the app a little better. Or maybe even isAutofill, since the "created" part is already implied?
| checkTimeoutReason = CheckTimeoutReason.APP_RESTARTED, | ||
| ) | ||
| } | ||
| private fun handleOnCreated(createdForAutofill: Boolean, isFirstCreated: Boolean) { |
There was a problem hiding this comment.
Borderline but if I remember correctly after the first param we'd usually break these onto new lines so its easier to add a 3rd, 4th, etc. later.
| } | ||
| private fun handleOnCreated(createdForAutofill: Boolean, isFirstCreated: Boolean) { | ||
| val userId = activeUserId ?: return | ||
| userIdTimerJobMap[userId]?.cancel() |
There was a problem hiding this comment.
I'm a little nervous about moving userIdTimerJobMap[userId]?.cancel() to handleOnCreated now that I see this. I wonder if we need to keep handleOnForeground just for this. I believe the intention of this line is to say "whenever the app is foregrounded, we shouldn't have a timer going for the current user". In which case moving this here is a behavior change.
| if (checkTimeoutReason == CheckTimeoutReason.APP_RESTARTED) { | ||
| if (checkTimeoutReason is CheckTimeoutReason.AppCreated) { | ||
| // On restart the vault should be locked already but we may need to soft-logout | ||
| // the user. |
There was a problem hiding this comment.
This doc likely needs updating because right now its only discussing the firstTimeCreation case.
…n app is foregrounded
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
=========================================
+ Coverage 0 88.93% +88.93%
=========================================
Files 0 459 +459
Lines 0 39853 +39853
Branches 0 5638 +5638
=========================================
+ Hits 0 35444 +35444
- Misses 0 2441 +2441
- Partials 0 1968 +1968 ☔ View full report in Codecov by Sentry. |
| private fun handleOnForeground() { | ||
| val userId = activeUserId ?: return | ||
| userIdTimerJobMap[userId]?.cancel() | ||
| } |
There was a problem hiding this comment.
Can we leave this function back where it was to make the diff a tiny bit smaller?
| * @param createdForAutofill if the the creation event is due to an activity being launched | ||
| * for autofill. | ||
| */ | ||
| data class AppCreated(val firstTimeCreation: Boolean, val createdForAutofill: Boolean) : |
There was a problem hiding this comment.
Let's update the formatting to
data class AppCreated(
val firstTimeCreation: Boolean,
val createdForAutofill: Boolean
) :CheckTimeoutReason()
dseverns-livefront
deleted the
PM-16062-vault-locking-each-time-autofill-starts
branch
2 participants
🎟️ Tracking
PM-16062
📔 Objective
VaultLockManager.VaultLockManagerto check the timeout action instead on events of the activity being created for the first time with the added context of if was created for Autofill purposes.📸 Screenshots
pm16062.mp4
⏰ 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