Merge branch 'pu/je/admin_user_must_change_pw' into 'main'

tweak(Admin): improve "must change password" handling See merge request tine20/tine20!6218
tine-groupware · Nov 25, 2024 · ce919c0 · ce919c0
2 parents 65f96b8 + 6583ac6
commit ce919c0
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 30 deletions.
diff --git a/tests/tine20/Tinebase/User/SqlTest.php b/tests/tine20/Tinebase/User/SqlTest.php
@@ -425,7 +425,7 @@ public function testPasswordMustChange()
         self::assertNull($user->mustChangePassword());
 
         // change days config to 10
-        Tinebase_Config::getInstance()->set(Tinebase_Config::PASSWORD_POLICY_CHANGE_AFTER, 10);
+        Tinebase_Config::getInstance()->set(Tinebase_Config::USER_PASSWORD_POLICY, [Tinebase_Config::PASSWORD_POLICY_CHANGE_AFTER => 10]);
 
         // set password change: 11 days ago)
         $now = Tinebase_DateTime::now();

diff --git a/tine20/Admin/Frontend/Json.php b/tine20/Admin/Frontend/Json.php
@@ -194,6 +194,8 @@ public function getUser($id)
     {
         if (!empty($id)) {
             $user = Admin_Controller_User::getInstance()->get($id);
+            $user->must_change_password = $user->getMustChangePassword();
+
             $userArray = $this->_recordToJson($user);
 
             // don't send some infos to the client: unset email uid+gid
@@ -223,7 +225,7 @@ public function getUser($id)
                 );
                 $userRoles = array();
             }
-            
+
 
         } else {
             $userArray = array('accountStatus' => 'enabled', 'visibility' => 'displayed');
@@ -271,6 +273,10 @@ public function getUser($id)
     public function getUsers($filter, $sort, $dir, $start, $limit)
     {
         $accounts = Admin_Controller_User::getInstance()->searchFullUsers($filter, $sort, $dir, $start, $limit);
+        /* @var Tinebase_Model_FullUser $account */
+        foreach ($accounts as $account) {
+            $account->must_change_password = $account->getMustChangePassword();
+        }
         $results = array();
         foreach ($this->_multipleRecordsToJson($accounts) as $val) {
             $val['filesystemSize'] = null;

diff --git a/tine20/Admin/js/user/EditDialog.js b/tine20/Admin/js/user/EditDialog.js
@@ -143,22 +143,21 @@ Tine.Admin.UserEditDialog = Ext.extend(Tine.widgets.dialog.EditDialog, {
      * @private
      */
     mustChangePasswordCheck: function () {
-        const changeAfter = Tine.Tinebase.configManager.get('userPwPolicy.pwPolicyChangeAfter', 'Tinebase')
-        if (!changeAfter || changeAfter === 0) return
-
-        const lastChangeDate = this.record.get('accountLastPasswordChangeRaw')
-
-        const maxDate = new Date(lastChangeDate).add('d', changeAfter)
-        if (maxDate > new Date()) return
+        const must_change_password = this.record.get('must_change_password')
+        if (!must_change_password || must_change_password !== 'expired') return
 
         const lastChangeField = this.getForm().findField('accountLastPasswordChange')
         const mustChangeField = this.getForm().findField('password_must_change')
 
         lastChangeField.addClass('tinebase-warning')
-        mustChangeField.disable()
 
+        // store real value of the flag, so it is not overwritten later
         this.record.set('password_must_change_actual', this.record.get('password_must_change'))
-        this.record.set('password_must_change', 1)
+
+        // password must be changed no matter whether the flag is set
+        this.record.set('password_must_change', true)
+        mustChangeField.disable()
+
         this.mustChangeTriggerPlugin.setVisible(true)
     },
 

diff --git a/tine20/Admin/js/user/GridPanel.js b/tine20/Admin/js/user/GridPanel.js
@@ -202,7 +202,7 @@ Tine.Admin.user.GridPanel = Ext.extend(Tine.widgets.grid.GridPanel, {
             { header: this.app.i18n._('Last login at'), id: 'accountLastLogin', hidden: this.isLdapBackend, renderer: Tine.Tinebase.common.dateTimeRenderer},
             { header: this.app.i18n._('Last login from'), id: 'accountLastLoginfrom', width: 120, hidden: this.isLdapBackend},
             { header: this.app.i18n._('MFA Configured'), id: 'mfa_configs', renderer: this.mfaRenderer.createDelegate(this), hidden: false},
-            { header: this.app.i18n._('Password Must Change'), id: 'password_must_change', renderer: this.mustChangeRenderer, hidden: false},
+            { header: this.app.i18n._('Password Must Change'), id: 'must_change_password', renderer: this.mustChangeRenderer, hidden: false, sortable: false},
             { header: this.app.i18n._('Password changed'), id: 'accountLastPasswordChange', renderer: this.dateTimeRenderer, hidden: false},
             { header: this.app.i18n._('Expires'), id: 'accountExpires', renderer: Tine.Tinebase.common.dateTimeRenderer, hidden: false},
             { header: this.app.i18n._('Full Name'), id: 'accountFullName'}
@@ -232,25 +232,17 @@ Tine.Admin.user.GridPanel = Ext.extend(Tine.widgets.grid.GridPanel, {
         return rendered
     },
 
-    mustChangeRenderer: function (_value, metadata, _record) {
-        let changeAfter = Tine.Tinebase.configManager.get('userPwPolicy.pwPolicyChangeAfter', 'Tinebase')
-        if (!changeAfter || changeAfter === 0) return Tine.Tinebase.common.booleanRenderer(_value)
-
-        let hoverText = i18n._('Password is expired in accordance with the password policy and needs to be changed')
-
-        let lastChangeDate = _record.get('accountLastPasswordChange')
-        if (!lastChangeDate) {
+    mustChangeRenderer: function (_value, metadata) {
+        if (_value === 'expired') {
+            const hoverText = i18n._('Password is expired in accordance with the password policy and needs to be changed')
             metadata.cellAttr = 'title="'+hoverText+'"'
-            return Tine.Tinebase.common.booleanRenderer('yes')
+        } else {
+            metadata.cellAttr = ''
         }
-
-        let maxDate = new Date(lastChangeDate).add('d', changeAfter)
-        if (maxDate < new Date()) {
-            metadata.cellAttr = 'title="'+hoverText+'"'
-            return Tine.Tinebase.common.booleanRenderer('yes')
+        if (_value !== null) {
+            return Tine.Tinebase.common.booleanRenderer('1')
         }
-
-        return Tine.Tinebase.common.booleanRenderer(_value)
+        return Tine.Tinebase.common.booleanRenderer('0')
     },
 
     enableDisableButtonHandler: function(status) {

diff --git a/tine20/Admin/js/user/Users.js b/tine20/Admin/js/user/Users.js
@@ -67,7 +67,8 @@ Tine.Admin.Model.UserArray = [
     { name: 'effectiveAndLocalQuota'},
     { name: 'mfa_configs'},
     { name: 'xprops'},
-    { name: 'type'}
+    { name: 'type'},
+    { name: 'must_change_password'} // dynamically calculated value, not a persisted property
 ];
 
 Tine.Admin.Model.User = Tine.Tinebase.data.Record.create(Tine.Tinebase.Model.genericFields.concat(Tine.Admin.Model.UserArray), {

diff --git a/tine20/Tinebase/Model/FullUser.php b/tine20/Tinebase/Model/FullUser.php
@@ -59,6 +59,11 @@ class Tinebase_Model_FullUser extends Tinebase_Model_User
     public const USER_TYPE_USER = 'user';
     public const USER_TYPE_VOLUNTEER = 'volunteer';
 
+    public const PASSWORD_EXPIRED = 'expired';
+    public const PASSWORD_FLAGGED = 'flagged';
+    public const PASSWORD_NEVER_CHANGED = 'neverChanged';
+
+
     /**
      * holds the configuration object (must be declared in the concrete class)
      *
@@ -251,6 +256,12 @@ class Tinebase_Model_FullUser extends Tinebase_Model_User
                 self::TYPE          => self::TYPE_VIRTUAL,
                 self::VALIDATORS    => [Zend_Filter_Input::ALLOW_EMPTY => true],
             ),
+            'must_change_password'  => [
+                self::TYPE              => self::TYPE_STRING,
+                self::NULLABLE          => true,
+                self::IS_VIRTUAL        => true,
+                self::VALIDATORS        => [Zend_Filter_Input::ALLOW_EMPTY => true],
+            ]
         ],
     ];
 
@@ -421,7 +432,7 @@ protected function _sqlPasswordChangeNeeded()
             return true;
         }
 
-        $passwordChangeDays = Tinebase_Config::getInstance()->get(Tinebase_Config::PASSWORD_POLICY_CHANGE_AFTER);
+        $passwordChangeDays = $this->getPasswordChangeDays();
 
         if ($passwordChangeDays > 0) {
             $now = Tinebase_DateTime::now();
@@ -512,4 +523,34 @@ public function getEmailUserId($type = self::XPROP_EMAIL_USERID_IMAP)
     {
         return Tinebase_EmailUser_XpropsFacade::getEmailUserId($this, $type);
     }
+
+    public function getMustChangePassword(): ?string
+    {
+        if (empty($this->accountLastPasswordChange)) {
+            return self::PASSWORD_NEVER_CHANGED;
+        }
+
+        $mustChange = null;
+        if ($this->password_must_change) {
+            $mustChange = self::PASSWORD_FLAGGED;
+        }
+
+        $passwordChangeDays = $this->getPasswordChangeDays();
+        if ($passwordChangeDays > 0) {
+            $now = Tinebase_DateTime::now();
+            $mustChange = $this->accountLastPasswordChange->isEarlier($now->subDay($passwordChangeDays)) ? self::PASSWORD_EXPIRED : $mustChange;
+        }
+        return $mustChange;
+    }
+
+    public function getPasswordChangeDays(): ?int
+    {
+        $passwordPolicy = Tinebase_Config::getInstance()->get(Tinebase_Config::USER_PASSWORD_POLICY);
+
+        $passwordChangeDays = 0;
+        if ($passwordPolicy) {
+            $passwordChangeDays = $passwordPolicy->get(Tinebase_Config::PASSWORD_POLICY_CHANGE_AFTER);
+        }
+        return $passwordChangeDays;
+    }
 }