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:

activity/lib/FilesHooks.php

Lines 569 to 580 in 22bf3f2

	protected function getUserPathsFromPath($path, $uidOwner) {
	try {
	$node = $this->rootFolder->getUserFolder($uidOwner)->get($path);
	} catch (NotFoundException $e) {
	return [];
	}
	if (!$node instanceof Node) {
	return [];
	}
	$accessList = $this->shareHelper->getPathsForAccessList($node);

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 #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

activity/lib/FilesHooks.php

Lines 277 to 285 in 22bf3f2

	$oldAccessList = $this->getUserPathsFromPath($this->oldParentPath, $this->oldParentOwner);
	// file can be shared using GroupFolders, including ACL check
	if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) {
	[, , $oldFileId] = $this->getSourcePathAndOwner($oldPath);
	$oldAccessList['users'] = array_merge($oldAccessList['users'], $this->getAffectedUsersFromCachedMounts($oldFileId));
	}
	$this->oldAccessList = $oldAccessList;

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

[artonge]

As most returns from fileMove are legitimate and not fishy, I think I would not add a log here and rather add some in the fileMove move early returns that are unexpected. And I would also be more explicit in the message.
Sorry for the numerous back and forth, and thanks again for addressing the issue :).

[Messj1]

As most returns from fileMove are legitimate and not fishy, I think I would not add a log here and rather add some in the fileMove move early returns that are unexpected.

Yeah, good point.

The state $oldAccessList === null is illegal (unhealthy state). It means, that the FilesHooks::fileMove function gets ended unexpected.

I count 4 return points in FilesHooks::fileMove :

1. activity/lib/FilesHooks.php

   Lines 218 to 219 in 70362c1

   	$this->moveCase = false;
   	return;

2. activity/lib/FilesHooks.php

   Lines 233 to 234 in 70362c1

   	$this->moveCase = 'rename';
   	return;

3. activity/lib/FilesHooks.php

   Lines 273 to 274 in 70362c1

   	$this->moveCase = false;
   	return;

4. activity/lib/FilesHooks.php

   Lines 284 to 286 in 70362c1

   	$this->oldAccessList = $oldAccessList;
   	}

And only the 4. case calls the FilesHooks::fileMoving function.

activity/lib/FilesHooks.php

Lines 295 to 312 in 70362c1

	public function fileMovePost($oldPath, $newPath) {
	// Do not add activities for .part-files
	if ($this->moveCase === false) {
	return;
	}
	switch ($this->moveCase) {
	case 'rename':
	$this->fileRenaming($oldPath, $newPath);
	break;
	case 'moveUp':
	case 'moveDown':
	case 'moveCross':
	$this->fileMoving($oldPath, $newPath);
	break;
	}
	$this->moveCase = false;

A more appropriate way would be to invalidate the post process and let error be error, since it is generated by 3rd party.

Cache the case $this->moveCase instead of set it direct:

+		$this->moveCase = false;
+		$moveCase = false;
		if (strpos($oldDir, $newDir) === 0) {
			/**
			 * a/b/c moved to a/c
			 *
			 * Cases:
			 * - a/b/c shared: no visible change
			 * - a/b/ shared: delete
			 * - a/ shared: move/rename
			 */
-			$this->moveCase = 'moveUp';
+			$moveCase = 'moveUp';
		} elseif (strpos($newDir, $oldDir) === 0) {
			/**
			 * a/b moved to a/c/b
			 *
			 * Cases:
			 * - a/b shared: no visible change
			 * - a/c/ shared: add
			 * - a/ shared: move/rename
			 */
-			$this->moveCase = 'moveDown';
+			$moveCase = 'moveDown';
		} else {
			/**
			 * a/b/c moved to a/d/c
			 *
			 * Cases:
			 * - a/b/c shared: no visible change
			 * - a/b/ shared: delete
			 * - a/d/ shared: add
			 * - a/ shared: move/rename
			 */
-			$this->moveCase = 'moveCross';
+			$moveCase = 'moveCross';
		}

And set de case at the end to guarantee the availability of $this->oldAccessList

		}

+		$this->moveCase = $moveCase;
		$this->oldAccessList = $oldAccessList;
	}

So I would be able to remove the null check and logging because it is guaranteed

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

	}

[artonge]

It's a warning, no need to add such statement. :)

Suggested change

	$this->logger->warning('Something fishy happens: Path "{path}" with owner "{uidOwner}" was not found',
	$this->logger->warning('Path "{path}" with owner "{uidOwner}" was not found',

[artonge]

Could you add a comment here? I am not sure to understand why you are caching the value before assigning it?

[artonge]

I still don't get why this is safer to set it here

[Messj1]

There are 2 Signals invoked:

activity/lib/AppInfo/Application.php

Lines 166 to 167 in 06aabff

	Util::connectHook('OC_Filesystem', 'rename', FilesHooksStatic::class, 'fileMove');
	Util::connectHook('OC_Filesystem', 'post_rename', FilesHooksStatic::class, 'fileMovePost');

The function fileMove is called before the action and the function fileMovePost afterwards.

$this->moveCase gets only defined in function fileMove.

In fileMovePost the $this->moveCase is consumed and will be reset

activity/lib/FilesHooks.php

Lines 310 to 313 in 06aabff

	}
	$this->moveCase = false;
	}

If the value of $this->moveCase is not set the function returns immediately.

activity/lib/FilesHooks.php

Lines 295 to 299 in 06aabff

	public function fileMovePost($oldPath, $newPath) {
	// Do not add activities for .part-files
	if ($this->moveCase === false) {
	return;
	}

Conclusion:
@artonge It is save the set the variable at the end cause everything is calculated and there was no Error/Exception.
-> Ready to go to next stage

Currently it goes into post process and the $this->oldAccessList Variable is not defined because it gets defined at the end and in my case there was an exception thrown by 3rd party (circle) during calculate accesslist. 😢

activity/lib/FilesHooks.php

Lines 284 to 286 in 06aabff

	$this->oldAccessList = $oldAccessList;
	}

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