Adding Google Authenticator #439

Merged
merged 17 commits into from Nov 2, 2016

Conversation

Projects
None yet
3 participants
@anvyst
Contributor

anvyst commented Oct 31, 2016

I've expanded a bit the default LoginTrait to support Google Authenticator mechanism, by using TwoFactorAuth plugin (since version 1.5.2, which became PSR-4 compatible for CakePHP3.x).

In order to function properly, there was also added migration file, to expand users table with secret, secret_verified fields. One holds Base32 shared secret, the second one - makes sure QR-code is shown in verify.ctp until the first successful verification/login into the system.

After the user is authorised from /login, I'm creating temporary session, with holds all required data for me, removing Auth.Users session (to avoid access for restricted url's), and redirect to /verify, where verification process takes place. Once verification is successful, I'm restoring Auth.Users session to let AuthComponent magic to work.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 31, 2016

Coverage Status

Coverage decreased (-3.4%) to 77.391% when pulling ffebc26 on anvyst:master into 647ce97 on CakeDC:master.

Coverage Status

Coverage decreased (-3.4%) to 77.391% when pulling ffebc26 on anvyst:master into 647ce97 on CakeDC:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 31, 2016

Coverage Status

Coverage decreased (-2.4%) to 78.404% when pulling fd2fa2b on anvyst:master into 647ce97 on CakeDC:master.

Coverage Status

Coverage decreased (-2.4%) to 78.404% when pulling fd2fa2b on anvyst:master into 647ce97 on CakeDC:master.

@steinkel

Please check review comments

composer.json
@@ -27,7 +27,8 @@
"source": "https://github.com/CakeDC/users"
},
"require": {
- "cakephp/cakephp": ">=3.2.9 <4.0.0"
+ "cakephp/cakephp": ">=3.2.9 <4.0.0",
+ "robthree/twofactorauth": "^1.5.2"

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

we should not require this lib by default, we are trying to keep the plugin as clean as possible and let every user include the required packages if needed, for example see the oauth related packages

@steinkel

steinkel Nov 1, 2016

Member

we should not require this lib by default, we are trying to keep the plugin as clean as possible and let every user include the required packages if needed, for example see the oauth related packages

@@ -36,15 +37,17 @@
"league/oauth2-instagram": "@stable",
"league/oauth2-google": "@stable",
"league/oauth2-linkedin": "@stable",
- "google/recaptcha": "@stable"
+ "google/recaptcha": "@stable",
+ "robthree/twofactorauth": "^1.5.2"

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

Is this needed here? or could we leave this as suggest only and add some docs on how to properly setup the 2factor feature?

@steinkel

steinkel Nov 1, 2016

Member

Is this needed here? or could we leave this as suggest only and add some docs on how to properly setup the 2factor feature?

This comment has been minimized.

@anvyst

anvyst Nov 1, 2016

Contributor

@steinkel , I've removed it from require param, and left it only in require-dev just to make sure that unit tests will pass.

@anvyst

anvyst Nov 1, 2016

Contributor

@steinkel , I've removed it from require param, and left it only in require-dev just to make sure that unit tests will pass.

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

makes sense, thanks

@steinkel

steinkel Nov 1, 2016

Member

makes sense, thanks

+ $table->addColumn('secret', 'string', [
+ 'after' => 'activation_date',
+ 'default' => null,
+ 'limit' => 255,

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

secret is using any fixed size or should we use text instead?

@steinkel

steinkel Nov 1, 2016

Member

secret is using any fixed size or should we use text instead?

config/users.php
+ 'plugin' => 'CakeDC/Users',
+ 'controller' => 'Users',
+ 'action' => 'verify',
+ 'prefix' => null,

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

better to use "false" instead, to prevent issues with login inside a prefixed controller. We had issues with this before setting up the loginAction...

@steinkel

steinkel Nov 1, 2016

Member

better to use "false" instead, to prevent issues with login inside a prefixed controller. We had issues with this before setting up the loginAction...

@@ -32,6 +32,7 @@ class UsersAuthComponent extends Component
const EVENT_BEFORE_LOGOUT = 'Users.Component.UsersAuth.beforeLogout';
const EVENT_AFTER_LOGOUT = 'Users.Component.UsersAuth.afterLogout';
+

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

phpcs

@steinkel

steinkel Nov 1, 2016

Member

phpcs

src/Controller/Traits/LoginTrait.php
+ }
+
+ if ($this->request->is('post')) {
+ $verificationCode = $this->request->data['code'];

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

use request->data() instead

@steinkel

steinkel Nov 1, 2016

Member

use request->data() instead

src/Controller/Traits/LoginTrait.php
+ $verificationCode = $this->request->data['code'];
+ $user = $this->request->session()->read('temporarySession');
+
+ $codeVerified = $this->GoogleAuthenticator->verifyCode($user['secret'], $verificationCode);

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

check user keys before using them

@steinkel

steinkel Nov 1, 2016

Member

check user keys before using them

src/Controller/Traits/LoginTrait.php
+
+ $url = $this->Auth->redirectUrl();
+
+ $this->redirect($url);

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

use return

@steinkel

steinkel Nov 1, 2016

Member

use return

src/Controller/Traits/LoginTrait.php
+
+ $this->request->session()->destroy();
+
+ $this->redirect(Configure::read('Auth.loginAction'));

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

use return

@steinkel

steinkel Nov 1, 2016

Member

use return

src/Template/Users/verify.ctp
+ <?php if (!empty($secretDataUri)):?>
+ <p class='text-center'><img src="<?php echo $secretDataUri;?>"/></p>
+ <?php endif;?>
+ <?= $this->Form->input('code', ['required' => true,'label' => 'Verification Code']) ?>

This comment has been minimized.

@steinkel

steinkel Nov 1, 2016

Member

use __d() for label

@steinkel

steinkel Nov 1, 2016

Member

use __d() for label

@steinkel steinkel changed the base branch from master to develop Nov 1, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 1, 2016

Coverage Status

Coverage decreased (-2.7%) to 78.35% when pulling b1a0057 on anvyst:master into 372ad73 on CakeDC:develop.

Coverage Status

Coverage decreased (-2.7%) to 78.35% when pulling b1a0057 on anvyst:master into 372ad73 on CakeDC:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 1, 2016

Coverage Status

Coverage decreased (-2.7%) to 78.35% when pulling b1a0057 on anvyst:master into 372ad73 on CakeDC:develop.

Coverage Status

Coverage decreased (-2.7%) to 78.35% when pulling b1a0057 on anvyst:master into 372ad73 on CakeDC:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 2, 2016

Coverage Status

Coverage decreased (-2.7%) to 78.35% when pulling a90f722 on anvyst:master into 372ad73 on CakeDC:develop.

Coverage Status

Coverage decreased (-2.7%) to 78.35% when pulling a90f722 on anvyst:master into 372ad73 on CakeDC:develop.

@steinkel steinkel merged commit ebf2957 into CakeDC:develop Nov 2, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-2.7%) to 78.35%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@steinkel

This comment has been minimized.

Show comment
Hide comment
@steinkel

steinkel Feb 20, 2017

Member

looks like this is making our tests break against latest v1.6 of the two factor auth lib, I've created #500 and pinned version to 1.5. While checking v1.5 this param was not used at all in the lib, could you please take a look if this should be removed/fixed?

looks like this is making our tests break against latest v1.6 of the two factor auth lib, I've created #500 and pinned version to 1.5. While checking v1.5 this param was not used at all in the lib, could you please take a look if this should be removed/fixed?

This comment has been minimized.

Show comment
Hide comment
@anvyst

anvyst Feb 20, 2017

Contributor

@steinkel , i'll check it today, thanks for the note.

Contributor

anvyst replied Feb 20, 2017

@steinkel , i'll check it today, thanks for the note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment