diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt index 23f83e06f..e0e2280b4 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt @@ -385,29 +385,39 @@ object VectorService : IVectorDaemon.Stub() { val packageName = parts[0] val userId = parts[1].toIntOrNull() ?: return - val scopePackageName = data.path?.substring(1) ?: return // remove leading '/' + // Everything the one prompt asked about, in the order it listed them. ',' cannot occur in a + // package name, so this is the list the module named and not a guess at it. + val scopePackageNames = + data.path?.substring(1)?.split(",")?.filter { it.isNotEmpty() } ?: return // strip '/' + if (scopePackageNames.isEmpty()) return 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") + // the module, its user and the set of packages 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, scopePackageNames)) { + Log.d( + TAG, + "Ignoring $action of ${scopePackageNames.joinToString()} for $packageName:" + + " already answered") 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. Nothing is dropped for that any + // more. There used to be a `!callbackBinder.isBinderAlive` return above the claim, exempting + // only "never ask again", and it did more than lose an answer nobody was listening for: an + // approval the user had already given was thrown away, and because the return sat above the + // claim and the cancel, the prompt stayed on screen with live buttons that did nothing for the + // rest of the hour. An approval is a decision about the module's scope and is written down + // whether or not the module is there to hear it; deny and the timeout have nothing to record + // but still have a prompt to take down. What actually fails against a dead module is the + // callback below, and that is caught where it is made. val iCallback = IXposedScopeCallback.Stub.asInterface(callbackBinder) runCatching { // Answered before the requested package is looked up at all, because "never ask again" is @@ -419,12 +429,10 @@ object VectorService : IVectorDaemon.Stub() { // 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 + // The preference only stops the *next* request. A module that asked three times 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. + // answered in its own right, because each of them was asked in its own right. NotificationManager.withdrawScopeRequests(packageName).forEach { pending -> runCatching { pending.onScopeRequestFailed("Request blocked by configuration") } } @@ -437,31 +445,72 @@ object VectorService : IVectorDaemon.Stub() { 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@runCatching - } 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. + // + // Only under "approve", because only an approval has to name something real. Deny + // and the timeout used to be refused here too, so dismissing a prompt for a package + // that had since been uninstalled told the module "Package not found" when what had + // actually happened was that the user turned it down. + val granted = + scopePackageNames.filter { + it == "system" || packageManager?.getPackageInfoCompat(it, 0, userId) != null + } + if (granted.isEmpty()) { + // Logged, because until now this said nothing anywhere: the module was told + // "Package not found", the user was told nothing at all, and the daemon kept no + // record that the press had even arrived. The framework-scope failure above was + // invisible for exactly that reason. + Log.w( + TAG, + "None of ${scopePackageNames.joinToString()} resolve for user $userId;" + + " refusing the scope request of $packageName") + // 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@runCatching + } val scopes = ModuleDatabase.getModuleScope(packageName) ?: mutableListOf() - // Compared against where the row will land, not against the user who asked: the - // framework is stored under user 0 whoever requested it, so for "system" this test - // never matched and every approval appended a duplicate and rewrote the whole table. - val storedUserId = if (scopePackageName == "system") 0 else userId - if (scopes.none { it.packageName == scopePackageName && it.userId == storedUserId }) { - scopes.add( - ScopeEntry().apply { - this.packageName = scopePackageName - this.userId = storedUserId - }) - ModuleDatabase.setModuleScope(packageName, scopes) + var added = false + granted.forEach { scopePackageName -> + // Compared against where the row will land, not against the user who asked: the + // framework is stored under user 0 whoever requested it, so for "system" this test + // never matched and every approval appended a duplicate and rewrote the whole + // table. + val storedUserId = if (scopePackageName == "system") 0 else userId + val present = + scopes.any { it.packageName == scopePackageName && it.userId == storedUserId } + if (!present) { + scopes.add( + ScopeEntry().apply { + this.packageName = scopePackageName + this.userId = storedUserId + }) + added = true + } } - iCallback.onScopeRequestApproved(listOf(scopePackageName)) + // One write for the whole prompt, and none at all when the user approved what the + // module already had. `setModuleScope` replaces the module's rows wholesale and + // enables the module on the way through, so writing per package would rewrite the + // table once per package — leaving a window after each in which the scope is only + // partly what was agreed to — and writing unconditionally would let a module enable + // itself by asking again for what it has. + if (added) ModuleDatabase.setModuleScope(packageName, scopes) + Log.i(TAG, "Approved ${granted.joinToString()} for $packageName on user $userId") + // The packages that were granted, which is what the list in this callback is for. A + // module comparing it against what it asked for can see what it did not get. + iCallback.onScopeRequestApproved(granted) } "deny" -> iCallback.onScopeRequestFailed("Request denied by user") "delete" -> iCallback.onScopeRequestFailed("Request timeout") @@ -470,9 +519,9 @@ object VectorService : IVectorDaemon.Stub() { // onScopeRequestFailed declares @NonNull, and Throwable.message is frequently null. .onFailure { runCatching { iCallback.onScopeRequestFailed(it.message ?: it.toString()) } } - // 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) + // Only this one request goes; a module that asked more than once has a prompt still open for + // each of its other requests, and they are answered on their own. + NotificationManager.cancelScopeRequest(packageName, userId, scopePackageNames) } /** diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt index 8a3bbcd52..bdcbb8339 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt @@ -194,9 +194,26 @@ class ModuleAppService(private val loadedModule: LoadedModule) : IXposedService. ?.distinct() ?: emptyList() } + /** + * One request, one question, one answer. + * + * The AIDL hands over a list and takes a single [IXposedScopeCallback] for it, and the javadoc on + * the client's `OnScopeEventListener` says its listener runs "when the request is completed" — + * singular — with `onScopeRequestApproved` taking the *packages* that were approved. This used to + * put one prompt per package on screen, each answered in its own right, so a module asking for + * three packages made the user answer three questions and then fired that one listener three + * times. A module that took the first answer as the answer acted on a third of it. + * + * So the whole list goes up as one prompt and Approve answers for all of it. What the user gives + * away in one press is what the prompt lists, which is why it lists all of them rather than a + * count, and why the packages are sorted and deduplicated first: it is a set that is being agreed + * to, the same set asked for twice is the same question, and `NotificationManager` identifies a + * prompt by the set it names. + */ override fun requestScope(packages: List, callback: IXposedScopeCallback) { val userId = ensureModule() - if (packages.isEmpty()) { + val requested = packages.distinct().sorted() + if (requested.isEmpty()) { // Nothing was asked for, so the request is trivially satisfied. Returning without touching // the callback would leave the module waiting forever. callback.onScopeRequestApproved(emptyList()) @@ -205,7 +222,7 @@ class ModuleAppService(private val loadedModule: LoadedModule) : IXposedService. // A module that fixed its own scope in module.prop does not get to ask for more of it at // runtime. Prompting the user here would make "fixed" mean nothing. ConfigCache.staticScopeOf(loadedModule.packageName)?.let { claimed -> - val beyond = packages.filterNot { claimed.contains(it) } + val beyond = requested.filterNot { claimed.contains(it) } if (beyond.isNotEmpty()) { callback.onScopeRequestFailed( "This module declares a static scope, so ${beyond.joinToString()} cannot be added") @@ -213,9 +230,7 @@ class ModuleAppService(private val loadedModule: LoadedModule) : IXposedService. } } if (!PreferenceStore.isScopeRequestBlocked(loadedModule.packageName)) { - packages.forEach { pkg -> - NotificationManager.requestModuleScope(loadedModule.packageName, userId, pkg, callback) - } + NotificationManager.requestModuleScope(loadedModule.packageName, userId, requested, callback) } else { callback.onScopeRequestFailed("Scope request blocked by user configuration") } diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt index 602ab3883..8eaa9574d 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt @@ -41,20 +41,18 @@ private const val SCOPE_REQUEST_TIMEOUT_MS = 60L * 60 * 1000 /** * How many prompts one module may have waiting for an answer at once. * - * `IXposedService.requestScope` takes an unbounded list and there is now one prompt per package in - * it, none of them deduplicated beyond exact string equality and none checked for existence before - * it goes up. Nothing else bounds them: these are enqueued as "android", which - * NotificationManagerService exempts from its per-package limit, and they are IMPORTANCE_HIGH, so a - * module asking for a thousand packages gets a thousand heads-up prompts that each sit for - * [SCOPE_REQUEST_TIMEOUT_MS]. That was hidden before this branch only because every prompt of a - * module shared one tag and so replaced the one before it — which is the bug this branch fixed. + * Nothing else bounds them: these are enqueued as "android", which NotificationManagerService + * exempts from its per-package limit, and they are IMPORTANCE_HIGH, so a module calling + * `IXposedService.requestScope` in a loop would get a heads-up prompt per call, each sitting for + * [SCOPE_REQUEST_TIMEOUT_MS]. It is a *call* that costs a place, not a package: one request is one + * prompt however many packages it names, which is what makes a single Approve able to answer for + * all of them. * * Sixteen because it is far above anything an honest module asks for in one go, and low enough that * the worst a module can do to the shade is a screenful. It bounds what is *unanswered*, not what * may be asked over time: answering a prompt frees its place at once, so a module that asks a few - * questions and waits for them never meets it. A module that does meet it is told so per package - * rather than left waiting, because a callback that never fires is the failure this whole path - * exists to avoid. + * questions and waits for them never meets it. A module that does meet it is told so rather than + * left waiting, because a callback that never fires is the failure this whole path exists to avoid. */ private const val MAX_OPEN_SCOPE_REQUESTS_PER_MODULE = 16 @@ -161,9 +159,14 @@ object NotificationManager { * that each replaced the one before it: only the last request was ever answerable, the earlier * ones were never granted and their callbacks were never called at all. One module running under * two users collided in exactly the same way. + * + * The requested packages are named as a set rather than one at a time, because one call to + * `requestScope` is one prompt. Canonical order is the caller's job — see `ModuleAppService`, + * which sorts — so that the same request asked twice lands on the same tag and replaces its own + * prompt instead of stacking a second copy of the same question. */ - private fun scopeTag(modulePkg: String, moduleUserId: Int, scopePkg: String) = - "$modulePkg:$moduleUserId:$scopePkg" + private fun scopeTag(modulePkg: String, moduleUserId: Int, scopePkgs: List) = + "$modulePkg:$moduleUserId:${scopePkgs.joinToString(",")}" /** Cancels what we posted under [tag]; the id is derived from it exactly as it is on enqueue. */ private fun cancelByTag(tag: String) { @@ -179,13 +182,13 @@ object NotificationManager { } /** - * Takes down the prompt for one (module, user, requested package) once it has been answered. + * Takes down the prompt for one (module, user, requested set) once it has been answered. * - * It has to name the requested package, because a module asking for several has one prompt per - * package and answering one of them must not clear the rest. + * It has to name the requested set, because a module that asked twice for different sets has a + * prompt for each and answering one of them must not clear the other. */ - fun cancelScopeRequest(modulePkg: String, moduleUserId: Int, scopePkg: String) = - cancelByTag(scopeTag(modulePkg, moduleUserId, scopePkg)) + fun cancelScopeRequest(modulePkg: String, moduleUserId: Int, scopePkgs: List) = + cancelByTag(scopeTag(modulePkg, moduleUserId, scopePkgs)) /** * The "not activated yet" half of [notifyModuleUpdated], which is the half that can go stale. @@ -310,20 +313,20 @@ object NotificationManager { } /** - * Claims the right to answer the prompt for one (module, user, requested package). + * Claims the right to answer the prompt for one (module, user, requested set). * * @return true for the first caller, false for every later one — the module's * [IXposedScopeCallback] must be called exactly once per request. */ - fun claimScopeAnswer(modulePkg: String, moduleUserId: Int, scopePkg: String) = - OutstandingScopeRequests.claim(scopeTag(modulePkg, moduleUserId, scopePkg)) != null + fun claimScopeAnswer(modulePkg: String, moduleUserId: Int, scopePkgs: List) = + OutstandingScopeRequests.claim(scopeTag(modulePkg, moduleUserId, scopePkgs)) != null /** * Withdraws every prompt [modulePkg] still has on screen and hands back their callbacks, so the * caller can tell each of those requests it will not be granted. * - * What makes "never ask again" mean what it says. A module that asked for three packages now has - * a prompt for each of them; answering the user's "stop asking" by leaving two more questions on + * What makes "never ask again" mean what it says. A module that asked three times has a prompt + * for each of those requests; answering the user's "stop asking" by leaving two more questions on * screen — both still approvable — would be answering it with the opposite. * * They are claimed before they are cancelled, and that order is load-bearing, though not for the @@ -338,16 +341,14 @@ object NotificationManager { * moment, would otherwise answer a request we are in the middle of withdrawing. Claiming first * makes every one of those arrive at a closed door. * - * Each withdrawn request is handed back on its own, so the caller reports one failure per - * package. That is the honest reading — the module named each package separately and each was - * asked in its own right — but it is worth knowing what it costs: `requestScope` supplies one - * callback for the whole list, so those failures all land on the same binder, and the shipped - * client library does not collapse them. Its `OnScopeEventListener` wrapper calls the listener - * every time and merely drops its map entry afterwards, so a module written to the singular - * javadoc ("invoked when the request is completed") runs its handler once per package rather than - * once per call. Reporting the withdrawal once would be the other defensible choice, but it would - * have to pick one of the packages to name and would leave the rest with no answer at all, which - * is exactly the failure this map exists to prevent. + * Each withdrawn request is handed back on its own, and that is now one failure per + * `requestScope` call rather than one per package. It used to be per package, which put a module + * in an awkward spot: `requestScope` supplies one callback for the whole list, those failures all + * landed on the same binder, and the shipped client library does not collapse them — its + * `OnScopeEventListener` wrapper calls the listener every time and merely drops its map entry + * afterwards, so a module written to the singular javadoc ("invoked when the request is + * completed") ran its handler once per package. Batching the prompt is what fixed that, here and + * everywhere else on this path: the answer is now shaped like the question the module asked. */ fun withdrawScopeRequests(modulePkg: String): List { val withdrawn = OutstandingScopeRequests.claimAllOf(modulePkg) @@ -373,10 +374,10 @@ object NotificationManager { fun requestModuleScope( modulePkg: String, moduleUserId: Int, - scopePkg: String, + scopePkgs: List, callback: IXposedScopeCallback ) { - val tag = scopeTag(modulePkg, moduleUserId, scopePkg) + val tag = scopeTag(modulePkg, moduleUserId, scopePkgs) // Registered before the notification is built, let alone posted: the buttons are live from the // moment the platform accepts it, and a prompt the receiver does not know about is one whose // answer it drops. Registering is also what enforces the ceiling, so there is no point @@ -386,14 +387,13 @@ object NotificationManager { Log.w( TAG, "$modulePkg is already waiting on $MAX_OPEN_SCOPE_REQUESTS_PER_MODULE scope prompts;" + - " not asking about $scopePkg") - // Refused, not ignored. The module is told about this package rather than left holding a - // callback that can never fire, and the message names the package so a module developer can - // see which of their list did not make it. + " not asking about ${scopePkgs.joinToString()}") + // Refused, not ignored. The module is told rather than left holding a callback that can + // never fire, and the message names what did not make it so a module developer can see it. runCatching { callback.onScopeRequestFailed( "Too many scope requests are already waiting for an answer from the user," + - " so $scopePkg was not asked about") + " so ${scopePkgs.joinToString()} was not asked about") } .onFailure { Log.w(TAG, "Could not tell $modulePkg its request was refused", it) } return @@ -415,7 +415,12 @@ object NotificationManager { Uri.Builder() .scheme("module") .encodedAuthority("$modulePkg:$moduleUserId") - .encodedPath(scopePkg) + // The whole list, because one request is one prompt and one answer. ',' is a + // legal path character and cannot occur in a package name, so the receiver can + // split it back apart; it is also what keeps two requests naming different sets + // on separate PendingIntents, which are identified by their intent and not by + // the extras that carry the callback. + .encodedPath(scopePkgs.joinToString(",")) .appendQueryParameter("action", actionParams) .build() putExtras(Bundle().apply { putBinder("callback", callback.asBinder()) }) @@ -432,7 +437,10 @@ object NotificationManager { .setContentTitle(context.getString(R.string.xposed_module_request_scope_title)) .setContentText( context.getString( - R.string.xposed_module_request_scope_content, modulePkg, userName, scopePkg)) + R.string.xposed_module_request_scope_content, + modulePkg, + userName, + scopePkgs.joinToString())) .setSmallIcon(getNotificationIcon()) .addAction( Notification.Action.Builder( @@ -466,7 +474,14 @@ object NotificationManager { R.string.xposed_module_request_scope_content, modulePkg, userName, - scopePkg))) + // The whole list, wrapped over as many lines as it takes. Approve + // answers for all of it at once, so all of it is what the user is + // agreeing to; the collapsed line above is one line whatever we put in + // it, and this is where a request naming more packages than fit there + // becomes readable. Comma-separated rather than one per line because + // the string this fills is a sentence and the list sits mid-way + // through it. + scopePkgs.joinToString()))) .build() .apply { extras.putString("android.substName", BuildConfig.FRAMEWORK_NAME) }