PM-16705: Improve the node validation logic

Author: david-livefront

app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorImpl.kt

@@ -33,14 +33,13 @@ class BitwardenAccessibilityProcessorImpl(
     ) {
         // Only process the event if the tile was clicked
         val accessibilityAction = accessibilityAutofillManager.accessibilityAction ?: return
-        val eventNode = event.source ?: return
 
         // Prevent clearing the action until we receive a processable event in case unprocessable
         // events are still being received from the device. This can happen on slower devices or if
         // screen transitions are still being performed.
-        if (!eventNode.shouldProcessEvent(rootAccessibilityNodeInfoProvider)) {
-            return
-        }
+        val eventNode = event
+            .getValidNode(rootAccessibilityNodeInfoProvider = rootAccessibilityNodeInfoProvider)
+            ?: return
 
         // Clear the action since we are now acting on a supported node.
         accessibilityAutofillManager.accessibilityAction = null
@@ -53,35 +52,43 @@ class BitwardenAccessibilityProcessorImpl(
         }
     }
 
-    private fun AccessibilityNodeInfo.shouldProcessEvent(
+    private fun AccessibilityEvent.getValidNode(
         rootAccessibilityNodeInfoProvider: () -> AccessibilityNodeInfo?,
-    ): Boolean {
+    ): AccessibilityNodeInfo? {
+        val eventNode = this
+            .source
+            ?: run {
+                Timber.w("Accessibility event source is null, attempting root node")
+                // We only call for the root node once, after verifying that the there is an action
+                // to be filled. This is because it is a significant performance hit.
+                // Additionally, we do not use the root node if it does not have the same package
+                // as the triggering event.
+                rootAccessibilityNodeInfoProvider()?.takeIf { it.packageName == this.packageName }
+            }
+            ?: run {
+                Timber.w("Root node was also null, skipping this event")
+                return null
+            }
+
         // Ignore the event when the phone is inactive.
-        if (!powerManager.isInteractive) return false
+        if (!powerManager.isInteractive) return null
         // We skip if the system package.
-        if (this.isSystemPackage) {
-            Timber.d("Skipping autofill for system package $packageName.")
-            return false
+        if (eventNode.isSystemPackage) {
+            Timber.d("Skipping autofill for system package ${eventNode.packageName}.")
+            return null
         }
         // We skip any package that is explicitly blocked.
-        if (this.shouldSkipPackage) {
-            Timber.d("Skipping autofill on block-listed package $packageName.")
-            return false
+        if (eventNode.shouldSkipPackage) {
+            Timber.d("Skipping autofill on block-listed package ${eventNode.packageName}.")
+            return null
         }
         // We skip any package that is a launcher.
-        if (launcherPackageNameManager.launcherPackages.any { it == this.packageName }) {
-            Timber.d("Skipping autofill on launcher package $packageName.")
-            return false
-        }
-
-        // We only call for the root node once, after all other checks have passed, because it is a
-        // significant performance hit.
-        if (rootAccessibilityNodeInfoProvider()?.packageName != this.packageName) {
-            Timber.d("Skipping autofill due to package name mismatch.")
-            return false
+        if (launcherPackageNameManager.launcherPackages.any { it == eventNode.packageName }) {
+            Timber.d("Skipping autofill on launcher package ${eventNode.packageName}.")
+            return null
         }
 
-        return true
+        return eventNode
     }
 
     private fun handleAttemptParseUri(rootNode: AccessibilityNodeInfo) {

app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorTest.kt

@@ -77,7 +77,7 @@ class BitwardenAccessibilityProcessorTest {
     }
 
     @Test
-    fun `processAccessibilityEvent with null event source should return`() {
+    fun `processAccessibilityEvent with null event source and root node should return`() {
         val event = mockk<AccessibilityEvent> {
             every { source } returns null
         }
@@ -89,6 +89,41 @@ class BitwardenAccessibilityProcessorTest {
         }
     }
 
+    @Test
+    fun `processAccessibilityEvent with null event source and invalid root node should return`() {
+        val event = mockk<AccessibilityEvent> {
+            every { source } returns null
+            every { packageName } returns "event_package"
+        }
+        val rootNode = mockk<AccessibilityNodeInfo> {
+            every { packageName } returns "other_package"
+        }
+
+        bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode }
+
+        verify(exactly = 0) {
+            powerManager.isInteractive
+        }
+    }
+
+    @Test
+    fun `processAccessibilityEvent with null event source and valid root node should continue`() {
+        val event = mockk<AccessibilityEvent> {
+            every { source } returns null
+            every { packageName } returns "package"
+        }
+        val rootNode = mockk<AccessibilityNodeInfo> {
+            every { packageName } returns "package"
+        }
+        every { powerManager.isInteractive } returns false
+
+        bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode }
+
+        verify(exactly = 1) {
+            powerManager.isInteractive
+        }
+    }
+
     @Test
     fun `processAccessibilityEvent with powerManager not interactive should return`() {
         val event = mockk<AccessibilityEvent> {
@@ -184,40 +219,6 @@ class BitwardenAccessibilityProcessorTest {
         }
     }
 
-    @Test
-    fun `processAccessibilityEvent with mismatched package name should return`() {
-        val testPackageName = "com.android.chrome"
-        val rootNode = mockk<AccessibilityNodeInfo> {
-            every { packageName } returns "other.package.name"
-        }
-        val node = mockk<AccessibilityNodeInfo> {
-            every { isSystemPackage } returns false
-            every { shouldSkipPackage } returns false
-            every { packageName } returns testPackageName
-        }
-        val event = mockk<AccessibilityEvent> {
-            every { source } returns node
-            every { packageName } returns testPackageName
-        }
-        every { powerManager.isInteractive } returns true
-        every { launcherPackageNameManager.launcherPackages } returns emptyList()
-        every {
-            accessibilityAutofillManager.accessibilityAction
-        } returns AccessibilityAction.AttemptParseUri
-        every { accessibilityAutofillManager.accessibilityAction = null } just runs
-        every { accessibilityParser.parseForUriOrPackageName(rootNode = node) } returns null
-
-        bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode }
-
-        verify(exactly = 1) {
-            powerManager.isInteractive
-            node.isSystemPackage
-            node.shouldSkipPackage
-            launcherPackageNameManager.launcherPackages
-            accessibilityAutofillManager.accessibilityAction
-        }
-    }
-
     @Test
     fun `processAccessibilityEvent with AttemptParseUri and a invalid uri should show a toast`() {
         val testPackageName = "com.android.chrome"