Description
Jesse Maseto
2018-11-27 16:48:47 UTC
Comment hidden (obsolete)
Kyle, was this supposed to be here? General public has no access to RT :) Ah, sorry, assigned to Kyle, written by Jesse! This enhancement will provide a secure method for a patron to change their Koha password that does not require any staff mediation. This method would be applicable only to selected patron types, controlled by the library with a setting that the library can change without requesting special assistance. Patrons would be provided with an appropriate link from the OPAC that would lead to a password setting page/UI. Patrons would be required to enter their old password which the system would check to identify and authenticate the patron. New passwords would have to entered twice, would have to match, and would be subject to guidelines for creating passwords (length, required characters, etc.). Alternately, patrons who forget their passwords would be able to recover their account and reset their passwords via email. Hi Jesse, thx for the update! But I am not sure how this is different from the existing password reset and password strength features? (In reply to Katrin Fischer from comment #4) > Hi Jesse, thx for the update! But I am not sure how this is different from > the existing password reset and password strength features? It's a level of granularity, not all patrons will be able to reset, just a certain set of them, based on patron type. As kidclamp clarified on IRC: <kidclamp> this is to allow it for only certain categories - very useful in a setting with shib or ldap backing <kidclamp> some patrons should only have they SSO login, others may have a local, they can change via koha <kidclamp> and it adds some notification, so if password is reset we send an email to notify user (in case of fraud!) Created attachment 84595 [details] [review] Bug 21890: DB updates This patch adds a new column to the categories table: 'can_reset_password' which is a boolean, and represents, well... that. It also deletes the OpacResetPassword system preference, but first sets the OpacResetPassword value to all defined categories as it will be used in the same way. Created attachment 84596 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin This patch introduces a method to the Koha::Template::Plugin::Categories TT plugin. This methods queries for categories that have the can_reset_password flag set, and returns a boolean representing the fact that there's at least one category with the flag set. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => SUCCESS: Tests pass! - Sign off :-D Caveat: this patch/tests require the schema to be updated Created attachment 84597 [details] [review] Bug 21890: Handle can_reset_password on the staff interface This patch makes the staff interface handle the new flag when editing the categories. I didn't add the flag to the list of categories table because it makes it too big, but can be easily added if required by users or QA. To test: - Along with other patches from this bug, edit patron categories => SUCCESS: Notice the changes are correctly stored - Sign off :-D Created attachment 84598 [details] [review] Bug 21890: Allow restricting password resets by patron category This patch makes the templates relying on the OpacResetPassword syspref use the introduced TT plugin method instead by changing: [% IF Koha.Preference('OpacResetPassword') %] => [% IF Categories.can_any_reset_password %] To test: - Verify that all the places in which the 'forgot password' link is displayed in OPAC keep working, provided there's at least one category that has the flag set - Attempt to recover the password for a patron that belong to a valid category (i.e. that has the flag set) => SUCCESS: You can go through the normal process - Attempt to recover the password for a patron that belongs to a category with the flag unset. => SUCCESS: Once Koha identifies your category, you are told you are not allowed to do it - Sign off :-D Created attachment 84631 [details] [review] Bug 21890: DB updates This patch adds a new column to the categories table: 'can_reset_password' which is a boolean, and represents, well... that. It also deletes the OpacResetPassword system preference, but first sets the OpacResetPassword value to all defined categories as it will be used in the same way. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 84632 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin This patch introduces a method to the Koha::Template::Plugin::Categories TT plugin. This methods queries for categories that have the can_reset_password flag set, and returns a boolean representing the fact that there's at least one category with the flag set. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => SUCCESS: Tests pass! - Sign off :-D Caveat: this patch/tests require the schema to be updated Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 84633 [details] [review] Bug 21890: Handle can_reset_password on the staff interface This patch makes the staff interface handle the new flag when editing the categories. I didn't add the flag to the list of categories table because it makes it too big, but can be easily added if required by users or QA. To test: - Along with other patches from this bug, edit patron categories => SUCCESS: Notice the changes are correctly stored - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 84634 [details] [review] Bug 21890: Allow restricting password resets by patron category This patch makes the templates relying on the OpacResetPassword syspref use the introduced TT plugin method instead by changing: [% IF Koha.Preference('OpacResetPassword') %] => [% IF Categories.can_any_reset_password %] To test: - Verify that all the places in which the 'forgot password' link is displayed in OPAC keep working, provided there's at least one category that has the flag set - Attempt to recover the password for a patron that belong to a valid category (i.e. that has the flag set) => SUCCESS: You can go through the normal process - Attempt to recover the password for a patron that belongs to a category with the flag unset. => SUCCESS: Once Koha identifies your category, you are told you are not allowed to do it - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Hi Tomas, I think I would have liked a workflow where the value from the global preference is inherited and can be overwritten on category level a little better as this is yet another thing to remember when setting up patron categories (adding more steps and clicks in most use cases). An example for the different behaviour would be: Block expired patrons. (something to consider) 2) NOT allowed = 1 for the column categories.can_reset_password seems counter intuitive? Please check values. 3) The new column is missing from the category overview page and column configuration. I know others appear to be missing too, but I'd consider this a bug as it means you have to open each one to see where you might have made a wrong choice. 4) The message is: + [% ELSIF errNoBorrowerFound %] + <li>You are not allowed to reset your password. Contact library staff for assistance.</li> [% END %] I think the <li> is wrong here as we are not inside a list and the other message doesn't have it. I think it would be nice to add a span around the message with an id, so if libraries want to change the message it will be easy. The message might read a little harsh to some or they might want to explain. Created attachment 85001 [details] [review] Bug 21890: DB updates This patch adds a new column to the categories table: 'reset_password' which is a boolean, and represents, well... that. The value itself is nullable. The idea is that OpacResetPassword governs the general behaviour, and this can be overriden at category-level. Created attachment 85002 [details] [review] Bug 21890: Handle can_reset_password on the staff interface This patch makes the staff interface handle the new flag when editing the categories. I didn't add the flag to the list of categories table because it makes it too big, but can be easily added if required by users or QA. To test: - Along with other patches from this bug, edit patron categories => SUCCESS: Notice the changes are correctly stored - Sign off :-D Created attachment 85003 [details] [review] Bug 21890: Add Koha::Patron::Category->effective_reset_password method This method checks wether the local $self->reset_password is set to override the OpacResetPassword syspref (i.e. if it is set to a bool) or undef, in which case if falls back to the value of the syspref. To test: - Apply this patches - Make sure the DB is updated: $ updatedatabase - Update the schema files: $ dbic - Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Category.t => SUCCESS: Tests pass! - Sign off :-D Created attachment 85004 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin This patch introduces a method to the Koha::Template::Plugin::Categories TT plugin. This methods queries for categories filtering them by effective_reset_password flag set, and returns a boolean representing the fact that there's at least one category allowed. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => SUCCESS: Tests pass! - Sign off :-D Caveat: this patch/tests require the schema to be updated Created attachment 85005 [details] [review] Bug 21890: Allow restricting password resets by patron category This patch makes the templates relying on the OpacResetPassword syspref use the introduced TT plugin method instead by changing: [% IF Koha.Preference('OpacResetPassword') %] => [% IF Categories.can_any_reset_password %] To test: - Verify that all the places in which the 'forgot password' link is displayed in OPAC keep working, provided there's at least one category that has the flag set - Attempt to recover the password for a patron that belong to a valid category (i.e. that has the flag set) => SUCCESS: You can go through the normal process - Attempt to recover the password for a patron that belongs to a category with the flag unset. => SUCCESS: Once Koha identifies your category, you are told you are not allowed to do it - Sign off :-D Created attachment 85006 [details] [review] Bug 21890: DBIC update I reworked this so: - reset_password is a nullable bool. Null means global setting is picked, so 0/1 override global, as requested in comment #1 - #2 is solved, silly mistake - #3 Will deal with it in a follow-up bug. Can do here if blocker. But there will be another similar bug for password changes... I prefer to solve it for both in a separate bug. - #4 Solved! Hold on! Idiomatic issues: I use 'reset' in spanish usually in the context of the 'forgot password' scenario, and I mixed things :-D. The feature is sound, but every place it says 'reset it should say 'recovery'. I will submit a follow-up fixing this mix. *** Bug 3032 has been marked as a duplicate of this bug. *** With help from Katrin, I decided to keep it as-is, as it is consistent with how things are named on the UI and the controlling (existing) sysprefs: - OpacPasswordReset - OpacChangePassword Hi Tomas, I can't seem to get these to apply on latest master: error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt). error: could not build fake ancestor Patch failed at 0001 Bug 21890: Allow restricting password resets by patron category Sorry :( Cheers, Liz Created attachment 86843 [details] [review] Bug 21890: DB updates This patch adds a new column to the categories table: 'reset_password' which is a boolean, and represents, well... that. The value itself is nullable. The idea is that OpacResetPassword governs the general behaviour, and this can be overriden at category-level. Created attachment 86844 [details] [review] Bug 21890: Handle can_reset_password on the staff interface This patch makes the staff interface handle the new flag when editing the categories. I didn't add the flag to the list of categories table because it makes it too big, but can be easily added if required by users or QA. To test: - Along with other patches from this bug, edit patron categories => SUCCESS: Notice the changes are correctly stored - Sign off :-D Created attachment 86845 [details] [review] Bug 21890: Add Koha::Patron::Category->effective_reset_password method This method checks wether the local $self->reset_password is set to override the OpacResetPassword syspref (i.e. if it is set to a bool) or undef, in which case if falls back to the value of the syspref. To test: - Apply this patches - Make sure the DB is updated: $ updatedatabase - Update the schema files: $ dbic - Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Category.t => SUCCESS: Tests pass! - Sign off :-D Created attachment 86846 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin This patch introduces a method to the Koha::Template::Plugin::Categories TT plugin. This methods queries for categories filtering them by effective_reset_password flag set, and returns a boolean representing the fact that there's at least one category allowed. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => SUCCESS: Tests pass! - Sign off :-D Caveat: this patch/tests require the schema to be updated Created attachment 86847 [details] [review] Bug 21890: Allow restricting password resets by patron category This patch makes the templates relying on the OpacResetPassword syspref use the introduced TT plugin method instead by changing: [% IF Koha.Preference('OpacResetPassword') %] => [% IF Categories.can_any_reset_password %] To test: - Verify that all the places in which the 'forgot password' link is displayed in OPAC keep working, provided there's at least one category that has the flag set - Attempt to recover the password for a patron that belong to a valid category (i.e. that has the flag set) => SUCCESS: You can go through the normal process - Attempt to recover the password for a patron that belongs to a category with the flag unset. => SUCCESS: Once Koha identifies your category, you are told you are not allowed to do it - Sign off :-D I resubmitted after checking some merge conflicts. I removed the DBIC update so run: $ dbic on kohadevbox to regenerate them on testing. Created attachment 86866 [details] [review] Bug 21890: DB updates This patch adds a new column to the categories table: 'reset_password' which is a boolean, and represents, well... that. The value itself is nullable. The idea is that OpacResetPassword governs the general behaviour, and this can be overriden at category-level. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 86867 [details] [review] Bug 21890: Handle can_reset_password on the staff interface This patch makes the staff interface handle the new flag when editing the categories. I didn't add the flag to the list of categories table because it makes it too big, but can be easily added if required by users or QA. To test: - Along with other patches from this bug, edit patron categories => SUCCESS: Notice the changes are correctly stored - Sign off :-D Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 86868 [details] [review] Bug 21890: Add Koha::Patron::Category->effective_reset_password method This method checks wether the local $self->reset_password is set to override the OpacResetPassword syspref (i.e. if it is set to a bool) or undef, in which case if falls back to the value of the syspref. To test: - Apply this patches - Make sure the DB is updated: $ updatedatabase - Update the schema files: $ dbic - Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Category.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 86869 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin This patch introduces a method to the Koha::Template::Plugin::Categories TT plugin. This methods queries for categories filtering them by effective_reset_password flag set, and returns a boolean representing the fact that there's at least one category allowed. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => SUCCESS: Tests pass! - Sign off :-D Caveat: this patch/tests require the schema to be updated Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 86870 [details] [review] Bug 21890: Allow restricting password resets by patron category This patch makes the templates relying on the OpacResetPassword syspref use the introduced TT plugin method instead by changing: [% IF Koha.Preference('OpacResetPassword') %] => [% IF Categories.can_any_reset_password %] To test: - Verify that all the places in which the 'forgot password' link is displayed in OPAC keep working, provided there's at least one category that has the flag set - Attempt to recover the password for a patron that belong to a valid category (i.e. that has the flag set) => SUCCESS: You can go through the normal process - Attempt to recover the password for a patron that belongs to a category with the flag unset. => SUCCESS: Once Koha identifies your category, you are told you are not allowed to do it - Sign off :-D Signed-off-by: Liz Rea <wizzyrea@gmail.com> Hi, Quick caveat on the testing here, bug 22559 had to be applied before I could test this properly, and my signoff is dependent on that. Cheers, Liz Created attachment 86911 [details] [review] Bug 21890: Wrap message in span Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Comment on attachment 86869 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin Review of attachment 86869 [details] [review]: ----------------------------------------------------------------- ::: Koha/Template/Plugin/Categories.pm @@ +33,5 @@ > return Koha::Patron::Categories->find( $categorycode )->description; > } > > +sub can_any_reset_password { > + return ( grep { $_->effective_reset_password } @{ Koha::Patron::Categories->search->as_list } ) I'd prefer a List::Util::any call here: https://metacpan.org/pod/List::Util#any Created attachment 87289 [details] [review] Bug 21890: DB updates This patch adds a new column to the categories table: 'reset_password' which is a boolean, and represents, well... that. The value itself is nullable. The idea is that OpacResetPassword governs the general behaviour, and this can be overriden at category-level. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87290 [details] [review] Bug 21890: Handle can_reset_password on the staff interface This patch makes the staff interface handle the new flag when editing the categories. I didn't add the flag to the list of categories table because it makes it too big, but can be easily added if required by users or QA. To test: - Along with other patches from this bug, edit patron categories => SUCCESS: Notice the changes are correctly stored - Sign off :-D Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87291 [details] [review] Bug 21890: Add Koha::Patron::Category->effective_reset_password method This method checks wether the local $self->reset_password is set to override the OpacResetPassword syspref (i.e. if it is set to a bool) or undef, in which case if falls back to the value of the syspref. To test: - Apply this patches - Make sure the DB is updated: $ updatedatabase - Update the schema files: $ dbic - Run: $ kshell k$ prove t/db_dependent/Koha/Patron/Category.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87292 [details] [review] Bug 21890: Add can_any_reset_password() to the Categories TT plugin This patch introduces a method to the Koha::Template::Plugin::Categories TT plugin. This methods queries for categories filtering them by effective_reset_password flag set, and returns a boolean representing the fact that there's at least one category allowed. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => SUCCESS: Tests pass! - Sign off :-D Caveat: this patch/tests require the schema to be updated Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87293 [details] [review] Bug 21890: Allow restricting password resets by patron category This patch makes the templates relying on the OpacResetPassword syspref use the introduced TT plugin method instead by changing: [% IF Koha.Preference('OpacResetPassword') %] => [% IF Categories.can_any_reset_password %] To test: - Verify that all the places in which the 'forgot password' link is displayed in OPAC keep working, provided there's at least one category that has the flag set - Attempt to recover the password for a patron that belong to a valid category (i.e. that has the flag set) => SUCCESS: You can go through the normal process - Attempt to recover the password for a patron that belongs to a category with the flag unset. => SUCCESS: Once Koha identifies your category, you are told you are not allowed to do it - Sign off :-D Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87294 [details] [review] Bug 21890: Wrap message in span Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87295 [details] [review] Bug 21890: (QA follow-up) Use List::Util::any When possible it's a good idea to use `any` from List::Util to shortcut on the first occurence of a truthy value. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Works well, nice clear patches, no regressions and passes the qa scripts (with the notable exception of a false failure for the template plugin) Passing QA Awesome work all! Pushed to master for 19.05 Enhancement will not be backported to 18.11.x series. |