Refuse a scope target that reaches nothing, and name the framework

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

@@ -276,7 +276,15 @@ class ModulesCommand {
   }
 }
 
-@Command(name = "scope", description = ["Manage granular application injection scopes"])
+@Command(
+    name = "scope",
+    description =
+        [
+            "Manage granular application injection scopes",
+            "A target is a package name, optionally `package/user_id`.",
+            "The system server is `system`; `android` is the framework package, whose",
+            "injectable process is android:ui.",
+        ])
 class ScopeCommand {
   @ParentCommand lateinit var parent: Cli
 
@@ -286,7 +294,7 @@ class ScopeCommand {
     return OutputFormatter.print(VectorIPC.transmit(req), parent.json)
   }
 
-  @Command(name = "add", description = ["Append apps to scope (format: pkg/user_id)"])
+  @Command(name = "add", description = ["Append apps to scope (format: pkg/user_id, or `system`)"])
   fun add(
       @Parameters(index = "0", paramLabel = "MODULE_PKG") modulePkg: String,
       @Parameters(index = "1..*") apps: List<String>
@@ -295,7 +303,9 @@ class ScopeCommand {
     return OutputFormatter.print(VectorIPC.transmit(req), parent.json)
   }
 
-  @Command(name = "set", description = ["Overwrite entire scope (format: pkg/user_id)"])
+  @Command(
+      name = "set",
+      description = ["Overwrite entire scope (format: pkg/user_id, or `system`)"])
   fun set(
       @Parameters(index = "0", paramLabel = "MODULE_PKG") modulePkg: String,
       @Parameters(index = "1..*") apps: List<String>
@@ -304,7 +314,7 @@ class ScopeCommand {
     return OutputFormatter.print(VectorIPC.transmit(req), parent.json)
   }
 
-  @Command(name = "rm", description = ["Remove apps from scope (format: pkg/user_id)"])
+  @Command(name = "rm", description = ["Remove apps from scope (format: pkg/user_id, or `system`)"])
   fun rm(
       @Parameters(index = "0", paramLabel = "MODULE_PKG") modulePkg: String,
       @Parameters(index = "1..*") apps: List<String>

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

@@ -495,12 +495,6 @@ object VectorService : IVectorDaemon.Stub() {
 
           when (action) {
             "approve" -> {
-              // "system" is the framework and not a package: it names system_server, which belongs
-              // to no package and resolves for nobody. The lookup below therefore came back null
-              // for every framework prompt, and the approval the user had just given was answered
-              // "Package not found" — the request was closed, its notification cancelled, and no
-              // row written. The normalisation to user 0 further down could never once have run.
-              //
               // Package by package rather than all-or-nothing: the prompt may have been up for an
               // hour and one of the packages it named can have been uninstalled in the meantime,
               // which is no reason to throw away the user's answer about the rest.
@@ -511,7 +505,7 @@ object VectorService : IVectorDaemon.Stub() {
               // actually happened was that the user turned it down.
               val granted =
                   scopePackageNames.filter {
-                    it == "system" || packageManager?.getPackageInfoCompat(it, 0, userId) != null
+                    ConfigCache.isScopeTargetAvailable(it, userId) == true
                   }
               if (granted.isEmpty()) {
                 // Logged, because until now this said nothing anywhere: the module was told

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

@@ -42,6 +42,31 @@ object ConfigCache {
   /** The packages [modulePackage] claims, or null when it does not fix its scope. */
   fun staticScopeOf(modulePackage: String): Set<String>? = staticScopes[modulePackage]
 
+  /**
+   * Whether a scope row naming [packageName] under [userId] points at something that exists, or
+   * null when there is no package manager to ask.
+   *
+   * Three answers rather than two, for the reason the rebuild below refuses an empty user list:
+   * not being able to ask is not the same as being told no, and the CLI socket is listening before
+   * `main` has waited for the package service. A caller that acts on `false` must not act on
+   * `null`.
+   *
+   * `system` is the framework and not a package: it names the system server, which belongs to no
+   * package and resolves for nobody, so the package manager is the wrong thing to ask about it.
+   * Every other target is an ordinary package name, and a row naming one that is not installed
+   * reaches nothing — there is no process for the rebuild below to expand it into, and nothing
+   * anywhere said so.
+   *
+   * `isPackageAvailable` and not a plain `getPackageInfo`, because that is the predicate
+   * [getInstalledPackagesFromAllUsers] filters the manager's own list by: what the editor offers
+   * and what a write is allowed to name then mean one thing, hidden packages included.
+   */
+  fun isScopeTargetAvailable(packageName: String, userId: Int): Boolean? {
+    if (packageName == "system") return true
+    val pm = packageManager ?: return null
+    return pm.isPackageAvailable(packageName, userId, true)
+  }
+
   private val cacheUpdateChannel = Channel<Unit>(Channel.CONFLATED)
 
   // Held for the whole of a rebuild, so that only one runs at a time. The channel serialises the
@@ -376,6 +401,20 @@ object ConfigCache {
         val appUid = pkgInfo.applicationInfo!!.uid
 
         for (processName in processNames) {
+          // `system` at uid 1000 is the system server, and its module set is not read from here:
+          // `getModulesForSystemServer` asks the scope table for the rows spelled `system`,
+          // because it answers before any package manager exists to expand a package name with.
+          // So a row naming a package that runs a component in the system server expands to an
+          // entry nothing ever queries, and the module appeared to hold the framework while never
+          // being loaded into it. The entry goes; the row is still reported, so it can be removed.
+          if (processName == "system" && appUid == 1000) {
+            Log.w(
+                TAG,
+                "$appPkg runs a component in the system server, which is scoped as `system`; " +
+                    "the row naming $appPkg cannot take $modPkg there")
+            continue
+          }
+
           addToScope(processName, appUid, module)
 
           // A module in its own scope hooks itself in every user that has it — the copies share

daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/CliHandler.kt

@@ -104,18 +104,46 @@ object CliHandler {
     val modulePkg = request.targets[0]
     val apps = request.targets.drop(1)
 
+    // `pkg` or `pkg/user_id`, parsed once for every action: the three that take targets each
+    // spelled this out, and the check below has to read a target exactly as the write does.
+    fun target(appStr: String): ScopeEntry =
+        ScopeEntry().apply {
+          packageName = appStr.substringBefore('/')
+          userId = appStr.substringAfter('/', "").toIntOrNull() ?: 0
+        }
+
     // staticScope fixes the scope, so the CLI may still read it and take apps out of it, but not
     // put new ones in. Checked here as well as in the database so the answer names the packages.
-    fun rejectBeyondStaticScope(apps: List<String>) {
+    fun rejectBeyondStaticScope(targets: List<ScopeEntry>) {
       val claimed = ConfigCache.staticScopeOf(modulePkg) ?: return
-      val beyond = apps.map { it.substringBefore('/') }.filterNot { claimed.contains(it) }
+      val beyond = targets.map { it.packageName }.filterNot { claimed.contains(it) }
       if (beyond.isNotEmpty()) {
         throw IllegalArgumentException(
             "$modulePkg fixes its scope in module.prop, so ${beyond.joinToString()} cannot be " +
                 "added. It claims: ${claimed.sorted().joinToString().ifEmpty { "nothing" }}.")
       }
     }
 
+    // A target the daemon cannot reach is refused rather than written. Nothing checked, and the
+    // scope table takes any string: a name that resolves for nobody was accepted, listed back by
+    // `scope ls`, and reached nothing, which is a module that is configured and never loads with
+    // no error anywhere to say why. `scope rm` deliberately does not check, since a row already
+    // written this way has to stay removable.
+    //
+    // Refused on `false` alone and never on a null: this socket answers before the daemon has
+    // waited for the package service, and in that window nothing can be told apart from nothing.
+    fun rejectUnreachable(targets: List<ScopeEntry>) {
+      val unknown =
+          targets
+              .filter { ConfigCache.isScopeTargetAvailable(it.packageName, it.userId) == false }
+              .map { "${it.packageName}/${it.userId}" }
+      if (unknown.isNotEmpty()) {
+        throw IllegalArgumentException(
+            "Not installed: ${unknown.joinToString()}. The system server is scoped as `system`; " +
+                "every other target is a package name, optionally `package/user_id`.")
+      }
+    }
+
     return when (request.action) {
       "ls" -> {
         val scope =
@@ -125,19 +153,14 @@ object CliHandler {
       }
       "add" -> {
         if (apps.isEmpty()) throw IllegalArgumentException("No target apps provided.")
-        rejectBeyondStaticScope(apps)
+        val targets = apps.map(::target)
+        rejectBeyondStaticScope(targets)
+        rejectUnreachable(targets)
         val scope = ModuleDatabase.getModuleScope(modulePkg) ?: mutableListOf()
 
-        apps.forEach { appStr ->
-          val parts = appStr.split("/")
-          val pkg = parts[0]
-          val user = parts.getOrNull(1)?.toIntOrNull() ?: 0
-          if (scope.none { it.packageName == pkg && it.userId == user }) {
-            scope.add(
-                ScopeEntry().apply {
-                  packageName = pkg
-                  userId = user
-                })
+        targets.forEach { entry ->
+          if (scope.none { it.packageName == entry.packageName && it.userId == entry.userId }) {
+            scope.add(entry)
           }
         }
         ModuleDatabase.setModuleScope(modulePkg, scope)
@@ -146,29 +169,19 @@ object CliHandler {
       "set" -> {
         if (apps.isEmpty())
             throw IllegalArgumentException("No target apps provided for scope overwrite.")
-        rejectBeyondStaticScope(apps)
-        val scope = mutableListOf<ScopeEntry>()
-        apps.forEach { appStr ->
-          val parts = appStr.split("/")
-          val pkg = parts[0]
-          val user = parts.getOrNull(1)?.toIntOrNull() ?: 0
-          scope.add(
-              ScopeEntry().apply {
-                packageName = pkg
-                userId = user
-              })
-        }
-        ModuleDatabase.setModuleScope(modulePkg, scope)
+        val targets = apps.map(::target)
+        rejectBeyondStaticScope(targets)
+        rejectUnreachable(targets)
+        ModuleDatabase.setModuleScope(modulePkg, targets.toMutableList())
         "Successfully overwrote scope for $modulePkg (${apps.size} apps)."
       }
       "rm" -> {
         if (apps.isEmpty()) throw IllegalArgumentException("No target apps provided to remove.")
         var removedCount = 0
-        apps.forEach { appStr ->
-          val parts = appStr.split("/")
-          val pkg = parts[0]
-          val user = parts.getOrNull(1)?.toIntOrNull() ?: 0
-          if (ModuleDatabase.removeModuleScope(modulePkg, pkg, user)) removedCount++
+        apps.map(::target).forEach { entry ->
+          if (ModuleDatabase.removeModuleScope(modulePkg, entry.packageName, entry.userId)) {
+            removedCount++
+          }
         }
         "Successfully removed $removedCount apps from $modulePkg scope."
       }