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.

Sign up for GitHub

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

Jump to bottom

fix access array offset in FilesHook.php #1730

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix access array offset in FilesHook.php #1730

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
631c13d
changes accesslist default result from empty array to skeleton
Messj1 Jul 16, 2024
fa1aedd
log warnings to give admins more infos about possibly miscalculated a…
Messj1 Jul 22, 2024
151c6bd
fix access array offset which doesn't exist
Messj1 Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions lib/FilesHooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,9 @@ public function fileMove($oldPath, $newPath) {
return;
}

// cache moveCase result until attributes are calculated for next stage (fileMovePost)
// moveCase has to be set as last part before return
$moveCase = $this->moveCase;
if (strpos($oldDir, $newDir) === 0) {
/**
* a/b/c moved to a/c
Expand All @@ -243,7 +246,7 @@ public function fileMove($oldPath, $newPath) {
* - 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
Expand All @@ -253,7 +256,7 @@ public function fileMove($oldPath, $newPath) {
* - a/c/ shared: add
* - a/ shared: move/rename
*/
$this->moveCase = 'moveDown';
$moveCase = 'moveDown';
} else {
/**
* a/b/c moved to a/d/c
Expand All @@ -264,7 +267,7 @@ public function fileMove($oldPath, $newPath) {
* - a/d/ shared: add
* - a/ shared: move/rename
*/
$this->moveCase = 'moveCross';
$moveCase = 'moveCross';
}

[$this->oldParentPath, $this->oldParentOwner, $this->oldParentId] = $this->getSourcePathAndOwner($oldDir);
Expand All @@ -283,6 +286,9 @@ public function fileMove($oldPath, $newPath) {
}

$this->oldAccessList = $oldAccessList;

// now its save to set moveCase
$this->moveCase = $moveCase;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}


Expand Down Expand Up @@ -570,11 +576,21 @@ protected function getUserPathsFromPath($path, $uidOwner) {
try {
$node = $this->rootFolder->getUserFolder($uidOwner)->get($path);
} catch (NotFoundException $e) {
return [];
$this->logger->warning('Path "{path}" with owner "{uidOwner}" was not found',
['path'=> $path, 'uidOwner'=>$uidOwner]);
return [
'users' => [],
'remotes' => [],
];
}

if (!$node instanceof Node) {
return [];
$this->logger->warning('Path "{path}" of "{uidOwner}" is not a valid type.',
['path'=> $path, 'uidOwner'=>$uidOwner]);
return [
'users' => [],
'remotes' => [],
];
}

$accessList = $this->shareHelper->getPathsForAccessList($node);
Expand Down
Loading