Skip to content
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.

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

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

Open
nkakouros opened this issue Feb 9, 2018 · 10 comments
Open

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

nkakouros opened this issue Feb 9, 2018 · 10 comments
Assignees

Comments

@nkakouros
Copy link
Collaborator

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.

@nkakouros nkakouros self-assigned this Feb 9, 2018
@simonspa
Copy link
Contributor

simonspa commented Nov 4, 2019

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
Copy link
Collaborator Author

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
Copy link
Contributor

simonspa commented Nov 4, 2019

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
Copy link
Contributor

simonspa commented Nov 4, 2019

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
Copy link
Collaborator Author

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
Copy link
Contributor

simonspa commented Nov 4, 2019

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

@nkakouros
Copy link
Collaborator Author

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:

vars:
download_app: >-
{#- There are three checks here: -#}
{#- 1. The app does not exist in the nextcloud installation -#}
{#- 2. The version for an app is different to the one installed -#}
{#- 3. The app was specified without a version, meaning the latest
version available should be downloaded. So, the version from
the appstore is checked against the local version -#}
{{
(
item.name not in
nextcloud_shipped_apps.enabled
| combine(nextcloud_shipped_apps.disabled)
| combine(nextcloud_shipped_apps.enabled)
| combine(nextcloud_shipped_apps.disabled)
)
or
(
item.name in
nextcloud_installed_apps.enabled
| combine(nextcloud_installed_apps.disabled)
and
'version' in item
and
item.version | default('unset') !=
(
nextcloud_installed_apps.enabled
| combine(nextcloud_installed_apps.disabled)
)[item.name]
)
or
(
(
nextcloud_app_store_json
| selectattr('id', 'equalto', item.name)
| map(attribute='releases')
| first
| list
)[0].version
!=
(
(
nextcloud_installed_apps.enabled
| combine(nextcloud_installed_apps.disabled)
) [item.name]
)
)
}}

I already see some improvements to be made:

  • the 1st checks has an error where shipped apps are used twice
  • the first is not needed

@simonspa
Copy link
Contributor

simonspa commented Nov 4, 2019

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

@simonspa
Copy link
Contributor

simonspa commented Nov 4, 2019

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
Copy link
Contributor

simonspa commented Nov 4, 2019

nextcloud/server#17801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants