Skip to content

Commit 66e09eb

Browse files
ArtificialOwlArtificialOwl
authored
Merge pull request #49646 from nextcloud/fix/noid/trigger-field-insert
fix(signed-request): trigger metadata insert with default value manually
2 parents bca864d + b61a266 commit 66e09eb

8 files changed

Lines changed: 109 additions & 40 deletions

File tree

‎apps/cloud_federation_api/lib/Controller/RequestHandlerController.php‎

Lines changed: 35 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
namespace OCA\CloudFederationAPI\Controller;
77

8+
use NCU\Federation\ISignedCloudFederationProvider;
89
use NCU\Security\Signature\Exceptions\IdentityNotFoundException;
910
use NCU\Security\Signature\Exceptions\IncomingRequestException;
1011
use NCU\Security\Signature\Exceptions\SignatoryNotFoundException;
@@ -37,8 +38,6 @@
3738
use OCP\IURLGenerator;
3839
use OCP\IUserManager;
3940
use OCP\Share\Exceptions\ShareNotFound;
40-
use OCP\Share\IProviderFactory;
41-
use OCP\Share\IShare;
4241
use OCP\Util;
4342
use Psr\Log\LoggerInterface;
4443

@@ -68,7 +67,6 @@ public function __construct(
6867
private ICloudIdManager $cloudIdManager,
6968
private readonly ISignatureManager $signatureManager,
7069
private readonly OCMSignatoryManager $signatoryManager,
71-
private readonly IProviderFactory $shareProviderFactory,
7270
) {
7371
parent::__construct($appName, $request);
7472
}
@@ -234,16 +232,6 @@ public function addShare($shareWith, $name, $description, $providerId, $owner, $
234232
#[PublicPage]
235233
#[BruteForceProtection(action: 'receiveFederatedShareNotification')]
236234
public function receiveNotification($notificationType, $resourceType, $providerId, ?array $notification) {
237-
try {
238-
// if request is signed and well signed, no exception are thrown
239-
// if request is not signed and host is known for not supporting signed request, no exception are thrown
240-
$signedRequest = $this->getSignedRequest();
241-
$this->confirmShareOrigin($signedRequest, $notification['sharedSecret'] ?? '');
242-
} catch (IncomingRequestException $e) {
243-
$this->logger->warning('incoming request exception', ['exception' => $e]);
244-
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
245-
}
246-
247235
// check if all required parameters are set
248236
if ($notificationType === null ||
249237
$resourceType === null ||
@@ -259,6 +247,16 @@ public function receiveNotification($notificationType, $resourceType, $providerI
259247
);
260248
}
261249

250+
try {
251+
// if request is signed and well signed, no exception are thrown
252+
// if request is not signed and host is known for not supporting signed request, no exception are thrown
253+
$signedRequest = $this->getSignedRequest();
254+
$this->confirmNotificationIdentity($signedRequest, $resourceType, $notification);
255+
} catch (IncomingRequestException $e) {
256+
$this->logger->warning('incoming request exception', ['exception' => $e]);
257+
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
258+
}
259+
262260
try {
263261
$provider = $this->cloudFederationProviderManager->getCloudFederationProvider($resourceType);
264262
$result = $provider->notificationReceived($notificationType, $providerId, $notification);
@@ -387,50 +385,53 @@ private function confirmSignedOrigin(?IIncomingSignedRequest $signedRequest, str
387385
}
388386
}
389387

390-
391388
/**
392-
* confirm that the value related to share token is in format userid@hostname
393-
* and compare hostname with the origin of the signed request.
389+
* confirm identity of the remote instance on notification, based on the share token.
394390
*
395391
* If request is not signed, we still verify that the hostname from the extracted value does,
396392
* actually, not support signed request
397393
*
398394
* @param IIncomingSignedRequest|null $signedRequest
399-
* @param string $token
395+
* @param string $resourceType
396+
* @param string $sharedSecret
400397
*
401398
* @throws IncomingRequestException
399+
* @throws BadRequestException
402400
*/
403-
private function confirmShareOrigin(?IIncomingSignedRequest $signedRequest, string $token): void {
404-
if ($token === '') {
401+
private function confirmNotificationIdentity(
402+
?IIncomingSignedRequest $signedRequest,
403+
string $resourceType,
404+
array $notification,
405+
): void {
406+
$sharedSecret = $notification['sharedSecret'] ?? '';
407+
if ($sharedSecret === '') {
405408
throw new BadRequestException(['sharedSecret']);
406409
}
407410

408-
$provider = $this->shareProviderFactory->getProviderForType(IShare::TYPE_REMOTE);
409-
$share = $provider->getShareByToken($token);
410411
try {
411-
$this->confirmShareEntry($signedRequest, $share->getSharedWith());
412-
} catch (IncomingRequestException $e) {
413-
// notification might come from the instance that owns the share
414-
$this->logger->debug('could not confirm origin on sharedWith (' . $share->getSharedWIth() . '); going with shareOwner (' . $share->getShareOwner() . ')', ['exception' => $e]);
415-
try {
416-
$this->confirmShareEntry($signedRequest, $share->getShareOwner());
417-
} catch (IncomingRequestException $f) {
418-
// if both entry are failing, we log first exception as warning and second exception
419-
// will be logged as warning by the controller
420-
$this->logger->warning('could not confirm origin on sharedWith (' . $share->getSharedWIth() . '); going with shareOwner (' . $share->getShareOwner() . ')', ['exception' => $e]);
421-
throw $f;
412+
$provider = $this->cloudFederationProviderManager->getCloudFederationProvider($resourceType);
413+
if ($provider instanceof ISignedCloudFederationProvider) {
414+
$identity = $provider->getFederationIdFromSharedSecret($sharedSecret, $notification);
415+
} else {
416+
$this->logger->debug('cloud federation provider {provider} does not implements ISignedCloudFederationProvider', ['provider' => $provider::class]);
417+
return;
422418
}
419+
} catch (\Exception $e) {
420+
throw new IncomingRequestException($e->getMessage());
423421
}
422+
423+
$this->confirmNotificationEntry($signedRequest, $identity);
424424
}
425425

426+
426427
/**
427428
* @param IIncomingSignedRequest|null $signedRequest
428429
* @param string $entry
429430
*
430431
* @return void
431432
* @throws IncomingRequestException
432433
*/
433-
private function confirmShareEntry(?IIncomingSignedRequest $signedRequest, string $entry): void {
434+
private function confirmNotificationEntry(?IIncomingSignedRequest $signedRequest, string $entry): void {
434435
$instance = $this->getHostFromFederationId($entry);
435436
if ($signedRequest === null) {
436437
try {
@@ -440,7 +441,7 @@ private function confirmShareEntry(?IIncomingSignedRequest $signedRequest, strin
440441
return;
441442
}
442443
} elseif ($instance !== $signedRequest->getOrigin()) {
443-
throw new IncomingRequestException('token sharedWith (' . $instance . ') not linked to origin (' . $signedRequest->getOrigin() . ')');
444+
throw new IncomingRequestException('remote instance ' . $instance . ' not linked to origin ' . $signedRequest->getOrigin());
444445
}
445446
}
446447

‎apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
namespace OCA\FederatedFileSharing\OCM;
77

8+
use NCU\Federation\ISignedCloudFederationProvider;
89
use OC\AppFramework\Http;
910
use OC\Files\Filesystem;
1011
use OCA\FederatedFileSharing\AddressHandler;
@@ -21,7 +22,6 @@
2122
use OCP\Federation\Exceptions\BadRequestException;
2223
use OCP\Federation\Exceptions\ProviderCouldNotAddShareException;
2324
use OCP\Federation\ICloudFederationFactory;
24-
use OCP\Federation\ICloudFederationProvider;
2525
use OCP\Federation\ICloudFederationProviderManager;
2626
use OCP\Federation\ICloudFederationShare;
2727
use OCP\Federation\ICloudIdManager;
@@ -37,11 +37,13 @@
3737
use OCP\Server;
3838
use OCP\Share\Exceptions\ShareNotFound;
3939
use OCP\Share\IManager;
40+
use OCP\Share\IProviderFactory;
4041
use OCP\Share\IShare;
4142
use OCP\Util;
4243
use Psr\Log\LoggerInterface;
44+
use SensitiveParameter;
4345

44-
class CloudFederationProviderFiles implements ICloudFederationProvider {
46+
class CloudFederationProviderFiles implements ISignedCloudFederationProvider {
4547
/**
4648
* CloudFederationProvider constructor.
4749
*/
@@ -63,6 +65,7 @@ public function __construct(
6365
private Manager $externalShareManager,
6466
private LoggerInterface $logger,
6567
private IFilenameValidator $filenameValidator,
68+
private readonly IProviderFactory $shareProviderFactory,
6669
) {
6770
}
6871

@@ -747,4 +750,32 @@ public function getUserDisplayName(string $userId): string {
747750

748751
return $slaveService->getUserDisplayName($this->cloudIdManager->removeProtocolFromUrl($userId), false);
749752
}
753+
754+
/**
755+
* @inheritDoc
756+
*
757+
* @param string $sharedSecret
758+
* @param array $payload
759+
* @return string
760+
*/
761+
public function getFederationIdFromSharedSecret(
762+
#[SensitiveParameter]
763+
string $sharedSecret,
764+
array $payload,
765+
): string {
766+
$provider = $this->shareProviderFactory->getProviderForType(IShare::TYPE_REMOTE);
767+
try {
768+
$share = $provider->getShareByToken($sharedSecret);
769+
} catch (ShareNotFound) {
770+
return '';
771+
}
772+
773+
// if uid_owner is a local account, the request comes from the recipient
774+
// if not, request comes from the instance that owns the share and recipient is the re-sharer
775+
if ($this->userManager->get($share->getShareOwner()) !== null) {
776+
return $share->getSharedWith();
777+
} else {
778+
return $share->getShareOwner();
779+
}
780+
}
750781
}

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
'NCU\\Config\\Exceptions\\UnknownKeyException' => $baseDir . '/lib/unstable/Config/Exceptions/UnknownKeyException.php',
1313
'NCU\\Config\\IUserConfig' => $baseDir . '/lib/unstable/Config/IUserConfig.php',
1414
'NCU\\Config\\ValueType' => $baseDir . '/lib/unstable/Config/ValueType.php',
15+
'NCU\\Federation\\ISignedCloudFederationProvider' => $baseDir . '/lib/unstable/Federation/ISignedCloudFederationProvider.php',
1516
'NCU\\Security\\Signature\\Enum\\DigestAlgorithm' => $baseDir . '/lib/unstable/Security/Signature/Enum/DigestAlgorithm.php',
1617
'NCU\\Security\\Signature\\Enum\\SignatoryStatus' => $baseDir . '/lib/unstable/Security/Signature/Enum/SignatoryStatus.php',
1718
'NCU\\Security\\Signature\\Enum\\SignatoryType' => $baseDir . '/lib/unstable/Security/Signature/Enum/SignatoryType.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
5353
'NCU\\Config\\Exceptions\\UnknownKeyException' => __DIR__ . '/../../..' . '/lib/unstable/Config/Exceptions/UnknownKeyException.php',
5454
'NCU\\Config\\IUserConfig' => __DIR__ . '/../../..' . '/lib/unstable/Config/IUserConfig.php',
5555
'NCU\\Config\\ValueType' => __DIR__ . '/../../..' . '/lib/unstable/Config/ValueType.php',
56+
'NCU\\Federation\\ISignedCloudFederationProvider' => __DIR__ . '/../../..' . '/lib/unstable/Federation/ISignedCloudFederationProvider.php',
5657
'NCU\\Security\\Signature\\Enum\\DigestAlgorithm' => __DIR__ . '/../../..' . '/lib/unstable/Security/Signature/Enum/DigestAlgorithm.php',
5758
'NCU\\Security\\Signature\\Enum\\SignatoryStatus' => __DIR__ . '/../../..' . '/lib/unstable/Security/Signature/Enum/SignatoryStatus.php',
5859
'NCU\\Security\\Signature\\Enum\\SignatoryType' => __DIR__ . '/../../..' . '/lib/unstable/Security/Signature/Enum/SignatoryType.php',

‎lib/private/OCM/OCMDiscoveryService.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
namespace OC\OCM;
1111

12+
use GuzzleHttp\Exception\ConnectException;
1213
use JsonException;
1314
use OCP\AppFramework\Http;
1415
use OCP\Http\Client\IClientService;
@@ -50,7 +51,7 @@ public function discover(string $remote, bool $skipCache = false): IOCMProvider
5051
// if scheme not specified, we test both;
5152
try {
5253
return $this->discover('https://' . $remote, $skipCache);
53-
} catch (OCMProviderException) {
54+
} catch (OCMProviderException|ConnectException) {
5455
return $this->discover('http://' . $remote, $skipCache);
5556
}
5657
}

‎lib/private/Security/Signature/SignatureManager.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ private function confirmIncomingRequestSignature(
142142
if ($ttlSignatory > 0 && $knownSignatory->getLastUpdated() < (time() - $ttlSignatory)) {
143143
$signatory = $this->getSaneRemoteSignatory($signatoryManager, $signedRequest);
144144
$this->updateSignatoryMetadata($signatory);
145-
$knownSignatory->setMetadata($signatory->getMetadata());
145+
$knownSignatory->setMetadata($signatory->getMetadata() ?? []);
146146
}
147147

148148
$signedRequest->setSignatory($knownSignatory);
@@ -353,6 +353,7 @@ private function insertSignatory(Signatory $signatory): void {
353353
$time = time();
354354
$signatory->setCreation($time);
355355
$signatory->setLastUpdated($time);
356+
$signatory->setMetadata($signatory->getMetadata() ?? []); // trigger insert on field metadata using current or default value
356357
$this->mapper->insert($signatory);
357358
}
358359

‎lib/unstable/Federation/ISignedCloudFederationProvider.php‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2018 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace NCU\Federation;
10+
11+
use OCP\Federation\ICloudFederationProvider;
12+
13+
/**
14+
* Interface ICloudFederationProvider
15+
*
16+
* Enable apps to create their own cloud federation provider
17+
*
18+
* @experimental 31.0.0
19+
*/
20+
interface ISignedCloudFederationProvider extends ICloudFederationProvider {
21+
22+
/**
23+
* returns federationId in direct relation (as recipient or as author) of a sharedSecret
24+
* the federationId must be the one at the remote end
25+
*
26+
* @param string $sharedSecret
27+
* @param array $payload
28+
*
29+
* @experimental 31.0.0
30+
* @return string
31+
*/
32+
public function getFederationIdFromSharedSecret(string $sharedSecret, array $payload): string;
33+
}

‎lib/unstable/Security/Signature/Model/Signatory.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
* @method void setAccount(string $account)
4343
* @method string getAccount()
4444
* @method void setMetadata(array $metadata)
45-
* @method array getMetadata()
45+
* @method ?array getMetadata()
4646
* @method void setCreation(int $creation)
4747
* @method int getCreation()
4848
* @method void setLastUpdated(int $creation)
@@ -59,7 +59,7 @@ class Signatory extends Entity implements JsonSerializable {
5959
protected string $account = '';
6060
protected int $type = 9;
6161
protected int $status = 1;
62-
protected array $metadata = [];
62+
protected ?array $metadata = null;
6363
protected int $creation = 0;
6464
protected int $lastUpdated = 0;
6565

0 commit comments

Comments
 (0)