Skip to content

Commit 4d363ee

Browse files
solracsfsolracsf
authored
Merge pull request #55804 from nextcloud/backport/55800/stable32
[stable32] Fix chunked upload for file drop shares
2 parents 2a3d712 + b737336 commit 4d363ee

File tree

6 files changed

+108
-59
lines changed

6 files changed

+108
-59
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/Capabilities.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public function getCapabilities() {
2424
$capabilities = [
2525
'dav' => [
2626
'chunking' => '1.0',
27-
'public_shares_chunking' => false,
27+
'public_shares_chunking' => true,
2828
]
2929
];
3030
if ($this->config->getSystemValueBool('bulkupload.enabled', true)) {

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

Lines changed: 44 additions & 34 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 PUT and MOVE requests of a chunked upload it is necessary to modify the Destination header.
73+
if ($isChunkedUpload && $request->getMethod() !== 'MOVE' && $request->getMethod() !== 'PUT') {
74+
return;
75+
}
76+
6277
if (!$this->enabled || $this->share === null) {
6378
return;
6479
}
@@ -68,21 +83,25 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
6883
return;
6984
}
7085

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');
88+
}
89+
90+
// Extract the attributes for the file request
91+
$isFileRequest = false;
92+
$attributes = $this->share->getAttributes();
93+
if ($attributes !== null) {
94+
$isFileRequest = $attributes->getAttribute('fileRequest', 'enabled') === true;
95+
}
96+
7197
// Retrieve the nickname from the request
7298
$nickname = $request->hasHeader('X-NC-Nickname')
7399
? trim(urldecode($request->getHeader('X-NC-Nickname')))
74100
: null;
75101

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-
}
102+
// We need a valid nickname for file requests
103+
if ($isFileRequest && !$nickname) {
104+
throw new BadRequest('A nickname header is required for file requests');
86105
}
87106

88107
// If this is a folder creation request
@@ -95,35 +114,22 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
95114
// full path along the way. We'll only handle conflict
96115
// resolution on file conflicts, but not on folders.
97116

98-
// e.g files/dCP8yn3N86EK9sL/Folder/image.jpg
99-
$path = $request->getPath();
117+
if ($isChunkedUpload) {
118+
$destination = $request->getHeader('destination');
119+
$baseUrl = $request->getBaseUrl();
120+
// e.g files/dCP8yn3N86EK9sL/Folder/image.jpg
121+
$path = substr($destination, strpos($destination, $baseUrl) + strlen($baseUrl));
122+
} else {
123+
// e.g files/dCP8yn3N86EK9sL/Folder/image.jpg
124+
$path = $request->getPath();
125+
}
126+
100127
$token = $this->share->getToken();
101128

102129
// e.g files/dCP8yn3N86EK9sL
103130
$rootPath = substr($path, 0, strpos($path, $token) + strlen($token));
104131
// e.g /Folder/image.jpg
105132
$relativePath = substr($path, strlen($rootPath));
106-
$isRootUpload = substr_count($relativePath, '/') === 1;
107-
108-
// Extract the attributes for the file request
109-
$isFileRequest = false;
110-
$attributes = $this->share->getAttributes();
111-
if ($attributes !== null) {
112-
$isFileRequest = $attributes->getAttribute('fileRequest', 'enabled') === true;
113-
}
114-
115-
// We need a valid nickname for file requests
116-
if ($isFileRequest && !$nickname) {
117-
throw new BadRequest('A nickname header is required for file requests');
118-
}
119-
120-
// We're only allowing the upload of
121-
// long path with subfolders if a nickname is set.
122-
// This prevents confusion when uploading files and help
123-
// classify them by uploaders.
124-
if (!$nickname && !$isRootUpload) {
125-
throw new BadRequest('A nickname header is required when uploading subfolders');
126-
}
127133

128134
if ($nickname) {
129135
try {
@@ -187,7 +193,11 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
187193
$relativePath = substr($folder->getPath(), strlen($node->getPath()));
188194
$path = '/files/' . $token . '/' . $relativePath . '/' . $uniqueName;
189195
$url = rtrim($request->getBaseUrl(), '/') . str_replace('//', '/', $path);
190-
$request->setUrl($url);
196+
if ($isChunkedUpload) {
197+
$request->setHeader('destination', $url);
198+
} else {
199+
$request->setUrl($url);
200+
}
191201
}
192202

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

‎apps/dav/tests/unit/CapabilitiesTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public function testGetCapabilities(): void {
3030
$expected = [
3131
'dav' => [
3232
'chunking' => '1.0',
33-
'public_shares_chunking' => false,
33+
'public_shares_chunking' => true,
3434
],
3535
];
3636
$this->assertSame($expected, $capabilities->getCapabilities());
@@ -50,7 +50,7 @@ public function testGetCapabilitiesWithBulkUpload(): void {
5050
$expected = [
5151
'dav' => [
5252
'chunking' => '1.0',
53-
'public_shares_chunking' => false,
53+
'public_shares_chunking' => true,
5454
'bulkupload' => '1.0',
5555
],
5656
];
@@ -71,7 +71,7 @@ public function testGetCapabilitiesWithAbsence(): void {
7171
$expected = [
7272
'dav' => [
7373
'chunking' => '1.0',
74-
'public_shares_chunking' => false,
74+
'public_shares_chunking' => true,
7575
'absence-supported' => true,
7676
'absence-replacement' => true,
7777
],

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

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ class FilesDropPluginTest extends TestCase {
2424
private FilesDropPlugin $plugin;
2525

2626
private Folder&MockObject $node;
27+
private IAttributes&MockObject $attributes;
2728
private IShare&MockObject $share;
2829
private Server&MockObject $server;
2930
private RequestInterface&MockObject $request;
@@ -46,23 +47,16 @@ protected function setUp(): void {
4647
$this->request = $this->createMock(RequestInterface::class);
4748
$this->response = $this->createMock(ResponseInterface::class);
4849

49-
$attributes = $this->createMock(IAttributes::class);
50+
$this->attributes = $this->createMock(IAttributes::class);
5051
$this->share->expects($this->any())
5152
->method('getAttributes')
52-
->willReturn($attributes);
53+
->willReturn($this->attributes);
5354

5455
$this->share
5556
->method('getToken')
5657
->willReturn('token');
5758
}
5859

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-
6660
public function testValid(): void {
6761
$this->plugin->enable();
6862
$this->plugin->setShare($this->share);
@@ -112,34 +106,54 @@ public function testFileAlreadyExistsValid(): void {
112106
$this->plugin->beforeMethod($this->request, $this->response);
113107
}
114108

115-
public function testNoMKCOLWithoutNickname(): void {
109+
public function testFileDropMKCOLWithoutNickname(): void {
116110
$this->plugin->enable();
117111
$this->plugin->setShare($this->share);
118112

119113
$this->request->method('getMethod')
120114
->willReturn('MKCOL');
121115

116+
$this->expectNotToPerformAssertions();
117+
118+
$this->plugin->beforeMethod($this->request, $this->response);
119+
}
120+
121+
public function testFileRequestNoMKCOLWithoutNickname(): void {
122+
$this->plugin->enable();
123+
$this->plugin->setShare($this->share);
124+
125+
$this->request->method('getMethod')
126+
->willReturn('MKCOL');
127+
128+
$this->attributes
129+
->method('getAttribute')
130+
->with('fileRequest', 'enabled')
131+
->willReturn(true);
132+
122133
$this->expectException(BadRequest::class);
123134

124135
$this->plugin->beforeMethod($this->request, $this->response);
125136
}
126137

127-
public function testMKCOLWithNickname(): void {
138+
public function testFileRequestMKCOLWithNickname(): void {
128139
$this->plugin->enable();
129140
$this->plugin->setShare($this->share);
130141

131142
$this->request->method('getMethod')
132143
->willReturn('MKCOL');
133144

145+
$this->attributes
146+
->method('getAttribute')
147+
->with('fileRequest', 'enabled')
148+
->willReturn(true);
149+
134150
$this->request->method('hasHeader')
135151
->with('X-NC-Nickname')
136152
->willReturn(true);
137153
$this->request->method('getHeader')
138154
->with('X-NC-Nickname')
139155
->willReturn('nickname');
140156

141-
$this->expectNotToPerformAssertions();
142-
143157
$this->plugin->beforeMethod($this->request, $this->response);
144158
}
145159

‎build/integration/filesdrop_features/filesdrop.feature‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,53 +33,70 @@ Feature: FilesDrop
3333
And Downloading file "/drop/a (2).txt"
3434
Then Downloaded content should be "def"
3535

36-
Scenario: Files drop forbid directory without a nickname
36+
Scenario: Files request forbid directory without a nickname
3737
Given user "user0" exists
3838
And As an "user0"
3939
And user "user0" created a folder "/drop"
4040
And as "user0" creating a share with
4141
| path | drop |
4242
| shareType | 3 |
4343
| publicUpload | true |
44+
| attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
4445
And Updating last share with
4546
| permissions | 4 |
4647
When Dropping file "/folder/a.txt" with "abc"
4748
Then the HTTP status code should be "400"
4849

49-
Scenario: Files drop forbid MKCOL without a nickname
50+
Scenario: Files drop allow MKCOL without a nickname
51+
Given user "user0" exists
52+
And As an "user0"
53+
And user "user0" created a folder "/drop"
54+
And as "user0" creating a share with
55+
| path | drop |
56+
| shareType | 3 |
57+
| publicUpload | true |
58+
And Updating last share with
59+
| permissions | 4 |
60+
When Creating folder "folder" in drop
61+
Then the HTTP status code should be "201"
62+
63+
Scenario: Files request forbid MKCOL without a nickname
5064
Given user "user0" exists
5165
And As an "user0"
5266
And user "user0" created a folder "/drop"
5367
And as "user0" creating a share with
5468
| path | drop |
5569
| shareType | 3 |
5670
| publicUpload | true |
71+
| attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
5772
And Updating last share with
5873
| permissions | 4 |
5974
When Creating folder "folder" in drop
6075
Then the HTTP status code should be "400"
6176

62-
Scenario: Files drop allows MKCOL with a nickname
77+
Scenario: Files request allows MKCOL with a nickname
6378
Given user "user0" exists
6479
And As an "user0"
6580
And user "user0" created a folder "/drop"
6681
And as "user0" creating a share with
6782
| path | drop |
6883
| shareType | 3 |
6984
| publicUpload | true |
85+
| attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
7086
And Updating last share with
7187
| permissions | 4 |
7288
When Creating folder "folder" in drop as "nickname"
7389
Then the HTTP status code should be "201"
7490

75-
Scenario: Files drop forbid subfolder creation without a nickname
91+
Scenario: Files request forbid subfolder creation without a nickname
7692
Given user "user0" exists
7793
And As an "user0"
7894
And user "user0" created a folder "/drop"
7995
And as "user0" creating a share with
8096
| path | drop |
8197
| shareType | 3 |
8298
| publicUpload | true |
99+
| attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
83100
And Updating last share with
84101
| permissions | 4 |
85102
When dropping file "/folder/a.txt" with "abc"
@@ -139,7 +156,7 @@ Feature: FilesDrop
139156
When Downloading file "/drop/Alice/folder (2)"
140157
Then the HTTP status code should be "200"
141158
And Downloaded content should be "its a file"
142-
159+
143160
Scenario: Put file same file multiple times via files drop
144161
Given user "user0" exists
145162
And As an "user0"

0 commit comments

Comments
 (0)