From 70dfbbc15e05b8905711fa22ae5ad51eb4ab2718 Mon Sep 17 00:00:00 2001 From: bruno DA SILVA Date: Mon, 25 Nov 2019 16:25:38 +0100 Subject: [PATCH] =?UTF-8?q?n=C2=B0524=20-=20password=20policy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The code now uses the standard extension method (using interfaces) - the metamodel can now filter on iModuleExtension in order to leverage extensions modularity (see MetaModel::EnumPlugins second param) - during the setup, there is no pawsord policy control - there is now a default policy - new (more precie) translation reflecting the default policy - fix CI? --- application/applicationextension.inc.php | 13 ++ core/metamodel.class.php | 26 ++- .../authent-local/datamodel.authent-local.xml | 2 +- .../authent-local/en.dict.authent-local.php | 2 +- .../authent-local/fr.dict.authent-local.php | 2 +- .../2.x/authent-local/model.authent-local.php | 77 ++----- lib/composer/autoload_classmap.php | 1 + lib/composer/autoload_static.php | 1 + setup/applicationinstaller.class.inc.php | 6 + test/coreExtensions/UserLocalTest.php | 209 ++++++------------ .../UserLocalPasswordPolicyMock.php | 23 +- 11 files changed, 148 insertions(+), 214 deletions(-) diff --git a/application/applicationextension.inc.php b/application/applicationextension.inc.php index 73b5738c1..c7bbef56b 100644 --- a/application/applicationextension.inc.php +++ b/application/applicationextension.inc.php @@ -1528,3 +1528,16 @@ class RestUtils return $oObject; } } + + +/** + * Helpers for modules extensibility, with discover performed by the MetaModel. + * + * + * @api + * @package Extensibility + */ +interface iModuleExtension +{ + public function __construct(); +} \ No newline at end of file diff --git a/core/metamodel.class.php b/core/metamodel.class.php index f0363f583..aeaa50268 100644 --- a/core/metamodel.class.php +++ b/core/metamodel.class.php @@ -2794,7 +2794,7 @@ abstract class MetaModel // Build the list of available extensions // - $aInterfaces = array('iApplicationUIExtension', 'iPreferencesExtension', 'iApplicationObjectExtension', 'iLoginFSMExtension', 'iLoginUIExtension', 'iLogoutExtension', 'iQueryModifier', 'iOnClassInitialization', 'iPopupMenuExtension', 'iPageUIExtension', 'iPortalUIExtension', 'ModuleHandlerApiInterface', 'iNewsroomProvider'); + $aInterfaces = array('iApplicationUIExtension', 'iPreferencesExtension', 'iApplicationObjectExtension', 'iLoginFSMExtension', 'iLoginUIExtension', 'iLogoutExtension', 'iQueryModifier', 'iOnClassInitialization', 'iPopupMenuExtension', 'iPageUIExtension', 'iPortalUIExtension', 'ModuleHandlerApiInterface', 'iNewsroomProvider', 'iModuleExtension'); foreach($aInterfaces as $sInterface) { self::$m_aExtensionClasses[$sInterface] = array(); @@ -7346,19 +7346,31 @@ abstract class MetaModel /** * @param string $sInterface + * @param string|null $sFilterInstanceOf [optional] if given, only instance of this string will be returned * * @return array classes=>instance implementing the given interface */ - public static function EnumPlugins($sInterface) + public static function EnumPlugins($sInterface, $sFilterInstanceOf = null) { - if (array_key_exists($sInterface, self::$m_aExtensionClasses)) - { - return self::$m_aExtensionClasses[$sInterface]; - } - else + if (!array_key_exists($sInterface, self::$m_aExtensionClasses)) { return array(); } + + if (is_null($sFilterInstanceOf)) + { + return self::$m_aExtensionClasses[$sInterface]; + } + + $fFilterCallback = function ($instance) use ($sFilterInstanceOf) + { + return $instance instanceof $sFilterInstanceOf; + }; + + return array_filter( + self::$m_aExtensionClasses[$sInterface], + $fFilterCallback + ); } /** diff --git a/datamodels/2.x/authent-local/datamodel.authent-local.xml b/datamodels/2.x/authent-local/datamodel.authent-local.xml index ab59d7c70..b55239c91 100644 --- a/datamodels/2.x/authent-local/datamodel.authent-local.xml +++ b/datamodels/2.x/authent-local/datamodel.authent-local.xml @@ -3,7 +3,7 @@ - ^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^\da-zA-Z]).{8,15}$ + ^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^\da-zA-Z]).{8,}$ 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 77941dafa..c243e0cc2 100755 --- a/datamodels/2.x/authent-local/en.dict.authent-local.php +++ b/datamodels/2.x/authent-local/en.dict.authent-local.php @@ -41,5 +41,5 @@ Dict::Add('EN US', 'English', 'English', array( 'Class:UserLocal/Attribute:password' => 'Password', 'Class:UserLocal/Attribute:password+' => 'user authentication string', - 'Error:UserLocalPasswordValidator:UserPasswordPolicyRegex/validation_failed' => 'The password does not respect the policy', + 'Error:UserLocalPasswordValidator:UserPasswordPolicyRegex/validation_failed' => 'Password must be at least 8 characters and include uppercase, lowercase, numeric and special characters.', )); 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 1dad2019f..851a92008 100755 --- a/datamodels/2.x/authent-local/fr.dict.authent-local.php +++ b/datamodels/2.x/authent-local/fr.dict.authent-local.php @@ -25,5 +25,5 @@ Dict::Add('FR FR', 'French', 'Français', array( '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.', + 'Error:UserLocalPasswordValidator:UserPasswordPolicyRegex/validation_failed' => 'Le mot de passe doit contenir au moins 8 caractères, avec minuscule, majuscule, nombre et caractère spécial.', )); diff --git a/datamodels/2.x/authent-local/model.authent-local.php b/datamodels/2.x/authent-local/model.authent-local.php index e6a52ae9a..0849ae22b 100755 --- a/datamodels/2.x/authent-local/model.authent-local.php +++ b/datamodels/2.x/authent-local/model.authent-local.php @@ -157,60 +157,41 @@ class UserLocal extends UserInternal public function IsPasswordValid() { + if (ContextTag::Check('Setup')) + { + // during the setup, the admin account can have whatever password you want ... + return true; + } + return (empty($this->m_oPasswordValidity)) || ($this->m_oPasswordValidity->isPasswordValid()); } + + /** - * set the $m_oPasswordValidity + * set the $m_oPasswordValidity based on UserLocalPasswordValidator instances vote. * * @param string $proposedValue - * @param \Config|null $config + * @param Config|null $config internal use (unit tests) + * @param null|UserLocalPasswordValidator[] $aValidatorCollection internal use (unit tests) * * @return void */ - public function ValidatePassword($proposedValue, $config = null) + public function ValidatePassword($proposedValue, $config = null, $aValidatorCollection = null) { if (null == $config) { $config = MetaModel::GetConfig(); } - $aPasswordValidationClasses = $config->GetModuleSetting('authent-local', 'password_validation.classes'); - if (empty($aPasswordValidationClasses)) + if (null == $aValidatorCollection) { - $aPasswordValidationClasses = array(); + $aValidatorCollection = MetaModel::EnumPlugins('iModuleExtension', 'UserLocalPasswordValidator'); } - $sUserPasswordPolicyRegexPattern = $config->GetModuleSetting('authent-local', 'password_validation.pattern'); - if ($sUserPasswordPolicyRegexPattern) + foreach ($aValidatorCollection as $oUserLocalPasswordValidator) { - 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); + $this->m_oPasswordValidity = $oUserLocalPasswordValidator->ValidatePassword($proposedValue, $this, $config); if (!$this->m_oPasswordValidity->isPasswordValid()) { @@ -258,18 +239,16 @@ class UserLocal extends UserInternal -interface UserLocalPasswordValidator +interface UserLocalPasswordValidator extends iModuleExtension { - public function __construct(); - /** * @param string $proposedValue - * @param array $aOptions * @param UserLocal $oUserLocal + * @param Config $config * * @return UserLocalPasswordValidity */ - public function ValidatePassword($proposedValue, $aOptions, UserLocal $oUserLocal); + public function ValidatePassword($proposedValue, UserLocal $oUserLocal, $config); } class UserPasswordPolicyRegex implements UserLocalPasswordValidator @@ -280,34 +259,27 @@ class UserPasswordPolicyRegex implements UserLocalPasswordValidator /** * @param string $proposedValue - * @param array $aOptions * @param UserLocal $oUserLocal + * @param Config $config * * @return UserLocalPasswordValidity */ - public function ValidatePassword($proposedValue, $aOptions, UserLocal $oUserLocal) + public function ValidatePassword($proposedValue, UserLocal $oUserLocal, $config) { + $sPattern = $config->GetModuleSetting('authent-local', 'password_validation.pattern'); - 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.' + 'Unknown error : Failed to check the password.' ); } @@ -324,4 +296,3 @@ class UserPasswordPolicyRegex implements UserLocalPasswordValidator ); } } - diff --git a/lib/composer/autoload_classmap.php b/lib/composer/autoload_classmap.php index 09f32e3a1..6c605b6ce 100644 --- a/lib/composer/autoload_classmap.php +++ b/lib/composer/autoload_classmap.php @@ -1921,6 +1921,7 @@ return array( 'iLoginUIExtension' => $baseDir . '/application/applicationextension.inc.php', 'iLogoutExtension' => $baseDir . '/application/applicationextension.inc.php', 'iMetricComputer' => $baseDir . '/core/computing.inc.php', + 'iModuleExtension' => $baseDir . '/application/applicationextension.inc.php', 'iNewsroomProvider' => $baseDir . '/application/newsroomprovider.class.inc.php', 'iOnClassInitialization' => $baseDir . '/core/metamodelmodifier.inc.php', 'iPageUIExtension' => $baseDir . '/application/applicationextension.inc.php', diff --git a/lib/composer/autoload_static.php b/lib/composer/autoload_static.php index fdba44cab..d3312109d 100644 --- a/lib/composer/autoload_static.php +++ b/lib/composer/autoload_static.php @@ -2153,6 +2153,7 @@ class ComposerStaticInit0018331147de7601e7552f7da8e3bb8b 'iLoginUIExtension' => __DIR__ . '/../..' . '/application/applicationextension.inc.php', 'iLogoutExtension' => __DIR__ . '/../..' . '/application/applicationextension.inc.php', 'iMetricComputer' => __DIR__ . '/../..' . '/core/computing.inc.php', + 'iModuleExtension' => __DIR__ . '/../..' . '/application/applicationextension.inc.php', 'iNewsroomProvider' => __DIR__ . '/../..' . '/application/newsroomprovider.class.inc.php', 'iOnClassInitialization' => __DIR__ . '/../..' . '/core/metamodelmodifier.inc.php', 'iPageUIExtension' => __DIR__ . '/../..' . '/application/applicationextension.inc.php', diff --git a/setup/applicationinstaller.class.inc.php b/setup/applicationinstaller.class.inc.php index b8fd2a280..f191fa7de 100644 --- a/setup/applicationinstaller.class.inc.php +++ b/setup/applicationinstaller.class.inc.php @@ -685,6 +685,7 @@ class ApplicationInstaller $oProductionEnv = new RunTimeEnvironment($sTargetEnvironment); $oProductionEnv->InitDataModel($oConfig, true); // load data model only + $oContextTag = new ContextTag('Setup'); // Migrate columns self::MoveColumns($sDBPrefix); @@ -877,6 +878,7 @@ class ApplicationInstaller $oProductionEnv = new RunTimeEnvironment($sTargetEnvironment); $oProductionEnv->InitDataModel($oConfig, true); // load data model and connect to the database + $oContextTag = new ContextTag('Setup'); self::$bMetaModelStarted = true; // No need to reload the final MetaModel in case the installer runs synchronously // Perform here additional DB setup... profiles, etc... @@ -943,6 +945,8 @@ class ApplicationInstaller if (!self::$bMetaModelStarted) { $oProductionEnv->InitDataModel($oConfig, false); // load data model and connect to the database + $oContextTag = new ContextTag('Setup'); + self::$bMetaModelStarted = true; // No need to reload the final MetaModel in case the installer runs synchronously } @@ -1014,6 +1018,8 @@ class ApplicationInstaller // Record which modules are installed... $oProductionEnv = new RunTimeEnvironment($sTargetEnvironment); $oProductionEnv->InitDataModel($oConfig, true); // load data model and connect to the database + $oContextTag = new ContextTag('Setup'); + if (!$oProductionEnv->RecordInstallation($oConfig, $sDataModelVersion, $aSelectedModuleCodes, $aSelectedExtensionCodes, $sInstallComment)) { throw new Exception("Failed to record the installation information"); diff --git a/test/coreExtensions/UserLocalTest.php b/test/coreExtensions/UserLocalTest.php index 9496b18f7..d138d531f 100644 --- a/test/coreExtensions/UserLocalTest.php +++ b/test/coreExtensions/UserLocalTest.php @@ -10,18 +10,23 @@ namespace coreExtensions; use Combodo\iTop\Test\UnitTest\ItopTestCase; use UserLocal; +use UserLocalPasswordPolicyMockNotValid; +use UserLocalPasswordPolicyMockNotValidBis; +use UserLocalPasswordPolicyMockValid; +use UserLocalPasswordPolicyMockValidBis; use UserLocalPasswordValidity; +use UserPasswordPolicyRegex; class UserLocalTest extends ItopTestCase { public function setUp() { - - parent::setUp(); // TODO: Change the autogenerated stub + parent::setUp(); require_once(APPROOT.'application/startup.inc.php'); require_once (APPROOT.'test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php'); + require_once (APPROOT.'env-production/authent-local/model.authent-local.php'); } /** @@ -31,13 +36,13 @@ class UserLocalTest extends ItopTestCase * @preserveGlobalState disabled * @backupGlobals disabled */ - public function testValidatePassword($aValueMap, $sPassword, $bExpectedCheckStatus, $expectedCheckIssues = null) + public function testValidatePassword($sPassword, $aValidatorCollection, $aConfigValueMap, $bExpectedCheckStatus, $expectedCheckIssues = null) { $configMock = $this->createMock(\Config::class); $configMock ->method('GetModuleSetting') - ->willReturnMap($aValueMap); + ->willReturnMap($aConfigValueMap); /** @var UserLocal $oUserLocal */ $oUserLocal = \MetaModel::NewObject('UserLocal', array('login' => 'john')); @@ -48,7 +53,7 @@ class UserLocalTest extends ItopTestCase \MetaModel::NewObject('URP_UserProfile', array('profileid' => 1)) ); - $oUserLocal->ValidatePassword($sPassword, $configMock); + $oUserLocal->ValidatePassword($sPassword, $configMock, $aValidatorCollection); list($bCheckStatus, $aCheckIssues, $aSecurityIssues) = $oUserLocal->CheckToWrite(); @@ -62,191 +67,107 @@ class UserLocalTest extends ItopTestCase public function ProviderValidatePassword() { + parent::setUp(); + require_once (APPROOT.'env-production/authent-local/model.authent-local.php'); + require_once (APPROOT.'test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php'); + + $oUserPasswordPolicyRegex = new UserPasswordPolicyRegex(); + + $oUserLocalPasswordPolicyMockValid = new UserLocalPasswordPolicyMockValid(); + $oUserLocalPasswordPolicyMockNotValid = new UserLocalPasswordPolicyMockNotValid(); + $oUserLocalPasswordPolicyMockValidBis = new UserLocalPasswordPolicyMockValidBis(); + $oUserLocalPasswordPolicyMockNotValidBis = new UserLocalPasswordPolicyMockNotValidBis(); + + return array( 'validPattern' => array( + 'password' => 'foo', + 'aValidatorCollection' => array( + $oUserPasswordPolicyRegex, + ), 'valueMap' => array( array('authent-local', 'password_validation.pattern', null, '.{1,10}') ), - 'password' => 'foo', 'expectedCheckStatus' => true, ), 'notValidPattern' => array( + 'password' => 'foo', + 'aValidatorCollection' => array( + $oUserPasswordPolicyRegex, + ), 'valueMap' => array( array('authent-local', 'password_validation.pattern', null, '.{6,10}') ), - 'password' => 'foo', 'expectedCheckStatus' => false, ), 'noPattern' => array( + 'password' => 'foo', + 'aValidatorCollection' => array( + $oUserPasswordPolicyRegex, + ), 'valueMap' => array( array('authent-local', 'password_validation.pattern', null, '') ), - 'password' => 'foo', 'expectedCheckStatus' => true, ), 'validClass' => array( - 'valueMap' => array( - array( - 'authent-local', - 'password_validation.classes', - null, - array( - 'UserLocalPasswordPolicyMock' => array( - 'bCheckStatus' => true, - ) - ) - ) - ), 'password' => 'foo', + 'aValidatorCollection' => array( + $oUserLocalPasswordPolicyMockValid, + ), + 'valueMap' => array(), 'expectedCheckStatus' => true, ), 'notValidClass' => array( - 'valueMap' => array( - array( - 'authent-local', - 'password_validation.classes', - null, - array( - 'UserLocalPasswordPolicyMock' => array( - 'bCheckStatus' => false, - ) - ) - ) - ), 'password' => 'foo', + 'aValidatorCollection' => array( + $oUserLocalPasswordPolicyMockNotValid, + ), + 'valueMap' => array(), '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', + 'aValidatorCollection' => array( + $oUserLocalPasswordPolicyMockValid, + $oUserLocalPasswordPolicyMockNotValid, + ), + 'valueMap' => array(), 'expectedCheckStatus' => false, - 'expectedCheckIssues' => 'UserLocalPasswordPolicyMockBis', + 'expectedCheckIssues' => 'UserLocalPasswordPolicyMockNotValid', ), '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', + 'aValidatorCollection' => array( + $oUserLocalPasswordPolicyMockNotValid, + $oUserLocalPasswordPolicyMockValid, + ), + 'valueMap' => array(), 'expectedCheckStatus' => false, - 'expectedCheckIssues' => 'UserLocalPasswordPolicyMock', + 'expectedCheckIssues' => 'UserLocalPasswordPolicyMockNotValid', ), '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', + 'aValidatorCollection' => array( + $oUserLocalPasswordPolicyMockValid, + $oUserLocalPasswordPolicyMockValidBis, + ), + 'valueMap' => array(), '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', + 'aValidatorCollection' => array( + $oUserLocalPasswordPolicyMockNotValid, + $oUserLocalPasswordPolicyMockNotValidBis, + ), + 'valueMap' => array(), 'expectedCheckStatus' => false, - 'expectedCheckIssues' => 'UserLocalPasswordPolicyMock', + 'expectedCheckIssues' => 'UserLocalPasswordPolicyMockNotValid', ), ); diff --git a/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php b/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php index 065ccf50b..8ee95cb70 100644 --- a/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php +++ b/test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php @@ -1,6 +1,8 @@