Skip to content

Commit 36bcafd

Browse files
max-nextcloudmax-nextcloud
committed
fix(middleware): get fileId from context rather than documentId
Signed-off-by: Max <max@nextcloud.com>
1 parent aac21d3 commit 36bcafd

4 files changed

Lines changed: 36 additions & 29 deletions

File tree

‎lib/Middleware/SessionMiddleware.php‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,18 +128,24 @@ private function assertDocumentSession(ISessionAwareController $controller): voi
128128
* @throws InvalidSessionException
129129
*/
130130
private function assertUserOrShareToken(ISessionAwareController $controller): void {
131-
$fileId = (int)$this->request->getParam('documentId');
131+
$documentId = (int)$this->request->getParam('documentId');
132132
$shareToken = (string)$this->request->getParam('shareToken');
133133
$userId = $this->userSession->getUser()?->getUID();
134134

135+
$document = $this->documentService->getDocument($documentId);
136+
if (!$document || $document->getContextType() !== 'file') {
137+
throw new InvalidSessionException();
138+
}
139+
$fileId = $document->getContextId();
140+
135141
if ($shareToken !== '') {
136-
$documentId = $this->fileService->getDocumentIdFromShare($fileId, $shareToken);
142+
$this->fileService->checkFileAccessFromShare($fileId, $shareToken);
137143
$controller->setDocumentId($documentId);
138144
return;
139145
}
140146

141147
if ($userId !== null) {
142-
$documentId = $this->fileService->getDocumentIdForUser($fileId, $userId);
148+
$this->fileService->checkFileAccessForUser($fileId, $userId);
143149
$controller->setUserId($userId);
144150
$controller->setDocumentId($documentId);
145151
return;

‎lib/Service/FileService.php‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ public function checkSharePermissions(string $shareToken, int $permission = Cons
168168
}
169169
}
170170

171-
public function getDocumentIdFromShare(int $fileId, string $shareToken): int {
171+
public function checkFileAccessFromShare(int $fileId, string $shareToken): void {
172172
try {
173173
$share = $this->shareManager->getShareByToken($shareToken);
174174
} catch (ShareNotFound) {
@@ -208,15 +208,12 @@ public function getDocumentIdFromShare(int $fileId, string $shareToken): int {
208208
if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
209209
throw new InvalidSessionException();
210210
}
211-
212-
return $fileId;
213211
}
214212

215-
public function getDocumentIdForUser(int $fileId, string $userId): int {
216-
if ($this->rootFolder->getUserFolder($userId)->getFirstNodeById($fileId) !== null) {
217-
return $fileId;
213+
public function checkFileAccessForUser(int $fileId, string $userId): void {
214+
if ($this->rootFolder->getUserFolder($userId)->getFirstNodeById($fileId) === null) {
215+
throw new InvalidSessionException();
218216
}
219-
throw new InvalidSessionException();
220217
}
221218

222219
public function loadContent(File $file): ?string {

‎tests/unit/Middleware/SessionMiddlewareTest.php‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,13 @@ protected function setUp(): void {
3939
$this->userManager = $this->createMock(IUserManager::class);
4040
$this->fileService = $this->createMock(FileService::class);
4141

42+
$document = new Document(
43+
id: 111,
44+
contextType: 'file',
45+
contextId: 999,
46+
);
47+
$this->documentService->method('getDocument')->with(111)->willReturn($document);
48+
4249
$this->middleware = new SessionMiddleware(
4350
$this->request,
4451
$this->sessionService,
@@ -53,20 +60,20 @@ protected function setUp(): void {
5360
public function testUnauthenticatedAccessBlocked(): void {
5461
$this->expectException(InvalidSessionException::class);
5562

56-
$this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException());
63+
$this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException());
5764

5865
$this->invokeMiddleware('token');
5966
}
6067

6168
public function testAuthenticatedSingleIdAllowed(): void {
62-
$this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willReturn(999);
69+
$this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token')->willReturn();
6370

6471
$this->invokeMiddleware('token');
6572
$this->assertTrue(true);
6673
}
6774

6875
public function testLoggedInUserWithValidToken(): void {
69-
$this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willReturn(999);
76+
$this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token')->willReturn();
7077

7178
$controller = $this->createMock(ISessionAwareController::class);
7279
$controller->expects($this->never())->method('setUserId');
@@ -79,7 +86,7 @@ public function testLoggedInUserWithOwnFile(): void {
7986
$user = $this->createMock(IUser::class);
8087
$user->method('getUID')->willReturn('user1');
8188

82-
$this->fileService->method('getDocumentIdForUser')->with(999, 'user1')->willReturn(999);
89+
$this->fileService->expects($this->once())->method('checkFileAccessForUser')->with(999, 'user1')->willReturn();
8390

8491
$controller = $this->createMock(ISessionAwareController::class);
8592
$controller->expects($this->once())->method('setUserId');
@@ -94,7 +101,7 @@ public function testLoggedInUserMissingFile(): void {
94101
$user = $this->createMock(IUser::class);
95102
$user->method('getUID')->willReturn('user1');
96103

97-
$this->fileService->method('getDocumentIdForUser')->with(999, 'user1')->willThrowException(new InvalidSessionException());
104+
$this->fileService->expects($this->once())->method('checkFileAccessForUser')->with(999, 'user1')->willThrowException(new InvalidSessionException());
98105

99106
$this->invokeMiddleware(null, 'user1');
100107
}
@@ -105,7 +112,7 @@ public function testLoggedInUserWithValidTokenMissingPassword(): void {
105112
$user = $this->createMock(IUser::class);
106113
$user->method('getUID')->willReturn('user1');
107114

108-
$this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException());
115+
$this->fileService->expects($this->once))->method('checkFileAccessFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException());
109116

110117
$this->invokeMiddleware('token', 'user1');
111118
}
@@ -196,7 +203,7 @@ private function invokeAssertDocumentSession(ISessionAwareController $controller
196203

197204
private function invokeMiddleware(?string $token, ?string $userName = null, ?ISessionAwareController $controller = null): void {
198205
$this->request->method('getParam')->willReturnMap([
199-
['documentId', null, 999],
206+
['documentId', null, 111],
200207
['shareToken', null, $token],
201208
]);
202209

‎tests/unit/Service/FileServiceTest.php‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -79,14 +79,13 @@ public function testInvalidToken(): void {
7979

8080
$this->shareManager->method('getShareByToken')->with('invalid')->willThrowException(new ShareNotFound());
8181

82-
$this->fileService->getDocumentIdFromShare(123, 'invalid');
82+
$this->fileService->checkFileAccessFromShare(123, 'invalid');
8383
}
8484

8585
public function testValidTokenWithoutPassword(): void {
8686
$share = $this->createShare('plain-share');
8787

88-
$result = $this->invokeGetDocumentIdFromShare(123, $share);
89-
self::assertEquals(123, $result);
88+
$this->invokeCheckFileAccessFromShare(123, $share);
9089
}
9190

9291
public function testValidTokenMissingPassword(): void {
@@ -95,23 +94,21 @@ public function testValidTokenMissingPassword(): void {
9594
$share = $this->createShare('protected-share', 'password');
9695
$this->session->method('get')->with('public_link_authenticated')->willReturn(null);
9796

98-
$this->invokeGetDocumentIdFromShare(123, $share);
97+
$this->invokeCheckFileAccessFromShare(123, $share);
9998
}
10099

101100
public function testValidTokenWithPasswordArray(): void {
102101
$share = $this->createShare('42', 'password');
103102
$this->session->method('get')->with('public_link_authenticated')->willReturn(['1', '42']);
104103

105-
$result = $this->invokeGetDocumentIdFromShare(123, $share);
106-
self::assertEquals(123, $result);
104+
$this->invokeCheckFileAccessFromShare(123, $share);
107105
}
108106

109107
public function testValidTokenWithSinglePassword(): void {
110108
$share = $this->createShare('42', 'password');
111109
$this->session->method('get')->with('public_link_authenticated')->willReturn('42');
112110

113-
$result = $this->invokeGetDocumentIdFromShare(123, $share);
114-
self::assertEquals(123, $result);
111+
$this->invokeCheckFileAccessFromShare(123, $share);
115112
}
116113

117114
public function testValidTokenWithOtherPassword(): void {
@@ -120,7 +117,7 @@ public function testValidTokenWithOtherPassword(): void {
120117
$share = $this->createShare('42', 'password');
121118
$this->session->method('get')->with('public_link_authenticated')->willReturn('10');
122119

123-
$this->invokeGetDocumentIdFromShare(123, $share);
120+
$this->invokeCheckFileAccessFromShare(123, $share);
124121
}
125122

126123
public function testValidTokenWithOtherPasswords(): void {
@@ -129,17 +126,17 @@ public function testValidTokenWithOtherPasswords(): void {
129126
$share = $this->createShare('42', 'password');
130127
$this->session->method('get')->with('public_link_authenticated')->willReturn(['10', '20', '30']);
131128

132-
$this->invokeGetDocumentIdFromShare(123, $share);
129+
$this->invokeCheckFileAccessFromShare(123, $share);
133130
}
134131

135-
private function invokeGetDocumentIdFromShare(int $fileId, IShare $share): int {
132+
private function invokeCheckFileAccessFromShare(int $fileId, IShare $share): void {
136133
$this->shareManager->method('getShareByToken')->willReturn($share);
137134

138135
$folder = $this->createMock(Folder::class);
139136
$folder->method('getFirstNodeById')->willReturn($this->createMock(File::class));
140137
$this->rootFolder->method('getUserFolder')->with('owner')->willReturn($folder);
141138

142-
return $this->fileService->getDocumentIdFromShare($fileId, 'token');
139+
return $this->fileService->checkFileAccessFromShare($fileId, 'token');
143140
}
144141

145142
private function createShare(string $id, ?string $password = null): IShare {

0 commit comments

Comments
 (0)