Bug 21336 - GDPR: Handle unsubscribe requests automatically by optional (administrative) lock, anonymize and remove
Summary: GDPR: Handle unsubscribe requests automatically by optional (administrative) ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords: Manual
Depends on: 20819 21337 21998
Blocks: 18081 21191 21712 21312 21533 21535 22729
  Show dependency treegraph
 
Reported: 2018-09-12 11:25 UTC by Marcel de Rooy
Modified: 2019-05-22 14:47 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Add preferences UnsubscribeReflectionDelay, PatronAnonymizeDelay and PatronRemovalDelay. Add db column borrowers.flgAnonymized. Add Koha::Patron->lock for administrative lockout. Add Koha::Patron->anonymize for scrambling patron data. Actions are controlled by preferences and run by cleanup_database cron job.
Version(s) released in:
19.05.00


Attachments
Bug 21336: Small adjustment in Koha::Token (2.17 KB, patch)
2018-09-20 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.53 KB, patch)
2018-09-20 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: DBIx schema changes (3.00 KB, patch)
2018-09-20 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.47 KB, patch)
2018-09-20 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (16.15 KB, patch)
2018-09-20 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (3.56 KB, patch)
2018-09-20 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Small adjustment in Koha::Token (2.17 KB, patch)
2018-10-08 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.53 KB, patch)
2018-10-08 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: DBIx schema changes (3.00 KB, patch)
2018-10-08 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.47 KB, patch)
2018-10-08 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.63 KB, patch)
2018-10-08 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.59 KB, patch)
2018-10-08 08:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.76 KB, patch)
2018-10-08 08:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.26 KB, patch)
2018-10-08 08:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.79 KB, patch)
2018-10-08 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.26 KB, patch)
2018-10-08 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.33 KB, patch)
2018-10-11 08:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.33 KB, patch)
2018-10-11 08:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.34 KB, patch)
2018-10-11 12:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Small adjustment in Koha::Token (2.17 KB, patch)
2018-10-25 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.53 KB, patch)
2018-10-25 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: DBIx schema changes (3.00 KB, patch)
2018-10-25 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.46 KB, patch)
2018-10-25 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.63 KB, patch)
2018-10-25 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.59 KB, patch)
2018-10-25 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.81 KB, patch)
2018-10-25 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.38 KB, patch)
2018-10-25 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.40 KB, patch)
2018-10-26 10:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.40 KB, patch)
2018-10-29 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Small adjustment in Koha::Token (2.17 KB, patch)
2018-10-29 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.53 KB, patch)
2018-10-29 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: DBIx changes (2.88 KB, patch)
2018-10-29 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.46 KB, patch)
2018-10-29 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.63 KB, patch)
2018-10-29 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.59 KB, patch)
2018-10-29 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.81 KB, patch)
2018-10-29 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.38 KB, patch)
2018-10-29 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.40 KB, patch)
2018-10-29 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Small adjustment in Koha::Token (2.17 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.53 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: DBIx changes (2.88 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.46 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.61 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.59 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.81 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.38 KB, patch)
2018-11-08 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.40 KB, patch)
2018-11-08 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.53 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: DBIx changes (2.88 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.46 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.61 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.59 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.81 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.38 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.40 KB, patch)
2018-12-14 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column (2.33 KB, patch)
2018-12-17 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.54 KB, patch)
2019-03-29 08:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.48 KB, patch)
2019-03-29 08:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.61 KB, patch)
2019-03-29 08:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.60 KB, patch)
2019-03-29 08:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.66 KB, patch)
2019-03-29 08:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.40 KB, patch)
2019-03-29 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.40 KB, patch)
2019-03-29 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column (2.33 KB, patch)
2019-03-29 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: [DO_NOT_PUSH] DBIx changes (3.96 KB, patch)
2019-03-29 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: (QA follow-up) Add column_exists in db revision (1.44 KB, patch)
2019-03-29 08:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: [DO_NOT_PUSH] DBIx changes (3.96 KB, patch)
2019-03-29 08:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.56 KB, patch)
2019-03-29 10:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.50 KB, patch)
2019-03-29 10:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.62 KB, patch)
2019-03-29 10:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.62 KB, patch)
2019-03-29 10:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.67 KB, patch)
2019-03-29 10:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.41 KB, patch)
2019-03-29 10:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.42 KB, patch)
2019-03-29 10:56 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column (2.35 KB, patch)
2019-03-29 10:56 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: (QA follow-up) Add column_exists in db revision (1.46 KB, patch)
2019-03-29 10:56 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: [DO_NOT_PUSH] DBIx changes (3.98 KB, patch)
2019-03-29 10:56 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21336: Database revision for borrowers.flgAnonymized (2.62 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: Dbrev for new prefs (5.54 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: Do not increase login_attempts after locking (5.67 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: Introduce administrative lockout (3.68 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: Search, lock and anonymize methods (15.72 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: Adjust cleanup_database.pl (4.48 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: (follow-up) Fix last line after PatronRemovalDelay in pref file (1.48 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: (follow-up) Handle strict SQL mode in _anonymize_column (2.41 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: (QA follow-up) Add column_exists in db revision (1.52 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: (QA follow-up) Add tests for administrative lockout (1.98 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: [DO_NOT_PUSH] DBIx changes (4.04 KB, patch)
2019-04-04 08:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: (QA follow-up) Add tests for administrative lockout (2.04 KB, patch)
2019-04-04 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: [DO_NOT_PUSH] DBIx changes (4.04 KB, patch)
2019-04-04 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21336: (QA follow-up) Rename flgAnonymized column for anonymized (11.24 KB, patch)
2019-04-17 14:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21336: Fix the API (1.29 KB, patch)
2019-04-17 14:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21336: Fix the API (3.62 KB, patch)
2019-04-17 17:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21336: Add borrowers.flgAnonymized in an explicit position (1.54 KB, patch)
2019-04-17 17:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-09-12 11:25:18 UTC
This should be a follow-up of bug 20819 (adding a consent form).
A script (preferably cleanup_database) should take care of the registered refusals (or unsubscribe requests).
Based on some preferences and corresponding time frames, we should lock accounts, anonymize patron data and finally remove patron data.
Comment 1 Marcel de Rooy 2018-09-20 12:36:17 UTC
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>
Comment 2 Marcel de Rooy 2018-09-20 12:36:23 UTC
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>
Comment 3 Marcel de Rooy 2018-09-20 12:36:27 UTC
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>
Comment 4 Marcel de Rooy 2018-09-20 12:36:31 UTC
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>
Comment 5 Marcel de Rooy 2018-09-20 12:36:35 UTC
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>
Comment 6 Marcel de Rooy 2018-09-20 12:36:40 UTC
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>
Comment 7 Marcel de Rooy 2018-09-28 05:38:09 UTC
Still needs some attention. Coming back soon
Comment 8 Marcel de Rooy 2018-10-08 08:35:41 UTC
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>
Comment 9 Marcel de Rooy 2018-10-08 08:35:46 UTC
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>
Comment 10 Marcel de Rooy 2018-10-08 08:35:50 UTC
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>
Comment 11 Marcel de Rooy 2018-10-08 08:35:55 UTC
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>
Comment 12 Marcel de Rooy 2018-10-08 08:35:59 UTC
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>
Comment 13 Marcel de Rooy 2018-10-08 08:36:04 UTC
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
Comment 14 Marcel de Rooy 2018-10-08 08:36:09 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2018-10-08 08:36:14 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2018-10-08 09:08:49 UTC
(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.
Comment 17 Michal Denar 2018-10-08 09:25:43 UTC
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
Comment 18 Marcel de Rooy 2018-10-08 12:52:24 UTC
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>
Comment 19 Marcel de Rooy 2018-10-08 12:52:29 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2018-10-08 12:54:20 UTC
Made the test search_anonymized a tiny bit smarter :)
Just changed two lines..
Comment 21 Marcel de Rooy 2018-10-08 12:54:46 UTC
(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
Comment 22 M. Tompsett 2018-10-10 15:02:48 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2018-10-11 08:11:23 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2018-10-11 08:12:09 UTC
(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 $@
Comment 25 Marcel de Rooy 2018-10-11 08:13:06 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2018-10-11 12:13:00 UTC
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
Comment 27 Marcel de Rooy 2018-10-25 08:38:31 UTC
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>
Comment 28 Marcel de Rooy 2018-10-25 08:38:36 UTC
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>
Comment 29 Marcel de Rooy 2018-10-25 08:38:41 UTC
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>
Comment 30 Marcel de Rooy 2018-10-25 08:38:45 UTC
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>
Comment 31 Marcel de Rooy 2018-10-25 08:38:50 UTC
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>
Comment 32 Marcel de Rooy 2018-10-25 08:38:56 UTC
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
Comment 33 Marcel de Rooy 2018-10-25 08:39:00 UTC
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>
Comment 34 Marcel de Rooy 2018-10-25 08:39:07 UTC
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
Comment 35 Marcel de Rooy 2018-10-25 08:39:52 UTC
Trivial rebase
Comment 36 Marcel de Rooy 2018-10-26 10:22:09 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2018-10-29 12:34:22 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2018-10-29 12:34:42 UTC
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>
Comment 39 Marcel de Rooy 2018-10-29 12:34:46 UTC
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>
Comment 40 Marcel de Rooy 2018-10-29 12:34:51 UTC
Created attachment 81477 [details] [review]
Bug 21336: DBIx changes
Comment 41 Marcel de Rooy 2018-10-29 12:34:56 UTC
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>
Comment 42 Marcel de Rooy 2018-10-29 12:35:01 UTC
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>
Comment 43 Marcel de Rooy 2018-10-29 12:35:07 UTC
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
Comment 44 Marcel de Rooy 2018-10-29 12:35:12 UTC
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>
Comment 45 Marcel de Rooy 2018-10-29 12:35:17 UTC
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
Comment 46 Marcel de Rooy 2018-10-29 12:35:22 UTC
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>
Comment 47 Marcel de Rooy 2018-10-29 13:08:03 UTC
(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.
Comment 48 Marcel de Rooy 2018-10-29 13:28:21 UTC
(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
Comment 49 Marcel de Rooy 2018-11-08 07:56:16 UTC
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>
Comment 50 Marcel de Rooy 2018-11-08 07:56:22 UTC
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>
Comment 51 Marcel de Rooy 2018-11-08 07:56:27 UTC
Created attachment 82053 [details] [review]
Bug 21336: DBIx changes
Comment 52 Marcel de Rooy 2018-11-08 07:56:33 UTC
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>
Comment 53 Marcel de Rooy 2018-11-08 07:56:38 UTC
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>
Comment 54 Marcel de Rooy 2018-11-08 07:56:45 UTC
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
Comment 55 Marcel de Rooy 2018-11-08 07:56:51 UTC
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>
Comment 56 Marcel de Rooy 2018-11-08 07:56:57 UTC
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
Comment 57 Marcel de Rooy 2018-11-08 07:57:04 UTC
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>
Comment 58 Marcel de Rooy 2018-12-14 08:54:46 UTC
First patch moved to 21998
Comment 59 Marcel de Rooy 2018-12-14 08:56:23 UTC
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>
Comment 60 Marcel de Rooy 2018-12-14 08:56:28 UTC
Created attachment 83203 [details] [review]
Bug 21336: DBIx changes
Comment 61 Marcel de Rooy 2018-12-14 08:56:32 UTC
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>
Comment 62 Marcel de Rooy 2018-12-14 08:56:38 UTC
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>
Comment 63 Marcel de Rooy 2018-12-14 08:56:42 UTC
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
Comment 64 Marcel de Rooy 2018-12-14 08:56:47 UTC
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>
Comment 65 Marcel de Rooy 2018-12-14 08:56:52 UTC
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
Comment 66 Marcel de Rooy 2018-12-14 08:56:56 UTC
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>
Comment 67 Marcel de Rooy 2018-12-14 08:57:33 UTC
Rebased
Comment 68 Marcel de Rooy 2018-12-14 11:26:34 UTC
Issue in anonymize with strict sql mode
Comment 69 Marcel de Rooy 2018-12-17 08:32:17 UTC
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>
Comment 70 Josef Moravec 2018-12-18 11:17:47 UTC
Just one small think:

database update should use column_exists on adding flgAnonymized column

but otherwise looking good for me...
Comment 71 Jonathan Druart 2019-02-16 17:51:27 UTC
(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.
Comment 72 Marcel de Rooy 2019-03-29 08:09:44 UTC
(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.
Comment 73 Marcel de Rooy 2019-03-29 08:22:36 UTC
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>
Comment 74 Marcel de Rooy 2019-03-29 08:22:42 UTC
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>
Comment 75 Marcel de Rooy 2019-03-29 08:22:47 UTC
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>
Comment 76 Marcel de Rooy 2019-03-29 08:22:52 UTC
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
Comment 77 Marcel de Rooy 2019-03-29 08:22:57 UTC
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>
Comment 78 Marcel de Rooy 2019-03-29 08:23:02 UTC
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
Comment 79 Marcel de Rooy 2019-03-29 08:23:07 UTC
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>
Comment 80 Marcel de Rooy 2019-03-29 08:23:12 UTC
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>
Comment 81 Marcel de Rooy 2019-03-29 08:23:18 UTC Comment hidden (obsolete)
Comment 82 Marcel de Rooy 2019-03-29 08:34:16 UTC
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>
Comment 83 Marcel de Rooy 2019-03-29 08:34:23 UTC
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.
Comment 84 Marcel de Rooy 2019-03-29 08:41:01 UTC
(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.
Comment 85 Josef Moravec 2019-03-29 10:55:36 UTC
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>
Comment 86 Josef Moravec 2019-03-29 10:55:41 UTC
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>
Comment 87 Josef Moravec 2019-03-29 10:55:46 UTC
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>
Comment 88 Josef Moravec 2019-03-29 10:55:50 UTC
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>
Comment 89 Josef Moravec 2019-03-29 10:55:55 UTC
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>
Comment 90 Josef Moravec 2019-03-29 10:55:59 UTC
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>
Comment 91 Josef Moravec 2019-03-29 10:56:04 UTC
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>
Comment 92 Josef Moravec 2019-03-29 10:56:09 UTC
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>
Comment 93 Josef Moravec 2019-03-29 10:56:13 UTC
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>
Comment 94 Josef Moravec 2019-03-29 10:56:17 UTC
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>
Comment 95 Martin Renvoize 2019-03-29 14:17:42 UTC
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?
Comment 96 Marcel de Rooy 2019-04-01 07:38:47 UTC
(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 97 Martin Renvoize 2019-04-04 08:21:04 UTC
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...
Comment 98 Martin Renvoize 2019-04-04 08:21:54 UTC
Ignore that.. I somehow missed it in the qa followup.
Comment 99 Martin Renvoize 2019-04-04 08:35:39 UTC
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)
Comment 100 Martin Renvoize 2019-04-04 08:46:13 UTC
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>
Comment 101 Martin Renvoize 2019-04-04 08:46:17 UTC
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>
Comment 102 Martin Renvoize 2019-04-04 08:46:21 UTC
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>
Comment 103 Martin Renvoize 2019-04-04 08:46:25 UTC
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>
Comment 104 Martin Renvoize 2019-04-04 08:46:29 UTC
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>
Comment 105 Martin Renvoize 2019-04-04 08:46:33 UTC
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>
Comment 106 Martin Renvoize 2019-04-04 08:46:36 UTC
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>
Comment 107 Martin Renvoize 2019-04-04 08:46:40 UTC
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>
Comment 108 Martin Renvoize 2019-04-04 08:46:44 UTC
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>
Comment 109 Martin Renvoize 2019-04-04 08:46:48 UTC
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>
Comment 110 Martin Renvoize 2019-04-04 08:46:52 UTC
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>
Comment 111 Martin Renvoize 2019-04-04 08:47:51 UTC
OK, I added a QA followup for that final failure..

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

Passing QA
Comment 112 Martin Renvoize 2019-04-04 08:53:26 UTC
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>
Comment 113 Martin Renvoize 2019-04-04 08:53:30 UTC
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>
Comment 114 Marcel de Rooy 2019-04-04 09:13:21 UTC
(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 !
Comment 115 Nick Clemens 2019-04-17 12:27:13 UTC
Awesome work all!

Pushed to master for 19.05
Comment 116 Nick Clemens 2019-04-17 12:31:58 UTC
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?
Comment 117 Martin Renvoize 2019-04-17 12:38:50 UTC
(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
Comment 118 Marcel de Rooy 2019-04-17 13:10:08 UTC
(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
Comment 119 Tomás Cohen Arazi 2019-04-17 14:32:19 UTC Comment hidden (obsolete)
Comment 120 Tomás Cohen Arazi 2019-04-17 14:32:26 UTC Comment hidden (obsolete)
Comment 121 Tomás Cohen Arazi 2019-04-17 17:39:51 UTC
Created attachment 88217 [details] [review]
Bug 21336: Fix the API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 122 Tomás Cohen Arazi 2019-04-17 17:39:57 UTC
Created attachment 88218 [details] [review]
Bug 21336: Add borrowers.flgAnonymized in an explicit position

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 123 Tomás Cohen Arazi 2019-04-17 17:41:58 UTC
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.
Comment 124 Nick Clemens 2019-04-18 09:51:26 UTC
(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
Comment 125 Martin Renvoize 2019-04-26 12:01:57 UTC
Enhancement will not be backported to 18.11.x series.
Comment 126 Jonathan Druart 2019-05-03 13:21:56 UTC
(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?
Comment 127 Marcel de Rooy 2019-05-06 06:47:36 UTC
(In reply to Jonathan Druart from comment #126)
> Should we discuss that now then?
Moved to bug 22854