Hide overwrites from disabled apps list on upgrade

[MichaIng]

If an incompatible app is enabled manually, it is added to the "app_install_overwrite" array in config.php. Nextcloud upgrades won't disable any app in this array, but they were still shown on the upgrade page as being disabled.

This commit assures that only apps are shown as "These incompatible apps will be disabled:" which are really disabled, i.e. which are not in the "app_install_overwrite" array.

[szaimen]

Hiding app overwrite on minor updates is fine, imo. For major updates, app overwrites should be deleted, though, ihmo.

[MichaIng]

For major updates, app overwrites should be deleted, though, ihmo.

This actually makes sense. Hmm, this would need to be done as an additional migration step, I guess? The array would then not be updated yet when this code runs, so we'd need to additionally check for a major upgrade and then change this code like:

if ($isMajorUpgrade || !in_array($appInfo['name'], $incompatibleOverwrites)) {

with $isMajorUpgrade being a boolean depending on whether $installedVersion and $currentVersion differ in major version number or not. I can do that once the overwrite deletion step is added.

[skjnldsv]

Hiding app overwrite on minor updates is fine, imo. For major updates, app overwrites should be deleted, though, ihmo.

This is very important, can you raise that today at the server call @szaimen ? :)

[szaimen]

This is very important, can you raise that today at the server call @szaimen ? :)

I can try :)

[icewind1991]

we can change the behavior around disabled apps on major upgrades in a separate PR

[MichaIng]

Just tested on upgrade from NC26 Beta 4 to Beta 5. Before, the admin panel shows:

Apps missing compatible version

• Impersonate ↗
• Notes ↗
• Ransomware protection ↗

Nextcloud config:

"app_install_overwrite": [
    "ransomware_protection",
    "contacts",
    "calendar",
    "tasks",
    "spreed",
    "notify_push"
],

After applying the first upgrade stage, the web interface shows:

Diese inkompatiblen Apps werden deaktiviert:

• Impersonate (impersonate)
• Ransomware protection (ransomware_protection)

Applying the patch:

cd /var/www/nextcloud
curl -sSfL https://github.com/nextcloud/server/pull/29988.patch | patch -p1

Afterwards:

Diese inkompatiblen Apps werden deaktiviert:

• Impersonate (impersonate)

Since I enabled the incompatible ransomware_protection app in the past, it has been added to app_install_overwrite in config.php and hence it is not disabled on Nextcloud upgrade and now as well now falsely shown anymore in the above list. The impersonate app has not been enabled as incompatible app, is hence not in this array and as such shown as + correctly disabled on upgrade.

So that part works, BUT after the upgrade finished, the message again is shown wrong, I'll look where this is applied:

Folgende Apps wurden deaktiviert: impersonate (inkompatibel), ransomware_protection (inkompatibel)

[MichaIng]

I don't get it. The list after the upgrade has finished is generated here, based on incompatibleAppDisabled events: https://github.com/nextcloud/server/blob/6eb08ef/core/ajax/update.php#L163-L165

Same for the CLI upgrade: https://github.com/nextcloud/server/blob/6eb08ef/core/Command/Upgrade.php#L194-L196

The event is emitted here, ONLY if $appManager=>disableApp() runs before: https://github.com/nextcloud/server/blob/6eb08ef/lib/private/Updater.php#L373-L397

disableApp is defined here, and I do not see any way how apps in the override array are excluded, but furthermore apps even seem to be uninstalled(?): https://github.com/nextcloud/server/blob/6eb08ef/lib/private/App/AppManager.php#L381-L414

But in fact apps in this array are NOT disabled after the upgrade. So either I missed something or those apps are first disabled, but later re-enabled, in which case we'd need to add an additional event listener which again removes apps which are re-enabled from the finally printed list. Or better, apps which are going to be re-enabled anyway, are not disabled in the first place.

[MichaIng]

Found it:

• All app store apps, including automatically disabled apps (i.e. the incompatible ones), are upgraded here: https://github.com/nextcloud/server/blob/6eb08efe430c3c80f41616cc1c9c2c1fb3ccd8aa/lib/private/Updater.php#L280-L285
• The automatically disabled/incompatible ones are re-enabled afterwards here, regardless whether an upgrade was applied or not: https://github.com/nextcloud/server/blob/6eb08efe430c3c80f41616cc1c9c2c1fb3ccd8aa/lib/private/Updater.php#L426-L433
• This calls the legacy function, which installs and enables the app: https://github.com/nextcloud/server/blob/6eb08efe430c3c80f41616cc1c9c2c1fb3ccd8aa/lib/private/legacy/OC_App.php#L451-L466
• The install function throws an exception, if the incompatible app is not in the overwrite array, and I can see this error in my logs for the impersonate app but not for the ransomware protection app, accordingly: https://github.com/nextcloud/server/blob/6eb08efe430c3c80f41616cc1c9c2c1fb3ccd8aa/lib/private/Installer.php#L126-L137
• The legacy enable function would now call the related modern interface to enable the app. But when installApp throws the exception, I guess the legacy enable function returns as well immediately: https://github.com/nextcloud/server/blob/6eb08efe430c3c80f41616cc1c9c2c1fb3ccd8aa/lib/private/App/AppManager.php#L313-L327

It looks like there all could benefit from some cleanup, essentially eliminating the legacy OC_App.enable() call and by this the redundant app install for apps which are installed already, and doing the app_install_overwrite check for whether to enable the app or not right in the Updater.php. Or probably better not disabling incompatible apps in this array in the first place, but maybe there is a reason for this?

Also I'm not sure how to best for the list of disabled incompatible apps at the end of the upgrade. The array used currently is generated based on thrown events when these apps are disabled, but there is no event for the case that apps are afterwards re-enabled, and it seems wrong to implement them into the legacy OC_App functions. Least intrusive seems to adjust the list simply based on whether the app is finally enabled or not, or whether it is in the array or not. Will go this route for now, and maybe I find time in the future to cleanup this whole thing properly.

[MichaIng]

Next test NC26 beta 5 => RC1

  'app_install_overwrite' =>
  array (
    0 => 'ransomware_protection',
    1 => 'contacts',
    2 => 'calendar',
    3 => 'tasks',
    4 => 'spreed',
    5 => 'notify_push',
  ),

Admin panel

Apps missing compatible version AppOrder ↗ Notes ↗ Ransomware protection ↗

Here is a bug in the compatibility test here. While the Notes version has not been changed, it has a Nextcloud 26 archive in the store. The admin panel shows it as incompatible regardless, while the updater does not list it as such and does not need an entry in the override array above to keep it active.

Updater start

Diese inkompatiblen Apps werden deaktiviert:

• AppOrder (apporder)

Updater end

Folgende Apps wurden deaktiviert: apporder (inkompatibel)

Logs (not changed with this PR)

Level	App	Message
Info	updater	\OC\Updater::incompatibleAppDisabled: Disabled incompatible app: apporder
Info	updater	\OC\Updater::incompatibleAppDisabled: Disabled incompatible app: ransomware_protection
Debug	updater	\OC\Updater::checkAppStoreAppBefore: Checking for update of app "apporder" in appstore
Debug	updater	\OC\Updater::checkAppStoreApp: Checked for update of app "apporder" in appstore
Error	no app in context	Exception: App "AppOrder" cannot be installed because it is not compatible with this version of the server.
Debug	updater	\OC\Updater::checkAppStoreAppBefore: Checking for update of app "ransomware_protection" in appstore
Debug	updater	\OC\Updater::checkAppStoreApp: Checked for update of app "ransomware_protection" in appstore
Info	updater	OC\Repair\Events\RepairStepEvent: Repair step: Repair MySQL collation

As good as it gets without a larger update of the updater. No false information shown anywhere now 👍.

[nickvergessen]

Regarding app_install_overwrite: from my pov that thing is problematic in it's current way. It should either be an array per major version or apps should be removed once a compatible version was installed. Because just because you force enabled the app on the upgrade 24->25 that doesn't mean you want to do the same for 25->26

[MichaIng]

Personally I always see the same apps not declaring latest NC version compatibility on upgrade, so that I like the overrides staying in place. I never had an issue with it. The only app incompatibility issue I remember was with Nextcloud Office when it did already declare NC25 compatibility but lead to a 5xx after upgrade 😄 (fixed in the meantime).

However, generally I agree. As discussed above already, this PR is about fixing the info the updater shows, not changing the behaviour, which I'd like to do in a dedicated PR. But first I'd rework relevant code parts to:

• switch form legacy OC_App to direct AppManager use
• not disable (and as code states even uninstall) incompatible apps first, only to (reinstall and) re-enable them later when in the array; The current behaviour has the effect of re-enabling apps for all groups which were previously restricted for specific groups, AFAIK.

So the app is either kept enabled or disabled at one place during the update and remains like that, cleaning up round trips and related log entries. Another PR can then hook in there, remove apps from app_install_overwrite and have them disabled on major NC upgrades accordingly.

[MichaIng]

Any idea what "Conventional Commits" does not like about the commits text? I added type and scope, so that should be fine: https://www.conventionalcommits.org/en/v1.0.0/

Probably something for the dev documentation, and probably the action can be configured to show the actual tests/rules which have been failed, to give any hint.

[skjnldsv]

Any idea what "Conventional Commits" does not like about the commits text? I added type and scope, so that should be fine: https://www.conventionalcommits.org/en/v1.0.0/

Enh is not valid. Feat is :)

[MichaIng]

Ah, it says "types other than fix: and feat: are allowed", so I thought any type is valid. But it seams that it is one of this array by default: https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional#type-enum

This PR is not really a (new) feature, I'd say, but there is no better matching type indeed 🙂. EDIT: Jep, that was it, thanks!