fix(occ): occ integrity:check-app and Admin panel "rescan" deliver inconsistent results

[ehfd]

• Resolves: occ integrity:check-app and Admin panel "rescan" deliver inconsistent results #17801

Summary

This issue fixes the behavior discrepancy between the occ integrity:check-app command and the Admin panel "rescan" button.
The fix allows occ integrity:check-app to gracefully skip the signature check of the app instead of throwing an exception, as instructed in #17801 (comment).

TODO

• Independent validation by a reviewer. Check the below custom_apps apps (open the dropdown) that do not ship with signatures. bookmarks is a good example (Add signature to app bookmarks#1941).

Technical information
=====================
The following list covers which files have failed the integrity check. Please read
the previous linked documentation to learn more about the errors and how to fix
them.

Results
=======
- apporder
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- bookmarks
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- bruteforcesettings
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- calendar
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- drawio
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- event_update_notification
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- impersonate
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- metadata
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- previewgenerator
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- twofactor_admin
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- twofactor_totp
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.
- twofactor_u2f
	- EXCEPTION
		- OC\IntegrityCheck\Exceptions\InvalidSignatureException
		- Signature data not found.

Raw output
==========
Array
(
    [apporder] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [bookmarks] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [bruteforcesettings] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [calendar] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [drawio] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [event_update_notification] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [impersonate] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [metadata] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [previewgenerator] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [twofactor_admin] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [twofactor_totp] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

    [twofactor_u2f] => Array
        (
            [EXCEPTION] => Array
                (
                    [class] => OC\IntegrityCheck\Exceptions\InvalidSignatureException
                    [message] => Signature data not found.
                )

        )

)

• Identify unit testing requirements.

Checklist

• Code is properly formatted - Correct if workflow passes
• Sign-off message is added to all commits - Done
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes - Not applicable
• Documentation (manuals or wiki) has been updated or is not required - Not required
• Backports requested where applicable (ex: critical bugfixes) - Not required because this is annoying but not critical, but may be nonetheless preferred, based on reviewer discretion

CC @joshtrichards @simonspa @szaimen

[ehfd]

No labels, nothing? @joshtrichards @simonspa @szaimen

[joshtrichards]

I think the only thing missing that I see (for parity) is an isShipped() check (shipped apps are always verified; if they're missing a signature file something is wrong so we still want to throw an exception for those). Looks reasonable otherwise.

server/lib/private/IntegrityCheck/Checker.php

Lines 541 to 553 in f9fcc5b

	// If an application is shipped a valid signature is required
	$isShipped = $this->appManager->isShipped($appId);
	$appNeedsToBeChecked = false;
	if ($isShipped) {
	$appNeedsToBeChecked = true;
	} elseif ($this->fileAccessHelper->file_exists($this->appLocator->getAppPath($appId) . '/appinfo/signature.json')) {
	// Otherwise only if the application explicitly ships a signature.json file
	$appNeedsToBeChecked = true;
	}
	if ($appNeedsToBeChecked) {
	$this->verifyAppSignature($appId);
	}

I'm mildly tempted to add another return/exit code (3 maybe) so that automated runners can easily detect if things only passed due to a missing signature file, but that opens a can of worms we should probably avoid right now (i.e. auditing the return codes we use across our occ commands; adding one arbitrarily right now just creates later tech debt... so probably easier to add that code later when/if deemed important I guess).

[joshtrichards]

Hi @ehfd - Thanks for the ping and PR. And sorry for the delay.

[joshtrichards]

Identify unit testing requirements.

Hmm. I don't currently see any test coverage for the integrity command namespace themselves:

https://github.com/nextcloud/server/tree/f5f9691f387473982bff7d9788fe3350979e7cbc/tests/Core/Command

Might be a good opportunity to add some. :)

We do have test coverage for the underlying IntegrityCheck\Checker itself however at least.

[ehfd]

#49577 (review)
@joshtrichards
Requested changes are implemented in 2314a2f.
I agree that returning a new exit code should be something for the future.

As for the tests, feel free to modify the branch. Otherwise, new issue and PR for the tests in order to not bloat the PR.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)