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 17 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
3 changes: 3 additions & 0 deletions config/vufind/SierraRest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ use_prefixed_ids = false
; used (redirect_uri above is not set).
username_field = "code"
password_field = "pin"
; Some library systems use a four digit access pin. Others prefer to have alpha numeric
; passwords. set digits_only to true for pin functionality. Default is false (alpha-num).
digits_only = true
Copy link
Member

Choose a reason for hiding this comment

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

There are higher-level password rules defined in config.ini. I wonder if it would be beneficial to make this setting more parallel with those (especially if we could refactor code to make the password validation logic reusable -- I haven't inspected to see how feasible this is, but it seems likely to be an option).

Alternatively, if there are really only two different options in Sierra, maybe it would be more clear to call this setting four_digit_pin (to account for both the length restriction and the content restriction), or to have a setting like password_mode = pin|password (if we want to allow for more possibilities in the future than a binary setting can provide.

Copy link
Member

Choose a reason for hiding this comment

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

Just checking to see if you have any thoughts on this point, since there hasn't been further discussion on it since October. (Also happy to hear @EreMaijala's thoughts, if any).

Copy link
Contributor

@EreMaijala EreMaijala Dec 16, 2024

Choose a reason for hiding this comment

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

I think we should use the same rules as when changing a password, i.e. the [changePassword] section in ILS driver's ini:

; Uncomment the following lines to enable password (PIN) change
;[changePassword]
; PIN change parameters. The default limits are taken from the interface documentation.
;minLength = 4
;maxLength = 4
; See the password_pattern/password_hint settings in the [Authentication] section
; of config.ini for notes on these settings. When set here, these will override the
; config.ini defaults when Sierra is used for authentication.
;pattern = "numeric"
;hint = "Your optional custom hint can go here."

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'm still not sure how I've managed to miss this section of the ini so many times. I had to have looked over it hundreds of times without seeing that there was a [changePassword] section. I'll remove the custom stuff I did but I'll also have to make a change to the changePassword function in the SierraRest driver in order to remove the hard-coded digits only section of password change.

; Authentication method to use with global patron data access. Default is "native".
; "ldap" is supported for REST API v6 and later (see api_version above).
; IMPORTANT: when using "native" authentication, the driver assumes by default that
Expand Down
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)
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 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 resetPassword would be a better name than newPassword since it's more verb-based. Also, if we make this change here, do we need to make parallel changes to VuFind\Auth\Database since it is currently implemented to do password resets using only updatePassword, and some of the other code changes here might conflict with that?

Copy link
Contributor

Choose a reason for hiding this comment

The 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'],
]
Expand Down
56 changes: 32 additions & 24 deletions module/VuFind/src/VuFind/Auth/ILSAuthenticator.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@

namespace VuFind\Auth;

use Laminas\Config\Config;
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
use Laminas\Crypt\BlockCipher;
use Laminas\Crypt\Symmetric\Openssl;
use Closure;
use VuFind\Config\Config;
use VuFind\Db\Entity\UserEntityInterface;
use VuFind\Db\Service\DbServiceAwareInterface;
use VuFind\Db\Service\DbServiceAwareTrait;
Expand All @@ -53,13 +52,6 @@ class ILSAuthenticator implements DbServiceAwareInterface
{
use DbServiceAwareTrait;

/**
* Callback for retrieving the authentication manager
*
* @var callable
*/
protected $authManagerCallback;

/**
* Authentication manager
*
Expand Down Expand Up @@ -91,18 +83,19 @@ class ILSAuthenticator implements DbServiceAwareInterface
/**
* Constructor
*
* @param callable $authCB Auth manager callback
* @param ILSConnection $catalog ILS connection
* @param ?EmailAuthenticator $emailAuthenticator Email authenticator
* @param ?Config $config Configuration from config.ini
* @param Closure $authManagerCallback Auth manager callback
* @param Closure $cipherFactory BlockCipher object factory (takes algorithm as argument)
* @param ILSConnection $catalog ILS connection
* @param ?EmailAuthenticator $emailAuthenticator Email authenticator
* @param ?Config $config Configuration from config.ini
*/
public function __construct(
callable $authCB,
protected Closure $authManagerCallback,
protected Closure $cipherFactory,
protected ILSConnection $catalog,
protected ?EmailAuthenticator $emailAuthenticator = null,
protected ?Config $config = null
) {
$this->authManagerCallback = $authCB;
}

/**
Expand Down Expand Up @@ -163,7 +156,7 @@ protected function encryptOrDecrypt(?string $text, bool $encrypt = true)
return null;
}

$configAuth = $this->config->Authentication ?? new \Laminas\Config\Config([]);
$configAuth = $this->config->Authentication ?? new Config([]);

// Load encryption key from configuration if not already present:
if ($this->encryptionKey === null) {
Expand All @@ -181,7 +174,7 @@ protected function encryptOrDecrypt(?string $text, bool $encrypt = true)

// Check if OpenSSL error is caused by blowfish support
try {
$cipher = new BlockCipher(new Openssl(['algorithm' => $algo]));
$cipher = ($this->cipherFactory)($algo);
if ($algo == 'blowfish') {
trigger_error(
'Deprecated encryption algorithm (blowfish) detected',
Expand Down Expand Up @@ -309,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 @@ -336,8 +330,22 @@ public function storedCatalogLogin()
$this->ilsAccount[$username] = $patron;
return $patron;
}
} elseif (!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.

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;
}

Expand All @@ -347,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
31 changes: 30 additions & 1 deletion module/VuFind/src/VuFind/Auth/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@

namespace VuFind\Auth;

use Laminas\Config\Config;
use Laminas\Session\SessionManager;
use LmcRbacMvc\Identity\IdentityInterface;
use VuFind\Config\Config;
use VuFind\Cookie\CookieManager;
use VuFind\Db\Entity\UserEntityInterface;
use VuFind\Db\Service\UserServiceInterface;
Expand Down 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)
Copy link
Member

Choose a reason for hiding this comment

The 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?
*
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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
*
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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

    public function ilsSupportsPasswordRecovery($target)
    {
        $catalog = $this->getCatalog();
        $recoveryConfig = $catalog->checkFunction(
            'recoverPassword',
            ['cat_username' => "$target.123"]
        );
        return $recoveryConfig ? true : false;
    }

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;
}
}
23 changes: 20 additions & 3 deletions module/VuFind/src/VuFind/Controller/AbstractBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use Laminas\ServiceManager\ServiceLocatorInterface;
use Laminas\Uri\Http;
use Laminas\View\Model\ViewModel;
use VuFind\Config\Feature\EmailSettingsTrait;
use VuFind\Controller\Feature\AccessPermissionInterface;
use VuFind\Db\Entity\UserEntityInterface;
use VuFind\Exception\Auth as AuthException;
Expand Down Expand Up @@ -77,6 +78,7 @@
*/
class AbstractBase extends AbstractActionController implements AccessPermissionInterface, TranslatorAwareInterface
{
use EmailSettingsTrait;
use GetServiceTrait;
use TranslatorAwareTrait;

Expand Down Expand Up @@ -266,7 +268,7 @@ protected function createEmailViewModel($params = null, $defaultSubject = null)
// Fail if we're missing a from and the form element is disabled:
if ($view->disableFrom) {
if (empty($view->from)) {
$view->from = $config->Site->email;
$view->from = $this->getEmailSenderAddress($config);
}
if (empty($view->from)) {
throw new \Exception('Unable to determine email from address');
Expand Down Expand Up @@ -449,7 +451,7 @@ protected function catalogLogin()
*
* @param string $id Configuration identifier (default = main VuFind config)
*
* @return \Laminas\Config\Config
* @return \VuFind\Config\Config
*/
public function getConfig($id = 'config')
{
Expand Down Expand Up @@ -710,6 +712,14 @@ protected function setFollowupUrlToReferer(bool $allowCurrentUrl = true, array $
if ($mrhuNorm === $refererNorm) {
return;
}
// if is the stored current url of the lightbox
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 this comment, and if is suggests to me that maybe there are extra or missing words.

// 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 Expand Up @@ -868,12 +878,19 @@ protected function getILSLoginSettings()
* Construct an HTTP 205 (refresh) response. Useful for reporting success
* in the lightbox without actually rendering content.
*
* @param bool $forceGet If true, sends a custom header indicating that the page should be reloaded with a GET
* request. This can be useful when it is known that the current page only receives transient params in a POST
* request (such as canceling of holds).
*
* @return \Laminas\Http\Response
*/
protected function getRefreshResponse()
protected function getRefreshResponse(bool $forceGet = false)
{
$response = $this->getResponse();
$response->setStatusCode(205);
if ($forceGet) {
$response->getHeaders()->addHeaderLine('X-VuFind-Refresh-Method', 'GET');
}
return $response;
}

Expand Down
Loading
Loading