Initial OTP implementation #981
Conversation
ajibarra
force-pushed
the
feature/otp-7cake4
branch
from
4d1e295
to
adceb67
Compare
ajibarra
force-pushed
the
feature/otp-7cake4
branch
from
adceb67
to
35862b1
Compare
ajibarra
force-pushed
the
feature/otp-7cake4
branch
from
35862b1
to
8bb108a
Compare
config/users.php
Outdated
| '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'), |
There was a problem hiding this comment.
I don't think the {0} and {1} will work here as expected.
There was a problem hiding this comment.
Removed _d( since we will replace {0} with code and {1} with App Name. Added a comment in users.php.
| 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)) { |
There was a problem hiding this comment.
Will this work with any country? if not we may need to move this so the app can extend
There was a problem hiding this comment.
I am moving this to a config since it works with any country but may not work with any sms service. Defaulting on Twilio to the current one.
| $this->request->getSession()->write(TwoFactorAuthenticator::USER_SESSION_KEY, $data['user']); | ||
| return $this->redirectWithQuery(Configure::read('Auth.AuthenticationComponent.loginAction')); | ||
| } catch (\Exception $e) { | ||
| $this->Flash->error($e->getMessage()); |
There was a problem hiding this comment.
Are we sure that we will not show bad error messages like database error or expose file path?
There was a problem hiding this comment.
only specific exceptions are catched now
| try { | ||
| $OtpCodes->sendCode2f($data['user']['id'], $resend); | ||
| } catch (\Exception $e) { | ||
| $this->Flash->error($e->getMessage()); |
There was a problem hiding this comment.
Are we sure that we will not show bad error messages like database error or expose file path?
There was a problem hiding this comment.
I have encapsulated potential exceptions logging them and throwing new ones with generic messages. Additionally only specific exceptions are catched now
| use Cake\Mailer\Message; | ||
| use Twilio\Rest\Client; | ||
|
|
||
| class TwilioTransport extends AbstractTransport |
There was a problem hiding this comment.
Do we need this tranport class? I see twilio is required in composer but I don't see when we are using this in the code.
There was a problem hiding this comment.
We do, this is the default transport class provided to users who wants otp with SMS out of the box. They can always implement a new transport or use email of course.
ajibarra
force-pushed
the
feature/otp-7cake4
branch
2 times, most recently
from
9f48763
to
65c8c45
Compare
ajibarra
force-pushed
the
feature/otp-7cake4
branch
from
65c8c45
to
ef3a33e
Compare
ajibarra
force-pushed
the
feature/otp-7cake4
branch
from
42d6d14
to
4a02570
Compare
No description provided.