Updates from code review

CakeDC · Feb 25, 2022 · ef3a33e · ef3a33e
1 parent 8bb108a
commit ef3a33e
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 21 deletions.
diff --git a/config/users.php b/config/users.php
@@ -145,7 +145,7 @@
         'checker' => \CakeDC\Auth\Authentication\DefaultCode2fAuthenticationChecker::class,
         'type' => \CakeDC\Auth\Authentication\Code2fAuthenticationCheckerInterface::CODE2F_TYPE_EMAIL,
         'config' => 'default',
-        'message' => __d('cake_d_c/users', '{0} is your {1} verification code'),
+        'message' => '{0} is your {1} verification code', //{0} will be replaced by code, {1} by App.name
         'maxSeconds' => 300,
         'maxTries' => 3
     ],

diff --git a/src/Controller/Traits/Code2fTrait.php b/src/Controller/Traits/Code2fTrait.php
@@ -13,11 +13,15 @@
 namespace CakeDC\Users\Controller\Traits;
 
 use Cake\Core\Configure;
+use Cake\Datasource\Exception\RecordNotFoundException;
+use Cake\Mailer\Mailer;
+use Cake\Mailer\TransportFactory;
 use Cake\ORM\TableRegistry;
 use CakeDC\Auth\Authentication\AuthenticationService;
 use CakeDC\Auth\Authentication\Code2fAuthenticationCheckerFactory;
 use CakeDC\Auth\Authentication\Code2fAuthenticationCheckerInterface;
 use CakeDC\Auth\Authenticator\TwoFactorAuthenticator;
+use CakeDC\Users\Exception\TokenExpiredException;
 use CakeDC\Users\Model\Table\OtpCodesTable;
 
 /**
@@ -74,9 +78,21 @@ public function code2fRegister()
         if ($this->getRequest()->is(['post', 'put'])) {
 
             $value = $this->getRequest()->getData($field);
-            if ($data['field'] === Code2fAuthenticationCheckerInterface::CODE2F_TYPE_PHONE && !preg_match('/^\+[1-9]\d{1,14}$/i', $value)) {
-                $this->Flash->error(__d('cake_d_c/users', 'Invalid phone number: Format must be +1234567890'));
-            } else {
+
+            $validated = true;
+            if ($data['field'] === Code2fAuthenticationCheckerInterface::CODE2F_TYPE_PHONE) {
+                $config = Mailer::getConfig(Configure::read('Code2f.config', 'sms'));
+                $phonePattern = TransportFactory::get($config['transport'])->getConfig('phonePattern');
+                if (!$phonePattern) {
+                    throw new \UnexpectedValueException(__d('cake_d_c/users', 'You must define `phonePattern` in your transport ({0}) config.', $config));
+                }
+                if (!preg_match($phonePattern, $value)) {
+                    $this->Flash->error(__d('cake_d_c/users', 'Invalid phone number: Format must be {0}}', $phonePattern));
+                    $validated = false;
+                }
+            }
+
+            if ($validated) {
                 $data['user'][$field] = $value;
                 $user = $this->getUsersTable()->saveOrFail($data['user'], ['checkRules' => false]);
                 $this->getRequest()->getSession()->write(AuthenticationService::CODE2F_SESSION_KEY, $user);
@@ -119,13 +135,13 @@ public function code2fAuthenticate()
                     $this->request->getSession()->delete(AuthenticationService::CODE2F_SESSION_KEY);
                     $this->request->getSession()->write(TwoFactorAuthenticator::USER_SESSION_KEY, $data['user']);
                     return $this->redirectWithQuery(Configure::read('Auth.AuthenticationComponent.loginAction'));
-            } catch (\Exception $e) {
+            } catch (TokenExpiredException | \InvalidArgumentException $e) {
                 $this->Flash->error($e->getMessage());
             }
         } else {
             try {
                 $OtpCodes->sendCode2f($data['user']['id'], $resend);
-            } catch (\Exception $e) {
+            } catch (\UnexpectedValueException | RecordNotFoundException | \OverflowException $e) {
                 $this->Flash->error($e->getMessage());
             }
             if ($resend) {

diff --git a/src/Mailer/SMSMailer.php b/src/Mailer/SMSMailer.php
@@ -16,6 +16,7 @@
 use Cake\Datasource\EntityInterface;
 use Cake\Mailer\Mailer;
 use Cake\Mailer\Message;
+use Cake\Mailer\TransportFactory;
 use CakeDC\Users\Utility\UsersUrl;
 
 /**
@@ -26,15 +27,20 @@ class SMSMailer extends Mailer
     public function __construct($config = null)
     {
         parent::__construct();
-        $this->setEmailPattern('/^\+[1-9]\d{1,14}$/m');
+        $smsConfig = Mailer::getConfig(Configure::read('Code2f.config', 'sms'));
+        $phonePattern = TransportFactory::get($smsConfig['transport'])->getConfig('phonePattern');
+        if (!$phonePattern) {
+            throw new \UnexpectedValueException(__d('cake_d_c/users', 'You must define `phonePattern` in your transport ({0}) config.', $config));
+        }
+        $this->setEmailPattern($phonePattern);
         $this->setProfile($config);
         $this->setEmailFormat('text');
     }
 
     public function otp(EntityInterface $user, $code)
     {
         $this->setTo($user->phone);
-        $this->deliver(__(Configure::read('Code2f.message'), $code, Configure::read('App.name')));
+        $this->deliver(__d('cake_d_c/users', Configure::read('Code2f.message'), $code, Configure::read('App.name')));
     }
 
 }
diff --git a/src/Mailer/Transport/TwilioTransport.php b/src/Mailer/Transport/TwilioTransport.php
@@ -18,6 +18,10 @@
 
 class TwilioTransport extends AbstractTransport
 {
+    protected $_defaultConfig = [
+        'phonePattern' => '/^\+[1-9]\d{1,14}$/i'
+    ];
+
     public function send(Message $message): array
     {
         $sid = $this->getConfig('sid');
@@ -28,13 +32,12 @@ public function send(Message $message): array
         $recipients = collection($to);
 
         $recipients->each(function ($recipient) {
-            if (!preg_match('/^\+[1-9]\d{1,14}$/i',$recipient)) {
-                throw new \InvalidArgumentException(__d('cake_d_c/users', 'Invalid Recipient {0}: Format must be +1234567890', $recipient));
+            if (!preg_match($this->getConfig('phonePattern'), $recipient)) {
+                throw new \InvalidArgumentException(__d('cake_d_c/users', 'Invalid Recipient {0}: Format must be {1}', $recipient, $this->getConfig('phonePattern')));
             }
         });
 
         $responses = [];
-        return $responses;
         foreach ($recipients as $recipient) {
             $responses[] = $client->messages->create(
                 $recipient,

diff --git a/src/Mailer/UsersMailer.php b/src/Mailer/UsersMailer.php
@@ -123,7 +123,7 @@ protected function socialAccountValidation(EntityInterface $user, EntityInterfac
     public function otp(EntityInterface $user, $code)
     {
         $this->setTo($user->email);
-        $this->deliver(__(Configure::read('Code2f.message'), $code, Configure::read('App.name')));
+        $this->deliver(__d('cake_d_c/users', Configure::read('Code2f.message'), $code, Configure::read('App.name')));
     }
 
 }
diff --git a/src/Model/Table/OtpCodesTable.php b/src/Model/Table/OtpCodesTable.php
@@ -15,6 +15,7 @@
 use Cake\Core\Configure;
 use Cake\Datasource\Exception\RecordNotFoundException;
 use Cake\I18n\FrozenTime;
+use Cake\Log\Log;
 use Cake\Mailer\Mailer;
 use Cake\ORM\Query;
 use Cake\ORM\RulesChecker;
@@ -109,23 +110,34 @@ public function sendCode2f($userId, $resend = false)
     {
         $user = $this->Users->get($userId);
         $new = false;
-        if ($otpCode = $this->_getCurrent($userId)) {
-            if (!$resend) return $otpCode;
-        } else {
-            $new = true;
-            $otpCode = $this->_generateCode($userId);
+        try {
+            if ($otpCode = $this->_getCurrent($userId)) {
+                if (!$resend) return $otpCode;
+            } else {
+                $new = true;
+                $otpCode = $this->_generateCode($userId);
+            }
+        } catch (\Exception $e) {
+            Log::error($e->getMessage());
+            throw new \UnexpectedValueException(__d('cake_d_c/users', 'An error has occurred generating code. Please try again.'));
         }
+
         if (!$otpCode) {
             throw new RecordNotFoundException(__d('cake_d_c/users', 'Verification code could not be generated'));
         }
         if ($resend && !$new && (new FrozenTime())->diffInSeconds($otpCode->created) < 60) {
             throw new \OverflowException(__d('cake_d_c/users', 'You need to wait at least 60 seconds to request a new code'));
         }
         $type = Configure::read('Code2f.type');
-        if ($type === Code2fAuthenticationCheckerInterface::CODE2F_TYPE_PHONE) {
-            (new SMSMailer(Configure::read('Code2f.config', 'sms')))->otp($user, $otpCode->code);
-        } elseif ($type === Code2fAuthenticationCheckerInterface::CODE2F_TYPE_EMAIL) {
-            (new UsersMailer(Configure::read('Code2f.config', 'default')))->otp($user, $otpCode->code);
+        try {
+            if ($type === Code2fAuthenticationCheckerInterface::CODE2F_TYPE_PHONE) {
+                (new SMSMailer(Configure::read('Code2f.config', 'sms')))->otp($user, $otpCode->code);
+            } elseif ($type === Code2fAuthenticationCheckerInterface::CODE2F_TYPE_EMAIL) {
+                (new UsersMailer(Configure::read('Code2f.config', 'default')))->otp($user, $otpCode->code);
+            }
+        } catch (\Exception $e) {
+            Log::error($e->getMessage());
+            throw new \UnexpectedValueException(__d('cake_d_c/users', 'An error has occurred sending code. Please try again.'));
         }
 
         return $otpCode;