Skip to content

Commit 63e627b

Browse files
zak39zak39
authored
Merge pull request #1723 from arawa/fix/restrict-workspace-members-endpoints
Prevent workspace managers from seeing the members of workspaces they don't belong to
2 parents 264812f + 0367a7d commit 63e627b

6 files changed

Lines changed: 283 additions & 0 deletions

File tree

‎lib/AppInfo/Application.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use OCA\Workspace\Middleware\SpacenameForbiddenCharactersMiddleware;
3939
use OCA\Workspace\Middleware\WorkspaceAccessControlMiddleware;
4040
use OCA\Workspace\Middleware\WorkspaceManagerAccessMiddleware;
41+
use OCA\Workspace\Middleware\WorkspaceMemberAccessMiddleware;
4142
use OCA\Workspace\Service\SpaceService;
4243
use OCA\Workspace\Service\UserService;
4344
use OCP\AppFramework\App;
@@ -93,6 +94,7 @@ public function register(IRegistrationContext $context): void {
9394
$context->registerMiddleware(IsGeneralManagerMiddleware::class);
9495
$context->registerMiddleware(GeneralManagerAccessMiddleware::class);
9596
$context->registerMiddleware(WorkspaceManagerAccessMiddleware::class);
97+
$context->registerMiddleware(WorkspaceMemberAccessMiddleware::class);
9698
$context->registerMiddleware(NotificationMiddleware::class);
9799
$context->registerMiddleware(DuplicateSpacenameMiddleware::class);
98100
$context->registerMiddleware(SpacenameForbiddenCharactersMiddleware::class);

‎lib/Attributes/WorkspaceMemberRequired.php‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<?php
2+
3+
/**
4+
* @copyright Copyright (c) 2022 Arawa
5+
*
6+
* @author 2026 Baptiste Fotia <fotia.baptiste@hotmail.com>
7+
* @license GNU AGPL version 3 or any later version
8+
*
9+
* This program is free software: you can redistribute it and/or modify
10+
* it under the terms of the GNU Affero General Public License as
11+
* published by the Free Software Foundation, either version 3 of the
12+
* License, or (at your option) any later version.
13+
*
14+
* This program is distributed in the hope that it will be useful,
15+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
16+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
17+
* GNU Affero General Public License for more details.
18+
*
19+
* You should have received a copy of the GNU Affero General Public License
20+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
21+
*
22+
*/
23+
24+
namespace OCA\Workspace\Attribute;
25+
26+
use Attribute;
27+
28+
/**
29+
* All methods with this attribute will be accessible to General Managers, and to users who are Workspace Manager or member of the workspace given by the `spaceId` parameter.
30+
*/
31+
#[Attribute(Attribute::TARGET_METHOD)]
32+
class WorkspaceMemberRequired {
33+
}

‎lib/Controller/WorkspaceController.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
namespace OCA\Workspace\Controller;
2727

28+
use OCA\Workspace\Attribute\WorkspaceMemberRequired;
2829
use OCA\Workspace\Db\SpaceMapper;
2930
use OCA\Workspace\Exceptions\BadRequestException;
3031
use OCA\Workspace\Folder\RootFolder;
@@ -211,6 +212,7 @@ public function countWorkspaces(?string $search = null): JSONResponse {
211212
/**
212213
* @NoAdminRequired
213214
*/
215+
#[WorkspaceMemberRequired]
214216
public function getUsers(int $spaceId): JSONResponse {
215217

216218
$space = $this->spaceMapper->find($spaceId);
@@ -241,6 +243,7 @@ public function getUsers(int $spaceId): JSONResponse {
241243
/**
242244
* @NoAdminRequired
243245
*/
246+
#[WorkspaceMemberRequired]
244247
public function getAdmins(int $spaceId): JSONResponse {
245248

246249
$space = $this->spaceMapper->find($spaceId);

‎lib/Middleware/WorkspaceMemberAccessMiddleware.php‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<?php
2+
3+
namespace OCA\Workspace\Middleware;
4+
5+
use Exception;
6+
use OCA\Workspace\Attribute\WorkspaceMemberRequired;
7+
use OCA\Workspace\Middleware\Exceptions\AccessDeniedException;
8+
use OCA\Workspace\Service\SpaceService;
9+
use OCA\Workspace\Service\UserService;
10+
use OCP\AppFramework\Controller;
11+
use OCP\AppFramework\Http;
12+
use OCP\AppFramework\Http\JSONResponse;
13+
use OCP\AppFramework\Http\Response;
14+
use OCP\AppFramework\Middleware;
15+
use OCP\IRequest;
16+
17+
class WorkspaceMemberAccessMiddleware extends Middleware {
18+
19+
public function __construct(
20+
private IRequest $request,
21+
private UserService $userService,
22+
private SpaceService $spaceService,
23+
) {
24+
}
25+
26+
public function beforeController(Controller $controller, string $methodName): void {
27+
$reflectionMethod = new \ReflectionMethod($controller, $methodName);
28+
$hasAttribute = $reflectionMethod->getAttributes(WorkspaceMemberRequired::class);
29+
30+
if (empty($hasAttribute)) {
31+
return;
32+
}
33+
34+
if ($this->userService->isUserGeneralAdmin()) {
35+
return;
36+
}
37+
38+
$spaceId = $this->request->getParam('spaceId');
39+
$space = $spaceId !== null ? $this->spaceService->find((int)$spaceId) : null;
40+
41+
if ($space === null) {
42+
throw new AccessDeniedException();
43+
}
44+
45+
$space = $space->jsonSerialize();
46+
47+
if ($this->userService->isSpaceManagerOfSpace($space) || $this->userService->isUserOfSpace($space)) {
48+
return;
49+
}
50+
51+
throw new AccessDeniedException();
52+
}
53+
54+
public function afterException(Controller $controller, string $methodName, Exception $exception): Response {
55+
if ($exception instanceof AccessDeniedException) {
56+
return new JSONResponse([
57+
'status' => 'forbidden',
58+
'msg' => 'You are not allowed to perform this action.'
59+
], Http::STATUS_FORBIDDEN);
60+
}
61+
62+
throw $exception;
63+
}
64+
}

‎lib/Service/UserService.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,14 @@ public function canAccessApp(): bool {
142142
return false;
143143
}
144144

145+
/**
146+
* @param array $space The space
147+
* @return boolean true if user is member of the user group of the specified workspace, false otherwise
148+
*/
149+
public function isUserOfSpace(array $space): bool {
150+
return $this->groupManager->isInGroup($this->userSession->getUser()->getUID(), UserGroup::get($space['id']));
151+
}
152+
145153
/**
146154
* @param array $id The space id
147155
* @return boolean true if user is space manager of the specified workspace, false otherwise

‎tests/Unit/Middleware/WorkspaceMemberAccessMiddlewareTest.php‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
<?php
2+
3+
namespace OCA\Workspace\Tests\Unit\Middleware;
4+
5+
use OCA\Workspace\Controller\WorkspaceController;
6+
use OCA\Workspace\Db\Space;
7+
use OCA\Workspace\Middleware\Exceptions\AccessDeniedException;
8+
use OCA\Workspace\Middleware\WorkspaceMemberAccessMiddleware;
9+
use OCA\Workspace\Service\SpaceService;
10+
use OCA\Workspace\Service\UserService;
11+
use OCP\AppFramework\Http;
12+
use OCP\IRequest;
13+
use PHPUnit\Framework\MockObject\MockObject;
14+
use PHPUnit\Framework\TestCase;
15+
16+
class WorkspaceMemberAccessMiddlewareTest extends TestCase {
17+
18+
private MockObject&IRequest $request;
19+
private MockObject&UserService $userService;
20+
private MockObject&SpaceService $spaceService;
21+
private MockObject&WorkspaceController $controller;
22+
private WorkspaceMemberAccessMiddleware $middleware;
23+
24+
public function setUp(): void {
25+
parent::setUp();
26+
27+
$this->request = $this->createMock(IRequest::class);
28+
$this->userService = $this->createMock(UserService::class);
29+
$this->spaceService = $this->createMock(SpaceService::class);
30+
31+
// No method is mocked, so the attributes of the real methods stay readable by reflection.
32+
$this->controller = $this->getMockBuilder(WorkspaceController::class)
33+
->disableOriginalConstructor()
34+
->onlyMethods([])
35+
->getMock()
36+
;
37+
38+
$this->middleware = new WorkspaceMemberAccessMiddleware(
39+
$this->request,
40+
$this->userService,
41+
$this->spaceService,
42+
);
43+
}
44+
45+
private function mockSpace(int $spaceId): void {
46+
/** @var MockObject&Space */
47+
$space = $this->createMock(Space::class);
48+
$space
49+
->method('jsonSerialize')
50+
->willReturn(['id' => $spaceId])
51+
;
52+
53+
$this->request
54+
->method('getParam')
55+
->with('spaceId')
56+
->willReturn((string)$spaceId)
57+
;
58+
59+
$this->spaceService
60+
->method('find')
61+
->with($spaceId)
62+
->willReturn($space)
63+
;
64+
}
65+
66+
public function testMethodWithoutAttributeIsNotChecked(): void {
67+
$this->userService
68+
->expects($this->never())
69+
->method('isUserGeneralAdmin')
70+
;
71+
72+
$this->request
73+
->expects($this->never())
74+
->method('getParam')
75+
;
76+
77+
$this->middleware->beforeController($this->controller, 'findAll');
78+
}
79+
80+
public function testGeneralManagerIsAllowed(): void {
81+
$this->userService
82+
->expects($this->once())
83+
->method('isUserGeneralAdmin')
84+
->willReturn(true)
85+
;
86+
87+
$this->spaceService
88+
->expects($this->never())
89+
->method('find')
90+
;
91+
92+
$this->userService
93+
->expects($this->never())
94+
->method('isSpaceManagerOfSpace')
95+
;
96+
97+
$this->middleware->beforeController($this->controller, 'getUsers');
98+
}
99+
100+
public function testWorkspaceManagerOfTheSpaceIsAllowed(): void {
101+
$this->mockSpace(6);
102+
103+
$this->userService
104+
->expects($this->once())
105+
->method('isSpaceManagerOfSpace')
106+
->with(['id' => 6])
107+
->willReturn(true)
108+
;
109+
110+
// A manager is accepted without checking the user group.
111+
$this->userService
112+
->expects($this->never())
113+
->method('isUserOfSpace')
114+
;
115+
116+
$this->middleware->beforeController($this->controller, 'getAdmins');
117+
}
118+
119+
public function testMemberOfTheSpaceIsAllowed(): void {
120+
$this->mockSpace(6);
121+
122+
$this->userService
123+
->expects($this->once())
124+
->method('isSpaceManagerOfSpace')
125+
->with(['id' => 6])
126+
->willReturn(false)
127+
;
128+
129+
$this->userService
130+
->expects($this->once())
131+
->method('isUserOfSpace')
132+
->with(['id' => 6])
133+
->willReturn(true)
134+
;
135+
136+
$this->middleware->beforeController($this->controller, 'getUsers');
137+
}
138+
139+
public function testUserOutsideTheSpaceIsDenied(): void {
140+
$this->mockSpace(4);
141+
142+
$this->userService->method('isSpaceManagerOfSpace')->willReturn(false);
143+
$this->userService->method('isUserOfSpace')->willReturn(false);
144+
145+
$this->expectException(AccessDeniedException::class);
146+
147+
$this->middleware->beforeController($this->controller, 'getUsers');
148+
}
149+
150+
public function testUnknownSpaceIsDenied(): void {
151+
$this->request
152+
->method('getParam')
153+
->with('spaceId')
154+
->willReturn('999')
155+
;
156+
157+
$this->spaceService
158+
->method('find')
159+
->with(999)
160+
->willReturn(null)
161+
;
162+
163+
$this->expectException(AccessDeniedException::class);
164+
165+
$this->middleware->beforeController($this->controller, 'getAdmins');
166+
}
167+
168+
public function testAccessDeniedIsTurnedIntoForbiddenResponse(): void {
169+
$response = $this->middleware->afterException($this->controller, 'getUsers', new AccessDeniedException());
170+
171+
$this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus());
172+
}
173+
}

0 commit comments

Comments
 (0)