fix access array offset in FilesHook.php

[Messj1]

prevent access array offset which doesn't exist and check if returned value is null

fixes #1056

The Problem is related to the empty default array: https://github.com/nextcloud/activity/blob/22bf3f2329e5c1e7cf2b9fa627a59c02ae8ae86a/lib/FilesHooks.php#L569-L580

compared to server implemented skeleton: https://github.com/nextcloud/server/blob/d237fd0e78af8bbff51f2802efd4db1d88457579/lib/private/Share20/ShareHelper.php#L27-L32

	public function getPathsForAccessList(Node $node) {
		$result = [
			'users' => [],
			'remotes' => [],
		];

[Messj1]

Think there could be a cleaner solution that fits the case:

Revert changes:

+		$oldUsers = $this->oldAccessList['users'];
-		$oldUsers = $this->oldAccessList['users'] ?? [];

...

+		$beforeRemotes = $this->oldAccessList['remotes'];
-		$beforeRemotes = $this->oldAccessList['remotes'] ?? [];

And set a default skeleton in case oldAccessList was not set (Error or Exception):

+		if($this->oldAccessList == null) {
+			$this->oldAccessList = [
+				'users' => [],
+				'remotes' => [],
+			];
+		}
		$oldUsers = $this->oldAccessList['users'];

So it would be simply to implement logging like:

		if($this->oldAccessList == null) {
+			$this->logger->warning("Something fishy happens: function fileMove was not finished");
			$this->oldAccessList = [
				'users' => [],
				'remotes' => [],
			];
		}
		$oldUsers = $this->oldAccessList['users'];

and

	protected function getUserPathsFromPath($path, $uidOwner) {
		try {
			$node = $this->rootFolder->getUserFolder($uidOwner)->get($path);
		} catch (NotFoundException $e) {
+			$this->logger->warning("Something fishy happens: Path was not found", ['path'=> $path, 'uidOwner'=>$uidOwner]);
			return [
				'users' => [],
				'remotes' => [],
			];
		}

		if (!$node instanceof Node) {
+			$this->logger->warning("Something fishy happens: 'Folder' didn't return a Node", ['path'=> $path, 'uidOwner'=>$uidOwner]);
			return [
				'users' => [],
				'remotes' => [],
			];
		}

Opinions?

inspired by https://github.com/nextcloud/activity/pull/1281

I am wondering if this won't juste hide another issue. Can you investigate ?

[artonge]

Indeed, thanks for linking the existing PR. I forgot about that one.

Maybe having getUserPathsFromPath to return a skeleton instead of an empty array would be enough? Adding logs is also a good idea to further address the issue later :). Then, I don't think having a skeleton in fileMoving is necessary.

[Messj1]

Maybe having getUserPathsFromPath to return a skeleton instead of an empty array would be enough? ... Then, I don't think having a skeleton in fileMoving is necessary.

This PR is related to 2 bugs:

1. The error handling in getUserPathsFromPath was not returning a proper value (skeleton)
2. If function fileMove is not called or if it not reach L277-L285

https://github.com/nextcloud/activity/blob/22bf3f2329e5c1e7cf2b9fa627a59c02ae8ae86a/lib/FilesHooks.php#L277-L285

So for the second case we need to check if $this->oldAccessList is not null

[github-actions[bot]]

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.)