Skip to content

Commit 84be993

Browse files
blizzzblizzz
authored
Merge pull request #55800 from nextcloud/fix/file-drop/chunked-upload
Fix chunked upload for file drop shares
2 parents 3f85bcc + 3a24216 commit 84be993

File tree

5 files changed

+52
-77
lines changed

5 files changed

+52
-77
lines changed

‎apps/dav/appinfo/v2/publicremote.php‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@
8181
$filesDropPlugin = new FilesDropPlugin();
8282

8383
/** @var string $baseuri defined in public.php */
84-
$server = $serverFactory->createServer(true, $baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($authBackend, $linkCheckPlugin, $filesDropPlugin) {
84+
$server = $serverFactory->createServer(true, $baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($baseuri, $requestUri, $authBackend, $linkCheckPlugin, $filesDropPlugin) {
8585
// GET must be allowed for e.g. showing images and allowing Zip downloads
8686
if ($server->httpRequest->getMethod() !== 'GET') {
8787
// If this is *not* a GET request we only allow access to public DAV from AJAX or when Server2Server is allowed
@@ -103,8 +103,16 @@
103103
$previousLog = Filesystem::logWarningWhenAddingStorageWrapper(false);
104104

105105
/** @psalm-suppress MissingClosureParamType */
106-
Filesystem::addStorageWrapper('sharePermissions', function ($mountPoint, $storage) use ($share) {
107-
return new PermissionsMask(['storage' => $storage, 'mask' => $share->getPermissions() | Constants::PERMISSION_SHARE]);
106+
Filesystem::addStorageWrapper('sharePermissions', function ($mountPoint, $storage) use ($requestUri, $baseuri, $share) {
107+
$mask = $share->getPermissions() | Constants::PERMISSION_SHARE;
108+
109+
// For chunked uploads it is necessary to have read and delete permission,
110+
// so the temporary directory, chunks and destination file can be read and delete after the assembly.
111+
if (str_starts_with(substr($requestUri, strlen($baseuri) - 1), '/uploads/')) {
112+
$mask |= Constants::PERMISSION_READ | Constants::PERMISSION_DELETE;
113+
}
114+
115+
return new PermissionsMask(['storage' => $storage, 'mask' => $mask]);
108116
});
109117

110118
/** @psalm-suppress MissingClosureParamType */

‎apps/dav/lib/Files/Sharing/FilesDropPlugin.php‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,10 @@ public function initialize(\Sabre\DAV\Server $server): void {
4242
}
4343

4444
public function onMkcol(RequestInterface $request, ResponseInterface $response) {
45+
if ($this->isChunkedUpload($request)) {
46+
return;
47+
}
48+
4549
if (!$this->enabled || $this->share === null) {
4650
return;
4751
}
@@ -58,7 +62,18 @@ public function onMkcol(RequestInterface $request, ResponseInterface $response)
5862
return false;
5963
}
6064

65+
private function isChunkedUpload(RequestInterface $request): bool {
66+
return str_starts_with(substr($request->getUrl(), strlen($request->getBaseUrl()) - 1), '/uploads/');
67+
}
68+
6169
public function beforeMethod(RequestInterface $request, ResponseInterface $response) {
70+
$isChunkedUpload = $this->isChunkedUpload($request);
71+
72+
// For the final MOVE request of a chunked upload it is necessary to modify the Destination header.
73+
if ($isChunkedUpload && $request->getMethod() !== 'MOVE') {
74+
return;
75+
}
76+
6277
if (!$this->enabled || $this->share === null) {
6378
return;
6479
}
@@ -68,21 +83,8 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
6883
return;
6984
}
7085

71-
// Retrieve the nickname from the request
72-
$nickname = $request->hasHeader('X-NC-Nickname')
73-
? trim(urldecode($request->getHeader('X-NC-Nickname')))
74-
: null;
75-
76-
if ($request->getMethod() !== 'PUT') {
77-
// If uploading subfolders we need to ensure they get created
78-
// within the nickname folder
79-
if ($request->getMethod() === 'MKCOL') {
80-
if (!$nickname) {
81-
throw new BadRequest('A nickname header is required when uploading subfolders');
82-
}
83-
} else {
84-
throw new MethodNotAllowed('Only PUT is allowed on files drop');
85-
}
86+
if ($request->getMethod() !== 'PUT' && $request->getMethod() !== 'MKCOL' && (!$isChunkedUpload || $request->getMethod() !== 'MOVE')) {
87+
throw new MethodNotAllowed('Only PUT, MKCOL and MOVE are allowed on files drop');
8688
}
8789

8890
// If this is a folder creation request
@@ -95,8 +97,16 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
9597
// full path along the way. We'll only handle conflict
9698
// resolution on file conflicts, but not on folders.
9799

98-
// e.g files/dCP8yn3N86EK9sL/Folder/image.jpg
99-
$path = $request->getPath();
100+
if ($isChunkedUpload) {
101+
$destination = $request->getHeader('destination');
102+
$baseUrl = $request->getBaseUrl();
103+
// e.g files/dCP8yn3N86EK9sL/Folder/image.jpg
104+
$path = substr($destination, strpos($destination, $baseUrl) + strlen($baseUrl));
105+
} else {
106+
// e.g files/dCP8yn3N86EK9sL/Folder/image.jpg
107+
$path = $request->getPath();
108+
}
109+
100110
$token = $this->share->getToken();
101111

102112
// e.g files/dCP8yn3N86EK9sL
@@ -112,6 +122,11 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
112122
$isFileRequest = $attributes->getAttribute('fileRequest', 'enabled') === true;
113123
}
114124

125+
// Retrieve the nickname from the request
126+
$nickname = $request->hasHeader('X-NC-Nickname')
127+
? trim(urldecode($request->getHeader('X-NC-Nickname')))
128+
: null;
129+
115130
// We need a valid nickname for file requests
116131
if ($isFileRequest && !$nickname) {
117132
throw new BadRequest('A nickname header is required for file requests');
@@ -187,7 +202,11 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
187202
$relativePath = substr($folder->getPath(), strlen($node->getPath()));
188203
$path = '/files/' . $token . '/' . $relativePath . '/' . $uniqueName;
189204
$url = rtrim($request->getBaseUrl(), '/') . str_replace('//', '/', $path);
190-
$request->setUrl($url);
205+
if ($isChunkedUpload) {
206+
$request->setHeader('destination', $url);
207+
} else {
208+
$request->setUrl($url);
209+
}
191210
}
192211

193212
private function getPathSegments(string $path): array {

‎apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php‎

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
use OCP\Share\IAttributes;
1414
use OCP\Share\IShare;
1515
use PHPUnit\Framework\MockObject\MockObject;
16-
use Sabre\DAV\Exception\BadRequest;
1716
use Sabre\DAV\Server;
1817
use Sabre\HTTP\RequestInterface;
1918
use Sabre\HTTP\ResponseInterface;
@@ -56,13 +55,6 @@ protected function setUp(): void {
5655
->willReturn('token');
5756
}
5857

59-
public function testNotEnabled(): void {
60-
$this->request->expects($this->never())
61-
->method($this->anything());
62-
63-
$this->plugin->beforeMethod($this->request, $this->response);
64-
}
65-
6658
public function testValid(): void {
6759
$this->plugin->enable();
6860
$this->plugin->setShare($this->share);
@@ -112,32 +104,13 @@ public function testFileAlreadyExistsValid(): void {
112104
$this->plugin->beforeMethod($this->request, $this->response);
113105
}
114106

115-
public function testNoMKCOLWithoutNickname(): void {
107+
public function testMKCOL(): void {
116108
$this->plugin->enable();
117109
$this->plugin->setShare($this->share);
118110

119111
$this->request->method('getMethod')
120112
->willReturn('MKCOL');
121113

122-
$this->expectException(BadRequest::class);
123-
124-
$this->plugin->beforeMethod($this->request, $this->response);
125-
}
126-
127-
public function testMKCOLWithNickname(): void {
128-
$this->plugin->enable();
129-
$this->plugin->setShare($this->share);
130-
131-
$this->request->method('getMethod')
132-
->willReturn('MKCOL');
133-
134-
$this->request->method('hasHeader')
135-
->with('X-NC-Nickname')
136-
->willReturn(true);
137-
$this->request->method('getHeader')
138-
->with('X-NC-Nickname')
139-
->willReturn('nickname');
140-
141114
$this->expectNotToPerformAssertions();
142115

143116
$this->plugin->beforeMethod($this->request, $this->response);

‎build/integration/features/bootstrap/FilesDropContext.php‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public function droppingFileWithAs($path, $content, $nickname) {
5757
/**
5858
* @When Creating folder :folder in drop
5959
*/
60-
public function creatingFolderInDrop($folder, $nickname = null) {
60+
public function creatingFolderInDrop($folder) {
6161
$client = new Client();
6262
$options = [];
6363
if (count($this->lastShareData->data->element) > 0) {
@@ -73,22 +73,10 @@ public function creatingFolderInDrop($folder, $nickname = null) {
7373
'X-REQUESTED-WITH' => 'XMLHttpRequest',
7474
];
7575

76-
if ($nickname) {
77-
$options['headers']['X-NC-NICKNAME'] = $nickname;
78-
}
79-
8076
try {
8177
$this->response = $client->request('MKCOL', $fullUrl, $options);
8278
} catch (\GuzzleHttp\Exception\ClientException $e) {
8379
$this->response = $e->getResponse();
8480
}
8581
}
86-
87-
88-
/**
89-
* @When Creating folder :folder in drop as :nickName
90-
*/
91-
public function creatingFolderInDropWithNickname($folder, $nickname) {
92-
return $this->creatingFolderInDrop($folder, $nickname);
93-
}
9482
}

‎build/integration/filesdrop_features/filesdrop.feature‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ Feature: FilesDrop
4646
When Dropping file "/folder/a.txt" with "abc"
4747
Then the HTTP status code should be "400"
4848

49-
Scenario: Files drop forbid MKCOL without a nickname
49+
Scenario: Files drop allows MKCOL
5050
Given user "user0" exists
5151
And As an "user0"
5252
And user "user0" created a folder "/drop"
@@ -57,19 +57,6 @@ Feature: FilesDrop
5757
And Updating last share with
5858
| permissions | 4 |
5959
When Creating folder "folder" in drop
60-
Then the HTTP status code should be "400"
61-
62-
Scenario: Files drop allows MKCOL with a nickname
63-
Given user "user0" exists
64-
And As an "user0"
65-
And user "user0" created a folder "/drop"
66-
And as "user0" creating a share with
67-
| path | drop |
68-
| shareType | 3 |
69-
| publicUpload | true |
70-
And Updating last share with
71-
| permissions | 4 |
72-
When Creating folder "folder" in drop as "nickname"
7360
Then the HTTP status code should be "201"
7461

7562
Scenario: Files drop forbid subfolder creation without a nickname
@@ -139,7 +126,7 @@ Feature: FilesDrop
139126
When Downloading file "/drop/Alice/folder (2)"
140127
Then the HTTP status code should be "200"
141128
And Downloaded content should be "its a file"
142-
129+
143130
Scenario: Put file same file multiple times via files drop
144131
Given user "user0" exists
145132
And As an "user0"

0 commit comments

Comments
 (0)