PM-16062 Prevent account locks for ongoing autofill requests #4498

dseverns-livefront · 2024-12-20T15:35:31Z

🎟️ Tracking

PM-16062

📔 Objective

When the app was being opened by the AutoFill/Accessibility services from a cold start, the user would need to unlock the vault, and once completing the AutoFill request the vault was being locked based on the state being send to the VaultLockManager.
This was especially called out for logins that have a (2 page) flow, where the user was being asked to unlock the vault again to grab the password on the second page.
This change changes the VaultLockManager to 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

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation or informed the documentation team

🦮 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

…ly for autofill and update restart timeout to look at app created events

github-actions · 2024-12-20T15:40:23Z

Checkmarx One – Scan Summary & Details – 312f80fe-1f8c-4228-96cf-f00e058b3d71

No New Or Fixed Issues Found

brian-livefront · 2024-12-20T15:56:57Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/AppCreationState.kt

     */
-    CREATED,
+    data class Created(val createdForAutofill: Boolean) : AppCreationState()


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?

app/src/main/java/com/x8bit/bitwarden/data/autofill/util/AutofillIntentUtils.kt

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/AppCreationState.kt

brian-livefront · 2024-12-20T16:52:53Z

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

-                checkTimeoutReason = CheckTimeoutReason.APP_RESTARTED,
-            )
-        }
+    private fun handleOnCreated(createdForAutofill: Boolean, isFirstCreated: Boolean) {


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.

brian-livefront · 2024-12-20T16:56:04Z

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

-        }
+    private fun handleOnCreated(createdForAutofill: Boolean, isFirstCreated: Boolean) {
+        val userId = activeUserId ?: return
+        userIdTimerJobMap[userId]?.cancel()


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.

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

brian-livefront · 2024-12-20T17:08:15Z

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

@@ -491,27 +486,37 @@ class VaultLockManagerImpl(

            VaultTimeout.OnAppRestart -> {
                // If this is an app restart, trigger the timeout action; otherwise ignore.
-                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.


This doc likely needs updating because right now its only discussing the firstTimeCreation case.

…n app is foregrounded

codecov · 2024-12-20T18:50:27Z

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.93%. Comparing base (8548c74) to head (ea6f0d6).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...twarden/data/vault/manager/VaultLockManagerImpl.kt	94.44%	1 Missing and 1 partial ⚠️

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.
📢 Have feedback on the report? Share it here.

brian-livefront · 2024-12-20T19:27:27Z

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

+    private fun handleOnForeground() {
+        val userId = activeUserId ?: return
+        userIdTimerJobMap[userId]?.cancel()
+    }


Can we leave this function back where it was to make the diff a tiny bit smaller?

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

brian-livefront · 2024-12-20T19:28:45Z

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

+         * @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) :


Let's update the formatting to

data class AppCreated( val firstTimeCreation: Boolean, val createdForAutofill: Boolean ) :CheckTimeoutReason()

PM-16062 prevent account lock when new activity is created specifical…

5500f92

…ly for autofill and update restart timeout to look at app created events

dseverns-livefront requested a review from SaintPatrck December 20, 2024 15:35

dseverns-livefront requested review from brian-livefront, david-livefront, ahaisting-livefront, phil-livefront and a team as code owners December 20, 2024 15:35

update docs

1f583de

brian-livefront reviewed Dec 20, 2024

View reviewed changes

parameter name

40dc0e9

dseverns-livefront requested a review from brian-livefront December 20, 2024 16:36

brian-livefront reviewed Dec 20, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/autofill/util/AutofillIntentUtils.kt Show resolved Hide resolved

brian-livefront reviewed Dec 20, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/AppCreationState.kt Show resolved Hide resolved

brian-livefront reviewed Dec 20, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt Show resolved Hide resolved

brian-livefront reviewed Dec 20, 2024

View reviewed changes

pr feedback: formatting, docs, and making sure timer is cancelled whe…

1373951

…n app is foregrounded

dseverns-livefront requested a review from brian-livefront December 20, 2024 19:13

brian-livefront reviewed Dec 20, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt Show resolved Hide resolved

brian-livefront reviewed Dec 20, 2024

View reviewed changes

feedback, fmting, attempting to rightsize tests

ea6f0d6

dseverns-livefront requested a review from brian-livefront December 20, 2024 21:09

brian-livefront approved these changes Dec 20, 2024

View reviewed changes

dseverns-livefront added this pull request to the merge queue Dec 20, 2024

Merged via the queue into main with commit 6223f36 Dec 20, 2024
8 checks passed

dseverns-livefront deleted the PM-16062-vault-locking-each-time-autofill-starts branch December 20, 2024 22:29

differsthecat linked an issue Dec 30, 2024 that may be closed by this pull request

Inline Tab Autofill Requires Unlocking Multiple Times Despite Session Timeout Settings #4475

Closed

1 task

antiquely8637 mentioned this pull request Jan 9, 2025

Vault locks immediately after auto-filling #4408

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PM-16062 Prevent account locks for ongoing autofill requests #4498

PM-16062 Prevent account locks for ongoing autofill requests #4498

dseverns-livefront commented Dec 20, 2024 •

edited by jira bot

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

brian-livefront Dec 20, 2024 •

edited

Loading

brian-livefront Dec 20, 2024

brian-livefront Dec 20, 2024

brian-livefront Dec 20, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading

brian-livefront Dec 20, 2024

brian-livefront Dec 20, 2024

PM-16062 Prevent account locks for ongoing autofill requests #4498

PM-16062 Prevent account locks for ongoing autofill requests #4498

Conversation

dseverns-livefront commented Dec 20, 2024 • edited by jira bot Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Dec 20, 2024 • edited Loading

No New Or Fixed Issues Found

brian-livefront Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

brian-livefront Dec 20, 2024

Choose a reason for hiding this comment

brian-livefront Dec 20, 2024

Choose a reason for hiding this comment

brian-livefront Dec 20, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

brian-livefront Dec 20, 2024

Choose a reason for hiding this comment

brian-livefront Dec 20, 2024

Choose a reason for hiding this comment

dseverns-livefront commented Dec 20, 2024 •

edited by jira bot

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

brian-livefront Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading