Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable password recovery when using ILS Authentication #3997

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fa64c72
Initial changes to enable password recovery when using ILS Authentica…
oharacj Oct 8, 2024
e80e27b
Initial changes to enable password recovery when using ILS Authentica…
oharacj Oct 8, 2024
a06804d
Initial changes to enable password recovery when using ILS Authentica…
oharacj Oct 8, 2024
6afdc74
Remove the return clause in setFollowupUrlToReferer method.
oharacj Oct 8, 2024
e61daf3
Updated based on Demian's comments
oharacj Oct 15, 2024
95926c5
Add getPatronFromUsername to SierraRest driver.
oharacj Oct 15, 2024
f4e32f2
Add getPatronFromUsername to SierraRest driver.
oharacj Oct 15, 2024
e977b33
Add getPatronFromUsername to SierraRest driver.
oharacj Oct 15, 2024
03bff51
Add getPatronFromUsername to SierraRest driver.
oharacj Oct 15, 2024
58a2f2f
Multi-ILS Driver work and bug fixes
oharacj Dec 11, 2024
9a8defa
Fixed some encoding and comment issues
oharacj Dec 11, 2024
f731cde
removed comment block in .js file
oharacj Dec 11, 2024
a85a525
added displayILSPasswordRecoveryLink to the top exported comment
oharacj Dec 11, 2024
fd7d5a7
PHP 8.3 updates and space fixes
oharacj Dec 11, 2024
44d8456
PHP 8.3 updates and space fixes
oharacj Dec 11, 2024
9800f5b
PHP 8.3 double quotes in template
oharacj Dec 11, 2024
4183693
PHP 8.3 double quotes in template
oharacj Dec 11, 2024
072d3fb
Merge branch 'dev' into ilsPasswordRecovery
oharacj Dec 23, 2024
11e26a4
Remove hard-coded pin length in changePassword function
oharacj Dec 23, 2024
475f222
Update settings file to remove redundant setting.
oharacj Dec 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 72 additions & 1 deletion module/VuFind/src/VuFind/Auth/ILS.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,23 @@ public function authenticate($request)
return $this->handleLogin($username, $password, $loginMethod, $rememberMe);
}

/**
* Does this authentication method support password recovery
*
* @return bool
*/
public function supportsPasswordRecovery()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial work is only supposed to be for the ILS auth. MultiILS is something I'll look at in the future but I'm not sure it's part of the scope of this work. Is this piece a necessity? If so, I can start working on this and I'll resubmit when I have it done. I only have one ILS and I can't really test but I'm happy to look into it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could potentially use the Demo driver as a second ILS for testing purposes, if that helps! Let me know if you need more details/help to get that set up.

{
$driver = $this->getCatalog()->getDriver();
if (
method_exists($driver, 'recoverPassword')
&& ($this->config['Authentication']['recover_password'] ?? false)
) {
return true;
}
return false;
}

/**
* Does this authentication method support password changing
*
Expand Down Expand Up @@ -166,8 +183,13 @@ public function updatePassword($request)
foreach (['oldpwd', 'password', 'password2'] as $param) {
$params[$param] = $request->getPost()->get($param, '');
}

// Connect to catalog:
if ($hash = $request->getPost('hash') ?? false) {
if ($user = $this->getDbService(UserServiceInterface::class)->getUserByVerifyHash($hash)) {
$username = $user->getUsername();
}
}

if (!($patron = $this->authenticator->storedCatalogLogin())) {
throw new AuthException('authentication_error_technical');
}
Expand All @@ -178,6 +200,55 @@ public function updatePassword($request)
$result = $this->getCatalog()->changePassword(
[
'patron' => $patron,
'username' => $patron['cat_username'],
'oldPassword' => $params['oldpwd'],
'newPassword' => $params['password'],
]
);
if (!$result['success']) {
throw new AuthException($result['status']);
}

// Update the user and send it back to the caller:
$username = $patron[$this->getUsernameField()];
$user = $this->getOrCreateUserByUsername($username);
$this->authenticator->saveUserCatalogCredentials($user, $patron['cat_username'], $params['password']);
return $user;
}

/**
* Change/Reset a user's password when they've forgotten.
*
* @param Request $request Request object containing new account details.
*
* @return UserEntityInterface Updated user entity.
*
* @throws PasswordSecurity
* @throws AuthException
* @throws \Exception
*/
public function newPassword($request)
{
foreach (['oldpwd', 'password', 'password2'] as $param) {
$params[$param] = $request->getPost()->get($param, '');
}
// Connect to catalog:
if ($hash = $request->getPost('hash') ?? false) {
if ($user = $this->getDbService(UserServiceInterface::class)->getUserByVerifyHash($hash)) {
$username = $user->getUsername();
}
}
if (!($patron = $this->authenticator->storedCatalogLogin($username ?? null))) {
throw new AuthException('authentication_error_technical');
}
$this->validatePasswordUpdate($params);
if (empty($params['oldpwd'])) {
$params['oldpwd'] = $patron['cat_password'];
}
$result = $this->getCatalog()->recoverPassword(
[
'patron' => $patron,
'username' => $patron['cat_username'],
'oldPassword' => $params['oldpwd'],
'newPassword' => $params['password'],
]
Expand Down
27 changes: 21 additions & 6 deletions module/VuFind/src/VuFind/Auth/ILSAuthenticator.php
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,12 @@ public function getStoredCatalogCredentials()
* fails, clear the user's stored credentials so they can enter new, corrected
* ones.
*
* Returns associative array of patron data on success, false on failure.
* @param $userName - the username/barcode for ILS password reset
*
* @return array|bool
* @return array|bool Returns associative array of patron data on success,
* false on failure.
*/
public function storedCatalogLogin()
public function storedCatalogLogin($userName = null)
{
// Fail if no username is found, but allow a missing password (not every ILS
// requires a password to connect).
Expand All @@ -329,8 +330,22 @@ public function storedCatalogLogin()
$this->ilsAccount[$username] = $patron;
return $patron;
}
} elseif (!empty($userName)) {
if (isset($this->ilsAccount[$userName])) {
return $this->ilsAccount[$userName];
}
$user = $this->getDbService(UserServiceInterface::class)->getUserByUsername($userName);
if (empty($user)) {
return false;
}
$patron = $this->catalog->patronLogin($userName, $this->getCatPasswordForUser($user));
if (empty($patron)) {
$user->setCatUsername(null)->setRawCatPassword(null)->setCatPassEnc(null);
} else {
$this->ilsAccount[$userName] = $patron;
return $patron;
}
}

return false;
}

Expand All @@ -340,9 +355,9 @@ public function storedCatalogLogin()
* @param string $username Catalog username
* @param string $password Catalog password
*
* Returns associative array of patron data on success, false on failure.
* @return array|bool Returns associative array of patron data on success,
* false on failure.
*
* @return array|bool
* @throws ILSException
*/
public function newCatalogLogin($username, $password)
Expand Down
29 changes: 29 additions & 0 deletions module/VuFind/src/VuFind/Auth/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,19 @@ public function supportsRecovery($authMethod = null)
&& $this->getAuth($authMethod)->supportsPasswordRecovery();
}

/**
* Multi-ILS Only, Does the target Auth Method support password Recovery?
*
* @param $target string the target to check against
*
* @return bool
*/
public function multiSupportsPasswordRecovery($target)
{
return ($this->config->Authentication->recover_password ?? false)
&& $this->getAuth()->supportsPasswordRecovery($target);
}

/**
* Is email changing currently allowed?
*
Expand Down Expand Up @@ -670,6 +683,22 @@ public function updatePassword($request)
return $user;
}

/**
* Reset a user's password from the request.
*
* @param \Laminas\Http\PhpEnvironment\Request $request Request object containing
* password change details.
*
* @throws AuthException
* @return UserEntityInterface Updated user entity.
*/
public function newPassword($request)
{
$user = $this->getAuth()->newPassword($request);
$this->updateSession($user);
return $user;
}

/**
* Update a user's email from the request.
*
Expand Down
21 changes: 21 additions & 0 deletions module/VuFind/src/VuFind/Auth/MultiILS.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

use VuFind\Db\Entity\UserEntityInterface;
use VuFind\Exception\Auth as AuthException;
use VuFind\ILS\Connection;
use VuFind\ILS\Driver\MultiBackend;

use function in_array;
Expand Down Expand Up @@ -123,4 +124,24 @@ public function setCatalog(\VuFind\ILS\Connection $connection)
}
parent::setCatalog($connection);
}

/**
* Test to see if the target ILS supports password Recovery
*
* @param string $target The ILS we are checking
*
* @return bool
*
* @throws \Exception
*/
public function supportsPasswordRecovery($target = null)
{
$catalog = $this->getCatalog();
$driver = $catalog->getDriver();
$targetDriver = $driver;
if ($target != null) {
$targetDriver = $driver->getDriverFromTarget($target);
}
return $targetDriver->supportsMethod('recoverPassword', []) ?? false;
}
}
8 changes: 8 additions & 0 deletions module/VuFind/src/VuFind/Controller/AbstractBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,14 @@ protected function setFollowupUrlToReferer(bool $allowCurrentUrl = true, array $
if ($mrhuNorm === $refererNorm) {
return;
}
// if is the stored current url of the lightbox
// which overrides the url from the server request when present
$normReferer = $this->normalizeUrlForComparison($referer);
$myUserReset = $this->getServerUrl('myresearch-verify');
$murNorm = $this->normalizeUrlForComparison($myUserReset);
if (str_starts_with($normReferer, $murNorm)) {
return;
}

// If the referer is the MyResearch/UserLogin action, it probably means
// that the user is repeatedly mistyping their password. We should
Expand Down
41 changes: 34 additions & 7 deletions module/VuFind/src/VuFind/Controller/MyResearchController.php
Original file line number Diff line number Diff line change
Expand Up @@ -1725,6 +1725,23 @@ public function recoverAction()
} elseif ($username = $this->params()->fromPost('username')) {
$user = $userService->getUserByUsername($username);
}
//ILS Driver:
//if the user hasn't logged in yet, but is found by the ILS, call function
//getPatronFromUsername
if (!$user && $this->formWasSubmitted() && !empty($username)) {
$dbService = $this->getDbService(UserServiceInterface::class);
$entity = $dbService->createEntityForUsername($username);
$catalog = $this->getILS()->getDriver();
if ($catalog->supportsMethod('getPatronFromUsername', $username)) {
$patron = $catalog->getPatronFromUsername($username);
$entity->setEmail($patron['email']);
$entity->setCatPassEnc($patron['password']);
$entity->setFirstname($patron['firstname']);
$entity->setLastname($patron['lastname']);
$dbService->persistEntity($entity);
}
$user = $dbService->getUserByUsername($username);
}
$view = $this->createViewModel();
$view->useCaptcha = $this->captcha()->active('passwordRecovery');
// If we have a submitted form
Expand Down Expand Up @@ -1766,14 +1783,18 @@ protected function sendRecoveryEmail(UserEntityInterface $user, $config)
$config = $this->getConfig();
$renderer = $this->getViewRenderer();
$method = $this->getAuthManager()->getAuthMethod();
// If target exists create query string to include it as part of reset url
$target = $this->getRequest()->getQuery('target') ? '&target='
. $this->getRequest()->getQuery('target') : null;
// Custom template for emails (text-only)
$message = $renderer->render(
'Email/recover-password.phtml',
[
'library' => $config->Site->title,
'url' => $this->getServerUrl('myresearch-verify')
. '?hash='
. $user->getVerifyHash() . '&auth_method=' . $method,
. $user->getVerifyHash() . '&auth_method=' . $method
. $target,
]
);
$this->getService(Mailer::class)->send(
Expand Down Expand Up @@ -1938,6 +1959,7 @@ public function verifyAction()
$view->auth_method = $this->getAuthManager()->getAuthMethod();
$view->hash = $hash;
$view->username = $user->getUsername();
$view->target = $this->getRequest()->getQuery('target') ?? null;
$view->useCaptcha = $this->captcha()->active('changePassword');
$view->passwordPolicy = $this->getAuthManager()
->getPasswordPolicy();
Expand Down Expand Up @@ -2066,18 +2088,23 @@ public function newPasswordAction()
return $view;
}
}
// Update password
// Set/Reset password
try {
$user = $this->getAuthManager()->updatePassword($this->getRequest());
$user = $this->getAuthManager()->newPassword($this->getRequest());
} catch (AuthException $e) {
$this->flashMessenger()->addMessage($e->getMessage(), 'error');
return $view;
}
// Update hash to prevent reusing hash
$this->getAuthManager()->updateUserVerifyHash($user);
// Login
if ($followUp = $this->followup()->retrieve('url')) {
//This exists because the followupURL gets set to Verify which returns
//an error message due to trying to check the hash a second time
Comment on lines +2101 to +2102
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style suggestion -- add spaces after //:

Suggested change
//This exists because the followupURL gets set to Verify which returns
//an error message due to trying to check the hash a second time
// This exists because the followupURL gets set to Verify which returns
// an error message due to trying to check the hash a second time

$followUpUrl = str_contains($followUp, 'Verify') ? $this->url()->fromRoute('home') : $followUp;
$this->followup()->clear('url');
$this->followup()->store([], $followUpUrl);
}
$this->getAuthManager()->login($this->request);
// Return to account home
$this->flashMessenger()->addMessage('new_password_success', 'success');
return $this->redirect()->toRoute('myresearch-home');
}
Expand Down Expand Up @@ -2238,9 +2265,9 @@ protected function setUpAuthenticationFromRequest()
$this->params()->fromPost('auth_method')
)
);

if (!empty($method)) {
$this->getAuthManager()->setAuthMethod($method);
}
} $this->getAuthManager()->setAuthMethod($method);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a formatting problem here -- did this brace get moved up a line somehow?

}

/**
Expand Down
17 changes: 17 additions & 0 deletions module/VuFind/src/VuFind/ILS/Driver/Demo.php
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,23 @@ public function init()
$this->checkIntermittentFailure();
}

/**
* Helper method to determine whether or not a certain method can be
* called on this driver. Required method for any smart drivers.
*
* @param string $method The name of the called method.
* @param array $params Array of passed parameters
*
* @return bool True if the method can be called with the given parameters,
* false otherwise.
*
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
*/
public function supportsMethod($method, $params)
{
return is_callable([$this, $method]);
}

/**
* Check for a simulated failure. Returns true for failure, false for
* success.
Expand Down
12 changes: 12 additions & 0 deletions module/VuFind/src/VuFind/ILS/Driver/MultiBackend.php
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,18 @@ public function getLoginDrivers()
return $this->config['Login']['drivers'] ?? [];
}

/**
* Retrieve the name of the ILS when provided the alias
*
* @param $target string the alias of the driver
*
* @return mixed
*/
public function getDriverFromTarget($target)
{
return $this->getDriver($target);
}

/**
* Get default login driver
*
Expand Down
Loading
Loading