N°6171 - Password Expiration: can expire mode has no effect on user who have never changed their password - finalize work

datamodels/2.x/authent-local/model.authent-local.php

@@ -3,7 +3,7 @@
 //
 //   This file is part of iTop.
 //
-//   iTop is free software; you can redistribute it and/or modify	
+//   iTop is free software; you can redistribute it and/or modify
 //   it under the terms of the GNU Affero General Public License as published by
 //   the Free Software Foundation, either version 3 of the License, or
 //   (at your option) any later version.
@@ -69,7 +69,7 @@ class UserLocal extends UserInternal
 	const EXPIRE_NEVER = 'never_expire';
 	const EXPIRE_FORCE = 'force_expire';
 	const EXPIRE_ONE_TIME_PWD = 'otp_expire';
-	
+
 	/** @var UserLocalPasswordValidity|null */
 	protected $m_oPasswordValidity = null;
 
@@ -160,7 +160,7 @@ class UserLocal extends UserInternal
 
 	/**
 	 * Use with care!
-	 */	 	
+	 */
 	public function SetPassword($sNewPassword)
 	{
 		$this->Set('password', $sNewPassword);
@@ -197,19 +197,39 @@ class UserLocal extends UserInternal
 
 	protected function OnWrite()
 	{
-		if (empty($this->m_oPasswordValidity))
-		{
-			return;
-		}
-
 		if (array_key_exists('password_renewed_date', $this->ListChanges()))
 		{
 			return;
 		}
 
+		if (empty($this->m_oPasswordValidity))
+		{
+			//password unchanged
+			if (is_null($this->Get('password_renewed_date')))
+			{
+				//initialize password_renewed_date with User creation date
+				$sKey = $this->GetKey();
+$sOql = <<<OQL
+SELECT CMDBChangeOpCreate AS ccc
+JOIN CMDBChange AS c ON ccc.change = c.id
+WHERE ccc.objclass="UserLocal" AND ccc.objkey="$sKey"
+OQL;
+					$oCmdbChangeOpSearch = \DBObjectSearch::FromOQL($sOql);
+					$oSet = new \DBObjectSet($oCmdbChangeOpSearch);
+					$oCMDBChangeOpCreate = $oSet->Fetch();
+					if (! is_null($oCMDBChangeOpCreate))
+					{
+						$oUserCreationDateTime = \DateTime::createFromFormat(AttributeDateTime::GetInternalFormat(), $oCMDBChangeOpCreate->Get('date'));
+						$sCreationDate = $oUserCreationDateTime->format(\AttributeDate::GetInternalFormat());
+						$this->Set('password_renewed_date', $sCreationDate);
+					}
+			}
+			return;
+		}
+
 		$sNow = date(\AttributeDate::GetInternalFormat());
 		$this->Set('password_renewed_date', $sNow);
-	
+
 		// Reset the "force" expiration flag when the user updates her/his own password!
 		if ($this->IsCurrentUser())
 		{
@@ -294,7 +314,7 @@ class UserLocal extends UserInternal
 		{
 			$this->m_aCheckIssues[] = $this->m_oPasswordValidity->getPasswordValidityMessage();
 		}
-		
+
 		// A User cannot force a one-time password on herself/himself
 		if ($this->IsCurrentUser()) {
 			if (array_key_exists('expiration', $this->ListChanges()) && ($this->Get('expiration') == self::EXPIRE_ONE_TIME_PWD)) {

tests/php-unit-tests/unitary-tests/datamodels/2.x/authent-local/UserLocalTest.php

@@ -8,7 +8,13 @@
 namespace Combodo\iTop\Test\UnitTest\Module\AuthentLocal;
 
 
+use AttributeDate;
 use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
+use Config;
+use Dict;
+use MetaModel;
+use ormLinkSet;
+use URP_UserProfile;
 use UserLocal;
 
 /**
@@ -34,23 +40,23 @@ class UserLocalTest extends ItopDataTestCase
 	 */
 	public function testValidatePassword($sPassword, $aValidatorNames, $aConfigValueMap, $bExpectedCheckStatus, $expectedCheckIssues = null, $sUserLanguage = null)
 	{
-		$configMock = $this->createMock(\Config::class);
+		$configMock = $this->createMock(Config::class);
 		$configMock
 			->method('GetModuleSetting')
 			->willReturnMap($aConfigValueMap);
 		restore_error_handler();
 
 		if (isset($sUserLanguage)) {
-			\Dict::SetUserLanguage($sUserLanguage);
+			Dict::SetUserLanguage($sUserLanguage);
 		}
 
 		/** @var UserLocal $oUserLocal */
-		$oUserLocal = \MetaModel::NewObject('UserLocal', array('login' => 'john'));
-		/** @var \ormLinkSet $oProfileSet */
+		$oUserLocal = MetaModel::NewObject(UserLocal::class, array('login' => 'john'));
+		/** @var ormLinkSet $oProfileSet */
 		$oProfileSet = $oUserLocal->Get('profile_list');
 
 		$oProfileSet->AddItem(
-			\MetaModel::NewObject('URP_UserProfile', array('profileid' => 1))
+			MetaModel::NewObject(URP_UserProfile::class, array('profileid' => 1))
 		);
 
 		$aValidatorCollection = array();
@@ -242,9 +248,10 @@ class UserLocalTest extends ItopDataTestCase
 	 */
 	public function testPasswordRenewal($sBefore, $sExpectedAfter)
 	{
-		$oBefore = is_null($sBefore) ? null : date(\AttributeDate::GetInternalFormat(), strtotime($sBefore));
-		$oNow = date(\AttributeDate::GetInternalFormat());
-		$oExpectedAfter = is_null($sExpectedAfter) ? null : date(\AttributeDate::GetInternalFormat(), strtotime($sExpectedAfter));
+		$sDateFormat = AttributeDate::GetInternalFormat();
+		$oBefore = is_null($sBefore) ? null : date($sDateFormat, strtotime($sBefore));
+		$oNow = date($sDateFormat);
+		$oExpectedAfter = is_null($sExpectedAfter) ? null : date($sDateFormat, strtotime($sExpectedAfter));
 
 		$aUserLocalValues = array('login' => 'john');
 		if (!is_null($oBefore))
@@ -253,15 +260,14 @@ class UserLocalTest extends ItopDataTestCase
 		}
 
 		/** @var UserLocal $oUserLocal */
-		$oUserLocal = \MetaModel::NewObject('UserLocal', $aUserLocalValues);
-		/** @var \ormLinkSet $oProfileSet */
+		$oUserLocal = MetaModel::NewObject(UserLocal::class, $aUserLocalValues);
+		/** @var ormLinkSet $oProfileSet */
 		$oProfileSet = $oUserLocal->Get('profile_list');
 
 		$oProfileSet->AddItem(
-			\MetaModel::NewObject('URP_UserProfile', array('profileid' => 1))
+			MetaModel::NewObject(URP_UserProfile::class, array('profileid' => 1))
 		);
 
-
 		$this->assertEquals($oBefore, $oUserLocal->Get('password_renewed_date'));
 
 		//INSERT
@@ -270,17 +276,19 @@ class UserLocalTest extends ItopDataTestCase
 		$this->assertEquals($oNow, $oUserLocal->Get('password_renewed_date'), 'INSERT sets the "password_renewed_date" to the current date');
 
 		//UPDATE password_renewed_date
+		$oUserLocal = MetaModel::GetObject(UserLocal::class, $oUserLocal->GetKey());
 		$oUserLocal->Set('password_renewed_date', $oBefore);
 		$oUserLocal->DBWrite();
 		$this->assertEquals($oBefore, $oUserLocal->Get('password_renewed_date'), 'UPDATE can target and change the "password_renewed_date"');
 
 		//UPDATE password
+		$oUserLocal = MetaModel::GetObject(UserLocal::class, $oUserLocal->GetKey());
 		$oUserLocal->Set('password', 'fooBar1???1');
 		$oUserLocal->DBWrite();
 		$this->assertEquals($oExpectedAfter, $oUserLocal->Get('password_renewed_date'), 'UPDATE "password" fields trigger automatic change of the  "password_renewed_date" field');
 
-
 		//UPDATE both password & password_renewed_date
+		$oUserLocal = MetaModel::GetObject(UserLocal::class, $oUserLocal->GetKey());
 		$oUserLocal->Set('password', 'fooBar1???2');
 		$oUserLocal->Set('password_renewed_date', $oBefore);
 		$oUserLocal->DBWrite();
@@ -304,5 +312,85 @@ class UserLocalTest extends ItopDataTestCase
 			),
 		);
 	}
+
+	/**
+	 * @dataProvider CanExpireFixProvider
+	 *
+	 */
+	public function testCanExpireFix($sExpirationMode, $sBefore, bool $bRenewedDateTouched)
+	{
+		$oBefore = is_null($sBefore) ? null : date(AttributeDate::GetInternalFormat(), strtotime($sBefore));
+		$oNow = date(AttributeDate::GetInternalFormat());
+		$oExpectedAfter = $bRenewedDateTouched ? $oNow : $oBefore;
+
+		$aUserLocalValues = array('login' => 'john');
+		if (!is_null($oBefore))
+		{
+			$aUserLocalValues['password_renewed_date'] = $oBefore;
+		}
+
+		/** @var UserLocal $oUserLocal */
+		$oUserLocal = MetaModel::NewObject(UserLocal::class, $aUserLocalValues);
+		/** @var ormLinkSet $oProfileSet */
+		$oProfileSet = $oUserLocal->Get('profile_list');
+
+		$oProfileSet->AddItem(
+			MetaModel::NewObject(URP_UserProfile::class, array('profileid' => 1))
+		);
+
+		$this->assertEquals($oBefore, $oUserLocal->Get('password_renewed_date'));
+
+		//INSERT
+		$oUserLocal->Set('password', 'fooBar1???');
+		$oUserLocal->DBWrite();
+		$this->assertEquals($oNow, $oUserLocal->Get('password_renewed_date'), 'INSERT sets the "password_renewed_date" to the current date');
+
+		$oUserLocal = MetaModel::GetObject(UserLocal::class, $oUserLocal->GetKey());
+		$oUserLocal->Set('password_renewed_date', $oBefore);
+		$oUserLocal->DBWrite();
+		$this->assertEquals($oBefore, $oUserLocal->Get('password_renewed_date'), 'UPDATE can target and change the "password_renewed_date"');
+
+		//UPDATE password
+		$oUserLocal = MetaModel::GetObject(UserLocal::class, $oUserLocal->GetKey());
+		$oUserLocal->Set('expiration', $sExpirationMode);
+		$oUserLocal->DBWrite();
+		$this->assertEquals($oExpectedAfter, $oUserLocal->Get('password_renewed_date'), 'UPDATE "password" fields trigger automatic change of the  "password_renewed_date" field');
+	}
+
+	public function CanExpireFixProvider()
+	{
+		return array(
+			'EXPIRE_CAN: nominal case' => array(
+				'sExpirationMode' => 'can_expire',
+				'oExpectedBefore' => null,
+				'bRenewedDateTouched' => true,
+			),
+			'EXPIRE_NEVER (default mode): nothing changed on UserLocal' => array(
+				'sExpirationMode' => 'never_expire',
+				'oExpectedBefore' => null,
+				'bRenewedDateTouched' => false,
+			),
+			'EXPIRE_FORCE: nominal case' => array(
+				'sExpirationMode' => 'force_expire',
+				'oExpectedBefore' => null,
+				'bRenewedDateTouched' => true,
+			),
+			'EXPIRE_ONE_TIME_PWD: nominal case' => array(
+				'sExpirationMode' => 'otp_expire',
+				'oExpectedBefore' => null,
+				'bRenewedDateTouched' => true,
+			),
+			'date initiated' => array(
+				'sExpirationMode' => 'can_expire',
+				'oBefore' => '-1 day',
+				'bRenewedDateTouched' => false,
+			),
+			'date initiated in the future' => array(
+				'sExpirationMode' => 'can_expire',
+				'oBefore' => '+1 day',
+				'bRenewedDateTouched' => false,
+			),
+		);
+	}
 }