Skip to content

fix: always use proper path on node api when calling the view - #36774

Merged
szaimen merged 2 commits into
masterfrom
bugfix/noid/sabre-nodes
Apr 18, 2023
Merged

fix: always use proper path on node api when calling the view#36774
szaimen merged 2 commits into
masterfrom
bugfix/noid/sabre-nodes

Conversation

@juliusknorr

@juliusknorrjuliusknorr commented Feb 18, 2023

Copy link
Copy Markdown
Member

Make sure that the nodes API always uses the full path and root view.

This is only noticeable on places where the dav app already uses the new nodes api but could cause trouble in the future.

Seen this being an issue when the dav app passes a relative path and fake root view for the user but the full file system setup was dropped by #36589 when:

  • Incoming share was moved to a subfolder A/B/share
  • Perform the following query: curl -X PROPFIND http://admin:admin@nextcloud.dev.local/remote.php/webdav/A -H 'Depth: 2'

@juliusknorr
juliusknorrforce-pushed the bugfix/noid/sabre-nodes branch 3 times, most recently from a560685 to 8b9b738CompareFebruary 18, 2023 16:09
@juliusknorr
juliusknorr marked this pull request as ready for review February 20, 2023 07:45
@juliusknorr
juliusknorr requested review from a team, ArtificialOwl, blizzz and miaulalala and removed request for a teamFebruary 20, 2023 07:45
@juliusknorrjuliusknorr added bug 3. to review Waiting for reviews labels Feb 20, 2023
@juliusknorrjuliusknorr added this to the Nextcloud 26 milestone Feb 20, 2023

@miaulalalamiaulalala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense to me but I'm no expert.

@come-nccome-nc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does that fix #34932 (comment) ?

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Does that fix #34932 (comment) ?

Not directly I think, but we could probably refactor to just avoid using the View in

try {
// For non-chunked upload it is enough to check if we can create a new file
if (!$this->fileView->isCreatable($this->path)) {
thrownewException\Forbidden();
}
$this->fileView->verifyPath($this->path, $name);
$path = $this->fileView->getAbsolutePath($this->path) . '/' . $name;
// in case the file already exists/overwriting
$info = $this->fileView->getFileInfo($this->path . '/' . $name);
if (!$info) {
// use a dummy FileInfo which is acceptable here since it will be refreshed after the put is complete
$info = new \OC\Files\FileInfo($path, null, null, [
'type' => FileInfo::TYPE_FILE
], null);
}
$node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info);
// only allow 1 process to upload a file at once but still allow reading the file while writing the part file
$node->acquireLock(ILockingProvider::LOCK_SHARED);
$this->fileView->lockFile($path . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
$result = $node->put($data);
$this->fileView->unlockFile($path . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
$node->releaseLock(ILockingProvider::LOCK_SHARED);
return$result;
} catch (StorageNotAvailableException$e) {
thrownewServiceUnavailable($e->getMessage());
} catch (InvalidPathException$ex) {
thrownewInvalidPath($ex->getMessage(), false, $ex);
} catch (ForbiddenException$ex) {
thrownewForbidden($ex->getMessage(), $ex->getRetry(), $ex);
} catch (LockedException$e) {
thrownewFileLocked($e->getMessage(), $e->getCode(), $e);
}
and go directly through the Nodes API instead. This would get rid of the separate handling of hooks in this case in the dav app.

icewind1991
icewind1991 previously requested changes Feb 20, 2023

@icewind1991icewind1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sabre\Node::refreshInfo will also need updating.

Overall the change is the right direction imo, and ensuring some consistency here should also make an eventual move to using the Node api directly a bit easier

Comment threadlib/private/Files/Node/Folder.php Outdated
@nickvergessen

Copy link
Copy Markdown
Member

I think we have to revert #36589 because it causes a major regression with Talk/ attachment handling, see #36787

I also tested with this PR here and Talk/ is still not working. Given that we are behind feature freeze I vote to delay this here, revert the other PR on master until stable26 is branched off and try again for 27.l

@nickvergessennickvergessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

blocking as per above

juliusknorr added a commit to nextcloud/spreed that referenced this pull request Mar 27, 2023
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Dry run of talk integration tests passes nextcloud/spreed#9173

@juliusknorr
juliusknorr dismissed nickvergessen’s stale reviewMarch 27, 2023 08:50

Failing PR was reverted, this change is still good besides that for 27 now.

Comment threadapps/dav/lib/Connector/Sabre/Node.php
@juliusknorr
juliusknorrforce-pushed the bugfix/noid/sabre-nodes branch from 1efa1c6 to 2b85b04CompareApril 12, 2023 12:02
$this->node = new Folder($root, $rootView, $this->fileView->getAbsolutePath($this->path), $info);
} else {
$this->node = new File($root, $view, $this->path, $info);
$this->node = new File($root, $rootView, $this->fileView->getAbsolutePath($this->path), $info);

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 3 of OC\Files\Node\File::__construct cannot be null, possibly null value provided
$root = \OC::$server->get(IRootFolder::class);
if ($info->getType() === FileInfo::TYPE_FOLDER) {
$this->node = new Folder($root, $view, $this->path, $info);
$this->node = new Folder($root, $rootView, $this->fileView->getAbsolutePath($this->path), $info);

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 3 of OC\Files\Node\Folder::__construct cannot be null, possibly null value provided
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@szaimen
szaimenforce-pushed the bugfix/noid/sabre-nodes branch from 2b85b04 to d9c81f5CompareApril 17, 2023 21:47
@szaimen
szaimen enabled auto-merge April 17, 2023 21:47
@szaimen
szaimen merged commit 855e7a2 into masterApr 18, 2023
@szaimen
szaimen deleted the bugfix/noid/sabre-nodes branch April 18, 2023 02:11
@icewind1991

Copy link
Copy Markdown
Member

/backport to stable26

@icewind1991

Copy link
Copy Markdown
Member

/backport to stable25

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@juliusknorr@nickvergessen@icewind1991@artonge@miaulalala@github-advanced-security@come-nc@szaimen