Skip to content

Commit 9df63dc

Browse files
icewind1991icewind1991
committed
refactor: always make user in external share manager explicit
relying on the session user is inconsistent as there might not be a logged in user, or the logged in user might not be the recipient of the share (reshares) Signed-off-by: Robin Appelman <robin@icewind.nl>
1 parent ee45027 commit 9df63dc

9 files changed

Lines changed: 149 additions & 183 deletions

File tree

‎apps/files_sharing/lib/Controller/ExternalSharesController.php‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use OCP\AppFramework\Http\JSONResponse;
1616
use OCP\BackgroundJob\IJobList;
1717
use OCP\IRequest;
18+
use OCP\IUser;
1819

1920
/**
2021
* Class ExternalSharesController
@@ -26,7 +27,8 @@ public function __construct(
2627
string $appName,
2728
IRequest $request,
2829
private readonly Manager $externalManager,
29-
private IJobList $jobList,
30+
private readonly IJobList $jobList,
31+
private readonly IUser $user,
3032
) {
3133
parent::__construct($appName, $request);
3234
}
@@ -36,17 +38,17 @@ public function __construct(
3638
*/
3739
#[NoAdminRequired]
3840
public function index(): JSONResponse {
39-
return new JSONResponse($this->externalManager->getOpenShares());
41+
return new JSONResponse($this->externalManager->getOpenShares($this->user));
4042
}
4143

4244
/**
4345
* @NoOutgoingFederatedSharingRequired
4446
*/
4547
#[NoAdminRequired]
4648
public function create(string $id): JSONResponse {
47-
$externalShare = $this->externalManager->getShare($id);
49+
$externalShare = $this->externalManager->getShare($id, $this->user);
4850
if ($externalShare !== false) {
49-
$this->externalManager->acceptShare($externalShare);
51+
$this->externalManager->acceptShare($externalShare, $this->user);
5052
$this->jobList->add(ExternalShareScanJob::class, [$externalShare->getUser(), $externalShare->getMountpoint()]);
5153
}
5254
return new JSONResponse();
@@ -57,9 +59,9 @@ public function create(string $id): JSONResponse {
5759
*/
5860
#[NoAdminRequired]
5961
public function destroy(string $id): JSONResponse {
60-
$externalShare = $this->externalManager->getShare($id);
62+
$externalShare = $this->externalManager->getShare($id, $this->user);
6163
if ($externalShare !== false) {
62-
$this->externalManager->declineShare($externalShare);
64+
$this->externalManager->declineShare($externalShare, $this->user);
6365
}
6466
return new JSONResponse();
6567
}

‎apps/files_sharing/lib/Controller/RemoteController.php‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\AppFramework\OCSController;
2020
use OCP\Files\IRootFolder;
2121
use OCP\IRequest;
22+
use OCP\IUser;
2223
use Psr\Log\LoggerInterface;
2324

2425
/**
@@ -34,7 +35,7 @@ public function __construct(
3435
IRequest $request,
3536
private readonly Manager $externalManager,
3637
private readonly LoggerInterface $logger,
37-
private readonly ?string $userId,
38+
private readonly IUser $user,
3839
private readonly IRootFolder $rootFolder,
3940
) {
4041
parent::__construct($appName, $request);
@@ -49,7 +50,7 @@ public function __construct(
4950
*/
5051
#[NoAdminRequired]
5152
public function getOpenShares(): DataResponse {
52-
$shares = $this->externalManager->getOpenShares();
53+
$shares = $this->externalManager->getOpenShares($this->user);
5354
$shares = array_map($this->extendShareInfo(...), $shares);
5455
return new DataResponse($shares);
5556
}
@@ -65,13 +66,13 @@ public function getOpenShares(): DataResponse {
6566
*/
6667
#[NoAdminRequired]
6768
public function acceptShare(string $id): DataResponse {
68-
$externalShare = $this->externalManager->getShare($id);
69+
$externalShare = $this->externalManager->getShare($id, $this->user);
6970
if ($externalShare === false) {
7071
$this->logger->error('Could not accept federated share with id: ' . $id . ' Share not found.', ['app' => 'files_sharing']);
7172
throw new OCSNotFoundException('Wrong share ID, share does not exist.');
7273
}
7374

74-
if (!$this->externalManager->acceptShare($externalShare)) {
75+
if (!$this->externalManager->acceptShare($externalShare, $this->user)) {
7576
$this->logger->error('Could not accept federated share with id: ' . $id, ['app' => 'files_sharing']);
7677
throw new OCSNotFoundException('Wrong share ID, share does not exist.');
7778
}
@@ -90,13 +91,13 @@ public function acceptShare(string $id): DataResponse {
9091
*/
9192
#[NoAdminRequired]
9293
public function declineShare(string $id): DataResponse {
93-
$externalShare = $this->externalManager->getShare($id);
94+
$externalShare = $this->externalManager->getShare($id, $this->user);
9495
if ($externalShare === false) {
9596
$this->logger->error('Could not decline federated share with id: ' . $id . ' Share not found.', ['app' => 'files_sharing']);
9697
throw new OCSNotFoundException('Wrong share ID, share does not exist.');
9798
}
9899

99-
if (!$this->externalManager->declineShare($externalShare)) {
100+
if (!$this->externalManager->declineShare($externalShare, $this->user)) {
100101
$this->logger->error('Could not decline federated share with id: ' . $id, ['app' => 'files_sharing']);
101102
throw new OCSNotFoundException('Wrong share ID, share does not exist.');
102103
}
@@ -112,7 +113,7 @@ private function extendShareInfo(ExternalShare $share): array {
112113
$shareData = $share->jsonSerialize();
113114

114115
$shareData['parent'] = $shareData['parent'] !== '-1' ? $shareData['parent'] : null;
115-
$userFolder = $this->rootFolder->getUserFolder($this->userId);
116+
$userFolder = $this->rootFolder->getUserFolder($this->user->getUID());
116117

117118
try {
118119
$mountPointNode = $userFolder->get($share->getMountpoint());
@@ -139,7 +140,7 @@ private function extendShareInfo(ExternalShare $share): array {
139140
*/
140141
#[NoAdminRequired]
141142
public function getShares(): DataResponse {
142-
$shares = $this->externalManager->getAcceptedShares();
143+
$shares = $this->externalManager->getAcceptedShares($this->user);
143144
$shares = array_map(fn (ExternalShare $share) => $this->extendShareInfo($share), $shares);
144145
return new DataResponse($shares);
145146
}
@@ -155,7 +156,7 @@ public function getShares(): DataResponse {
155156
*/
156157
#[NoAdminRequired]
157158
public function getShare(string $id): DataResponse {
158-
$shareInfo = $this->externalManager->getShare($id);
159+
$shareInfo = $this->externalManager->getShare($id, $this->user);
159160

160161
if ($shareInfo === false) {
161162
throw new OCSNotFoundException('share does not exist');
@@ -177,15 +178,15 @@ public function getShare(string $id): DataResponse {
177178
*/
178179
#[NoAdminRequired]
179180
public function unshare(string $id): DataResponse {
180-
$shareInfo = $this->externalManager->getShare($id);
181+
$shareInfo = $this->externalManager->getShare($id, $this->user);
181182

182183
if ($shareInfo === false) {
183184
throw new OCSNotFoundException('Share does not exist');
184185
}
185186

186-
$mountPoint = '/' . $this->userId . '/files' . $shareInfo->getMountpoint();
187+
$mountPoint = '/' . $this->user->getUID() . '/files' . $shareInfo->getMountpoint();
187188

188-
if ($this->externalManager->removeShare($mountPoint) === true) {
189+
if ($this->externalManager->removeShare($this->user, $mountPoint) === true) {
189190
return new DataResponse();
190191
} else {
191192
throw new OCSForbiddenException('Could not unshare');

‎apps/files_sharing/lib/External/Manager.php‎

Lines changed: 31 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -22,43 +22,33 @@
2222
use OCP\Files\ISetupManager;
2323
use OCP\Files\NotFoundException;
2424
use OCP\Files\NotPermittedException;
25-
use OCP\Files\Storage\IStorageFactory;
2625
use OCP\Http\Client\IClientService;
27-
use OCP\ICertificateManager;
28-
use OCP\IConfig;
2926
use OCP\IDBConnection;
3027
use OCP\IGroup;
3128
use OCP\IGroupManager;
3229
use OCP\IUser;
33-
use OCP\IUserSession;
3430
use OCP\Notification\IManager;
3531
use OCP\OCS\IDiscoveryService;
3632
use OCP\Share\IShare;
3733
use OCP\User\Exceptions\UserNotFoundException;
3834
use Psr\Log\LoggerInterface;
3935

4036
class Manager {
41-
private ?IUser $user;
42-
4337
public function __construct(
44-
private IDBConnection $connection,
45-
private \OC\Files\Mount\Manager $mountManager,
46-
private IStorageFactory $storageLoader,
47-
private IClientService $clientService,
48-
private IManager $notificationManager,
49-
private IDiscoveryService $discoveryService,
50-
private ICloudFederationProviderManager $cloudFederationProviderManager,
51-
private ICloudFederationFactory $cloudFederationFactory,
52-
private IGroupManager $groupManager,
53-
IUserSession $userSession,
54-
private IEventDispatcher $eventDispatcher,
55-
private LoggerInterface $logger,
56-
private IRootFolder $rootFolder,
57-
private ISetupManager $setupManager,
58-
private ICertificateManager $certificateManager,
59-
private ExternalShareMapper $externalShareMapper,
38+
private readonly IDBConnection $connection,
39+
private readonly \OC\Files\Mount\Manager $mountManager,
40+
private readonly IClientService $clientService,
41+
private readonly IManager $notificationManager,
42+
private readonly IDiscoveryService $discoveryService,
43+
private readonly ICloudFederationProviderManager $cloudFederationProviderManager,
44+
private readonly ICloudFederationFactory $cloudFederationFactory,
45+
private readonly IGroupManager $groupManager,
46+
private readonly IEventDispatcher $eventDispatcher,
47+
private readonly LoggerInterface $logger,
48+
private readonly IRootFolder $rootFolder,
49+
private readonly ISetupManager $setupManager,
50+
private readonly ExternalShareMapper $externalShareMapper,
6051
) {
61-
$this->user = $userSession->getUser();
6252
}
6353

6454
/**
@@ -93,8 +83,7 @@ public function addShare(ExternalShare $externalShare, IUser|IGroup $shareWith):
9383
}
9484
}
9585

96-
public function getShare(string $id, ?IUser $user = null): ExternalShare|false {
97-
$user = $user ?? $this->user;
86+
public function getShare(string $id, IUser $user): ExternalShare|false {
9887
try {
9988
$externalShare = $this->externalShareMapper->getById($id);
10089
} catch (DoesNotExistException $e) {
@@ -193,16 +182,7 @@ private function updateSubShare(ExternalShare $externalShare, IUser $user, ?stri
193182
*
194183
* @return bool True if the share could be accepted, false otherwise
195184
*/
196-
public function acceptShare(ExternalShare $externalShare, ?IUser $user = null): bool {
197-
// If we're auto-accepting a share, we need to know the user id
198-
// as there is no session available while processing the share
199-
// from the remote server request.
200-
$user = $user ?? $this->user;
201-
if ($user === null) {
202-
$this->logger->error('No user specified for accepting share');
203-
return false;
204-
}
205-
185+
public function acceptShare(ExternalShare $externalShare, IUser $user): bool {
206186
$result = false;
207187
$this->setupManager->setupForUser($user);
208188
$folder = $this->rootFolder->getUserFolder($user->getUID());
@@ -250,13 +230,7 @@ public function acceptShare(ExternalShare $externalShare, ?IUser $user = null):
250230
*
251231
* @return bool True if the share could be declined, false otherwise
252232
*/
253-
public function declineShare(ExternalShare $externalShare, ?Iuser $user = null): bool {
254-
$user = $user ?? $this->user;
255-
if ($user === null) {
256-
$this->logger->error('No user specified for declining share');
257-
return false;
258-
}
259-
233+
public function declineShare(ExternalShare $externalShare, Iuser $user): bool {
260234
$result = false;
261235

262236
if ($externalShare->getShareType() === IShare::TYPE_USER) {
@@ -282,13 +256,7 @@ public function declineShare(ExternalShare $externalShare, ?Iuser $user = null):
282256
return $result;
283257
}
284258

285-
public function processNotification(ExternalShare $remoteShare, ?IUser $user = null): void {
286-
$user = $user ?? $this->user;
287-
if ($user === null) {
288-
$this->logger->error('No user specified for processing notification');
289-
return;
290-
}
291-
259+
public function processNotification(ExternalShare $remoteShare, IUser $user): void {
292260
$filter = $this->notificationManager->createNotification();
293261
$filter->setApp('files_sharing')
294262
->setUser($user->getUID())
@@ -371,33 +339,18 @@ protected function tryOCMEndPoint(ExternalShare $externalShare, string $feedback
371339
/**
372340
* remove '/user/files' from the path and trailing slashes
373341
*/
374-
protected function stripPath(string $path): string {
375-
$prefix = '/' . $this->user->getUID() . '/files';
342+
protected function stripPath(IUser $user, string $path): string {
343+
$prefix = '/' . $user->getUID() . '/files';
376344
return rtrim(substr($path, strlen($prefix)), '/');
377345
}
378346

379-
public function getMount(array $data, ?IUser $user = null): Mount {
380-
$user = $user ?? $this->user;
381-
$data['manager'] = $this;
382-
$mountPoint = '/' . $user->getUID() . '/files' . $data['mountpoint'];
383-
$data['mountpoint'] = $mountPoint;
384-
$data['certificateManager'] = $this->certificateManager;
385-
return new Mount(Storage::class, $mountPoint, $data, $this, $this->storageLoader);
386-
}
387-
388-
protected function mountShare(array $data, ?IUser $user = null): Mount {
389-
$mount = $this->getMount($data, $user);
390-
$this->mountManager->addMount($mount);
391-
return $mount;
392-
}
393-
394347
public function getMountManager(): \OC\Files\Mount\Manager {
395348
return $this->mountManager;
396349
}
397350

398-
public function setMountPoint(string $source, string $target): bool {
399-
$source = $this->stripPath($source);
400-
$target = $this->stripPath($target);
351+
public function setMountPoint(IUser $user, string $source, string $target): bool {
352+
$source = $this->stripPath($user, $source);
353+
$target = $this->stripPath($user, $target);
401354
$sourceHash = md5($source);
402355
$targetHash = md5($target);
403356

@@ -406,16 +359,16 @@ public function setMountPoint(string $source, string $target): bool {
406359
->set('mountpoint', $qb->createNamedParameter($target))
407360
->set('mountpoint_hash', $qb->createNamedParameter($targetHash))
408361
->where($qb->expr()->eq('mountpoint_hash', $qb->createNamedParameter($sourceHash)))
409-
->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($this->user->getUID())));
362+
->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($user->getUID())));
410363

411364
$result = (bool)$qb->executeStatement();
412365

413-
$this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent($this->user));
366+
$this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent($user));
414367

415368
return $result;
416369
}
417370

418-
public function removeShare(string $mountPoint): bool {
371+
public function removeShare(IUser $user, string $mountPoint): bool {
419372
try {
420373
$mountPointObj = $this->mountManager->find($mountPoint);
421374
} catch (NotFoundException $e) {
@@ -428,11 +381,11 @@ public function removeShare(string $mountPoint): bool {
428381
}
429382
$id = $mountPointObj->getStorage()->getCache()->getId('');
430383

431-
$mountPoint = $this->stripPath($mountPoint);
384+
$mountPoint = $this->stripPath($user, $mountPoint);
432385

433386
try {
434387
try {
435-
$externalShare = $this->externalShareMapper->getByMountPointAndUser($mountPoint, $this->user);
388+
$externalShare = $this->externalShareMapper->getByMountPointAndUser($mountPoint, $user);
436389
} catch (DoesNotExistException $e) {
437390
// ignore
438391
$this->removeReShares((string)$id);
@@ -517,9 +470,9 @@ public function removeGroupShares(IGroup $group): bool {
517470
*
518471
* @return list<ExternalShare> list of open server-to-server shares
519472
*/
520-
public function getOpenShares(): array {
473+
public function getOpenShares(IUser $user): array {
521474
try {
522-
return $this->externalShareMapper->getShares($this->user, IShare::STATUS_PENDING);
475+
return $this->externalShareMapper->getShares($user, IShare::STATUS_PENDING);
523476
} catch (Exception $e) {
524477
$this->logger->emergency('Error when retrieving shares', ['exception' => $e]);
525478
return [];
@@ -531,9 +484,9 @@ public function getOpenShares(): array {
531484
*
532485
* @return list<ExternalShare> list of accepted server-to-server shares
533486
*/
534-
public function getAcceptedShares(): array {
487+
public function getAcceptedShares(IUser $user): array {
535488
try {
536-
return $this->externalShareMapper->getShares($this->user, IShare::STATUS_ACCEPTED);
489+
return $this->externalShareMapper->getShares($user, IShare::STATUS_ACCEPTED);
537490
} catch (Exception $e) {
538491
$this->logger->emergency('Error when retrieving shares', ['exception' => $e]);
539492
return [];

0 commit comments

Comments
 (0)