Laying groundwork for improved password security (VUFIND-340):

- Moved database-authentication-specific checkPassword method out of user row object and into auth object
- Eliminated direct access to cat_password property of user row object by using getters/setters

module/VuFind/src/VuFind/Auth/Database.php

@@ -66,7 +66,7 @@ class Database extends AbstractBase
 
         // Validate the credentials:
         $user = $this->getUserTable()->getByUsername($this->username, false);
-        if (!is_object($user) || !$user->checkPassword($this->password)) {
+        if (!is_object($user) || !$this->checkPassword($this->password, $user)) {
             throw new AuthException('authentication_error_invalid');
         }
 
@@ -139,6 +139,20 @@ class Database extends AbstractBase
         return $table->getByUsername($params['username'], false);
     }
 
+    /**
+     * Check that the user's password matches the provided value.
+     *
+     * @param string $password Password to check.
+     * @param object $userRow  The user row.
+     *
+     * @return bool
+     */
+    protected function checkPassword($input_password, $userRow)
+    {
+        return $input_password == $userRow->password;
+    }
+
+
     /**
      * Does this authentication method support account creation?
      *

module/VuFind/src/VuFind/Auth/ILS.php

@@ -137,16 +137,16 @@ class ILS extends AbstractBase
         // Update user information based on ILS data:
         $user->firstname = !isset($info['firstname']) ? " " : $info['firstname'];
         $user->lastname = !isset($info['lastname']) ? " " : $info['lastname'];
-        $user->cat_username = !isset($info['cat_username'])
-            ? " " : $info['cat_username'];
-        $user->cat_password = !isset($info['cat_password'])
-            ? " " : $info['cat_password'];
         $user->email = !isset($info['email']) ? " " : $info['email'];
         $user->major = !isset($info['major']) ? " " : $info['major'];
         $user->college = !isset($info['college']) ? " " : $info['college'];
 
         // Update the user in the database, then return it to the caller:
-        $user->save();
+        $user->saveCredentials(
+            !isset($info['cat_username']) ? " " : $info['cat_username'],
+            !isset($info['cat_password']) ? " " : $info['cat_password']
+        );
+
         return $user;
     }
 }
\ No newline at end of file

module/VuFind/src/VuFind/Auth/Manager.php

@@ -301,8 +301,7 @@ class Manager implements ServiceLocatorAwareInterface
         if ($user && isset($user->cat_username) && !empty($user->cat_username)) {
             try {
                 $patron = $catalog->patronLogin(
-                    $user->cat_username,
-                    isset($user->cat_password) ? $user->cat_password : null
+                    $user->cat_username, $user->getCatPassword()
                 );
             } catch (ILSException $e) {
                 $patron = null;
@@ -311,8 +310,7 @@ class Manager implements ServiceLocatorAwareInterface
                 // Problem logging in -- clear user credentials so they can be
                 // prompted again; perhaps their password has changed in the
                 // system!
-                $user->cat_username = null;
-                $user->cat_password = null;
+                $user->clearCredentials();
             } else {
                 $this->ilsAccount = $patron;    // cache for future use
                 return $patron;

module/VuFind/src/VuFind/Auth/SIP2.php

@@ -137,11 +137,7 @@ class SIP2 extends AbstractBase
         $user->lastname = trim(substr($ae, 0, strripos($ae, ',')));
         // I'm inserting the sip username and password since the ILS is the source.
         // Should revisit this.
-        $user->cat_username = $username;
-        $user->cat_password = $password;
-
-        // Update the user in the database, then return it to the caller:
-        $user->save();
+        $user->saveCredentials($username, $password);
         return $user;
     }
 }
\ No newline at end of file

module/VuFind/src/VuFind/Db/Row/User.php

@@ -86,6 +86,17 @@ class User extends ServiceLocatorAwareGateway
         return parent::save();
     }
 
+    /**
+     * Reset ILS login credentials.
+     *
+     * @return void
+     */
+    public function clearCredentials()
+    {
+        $this->cat_username = null;
+        $this->cat_password = null;
+    }
+
     /**
      * Save ILS login credentials.
      *
@@ -101,6 +112,17 @@ class User extends ServiceLocatorAwareGateway
         return $this->save();
     }
 
+    /**
+     *  This is a getter for the Catalog Password. It will return a plaintext version
+     *  of the password.
+     *
+     *  @return string The Catalog password in plain text
+     */
+    public function getCatPassword()
+    {
+        return isset($this->cat_password) ? $this->cat_password : null;
+    }
+
     /**
      * Change home library.
      *
@@ -254,20 +276,7 @@ class User extends ServiceLocatorAwareGateway
         return $table->getSavedData($resourceId, $source, $listId, $this->id);
     }
 
-    /**
-     * Check that the user's password matches the provided value.
-     *
-     * @param string $password Password to check.
-     *
-     * @return bool
-     */
-    public function checkPassword($password)
-    {
-        // Simple check for now since we store passwords in the database; we may
-        // eventually want to replace this with some kind of hash mechanism for
-        // better security.
-        return $this->password == $password;
-    }
+
     /**
      * Add/update a resource in the user's account.
      *

module/VuFind/src/VuFind/ILS/Driver/PICA.php

@@ -172,7 +172,7 @@ class PICA extends DAIA
             "ACT" => "UI_DATA",
             "LNG" => "DU",
             "BOR_U" => $_SESSION['picauser']->username,
-            "BOR_PW" => $_SESSION['picauser']->cat_password
+            "BOR_PW" => $_SESSION['picauser']->getCatPassword()
         );
         $postit = $this->postit($URL, $POST);
         // How many messages are there?
@@ -211,7 +211,7 @@ class PICA extends DAIA
             "ACT" => "UI_LOL",
             "LNG" => "DU",
             "BOR_U" => $_SESSION['picauser']->username,
-            "BOR_PW" => $_SESSION['picauser']->cat_password
+            "BOR_PW" => $_SESSION['picauser']->getCatPassword()
         );
         $postit = $this->postit($URL, $POST);
         // How many items are there?
@@ -384,7 +384,7 @@ class PICA extends DAIA
         $POST = array(
             "ACT" => "UI_RENEWLOAN",
             "BOR_U" => $_SESSION['picauser']->username,
-            "BOR_PW" => $_SESSION['picauser']->cat_password
+            "BOR_PW" => $_SESSION['picauser']->getCatPassword()
         );
         if (is_array($recordId) === true) {
             foreach ($recordId as $rid) {
@@ -417,7 +417,7 @@ class PICA extends DAIA
         $POST = array(
             "ACT" => "UI_LOC",
             "BOR_U" => $_SESSION['picauser']->username,
-            "BOR_PW" => $_SESSION['picauser']->cat_password
+            "BOR_PW" => $_SESSION['picauser']->getCatPassword()
         );
         $postit = $this->postit($URL, $POST);
 
@@ -483,7 +483,7 @@ class PICA extends DAIA
         $POST = array(
             "ACT" => "UI_LOR",
             "BOR_U" => $_SESSION['picauser']->username,
-            "BOR_PW" => $_SESSION['picauser']->cat_password
+            "BOR_PW" => $_SESSION['picauser']->getCatPassword()
         );
         $postit = $this->postit($URL, $POST);
 
@@ -549,7 +549,7 @@ class PICA extends DAIA
         $POST_LOL = array(
             "ACT" => "UI_LOL",
             "BOR_U" => $_SESSION['picauser']->username,
-            "BOR_PW" => $_SESSION['picauser']->cat_password
+            "BOR_PW" => $_SESSION['picauser']->getCatPassword()
         );
         $postit_lol = $this->postit($URL, $POST_LOL);