-
Notifications
You must be signed in to change notification settings - Fork 359
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
base: dev
Are you sure you want to change the base?
Changes from 20 commits
fa64c72
e80e27b
a06804d
6afdc74
e61daf3
95926c5
f4e32f2
e977b33
03bff51
58a2f2f
9a8defa
f731cde
a85a525
fd7d5a7
44d8456
9800f5b
4183693
072d3fb
11e26a4
475f222
7646e33
0d5b9d9
eb7ea64
85cd191
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
{ | ||
$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 | ||
* | ||
|
@@ -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'); | ||
} | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sure (without spending more time digging deeper into the code and the history) why we need to split updatePassword into two methods... but if this is unavoidable, I wonder if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure that updatePassword is able to do everything needed, so this method shouldn't be needed. |
||
{ | ||
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'], | ||
] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). | ||
|
@@ -329,8 +330,22 @@ public function storedCatalogLogin() | |
$this->ilsAccount[$username] = $patron; | ||
return $patron; | ||
} | ||
} elseif (!empty($userName)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this for? I don't think this should be needed for password recovery. |
||
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; | ||
} | ||
|
||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might it be possible to add a second optional argument to supportsPasswordRecovery instead of a whole new method here? Or is this really unavoidable? |
||
{ | ||
return ($this->config->Authentication->recover_password ?? false) | ||
&& $this->getAuth()->supportsPasswordRecovery($target); | ||
} | ||
|
||
/** | ||
* Is email changing currently allowed? | ||
* | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be necessary either. |
||
{ | ||
$user = $this->getAuth()->newPassword($request); | ||
$this->updateSession($user); | ||
return $user; | ||
} | ||
|
||
/** | ||
* Update a user's email from the request. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to leave the details inside MultiBackend and just pass enough information to it for it to be able to choose the correct driver. Something like:
Admittedly not exactly beautiful, and it's my intention make it possible to just indicate the target more elegantly. Somewhere in future. |
||
} | ||
return $targetDriver->supportsMethod('recoverPassword', []) ?? false; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -712,6 +712,14 @@ protected function setFollowupUrlToReferer(bool $allowCurrentUrl = true, array $ | |
if ($mrhuNorm === $refererNorm) { | ||
return; | ||
} | ||
// if is the stored current url of the lightbox | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand this comment, and |
||
// 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 | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||
Comment on lines
+1728
to
+1730
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another minor style tweak that you can commit directly from this comment:
Suggested change
|
||||||||||||||
if (!$user && $this->formWasSubmitted() && !empty($username)) { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||
$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 | ||||||||||||||
|
@@ -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; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you concatenate this to a string down below, I don't think you ever want it to be null -- should probably be empty string instead. Also, it's probably best to encode the value for safety. And it might make sense to refactor to avoid calling getQuery so many times, e.g.:
Then down below, you can use $targetParam in your concatenation, and you can assign $target to $view->target instead of fetching the value yet another time. |
||||||||||||||
// 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( | ||||||||||||||
|
@@ -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(); | ||||||||||||||
|
@@ -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 | ||||||||||||||
oharacj marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
$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'); | ||||||||||||||
} | ||||||||||||||
|
@@ -2238,9 +2265,9 @@ protected function setUpAuthenticationFromRequest() | |||||||||||||
$this->params()->fromPost('auth_method') | ||||||||||||||
) | ||||||||||||||
); | ||||||||||||||
|
||||||||||||||
if (!empty($method)) { | ||||||||||||||
$this->getAuthManager()->setAuthMethod($method); | ||||||||||||||
} | ||||||||||||||
} $this->getAuthManager()->setAuthMethod($method); | ||||||||||||||
demiankatz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a counterpart in MultiILS that supports checking the selected target. For inspiration:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.