Add: secret verification before applying it to the user

application/forms/Account/TotpForm.php

@@ -71,7 +71,6 @@ class TotpForm extends PreferenceForm
 
         if (isset($formData['enabled_2fa']) && $formData['enabled_2fa']
             || $this->enabled2FA) {
-
             $this->addElement(
                 'text',
                 'totp_secret',
@@ -87,7 +86,55 @@ class TotpForm extends PreferenceForm
                 ]
             );
 
-            if ($this->totp->getSecret() !== null) {
+            $this->addElement(
+                'text',
+                'new_totp_secret',
+                [
+                    'label' => $this->translate('New TOTP Secret:'),
+                    'value' => $this->totp->getTemporarySecret() ?? $this->translate('No Secret set'),
+                    'description' => $this->translate(
+                        'If you generate a new TOTP secret, you will need to reconfigure your TOTP application with the new secret. ' .
+                        'If you reset the TOTP secret, you will lose access to your TOTP application and will need to set it up again.'
+                    ),
+                    'disabled' => true,
+//                    'decorators' => ['ViewHelper']
+                ]
+            );
+
+            if ($this->totp->getTemporarySecret() !== null) {
+                $this->addElement(
+                    'text',
+                    'totp_verification_code',
+                    [
+                        'label' => $this->translate('Verification Code:'),
+                        'description' => $this->translate(
+                            'Please enter the verification code from your TOTP application to verify the new secret.'
+                        ),
+                        'class' => 'autofocus content-centered',
+                        'style' => 'width: 200px;',
+                        'autocomplete' => 'off',
+                    ]
+                );
+                $this->addElement(
+                    'submit',
+                    'btn_verify_totp',
+                    array(
+                        'ignore' => true,
+                        'label' => $this->translate('Verify TOTP Secret'),
+                        'decorators' => array('ViewHelper'),
+                    )
+                );
+                $this->addDisplayGroup(
+                    array('totp_verification_code', 'btn_verify_totp'),
+                    'verify_buttons',
+                    array(
+                        'decorators' => array(
+                            'FormElements',
+                            array('HtmlTag', array('tag' => 'div', 'class' => 'control-group form-controls'))
+                        )
+                    )
+                );
+
                 $this->addElement(
                     'submit',
                     'btn_renew_totp',
@@ -131,8 +178,22 @@ class TotpForm extends PreferenceForm
             )
         );
 
+
+        if (isset($formData['enabled_2fa']) && $formData['enabled_2fa']) {
             $this->addDisplayGroup(
-            array('btn_submit', 'btn_delete_totp', 'btn_renew_totp', 'btn_generate_totp'),
+                array('btn_delete_totp', 'btn_renew_totp', 'btn_generate_totp'),
+                'change_buttons',
+                array(
+                    'decorators' => array(
+                        'FormElements',
+                        array('HtmlTag', array('tag' => 'div', 'class' => 'control-group form-controls'))
+                    )
+                )
+            );
+        }
+
+        $this->addDisplayGroup(
+            array('btn_submit'),
             'submit_buttons',
             array(
                 'decorators' => array(
@@ -152,12 +213,15 @@ class TotpForm extends PreferenceForm
             if ($this->getElement('btn_submit') && $this->getElement('btn_submit')->isChecked()) {
                 $this->preferences = new Preferences($this->store ? $this->store->load() : array());
                 $webPreferences = $this->preferences->get('icingaweb');
+                if ($this->totp->hasPendingChanges()
+                || $this->getValue('enabled_2fa') !== $webPreferences['enabled_2fa']) {
+                    if (! $this->totp->requiresSecretCheck()) {
                         foreach ($this->getValues() as $key => $value) {
                             if (in_array($key, self::PREFERENCE_KEYS, true)) {
                                 $webPreferences[$key] = $value;
                             }
                         }
-                $this->totp->makeStatePersistent();
+                        $this->totp->makeChangesPermanent();
                         Session::getSession()->delete('enabled_2fa');
                         if ($webPreferences['enabled_2fa'] == 1) {
                             $webPreferences['enabled_2fa'] = $this->totp->userHasSecret() ? '1' : '0';
@@ -165,24 +229,41 @@ class TotpForm extends PreferenceForm
                         $this->preferences->icingaweb = $webPreferences;
                         Session::getSession()->user->setPreferences($this->preferences);
                         $this->save();
-                Notification::success($this->translate('Submitted btn_submit'));
+                        Notification::success($this->translate('Saved Changes.'));
 
                         return true;
+                    } else {
+                        Notification::warning($this->translate('The new secret needs to be verified before saving.'));
+                    }
+                } else {
+                    Notification::info($this->translate('No changes to save.'));
+                }
             } elseif ($this->getElement('btn_generate_totp') && $this->getElement('btn_generate_totp')->isChecked()) {
                 $this->totp->generateSecret()->saveTemporaryInSession();
-                Notification::success($this->translate('Submitted btn_generate_totp'));
+                Notification::info($this->translate('A TOTP Secret has been generated.'));
 
                 return true;
             } elseif ($this->getElement('btn_renew_totp') && $this->getElement('btn_renew_totp')->isChecked()) {
-                $this->totp->renewSecret()->saveTemporaryInSession();
+                $this->totp->generateSecret()->saveTemporaryInSession();
-                Notification::success($this->translate('Submitted btn_renew_totp'));
+                Notification::info($this->translate('The TOTP Secret has been renewed.'));
 
                 return true;
             } elseif ($this->getElement('btn_delete_totp') && $this->getElement('btn_delete_totp')->isChecked()) {
-                $this->totp->deleteSecret()->saveTemporaryInSession();
+                $this->totp->deleteSecrets()->saveTemporaryInSession();
-                Notification::success($this->translate('Submitted btn_delete_totp'));
+                Notification::info($this->translate('Deleted TOTP Secret'));
 
                 return true;
+            } elseif ($this->getElement('btn_verify_totp') && $this->getElement('btn_verify_totp')->isChecked()) {
+                $verificationCode = $this->getValue('totp_verification_code');
+                if ($this->totp->approveTemporarySecret($verificationCode)) {
+                    Notification::success($this->translate('TOTP Secret verified successfully.'));
+
+                    return true;
+                } else {
+                    $this->getElement('totp_verification_code')->addError(
+                        $this->translate('Verification code is invalid.')
+                    );
+                }
             }
         } catch (Exception $e) {
             Logger::error($e);
@@ -219,6 +300,7 @@ class TotpForm extends PreferenceForm
             ($this->getElement('btn_generate_totp') && $this->getElement('btn_generate_totp')->isChecked())
             || ($this->getElement('btn_renew_totp') && $this->getElement('btn_renew_totp')->isChecked())
             || ($this->getElement('btn_delete_totp') && $this->getElement('btn_delete_totp')->isChecked())
+            || ($this->getElement('btn_verify_totp') && $this->getElement('btn_verify_totp')->isChecked())
             || ($this->getElement('btn_submit') && $this->getElement('btn_submit')->isChecked())
         ) {
             return true;

library/Icinga/Authentication/Totp.php

@@ -46,9 +46,16 @@ class Totp
      */
     const COLUMN_MODIFIED_TIME = 'mtime';
 
+    const STATE_SECRET_CHECK_REQUIRED = 'secret_check_required';
+    const STATE_HAS_PENDING_CHANGES = 'has_pending_changes';
+    const STATE_APPROVED_TEMPORARY_SECRET = 'approve_temporary_secret';
+
+
     protected string $username;
     protected PsrClock $clock;
-    protected extTOTP $totpObject;
+    protected ?extTOTP $totpObject = null;
+    protected ?extTOTP $temporaryTotpObject;
+    protected array $currentStates = [];
     private ?string $secret = null;
     private ?string $temporarySecret = null;
 
@@ -58,7 +65,7 @@ class Totp
         $this->username = $username;
         $this->clock = new PsrClock();
         $this->temporarySecret = $secret;
-        $this->setTotpObject();
+        $this->setTotpObjects();
     }
 
 
@@ -69,10 +76,19 @@ class Totp
      */
     public function userHasSecret(): bool
     {
-
         return $this->secret !== null;
     }
 
+    public function hasPendingChanges(): bool
+    {
+        return in_array(self::STATE_HAS_PENDING_CHANGES, $this->currentStates, true);
+    }
+
+    public function requiresSecretCheck(): bool
+    {
+        return in_array(self::STATE_SECRET_CHECK_REQUIRED, $this->currentStates, true);
+    }
+
 
     /**
      * Verifies the provided TOTP code against the user's secret.
@@ -82,44 +98,48 @@ class Totp
      */
     public function verify(string $code): bool
     {
-        if ($this->secret === null) {
+        if ($this->totpObject === null) {
+
             return false;
         }
-        return $this->totpObject->verify($code);
 
+        return $this->totpObject->verify($code);
+    }
+
+    public function approveTemporarySecret(string $code): bool
+    {
+        if ($this->temporarySecret !== null & $this->temporaryTotpObject->verify($code)) {
+            $this->setState(self::STATE_APPROVED_TEMPORARY_SECRET);
+            $this->removeState(self::STATE_SECRET_CHECK_REQUIRED);
+
+            return true;
+        }
+
+        return false;
     }
 
     public function generateSecret(): self
     {
-        $newSecret = $this->totpObject->getSecret();
+        $this->setTotpObjects(true)
-        if ($newSecret !== $this->secret && $newSecret !== $this->temporarySecret) {
+            ->setState(self::STATE_SECRET_CHECK_REQUIRED)
-            $this->temporarySecret = $this->totpObject->getSecret();
+            ->setState(self::STATE_HAS_PENDING_CHANGES)
-        }
+        ->removeState(self::STATE_APPROVED_TEMPORARY_SECRET);
 
         return $this;
     }
 
-    public function renewSecret(): self
+    public function deleteSecrets(): self
-    {
-        if (!($this->secret === null && $this->temporarySecret === null)) {
-            if (
-                ($currentSecret = $this->totpObject->getSecret())
-                && $currentSecret !== $this->temporarySecret
-                && $currentSecret !== $this->secret
-            ) {
-                $this->temporarySecret = $this->totpObject->getSecret();
-            } else {
-                $this->setTotpObject(true)->generateSecret();
-            }
-        }
-
-        return $this;
-    }
-
-    public function deleteSecret(): self
     {
+        if ($this->secret !== null || $this->totpObject !== null
+            || $this->temporarySecret !== null || $this->temporaryTotpObject !== null) {
             $this->secret = null;
+            $this->totpObject = null;
             $this->temporarySecret = null;
+            $this->temporaryTotpObject = null;
+            $this->setState(self::STATE_HAS_PENDING_CHANGES)
+                ->removeState(self::STATE_SECRET_CHECK_REQUIRED)
+                ->removeState(self::STATE_APPROVED_TEMPORARY_SECRET);
+        }
 
         return $this;
     }
@@ -133,7 +153,8 @@ class Totp
 
         return $this;
     }
-    public function makeStatePersistent(): self
+
+    public function makeChangesPermanent(): self
     {
         $db = $this->getWebDb();
         $db->beginTransaction();
@@ -141,8 +162,8 @@ class Totp
         $dbEntry = $db->prepexec(
             (new Select())
                 ->columns(['*'])
-                ->from('icingaweb_totp')
+                ->from(self::TABLE_NAME)
-                ->where(['username = ?' => $this->username])
+                ->where([self::COLUMN_USERNAME . ' = ?' => $this->username])
         )->getIterator()->current();
 
         try {
@@ -171,10 +192,8 @@ class Totp
                             ->where([self::COLUMN_USERNAME . ' = ?' => $this->username])
                     );
                 }
-                $this->secret = $this->temporarySecret;
+                $this->makeTemporaryObjectPermanent();
-                $this->temporarySecret = null;
                 $db->commitTransaction();
-
             } elseif ($this->secret === null && $dbEntry && $dbEntry->secret !== null) {
                 $db->prepexec(
                     (new Delete())
@@ -182,38 +201,38 @@ class Totp
                         ->where([self::COLUMN_USERNAME . ' = ?' => $this->username])
                 );
                 $db->commitTransaction();
-                $this->setTotpObject(true);
             }
 
+            $this->purgeStates();
             $this->saveTemporaryInSession();
         } catch (\Exception $e) {
             $db->rollBackTransaction();
-            throw new ConfigurationError(sprintf(
+            throw new ConfigurationError(
+                sprintf(
                     'Failed to persist TOTP state for user %s: %s',
                     $this->username,
                     $e->getMessage()
-            ), 0, $e);
+                ), 0, $e
+            );
         }
 
 
         return $this;
     }
 
+    public function getCurrentCode(): string
+    {
+        return $this->totpObject->now();
+    }
+
     /**
      * Retrieves the TOTP secret for the current user.
      *
      * @return string|null The TOTP secret or null if not found
      */
-
-
-    public function getCurrentCode(): string
-    {
-        return $this->totpObject->now();
-    }
     public function getSecret(): ?string
     {
-
+        return $this->secret;
-        return $this->temporarySecret ?? $this->secret;
     }
 
     public function getTemporarySecret(): ?string
@@ -221,6 +240,11 @@ class Totp
         return $this->temporarySecret;
     }
 
+    public function getSecretToDisplay(): ?string
+    {
+        return $this->temporarySecret ?? $this->secret;
+    }
+
 
     /**
      * Returns a query for the TOTP model.
@@ -277,10 +301,12 @@ class Totp
     private function ensureIsTotpModel(Model $totp): ?TotpModel
     {
         if (!$totp instanceof TotpModel) {
-            throw new ConfigurationError(sprintf(
+            throw new ConfigurationError(
+                sprintf(
                     'Expected TotpModel, got %s',
                     get_class($totp)
-            ));
+                )
+            );
         }
 
         return $totp;
@@ -293,30 +319,69 @@ class Totp
      * @param bool $new If true, a new TOTP object is created regardless of the existing secret
      * @return self Returns the current instance for method chaining
      */
-    private function setTotpObject(bool $new = false): self
+    private function setTotpObjects(bool $new = false): self
     {
-        $totpModel = null;
+        if (!$new) {
             if ($this->secret === null && ($totpModel = $this->getTotpModel()) !== null) {
                 $this->secret = $totpModel->secret;
             }
 
-        if (!$new) {
             if (isset($this->totpObject)) {
-
                 return $this;
-            } elseif ($this->temporarySecret !== null) {
+            }
-                $this->totpObject = extTOTP::createFromSecret($this->temporarySecret, $this->clock);
+            $this->temporaryTotpObject = $this->temporarySecret !== null
+                ? extTOTP::createFromSecret($this->temporarySecret, $this->clock)
+                : null;
 
-                return $this;
+            $this->totpObject = $this->secret !== null
-            } elseif ($totpModel !== null) {
+                ? extTOTP::createFromSecret($this->secret, $this->clock)
-                $this->totpObject = extTOTP::createFromSecret($this->secret, $this->clock);
+                : null;
+        } else {
+            $this->temporaryTotpObject = extTOTP::generate($this->clock);
+            $this->temporarySecret = $this->temporaryTotpObject->getSecret();
+        }
 
         return $this;
     }
-        }
 
-        $this->totpObject = extTOTP::generate($this->clock);
+    private function makeTemporaryObjectPermanent(): self
+    {
+        if ($this->temporaryTotpObject !== null) {
+            $this->totpObject = $this->temporaryTotpObject;
+            $this->secret = $this->totpObject->getSecret();
+            $this->temporarySecret = null;
+            $this->temporaryTotpObject = null;
+        }
 
         return $this;
     }
+
+    private function setState(string $key): self
+    {
+        if (! in_array($key, $this->currentStates, true)) {
+            $this->currentStates[] = $key;
+        }
+
+        return $this;
+    }
+
+    private function removeState(string $key): self
+    {
+        $this->currentStates = array_filter(
+            $this->currentStates,
+            function ($state) use ($key) {
+                return $state !== $key;
+            }
+        );
+
+        return $this;
+    }
+
+    private function purgeStates(): self
+    {
+        $this->currentStates = [];
+
+        return $this;
+    }
+
 }