Bug 28912 - Pseudonymization should display a nice error message when brcypt_settings are not defined
Summary: Pseudonymization should display a nice error message when brcypt_settings are...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 24151
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-27 14:05 UTC by Nick Clemens
Modified: 2022-06-06 20:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04,20.11.10


Attachments
Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set (5.11 KB, patch)
2021-08-27 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28912: Remove duplicated warnings on the about page (1.35 KB, patch)
2021-08-27 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set (5.17 KB, patch)
2021-08-27 16:13 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28912: Remove duplicated warnings on the about page (1.41 KB, patch)
2021-08-27 16:13 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set (5.34 KB, patch)
2021-08-30 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28912: Remove duplicated warnings on the about page (1.41 KB, patch)
2021-08-30 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28912: Prevent Pseudonymization to be set if bcrypt_settings not set (5.48 KB, patch)
2021-09-05 15:03 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28912: Remove duplicated warnings on the about page (1.55 KB, patch)
2021-09-05 15:03 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-08-27 14:05:42 UTC
Currently we cause an ISE when bcrypt_settings are missing

We should throw/catch an exception and display a nicer error message

To recreate:
1 - edit koha-conf, remove 'bcrypt_settings'
2 - Enable pseudonymization
3 - Attempt a checkout
4 - Boom!
Comment 1 Jonathan Druart 2021-08-27 15:00:38 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
Comment 2 Jonathan Druart 2021-08-27 15:00:42 UTC
Created attachment 124187 [details] [review]
Bug 28912: Remove duplicated warnings on the about page

Bad conflict resolution
Comment 3 Andrew Fuerste-Henry 2021-08-27 16:13:02 UTC
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>
Comment 4 Andrew Fuerste-Henry 2021-08-27 16:13:05 UTC
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>
Comment 5 Katrin Fischer 2021-08-29 11:40:30 UTC
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!
Comment 6 Katrin Fischer 2021-08-29 11:42:44 UTC
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
Comment 7 Jonathan Druart 2021-08-30 09:20:14 UTC
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
Comment 8 Jonathan Druart 2021-08-30 09:20:19 UTC
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>
Comment 9 Jonathan Druart 2021-08-30 09:21:09 UTC
Oops, fixed!
Comment 10 Joonas Kylmälä 2021-09-05 15:03:26 UTC
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>
Comment 11 Joonas Kylmälä 2021-09-05 15:03:30 UTC
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>
Comment 12 Joonas Kylmälä 2021-09-05 15:05:21 UTC
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.
Comment 13 Jonathan Druart 2021-09-09 09:00:59 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 14 Kyle M Hall 2021-09-10 12:08:42 UTC
Pushed to 21.05.x for 21.05.04
Comment 15 Fridolin Somers 2021-09-16 07:49:00 UTC
Pushed to 20.11.x for 20.11.10
Comment 16 Victor Grousset/tuxayo 2021-09-20 11:57:30 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.