From 1c09dee5e922c6048bf76db7857d0f4c02c23e56 Mon Sep 17 00:00:00 2001
From: Demian Katz <demian.katz@villanova.edu>
Date: Mon, 7 Jan 2013 10:30:16 -0500
Subject: [PATCH] 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 | 16 ++++++++-
module/VuFind/src/VuFind/Auth/ILS.php | 10 +++---
module/VuFind/src/VuFind/Auth/Manager.php | 6 ++--
module/VuFind/src/VuFind/Auth/SIP2.php | 6 +---
module/VuFind/src/VuFind/Db/Row/User.php | 37 ++++++++++++--------
module/VuFind/src/VuFind/ILS/Driver/PICA.php | 12 +++----
6 files changed, 52 insertions(+), 35 deletions(-)
diff --git a/module/VuFind/src/VuFind/Auth/Database.php b/module/VuFind/src/VuFind/Auth/Database.php
index 14293e7618e..144ac8dd18f 100644
--- a/module/VuFind/src/VuFind/Auth/Database.php
+++ b/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?
*
diff --git a/module/VuFind/src/VuFind/Auth/ILS.php b/module/VuFind/src/VuFind/Auth/ILS.php
index 79e5dc139b6..074b5013f05 100644
--- a/module/VuFind/src/VuFind/Auth/ILS.php
+++ b/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
diff --git a/module/VuFind/src/VuFind/Auth/Manager.php b/module/VuFind/src/VuFind/Auth/Manager.php
index 86ea1e4f109..00aee5b9737 100644
--- a/module/VuFind/src/VuFind/Auth/Manager.php
+++ b/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;
diff --git a/module/VuFind/src/VuFind/Auth/SIP2.php b/module/VuFind/src/VuFind/Auth/SIP2.php
index 36a761c23c1..931889bfeaf 100644
--- a/module/VuFind/src/VuFind/Auth/SIP2.php
+++ b/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
diff --git a/module/VuFind/src/VuFind/Db/Row/User.php b/module/VuFind/src/VuFind/Db/Row/User.php
index 14d5191e2cf..639e6e5750f 100644
--- a/module/VuFind/src/VuFind/Db/Row/User.php
+++ b/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.
*
diff --git a/module/VuFind/src/VuFind/ILS/Driver/PICA.php b/module/VuFind/src/VuFind/ILS/Driver/PICA.php
index 00776b7bb8a..22bcfbed233 100644
--- a/module/VuFind/src/VuFind/ILS/Driver/PICA.php
+++ b/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);
--
GitLab