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 24 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
24 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
7646e33
updating indentation
oharacj Jan 8, 2025
0d5b9d9
update spacing in comment
oharacj Jan 8, 2025
eb7ea64
update spacing and indents
oharacj Jan 8, 2025
85cd191
update spacing and indents
oharacj Jan 8, 2025
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
25 changes: 24 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, 'changePassword')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest a new method for updating the password during recovery, since changePassword requires the patron from patronLogin method. Our custom code uses this:
https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/Driver/KohaRest.php#L792

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to use a resetPassword method for this. I've added my resetPassword to the SierraRest Driver.

&& ($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,7 @@ public function updatePassword($request)
$result = $this->getCatalog()->changePassword(
[
'patron' => $patron,
'username' => $patron['cat_username'],
'oldPassword' => $params['oldpwd'],
'newPassword' => $params['password'],
]
Expand Down
23 changes: 18 additions & 5 deletions module/VuFind/src/VuFind/Auth/ILSAuthenticator.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public function encrypt(?string $text)
*
* @param ?string $text The text to be encrypted or decrypted
* @param bool $encrypt True if we wish to encrypt text, False if we wish to
* decrypt text.
* decrypt text.
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
*
* @return ?string|bool The encrypted/decrypted string (null = empty input; false = error)
* @throws \VuFind\Exception\PasswordSecurity
Expand Down Expand Up @@ -309,11 +309,13 @@ 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 $user_name - the username/barcode for ILS password reset
Copy link
Member

Choose a reason for hiding this comment

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

Minor points of style: I'd suggest $username or $userName instead of $user_name -- we rarely use snake_case in our code, so the other options would be more consistent. Also, you don't need the "-" separator in this comment.

*
* Returns associative array of patron data on success, false on failure.
*
* @return array|bool
*/
public function storedCatalogLogin()
public function storedCatalogLogin($user_name = null)
{
// Fail if no username is found, but allow a missing password (not every ILS
// requires a password to connect).
Expand All @@ -336,8 +338,19 @@ public function storedCatalogLogin()
$this->ilsAccount[$username] = $patron;
return $patron;
}
} elseif (!empty($user_name)) {
$user = $this->getDbService(UserServiceInterface::class)->getUserByUsername($user_name);
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
if (isset($this->ilsAccount[$user_name])) {
return $this->ilsAccount[$user_name];
}
$patron = $this->catalog->patronLogin($user_name, $this->getCatPasswordForUser($user));
if (empty($patron)) {
$user->setCatUsername(null)->setRawCatPassword(null)->setCatPassEnc(null);
} else {
$this->ilsAccount[$user_name] = $patron;
return $patron;
}
}

return false;
}

Expand All @@ -347,7 +360,7 @@ public function storedCatalogLogin()
* @param string $username Catalog username
* @param string $password Catalog password
*
* Returns associative array of patron data on success, false on failure.
* Returns associative array of patron data on success, false on failure.
*
* @return array|bool
* @throws ILSException
Expand Down
57 changes: 53 additions & 4 deletions module/VuFind/src/VuFind/Controller/MyResearchController.php
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,35 @@ protected function processAuthenticationException(AuthException $e)
$this->flashMessenger()->addMessage($msg, 'error');
}

/**
* Store a referer (if appropriate) to keep post-login redirect pointing
* to an appropriate location. This is used when the user clicks the
* log in link from an arbitrary page or when a password is mistyped;
* separate logic is used for storing followup information when VuFind
* forces the user to log in from another context.
*
* @param bool $allowCurrentUrl Whether the current URL is valid for followup
* @param array $extras Extra data for the followup
*
* @return void
*/
protected function setFollowupUrlToReferer(bool $allowCurrentUrl = true, array $extras = [])
{
// lbreferer is the stored current url of the lightbox
// which overrides the url from the server request when present
$refer = $this->getRequest()->getQuery()->get(
'lbreferer',
$this->getRequest()->getServer()->get('HTTP_REFERER', null)
);
$normReferer = $this->normalizeUrlForComparison($refer);
$myUserReset = $this->getServerUrl('myresearch-verify');
$murNorm = $this->normalizeUrlForComparison($myUserReset);
if (str_starts_with($normReferer, $murNorm)) {
return;
}
parent::setFollowupUrlToReferer($allowCurrentUrl, $extras);
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Maintaining this method for backwards compatibility;
* logic moved to parent and method re-named
Expand Down Expand Up @@ -937,7 +966,7 @@ public function performDeleteFavorite($id, $source)
* @param UserEntityInterface $user Logged-in user
* @param \VuFind\RecordDriver\AbstractBase $driver Record driver for favorite
* @param int $listID List being edited (null
* if editing all favorites)
* if editing all favorites)
*
* @return object
*/
Expand Down Expand Up @@ -1729,6 +1758,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
//getPatronFromBarcode
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
if (!$user && $this->formWasSubmitted() && !empty($username)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this, as opposed to updatePassword, should be split to ILS-specific part. And because the ILS's have different functionality, we can't rely on always being able to fetch a user by username. It could also be email address.
It might also make sense to not even try to retrieve a full user in this situation, but just whatever information is needed when the password is reset. In some cases it could be that the ILS supports password reset by way of a recovery token instead of username, but e.g. for Koha we should just need patron id (borrowernumber).

$dbService = $this->getDbService(UserServiceInterface::class);
$entity = $dbService->createEntityForUsername($username);
$catalog = $this->getILS()->getDriver();
if ($catalog->supportsMethod('getPatronFromBarcode', $username)) {
$patron = $catalog->getPatronFromBarcode($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 @@ -1854,7 +1900,7 @@ protected function sendChangeNotificationEmail($user, $newEmail)
*
* @param ?UserEntityInterface $user User object we're recovering
* @param bool $change Is the user changing their email (true)
* or setting up a new account (false).
* or setting up a new account (false).
*
* @return void (sends email or adds error message)
*/
Expand Down Expand Up @@ -2079,9 +2125,12 @@ public function newPasswordAction()
}
// Update hash to prevent reusing hash
$this->getAuthManager()->updateUserVerifyHash($user);
// Login
if ($followUp = $this->followup()->retrieve('url')) {
$newUrl = strstr($followUp, 'Verify', true) . 'Home';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the purpose of this; maybe a comment is in order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment. Happy to answer any other questions about this or taking ANY suggestions. Image is before this code block.

image

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that makes sense, though I'm not sure I understand exactly what $followUpUrl = strstr($followUp, 'Verify', true) . 'Home'; is trying to do. I wonder if it might be better to do something like $followUpUrl = str_contains($followUp, 'Verify') ? $this->url()->fromRoute('home') : $followUp; (so we're using the router instead of string manipulation to pick the destination URL... but maybe I'm misunderstanding which target route is actually desired).

$this->followup()->clear('url');
$this->followup()->store([], $newUrl);
}
$this->getAuthManager()->login($this->request);
// Return to account home
$this->flashMessenger()->addMessage('new_password_success', 'success');
return $this->redirect()->toRoute('myresearch-home');
}
Expand Down
8 changes: 8 additions & 0 deletions themes/bootstrap3/templates/Auth/ILS/recovery.phtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div class="form-group">
<label class="control-label" for="recovery_username"><?=$this->transEsc('recovery_by_username') ?>:</label>
<input type="text" name="username" class="form-control" id="recovery_username" autocomplete="username">
</div>
<?=$this->captcha()->html($this->useCaptcha) ?>
<div class="form-group">
<input class="btn btn-primary" name="submitButton" type="submit" value="<?=$this->transEscAttr('Recover Account') ?>">
</div>
8 changes: 8 additions & 0 deletions themes/bootstrap5/templates/Auth/ILS/recovery.phtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div class="form-group">
<label class="control-label" for="recovery_username"><?=$this->transEsc('recovery_by_username') ?>:</label>
<input type="text" name="username" class="form-control" id="recovery_username" autocomplete="username">
</div>
<?=$this->captcha()->html($this->useCaptcha) ?>
<div class="form-group">
<input class="btn btn-primary" name="submitButton" type="submit" value="<?=$this->transEscAttr('Recover Account') ?>">
</div>
Loading