Fix module scope writes and the daemon notifications the manager dropped

daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt

@@ -266,6 +266,20 @@ object VectorService : IDaemonService.Stub() {
           // Otherwise, only wipe it for the user that just uninstalled it.
           val targetUser = if (isRemovedForAllUsers) null else userId
           PreferenceStore.deleteModulePrefs(moduleName, userId, group = null)
+          // The one preference of a module that is not stored under the module. "Never ask again"
+          // writes the package into a set belonging to "lspd", so the line above — which deletes
+          // what is filed under the module's own name — walks straight past it, and the package
+          // stayed blocked after it was uninstalled. Nothing else ever removes it: the CLI does not
+          // know the key and the manager offers no way back, so a module blocked by a mis-tap could
+          // never ask again on that device, and reinstalling it did not help.
+          //
+          // Asked of every package that goes for good, not only of modules, and deliberately so:
+          // `moduleName` here is whatever the broadcast named, and once a package is fully removed
+          // its metadata is gone, so this branch cannot tell a module from anything else —
+          // `isXposedModule` is decided below, and only for one that was still in our database. A
+          // package that was never blocked costs the one read of the "lspd" config row that
+          // [unblockScopeRequests] starts with, and nothing else.
+          if (isRemovedForAllUsers) unblockScopeRequests(moduleName)
           if (isRemovedForAllUsers && ModuleDatabase.removeModule(moduleName)) {
             // If it was in our DB and we successfully removed it, we treat it as an Xposed module.
             isXposedModule = true
@@ -356,7 +370,6 @@ object VectorService : IDaemonService.Stub() {
     val data = intent.data ?: return
     val extras = intent.extras ?: return
     val callbackBinder = extras.getBinder("callback") ?: return
-    if (!callbackBinder.isBinderAlive) return
 
     val authority = data.encodedAuthority ?: return
     val parts = authority.split(":", limit = 2)
@@ -367,12 +380,63 @@ object VectorService : IDaemonService.Stub() {
     val scopePackageName = data.path?.substring(1) ?: return // remove leading '/'
     val action = data.getQueryParameter("action") ?: return
 
+    // A prompt outlives the process that asked for it: it sits for an hour, and the app the module
+    // is running inside can be killed at any point in that hour. For approve, deny and the timeout
+    // there is then nobody left to tell, so those are dropped where they always were. "Never ask
+    // again" is not like them — it is the user's decision about the module, not an answer owed to a
+    // caller who is still listening — so it is honoured whether or not anyone is there to hear it,
+    // and the claim below still takes the prompt down.
+    if (!callbackBinder.isBinderAlive && action != "block") return
+
+    // One prompt reaches this receiver from four places — its three buttons and its delete intent
+    // — and a swipe or the one-hour timeout fires the delete intent whether or not a button was
+    // pressed first. Answering the module twice, an approval followed by a spurious "Request
+    // timeout", would be worse than the dismissal never reaching it, so whichever of the four
+    // arrives first is the one that answers and the rest are dropped. The request is identified by
+    // the module, its user and the package it asked for; the action is deliberately not part of
+    // that, since the whole point is that a second *different* action must not answer again.
+    if (!NotificationManager.claimScopeAnswer(packageName, userId, scopePackageName)) {
+      Log.d(TAG, "Ignoring $action of $scopePackageName for $packageName: already answered")
+      return
+    }
+
     val iCallback = IXposedScopeCallback.Stub.asInterface(callbackBinder)
     runCatching {
+          // Answered before the requested package is looked up at all, because "never ask again" is
+          // a decision about the *module* and not about the package this particular prompt happened
+          // to name. It used to sit in the when below, under the lookup, so a prompt naming a
+          // package that no longer resolves — uninstalled or disabled while the prompt was up,
+          // hidden by a profile owner, or never installed for this user — returned at "Package not
+          // found" and never reached blockScopeRequests: the user said stop asking, the prompt came
+          // down, and the module was free to ask again a second later.
+          if (action == "block") {
+            blockScopeRequests(packageName)
+            // The preference only stops the *next* request. A module that asked for three packages
+            // has a prompt up for each, so without this the user says "never ask again" and is left
+            // looking at two more questions, both still approvable. Each withdrawn request is
+            // answered in its own right, because each of them was asked in its own right; what that
+            // costs a module whose listener expects one call is written out on
+            // withdrawScopeRequests.
+            NotificationManager.withdrawScopeRequests(packageName).forEach { pending ->
+              runCatching { pending.onScopeRequestFailed("Request blocked by configuration") }
+            }
+            // Last, and caught, because this is the only call here that reaches into the module's
+            // process: it may be gone by now, and neither the preference write nor the withdrawal
+            // above must be lost to a dead binder. They can fail in their own ways — the write goes
+            // through a SQLite transaction — which is why the whole branch runs inside runCatching
+            // as well.
+            runCatching { iCallback.onScopeRequestFailed("Request blocked by configuration") }
+            return@runCatching
+          }
+
           val appInfo = packageManager?.getPackageInfoCompat(scopePackageName, 0, userId)
           if (appInfo == null) {
+            // Leaving the whole function here skipped the cancel below, which used to be merely
+            // untidy and is now a prompt nobody can use: the request has been answered, so every
+            // later press of its buttons is dropped. The request is over either way, so the
+            // notification goes with it.
             iCallback.onScopeRequestFailed("Package not found")
-            return
+            return@runCatching
           }
           when (action) {
             "approve" -> {
@@ -389,20 +453,48 @@ object VectorService : IDaemonService.Stub() {
             }
             "deny" -> iCallback.onScopeRequestFailed("Request denied by user")
             "delete" -> iCallback.onScopeRequestFailed("Request timeout")
-            "block" -> {
-              val blocked =
-                  PreferenceStore.getModulePrefs("lspd", 0, "config")["scope_request_blocked"]
-                      as? Set<String> ?: emptySet()
-              PreferenceStore.updateModulePref(
-                  "lspd", 0, "config", "scope_request_blocked", blocked + packageName)
-              iCallback.onScopeRequestFailed("Request blocked by configuration")
-            }
           }
         }
         // onScopeRequestFailed declares @NonNull, and Throwable.message is frequently null.
         .onFailure { runCatching { iCallback.onScopeRequestFailed(it.message ?: it.toString()) } }
 
-    NotificationManager.cancelNotification(
-        NotificationManager.SCOPE_CHANNEL_ID, packageName, userId)
+    // Only this one request goes; a module that asked for several packages has a prompt still open
+    // for each of the others, and they are answered on their own.
+    NotificationManager.cancelScopeRequest(packageName, userId, scopePackageName)
+  }
+
+  /**
+   * The modules that may not ask for scope again.
+   *
+   * Filed under "lspd" rather than under the module it names, because it records the user's decision
+   * about a module rather than that module's own configuration. That is also why uninstalling a
+   * module does not take it away — `deleteModulePrefs` deletes what is filed under the module's own
+   * name — and why [unblockScopeRequests] has to exist.
+   */
+  @Suppress("UNCHECKED_CAST")
+  private fun blockedScopeRequests(): Set<String> =
+      PreferenceStore.getModulePrefs("lspd", 0, "config")["scope_request_blocked"] as? Set<String>
+          ?: emptySet()
+
+  private fun blockScopeRequests(packageName: String) {
+    PreferenceStore.updateModulePref(
+        "lspd", 0, "config", "scope_request_blocked", blockedScopeRequests() + packageName)
+  }
+
+  /**
+   * Lets an uninstalled module ask again if it comes back.
+   *
+   * "Never ask again" is a button in a notification, one tap from Approve, and nothing anywhere
+   * undid it: the socket CLI does not know the key, the manager offers no control for it, and the
+   * uninstall path missed it. A module blocked by a mis-tap was blocked on that device forever, and
+   * reinstalling it changed nothing. Uninstalling is a deliberate enough act to count as taking it
+   * back — and a module that is gone has no decision left to honour.
+   */
+  private fun unblockScopeRequests(packageName: String) {
+    val blocked = blockedScopeRequests()
+    if (packageName !in blocked) return
+    PreferenceStore.updateModulePref(
+        "lspd", 0, "config", "scope_request_blocked", blocked - packageName)
+    Log.i(TAG, "$packageName was uninstalled; it may ask for scope again if it returns")
   }
 }

daemon/src/main/kotlin/org/matrix/vector/daemon/data/FileSystem.kt

@@ -204,10 +204,25 @@ object FileSystem {
                     }
                   }
               if (!props.getProperty("staticScope").toBoolean()) return@use null
-              val entry = zip.getEntry("META-INF/xposed/scope.list") ?: return@use emptySet()
-              zip.getInputStream(entry).bufferedReader().useLines { lines ->
-                lines.filter { it.isNotEmpty() }.toSet()
+              val claimed =
+                  zip.getEntry("META-INF/xposed/scope.list")?.let { entry ->
+                    zip.getInputStream(entry).bufferedReader().useLines { lines ->
+                      lines.map { it.trim() }.filter { it.isNotEmpty() }.toSet()
+                    }
+                  } ?: emptySet()
+              // A module that fixes its scope and then names nothing has fixed it at "no apps at
+              // all": every write through here is refused, and pruneScopeToClaimed deletes the rows
+              // the user had already chosen on the next cache rebuild. That is a packaging mistake
+              // rather than an intention — a module ships staticScope=true with a scope.list it
+              // forgot to generate — and the cost of reading it literally is a module that can
+              // never hook anything, silently. So the declaration is ignored and the scope stays
+              // the user's; the manager's ModuleDetection ignores it too, so the picker it draws
+              // and the writes accepted here agree about what the module may hook.
+              if (claimed.isEmpty()) {
+                Log.w(TAG, "$apkPath fixes its scope but names nothing; ignoring staticScope")
+                return@use null
               }
+              claimed
             }
           }
           .onFailure { Log.w(TAG, "Cannot read the scope list of $apkPath", it) }

daemon/src/main/kotlin/org/matrix/vector/daemon/data/ModuleDatabase.kt

@@ -4,6 +4,7 @@ import android.content.ContentValues
 import android.database.sqlite.SQLiteDatabase
 import android.util.Log
 import org.lsposed.lspd.models.Application
+import org.matrix.vector.daemon.system.NotificationManager
 
 private const val TAG = "VectorModuleDatabase"
 
@@ -209,7 +210,30 @@ object ModuleDatabase {
       changed = db.update("modules", values, "module_pkg_name = ?", arrayOf(packageName)) > 0
     }
 
-    if (changed) ConfigCache.requestCacheUpdate()
+    if (changed) {
+      ConfigCache.requestCacheUpdate()
+      // The shade may be telling the user this module "is not activated yet". It has just been
+      // activated, and nothing else was ever going to take that notice down: it is only marked
+      // auto-cancel, which fires when it is tapped, and the sole cancel path belonged to the scope
+      // prompt. The manager cannot do it either — the AIDL exposes no cancel, and a parasitic
+      // manager could not cancel a notification posted as "android" in any case.
+      //
+      // It lives here, at the data layer, rather than in ManagerService because this function is
+      // where the activations that go through the module table converge: the manager's toggle
+      // (ManagerService.enableModule), the socket CLI's `modules enable`, a manager backup restore
+      // — which replays what it read one setModuleEnabled at a time — and setModuleScope's
+      // implicit enable below. Putting it one level up would cover the manager alone and leave the
+      // other three lying to the user. Reaching out of the database for it is the same reach
+      // requestCacheUpdate() above already makes, and for the same reason — a row changed, and
+      // something outside has to be told.
+      //
+      // Not *every* activation, though, and the exception is worth knowing: the socket CLI's
+      // `db restore` copies a whole database file over the live one and calls nothing here, so a
+      // module the restored file has enabled keeps a stale "not activated yet" notice in the shade
+      // until something touches it again. That is a root-shell command that replaces the
+      // configuration wholesale, and the notice is one of several things it does not reconcile.
+      NotificationManager.cancelModuleUpdated(packageName)
+    }
     return changed
   }