Enable password recovery when using ILS Authentication #3997

oharacj · 2024-10-08T20:30:25Z

Add handling to enable password recovery when using ILS Authentication method

…tion.

…tion. Updated based on the PHP 8.3 test failures.

…tion. Updated based on the PHP 8.3 test failures. When I ran php cbf it made changes to the if statements that caused this failure.

demiankatz

Thanks, @oharacj! See below for a few initial thoughts and questions.

module/VuFind/src/VuFind/Auth/ILSAuthenticator.php

module/VuFind/src/VuFind/Controller/MyResearchController.php

demiankatz · 2024-10-09T11:46:53Z

module/VuFind/src/VuFind/Controller/MyResearchController.php

@@ -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';


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

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

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).

EreMaijala

This should work without an existing VuFind account. My proposal would be to add another ILS driver method for looking up the patron with email address and card number. Since we already have a local implementation that works across ILS systems, our experience is that this allows recovery with any ILS that can do the lookup and update the password. See https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/Driver/KohaRest.php#L747 for our implementation with Koha.

The recovery token data and any other information needed (e.g. target ILS for MultiILS) from the lookup method can be stored in access_token table with a random hash as the id. This hash should be included in the recovery link in the email along.

Since this functionality is somewhat different from internal password recovery, it might make sense to separate the implementation. That's why we chose locally to use different method names etc.

EreMaijala · 2024-10-10T08:20:32Z

module/VuFind/src/VuFind/Auth/ILS.php

+     *
+     * @return bool
+     */
+    public function supportsPasswordRecovery()


This needs a counterpart in MultiILS that supports checking the selected target. For inspiration:

https://github.com/NatLibFi/NDL-VuFind2/blob/dev/themes/finna2/templates/Auth/MultiILS/login.phtml#L64

https://github.com/NatLibFi/NDL-VuFind2/blob/dev/themes/finna2/js/finna-layout.js#L372

https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/Auth/ILSFinna.php#L53

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.

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.

EreMaijala · 2024-10-10T08:24:20Z

module/VuFind/src/VuFind/Auth/ILS.php

+    {
+        $driver = $this->getCatalog()->getDriver();
+        if (
+            method_exists($driver, 'changePassword')


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

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

Updated comments, line endings, follow-up edits have been put in abstractbase, explained the redirect.

updated formatting.

added trailing comma to return array

Object-Oriented Programming is a thing.

oharacj · 2024-10-16T15:16:29Z

This should work without an existing VuFind account. My proposal would be to add another ILS driver method for looking up the patron with email address and card number. Since we already have a local implementation that works across ILS systems, our experience is that this allows recovery with any ILS that can do the lookup and update the password. See https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/Driver/KohaRest.php#L747 for our implementation with Koha.

I have an ILS method called getPatronFromUsername which provides this function exactly. It has been added to the SierraRest driver.

The recovery token data and any other information needed (e.g. target ILS for MultiILS) from the lookup method can be stored in access_token table with a random hash as the id. This hash should be included in the recovery link in the email along.

Since this functionality is somewhat different from internal password recovery, it might make sense to separate the implementation. That's why we chose locally to use different method names etc.

I am partially utilising the internal password recovery methods in order to keep from duplicating functionality

demiankatz

Thanks for the progress, @oharacj -- see below for a few more tips, questions and ideas. :-)

demiankatz · 2024-10-17T15:42:36Z

config/vufind/SierraRest.ini

@@ -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


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.

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).

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."

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.

demiankatz · 2024-10-17T15:43:25Z

module/VuFind/src/VuFind/Auth/ILS.php

+     *
+     * @return bool
+     */
+    public function supportsPasswordRecovery()


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.

module/VuFind/src/VuFind/ILS/Driver/SierraRest.php

module/VuFind/src/VuFind/Controller/MyResearchController.php

demiankatz · 2024-10-17T15:49:31Z

module/VuFind/src/VuFind/Auth/ILSAuthenticator.php

@@ -309,11 +309,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 $user_name - the username/barcode for ILS password reset


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.

demiankatz · 2024-10-17T16:00:56Z

module/VuFind/src/VuFind/Controller/MyResearchController.php

@@ -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';


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).

demiankatz

Thanks for the ongoing work, @oharacj -- I started my review this morning but ran into some questions that I think will be resolved by merging the dev branch into this PR, so I decided to stop until that's done to be sure I'm reviewing everything in an up-to-date and working state. Please let me know if you run into any trouble with this!

demiankatz · 2024-12-13T13:51:50Z

config/vufind/SierraRest.ini

@@ -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


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).

module/VuFind/src/VuFind/Auth/ILSAuthenticator.php

demiankatz

I didn't have time for a full review yet, but I noticed one thing that looks like a problem and had a couple of very minor style suggestions that you can apply by just committing my suggestions from within the comments below. :-)

module/VuFind/src/VuFind/Controller/MyResearchController.php

themes/bootstrap5/templates/myresearch/newpassword.phtml

Co-authored-by: Demian Katz <[email protected]>

demiankatz

@oharacj, I've given this another relatively high-level review (since much of this code is outside of my area of expertise, it would take me longer to really refresh all my memory on the details, so I'm just trying to chip away at issues while waiting for others with more experience to find time to review more deeply). Please feel free to ignore any comments that are not relevant due to my naivete. I hope the bombardment of small reviews is not a nuisance! ;-)

demiankatz · 2025-01-10T19:00:30Z

module/VuFind/src/VuFind/Auth/ILS.php

+     * @throws AuthException
+     * @throws \Exception
+     */
+    public function newPassword($request)


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?

I'm pretty sure that updatePassword is able to do everything needed, so this method shouldn't be needed.

demiankatz · 2025-01-10T19:03:09Z

module/VuFind/src/VuFind/Auth/Manager.php

+     *
+     * @return bool
+     */
+    public function multiSupportsPasswordRecovery($target)


Might it be possible to add a second optional argument to supportsPasswordRecovery instead of a whole new method here? Or is this really unavoidable?

demiankatz · 2025-01-10T19:04:28Z

module/VuFind/src/VuFind/Controller/AbstractBase.php

@@ -712,6 +712,14 @@ protected function setFollowupUrlToReferer(bool $allowCurrentUrl = true, array $
        if ($mrhuNorm === $refererNorm) {
            return;
        }
+        // if  is the stored current url of the lightbox


I'm not sure I understand this comment, and if is suggests to me that maybe there are extra or missing words.

demiankatz · 2025-01-10T19:05:03Z

module/VuFind/src/VuFind/Controller/MyResearchController.php

+        //ILS Driver:
+        //if the user hasn't logged in yet, but is found by the ILS, call function
+        //getPatronFromUsername


Another minor style tweak that you can commit directly from this comment:

Suggested change

//ILS Driver:

//if the user hasn't logged in yet, but is found by the ILS, call function

//getPatronFromUsername

// ILS Driver:

// if the user hasn't logged in yet, but is found by the ILS, call function

// getPatronFromUsername

demiankatz · 2025-01-10T19:10:50Z

themes/bootstrap5/templates/myresearch/newpassword.phtml

@@ -23,6 +23,9 @@
    <input type="hidden" value="<?=$this->escapeHtmlAttr($this->hash) ?>" name="hash">
    <input type="hidden" value="<?=$this->escapeHtmlAttr($this->username) ?>" name="username">
    <input type="hidden" value="<?=$this->escapeHtmlAttr($this->auth_method) ?>" name="auth_method">
+    <?php if (!empty($this->target)): ?>
+      <input type="hidden" value="<?=$this->escapeHtmlAttr($this->target) ?>" name="target">


Would it make sense to use a more descriptive name here, like target_ils? At a glance, it's not immediately obvious to me what target means, so a little context might help.

demiankatz · 2025-01-10T19:13:16Z

module/VuFind/src/VuFind/Controller/MyResearchController.php

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


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

$target = $this->getRequest()->getQuery('target'); $targetParam = $target ? '&target=' . urlencode($target) : '';

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.

EreMaijala · 2025-01-16T19:13:31Z

module/VuFind/src/VuFind/Auth/ILS.php

+     * @throws AuthException
+     * @throws \Exception
+     */
+    public function newPassword($request)


I'm pretty sure that updatePassword is able to do everything needed, so this method shouldn't be needed.

EreMaijala · 2025-01-16T19:16:54Z

module/VuFind/src/VuFind/Auth/ILSAuthenticator.php

@@ -329,8 +330,22 @@ public function storedCatalogLogin()
                $this->ilsAccount[$username] = $patron;
                return $patron;
            }
+        } elseif (!empty($userName)) {


What's this for? I don't think this should be needed for password recovery.

EreMaijala · 2025-01-16T19:17:44Z

module/VuFind/src/VuFind/Auth/Manager.php

+     * @throws AuthException
+     * @return UserEntityInterface Updated user entity.
+     */
+    public function newPassword($request)


This should not be necessary either.

EreMaijala · 2025-01-16T19:22:57Z

module/VuFind/src/VuFind/Auth/MultiILS.php

+        $driver = $catalog->getDriver();
+        $targetDriver = $driver;
+        if ($target != null) {
+            $targetDriver = $driver->getDriverFromTarget($target);


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.

EreMaijala · 2025-01-16T19:28:23Z

module/VuFind/src/VuFind/Controller/MyResearchController.php

+        //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)) {


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).

EreMaijala · 2025-01-16T19:34:34Z

themes/bootstrap3/templates/myresearch/newpassword.phtml

@@ -18,11 +18,14 @@
 <?php elseif (!isset($this->hash)): ?>
  <div class="error"><?=$this->transEsc('recovery_user_not_found') ?></div>
 <?php else: ?>
-  <form id="newpassword" class="form-new-password" action="<?=$this->url('myresearch-newpassword') ?>" method="post" data-toggle="validator">
+  <form id="newpassword" class="form-new-password" action="<?=$this->url('myresearch-newpassword') ?>" method="post" data-bs-toggle="validator">


There should be data-toggle, no data-bs-toggle in bootstrap3

oharacj added 4 commits October 8, 2024 14:24

Initial changes to enable password recovery when using ILS Authentica…

fa64c72

…tion.

Initial changes to enable password recovery when using ILS Authentica…

e80e27b

…tion. Updated based on the PHP 8.3 test failures.

Initial changes to enable password recovery when using ILS Authentica…

a06804d

…tion. Updated based on the PHP 8.3 test failures. When I ran php cbf it made changes to the if statements that caused this failure.

Remove the return clause in setFollowupUrlToReferer method.

6afdc74

demiankatz requested changes Oct 9, 2024

View reviewed changes

EreMaijala requested changes Oct 10, 2024

View reviewed changes

oharacj added 5 commits October 15, 2024 13:52

Updated based on Demian's comments

e61daf3

Updated comments, line endings, follow-up edits have been put in abstractbase, explained the redirect.

Add getPatronFromUsername to SierraRest driver.

95926c5

Add getPatronFromUsername to SierraRest driver.

f4e32f2

updated formatting.

Add getPatronFromUsername to SierraRest driver.

e977b33

added trailing comma to return array

Add getPatronFromUsername to SierraRest driver.

03bff51

Object-Oriented Programming is a thing.

demiankatz requested changes Oct 17, 2024

View reviewed changes

oharacj added 8 commits December 11, 2024 12:57

Multi-ILS Driver work and bug fixes

58a2f2f

Fixed some encoding and comment issues

9a8defa

removed comment block in .js file

f731cde

added displayILSPasswordRecoveryLink to the top exported comment

a85a525

PHP 8.3 updates and space fixes

fd7d5a7

PHP 8.3 updates and space fixes

44d8456

PHP 8.3 double quotes in template

9800f5b

PHP 8.3 double quotes in template

4183693

oharacj requested review from demiankatz and EreMaijala December 12, 2024 15:58

demiankatz requested changes Dec 13, 2024

View reviewed changes

oharacj added 3 commits December 23, 2024 12:15

Merge branch 'dev' into ilsPasswordRecovery

072d3fb

Remove hard-coded pin length in changePassword function

11e26a4

Update settings file to remove redundant setting.

475f222

oharacj requested a review from demiankatz December 30, 2024 15:43

demiankatz requested changes Jan 2, 2025

View reviewed changes

module/VuFind/src/VuFind/Controller/MyResearchController.php Outdated Show resolved Hide resolved

module/VuFind/src/VuFind/Controller/MyResearchController.php Outdated Show resolved Hide resolved

themes/bootstrap5/templates/myresearch/newpassword.phtml Outdated Show resolved Hide resolved

updating indentation

7646e33

Co-authored-by: Demian Katz <[email protected]>

oharacj and others added 3 commits January 8, 2025 09:00

update spacing in comment

0d5b9d9

Co-authored-by: Demian Katz <[email protected]>

update spacing and indents

eb7ea64

update spacing and indents

85cd191

oharacj requested a review from demiankatz January 8, 2025 16:57

demiankatz requested changes Jan 10, 2025

View reviewed changes

EreMaijala requested changes Jan 16, 2025

View reviewed changes

Enable password recovery when using ILS Authentication #3997

Are you sure you want to change the base?

Enable password recovery when using ILS Authentication #3997

Conversation

oharacj commented Oct 8, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EreMaijala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oharacj commented Oct 16, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EreMaijala Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EreMaijala Dec 16, 2024 •

edited

Loading