Skip to content

Commit 34699da

Browse files
sorbaughsorbaugh
authored
Merge pull request #55807 from nextcloud/fix/file-drop/mkcol
fix(FilesDropPlugin): Ensure all request for file request have a nickname
2 parents 0da54d3 + 901859e commit 34699da

File tree

6 files changed

+113
-39
lines changed

6 files changed

+113
-39
lines changed

‎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: 19 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ private function isChunkedUpload(RequestInterface $request): bool {
6969
public function beforeMethod(RequestInterface $request, ResponseInterface $response) {
7070
$isChunkedUpload = $this->isChunkedUpload($request);
7171

72-
// For the final MOVE request of a chunked upload it is necessary to modify the Destination header.
73-
if ($isChunkedUpload && $request->getMethod() !== 'MOVE') {
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') {
7474
return;
7575
}
7676

@@ -87,6 +87,23 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
8787
throw new MethodNotAllowed('Only PUT, MKCOL and MOVE are allowed on files drop');
8888
}
8989

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+
97+
// Retrieve the nickname from the request
98+
$nickname = $request->hasHeader('X-NC-Nickname')
99+
? trim(urldecode($request->getHeader('X-NC-Nickname')))
100+
: null;
101+
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');
105+
}
106+
90107
// If this is a folder creation request
91108
// let's stop there and let the onMkcol handle it
92109
if ($request->getMethod() === 'MKCOL') {
@@ -113,32 +130,6 @@ public function beforeMethod(RequestInterface $request, ResponseInterface $respo
113130
$rootPath = substr($path, 0, strpos($path, $token) + strlen($token));
114131
// e.g /Folder/image.jpg
115132
$relativePath = substr($path, strlen($rootPath));
116-
$isRootUpload = substr_count($relativePath, '/') === 1;
117-
118-
// Extract the attributes for the file request
119-
$isFileRequest = false;
120-
$attributes = $this->share->getAttributes();
121-
if ($attributes !== null) {
122-
$isFileRequest = $attributes->getAttribute('fileRequest', 'enabled') === true;
123-
}
124-
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-
130-
// We need a valid nickname for file requests
131-
if ($isFileRequest && !$nickname) {
132-
throw new BadRequest('A nickname header is required for file requests');
133-
}
134-
135-
// We're only allowing the upload of
136-
// long path with subfolders if a nickname is set.
137-
// This prevents confusion when uploading files and help
138-
// classify them by uploaders.
139-
if (!$nickname && !$isRootUpload) {
140-
throw new BadRequest('A nickname header is required when uploading subfolders');
141-
}
142133

143134
if ($nickname) {
144135
try {

‎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: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCP\Share\IAttributes;
1414
use OCP\Share\IShare;
1515
use PHPUnit\Framework\MockObject\MockObject;
16+
use Sabre\DAV\Exception\BadRequest;
1617
use Sabre\DAV\Server;
1718
use Sabre\HTTP\RequestInterface;
1819
use Sabre\HTTP\ResponseInterface;
@@ -23,6 +24,7 @@ class FilesDropPluginTest extends TestCase {
2324
private FilesDropPlugin $plugin;
2425

2526
private Folder&MockObject $node;
27+
private IAttributes&MockObject $attributes;
2628
private IShare&MockObject $share;
2729
private Server&MockObject $server;
2830
private RequestInterface&MockObject $request;
@@ -45,10 +47,10 @@ protected function setUp(): void {
4547
$this->request = $this->createMock(RequestInterface::class);
4648
$this->response = $this->createMock(ResponseInterface::class);
4749

48-
$attributes = $this->createMock(IAttributes::class);
50+
$this->attributes = $this->createMock(IAttributes::class);
4951
$this->share->expects($this->any())
5052
->method('getAttributes')
51-
->willReturn($attributes);
53+
->willReturn($this->attributes);
5254

5355
$this->share
5456
->method('getToken')
@@ -104,7 +106,7 @@ public function testFileAlreadyExistsValid(): void {
104106
$this->plugin->beforeMethod($this->request, $this->response);
105107
}
106108

107-
public function testMKCOL(): void {
109+
public function testFileDropMKCOLWithoutNickname(): void {
108110
$this->plugin->enable();
109111
$this->plugin->setShare($this->share);
110112

@@ -116,6 +118,45 @@ public function testMKCOL(): void {
116118
$this->plugin->beforeMethod($this->request, $this->response);
117119
}
118120

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+
133+
$this->expectException(BadRequest::class);
134+
135+
$this->plugin->beforeMethod($this->request, $this->response);
136+
}
137+
138+
public function testFileRequestMKCOLWithNickname(): void {
139+
$this->plugin->enable();
140+
$this->plugin->setShare($this->share);
141+
142+
$this->request->method('getMethod')
143+
->willReturn('MKCOL');
144+
145+
$this->attributes
146+
->method('getAttribute')
147+
->with('fileRequest', 'enabled')
148+
->willReturn(true);
149+
150+
$this->request->method('hasHeader')
151+
->with('X-NC-Nickname')
152+
->willReturn(true);
153+
$this->request->method('getHeader')
154+
->with('X-NC-Nickname')
155+
->willReturn('nickname');
156+
157+
$this->plugin->beforeMethod($this->request, $this->response);
158+
}
159+
119160
public function testSubdirPut(): void {
120161
$this->plugin->enable();
121162
$this->plugin->setShare($this->share);

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

Lines changed: 13 additions & 1 deletion
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) {
60+
public function creatingFolderInDrop($folder, $nickname = null) {
6161
$client = new Client();
6262
$options = [];
6363
if (count($this->lastShareData->data->element) > 0) {
@@ -73,10 +73,22 @@ public function creatingFolderInDrop($folder) {
7373
'X-REQUESTED-WITH' => 'XMLHttpRequest',
7474
];
7575

76+
if ($nickname) {
77+
$options['headers']['X-NC-NICKNAME'] = $nickname;
78+
}
79+
7680
try {
7781
$this->response = $client->request('MKCOL', $fullUrl, $options);
7882
} catch (\GuzzleHttp\Exception\ClientException $e) {
7983
$this->response = $e->getResponse();
8084
}
8185
}
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+
}
8294
}

‎build/integration/filesdrop_features/filesdrop.feature‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,40 +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 allows MKCOL
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
75+
Then the HTTP status code should be "400"
76+
77+
Scenario: Files request allows MKCOL with a nickname
78+
Given user "user0" exists
79+
And As an "user0"
80+
And user "user0" created a folder "/drop"
81+
And as "user0" creating a share with
82+
| path | drop |
83+
| shareType | 3 |
84+
| publicUpload | true |
85+
| attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
86+
And Updating last share with
87+
| permissions | 4 |
88+
When Creating folder "folder" in drop as "nickname"
6089
Then the HTTP status code should be "201"
6190

62-
Scenario: Files drop forbid subfolder creation without a nickname
91+
Scenario: Files request forbid subfolder creation without a nickname
6392
Given user "user0" exists
6493
And As an "user0"
6594
And user "user0" created a folder "/drop"
6695
And as "user0" creating a share with
6796
| path | drop |
6897
| shareType | 3 |
6998
| publicUpload | true |
99+
| attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
70100
And Updating last share with
71101
| permissions | 4 |
72102
When dropping file "/folder/a.txt" with "abc"

0 commit comments

Comments
 (0)