From d405106aaaf3ce5b9edd480c2636d5db3ffae61b Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 24 Jan 2024 15:16:19 +1300 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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 {