Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
provokateurin wants to merge 3 commits into
base: master
Choose a base branch
from
+118 −60
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e8786c
fix(Installer): Actually remove apps from separate apps roots and res…
provokateurin Mar 24, 2025
75b59b9
fix(Installer): Download app into the previous app root and reject if…
provokateurin Mar 24, 2025
f206c60
fix(settings): Rework download/install/update/remove app handling and…
provokateurin Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions apps/settings/lib/Controller/AppSettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,6 @@ public function listApps(): JSONResponse {
}
}
$appData['groups'] = $groups;
$appData['canUnInstall'] = !$appData['active'] && $appData['removable'];

// fix licence vs license
if (isset($appData['license']) && !isset($appData['licence'])) {
Expand Down Expand Up @@ -381,6 +380,14 @@ public function listApps(): JSONResponse {
* @throws \Exception
*/
private function getAppsForCategory($requestedCategory = ''): array {
$anyAppsRootWritable = false;
foreach (\OC::$APPSROOTS as $appsRoot) {
Copy link
Contributor

@come-nc come-nc Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a public getter somewhere for app roots instead of accessing an OC static var from an application.

Also, this logic of testing whether at least one app folder is writable could be in a method of the AppManager directly.

if ($appsRoot['writable'] ?? false) {
$anyAppsRootWritable = true;
break;
}
}

$versionParser = new VersionParser();
$formattedApps = [];
$apps = $this->appFetcher->get();
Expand Down Expand Up @@ -411,11 +418,23 @@ private function getAppsForCategory($requestedCategory = ''): array {
}
$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 = [];
Expand Down Expand Up @@ -482,9 +501,11 @@ private function getAppsForCategory($requestedCategory = ''): array {
'score' => $app['ratingOverall'],
'ratingNumOverall' => $app['ratingNumOverall'],
'ratingNumThresholdReached' => $app['ratingNumOverall'] > 5,
'removable' => $existsLocally,
'active' => $this->appManager->isEnabledForUser($app['id']),
'needsDownload' => !$existsLocally,
'canDownload' => $anyAppsRootWritable,
'canUpdate' => $canUpdate,
Copy link
Member

@nickvergessen nickvergessen Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would remove the crucial part that allows updating apps into another folder when the current one is read-only.
At least in the past we allowed an app to be in multiple app directories and the code simply loaded the one with the highest number in appinfo.xml

Copy link
Member Author

@provokateurin provokateurin Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But unfortunately it's also the case, that some paths are hardcoded to apps/, so I'm not sure that this really worked as intended.
Like I said, app directories are very broken from what I had to dig through.

Copy link
Member

@nickvergessen nickvergessen Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But unfortunately it's also the case, that some paths are hardcoded to apps/

What? Can you provide links?

'canUnInstall' => $canUnInstall && !$this->appManager->isShipped($app['id']),
'active' => $this->appManager->isEnabledForAnyone($app['id']),
'needsDownload' => $needsDownload,
'groups' => $groups,
'fromAppStore' => true,
'appstoreData' => $app,
Expand Down
3 changes: 2 additions & 1 deletion apps/settings/src/app-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion apps/settings/src/components/AppList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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 : []
Expand Down Expand Up @@ -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)
}))
Expand Down
40 changes: 21 additions & 19 deletions apps/settings/src/components/AppList/AppItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down
40 changes: 21 additions & 19 deletions apps/settings/src/components/AppStoreSidebar/AppDetailsTab.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,36 +47,38 @@
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"
type="button"
: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"
Expand Down
3 changes: 0 additions & 3 deletions apps/settings/src/store/apps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
49 changes: 41 additions & 8 deletions lib/private/Installer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
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;
Expand Down Expand Up @@ -176,10 +177,28 @@ private function splitCerts(string $cert): array {
*/
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;
Copy link
Contributor

@come-nc come-nc Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OC_App::getInstallPath is only used in this class it seems, it should be moved into it and deprecated in OC_App. It should also have a better name.

}

$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');
Expand Down Expand Up @@ -322,15 +341,14 @@ public function downloadApp(string $appId, bool $allowUnstable = false): void {
);
}

$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;
}
Expand Down Expand Up @@ -446,11 +464,26 @@ public function isDownloaded(string $name): bool {
*/
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;
}
}
Comment on lines +475 to +481
Copy link
Contributor

@come-nc come-nc Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is duplicated in your PR, please create a method for it.

if (!$appsRootWritable) {
return false;
}

OC_Helper::rmdirr($appPath);
return true;
} else {
$this->logger->error('can\'t remove app ' . $appId . '. It is not installed.');
Expand Down
2 changes: 0 additions & 2 deletions lib/private/legacy/OC_App.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,10 +510,8 @@ public function listAllApps(): array {
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)) {
Expand Down
Loading