n°524 - password policy

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;
 				}

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
+{
+}

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

datamodels/2.x/authent-local/datamodel.authent-local.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<itop_design xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="1.7">
+
+    <module_parameters>
+        <parameters id="authent-local" _delta="define">
+            <password_validation.pattern>^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^\da-zA-Z]).{8,15}$</password_validation.pattern>
+            <password_validation.classes></password_validation.classes>
+        </parameters>
+    </module_parameters>
+</itop_design>

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',
 ));

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.',
 ));

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
+		);
+	}
+}
+

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!
 		),
 	)
 );

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',

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',

templates/login/password/changepwdform.html.twig

@@ -7,7 +7,11 @@
 	<div id="login-content">
 		<h1>{{ 'UI:Login:ChangeYourPassword'|dict_s }}</h1>
 		{% if bFailedLogin %}
-			<p class="hilite">{{ 'UI:Login:IncorrectOldPassword'|dict_s }}</p>
+			{% if sIssue is not null %}
+				<p class="hilite">{{ sIssue|raw }}</p>
+			{% else %}
+				<p class="hilite">{{ 'UI:Login:IncorrectOldPassword'|dict_s }}</p>
+			{% endif %}
 		{% endif %}
 		<form method="post">
 			<div id="login-form-content">

test/coreExtensions/UserLocalTest.php

@@ -0,0 +1,245 @@
+<?php
+/**
+ * Created by Bruno DA SILVA, working for Combodo
+ * Date: 21/11/2019
+ * Time: 09:14
+ */
+
+namespace coreExtensions;
+
+
+use Combodo\iTop\Test\UnitTest\ItopTestCase;
+use UserLocal;
+use UserLocalPasswordValidity;
+
+class UserLocalTest extends ItopTestCase
+{
+
+	public function setUp()
+	{
+
+		parent::setUp(); // TODO: Change the autogenerated stub
+
+		require_once(APPROOT.'application/startup.inc.php');
+		require_once (APPROOT.'test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php');
+	}
+
+	/**
+	 * @dataProvider ProviderValidatePassword
+	 */
+	public function testValidatePassword($aValueMap, $sPassword, $bExpectedCheckStatus, $expectedCheckIssues = null)
+	{
+		$configMock = $this->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',
+			),
+
+		);
+	}
+
+}
+

test/coreExtensions/UserLocalTest/UserLocalPasswordPolicyMock.php

@@ -0,0 +1,28 @@
+<?php
+class UserLocalPasswordPolicyMock implements \UserLocalPasswordValidator
+{
+
+	public function __construct()
+	{
+	}
+
+	/**
+	 * @param string $proposedValue
+	 * @param array $aOptions
+	 * @param UserLocal $oUserLocal
+	 *
+	 * @return UserLocalPasswordValidity
+	 */
+	public function ValidatePassword($proposedValue, $aOptions, UserLocal $oUserLocal)
+	{
+		$message = (isset($aOptions['sCheckIssues'])) ? $aOptions['sCheckIssues'] : 'UserLocalPasswordPolicyMock error message';
+
+		return new UserLocalPasswordValidity($aOptions['bCheckStatus'], $message);
+	}
+}
+
+class UserLocalPasswordPolicyMockBis extends UserLocalPasswordPolicyMock
+{
+
+
+}

test/phpunit.xml.dist

@@ -56,6 +56,9 @@
         <testsuite name="Status">
             <directory>status</directory>
         </testsuite>
+        <testsuite name="CoreExtensions">
+            <directory>coreExtensions</directory>
+        </testsuite>
     </testsuites>
 
     <!-- Code coverage white list -->