Description
Nick Clemens (kidclamp)
2021-08-27 14:05:42 UTC
Created attachment 124186 [details] [review] Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set If Pseudonymization is set but the bcrypt_settings config used by the feature is not set, then there is an ugly 500 on checking out. bad bcrypt settings at /kohadevbox/koha/Koha/PseudonymizedTransaction.pm line 116. However it's pretty hard to handle correctly this exception (and that's why it hasn't be done initially). However we could prevent the pref to be turned on if the config entry is not present. Test plan: Remove the bcrypt_settings from the config Try to turn the syspref on Add the config Try to turn the syspref on/off Created attachment 124187 [details] [review] Bug 28912: Remove duplicated warnings on the about page Bad conflict resolution Created attachment 124191 [details] [review] Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set If Pseudonymization is set but the bcrypt_settings config used by the feature is not set, then there is an ugly 500 on checking out. bad bcrypt settings at /kohadevbox/koha/Koha/PseudonymizedTransaction.pm line 116. However it's pretty hard to handle correctly this exception (and that's why it hasn't be done initially). However we could prevent the pref to be turned on if the config entry is not present. Test plan: Remove the bcrypt_settings from the config Try to turn the syspref on Add the config Try to turn the syspref on/off Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 124192 [details] [review] Bug 28912: Remove duplicated warnings on the about page Bad conflict resolution Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> There are 2 fails here in the QA script: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt FAIL filters missing_filter at line 118 ( <select name="pref_[% CHUNK.name | html %]" id="pref_[% CHUNK.name | html %]" [% disabled %] class="preference preference-[% CHUNK.class or "choice" | html %]">) missing_filter at line 187 ( [% CASE %]Unknown warning "[% w %]") OK forbidden patterns OK git manipulation OK js_in_body OK spelling FAIL tt_valid lines 118 Please fix! Hm, updated QA test tools to make sure, but it just added one more to the list: FAIL admin/preferences.pl OK critic FAIL forbidden patterns forbidden pattern: Use of Data::Printer, should only be used for devlopment purpose (line 238) OK git manipulation Created attachment 124236 [details] [review] Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set If Pseudonymization is set but the bcrypt_settings config used by the feature is not set, then there is an ugly 500 on checking out. bad bcrypt settings at /kohadevbox/koha/Koha/PseudonymizedTransaction.pm line 116. However it's pretty hard to handle correctly this exception (and that's why it hasn't be done initially). However we could prevent the pref to be turned on if the config entry is not present. Test plan: Remove the bcrypt_settings from the config Try to turn the syspref on Add the config Try to turn the syspref on/off Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> JD amended patch: fix qa failures Created attachment 124237 [details] [review] Bug 28912: Remove duplicated warnings on the about page Bad conflict resolution Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Oops, fixed! Created attachment 124519 [details] [review] Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set If Pseudonymization is set but the bcrypt_settings config used by the feature is not set, then there is an ugly 500 on checking out. bad bcrypt settings at /kohadevbox/koha/Koha/PseudonymizedTransaction.pm line 116. However it's pretty hard to handle correctly this exception (and that's why it hasn't be done initially). However we could prevent the pref to be turned on if the config entry is not present. Test plan: Remove the bcrypt_settings from the config Try to turn the syspref on Add the config Try to turn the syspref on/off Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> JD amended patch: fix qa failures Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Created attachment 124520 [details] [review] Bug 28912: Remove duplicated warnings on the about page Bad conflict resolution Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> This is a nice improvement and blocks the common case where the ISE might happen. If someone wants to further improve this the specification for the bcrypt setting string is defined in https://metacpan.org/pod/Crypt::Eksblowfish::Bcrypt#bcrypt(PASSWORD,-SETTINGS) and we could handle the error more gracefully if it doesn't follow the format. Pushed to master for 21.11, thanks to everybody involved! Pushed to 21.05.x for 21.05.04 Pushed to 20.11.x for 20.11.10 Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. |