nkakouros-original / ansible-role-nextcloud

An Ansible role to install Nextcloud
GNU General Public License v3.0
11 stars 5 forks source link

Integrity check shows error even if no errors are there #8

Open nkakouros opened 6 years ago

nkakouros commented 6 years ago

Sometimes, even after the role has taken care of integrity errors, an error message is shown in the admin interface. If I click rescan, the error is gone.

simonspa commented 4 years ago

I also experience this - the problem seems to be that the occ integrity:check-app command does not seem to find the signature data when called from the command line (even manually):

# sudo -u www-data php occ integrity:check-app apporder
  - EXCEPTION:
    - class: OC\IntegrityCheck\Exceptions\InvalidSignatureException
    - message: Signature data not found.

which is also what we get in the return value from ansible. We however don't parse the result and check for success. I also see that when rescanning from the Admin panel, it works just fine.

I could not find a nextcloud bug report for this behavior - does this have to do with the way we install apps? Rught now we just download and unzip, in principle we could use occ app:install:

https://docs.nextcloud.com/server/16/admin_manual/configuration_server/occ_command.html#apps-commands-label

nkakouros commented 4 years ago

It could very well be the way the role installs the apps. Again, I do not remember why I chose this way of installing apps. That part of the role was written against version 12, I think, that according to the documentation did not have app:install or app:update, so that must have been the reason. We could therefore rewrite that part using those commands. There might be more parts of the role that could be simplified using occ commands that were added in later versions.

If we do so, perhaps the integrity check will be gone. It will also make the role's logic less dependent on the internal implementation of Nextcloud (getting a json file, parsing it, etc which might change in the future). Yet, the option for the user to specify a specific app version to download will also be gone. I haven't found an option in app:install or app:update to specify a version of the app to download other that the latest.

Of course, we could try to find a way to have the integrity checks succeed with the role as it is now.

I am not sure what the best approach is.

simonspa commented 4 years ago

Indeed the app:install command is quite basic: https://github.com/nextcloud/server/blob/2cc411862912b2e890c0d500b5ba1ce13a6013b6/core/Command/App/Install.php

What we could still retain despite switching to occ app:install is to retain a certain version of the app, i.e. do not update it. But for initial installation we can only get the most up-to-date one this way.

For me this wouldn't matter that much, not sure if others would have this as requirement.

I just noticed that currently updating seems to be completely broken, I'm still trying to find out why.

simonspa commented 4 years ago

I dug a bit around in the server code, thinking we could maybe add the possibility to install different version - but they filter for the newest the lowest level already, namely when they fetch the information from the store: https://github.com/nextcloud/server/blob/18d85d46d19ab5b8629fc3b0f46518e3a4ff240b/lib/private/App/AppStore/Fetcher/AppFetcher.php#L121

I guess a PR changing this would not be welcome.

nkakouros commented 4 years ago

I guess so too. I suggest that we leave the functionality as is until it breaks due to nextcloud changing the internal implementation that we replicate. At that point we will have to make a decision again, re-implement it or use occ.

In the mean time, we can have the extra functionality of specifying an app version. Let's see if there is a way to fix the integrity issues.

simonspa commented 4 years ago

Okay, agreed. Integrity but also app updates need fixing, the Admin panel shows me available updates while the role happily plows on...

nkakouros commented 4 years ago

I am currently a bit swamped and drowning, so I will have to look into it in the weekend. If you want to look into it, I would start from this conditional:

https://github.com/nkakouros-original/ansible-role-nextcloud/blob/31364c1bcfa41659f7c1651541a1db637111745e/tasks/core/apps.yml#L108-L155

I already see some improvements to be made:

simonspa commented 4 years ago

Jup, will look into this later and have also found these issues already. No worries, I'll come up with a new PR. ;)

simonspa commented 4 years ago

I found out why the integrity is failing from the command line while working from the Admin panel: in the rescan they simply ignore all apps without appinfo/signature.json shipped:

https://github.com/nextcloud/server/blob/65abdea3306ef27c4ce9b1c2480bea05dd105138/lib/private/IntegrityCheck/Checker.php#L578

We should probably file a bug report against the server because this means that the cli integrity check (which doesn't have this option) will always fail for unsigned apps.

simonspa commented 4 years ago

https://github.com/nextcloud/server/issues/17801