diff --git a/application/loginwebpage.class.inc.php b/application/loginwebpage.class.inc.php index 788bf6003..8d32ed890 100644 --- a/application/loginwebpage.class.inc.php +++ b/application/loginwebpage.class.inc.php @@ -322,11 +322,12 @@ class LoginWebPage extends NiceWebPage $oTwigContext->Render($this, 'resetpwddone.html.twig', $aVars); } - public function DisplayChangePwdForm($bFailedLogin = false) + public function DisplayChangePwdForm($bFailedLogin = false, $sIssue = null) { $oTwigContext = new LoginTwigRenderer(); $aVars = $oTwigContext->GetDefaultVars(); $aVars['bFailedLogin'] = $bFailedLogin; + $aVars['sIssue'] = $sIssue; $oTwigContext->Render($this, 'changepwdform.html.twig', $aVars); } @@ -1060,10 +1061,21 @@ class LoginWebPage extends NiceWebPage UserRights::Login($sAuthUser); // Set the user's language $sOldPwd = utils::ReadPostedParam('old_pwd', '', 'raw_data'); $sNewPwd = utils::ReadPostedParam('new_pwd', '', 'raw_data'); - if (UserRights::CanChangePassword() && ((!UserRights::CheckCredentials($sAuthUser, $sOldPwd)) || (!UserRights::ChangePassword($sOldPwd, $sNewPwd)))) + + try + { + if (UserRights::CanChangePassword() && ((!UserRights::CheckCredentials($sAuthUser, $sOldPwd)) || (!UserRights::ChangePassword($sOldPwd, $sNewPwd)))) + { + $oPage = self::NewLoginWebPage(); + $oPage->DisplayChangePwdForm(true); // old pwd was wrong + $oPage->output(); + exit; + } + } + catch (CoreCannotSaveObjectException $e) { $oPage = self::NewLoginWebPage(); - $oPage->DisplayChangePwdForm(true); // old pwd was wrong + $oPage->DisplayChangePwdForm(true, $e->getIssue()); // password policy was not met. $oPage->output(); exit; } diff --git a/core/coreexception.class.inc.php b/core/coreexception.class.inc.php index 112c61ed1..0017955ed 100644 --- a/core/coreexception.class.inc.php +++ b/core/coreexception.class.inc.php @@ -237,3 +237,13 @@ class ArchivedObjectException extends CoreException class InvalidConfigParamException extends CoreException { } + + +/** + * Throwned when the password is not valid + * + * @since 2.7.0 + */ +class InvalidPasswordAttributeOneWayPassword extends CoreException +{ +} \ No newline at end of file diff --git a/core/ormpassword.class.inc.php b/core/ormpassword.class.inc.php index 4b008f992..ce1963226 100644 --- a/core/ormpassword.class.inc.php +++ b/core/ormpassword.class.inc.php @@ -35,7 +35,7 @@ class ormPassword { protected $m_sHashed; protected $m_sSalt; - + /** * Constructor, initializes the password from the encrypted values */ @@ -53,7 +53,7 @@ class ormPassword { $this->m_sHashed = password_hash($sClearTextPassword, PASSWORD_DEFAULT); } - + /** * Print the password: displays some stars * @return string diff --git a/datamodels/2.x/authent-local/datamodel.authent-local.xml b/datamodels/2.x/authent-local/datamodel.authent-local.xml new file mode 100644 index 000000000..ab59d7c70 --- /dev/null +++ b/datamodels/2.x/authent-local/datamodel.authent-local.xml @@ -0,0 +1,10 @@ + + + + + + ^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^\da-zA-Z]).{8,15}$ + + + + diff --git a/datamodels/2.x/authent-local/en.dict.authent-local.php b/datamodels/2.x/authent-local/en.dict.authent-local.php index f3a3e475c..77941dafa 100755 --- a/datamodels/2.x/authent-local/en.dict.authent-local.php +++ b/datamodels/2.x/authent-local/en.dict.authent-local.php @@ -40,4 +40,6 @@ Dict::Add('EN US', 'English', 'English', array( 'Class:UserLocal+' => 'User authentified by iTop', 'Class:UserLocal/Attribute:password' => 'Password', 'Class:UserLocal/Attribute:password+' => 'user authentication string', + + 'Error:UserLocalPasswordValidator:UserPasswordPolicyRegex/validation_failed' => 'The password does not respect the policy', )); diff --git a/datamodels/2.x/authent-local/fr.dict.authent-local.php b/datamodels/2.x/authent-local/fr.dict.authent-local.php index c5c2b1134..1dad2019f 100755 --- a/datamodels/2.x/authent-local/fr.dict.authent-local.php +++ b/datamodels/2.x/authent-local/fr.dict.authent-local.php @@ -24,4 +24,6 @@ Dict::Add('FR FR', 'French', 'Français', array( 'Class:UserLocal+' => 'Utilisateur authentifié par iTop', 'Class:UserLocal/Attribute:password' => 'Mot de passe', 'Class:UserLocal/Attribute:password+' => '', + + 'Error:UserLocalPasswordValidator:UserPasswordPolicyRegex/validation_failed' => 'Le mot de passe ne respecte pas la politique de mot de passe.', )); diff --git a/datamodels/2.x/authent-local/model.authent-local.php b/datamodels/2.x/authent-local/model.authent-local.php index 5b000b040..a2ae3c93e 100755 --- a/datamodels/2.x/authent-local/model.authent-local.php +++ b/datamodels/2.x/authent-local/model.authent-local.php @@ -26,8 +26,48 @@ */ +class UserLocalPasswordValidity +{ + /** @var bool */ + protected $m_bPasswordValidity; + /** @var string|null */ + protected $m_sPasswordValidityMessage; + + /** + * UserLocalPasswordValidity constructor. + * + * @param bool $m_bPasswordValidity + * @param string $m_sPasswordValidityMessage + */ + public function __construct($m_bPasswordValidity, $m_sPasswordValidityMessage = null) + { + $this->m_bPasswordValidity = $m_bPasswordValidity; + $this->m_sPasswordValidityMessage = $m_sPasswordValidityMessage; + } + + /** + * @return bool + */ + public function isPasswordValid() + { + return $this->m_bPasswordValidity; + } + + + /** + * @return string + */ + public function getPasswordValidityMessage() + { + return $this->m_sPasswordValidityMessage; + } +} + class UserLocal extends UserInternal { + /** @var UserLocalPasswordValidity|null */ + protected $m_oPasswordValidity = null; + public static function Init() { $aParams = array @@ -82,13 +122,14 @@ class UserLocal extends UserInternal public function ChangePassword($sOldPassword, $sNewPassword) { - $oPassword = $this->Get('password'); // ormPassword object + /** @var \ormPassword $oPassword */ + $oPassword = $this->Get('password'); // Cannot compare directly the values since they are hashed, so // Let's ask the password to compare the hashed values if ($oPassword->CheckPassword($sOldPassword)) { $this->SetPassword($sNewPassword); - return true; + return $this->IsPasswordValid(); } return false; } @@ -102,6 +143,92 @@ class UserLocal extends UserInternal $this->DBUpdate(); } + public function Set($sAttCode, $value) + { + $result = parent::Set($sAttCode, $value); + + if ('password' == $sAttCode) + { + $this->ValidatePassword($value); + } + + return $result; + } + + public function IsPasswordValid() + { + return (isset($this->m_oPasswordValidity)) && ($this->m_oPasswordValidity->isPasswordValid()); + } + + /** + * set the $m_oPasswordValidity + * + * @param string $proposedValue + * @param \Config|null $config + * + * @return void + */ + public function ValidatePassword($proposedValue, $config = null) + { + if (null == $config) + { + $config = MetaModel::GetConfig(); + } + + $aPasswordValidationClasses = $config->GetModuleSetting('authent-local', 'password_validation.classes'); + if (empty($aPasswordValidationClasses)) + { + $aPasswordValidationClasses = array(); + } + + $sUserPasswordPolicyRegexPattern = $config->GetModuleSetting('authent-local', 'password_validation.pattern'); + if ($sUserPasswordPolicyRegexPattern) + { + if (array_key_exists('UserPasswordPolicyRegex', $aPasswordValidationClasses)) + { + $this->m_oPasswordValidity = new UserLocalPasswordValidity( + false, + "Invalid configuration: 'UserPasswordPolicyRegex' was defined twice (once into UserLocal.password_validation_advanced, once into UserLocal.password_validation)." + ); + return; + } + + $aPasswordValidationClasses['UserPasswordPolicyRegex'] = array('pattern' => $sUserPasswordPolicyRegexPattern); + } + + foreach ($aPasswordValidationClasses as $sClass => $aOptions) + { + if (!is_subclass_of($sClass, 'UserLocalPasswordValidator')) + { + $this->m_oPasswordValidity = new UserLocalPasswordValidity( + false, + "Invalid configuration: '{$sClass}' must implements ".UserLocalPasswordValidator::class + ); + return; + } + + /** @var \UserLocalPasswordValidator */ + $oInstance = new $sClass(); + + $this->m_oPasswordValidity = $oInstance->ValidatePassword($proposedValue, $aOptions, $this); + + if (!$this->m_oPasswordValidity->isPasswordValid()) + { + return; + } + } + } + + public function DoCheckToWrite() + { + if (! $this->IsPasswordValid()) + { + $this->m_aCheckIssues[] = $this->m_oPasswordValidity->getPasswordValidityMessage(); + } + + parent::DoCheckToWrite(); + } + /** * Returns the set of flags (OPT_ATT_HIDDEN, OPT_ATT_READONLY, OPT_ATT_MANDATORY...) * for the given attribute in the current state of the object @@ -129,3 +256,72 @@ class UserLocal extends UserInternal } } + + +interface UserLocalPasswordValidator +{ + public function __construct(); + + /** + * @param string $proposedValue + * @param array $aOptions + * @param UserLocal $oUserLocal + * + * @return UserLocalPasswordValidity + */ + public function ValidatePassword($proposedValue, $aOptions, UserLocal $oUserLocal); +} + +class UserPasswordPolicyRegex implements UserLocalPasswordValidator +{ + public function __construct() + { + } + + /** + * @param string $proposedValue + * @param array $aOptions + * @param UserLocal $oUserLocal + * + * @return UserLocalPasswordValidity + */ + public function ValidatePassword($proposedValue, $aOptions, UserLocal $oUserLocal) + { + + if (! array_key_exists('pattern', $aOptions) ) + { + return new UserLocalPasswordValidity( + false, + "Invalid configuration: key 'pattern' is mandatory" + ); + } + + $sPattern = $aOptions['pattern']; + if ('' == $sPattern) + { + return new UserLocalPasswordValidity(true); + } + + $isMatched = preg_match("/{$sPattern}/", $proposedValue); + if ($isMatched === false) + { + return new UserLocalPasswordValidity( + false, + 'Unknown error : Failed to check the password, please verify the password\'s Data Model.' + ); + } + + if ($isMatched === 1) + { + return new UserLocalPasswordValidity(true); + } + + $sMessage = Dict::S('Error:UserLocalPasswordValidator:UserPasswordPolicyRegex/validation_failed'); + + return new UserLocalPasswordValidity( + false, + $sMessage + ); + } +} + diff --git a/datamodels/2.x/authent-local/module.authent-local.php b/datamodels/2.x/authent-local/module.authent-local.php index 412c9520a..09a2d9b62 100755 --- a/datamodels/2.x/authent-local/module.authent-local.php +++ b/datamodels/2.x/authent-local/module.authent-local.php @@ -3,7 +3,7 @@ SetupWebPage::AddModule( __FILE__, // Path to the current file, all other file names are relative to the directory containing this file - 'authent-local/2.6.2', + 'authent-local/2.7.0', array( // Identification // @@ -36,7 +36,9 @@ SetupWebPage::AddModule( // Default settings // + 'settings' => array( + // see the './datamodel.authent-local.xml' for the default settings! ), ) ); diff --git a/lib/composer/autoload_classmap.php b/lib/composer/autoload_classmap.php index 8aeea3e25..09f32e3a1 100644 --- a/lib/composer/autoload_classmap.php +++ b/lib/composer/autoload_classmap.php @@ -258,6 +258,7 @@ return array( 'IntervalOqlExpression' => $baseDir . '/core/oql/oqlquery.class.inc.php', 'Introspection' => $baseDir . '/core/introspection.class.inc.php', 'InvalidConfigParamException' => $baseDir . '/core/coreexception.class.inc.php', + 'InvalidPasswordAttributeOneWayPassword' => $baseDir . '/core/coreexception.class.inc.php', 'IssueLog' => $baseDir . '/core/log.class.inc.php', 'ItopCounter' => $baseDir . '/core/counter.class.inc.php', 'JSButtonItem' => $baseDir . '/application/applicationextension.inc.php', diff --git a/lib/composer/autoload_static.php b/lib/composer/autoload_static.php index fdf6ba4d9..fdba44cab 100644 --- a/lib/composer/autoload_static.php +++ b/lib/composer/autoload_static.php @@ -490,6 +490,7 @@ class ComposerStaticInit0018331147de7601e7552f7da8e3bb8b 'IntervalOqlExpression' => __DIR__ . '/../..' . '/core/oql/oqlquery.class.inc.php', 'Introspection' => __DIR__ . '/../..' . '/core/introspection.class.inc.php', 'InvalidConfigParamException' => __DIR__ . '/../..' . '/core/coreexception.class.inc.php', + 'InvalidPasswordAttributeOneWayPassword' => __DIR__ . '/../..' . '/core/coreexception.class.inc.php', 'IssueLog' => __DIR__ . '/../..' . '/core/log.class.inc.php', 'ItopCounter' => __DIR__ . '/../..' . '/core/counter.class.inc.php', 'JSButtonItem' => __DIR__ . '/../..' . '/application/applicationextension.inc.php', diff --git a/templates/login/password/changepwdform.html.twig b/templates/login/password/changepwdform.html.twig index 95f03f85b..9676b1828 100644 --- a/templates/login/password/changepwdform.html.twig +++ b/templates/login/password/changepwdform.html.twig @@ -7,7 +7,11 @@

{{ 'UI:Login:ChangeYourPassword'|dict_s }}

{% if bFailedLogin %} -

{{ 'UI:Login:IncorrectOldPassword'|dict_s }}

+ {% if sIssue is not null %} +

{{ sIssue|raw }}

+ {% else %} +

{{ 'UI:Login:IncorrectOldPassword'|dict_s }}

+ {% endif %} {% endif %}
diff --git a/test/coreExtensions/UserLocalTest.php b/test/coreExtensions/UserLocalTest.php new file mode 100644 index 000000000..2f06fdb7b --- /dev/null +++ b/test/coreExtensions/UserLocalTest.php @@ -0,0 +1,245 @@ +createMock(\Config::class); + + $configMock + ->method('GetModuleSetting') + ->willReturnMap($aValueMap); + + /** @var UserLocal $oUserLocal */ + $oUserLocal = \MetaModel::NewObject('UserLocal', array('login' => 'john')); + /** @var \ormLinkSet $oProfileSet */ + $oProfileSet = $oUserLocal->Get('profile_list'); + + $oProfileSet->AddItem( + \MetaModel::NewObject('URP_UserProfile', array('profileid' => 1)) + ); + + $oUserLocal->ValidatePassword($sPassword, $configMock); + + list($bCheckStatus, $aCheckIssues, $aSecurityIssues) = $oUserLocal->CheckToWrite(); + + $this->assertSame($bExpectedCheckStatus, $bCheckStatus); + + if (isset($expectedCheckIssues)) + { + $this->assertContains($expectedCheckIssues, $aCheckIssues); + } + } + + public function ProviderValidatePassword() + { + return array( + 'validPattern' => array( + 'valueMap' => array( + array('authent-local', 'password_validation.pattern', null, '.{1,10}') + ), + 'password' => 'foo', + 'expectedCheckStatus' => true, + ), + 'notValidPattern' => array( + 'valueMap' => array( + array('authent-local', 'password_validation.pattern', null, '.{6,10}') + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + ), + 'validClass' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserLocalPasswordPolicyMock' => array( + 'bCheckStatus' => true, + ) + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => true, + ), + 'notValidClass' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserLocalPasswordPolicyMock' => array( + 'bCheckStatus' => false, + ) + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + ), + + + 'UserPasswordPolicyRegex_configured_twice' => array( + 'valueMap' => array( + array('authent-local', 'password_validation.pattern', null, '.*'), + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserPasswordPolicyRegex' => array( + 'pattern' => '.*', + ) + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + 'expectedCheckIssues' => 'Invalid configuration: \'UserPasswordPolicyRegex\' was defined twice (once into UserLocal.password_validation_advanced, once into UserLocal.password_validation).', + ), + + 'classNotImplementsUserLocalPasswordValidator' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'StdClass' => array() + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + 'expectedCheckIssues' => 'Invalid configuration: \'StdClass\' must implements UserLocalPasswordValidator', + ), + + + 'validation_composition_10' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserLocalPasswordPolicyMock' => array( + 'bCheckStatus' => true, + 'sCheckIssues' => 'UserLocalPasswordPolicyMock', + ), + + 'UserLocalPasswordPolicyMockBis' => array( + 'bCheckStatus' => false, + 'sCheckIssues' => 'UserLocalPasswordPolicyMockBis', + ), + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + 'expectedCheckIssues' => 'UserLocalPasswordPolicyMockBis', + ), + + + 'validation_composition_01' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserLocalPasswordPolicyMock' => array( + 'bCheckStatus' => false, + 'sCheckIssues' => 'UserLocalPasswordPolicyMock', + ), + + 'UserLocalPasswordPolicyMockBis' => array( + 'bCheckStatus' => true, + 'sCheckIssues' => 'UserLocalPasswordPolicyMockBis', + ), + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + 'expectedCheckIssues' => 'UserLocalPasswordPolicyMock', + ), + + 'validation_composition_11' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserLocalPasswordPolicyMock' => array( + 'bCheckStatus' => true, + 'sCheckIssues' => 'UserLocalPasswordPolicyMock', + ), + + 'UserLocalPasswordPolicyMockBis' => array( + 'bCheckStatus' => true, + 'sCheckIssues' => 'UserLocalPasswordPolicyMockBis', + ), + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => true, + ), + 'validation_composition_00' => array( + 'valueMap' => array( + array( + 'authent-local', + 'password_validation.classes', + null, + array( + 'UserLocalPasswordPolicyMock' => array( + 'bCheckStatus' => false, + 'sCheckIssues' => 'UserLocalPasswordPolicyMock', + ), + + 'UserLocalPasswordPolicyMockBis' => array( + 'bCheckStatus' => false, + 'sCheckIssues' => 'UserLocalPasswordPolicyMockBis', + ), + ) + ) + ), + 'password' => 'foo', + 'expectedCheckStatus' => false, + 'expectedCheckIssues' => 'UserLocalPasswordPolicyMock', + ), + + ); + } + +} + diff --git a/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php b/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php new file mode 100644 index 000000000..065ccf50b --- /dev/null +++ b/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php @@ -0,0 +1,28 @@ + status + + coreExtensions +