fix(settings): Rework download/install/update/remove app handling and respect read-only app roots

Signed-off-by: provokateurin <kate@provokateurin.de>

apps/settings/lib/Controller/AppSettingsController.php

@@ -347,7 +347,6 @@ class AppSettingsController extends Controller {
 				}
 			}
 			$appData['groups'] = $groups;
-			$appData['canUnInstall'] = !$appData['active'] && $appData['removable'];
 
 			// fix licence vs license
 			if (isset($appData['license']) && !isset($appData['licence'])) {
@@ -381,6 +380,14 @@ class AppSettingsController extends Controller {
 	 * @throws \Exception
 	 */
 	private function getAppsForCategory($requestedCategory = ''): array {
+		$anyAppsRootWritable = false;
+		foreach (\OC::$APPSROOTS as $appsRoot) {
+			if ($appsRoot['writable'] ?? false) {
+				$anyAppsRootWritable = true;
+				break;
+			}
+		}
+
 		$versionParser = new VersionParser();
 		$formattedApps = [];
 		$apps = $this->appFetcher->get();
@@ -411,11 +418,23 @@ class AppSettingsController extends Controller {
 			}
 			$phpVersion = $versionParser->getVersion($app['releases'][0]['rawPhpVersionSpec']);
 
+			$needsDownload = true;
+			$canUpdate = false;
+			$canUnInstall = false;
+
 			try {
-				$this->appManager->getAppPath($app['id']);
-				$existsLocally = true;
+				$appPath = $this->appManager->getAppPath($app['id']);
+				$needsDownload = false;
+
+				$appRootPath = dirname($appPath);
+				foreach (\OC::$APPSROOTS as $appsRoot) {
+					if ($appsRoot['path'] === $appRootPath) {
+						$appsRootWritable = $appsRoot['writable'] ?? false;
+						$canUpdate = $appsRootWritable;
+						$canUnInstall = $appsRootWritable;
+					}
+				}
 			} catch (AppPathNotFoundException) {
-				$existsLocally = false;
 			}
 
 			$phpDependencies = [];
@@ -482,9 +501,11 @@ class AppSettingsController extends Controller {
 				'score' => $app['ratingOverall'],
 				'ratingNumOverall' => $app['ratingNumOverall'],
 				'ratingNumThresholdReached' => $app['ratingNumOverall'] > 5,
-				'removable' => $existsLocally,
-				'active' => $this->appManager->isEnabledForUser($app['id']),
-				'needsDownload' => !$existsLocally,
+				'canDownload' => $anyAppsRootWritable,
+				'canUpdate' => $canUpdate,
+				'canUnInstall' => $canUnInstall && !$this->appManager->isShipped($app['id']),
+				'active' => $this->appManager->isEnabledForAnyone($app['id']),
+				'needsDownload' => $needsDownload,
 				'groups' => $groups,
 				'fromAppStore' => true,
 				'appstoreData' => $app,

apps/settings/src/app-types.ts

@@ -44,9 +44,10 @@ export interface IAppstoreApp {
 	app_api: boolean
 	active: boolean
 	internal: boolean
-	removable: boolean
 	installed: boolean
+	canDownload: boolean
 	canInstall: boolean
+	canUpdate: boolean
 	canUnInstall: boolean
 	isCompatible: boolean
 	needsDownload: boolean

apps/settings/src/components/AppList.vue

@@ -17,6 +17,7 @@
 					<NcButton v-if="showUpdateAll"
 						id="app-list-update-all"
 						type="primary"
+						:disabled="!canUpdateAny"
 						@click="updateAll">
 						{{ n('settings', 'Update', 'Update all', counter) }}
 					</NcButton>
@@ -194,6 +195,9 @@ export default {
 		showUpdateAll() {
 			return this.hasPendingUpdate && this.useListView
 		},
+		canUpdateAny() {
+			return this.apps.filter(app => app.update && app.canUpdate).length > 0
+		},
 		apps() {
 			// Exclude ExApps from the list if AppAPI is disabled
 			const exApps = this.$store.getters.isAppApiEnabled ? this.appApiStore.getAllApps : []
@@ -324,7 +328,7 @@ export default {
 		updateAll() {
 			const limit = pLimit(1)
 			this.apps
-				.filter(app => app.update)
+				.filter(app => app.update && app.canUpdate)
 				.map((app) => limit(() => {
 					this.update(app.id)
 				}))

apps/settings/src/components/AppList/AppItem.vue

@@ -78,15 +78,15 @@
 			<div v-if="isLoading || isInitializing" class="icon icon-loading-small" />
 			<NcButton v-if="app.update"
 				type="primary"
-				:disabled="installing || isLoading || !defaultDeployDaemonAccessible || isManualInstall"
+				:disabled="installing || isLoading || !defaultDeployDaemonAccessible || isManualInstall || !app.canUpdate"
 				:title="updateButtonText"
 				@click.stop="update(app.id)">
 				{{ t('settings', 'Update to {update}', {update:app.update}) }}
 			</NcButton>
-			<NcButton v-if="app.canUnInstall"
+			<NcButton v-if="app.installed"
 				class="uninstall"
 				type="tertiary"
-				:disabled="installing || isLoading"
+				:disabled="installing || isLoading || !app.canUnInstall"
 				@click.stop="remove(app.id)">
 				{{ t('settings', 'Remove') }}
 			</NcButton>
@@ -95,22 +95,24 @@
 				@click.stop="disable(app.id)">
 				{{ disableButtonText }}
 			</NcButton>
-			<NcButton v-if="!app.active && (app.canInstall || app.isCompatible)"
-				:title="enableButtonTooltip"
-				:aria-label="enableButtonTooltip"
-				type="primary"
-				:disabled="!app.canInstall || installing || isLoading || !defaultDeployDaemonAccessible || isInitializing || isDeploying"
-				@click.stop="enable(app.id)">
-				{{ enableButtonText }}
-			</NcButton>
-			<NcButton v-else-if="!app.active"
-				:title="forceEnableButtonTooltip"
-				:aria-label="forceEnableButtonTooltip"
-				type="secondary"
-				:disabled="installing || isLoading || !defaultDeployDaemonAccessible"
-				@click.stop="forceEnable(app.id)">
-				{{ forceEnableButtonText }}
-			</NcButton>
+			<div v-if="!app.active && (!app.needsDownload || app.canDownload)">
+				<NcButton v-if="app.canInstall || app.isCompatible"
+					:title="enableButtonTooltip"
+					:aria-label="enableButtonTooltip"
+					type="primary"
+					:disabled="installing || isLoading || !defaultDeployDaemonAccessible || isInitializing || isDeploying"
+					@click.stop="enable(app.id)">
+					{{ enableButtonText }}
+				</NcButton>
+				<NcButton v-else
+					:title="forceEnableButtonTooltip"
+					:aria-label="forceEnableButtonTooltip"
+					type="secondary"
+					:disabled="installing || isLoading || !defaultDeployDaemonAccessible"
+					@click.stop="forceEnable(app.id)">
+					{{ forceEnableButtonText }}
+				</NcButton>
+			</div>
 		</component>
 	</component>
 </template>

apps/settings/src/components/AppStoreSidebar/AppDetailsTab.vue

@@ -47,13 +47,13 @@
 						class="update primary"
 						type="button"
 						:value="t('settings', 'Update to {version}', { version: app.update })"
-						:disabled="installing || isLoading || isManualInstall"
+						:disabled="installing || isLoading || isManualInstall || !app.canUpdate"
 						@click="update(app.id)">
-					<input v-if="app.canUnInstall"
+					<input v-if="app.installed"
 						class="uninstall"
 						type="button"
 						:value="t('settings', 'Remove')"
-						:disabled="installing || isLoading"
+						:disabled="installing || isLoading || !app.canUnInstall"
 						@click="remove(app.id, removeData)">
 					<input v-if="app.active"
 						class="enable"
@@ -61,22 +61,24 @@
 						:value="disableButtonText"
 						:disabled="installing || isLoading || isInitializing || isDeploying"
 						@click="disable(app.id)">
-					<input v-if="!app.active && (app.canInstall || app.isCompatible)"
-						:title="enableButtonTooltip"
-						:aria-label="enableButtonTooltip"
-						class="enable primary"
-						type="button"
-						:value="enableButtonText"
-						:disabled="!app.canInstall || installing || isLoading || !defaultDeployDaemonAccessible || isInitializing || isDeploying"
-						@click="enable(app.id)">
-					<input v-else-if="!app.active && !app.canInstall"
-						:title="forceEnableButtonTooltip"
-						:aria-label="forceEnableButtonTooltip"
-						class="enable force"
-						type="button"
-						:value="forceEnableButtonText"
-						:disabled="installing || isLoading"
-						@click="forceEnable(app.id)">
+					<div v-if="!app.active && (!app.needsDownload || app.canDownload)">
+						<input v-if="app.canInstall || app.isCompatible"
+							:title="enableButtonTooltip"
+							:aria-label="enableButtonTooltip"
+							class="enable primary"
+							type="button"
+							:value="enableButtonText"
+							:disabled="installing || isLoading || !defaultDeployDaemonAccessible || isInitializing || isDeploying"
+							@click="enable(app.id)">
+						<input v-else
+							:title="forceEnableButtonTooltip"
+							:aria-label="forceEnableButtonTooltip"
+							class="enable force"
+							type="button"
+							:value="forceEnableButtonText"
+							:disabled="installing || isLoading"
+							@click="forceEnable(app.id)">
+					</div>
 					<NcButton v-if="app?.app_api && (app.canInstall || app.isCompatible)"
 						:aria-label="t('settings', 'Advanced deploy options')"
 						type="secondary"

apps/settings/src/store/apps.js

@@ -87,9 +87,6 @@ const mutations = {
 		const app = state.apps.find(app => app.id === appId)
 		app.active = false
 		app.groups = []
-		if (app.removable) {
-			app.canUnInstall = true
-		}
 		if (app.id === 'app_api') {
 			state.appApiEnabled = false
 		}

lib/private/Installer.php

@@ -18,6 +18,7 @@ use OC\DB\Connection;
 use OC\DB\MigrationService;
 use OC_App;
 use OC_Helper;
+use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\HintException;
 use OCP\Http\Client\IClientService;
@@ -176,10 +177,28 @@ class Installer {
 	 */
 	public function downloadApp(string $appId, bool $allowUnstable = false): void {
 		$appId = strtolower($appId);
+		$appManager = \OCP\Server::get(IAppManager::class);
 
 		$apps = $this->appFetcher->get($allowUnstable);
 		foreach ($apps as $app) {
 			if ($app['id'] === $appId) {
+				try {
+					$appPath = $appManager->getAppPath($appId);
+				} catch (AppPathNotFoundException) {
+					$appPath = OC_App::getInstallPath() . '/' . $appId;
+				}
+
+				$appsRootWritable = false;
+				$appRootPath = dirname($appPath);
+				foreach (\OC::$APPSROOTS as $appsRoot) {
+					if ($appsRoot['path'] === $appRootPath) {
+						$appsRootWritable = $appsRoot['writable'] ?? false;
+					}
+				}
+				if (!$appsRootWritable) {
+					throw new \Exception(sprintf('App %s can not be updated because the app root is not writable.', $appId));
+				}
+
 				// Load the certificate
 				$certificate = new X509();
 				$rootCrt = file_get_contents(__DIR__ . '/../../resources/codesigning/root.crt');
@@ -322,15 +341,14 @@ class Installer {
 						);
 					}
 
-					$baseDir = OC_App::getInstallPath() . '/' . $appId;
 					// Remove old app with the ID if existent
-					OC_Helper::rmdirr($baseDir);
+					OC_Helper::rmdirr($appPath);
 					// Move to app folder
-					if (@mkdir($baseDir)) {
+					if (@mkdir($appPath)) {
 						$extractDir .= '/' . $folders[0];
-						OC_Helper::copyr($extractDir, $baseDir);
+						OC_Helper::copyr($extractDir, $appPath);
 					}
-					OC_Helper::copyr($extractDir, $baseDir);
+					OC_Helper::copyr($extractDir, $appPath);
 					OC_Helper::rmdirr($extractDir);
 					return;
 				}
@@ -446,11 +464,26 @@ class Installer {
 	 */
 	public function removeApp(string $appId): bool {
 		if ($this->isDownloaded($appId)) {
-			if (\OCP\Server::get(IAppManager::class)->isShipped($appId)) {
+			$appManager = \OCP\Server::get(IAppManager::class);
+
+			if ($appManager->isShipped($appId)) {
 				return false;
 			}
-			$appDir = OC_App::getInstallPath() . '/' . $appId;
-			OC_Helper::rmdirr($appDir);
+
+			$appPath = $appManager->getAppPath($appId);
+
+			$appsRootWritable = false;
+			$appRootPath = dirname($appPath);
+			foreach (\OC::$APPSROOTS as $appsRoot) {
+				if ($appsRoot['path'] === $appRootPath) {
+					$appsRootWritable = $appsRoot['writable'] ?? false;
+				}
+			}
+			if (!$appsRootWritable) {
+				return false;
+			}
+
+			OC_Helper::rmdirr($appPath);
 			return true;
 		} else {
 			$this->logger->error('can\'t remove app ' . $appId . '. It is not installed.');

lib/private/legacy/OC_App.php

@@ -510,10 +510,8 @@ class OC_App {
 				if ($appManager->isShipped($app)) {
 					$info['internal'] = true;
 					$info['level'] = self::officialApp;
-					$info['removable'] = false;
 				} else {
 					$info['internal'] = false;
-					$info['removable'] = true;
 				}
 
 				if (in_array($app, $supportedApps)) {