From fe13ec68fdbb29e4f6b14207ecda6b9b11eef4ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=B4me=20Chilliet?= <come@opensides.be>
Date: Tue, 21 May 2019 10:05:23 +0200
Subject: [PATCH] :ambulance: fix(core) Fix locking password in user template

Also cleaned up a bit password methods

issue #5990
---
 .../class_password-methods-clear.inc          | 15 ++---
 .../class_password-methods-crypt.inc          | 11 ++--
 .../class_password-methods-empty.inc          | 19 ++----
 .../class_password-methods-md5.inc            |  9 ++-
 .../class_password-methods-sasl.inc           | 13 ++--
 .../class_password-methods-sha.inc            | 16 ++---
 .../class_password-methods-smd5.inc           |  9 ++-
 .../class_password-methods-ssha.inc           | 12 ++--
 .../class_password-methods.inc                | 61 ++++++++++++-------
 .../generic/class_UserPasswordAttribute.inc   |  2 +-
 10 files changed, 93 insertions(+), 74 deletions(-)

diff --git a/include/password-methods/class_password-methods-clear.inc b/include/password-methods/class_password-methods-clear.inc
index cdfecd172..833629e74 100644
--- a/include/password-methods/class_password-methods-clear.inc
+++ b/include/password-methods/class_password-methods-clear.inc
@@ -42,22 +42,15 @@ class passwordMethodClear extends passwordMethod
   {
   }
 
-  /*!
-   * \brief Is available
-   *
-   * \return TRUE
-   */
-  function is_available ()
-  {
-    return TRUE;
-  }
-
   /*!
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     return $pwd;
   }
diff --git a/include/password-methods/class_password-methods-crypt.inc b/include/password-methods/class_password-methods-crypt.inc
index 39456bac6..0a45644bc 100644
--- a/include/password-methods/class_password-methods-crypt.inc
+++ b/include/password-methods/class_password-methods-crypt.inc
@@ -44,7 +44,7 @@ class passwordMethodCrypt extends passwordMethod
    *
    * \return TRUE if is avaibable, otherwise return false
    */
-  function is_available ()
+  public function is_available (): bool
   {
     return function_exists('crypt');
   }
@@ -53,8 +53,11 @@ class passwordMethodCrypt extends passwordMethod
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     if ($this->hash == "crypt/standard-des") {
       $salt = "";
@@ -105,7 +108,7 @@ class passwordMethodCrypt extends passwordMethod
     return '{CRYPT}'.($locked ? '!' : '').crypt($pwd, $salt);
   }
 
-  function checkPassword ($pwd, $hash)
+  function checkPassword ($pwd, $hash): bool
   {
     // Not implemented
     return FALSE;
@@ -151,7 +154,7 @@ class passwordMethodCrypt extends passwordMethod
    *
    * \param string $password_hash
    */
-  static function _extract_method ($password_hash)
+  static function _extract_method ($password_hash): string
   {
     if (!preg_match('/^{crypt}/i', $password_hash)) {
       return "";
diff --git a/include/password-methods/class_password-methods-empty.inc b/include/password-methods/class_password-methods-empty.inc
index bc75c17d0..3db1cbd49 100644
--- a/include/password-methods/class_password-methods-empty.inc
+++ b/include/password-methods/class_password-methods-empty.inc
@@ -43,22 +43,15 @@ class passwordMethodEmpty extends passwordMethod
   {
   }
 
-  /*!
-   * \brief Is available
-   *
-   * \return TRUE
-   */
-  function is_available ()
-  {
-    return TRUE;
-  }
-
   /*!
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     return ($locked ? static::LOCKVALUE : '');
   }
@@ -76,12 +69,12 @@ class passwordMethodEmpty extends passwordMethod
    *
    * \return boolean FALSE
    */
-  function need_password ()
+  function need_password (): bool
   {
     return FALSE;
   }
 
-  static function _extract_method ($password_hash)
+  static function _extract_method ($password_hash): string
   {
     if (empty($password_hash) || ($password_hash == static::LOCKVALUE)) {
       return static::get_hash_name();
diff --git a/include/password-methods/class_password-methods-md5.inc b/include/password-methods/class_password-methods-md5.inc
index bb910db53..10342e240 100644
--- a/include/password-methods/class_password-methods-md5.inc
+++ b/include/password-methods/class_password-methods-md5.inc
@@ -42,9 +42,9 @@ class passwordMethodMd5 extends passwordMethod
   /*!
    * \brief Is available
    *
-   * \return TRUE if is avaibable, otherwise return false
+   * \return TRUE if is available, otherwise return false
    */
-  function is_available ()
+  public function is_available (): bool
   {
     return function_exists('md5');
   }
@@ -53,8 +53,11 @@ class passwordMethodMd5 extends passwordMethod
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     return  '{MD5}'.($locked ? '!' : '').base64_encode(pack('H*', md5($pwd)));
   }
diff --git a/include/password-methods/class_password-methods-sasl.inc b/include/password-methods/class_password-methods-sasl.inc
index 7069578b9..da84a7b5c 100644
--- a/include/password-methods/class_password-methods-sasl.inc
+++ b/include/password-methods/class_password-methods-sasl.inc
@@ -72,7 +72,7 @@ class passwordMethodsasl extends passwordMethod
    *
    * \return TRUE if is avaibable
    */
-  function is_available ()
+  public function is_available (): bool
   {
     if (empty($this->realm) && empty($this->exop)) {
       return FALSE;
@@ -84,8 +84,11 @@ class passwordMethodsasl extends passwordMethod
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     if (empty($this->exop)) {
       if (empty($this->realm)) {
@@ -98,7 +101,7 @@ class passwordMethodsasl extends passwordMethod
     }
   }
 
-  function checkPassword ($pwd, $hash)
+  function checkPassword ($pwd, $hash): bool
   {
     // We do not store passwords, can’t know if they’re the same
     return FALSE;
@@ -115,9 +118,9 @@ class passwordMethodsasl extends passwordMethod
   /*!
    * \brief Password needed
    *
-   * \return boolean FALSE
+   * \return boolean
    */
-  function need_password ()
+  function need_password (): bool
   {
     global $config;
     return ($config->get_cfg_value('forceSaslPasswordAsk', 'FALSE') == 'TRUE');
diff --git a/include/password-methods/class_password-methods-sha.inc b/include/password-methods/class_password-methods-sha.inc
index f08802694..35080de8e 100644
--- a/include/password-methods/class_password-methods-sha.inc
+++ b/include/password-methods/class_password-methods-sha.inc
@@ -43,7 +43,7 @@ class passwordMethodsha extends passwordMethod
    *
    * \return TRUE if is avaibable, otherwise return false
    */
-  function is_available ()
+  public function is_available (): bool
   {
     return (function_exists('sha1') || function_exists('mhash'));
   }
@@ -51,17 +51,19 @@ class passwordMethodsha extends passwordMethod
   /*!
    * \brief Generate template hash
    *
-   * \param string $password Password
+   * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($password, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     if (function_exists('sha1')) {
-      $hash = '{SHA}'.($locked ? '!' : '').base64_encode(pack('H*', sha1($password)));
+      $hash = '{SHA}'.($locked ? '!' : '').base64_encode(pack('H*', sha1($pwd)));
     } elseif (function_exists('mhash')) {
-      $hash = '{SHA}'.($locked ? '!' : '').base64_encode(mHash(MHASH_SHA1, $password));
+      $hash = '{SHA}'.($locked ? '!' : '').base64_encode(mHash(MHASH_SHA1, $pwd));
     } else {
-      msg_dialog::display(_('Configuration error'), msgPool::missingext('mhash'), ERROR_DIALOG);
-      return FALSE;
+      throw new FusionDirectoryException(msgPool::missingext('mhash'));
     }
 
     return $hash;
diff --git a/include/password-methods/class_password-methods-smd5.inc b/include/password-methods/class_password-methods-smd5.inc
index f367d8a21..acb26ac30 100644
--- a/include/password-methods/class_password-methods-smd5.inc
+++ b/include/password-methods/class_password-methods-smd5.inc
@@ -43,7 +43,7 @@ class passwordMethodsmd5 extends passwordMethod
    *
    * \return TRUE if is avaibable, otherwise return false
    */
-  function is_available ()
+  public function is_available (): bool
   {
     return function_exists('md5');
   }
@@ -52,15 +52,18 @@ class passwordMethodsmd5 extends passwordMethod
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     $salt0  = substr(pack('h*', md5(random_int(0, PHP_INT_MAX))), 0, 8);
     $salt   = substr(pack('H*', md5($salt0 . $pwd)), 0, 4);
     return '{SMD5}'.($locked ? '!' : '').base64_encode(pack('H*', md5($pwd . $salt)) . $salt);
   }
 
-  function checkPassword ($pwd, $hash)
+  function checkPassword ($pwd, $hash): bool
   {
     $hash = base64_decode(substr($hash, 6));
     $salt = substr($hash, 16);
diff --git a/include/password-methods/class_password-methods-ssha.inc b/include/password-methods/class_password-methods-ssha.inc
index 0bf55370c..5c9e16d80 100644
--- a/include/password-methods/class_password-methods-ssha.inc
+++ b/include/password-methods/class_password-methods-ssha.inc
@@ -43,7 +43,7 @@ class passwordMethodssha extends passwordMethod
    *
    * \return TRUE if is avaibable, otherwise return false
    */
-  function is_available ()
+  public function is_available (): bool
   {
     return (function_exists('sha1') || function_exists('mhash'));
   }
@@ -52,8 +52,11 @@ class passwordMethodssha extends passwordMethod
    * \brief Generate template hash
    *
    * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
    */
-  function generate_hash ($pwd, $locked = FALSE)
+  public function generate_hash (string $pwd, bool $locked = FALSE): string
   {
     if (function_exists('sha1')) {
       $salt = substr(pack('h*', md5(random_int(0, PHP_INT_MAX))), 0, 8);
@@ -63,13 +66,12 @@ class passwordMethodssha extends passwordMethod
       $salt = mhash_keygen_s2k(MHASH_SHA1, $pwd, substr(pack('h*', md5(random_int(0, PHP_INT_MAX))), 0, 8), 4);
       $pwd  = '{SSHA}'.($locked ? '!' : '').base64_encode(mhash(MHASH_SHA1, $pwd.$salt).$salt);
     } else {
-      msg_dialog::display(_('Configuration error'), msgPool::missingext('mhash'), ERROR_DIALOG);
-      return FALSE;
+      throw new FusionDirectoryException(msgPool::missingext('mhash'));
     }
     return $pwd;
   }
 
-  function checkPassword ($pwd, $hash)
+  function checkPassword ($pwd, $hash): bool
   {
     $hash = base64_decode(substr($hash, 6));
     $salt = substr($hash, 20);
diff --git a/include/password-methods/class_password-methods.inc b/include/password-methods/class_password-methods.inc
index 20948d380..95e261f42 100644
--- a/include/password-methods/class_password-methods.inc
+++ b/include/password-methods/class_password-methods.inc
@@ -27,7 +27,7 @@
 /*!
  * \brief This class contains all the basic function for password methods
  */
-class passwordMethod
+abstract class passwordMethod
 {
   var $display  = FALSE;
   var $hash     = '';
@@ -47,9 +47,26 @@ class passwordMethod
   /*!
    * \brief Get the Hash name
    */
-  static function get_hash_name ()
+  abstract static function get_hash_name ();
+
+  /*!
+   * \brief Generate template hash
+   *
+   * \param string $pwd Password
+   * \param bool $locked Should the password be locked
+   *
+   * \return string the password hash
+   */
+  abstract public function generate_hash (string $pwd, bool $locked = FALSE): string;
+
+  /*!
+   * \brief Is available
+   *
+   * \return TRUE
+   */
+  public function is_available (): bool
   {
-    trigger_error("get_hash_name can't be called on main class");
+    return TRUE;
   }
 
   /*!
@@ -57,7 +74,7 @@ class passwordMethod
    *
    * \return boolean TRUE
    */
-  function need_password ()
+  public function need_password (): bool
   {
     return TRUE;
   }
@@ -67,7 +84,7 @@ class passwordMethod
    *
    * \return boolean
    */
-  function is_lockable ()
+  public function is_lockable (): bool
   {
     return $this->lockable;
   }
@@ -77,7 +94,7 @@ class passwordMethod
    *
    * \param string $dn The DN
    */
-  function is_locked ($dn = '', $pwd = '')
+  function is_locked ($dn = '', $pwd = ''): bool
   {
     global $config;
     if (!$this->lockable) {
@@ -105,7 +122,7 @@ class passwordMethod
    *
    * \param string $dn
    */
-  function lock_account ($dn = "")
+  function lock_account ($dn = '')
   {
     return $this->generic_modify_account($dn, 'LOCK');
   }
@@ -114,7 +131,7 @@ class passwordMethod
    * \brief Unlocks an account which was locked by 'lock_account()'.
    *        For details about the locking mechanism see 'lock_account()'.
    */
-  function unlock_account ($dn = "")
+  function unlock_account ($dn = '')
   {
     return $this->generic_modify_account($dn, 'UNLOCK');
   }
@@ -123,7 +140,7 @@ class passwordMethod
    * \brief Unlocks an account which was locked by 'lock_account()'.
    *        For details about the locking mechanism see 'lock_account()'.
    */
-  private function generic_modify_account ($dn, $mode)
+  private function generic_modify_account ($dn, string $mode)
   {
     global $config;
     if (!$this->lockable) {
@@ -203,7 +220,7 @@ class passwordMethod
   /*!
    * \brief This function returns all loaded classes for password encryption
    */
-  static function get_available_methods ()
+  static function get_available_methods (): array
   {
     global $class_mapping;
     $ret  = FALSE;
@@ -247,15 +264,15 @@ class passwordMethod
   /*!
    * \brief Get desciption
    */
-  function get_description ()
+  function get_description (): string
   {
-    return "";
+    return '';
   }
 
   /*!
    * \brief Method to check if a password matches a hash
    */
-  function checkPassword ($pwd, $hash)
+  function checkPassword ($pwd, $hash): bool
   {
     return ($hash == $this->generate_hash($pwd));
   }
@@ -264,7 +281,7 @@ class passwordMethod
   /*!
    * \brief Return true if this password method provides a configuration dialog
    */
-  function is_configurable ()
+  function is_configurable (): bool
   {
     return FALSE;
   }
@@ -272,9 +289,9 @@ class passwordMethod
   /*!
    * \brief Provide a subdialog to configure a password method
    */
-  function configure ()
+  function configure (): string
   {
-    return "";
+    return '';
   }
 
 
@@ -295,7 +312,7 @@ class passwordMethod
    *
    * \param string $dn The DN
    */
-  static function get_method ($password_hash, $dn = "")
+  static function get_method ($password_hash, $dn = ''): passwordMethod
   {
     $methods = passwordMethod::get_available_methods();
 
@@ -325,14 +342,14 @@ class passwordMethod
    *
    * \param string $password_hash
    */
-  static function _extract_method ($password_hash)
+  static function _extract_method ($password_hash): string
   {
     $hash = static::get_hash_name();
     if (preg_match("/^\{$hash\}/i", $password_hash)) {
       return $hash;
     }
 
-    return "";
+    return '';
   }
 
   /*!
@@ -342,7 +359,7 @@ class passwordMethod
    *
    * \param string $hash
    */
-  static function make_hash ($password, $hash)
+  static function make_hash ($password, $hash): string
   {
     $methods  = passwordMethod::get_available_methods();
     $tmp      = new $methods[$hash]();
@@ -376,10 +393,10 @@ class passwordMethod
    *
    * \param string $password The password
    */
-  static function is_harmless ($password)
+  static function is_harmless ($password): bool
   {
     global $config;
-    if ($config->get_cfg_value("strictPasswordRules") == "TRUE") {
+    if ($config->get_cfg_value('strictPasswordRules') == 'TRUE') {
       // Do we have UTF8 characters in the password?
       return ($password == utf8_decode($password));
     }
diff --git a/plugins/personal/generic/class_UserPasswordAttribute.inc b/plugins/personal/generic/class_UserPasswordAttribute.inc
index 4de38fb16..0a0956325 100644
--- a/plugins/personal/generic/class_UserPasswordAttribute.inc
+++ b/plugins/personal/generic/class_UserPasswordAttribute.inc
@@ -219,7 +219,7 @@ class UserPasswordAttribute extends CompositeAttribute
     $test = new $temp[$values[0]]($this->plugin->dn, $this->plugin);
     $test->set_hash($values[0]);
     if ($this->plugin->is_template) {
-      return $test->generate_hash($values[1], $values[4]).'|'.$values[1];
+      return $test->generate_hash($values[1], ($values[4] == 'TRUE')).'|'.$values[1];
     } else {
       return $test->generate_hash($values[1]);
     }
-- 
GitLab