Improve 2FA

6 files changed, 135 insertions, 77 deletions

diff --git a/app/Auth/TotpAuth.php b/app/Auth/TotpAuth.php
index f41fabd8..e1cc712c 100644
--- a/app/Auth/TotpAuth.php
+++ b/app/Auth/TotpAuth.php
@@ -40,7 +40,7 @@ class TotpAuth extends Base implements PostAuthenticationProviderInterface
      */
     public function getName()
     {
-        return 'Time-based One-time Password Algorithm';
+        return t('Time-based One-time Password Algorithm');
     }
 
     /**
@@ -56,6 +56,16 @@ class TotpAuth extends Base implements PostAuthenticationProviderInterface
     }
 
     /**
+     * Called before to prompt the user
+     *
+     * @access public
+     */
+    public function beforeCode()
+    {
+
+    }
+
+    /**
      * Set validation code
      *
      * @access public
@@ -67,6 +77,18 @@ class TotpAuth extends Base implements PostAuthenticationProviderInterface
     }
 
     /**
+     * Generate secret
+     *
+     * @access public
+     * @return string
+     */
+    public function generateSecret()
+    {
+        $this->secret = GoogleAuthenticator::generateRandom();
+        return $this->secret;
+    }
+
+    /**
      * Set secret token
      *
      * @access public
@@ -85,10 +107,6 @@ class TotpAuth extends Base implements PostAuthenticationProviderInterface
      */
     public function getSecret()
     {
-        if (empty($this->secret)) {
-            $this->secret = GoogleAuthenticator::generateRandom();
-        }
-
         return $this->secret;
     }
 
diff --git a/app/Controller/Twofactor.php b/app/Controller/Twofactor.php
index aeb13acc..8dbfcf66 100644
--- a/app/Controller/Twofactor.php
+++ b/app/Controller/Twofactor.php
@@ -23,7 +23,7 @@ class Twofactor extends User
     }
 
     /**
-     * Index
+     * Show form to disable/enable 2FA
      *
      * @access public
      */
@@ -31,54 +31,45 @@ class Twofactor extends User
     {
         $user = $this->getUser();
         $this->checkCurrentUser($user);
-
-        $provider = $this->authenticationManager->getPostAuthenticationProvider();
-        $label = $user['email'] ?: $user['username'];
-
-        $provider->setSecret($user['twofactor_secret']);
+        unset($this->sessionStorage->twoFactorSecret);
 
         $this->response->html($this->layout('twofactor/index', array(
             'user' => $user,
-            'qrcode_url' => $user['twofactor_activated'] == 1 ? $provider->getQrCodeUrl($label) : '',
-            'key_url' => $user['twofactor_activated'] == 1 ? $provider->getKeyUrl($label) : '',
+            'provider' => $this->authenticationManager->getPostAuthenticationProvider()->getName(),
         )));
     }
 
     /**
-     * Enable/disable 2FA
+     * Show page with secret and test form
      *
      * @access public
      */
-    public function save()
+    public function show()
     {
         $user = $this->getUser();
         $this->checkCurrentUser($user);
 
-        $values = $this->request->getValues();
+        $label = $user['email'] ?: $user['username'];
+        $provider = $this->authenticationManager->getPostAuthenticationProvider();
 
-        if (isset($values['twofactor_activated']) && $values['twofactor_activated'] == 1) {
-            $this->user->update(array(
-                'id' => $user['id'],
-                'twofactor_activated' => 1,
-                'twofactor_secret' => $this->authenticationManager->getPostAuthenticationProvider()->getSecret(),
-            ));
+        if (! isset($this->sessionStorage->twoFactorSecret)) {
+            $provider->generateSecret();
+            $provider->beforeCode();
+            $this->sessionStorage->twoFactorSecret = $provider->getSecret();
         } else {
-            $this->user->update(array(
-                'id' => $user['id'],
-                'twofactor_activated' => 0,
-                'twofactor_secret' => '',
-            ));
+            $provider->setSecret($this->sessionStorage->twoFactorSecret);
         }
 
-        // Allow the user to test or disable the feature
-        $this->userSession->disablePostAuthentication();
-
-        $this->flash->success(t('User updated successfully.'));
-        $this->response->redirect($this->helper->url->to('twofactor', 'index', array('user_id' => $user['id'])));
+        $this->response->html($this->layout('twofactor/show', array(
+            'user' => $user,
+            'secret' => $this->sessionStorage->twoFactorSecret,
+            'qrcode_url' => $provider->getQrCodeUrl($label),
+            'key_url' => $provider->getKeyUrl($label),
+        )));
     }
 
     /**
-     * Test code
+     * Test code and save secret
      *
      * @access public
      */
@@ -91,14 +82,47 @@ class Twofactor extends User
 
         $provider = $this->authenticationManager->getPostAuthenticationProvider();
         $provider->setCode(empty($values['code']) ? '' : $values['code']);
-        $provider->setSecret($user['twofactor_secret']);
+        $provider->setSecret($this->sessionStorage->twoFactorSecret);
 
         if ($provider->authenticate()) {
             $this->flash->success(t('The two factor authentication code is valid.'));
+
+            $this->user->update(array(
+                'id' => $user['id'],
+                'twofactor_activated' => 1,
+                'twofactor_secret' => $this->authenticationManager->getPostAuthenticationProvider()->getSecret(),
+            ));
+
+            unset($this->sessionStorage->twoFactorSecret);
+            $this->userSession->disablePostAuthentication();
+
+            $this->response->redirect($this->helper->url->to('twofactor', 'index', array('user_id' => $user['id'])));
         } else {
             $this->flash->failure(t('The two factor authentication code is not valid.'));
+            $this->response->redirect($this->helper->url->to('twofactor', 'show', array('user_id' => $user['id'])));
         }
+    }
 
+    /**
+     * Disable 2FA for the current user
+     *
+     * @access public
+     */
+    public function deactivate()
+    {
+        $user = $this->getUser();
+        $this->checkCurrentUser($user);
+
+        $this->user->update(array(
+            'id' => $user['id'],
+            'twofactor_activated' => 0,
+            'twofactor_secret' => '',
+        ));
+
+        // Allow the user to test or disable the feature
+        $this->userSession->disablePostAuthentication();
+
+        $this->flash->success(t('User updated successfully.'));
         $this->response->redirect($this->helper->url->to('twofactor', 'index', array('user_id' => $user['id'])));
     }
 
@@ -135,6 +159,12 @@ class Twofactor extends User
      */
     public function code()
     {
+        if (! isset($this->sessionStorage->twoFactorBeforeCodeCalled)) {
+            $provider = $this->authenticationManager->getPostAuthenticationProvider();
+            $provider->beforeCode();
+            $this->sessionStorage->twoFactorBeforeCodeCalled = true;
+        }
+
         $this->response->html($this->template->layout('twofactor/check', array(
             'title' => t('Check two factor authentication code'),
         )));
diff --git a/app/Core/Security/PostAuthenticationProviderInterface.php b/app/Core/Security/PostAuthenticationProviderInterface.php
index 88fc2fe5..3f628bb0 100644
--- a/app/Core/Security/PostAuthenticationProviderInterface.php
+++ b/app/Core/Security/PostAuthenticationProviderInterface.php
@@ -11,6 +11,13 @@ namespace Kanboard\Core\Security;
 interface PostAuthenticationProviderInterface extends AuthenticationProviderInterface
 {
     /**
+     * Called only one time before to prompt the user for pin code
+     *
+     * @access public
+     */
+    public function beforeCode();
+
+    /**
      * Set user pin-code
      *
      * @access public
@@ -19,6 +26,14 @@ interface PostAuthenticationProviderInterface extends AuthenticationProviderInte
     public function setCode($code);
 
     /**
+     * Generate secret if necessary
+     *
+     * @access public
+     * @return string
+     */
+    public function generateSecret();
+
+    /**
      * Set secret token (fetched from user profile)
      *
      * @access public
diff --git a/app/Core/Security/Token.php b/app/Core/Security/Token.php
index 9fd2d02b..cbd784a8 100644
--- a/app/Core/Security/Token.php
+++ b/app/Core/Security/Token.php
@@ -21,15 +21,7 @@ class Token extends Base
      */
     public static function getToken()
     {
-        if (function_exists('random_bytes')) {
-            return bin2hex(random_bytes(30));
-        } elseif (function_exists('openssl_random_pseudo_bytes')) {
-            return bin2hex(openssl_random_pseudo_bytes(30));
-        } elseif (ini_get('open_basedir') === '' && strtoupper(substr(PHP_OS, 0, 3)) !== 'WIN') {
-            return hash('sha256', file_get_contents('/dev/urandom', false, null, 0, 30));
-        }
-
-        return hash('sha256', uniqid(mt_rand(), true));
+        return bin2hex(random_bytes(30));
     }
 
     /**
diff --git a/app/Template/twofactor/index.php b/app/Template/twofactor/index.php
index 4c4ca088..b9ee4b49 100644
--- a/app/Template/twofactor/index.php
+++ b/app/Template/twofactor/index.php
@@ -2,42 +2,14 @@
     <h2><?= t('Two factor authentication') ?></h2>
 </div>
 
-<form method="post" action="<?= $this->url->href('twofactor', 'save', array('user_id' => $user['id'])) ?>" autocomplete="off">
-
+<form method="post" action="<?= $this->url->href('twofactor', $user['twofactor_activated'] == 1 ? 'deactivate' : 'show', array('user_id' => $user['id'])) ?>" autocomplete="off">
     <?= $this->form->csrf() ?>
-    <?= $this->form->checkbox('twofactor_activated', t('Enable/disable two factor authentication'), 1, isset($user['twofactor_activated']) && $user['twofactor_activated'] == 1) ?>
-
+    <p><?= t('Two-Factor Provider: ') ?><strong><?= $this->e($provider) ?></strong></p>
     <div class="form-actions">
-        <input type="submit" value="<?= t('Save') ?>" class="btn btn-blue"/>
-    </div>
-</form>
-
-<?php if ($user['twofactor_activated'] == 1): ?>
-<div class="listing">
-    <p><?= t('Secret key: ') ?><strong><?= $this->e($user['twofactor_secret']) ?></strong> (base32)</p>
-
-    <?php if (! empty($qrcode_url)): ?>
-        <p><br/><img src="<?= $qrcode_url ?>"/><br/><br/></p>
-    <?php endif ?>
-
-    <p>
-        <?php if (! empty($key_url)): ?>
-            <?= t('This QR code contains the key URI: ') ?><strong><?= $this->e($key_url) ?></strong>
-            <br/><br/>
+        <?php if ($user['twofactor_activated'] == 1): ?>
+            <input type="submit" value="<?= t('Disable two-factor authentication') ?>" class="btn btn-red"/>
+        <?php else: ?>
+            <input type="submit" value="<?= t('Enable two-factor authentication') ?>" class="btn btn-blue"/>
         <?php endif ?>
-        <?= t('Save the secret key in your TOTP software (by example Google Authenticator or FreeOTP).') ?>
-    </p>
-</div>
-
-<h3><?= t('Test your device') ?></h3>
-<form method="post" action="<?= $this->url->href('twofactor', 'test', array('user_id' => $user['id'])) ?>" autocomplete="off">
-
-    <?= $this->form->csrf() ?>
-    <?= $this->form->label(t('Code'), 'code') ?>
-    <?= $this->form->text('code', array(), array(), array('placeholder="123456"'), 'form-numeric') ?>
-
-    <div class="form-actions">
-        <input type="submit" value="<?= t('Check my code') ?>" class="btn btn-blue"/>
     </div>
 </form>
-<?php endif ?>
diff --git a/app/Template/twofactor/show.php b/app/Template/twofactor/show.php
new file mode 100644
index 00000000..dd72965a
--- /dev/null
+++ b/app/Template/twofactor/show.php
@@ -0,0 +1,31 @@
+<div class="page-header">
+    <h2><?= t('Two factor authentication') ?></h2>
+</div>
+
+<?php if (! empty($secret) || ! empty($qrcode_url) || ! empty($key_url)): ?>
+<div class="listing">
+    <?php if (! empty($secret)): ?>
+        <p><?= t('Secret key: ') ?><strong><?= $this->e($secret) ?></strong></p>
+    <?php endif ?>
+
+    <?php if (! empty($qrcode_url)): ?>
+        <p><br/><img src="<?= $qrcode_url ?>"/><br/><br/></p>
+    <?php endif ?>
+
+    <?php if (! empty($key_url)): ?>
+        <p><?= t('This QR code contains the key URI: ') ?><a href="<?= $this->e($key_url) ?>"><?= $this->e($key_url) ?></a></p>
+    <?php endif ?>
+</div>
+<?php endif ?>
+
+<h3><?= t('Test your device') ?></h3>
+<form method="post" action="<?= $this->url->href('twofactor', 'test', array('user_id' => $user['id'])) ?>" autocomplete="off">
+
+    <?= $this->form->csrf() ?>
+    <?= $this->form->label(t('Code'), 'code') ?>
+    <?= $this->form->text('code', array(), array(), array('placeholder="123456"', 'autofocus'), 'form-numeric') ?>
+
+    <div class="form-actions">
+        <input type="submit" value="<?= t('Check my code') ?>" class="btn btn-blue"/>
+    </div>
+</form>
\ No newline at end of file