From 772617f23178ca943c80e769673bf649232345dc Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 15 Jan 2024 19:52:40 +1300 Subject: [PATCH 01/13] Mark invalid targets on message send then delete on maintenance sweep --- app/config/collections.php | 11 +++++++ app/controllers/api/messaging.php | 2 +- app/init.php | 3 +- src/Appwrite/Platform/Tasks/Maintenance.php | 15 +++++++--- src/Appwrite/Platform/Tasks/ScheduleBase.php | 1 + .../Platform/Tasks/ScheduleMessages.php | 5 +++- src/Appwrite/Platform/Workers/Deletes.php | 30 ++++++++++++++----- src/Appwrite/Platform/Workers/Messaging.php | 8 +++-- 8 files changed, 58 insertions(+), 17 deletions(-) diff --git a/app/config/collections.php b/app/config/collections.php index 979a267fa4..5369025e24 100644 --- a/app/config/collections.php +++ b/app/config/collections.php @@ -1961,6 +1961,17 @@ $commonCollections = [ 'array' => false, 'filters' => [], ], + [ + '$id' => ID::custom('expired'), + 'type' => Database::VAR_BOOLEAN, + 'format' => '', + 'size' => 0, + 'signed' => true, + 'required' => false, + 'default' => false, + 'array' => false, + 'filters' => [], + ], ], 'indexes' => [ [ diff --git a/app/controllers/api/messaging.php b/app/controllers/api/messaging.php index 2471ab8b26..356ec7bcbe 100644 --- a/app/controllers/api/messaging.php +++ b/app/controllers/api/messaging.php @@ -1944,7 +1944,7 @@ App::delete('/v1/messaging/topics/:topicId') $dbForProject->deleteDocument('topics', $topicId); $queueForDeletes - ->setType(DELETE_TYPE_TOPIC) + ->setType(DELETE_TYPE_TOPICS) ->setDocument($topic); $queueForEvents diff --git a/app/init.php b/app/init.php index e08cca1954..075b79f3d5 100644 --- a/app/init.php +++ b/app/init.php @@ -170,7 +170,8 @@ const DELETE_TYPE_SESSIONS = 'sessions'; const DELETE_TYPE_CACHE_BY_TIMESTAMP = 'cacheByTimeStamp'; const DELETE_TYPE_CACHE_BY_RESOURCE = 'cacheByResource'; const DELETE_TYPE_SCHEDULES = 'schedules'; -const DELETE_TYPE_TOPIC = 'topic'; +const DELETE_TYPE_TARGETS = 'targets'; +const DELETE_TYPE_TOPICS = 'topics'; // Mail Types const MAIL_TYPE_VERIFICATION = 'verification'; const MAIL_TYPE_MAGIC_SESSION = 'magicSession'; diff --git a/src/Appwrite/Platform/Tasks/Maintenance.php b/src/Appwrite/Platform/Tasks/Maintenance.php index 58e3228d27..914e6b5651 100644 --- a/src/Appwrite/Platform/Tasks/Maintenance.php +++ b/src/Appwrite/Platform/Tasks/Maintenance.php @@ -56,6 +56,7 @@ class Maintenance extends Action $this->renewCertificates($dbForConsole, $queueForCertificates); $this->notifyDeleteCache($cacheRetention, $queueForDeletes); $this->notifyDeleteSchedules($schedulesDeletionRetention, $queueForDeletes); + $this->notifyDeleteTargets($schedulesDeletionRetention, $queueForDeletes); }, $interval); } @@ -134,8 +135,7 @@ class Maintenance extends Action private function notifyDeleteCache($interval, Delete $queueForDeletes): void { - - ($queueForDeletes) + $queueForDeletes ->setType(DELETE_TYPE_CACHE_BY_TIMESTAMP) ->setDatetime(DateTime::addSeconds(new \DateTime(), -1 * $interval)) ->trigger(); @@ -143,10 +143,17 @@ class Maintenance extends Action private function notifyDeleteSchedules($interval, Delete $queueForDeletes): void { - - ($queueForDeletes) + $queueForDeletes ->setType(DELETE_TYPE_SCHEDULES) ->setDatetime(DateTime::addSeconds(new \DateTime(), -1 * $interval)) ->trigger(); } + + private function notifyDeleteTargets($interval, Delete $queueForDeletes): void + { + $queueForDeletes + ->setType(DELETE_TYPE_TARGETS) + ->setDatetime(DateTime::addSeconds(new \DateTime(), -1 * $interval)) + ->trigger(); + } } diff --git a/src/Appwrite/Platform/Tasks/ScheduleBase.php b/src/Appwrite/Platform/Tasks/ScheduleBase.php index 1ec8e471bb..f0aa559a7f 100644 --- a/src/Appwrite/Platform/Tasks/ScheduleBase.php +++ b/src/Appwrite/Platform/Tasks/ScheduleBase.php @@ -73,6 +73,7 @@ abstract class ScheduleBase extends Action '$id' => $schedule->getId(), 'resourceId' => $schedule->getAttribute('resourceId'), 'schedule' => $schedule->getAttribute('schedule'), + 'active' => $schedule->getAttribute('active'), 'resourceUpdatedAt' => $schedule->getAttribute('resourceUpdatedAt'), 'project' => $project, // TODO: @Meldiron Send only ID to worker to reduce memory usage here 'resource' => $resource, // TODO: @Meldiron Send only ID to worker to reduce memory usage here diff --git a/src/Appwrite/Platform/Tasks/ScheduleMessages.php b/src/Appwrite/Platform/Tasks/ScheduleMessages.php index 471aeb8f2a..1e71d1423e 100644 --- a/src/Appwrite/Platform/Tasks/ScheduleMessages.php +++ b/src/Appwrite/Platform/Tasks/ScheduleMessages.php @@ -33,6 +33,10 @@ class ScheduleMessages extends ScheduleBase protected function enqueueResources(Group $pools, Database $dbForConsole): void { foreach ($this->schedules as $schedule) { + if (!$schedule['active']) { + continue; + } + $now = DateTime::now(); $scheduledAt = DateTime::formatTz($schedule['scheduledAt']); @@ -59,7 +63,6 @@ class ScheduleMessages extends ScheduleBase $queueForDeletes ->setType(DELETE_TYPE_SCHEDULES) - ->setDocument($schedule) ->trigger(); $queue->reclaim(); diff --git a/src/Appwrite/Platform/Workers/Deletes.php b/src/Appwrite/Platform/Workers/Deletes.php index 6ed0c5d496..bfd4e1d223 100644 --- a/src/Appwrite/Platform/Workers/Deletes.php +++ b/src/Appwrite/Platform/Workers/Deletes.php @@ -151,7 +151,10 @@ class Deletes extends Action case DELETE_TYPE_SCHEDULES: $this->deleteSchedules($dbForConsole, $getProjectDB, $datetime, $document); break; - case DELETE_TYPE_TOPIC: + case DELETE_TYPE_TARGETS: + $this->deleteTargets($project, $getProjectDB, $document); + break; + case DELETE_TYPE_TOPICS: $this->deleteTopic($project, $getProjectDB, $document); break; default: @@ -178,7 +181,6 @@ class Deletes extends Action 'schedules', [ Query::equal('region', [App::getEnv('_APP_REGION', 'default')]), - Query::equal('resourceType', [$document->getAttribute('resourceType')]), Query::lessThanEqual('resourceUpdatedAt', $datetime), Query::equal('active', [false]), ], @@ -213,23 +215,37 @@ class Deletes extends Action ); } + private function deleteTargets(Document $project, callable $getProjectDB, Document $target) + { + $this->deleteByGroup( + 'targets', + [ + Query::equal('expired', [true]) + ], + $getProjectDB($project) + ); + } + /** * @param Document $project * @param callable $getProjectDB * @param Document $topic * @throws Exception */ - protected function deleteTopic(Document $project, callable $getProjectDB, Document $topic) + private function deleteTopic(Document $project, callable $getProjectDB, Document $topic) { if ($topic->isEmpty()) { Console::error('Failed to delete subscribers. Topic not found'); return; } - $dbForProject = $getProjectDB($project); - $this->deleteByGroup('subscribers', [ - Query::equal('topicInternalId', [$topic->getInternalId()]) - ], $dbForProject); + $this->deleteByGroup( + 'subscribers', + [ + Query::equal('topicInternalId', [$topic->getInternalId()]) + ], + $getProjectDB($project) + ); } /** diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index 9b7bda57bb..6535dfce3d 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -232,9 +232,11 @@ class Messaging extends Action Query::equal('identifier', [$detail['recipient']]) ]); - if ($target instanceof Document && !$target->isEmpty()) { - $dbForProject->deleteDocument('targets', $target->getId()); - } + $dbForProject->updateDocument( + 'targets', + $target->getId(), + $target->setAttribute('expired', true) + ); } } } catch (\Exception $e) { From 9fc68d23a00f94c63376a8272991ec4b96416c86 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 19 Jan 2024 17:23:44 +1300 Subject: [PATCH 02/13] Fix delete type for expired targets --- app/init.php | 1 + src/Appwrite/Platform/Tasks/Maintenance.php | 7 ++- src/Appwrite/Platform/Workers/Deletes.php | 47 ++++++++++++--------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/app/init.php b/app/init.php index 2422a54ab3..5eba7ad993 100644 --- a/app/init.php +++ b/app/init.php @@ -172,6 +172,7 @@ const DELETE_TYPE_CACHE_BY_RESOURCE = 'cacheByResource'; const DELETE_TYPE_SCHEDULES = 'schedules'; const DELETE_TYPE_TOPIC = 'topic'; const DELETE_TYPE_TARGET = 'target'; +const DELETE_TYPE_EXPIRED_TARGETS = 'invalid_targets'; // Mail Types const MAIL_TYPE_VERIFICATION = 'verification'; const MAIL_TYPE_MAGIC_SESSION = 'magicSession'; diff --git a/src/Appwrite/Platform/Tasks/Maintenance.php b/src/Appwrite/Platform/Tasks/Maintenance.php index 914e6b5651..66ce40a0d2 100644 --- a/src/Appwrite/Platform/Tasks/Maintenance.php +++ b/src/Appwrite/Platform/Tasks/Maintenance.php @@ -56,7 +56,7 @@ class Maintenance extends Action $this->renewCertificates($dbForConsole, $queueForCertificates); $this->notifyDeleteCache($cacheRetention, $queueForDeletes); $this->notifyDeleteSchedules($schedulesDeletionRetention, $queueForDeletes); - $this->notifyDeleteTargets($schedulesDeletionRetention, $queueForDeletes); + $this->notifyDeleteTargets($queueForDeletes); }, $interval); } @@ -149,11 +149,10 @@ class Maintenance extends Action ->trigger(); } - private function notifyDeleteTargets($interval, Delete $queueForDeletes): void + private function notifyDeleteTargets(Delete $queueForDeletes): void { $queueForDeletes - ->setType(DELETE_TYPE_TARGETS) - ->setDatetime(DateTime::addSeconds(new \DateTime(), -1 * $interval)) + ->setType(DELETE_TYPE_EXPIRED_TARGETS) ->trigger(); } } diff --git a/src/Appwrite/Platform/Workers/Deletes.php b/src/Appwrite/Platform/Workers/Deletes.php index 11a7a69967..a98566f0f8 100644 --- a/src/Appwrite/Platform/Workers/Deletes.php +++ b/src/Appwrite/Platform/Workers/Deletes.php @@ -119,11 +119,9 @@ class Deletes extends Action break; } break; - case DELETE_TYPE_EXECUTIONS: $this->deleteExecutionLogs($dbForConsole, $getProjectDB, $datetime); break; - case DELETE_TYPE_AUDIT: if (!empty($datetime)) { $this->deleteAuditLogs($dbForConsole, $getProjectDB, $datetime); @@ -136,11 +134,9 @@ class Deletes extends Action case DELETE_TYPE_ABUSE: $this->deleteAbuseLogs($dbForConsole, $getProjectDB, $datetime); break; - case DELETE_TYPE_REALTIME: $this->deleteRealtimeUsage($dbForConsole, $datetime); break; - case DELETE_TYPE_SESSIONS: $this->deleteExpiredSessions($dbForConsole, $getProjectDB); break; @@ -156,18 +152,17 @@ class Deletes extends Action case DELETE_TYPE_SCHEDULES: $this->deleteSchedules($dbForConsole, $getProjectDB, $datetime, $document); break; - case DELETE_TYPE_TARGETS: - $this->deleteTargets($project, $getProjectDB, $document); - break; - case DELETE_TYPE_TOPICS: + case DELETE_TYPE_TOPIC: $this->deleteTopic($project, $getProjectDB, $document); break; case DELETE_TYPE_TARGET: $this->deleteTarget($project, $getProjectDB, $document); break; + case DELETE_TYPE_EXPIRED_TARGETS: + $this->deleteExpiredTargets($project, $getProjectDB); + break; default: throw new \Exception('No delete operation for type: ' . \strval($type)); - break; } } @@ -223,17 +218,6 @@ class Deletes extends Action ); } - private function deleteTargets(Document $project, callable $getProjectDB, Document $target) - { - $this->deleteByGroup( - 'targets', - [ - Query::equal('expired', [true]) - ], - $getProjectDB($project) - ); - } - /** * @param Document $project * @param callable $getProjectDB @@ -262,7 +246,7 @@ class Deletes extends Action * @param Document $target * @throws Exception */ - protected function deleteTarget(Document $project, callable $getProjectDB, Document $target) + private function deleteTarget(Document $project, callable $getProjectDB, Document $target) { /** @var Database */ $dbForProject = $getProjectDB($project); @@ -285,6 +269,27 @@ class Deletes extends Action ); } + /** + * @param Document $project + * @param callable $getProjectDB + * @param Document $target + * @return void + * @throws Exception + */ + private function deleteExpiredTargets(Document $project, callable $getProjectDB) + { + $this->deleteByGroup( + 'targets', + [ + Query::equal('expired', [true]) + ], + $getProjectDB($project), + function (Document $target) use ($getProjectDB, $project) { + $this->deleteTarget($project, $getProjectDB, $target); + } + ); + } + /** * @param Document $project * @param callable $getProjectDB From a970018b045b598e4ed82e8c3c591b8e09aaad80 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 19 Jan 2024 19:18:56 +1300 Subject: [PATCH 03/13] Fix tests --- app/controllers/api/messaging.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/messaging.php b/app/controllers/api/messaging.php index 51e046a6ec..ef72be19cf 100644 --- a/app/controllers/api/messaging.php +++ b/app/controllers/api/messaging.php @@ -1931,7 +1931,7 @@ App::delete('/v1/messaging/topics/:topicId') $dbForProject->deleteDocument('topics', $topicId); $queueForDeletes - ->setType(DELETE_TYPE_TOPICS) + ->setType(DELETE_TYPE_TOPIC) ->setDocument($topic); $queueForEvents From 8fd5a336af2ec5f0da869b324e25ca03ad171191 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 19 Jan 2024 20:18:10 +1300 Subject: [PATCH 04/13] Make sure target exists before updating --- src/Appwrite/Platform/Workers/Messaging.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index e95d7835fd..a5c8154b91 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -247,11 +247,13 @@ class Messaging extends Action Query::equal('identifier', [$detail['recipient']]) ]); - $dbForProject->updateDocument( - 'targets', - $target->getId(), - $target->setAttribute('expired', true) - ); + if ($target instanceof Document && !$target->isEmpty()) { + $dbForProject->updateDocument( + 'targets', + $target->getId(), + $target->setAttribute('expired', true) + ); + } } } } catch (\Exception $e) { From 52a9b31521a95db68ab418947fde6c6763719939 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 25 Jan 2024 20:01:15 +1300 Subject: [PATCH 05/13] Trigger deletes worker when target is deleted --- app/controllers/api/users.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index 117338b9a7..ddc925d1c2 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -1689,10 +1689,10 @@ App::delete('/v1/users/:userId/targets/:targetId') ->param('userId', '', new UID(), 'User ID.') ->param('targetId', '', new UID(), 'Target ID.') ->inject('queueForEvents') + ->inject('queueForDeletes') ->inject('response') ->inject('dbForProject') - ->action(function (string $userId, string $targetId, Event $queueForEvents, Response $response, Database $dbForProject) { - + ->action(function (string $userId, string $targetId, Event $queueForEvents, Delete $queueForDeletes, Response $response, Database $dbForProject) { $user = $dbForProject->getDocument('users', $userId); if ($user->isEmpty()) { @@ -1712,6 +1712,10 @@ App::delete('/v1/users/:userId/targets/:targetId') $dbForProject->deleteDocument('targets', $target->getId()); $dbForProject->deleteCachedDocument('users', $user->getId()); + $queueForDeletes + ->setType(DELETE_TYPE_TARGET) + ->setDocument($target); + $queueForEvents ->setParam('userId', $user->getId()) ->setParam('targetId', $target->getId()); From e81ab1d1e0fbcfc013b2c218ceab0b0ff2a3d107 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 26 Jan 2024 00:35:04 +1300 Subject: [PATCH 06/13] Remove redundant document pass through --- src/Appwrite/Platform/Workers/Deletes.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Deletes.php b/src/Appwrite/Platform/Workers/Deletes.php index cd04eb52b2..2de55b5cc0 100644 --- a/src/Appwrite/Platform/Workers/Deletes.php +++ b/src/Appwrite/Platform/Workers/Deletes.php @@ -153,7 +153,7 @@ class Deletes extends Action $this->deleteCacheByDate($project, $getProjectDB, $datetime); break; case DELETE_TYPE_SCHEDULES: - $this->deleteSchedules($dbForConsole, $getProjectDB, $datetime, $document); + $this->deleteSchedules($dbForConsole, $getProjectDB, $datetime); break; case DELETE_TYPE_TOPIC: $this->deleteTopic($project, $getProjectDB, $document); @@ -181,7 +181,7 @@ class Deletes extends Action * @throws Structure * @throws DatabaseException */ - private function deleteSchedules(Database $dbForConsole, callable $getProjectDB, string $datetime, ?Document $document = null): void + private function deleteSchedules(Database $dbForConsole, callable $getProjectDB, string $datetime): void { $this->listByGroup( 'schedules', From 34973b8413ad0ee524afcb8151213357a4629fb8 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 26 Jan 2024 00:35:37 +1300 Subject: [PATCH 07/13] Directly delete message schedule instead of deferring to worker --- src/Appwrite/Platform/Tasks/ScheduleMessages.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Appwrite/Platform/Tasks/ScheduleMessages.php b/src/Appwrite/Platform/Tasks/ScheduleMessages.php index fef0c79bb2..cc641b434a 100644 --- a/src/Appwrite/Platform/Tasks/ScheduleMessages.php +++ b/src/Appwrite/Platform/Tasks/ScheduleMessages.php @@ -48,23 +48,17 @@ class ScheduleMessages extends ScheduleBase $queue = $pools->get('queue')->pop(); $connection = $queue->getResource(); $queueForMessaging = new Messaging($connection); - $queueForDeletes = new Delete($connection); $queueForMessaging ->setMessageId($schedule['resourceId']) ->setProject($schedule['project']) ->trigger(); - $dbForConsole->updateDocument( + $dbForConsole->deleteDocument( 'schedules', $schedule['$id'], - new Document(['active' => false]) ); - $queueForDeletes - ->setType(DELETE_TYPE_SCHEDULES) - ->trigger(); - $queue->reclaim(); unset($this->schedules[$schedule['resourceId']]); From 89343704f75e1e7aeac10d5c68d77e0493802d94 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 26 Jan 2024 00:41:05 +1300 Subject: [PATCH 08/13] Rename deleteTargetSubscribers method to make actions clearer --- src/Appwrite/Platform/Workers/Deletes.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Deletes.php b/src/Appwrite/Platform/Workers/Deletes.php index 2de55b5cc0..cd62d2e621 100644 --- a/src/Appwrite/Platform/Workers/Deletes.php +++ b/src/Appwrite/Platform/Workers/Deletes.php @@ -159,7 +159,7 @@ class Deletes extends Action $this->deleteTopic($project, $getProjectDB, $document); break; case DELETE_TYPE_TARGET: - $this->deleteTarget($project, $getProjectDB, $document); + $this->deleteTargetSubscribers($project, $getProjectDB, $document); break; case DELETE_TYPE_EXPIRED_TARGETS: $this->deleteExpiredTargets($project, $getProjectDB); @@ -249,7 +249,7 @@ class Deletes extends Action * @param Document $target * @throws Exception */ - private function deleteTarget(Document $project, callable $getProjectDB, Document $target) + private function deleteTargetSubscribers(Document $project, callable $getProjectDB, Document $target) { /** @var Database */ $dbForProject = $getProjectDB($project); @@ -288,7 +288,7 @@ class Deletes extends Action ], $getProjectDB($project), function (Document $target) use ($getProjectDB, $project) { - $this->deleteTarget($project, $getProjectDB, $target); + $this->deleteTargetSubscribers($project, $getProjectDB, $target); } ); } @@ -634,14 +634,14 @@ class Deletes extends Action ], $dbForProject); // Delete targets - $this->listByGroup( + $this->deleteByGroup( 'targets', [ Query::equal('userInternalId', [$userInternalId]) ], $dbForProject, function (Document $target) use ($getProjectDB, $project) { - $this->deleteTarget($project, $getProjectDB, $target); + $this->deleteTargetSubscribers($project, $getProjectDB, $target); } ); } From d405106aaaf3ce5b9edd480c2636d5db3ffae61b Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 24 Jan 2024 15:16:19 +1300 Subject: [PATCH 09/13] Throw on enable if provider credentials are missing instead of ignoring --- app/config/errors.php | 5 + app/controllers/api/messaging.php | 208 ++++++++++++++++-------------- src/Appwrite/Extend/Exception.php | 2 +- 3 files changed, 118 insertions(+), 97 deletions(-) diff --git a/app/config/errors.php b/app/config/errors.php index e5c2d4e5bd..4cc8e734ee 100644 --- a/app/config/errors.php +++ b/app/config/errors.php @@ -811,6 +811,11 @@ return [ 'description' => 'Provider with the requested ID is of the incorrect type.', 'code' => 400, ], + Exception::PROVIDER_MISSING_CREDENTIALS => [ + 'name' => Exception::PROVIDER_MISSING_CREDENTIALS, + 'description' => 'Provider with the requested ID is missing credentials.', + 'code' => 400, + ], /** Topics */ Exception::TOPIC_NOT_FOUND => [ diff --git a/app/controllers/api/messaging.php b/app/controllers/api/messaging.php index ef72be19cf..169b51c481 100644 --- a/app/controllers/api/messaging.php +++ b/app/controllers/api/messaging.php @@ -917,9 +917,10 @@ App::patch('/v1/messaging/providers/mailgun/:providerId') if ($provider->isEmpty()) { throw new Exception(Exception::PROVIDER_NOT_FOUND); } - $providerAttr = $provider->getAttribute('provider'); - if ($providerAttr !== 'mailgun') { + $providerProvider = $provider->getAttribute('provider'); + + if ($providerProvider !== 'mailgun') { throw new Exception(Exception::PROVIDER_INCORRECT_TYPE); } @@ -949,7 +950,7 @@ App::patch('/v1/messaging/providers/mailgun/:providerId') $credentials = $provider->getAttribute('credentials'); - if ($isEuRegion === true || $isEuRegion === false) { + if (!\is_null($isEuRegion)) { $credentials['isEuRegion'] = $isEuRegion; } @@ -963,19 +964,21 @@ App::patch('/v1/messaging/providers/mailgun/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true && - \array_key_exists('isEuRegion', $credentials) && - \array_key_exists('apiKey', $credentials) && - \array_key_exists('domain', $credentials) && - \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('isEuRegion', $credentials) && + \array_key_exists('apiKey', $credentials) && + \array_key_exists('domain', $credentials) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1054,17 +1057,19 @@ App::patch('/v1/messaging/providers/sendgrid/:providerId') ]); } - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('apiKey', $provider->getAttribute('credentials')) - && \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('apiKey', $provider->getAttribute('credentials')) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1133,18 +1138,20 @@ App::patch('/v1/messaging/providers/msg91/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('senderId', $credentials) - && \array_key_exists('authKey', $credentials) - && \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('senderId', $credentials) && + \array_key_exists('authKey', $credentials) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1213,19 +1220,20 @@ App::patch('/v1/messaging/providers/telesign/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('username', $credentials) - && \array_key_exists('password', $credentials) - && \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('username', $credentials) && + \array_key_exists('password', $credentials) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1294,19 +1302,20 @@ App::patch('/v1/messaging/providers/textmagic/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('username', $credentials) - && \array_key_exists('apiKey', $credentials) - && \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('username', $credentials) && + \array_key_exists('apiKey', $credentials) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1375,19 +1384,20 @@ App::patch('/v1/messaging/providers/twilio/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('accountSid', $credentials) - && \array_key_exists('authToken', $credentials) - && \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('accountSid', $credentials) && + \array_key_exists('authToken', $credentials) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1456,19 +1466,20 @@ App::patch('/v1/messaging/providers/vonage/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('apiKey', $credentials) - && \array_key_exists('apiSecret', $credentials) - && \array_key_exists('from', $provider->getAttribute('options')) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('apiKey', $credentials) && + \array_key_exists('apiSecret', $credentials) && + \array_key_exists('from', $provider->getAttribute('options')) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1518,17 +1529,21 @@ App::patch('/v1/messaging/providers/fcm/:providerId') } if (!\is_null($serviceAccountJSON)) { - $provider->setAttribute('credentials', ['serviceAccountJSON' => $serviceAccountJSON]); + $provider->setAttribute('credentials', [ + 'serviceAccountJSON' => $serviceAccountJSON + ]); } - if ($enabled === true || $enabled === false) { - if ($enabled === true && \array_key_exists('serviceAccountJSON', $provider->getAttribute('credentials'))) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if (\array_key_exists('serviceAccountJSON', $provider->getAttribute('credentials'))) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); @@ -1601,20 +1616,21 @@ App::patch('/v1/messaging/providers/apns/:providerId') $provider->setAttribute('credentials', $credentials); - if ($enabled === true || $enabled === false) { - if ( - $enabled === true - && \array_key_exists('authKey', $credentials) - && \array_key_exists('authKeyId', $credentials) - && \array_key_exists('teamId', $credentials) - && \array_key_exists('bundleId', $credentials) - ) { - $enabled = true; + if (!\is_null($enabled)) { + if ($enabled) { + if ( + \array_key_exists('authKey', $credentials) && + \array_key_exists('authKeyId', $credentials) && + \array_key_exists('teamId', $credentials) && + \array_key_exists('bundleId', $credentials) + ) { + $provider->setAttribute('enabled', true); + } else { + throw new Exception(Exception::PROVIDER_MISSING_CREDENTIALS); + } } else { - $enabled = false; + $provider->setAttribute('enabled', false); } - - $provider->setAttribute('enabled', $enabled); } $provider = $dbForProject->updateDocument('providers', $provider->getId(), $provider); diff --git a/src/Appwrite/Extend/Exception.php b/src/Appwrite/Extend/Exception.php index 183f4a812a..d81589ed9d 100644 --- a/src/Appwrite/Extend/Exception.php +++ b/src/Appwrite/Extend/Exception.php @@ -244,7 +244,7 @@ class Exception extends \Exception public const PROVIDER_NOT_FOUND = 'provider_not_found'; public const PROVIDER_ALREADY_EXISTS = 'provider_already_exists'; public const PROVIDER_INCORRECT_TYPE = 'provider_incorrect_type'; - public const PROVIDER_INTERNAL_UPDATE_DISABLED = 'provider_internal_update_disabled'; + public const PROVIDER_MISSING_CREDENTIALS = 'provider_missing_credentials'; /** Topic */ public const TOPIC_NOT_FOUND = 'topic_not_found'; From 874e3dd8d8cbe9707f3a5648ded85498072b1227 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 24 Jan 2024 15:22:10 +1300 Subject: [PATCH 10/13] Add test ensuring 400 if trying to enable with missing credentials --- .../e2e/Services/Messaging/MessagingBase.php | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/e2e/Services/Messaging/MessagingBase.php b/tests/e2e/Services/Messaging/MessagingBase.php index 1a16674658..9a9d29d84b 100644 --- a/tests/e2e/Services/Messaging/MessagingBase.php +++ b/tests/e2e/Services/Messaging/MessagingBase.php @@ -185,6 +185,30 @@ trait MessagingBase return $providers; } + public function testUpdateProviderMissingCredentialsThrows(): void + { + // Create new FCM provider with no serviceAccountJSON + $response = $this->client->call(Client::METHOD_POST, '/messaging/providers/fcm', \array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'providerId' => ID::unique(), + 'name' => 'FCM3', + ]); + + $this->assertEquals(201, $response['headers']['status-code']); + + // Enable provider with no serviceAccountJSON + $response = $this->client->call(Client::METHOD_PATCH, '/messaging/providers/fcm/' . $response['body']['$id'], \array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'enabled' => true, + ]); + + $this->assertEquals(400, $response['headers']['status-code']); + } + /** * @depends testUpdateProviders */ From 7a5c226110743330ec7c9a1dfc5f3904abd7af8a Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 24 Jan 2024 17:34:59 +1300 Subject: [PATCH 11/13] Fix tests --- app/controllers/api/messaging.php | 12 ++++++++++-- tests/e2e/Services/Messaging/MessagingBase.php | 12 +++++++----- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/messaging.php b/app/controllers/api/messaging.php index 169b51c481..869b751a1d 100644 --- a/app/controllers/api/messaging.php +++ b/app/controllers/api/messaging.php @@ -616,9 +616,13 @@ App::post('/v1/messaging/providers/fcm') ->inject('queueForEvents') ->inject('dbForProject') ->inject('response') - ->action(function (string $providerId, string $name, ?array $serviceAccountJSON, ?bool $enabled, Event $queueForEvents, Database $dbForProject, Response $response) { + ->action(function (string $providerId, string $name, array|string|null $serviceAccountJSON, ?bool $enabled, Event $queueForEvents, Database $dbForProject, Response $response) { $providerId = $providerId == 'unique()' ? ID::unique() : $providerId; + $serviceAccountJSON = \is_string($serviceAccountJSON) + ? \json_decode($serviceAccountJSON, true) + : $serviceAccountJSON; + $credentials = []; if (!\is_null($serviceAccountJSON)) { @@ -1512,7 +1516,7 @@ App::patch('/v1/messaging/providers/fcm/:providerId') ->inject('queueForEvents') ->inject('dbForProject') ->inject('response') - ->action(function (string $providerId, string $name, ?bool $enabled, ?array $serviceAccountJSON, Event $queueForEvents, Database $dbForProject, Response $response) { + ->action(function (string $providerId, string $name, ?bool $enabled, array|string|null $serviceAccountJSON, Event $queueForEvents, Database $dbForProject, Response $response) { $provider = $dbForProject->getDocument('providers', $providerId); if ($provider->isEmpty()) { @@ -1529,6 +1533,10 @@ App::patch('/v1/messaging/providers/fcm/:providerId') } if (!\is_null($serviceAccountJSON)) { + $serviceAccountJSON = \is_string($serviceAccountJSON) + ? \json_decode($serviceAccountJSON, true) + : $serviceAccountJSON; + $provider->setAttribute('credentials', [ 'serviceAccountJSON' => $serviceAccountJSON ]); diff --git a/tests/e2e/Services/Messaging/MessagingBase.php b/tests/e2e/Services/Messaging/MessagingBase.php index 9a9d29d84b..0e7fca8507 100644 --- a/tests/e2e/Services/Messaging/MessagingBase.php +++ b/tests/e2e/Services/Messaging/MessagingBase.php @@ -188,10 +188,11 @@ trait MessagingBase public function testUpdateProviderMissingCredentialsThrows(): void { // Create new FCM provider with no serviceAccountJSON - $response = $this->client->call(Client::METHOD_POST, '/messaging/providers/fcm', \array_merge([ + $response = $this->client->call(Client::METHOD_POST, '/messaging/providers/fcm', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ + 'x-appwrite-key' => $this->getProject()['apiKey'], + ], [ 'providerId' => ID::unique(), 'name' => 'FCM3', ]); @@ -199,10 +200,11 @@ trait MessagingBase $this->assertEquals(201, $response['headers']['status-code']); // Enable provider with no serviceAccountJSON - $response = $this->client->call(Client::METHOD_PATCH, '/messaging/providers/fcm/' . $response['body']['$id'], \array_merge([ + $response = $this->client->call(Client::METHOD_PATCH, '/messaging/providers/fcm/' . $response['body']['$id'], [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ + 'x-appwrite-key' => $this->getProject()['apiKey'], + ], [ 'enabled' => true, ]); @@ -221,7 +223,7 @@ trait MessagingBase ]); $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals(\count($providers), \count($response['body']['providers'])); + $this->assertEquals(10, \count($response['body']['providers'])); return $providers; } From b3a4722758ad1daef139f292354692892a2b173c Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 25 Jan 2024 20:05:02 +1300 Subject: [PATCH 12/13] Fix validators --- app/controllers/api/messaging.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/messaging.php b/app/controllers/api/messaging.php index 869b751a1d..db9e9e4a09 100644 --- a/app/controllers/api/messaging.php +++ b/app/controllers/api/messaging.php @@ -62,10 +62,10 @@ App::post('/v1/messaging/providers/mailgun') ->param('domain', '', new Text(0), 'Mailgun Domain.', true) ->param('isEuRegion', null, new Boolean(), 'Set as EU region.', true) ->param('enabled', null, new Boolean(), 'Set as enabled.', true) - ->param('fromName', '', new Text(128), 'Sender Name.', true) + ->param('fromName', '', new Text(128, 0), 'Sender Name.', true) ->param('fromEmail', '', new Email(), 'Sender email address.', true) - ->param('replyToName', '', new Text(128), 'Name set in the reply to field for the mail. Default value is sender name. Reply to name must have reply to email as well.', true) - ->param('replyToEmail', '', new Text(128), 'Email set in the reply to field for the mail. Default value is sender email. Reply to email must have reply to name as well.', true) + ->param('replyToName', '', new Text(128, 0), 'Name set in the reply to field for the mail. Default value is sender name. Reply to name must have reply to email as well.', true) + ->param('replyToEmail', '', new Email(), 'Email set in the reply to field for the mail. Default value is sender email. Reply to email must have reply to name as well.', true) ->inject('queueForEvents') ->inject('dbForProject') ->inject('response') @@ -150,10 +150,10 @@ App::post('/v1/messaging/providers/sendgrid') ->param('name', '', new Text(128), 'Provider name.') ->param('apiKey', '', new Text(0), 'Sendgrid API key.', true) ->param('enabled', null, new Boolean(), 'Set as enabled.', true) - ->param('fromName', '', new Text(128), 'Sender Name.', true) + ->param('fromName', '', new Text(128, 0), 'Sender Name.', true) ->param('fromEmail', '', new Email(), 'Sender email address.', true) - ->param('replyToName', '', new Text(128), 'Name set in the reply to field for the mail. Default value is sender name.', true) - ->param('replyToEmail', '', new Text(128), 'Email set in the reply to field for the mail. Default value is sender email.', true) + ->param('replyToName', '', new Text(128, 0), 'Name set in the reply to field for the mail. Default value is sender name.', true) + ->param('replyToEmail', '', new Email(), 'Email set in the reply to field for the mail. Default value is sender email.', true) ->inject('queueForEvents') ->inject('dbForProject') ->inject('response') From fc41b0894044f8a218632b64f93495295d3bdf47 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 25 Jan 2024 20:05:36 +1300 Subject: [PATCH 13/13] Fix email credential checks --- app/controllers/api/messaging.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/messaging.php b/app/controllers/api/messaging.php index db9e9e4a09..a6c2f923eb 100644 --- a/app/controllers/api/messaging.php +++ b/app/controllers/api/messaging.php @@ -101,7 +101,7 @@ App::post('/v1/messaging/providers/mailgun') \array_key_exists('isEuRegion', $credentials) && \array_key_exists('apiKey', $credentials) && \array_key_exists('domain', $credentials) && - \array_key_exists('from', $options) + \array_key_exists('fromEmail', $options) ) { $enabled = true; } else { @@ -179,7 +179,7 @@ App::post('/v1/messaging/providers/sendgrid') if ( $enabled === true && \array_key_exists('apiKey', $credentials) - && \array_key_exists('from', $options) + && \array_key_exists('fromEmail', $options) ) { $enabled = true; } else { @@ -974,7 +974,7 @@ App::patch('/v1/messaging/providers/mailgun/:providerId') \array_key_exists('isEuRegion', $credentials) && \array_key_exists('apiKey', $credentials) && \array_key_exists('domain', $credentials) && - \array_key_exists('from', $provider->getAttribute('options')) + \array_key_exists('fromEmail', $options) ) { $provider->setAttribute('enabled', true); } else { @@ -1065,7 +1065,7 @@ App::patch('/v1/messaging/providers/sendgrid/:providerId') if ($enabled) { if ( \array_key_exists('apiKey', $provider->getAttribute('credentials')) && - \array_key_exists('from', $provider->getAttribute('options')) + \array_key_exists('fromEmail', $provider->getAttribute('options')) ) { $provider->setAttribute('enabled', true); } else {