From 222212b93ae5d8b3ac576a024242a26103dac5a5 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 28 Nov 2024 14:32:57 +0100 Subject: [PATCH 1/5] Fix checking directory url before synchronizing --- lib/Controller/DirectoryController.php | 28 ++++----- lib/Controller/ListingsController.php | 16 ++++-- lib/Exception/DirectoryUrlException.php | 14 +++++ lib/Service/DirectoryService.php | 76 +++++++++++++++++++++---- 4 files changed, 99 insertions(+), 35 deletions(-) create mode 100644 lib/Exception/DirectoryUrlException.php diff --git a/lib/Controller/DirectoryController.php b/lib/Controller/DirectoryController.php index 0c456f15..d10e970c 100644 --- a/lib/Controller/DirectoryController.php +++ b/lib/Controller/DirectoryController.php @@ -6,6 +6,7 @@ use OCA\OpenCatalogi\Db\ListingMapper; use OCA\OpenCatalogi\Service\DirectoryService; use OCA\OpenCatalogi\Service\ObjectService; +use OCA\OpenCatalogi\Exception\DirectoryUrlException; use OCP\AppFramework\Controller; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; @@ -75,25 +76,18 @@ public function update(): JSONResponse { // Get the URL from the request parameters $url = $this->request->getParam('directory'); - - // http://directory.opencatalogi.nl - // Check if the URL parameter is provided - if (empty($url) === true) { - return new JSONResponse(['error' => 'directory parameter is required'], 400); - } - - // Check if URL contains 'local' and throw exception if it does - if (str_contains(strtolower($url), 'local')) { - return new JSONResponse(['error' => 'Local URLs are not allowed'], 400); - } - - // Validate the URL - if (!filter_var($url, FILTER_VALIDATE_URL)) { - return new JSONResponse(['error' => 'Invalid URL provided'], 400); - } // Sync the external directory with the provided URL - $data = $this->directoryService->syncExternalDirectory($url); + try{ + $data = $this->directoryService->syncExternalDirectory($url); + } catch (DirectoryUrlException $exception) { + if($exception->getMessage() === 'URL is required') { + $exception->setMessage('Property "directory" is required'); + } + + + return new JSONResponse(data: ['message' => $exception->getMessage()], statusCode: 400); + } // Return JSON response with the sync result return new JSONResponse($data); diff --git a/lib/Controller/ListingsController.php b/lib/Controller/ListingsController.php index ba24d12f..63b24bb5 100644 --- a/lib/Controller/ListingsController.php +++ b/lib/Controller/ListingsController.php @@ -6,6 +6,7 @@ use OCA\OpenCatalogi\Db\ListingMapper; use OCA\OpenCatalogi\Service\ObjectService; use OCA\OpenCatalogi\Service\DirectoryService; +use OCA\OpenCatalogi\Exception\DirectoryUrlException; use OCP\AppFramework\Controller; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; @@ -186,13 +187,16 @@ public function add(): JSONResponse // Get the URL parameter from the request $url = $this->request->getParam('url'); - // Check if the URL parameter is provided - if (empty($url) === true) { - return new JSONResponse(['error' => 'URL parameter is required'], 400); - } - // Add the new listing using the provided URL - $result = $this->directoryService->syncExternalDirectory($url); + try{ + $result = $this->directoryService->syncExternalDirectory($url); + } catch (DirectoryUrlException $exception) { + if($exception->getMessage() === 'URL is required') { + $exception->setMessage('Property "url" is required'); + } + + return new JSONResponse(data: ['message' => $exception->getMessage()], statusCode: 400); + } // Return the result as a JSON response return new JSONResponse(['success' => $result]); diff --git a/lib/Exception/DirectoryUrlException.php b/lib/Exception/DirectoryUrlException.php new file mode 100644 index 00000000..07e636d0 --- /dev/null +++ b/lib/Exception/DirectoryUrlException.php @@ -0,0 +1,14 @@ +message = $message; + } + +} diff --git a/lib/Service/DirectoryService.php b/lib/Service/DirectoryService.php index fa3833a6..fc27049b 100644 --- a/lib/Service/DirectoryService.php +++ b/lib/Service/DirectoryService.php @@ -4,18 +4,24 @@ use DateTime; use GuzzleHttp\Client; +use GuzzleHttp\Exception\ClientException; use GuzzleHttp\Exception\GuzzleException; +use GuzzleHttp\Exception\RequestException; +use GuzzleHttp\Exception\ServerException; use OCA\OpenCatalogi\Db\Catalog; use OCA\OpenCatalogi\Db\CatalogMapper; use OCA\OpenCatalogi\Db\Listing; use OCA\OpenCatalogi\Db\ListingMapper; use OCA\OpenCatalogi\Service\BroadcastService; +use OCA\OpenCatalogi\Exception\DirectoryUrlException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; +use OCP\AppFramework\Http\JSONResponse; use OCP\IAppConfig; use OCP\IURLGenerator; use Psr\Container\ContainerExceptionInterface; use Psr\Container\NotFoundExceptionInterface; +use Symfony\Component\Routing\Generator\UrlGenerator; use Symfony\Component\Uid\Uuid; /** @@ -48,7 +54,7 @@ public function __construct( private readonly ObjectService $objectService, private readonly CatalogMapper $catalogMapper, private readonly ListingMapper $listingMapper, - private readonly BroadcastService $broadcastService + private readonly BroadcastService $broadcastService, ) { $this->client = new Client([]); @@ -228,16 +234,20 @@ public function doCronSync(): array { // Extract unique directory URLs // Get unique directories from listings $uniqueDirectories = array_unique(array_column($listings, 'directory')); - + // Add default OpenCatalogi directory if not already present $defaultDirectory = 'https://directory.opencatalogi.nl/apps/opencatalogi/api/directory'; if (!in_array($defaultDirectory, $uniqueDirectories)) { $uniqueDirectories[] = $defaultDirectory; - } + } // Sync each unique directory foreach ($uniqueDirectories as $directoryUrl) { - $result = $this->syncExternalDirectory($directoryUrl); + try { + $result = $this->syncExternalDirectory($directoryUrl); + } catch (DirectoryUrlException $exception) { + continue; + } $results = array_merge_recursive($results, $result); } @@ -269,7 +279,7 @@ public function validateExternalListing(array $listing): bool * @return array The updated listing * @throws DoesNotExistException|MultipleObjectsReturnedException|ContainerExceptionInterface|NotFoundExceptionInterface */ - public function updateListing(array $newListing, array $oldListing): array{ + public function updateListing(array $newListing, array $oldListing): array{ // Let's see if these changed by checking them against the hash $newHash = hash('sha256', json_encode($newListing)); $oldHash = hash('sha256', json_encode($oldListing)); @@ -283,7 +293,36 @@ public function updateListing(array $newListing, array $oldListing): array{ return $newListing->jsonSerialize(); } - + /** + * Checks if the URL complies to basic rules. + * + * @param string $url The url to check. + * @return void + * @throws DirectoryUrlException Thrown if the url is invalid. + */ + private function checkConditions(string $url): void + { + if (empty($url) === true) { + throw new DirectoryUrlException('URL is required'); + } + + // Check if URL contains the base url of this instance. + if (str_contains(haystack: strtolower($url), needle: $this->urlGenerator->getBaseUrl()) === true) { + throw new DirectoryUrlException('Cannot load current directory'); + } + + // Check if URL contains 'local' and throw exception if it does + if (str_contains(strtolower($url), 'local') === true) { + throw new DirectoryUrlException('Local urls are not allowed'); + } + + // Validate the URL + if (empty(filter_var($url, FILTER_VALIDATE_URL)) === false) { + throw new DirectoryUrlException('Invalid URL provided'); + } + } + + /** * Synchronize with an external directory * @@ -292,25 +331,32 @@ public function updateListing(array $newListing, array $oldListing): array{ * @return array An array containing synchronization results * @throws DoesNotExistException|MultipleObjectsReturnedException|ContainerExceptionInterface|NotFoundExceptionInterface * @throws GuzzleException|\OCP\DB\Exception + * @throws DirectoryUrlException */ public function syncExternalDirectory(string $url): array { // Log successful broadcast \OC::$server->getLogger()->info('Synchronizing directory with ' . $url); + $this->checkConditions($url); + try { + $checkUrls[] = $url; // Get the directory data $result = $this->client->get($url); // Fallback to the /api/directory endpoint if the result is not JSON if (str_contains($result->getHeader('Content-Type')[0], 'application/json') === false) { + + $checkUrls[] = $url.'/index.php/apps/opencatalogi/api/directory'; $url = rtrim($url, '/').'/apps/opencatalogi/api/directory'; $result = $this->client->get($url); + $checkUrls[] = $url; } - } catch (\GuzzleHttp\Exception\ClientException $e) { + } catch (ClientException|RequestException|ServerException $e) { // If we get a 404, the directory no longer exists if ($e->getResponse()->getStatusCode() === 404) { - // Delete all listings for this directory since it no longer exists + // Delete all listings for this directory since it no longer exists $this->deleteListingsByDirectory('listing', $url); throw new \Exception('Directory no longer exists at ' . $url); } @@ -324,10 +370,12 @@ public function syncExternalDirectory(string $url): array $currentListings = $this->objectService->getObjects( objectType: 'listing', filters: [ - 'directory'=>$url, + 'directory'=>$checkUrls, ] ); + + // Remove any listings without a catalog ID from the database foreach ($currentListings as $listing) { if (empty($listing['catalog'])) { @@ -342,11 +390,13 @@ public function syncExternalDirectory(string $url): array // array_column() with null as second parameter returns complete array entries // This will return complete listing objects indexed by their catalog ID $oldListings = array_column( - $currentListings, + $currentListings, null, // null returns complete array entries rather than a specific column 'catalog' // Index by catalog ID ); + var_dump($oldListings, $currentListings); + // Initialize arrays to store results $addedListings = []; $updatedListings = []; @@ -365,10 +415,12 @@ public function syncExternalDirectory(string $url): array // Check if we already have this listing by looking up its catalog ID in the oldListings array $oldListing = $oldListings[$listing['id']] ?? null; + var_dump($listing['id'], $oldListing, $oldListings); + // If no existing listing found, prepare the new listing data if ($oldListing === null) { $listing['hash'] = hash('sha256', json_encode($listing)); - $listing['catalog'] = $listing['id']; + $listing['catalog'] = $listing['id']; unset($listing['id']); } else { // Update existing listing @@ -420,7 +472,7 @@ private function deleteListingsByDirectory(string $directoryUrl): void { ] ); // Delete all listings - foreach ($currentListings as $listing) { + foreach ($currentListings as $listing) { $this->objectService->deleteObject('listing', $listing['id']); } } From 991a3da51a317522a42b3e687c0181ecfa3b45cd Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 28 Nov 2024 14:33:09 +0100 Subject: [PATCH 2/5] Use updates as patch --- lib/Service/ObjectService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Service/ObjectService.php b/lib/Service/ObjectService.php index f0688c0b..298fdc91 100644 --- a/lib/Service/ObjectService.php +++ b/lib/Service/ObjectService.php @@ -302,7 +302,7 @@ public function saveObject(string $objectType, array $object, bool $updateVersio $mapper = $this->getMapper($objectType); // If the object has an id, update it; otherwise, create a new object if (isset($object['id']) === true) { - return $mapper->updateFromArray($object['id'], $object, $updateVersion); + return $mapper->updateFromArray($object['id'], $object, $updateVersion, patch: true); } else { return $mapper->createFromArray($object); From 6b29788acb2a0d72965d5a9dcb49b092adb91157 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 28 Nov 2024 14:45:57 +0100 Subject: [PATCH 3/5] remove var_dumps --- lib/Service/DirectoryService.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/Service/DirectoryService.php b/lib/Service/DirectoryService.php index fc27049b..4f5ae535 100644 --- a/lib/Service/DirectoryService.php +++ b/lib/Service/DirectoryService.php @@ -46,7 +46,7 @@ class DirectoryService * @param ObjectService $objectService Object service for handling objects * @param CatalogMapper $catalogMapper Mapper for catalog objects * @param ListingMapper $listingMapper Mapper for listing objects - * @param BroadcastService $broadcastService Broadcast service for broadcasting + * @param BroadcastService $broadcastService Broadcast service for broadcasting */ public function __construct( private readonly IURLGenerator $urlGenerator, @@ -395,8 +395,6 @@ public function syncExternalDirectory(string $url): array 'catalog' // Index by catalog ID ); - var_dump($oldListings, $currentListings); - // Initialize arrays to store results $addedListings = []; $updatedListings = []; @@ -415,8 +413,6 @@ public function syncExternalDirectory(string $url): array // Check if we already have this listing by looking up its catalog ID in the oldListings array $oldListing = $oldListings[$listing['id']] ?? null; - var_dump($listing['id'], $oldListing, $oldListings); - // If no existing listing found, prepare the new listing data if ($oldListing === null) { $listing['hash'] = hash('sha256', json_encode($listing)); From 11bfadd6b9213951557fd4256f03af170c69e5ea Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 28 Nov 2024 14:51:50 +0100 Subject: [PATCH 4/5] Styling and making mappers compatible with being called with $patch --- lib/Controller/DirectoryController.php | 2 +- lib/Controller/ListingsController.php | 2 +- lib/Db/AttachmentMapper.php | 2 +- lib/Db/CatalogMapper.php | 4 ++-- lib/Db/ListingMapper.php | 2 +- lib/Db/OrganizationMapper.php | 2 +- lib/Db/PublicationMapper.php | 2 +- lib/Db/PublicationTypeMapper.php | 2 +- lib/Db/ThemeMapper.php | 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/Controller/DirectoryController.php b/lib/Controller/DirectoryController.php index d10e970c..211bf86f 100644 --- a/lib/Controller/DirectoryController.php +++ b/lib/Controller/DirectoryController.php @@ -78,7 +78,7 @@ public function update(): JSONResponse $url = $this->request->getParam('directory'); // Sync the external directory with the provided URL - try{ + try { $data = $this->directoryService->syncExternalDirectory($url); } catch (DirectoryUrlException $exception) { if($exception->getMessage() === 'URL is required') { diff --git a/lib/Controller/ListingsController.php b/lib/Controller/ListingsController.php index 63b24bb5..450a6166 100644 --- a/lib/Controller/ListingsController.php +++ b/lib/Controller/ListingsController.php @@ -188,7 +188,7 @@ public function add(): JSONResponse $url = $this->request->getParam('url'); // Add the new listing using the provided URL - try{ + try { $result = $this->directoryService->syncExternalDirectory($url); } catch (DirectoryUrlException $exception) { if($exception->getMessage() === 'URL is required') { diff --git a/lib/Db/AttachmentMapper.php b/lib/Db/AttachmentMapper.php index f5eec661..0832eaaf 100644 --- a/lib/Db/AttachmentMapper.php +++ b/lib/Db/AttachmentMapper.php @@ -126,7 +126,7 @@ public function createFromArray(array $object): Attachment * @throws DoesNotExistException If the entity is not found * @throws MultipleObjectsReturnedException|\OCP\DB\Exception If multiple entities are found */ - public function updateFromArray(int $id, array $object, bool $updateVersion = true): Attachment + public function updateFromArray(int $id, array $object, bool $updateVersion = true, bool $patch = false): Attachment { $attachment = $this->find($id); // Fallback to create if the attachment does not exist diff --git a/lib/Db/CatalogMapper.php b/lib/Db/CatalogMapper.php index 825597de..6bb66f91 100644 --- a/lib/Db/CatalogMapper.php +++ b/lib/Db/CatalogMapper.php @@ -140,9 +140,9 @@ public function createFromArray(array $object): Catalog * * @return Catalog The updated Catalog entity */ - public function updateFromArray(int $id, array $object, bool $updateVersion = true): Catalog + public function updateFromArray(int $id, array $object, bool $updateVersion = true, bool $patch = false): Catalog { - $catalog = $this->find($id); + $catalog = $this->find($id); // Fallback to create if the catalog does not exist if ($catalog === null) { $object['uuid'] = $id; diff --git a/lib/Db/ListingMapper.php b/lib/Db/ListingMapper.php index 21a4300d..05ddab36 100644 --- a/lib/Db/ListingMapper.php +++ b/lib/Db/ListingMapper.php @@ -234,7 +234,7 @@ public function createFromArray(array $object): Listing * @return Listing The updated Listing entity * @throws Exception */ - public function updateFromArray(int|string $id, array $object, bool $updateVersion = true): Listing + public function updateFromArray(int|string $id, array $object, bool $updateVersion = true, bool $patch = false): Listing { $listing = $this->find($id); // Fallback to create if the listing does not exist diff --git a/lib/Db/OrganizationMapper.php b/lib/Db/OrganizationMapper.php index c83a859c..3c605d4c 100644 --- a/lib/Db/OrganizationMapper.php +++ b/lib/Db/OrganizationMapper.php @@ -135,7 +135,7 @@ public function createFromArray(array $object): Organization * * @return Organization The updated Organization entity */ - public function updateFromArray(int $id, array $object, bool $updateVersion = true): Organization + public function updateFromArray(int $id, array $object, bool $updateVersion = true, bool $patch = false): Organization { $organization = $this->find($id); // Fallback to create if the organization does not exist diff --git a/lib/Db/PublicationMapper.php b/lib/Db/PublicationMapper.php index c3ced4f0..56363f96 100644 --- a/lib/Db/PublicationMapper.php +++ b/lib/Db/PublicationMapper.php @@ -224,7 +224,7 @@ public function createFromArray(array $object): Publication * * @return Publication The updated Publication entity */ - public function updateFromArray(int $id, array $object, bool $updateVersion = true): Publication + public function updateFromArray(int $id, array $object, bool $updateVersion = true, bool $patch = false): Publication { $publication = $this->find(id: $id); // Fallback to create if the publication does not exist diff --git a/lib/Db/PublicationTypeMapper.php b/lib/Db/PublicationTypeMapper.php index 7a2cc336..1474130d 100644 --- a/lib/Db/PublicationTypeMapper.php +++ b/lib/Db/PublicationTypeMapper.php @@ -155,7 +155,7 @@ public function createFromArray(array $object): PublicationType * @throws DoesNotExistException If the entity is not found * @throws MultipleObjectsReturnedException|\OCP\DB\Exception If multiple entities are found */ - public function updateFromArray(int $id, array $object, bool $updateVersion = true): PublicationType + public function updateFromArray(int $id, array $object, bool $updateVersion = true, bool $patch = false): PublicationType { $publicationType = $this->find($id); // Fallback to create if the publication type does not exist diff --git a/lib/Db/ThemeMapper.php b/lib/Db/ThemeMapper.php index 272159ea..8b0b239b 100644 --- a/lib/Db/ThemeMapper.php +++ b/lib/Db/ThemeMapper.php @@ -141,7 +141,7 @@ public function createFromArray(array $object): Theme * @throws DoesNotExistException If the entity is not found * @throws MultipleObjectsReturnedException|\OCP\DB\Exception If multiple entities are found */ - public function updateFromArray(int $id, array $object, bool $updateVersion = true): Theme + public function updateFromArray(int $id, array $object, bool $updateVersion = true, bool $patch = false): Theme { $theme = $this->find($id); // Fallback to create if the theme does not exist From e16860f14f40145b879563e5adfcdaec85c8cc8b Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 28 Nov 2024 15:00:38 +0100 Subject: [PATCH 5/5] remove newline --- lib/Controller/DirectoryController.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Controller/DirectoryController.php b/lib/Controller/DirectoryController.php index 211bf86f..4b6218db 100644 --- a/lib/Controller/DirectoryController.php +++ b/lib/Controller/DirectoryController.php @@ -85,7 +85,6 @@ public function update(): JSONResponse $exception->setMessage('Property "directory" is required'); } - return new JSONResponse(data: ['message' => $exception->getMessage()], statusCode: 400); }