Created attachment 79165 [details] [review]
Bug 21336: Small adjustment in Koha::Token

Allow a pattern too in Koha::Token->generate. Only supported length.
Does not affect CSRF calls. So tiny change without further impact.

Test plan:
Run  t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 79166 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 79167 [details] [review]
Bug 21336: DBIx schema changes

Changes for additional column flgAnonymized.
No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 79168 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 79169 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 79170 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Still needs some attention. Coming back soon

Created attachment 80201 [details] [review]
Bug 21336: Small adjustment in Koha::Token

Allow a pattern too in Koha::Token->generate. Only supported length.
Does not affect CSRF calls. So tiny change without further impact.

Test plan:
Run  t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80202 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80203 [details] [review]
Bug 21336: DBIx schema changes

Changes for additional column flgAnonymized.
No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80204 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80205 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80206 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

(In reply to Marcel de Rooy from comment #7)
> Still needs some attention. Coming back soon

Mainly added two patches in the middle. Introducing the term "administrative lockout" in order to distinguish from lockout by typing wrong passwords.

Hi Marcel,
patch looks very useful. I've just one notice. Is possible to add some e-mail report with list of anonymised borrower records? Because libraries still use paper contracts and after "digital" anynomisation we should remove "paper" information too.

Thank You

Mike

Created attachment 80213 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Made the test search_anonymized a tiny bit smarter :)
Just changed two lines..

(In reply to Michal Denar from comment #17)
> Hi Marcel,
> patch looks very useful. I've just one notice. Is possible to add some
> e-mail report with list of anonymised borrower records? Because libraries
> still use paper contracts and after "digital" anynomisation we should remove
> "paper" information too.

Will have a look

(In reply to M. Tompsett from comment #22)
> Eval may mask output, which the user should see.
> Though, I understand the use of eval, since the code should keep going.

Right. Added warn $@ if $@

Created attachment 80403 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Created attachment 81112 [details] [review]
Bug 21336: Small adjustment in Koha::Token

Allow a pattern too in Koha::Token->generate. Only supported length.
Does not affect CSRF calls. So tiny change without further impact.

Test plan:
Run  t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81113 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81114 [details] [review]
Bug 21336: DBIx schema changes

Changes for additional column flgAnonymized.
No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81115 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81116 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81117 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Created attachment 81118 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81119 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Trivial rebase

Created attachment 81475 [details] [review]
Bug 21336: Small adjustment in Koha::Token

Allow a pattern too in Koha::Token->generate. Only supported length.
Does not affect CSRF calls. So tiny change without further impact.

Test plan:
Run  t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81476 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81477 [details] [review]
Bug 21336: DBIx changes

Created attachment 81478 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81479 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81480 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Created attachment 81481 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 81482 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Created attachment 81483 [details] [review]
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

(In reply to Michal Denar from comment #17)
> Hi Marcel,
> patch looks very useful. I've just one notice. Is possible to add some
> e-mail report with list of anonymised borrower records? Because libraries
> still use paper contracts and after "digital" anynomisation we should remove
> "paper" information too.
> 
> Thank You
> 
> Mike

Moved this to its own new report.

(In reply to Marcel de Rooy from comment #47)
> (In reply to Michal Denar from comment #17)
> Moved this to its own new report.

== bug 21712

Created attachment 82051 [details] [review]
Bug 21336: Small adjustment in Koha::Token

Allow a pattern too in Koha::Token->generate. Only supported length.
Does not affect CSRF calls. So tiny change without further impact.

Test plan:
Run  t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 82052 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 82053 [details] [review]
Bug 21336: DBIx changes

Created attachment 82054 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 82055 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 82056 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Created attachment 82057 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 82058 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Created attachment 82059 [details] [review]
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

First patch moved to 21998

Created attachment 83202 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 83203 [details] [review]
Bug 21336: DBIx changes

Created attachment 83204 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 83205 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 83206 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Created attachment 83207 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 83208 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Created attachment 83209 [details] [review]
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Rebased

Issue in anonymize with strict sql mode

Created attachment 83256 [details] [review]
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column

When a field is not nullable we should pass empty string, zero or today
to a char, numeric or date column.
The mandatory parameter does not refer to a database constraint but a Koha
preference. Only for strings we generate a random value.

Test plan:
Enable strict_sql_modes.
Run t/db_dependent/Koha/Patrons.t again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Just one small think:

database update should use column_exists on adding flgAnonymized column

but otherwise looking good for me...

(In reply to Marcel de Rooy from comment #63)
> As a preparation for Koha::Patron->lock, we add the concept of administrative
> lockout. The account is locked just as it would have been by too much
> failed login attempts.
> This is handled by a negative value in borrowers.login_attempts.

I am not sure about that, it sounds better to me to have another attribute.

(In reply to Jonathan Druart from comment #71)

> I am not sure about that, it sounds better to me to have another attribute.

I would strongly prefer to do that on another report now.

Created attachment 87158 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87159 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87160 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87161 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Created attachment 87162 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87163 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Created attachment 87164 [details] [review]
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87165 [details] [review]
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column

When a field is not nullable we should pass empty string, zero or today
to a char, numeric or date column.
The mandatory parameter does not refer to a database constraint but a Koha
preference. Only for strings we generate a random value.

Test plan:
Enable strict_sql_modes.
Run t/db_dependent/Koha/Patrons.t again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87167 [details] [review]
Bug 21336: (QA follow-up) Add column_exists in db revision

As requested by Josef on comment70.

Test plan:
Run db revision twice. No warnings.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 87168 [details] [review]
Bug 21336: [DO_NOT_PUSH] DBIx changes

MariaDB somehow turns current_timestamp into current_timestamp().
The needed change is for column flgAnonymized.

(In reply to Josef Moravec from comment #70)
> Just one small think:
> 
> database update should use column_exists on adding flgAnonymized column
> 
> but otherwise looking good for me...

Rebased all stuff.
Added a follow-up for the column_exists.
Could you add your signoff line please?
Hopefully we can get another QAer to have a look in the next weeks. This code runs in production with me for about four months.

Created attachment 87176 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87177 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87178 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87179 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87180 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87181 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87182 [details] [review]
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87183 [details] [review]
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column

When a field is not nullable we should pass empty string, zero or today
to a char, numeric or date column.
The mandatory parameter does not refer to a database constraint but a Koha
preference. Only for strings we generate a random value.

Test plan:
Enable strict_sql_modes.
Run t/db_dependent/Koha/Patrons.t again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87184 [details] [review]
Bug 21336: (QA follow-up) Add column_exists in db revision

As requested by Josef on comment70.

Test plan:
Run db revision twice. No warnings.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 87185 [details] [review]
Bug 21336: [DO_NOT_PUSH] DBIx changes

MariaDB somehow turns current_timestamp into current_timestamp().
The needed change is for column flgAnonymized.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

QA Script is complaining.. though I'm not entirely sure what it means:

 FAIL   Koha/Patron.pm
   FAIL   valid
                Constant subroutine Koha::Patron::ADMINISTRATIVE_LOCKOUT redefined


Any thoughts?

(In reply to Martin Renvoize from comment #95)
> QA Script is complaining.. though I'm not entirely sure what it means:
> 
>  FAIL   Koha/Patron.pm
>    FAIL   valid
>                 Constant subroutine Koha::Patron::ADMINISTRATIVE_LOCKOUT
> redefined
> 
> 
> Any thoughts?

Look at Koha/Patron.pm without these patches. And run perl -cw
You will see lots of redefines warnings due to some circular inheritance issue there.
The new constant suffers from this problem too. So this is a false negative.

Back to SO

Comment on attachment 87176 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

Review of attachment 87176 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_21336a.perl
@@ +1,3 @@
> +$DBversion = 'XXX';  # will be replaced by the RM
> +if( CheckVersion( $DBversion ) ) {
> +    $dbh->do( "ALTER TABLE borrowers ADD COLUMN flgAnonymized tinyint DEFAULT 0" );

Sorry chaps.. we need the if column exists adding here to make it idempotent.

@@ +1,4 @@
> +$DBversion = 'XXX';  # will be replaced by the RM
> +if( CheckVersion( $DBversion ) ) {
> +    $dbh->do( "ALTER TABLE borrowers ADD COLUMN flgAnonymized tinyint DEFAULT 0" );
> +    $dbh->do( "ALTER TABLE deletedborrowers ADD COLUMN flgAnonymized tinyint DEFAULT 0" );

and here...

Ignore that.. I somehow missed it in the qa followup.

I think we're missing a test for the account_locked method for the administrative lockout case (i.e. the negative integer for login_attempts case)

Created attachment 87387 [details] [review]
Bug 21336: Database revision for borrowers.flgAnonymized

This new flag should speak for itself ;)

Test plan:
Run installer or updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87388 [details] [review]
Bug 21336: Dbrev for new prefs

This patch adds three new prefs:
- UnsubscribeReflectionDelay
- PatronAnonymizeDelay
- PatronRemovalDelay

Test plan:
Run upgrade.
Check patrons pref page. Look under Privacy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87389 [details] [review]
Bug 21336: Do not increase login_attempts after locking

If an account has been locked, there is no use to keep increasing this
number. It is not true too; after the pref number has been reached,
we can not really speak of login attempts anymore. The credentials are
just ignored.

Adding a dbrev to put existing values in line. And a simple test in
Auth.t to confirm that login_attempts stop increasing.

Note: It feels safe to keep the '>=' condition in account_locked. But it
could obviously be changed to '=='. (Added a test for that.)

Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all
tests will pass when the pref is enabled (though disabled by default).

Test plan:
Run dbrev with updatedatabase.pl.
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87390 [details] [review]
Bug 21336: Introduce administrative lockout

As a preparation for Koha::Patron->lock, we add the concept of administrative
lockout. The account is locked just as it would have been by too much
failed login attempts.
This is handled by a negative value in borrowers.login_attempts.

Test plan:
Run t/db_dependent/Auth.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87391 [details] [review]
Bug 21336: Search, lock and anonymize methods

Add Koha::Patron->lock and anonymize.
Add Koha::Patrons methods search_unsubscribed, search_anonymize_candidates
and search_anonymized. And wrappers for lock and anonymize.
Add unit tests.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87392 [details] [review]
Bug 21336: Adjust cleanup_database.pl

Add the new Patron routines to this cron job. Actions are performed only if
preferences are set.

Note: No specific command line flags for these actions are added here
(and probably not needed too). So no crontab changes too.

Test plan:
Add a new patron.
Enable GDPR_Policy and refuse consent on OPAC for this patron.
Set only the first delay to zero (0) for immediate action.
Run cleanup_database.pl --logs (or any other flag) for the first time.
Check lock and expiration.
Set the second delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the second time. Check anonymization.
Set the third delay to zero (0) for immediate action.
Run cleanup_database.pl --logs for the third time. Check removal.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the warn $@ line in cleanup_database.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87393 [details] [review]
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87394 [details] [review]
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column

When a field is not nullable we should pass empty string, zero or today
to a char, numeric or date column.
The mandatory parameter does not refer to a database constraint but a Koha
preference. Only for strings we generate a random value.

Test plan:
Enable strict_sql_modes.
Run t/db_dependent/Koha/Patrons.t again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87395 [details] [review]
Bug 21336: (QA follow-up) Add column_exists in db revision

As requested by Josef on comment70.

Test plan:
Run db revision twice. No warnings.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87396 [details] [review]
Bug 21336: (QA follow-up) Add tests for administrative lockout

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87397 [details] [review]
Bug 21336: [DO_NOT_PUSH] DBIx changes

MariaDB somehow turns current_timestamp into current_timestamp().
The needed change is for column flgAnonymized.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

OK, I added a QA followup for that final failure..

No regressions found, QA script is happy and tests are all passing.

Passing QA

Created attachment 87400 [details] [review]
Bug 21336: (QA follow-up) Add tests for administrative lockout

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 87401 [details] [review]
Bug 21336: [DO_NOT_PUSH] DBIx changes

MariaDB somehow turns current_timestamp into current_timestamp().
The needed change is for column flgAnonymized.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

(In reply to Martin Renvoize from comment #111)
> OK, I added a QA followup for that final failure..
> 
> No regressions found, QA script is happy and tests are all passing.
> 
> Passing QA

Thanks Martin !

Awesome work all!

Pushed to master for 19.05

One note here: When the anonymization is run, the changes are captured in the action_logs - so you get a mapping from the old values to the new values - maybe we need a way to not log this change?

(In reply to Nick Clemens from comment #116)
> One note here: When the anonymization is run, the changes are captured in
> the action_logs - so you get a mapping from the old values to the new values
> - maybe we need a way to not log this change?

Wow.. good catch.. I missed that entirely.  Probably a very sensible thing to clean up.. perhaps we need to log a new bug for it

(In reply to Nick Clemens from comment #116)
> One note here: When the anonymization is run, the changes are captured in
> the action_logs - so you get a mapping from the old values to the new values
> - maybe we need a way to not log this change?

Will have a look at that. Thx

Created attachment 88217 [details] [review]
Bug 21336: Fix the API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 88218 [details] [review]
Bug 21336: Add borrowers.flgAnonymized in an explicit position

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Moving the patches that rename the column to a separate bug for proper discussion in context. Keeping a minor fix on the schema and fixes to the API.

@RM: I tested with and without my updatedatabase.pl patch and the results are the same. It is just safer, just in case someone backports in the wrong order.

(In reply to Tomás Cohen Arazi from comment #123)
> Moving the patches that rename the column to a separate bug for proper
> discussion in context. Keeping a minor fix on the schema and fixes to the
> API.
> 
> @RM: I tested with and without my updatedatabase.pl patch and the results
> are the same. It is just safer, just in case someone backports in the wrong
> order.

Last two patches pushed to aster for 19.05

Enhancement will not be backported to 18.11.x series.

(In reply to Marcel de Rooy from comment #72)
> (In reply to Jonathan Druart from comment #71)
> 
> > I am not sure about that, it sounds better to me to have another attribute.
> 
> I would strongly prefer to do that on another report now.

Should we discuss that now then?

(In reply to Jonathan Druart from comment #126)
> Should we discuss that now then?
Moved to bug 22854

Got the following error during an upgrade:

DBD::mysql::db do failed: Truncated incorrect DOUBLE value: '' [for Statement "UPDATE borrowers SET login_attempts = ? WHERE login_attempts > ?"] at installer/data/mysql/updatedatabase.pl line 18091.


Please fix, see bug 23867.

I do not see a good reason to warn from the module, the script must done that job. Especially because misc/cronjobs/cleanup_database.pl uses print (STDOUT).

Can you explain this choice?

(In reply to Jonathan Druart from comment #129)
> I do not see a good reason to warn from the module, the script must done
> that job. Especially because misc/cronjobs/cleanup_database.pl uses print
> (STDOUT).
> 
> Can you explain this choice?

Probably simpler in the first place. And if this is indeed about the existing issues warn, it is not just an informational print line.
Surely, it can be done differently. In other places too?

(In reply to Marcel de Rooy from comment #130)
> (In reply to Jonathan Druart from comment #129)
> > I do not see a good reason to warn from the module, the script must done
> > that job. Especially because misc/cronjobs/cleanup_database.pl uses print
> > (STDOUT).
> > 
> > Can you explain this choice?
> 
> Probably simpler in the first place. And if this is indeed about the
> existing issues warn, it is not just an informational print line.
> Surely, it can be done differently. In other places too?

Which other places? I do not think there were any before this patchset.

I have opened bug 24114 to deal with the verbose flag.

(In reply to Jonathan Druart from comment #131)
> (In reply to Marcel de Rooy from comment #130)
> > (In reply to Jonathan Druart from comment #129)
> > > I do not see a good reason to warn from the module, the script must done
> > > that job. Especially because misc/cronjobs/cleanup_database.pl uses print
> > > (STDOUT).
> > > 
> > > Can you explain this choice?
> > 
> > Probably simpler in the first place. And if this is indeed about the
> > existing issues warn, it is not just an informational print line.
> > Surely, it can be done differently. In other places too?
> 
> Which other places? I do not think there were any before this patchset.
> 
> I have opened bug 24114 to deal with the verbose flag.

Answer on the new report.