Add INodeByPath to Directory

[salmart-dev]

Summary

Sabre has an interface INodeByPath that allows nodes to fetch a specific node by its path. This was not implemented in our Directory class, leading to any file query to fetch all parent nodes until the file node was reached. This PR makes Directory implement that interface, extends makes the NC Node class extend the Node class from Sabre (required step).

Before:

After:

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Screenshots before/after for front-end changes
• [ ] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)

[github-actions[bot]]

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/work/server/server/occ(33): require_once('...')
#9 {main}

[github-actions[bot]]

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/work/server/server/occ(33): require_once('...')
#9 {main}

[salmart-dev]

Performance regression warning is unrelated. The output of the check contains text which makes the json_decode part of the check fail and return null.

[github-actions[bot]]

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/actions-runner/_work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/actions-runner/_work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/actions-runner/_work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/actions-runner/_work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/actions-runner/_work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/actions-runner/_work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/actions-runner/_work/server/server/occ(33): require_once('...')
#9 {main}

[github-actions[bot]]

Possible performance regression detected

Show Output

An unhandled exception has been thrown:
TypeError: array_map(): Argument #2 ($array) must be of type array, null given in /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php:35
Stack trace:
#0 /home/runner/work/server/server/apps/profiler/lib/Command/Compare.php(35): array_map()
#1 /home/runner/work/server/server/3rdparty/symfony/console/Command/Command.php(326): OCAProfilerCommandCompare->execute()
#2 /home/runner/work/server/server/core/Command/Base.php(218): SymfonyComponentConsoleCommandCommand->run()
#3 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(1078): OCCoreCommandBase->run()
#4 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(324): SymfonyComponentConsoleApplication->doRunCommand()
#5 /home/runner/work/server/server/3rdparty/symfony/console/Application.php(175): SymfonyComponentConsoleApplication->doRun()
#6 /home/runner/work/server/server/lib/private/Console/Application.php(187): SymfonyComponentConsoleApplication->run()
#7 /home/runner/work/server/server/console.php(91): OCConsoleApplication->run()
#8 /home/runner/work/server/server/occ(33): require_once('...')
#9 {main}

[salmart-dev]

I converted the PR back to a draft because I ran into an issue with access control rules.

Problem statement

Apps that rely on path navigation to perform checks that block an action could be broken with this PR.

Example

The files_accesscontrol app relies on each node's name to match for names that have been blocked. With this PR, the intermediate nodes in a path may never be navigated to, creating an issue where a file deep in the tree can be accessed, although one of the skipped nodes is marked as not readable through the app.

In the current implementation, the app relies on the fact that isReadable gets called in every Node, which in turn triggers the check for the file name in the app.

How to reproduce

1. Create a public share
2. Add a folder inside the public share
3. Block access to the directory through a flow with the files_accesscontrol app
4. Try uploading inside the new folder

Uploading should fail but succeeds instead

Possible solution?

Alter files_accesscontrol

For the case in the example, a fix could be to change files_accesscontrol to stop relying on the assumption that all nodes in a path will be navigated to and compare each part of the exploded path with the given value to block, but I fear this would break other flows and checks.

---

Can someone come up with ideas on how to address this or other things that may break, so that if we try a new solution I can test it still works?

[icewind1991]

stop relying on the assumption that all nodes in a path will be navigated to and compare each part of the exploded path

This is probably the best option imo.

Another alternative would be to have a dav plugin in files_accesscontrol to do extra checking without effecting any of the other rules or non-dav code paths.

[salmart-dev]

Had to add a check for !$allowDirectory to skip navigating to the parent which is not readable in the context of a file drop (https://github.com/nextcloud/server/pull/54441/files#diff-bbeea4e9faca76aa9e6901242c70df675c85efe25134b50995bbe78109fb636cR559)

cc. @artonge

[salmart-dev]

@icewind1991 this needs another review from you :pray:

The issue with files_accesscontrol has been solved by checking if all parent folders are readable, seems to be working well with group folders as well.

A case that I was expecting to cause trouble with this PR also seems to have the same behaviour on master: within a group folder having a folder with read permissions removed for a specific user, but containing a file with read permissions allowed for the same user. This fails on master and on my PR, so from what I can test, this seems to be working well. The only drawback is that we lose the removal of the N+1 queries for navigating into deep folders, since navigating to the parents to check for readability will load their file infos.