diff --git a/application/forms/Account/TotpForm.php b/application/forms/Account/TotpForm.php index 146717552..236d61715 100644 --- a/application/forms/Account/TotpForm.php +++ b/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_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', 'btn_delete_totp', 'btn_renew_totp', 'btn_generate_totp'), + array('btn_submit'), 'submit_buttons', array( 'decorators' => array( @@ -152,37 +213,57 @@ 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'); - foreach ($this->getValues() as $key => $value) { - if (in_array($key, self::PREFERENCE_KEYS, true)) { - $webPreferences[$key] = $value; - } - } - $this->totp->makeStatePersistent(); - Session::getSession()->delete('enabled_2fa'); - if ($webPreferences['enabled_2fa'] == 1) { - $webPreferences['enabled_2fa'] = $this->totp->userHasSecret() ? '1' : '0'; - } - $this->preferences->icingaweb = $webPreferences; - Session::getSession()->user->setPreferences($this->preferences); - $this->save(); - Notification::success($this->translate('Submitted btn_submit')); + 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->makeChangesPermanent(); + Session::getSession()->delete('enabled_2fa'); + if ($webPreferences['enabled_2fa'] == 1) { + $webPreferences['enabled_2fa'] = $this->totp->userHasSecret() ? '1' : '0'; + } + $this->preferences->icingaweb = $webPreferences; + Session::getSession()->user->setPreferences($this->preferences); + $this->save(); + Notification::success($this->translate('Saved Changes.')); - return true; + 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(); - Notification::success($this->translate('Submitted btn_renew_totp')); + $this->totp->generateSecret()->saveTemporaryInSession(); + 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(); - Notification::success($this->translate('Submitted btn_delete_totp')); + $this->totp->deleteSecrets()->saveTemporaryInSession(); + 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); @@ -202,7 +283,7 @@ class TotpForm extends PreferenceForm $auth = Auth::getInstance(); $values = $auth->getUser()->getPreferences()->get('icingaweb'); - if (!isset($values['enabled_2fa']) && ! Session::getSession()->get('enabled_2fa', false)) { + if (!isset($values['enabled_2fa']) && !Session::getSession()->get('enabled_2fa', false)) { $values['enabled_2fa'] = '0'; } @@ -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; diff --git a/library/Icinga/Authentication/Totp.php b/library/Icinga/Authentication/Totp.php index 09c59c90c..629233c6b 100644 --- a/library/Icinga/Authentication/Totp.php +++ b/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,48 +98,52 @@ 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(); - if ($newSecret !== $this->secret && $newSecret !== $this->temporarySecret) { - $this->temporarySecret = $this->totpObject->getSecret(); - } + $this->setTotpObjects(true) + ->setState(self::STATE_SECRET_CHECK_REQUIRED) + ->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(); - } + 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; } - public function deleteSecret(): self - { - $this->secret = null; - $this->temporarySecret = null; - - return $this; - } - public function saveTemporaryInSession(): self { Session::getSession()->set( @@ -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') - ->where(['username = ?' => $this->username]) + ->from(self::TABLE_NAME) + ->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->temporarySecret = null; + $this->makeTemporaryObjectPermanent(); $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( - 'Failed to persist TOTP state for user %s: %s', - $this->username, - $e->getMessage() - ), 0, $e); + throw new ConfigurationError( + sprintf( + 'Failed to persist TOTP state for user %s: %s', + $this->username, + $e->getMessage() + ), 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->temporarySecret ?? $this->secret; + return $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( - 'Expected TotpModel, got %s', - get_class($totp) - )); + 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 ($this->secret === null && ($totpModel = $this->getTotpModel()) !== null) { - $this->secret = $totpModel->secret; - } - if (!$new) { + if ($this->secret === null && ($totpModel = $this->getTotpModel()) !== null) { + $this->secret = $totpModel->secret; + } + if (isset($this->totpObject)) { - - return $this; - } elseif ($this->temporarySecret !== null) { - $this->totpObject = extTOTP::createFromSecret($this->temporarySecret, $this->clock); - - return $this; - } elseif ($totpModel !== null) { - $this->totpObject = extTOTP::createFromSecret($this->secret, $this->clock); - return $this; } - } + $this->temporaryTotpObject = $this->temporarySecret !== null + ? extTOTP::createFromSecret($this->temporarySecret, $this->clock) + : null; - $this->totpObject = extTOTP::generate($this->clock); + $this->totpObject = $this->secret !== null + ? extTOTP::createFromSecret($this->secret, $this->clock) + : null; + } else { + $this->temporaryTotpObject = extTOTP::generate($this->clock); + $this->temporarySecret = $this->temporaryTotpObject->getSecret(); + } return $this; } + + 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; + } + }