Bug 28935 - No filtering on patron's data on member entry pages (OPAC, self registration, staff interface)
Summary: No filtering on patron's data on member entry pages (OPAC, self registration,...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens
URL:
Keywords:
Depends on: 28929
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-01 12:23 UTC by Jonathan Druart
Modified: 2021-09-30 07:57 UTC (History)
22 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04, 20.11.10, 20.05.16, 19.11.22


Attachments
Bug 28935: Filter user input through allowlist for patrons (11.27 KB, patch)
2021-09-01 12:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Centralize the allow list (12.10 KB, patch)
2021-09-01 12:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Enforce *UnwantedField sysprefs (4.51 KB, patch)
2021-09-01 13:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Allow gonenoaddress and lost for staff (1.08 KB, patch)
2021-09-02 06:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Allow gonenoaddress, lost and relationship for staff (1.19 KB, patch)
2021-09-02 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Move logic to Koha::Allowlist for reusability (9.66 KB, patch)
2021-09-02 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Take borrowernumber from the original input (1.05 KB, patch)
2021-09-02 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28935: Filter user input through allow list (14.71 KB, patch)
2021-09-03 06:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Fix module in memberentry and chmod Allowlist.t (1.02 KB, patch)
2021-09-03 06:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (11.20 KB, patch)
2021-09-03 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (14.04 KB, patch)
2021-09-03 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Apply modules change to controller scripts (2.82 KB, patch)
2021-09-03 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (14.04 KB, patch)
2021-09-03 13:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Apply modules change to controller scripts (2.62 KB, patch)
2021-09-03 13:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Filter user input through allow list (14.77 KB, patch)
2021-09-03 17:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Fix module in memberentry and chmod Allowlist.t (1.07 KB, patch)
2021-09-03 17:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (13.98 KB, patch)
2021-09-03 17:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Apply module changes to controller scripts (3.33 KB, patch)
2021-09-03 17:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Add logging to controller scripts (3.22 KB, patch)
2021-09-03 17:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Filter user input through allowlist for patrons (11.36 KB, patch)
2021-09-06 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Centralize the allow list (12.18 KB, patch)
2021-09-06 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Enforce *UnwantedField sysprefs (4.60 KB, patch)
2021-09-06 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Allow gonenoaddress, lost and relationship for staff (1.28 KB, patch)
2021-09-06 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Move logic to Koha::Allowlist for reusability (9.74 KB, patch)
2021-09-06 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Take borrowernumber from the original input (1.14 KB, patch)
2021-09-06 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Fix module in memberentry and chmod Allowlist.t (1.07 KB, patch)
2021-09-06 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (13.98 KB, patch)
2021-09-06 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Apply module changes to controller scripts (3.33 KB, patch)
2021-09-06 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Add logging to controller scripts (3.22 KB, patch)
2021-09-06 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: Filter user input through allowlist for patrons (11.37 KB, patch)
2021-09-08 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Centralize the allow list (12.20 KB, patch)
2021-09-08 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Enforce *UnwantedField sysprefs (4.62 KB, patch)
2021-09-08 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Allow gonenoaddress, lost and relationship for staff (1.30 KB, patch)
2021-09-08 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Move logic to Koha::Allowlist for reusability (9.76 KB, patch)
2021-09-08 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Take borrowernumber from the original input (1.16 KB, patch)
2021-09-08 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Fix module in memberentry and chmod Allowlist.t (1.09 KB, patch)
2021-09-08 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (14.00 KB, patch)
2021-09-08 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Apply module changes to controller scripts (3.35 KB, patch)
2021-09-08 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Add logging to controller scripts (3.24 KB, patch)
2021-09-08 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: (follow-up) Correct attribute test (983 bytes, patch)
2021-09-08 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: Filter user input through allowlist for patrons (11.44 KB, patch)
2021-09-10 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Centralize the allow list (12.26 KB, patch)
2021-09-10 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Enforce *UnwantedField sysprefs (4.68 KB, patch)
2021-09-10 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Allow gonenoaddress, lost and relationship for staff (1.36 KB, patch)
2021-09-10 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Move logic to Koha::Allowlist for reusability (9.82 KB, patch)
2021-09-10 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Take borrowernumber from the original input (1.22 KB, patch)
2021-09-10 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Fix module in memberentry and chmod Allowlist.t (1.15 KB, patch)
2021-09-10 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Rename get_ui_fields, introduce add and remove (14.06 KB, patch)
2021-09-10 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Apply module changes to controller scripts (3.41 KB, patch)
2021-09-10 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: Add logging to controller scripts (3.30 KB, patch)
2021-09-10 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: (follow-up) Correct attribute test (1.02 KB, patch)
2021-09-10 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28935: No filtering on patron's data on member entry pages [SIMPLE] (2.59 KB, patch)
2021-09-10 15:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: No filtering on patron's data on member entry pages (3.16 KB, patch)
2021-09-10 17:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: No filtering on patron's data on member entry pages (3.17 KB, patch)
2021-09-13 12:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: (follow-up) Use BorrowerUnwantedField on staff client (1.08 KB, patch)
2021-09-13 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28935: No filtering on patron's data on member entry pages (3.22 KB, patch)
2021-09-13 13:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28935: (follow-up) Use BorrowerUnwantedField on staff client (1.08 KB, patch)
2021-09-13 13:20 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28935: No filtering on patron's data on member entry pages (3.26 KB, patch)
2021-09-13 13:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28935: (QA follow-up) Use BorrowerUnwantedField on staff client (1.18 KB, patch)
2021-09-13 13:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-09-01 12:23:38 UTC
Same as bug 28929 for other data.
Comment 1 Jonathan Druart 2021-09-01 12:27:24 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2021-09-01 12:27:28 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2021-09-01 12:29:35 UTC
From bug 28929 comment 26

(In reply to Andreas Jonsson from bug 28929 comment #23)
> I think that the apply function should throw an exception if the input
> parameters are incorrect.  As it stands, it will silently allow everything
> if this is the case.
> 
>     if ($ui_fields && ref $ui_fields eq 'HASH'){
>         ...
> 
>         if ( $input && ref $input eq 'HASH' ){
>            ...

It depends on what we want this function to do.
Either it's a filter, or a confirmation we did things correctly prior to its call.

I have added a warn there and it highlights the problem: we now have a lot of warnings in the logs.


[2021/09/01 08:48:09] [WARN] Forbidden - Tried to modify 'dateexpiry' with '2029-12-01' from CGI::Compile::ROOT::kohadevbox_koha_opac_opac_2dmemberentry_2epl /kohadevbox/koha/opac/opac-memberentry.pl 277 at /kohadevbox/koha/Koha/Patron/Allowlist.pm line 56.
Comment 4 Jonathan Druart 2021-09-01 13:18:10 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2021-09-01 13:41:24 UTC
Have to say that we are adding a lot of code for two lines (staff and OPAC variant):

delete $borrower{$_} for qw/      borrowernumber      date_renewed      gonenoaddress      lost      debarred      debarredcomment      relationship      flags      privacy      privacy_guarantor_fines      privacy_guarantor_checkouts      checkprevcheckout      updated_on      lastseen      lang      login_attempts      overdrive_auth_token      anonymized      dateenrolled      dateexpiry      borrowernotes      opacnote      sort1      sort2      sms_provider_id      autorenew_checkouts/;

Note that fields not in the borrower table are "filtered" by DBIx ;) No big deal.
Comment 6 Jonathan Druart 2021-09-01 14:02:53 UTC
I like how I added the UnwantedField check in the second patch. I could have been "only 2 more lines", but you duplicate the logic.

Moreover I think that having the list of fields in only one place is a good thing, it would be a mess to have it in different places.
And there are tests ;)

The code is pretty trivial.
Comment 7 Marcel de Rooy 2021-09-01 14:29:07 UTC
We need a change like this to prevent the Cardnumber already exists error:

diff --git a/members/memberentry.pl b/members/memberentry.pl
index 23bbe65..88387c8 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -326,7 +326,7 @@ if ($op eq 'save' || $op eq 'insert'){
     # If the cardnumber is blank, treat it as null.
     $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/;

-    if (my $error_code = checkcardnumber($newdata{cardnumber},$newdata{borrowernumber})){
+    if (my $error_code = checkcardnumber( $newdata{cardnumber}, $borrowernumber )){
         push @errors, $error_code == 1
             ? 'ERROR_cardnumber_already_exists'
             : $error_code == 2
Comment 8 David Cook 2021-09-01 23:36:59 UTC
Comment on attachment 124327 [details] [review]
Bug 28935: Centralize the allow list

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

::: Koha/Patron/Allowlist.pm
@@ +159,5 @@
> +      primary_contact_method
> +    );
> +}
> +
> +sub _global_deny_list {

There are a few problems with this list.

Staff should be allowed to change at least the following: gonenoaddress, lost, relationship directly through the memberentry.pl script. We'll need to remove those from here and add them to the public denylist. 

(This is one of the things I don't like about denylists. We have to remember to make 2 changes: one to the allow list and one to the deny list. Whereas with only allowlists, there is less risk of forgetting to subtract authorization. If authorization isn't added, it'll be obvious during testing/use and better to error on the side of caution I think.)

(Fwiw I missed relationship in my initial list as well, since it's an atypical field used mostly by child users.)
Comment 9 Jonathan Druart 2021-09-02 06:36:36 UTC
(In reply to David Cook from comment #8)
> Comment on attachment 124327 [details] [review] [review]
> Bug 28935: Centralize the allow list
> 
> Review of attachment 124327 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Patron/Allowlist.pm
> @@ +159,5 @@
> > +      primary_contact_method
> > +    );
> > +}
> > +
> > +sub _global_deny_list {
> 
> There are a few problems with this list.
> 
> Staff should be allowed to change at least the following: gonenoaddress,
> lost, relationship directly through the memberentry.pl script. We'll need to
> remove those from here and add them to the public denylist. 
> 
> (This is one of the things I don't like about denylists. We have to remember
> to make 2 changes: one to the allow list and one to the deny list. Whereas
> with only allowlists, there is less risk of forgetting to subtract
> authorization. If authorization isn't added, it'll be obvious during
> testing/use and better to error on the side of caution I think.)
> 
> (Fwiw I missed relationship in my initial list as well, since it's an
> atypical field used mostly by child users.)

Indeed, gonenoaddress and lost are missing. Note that contacttitle and sms_provider_id were missing from the first patch.
That was a bit of a mess to compare the two lists. And that's why I think it's better with 1. the whole list of fields, 2. a global deny list, and 3. a specific deny list per interface.
Moreover the fields are listed identically as how it's in the DB (and tests prove that).
To me it's a real win for maintainability. As well for the readability but that is subjective apparently.
Comment 10 Jonathan Druart 2021-09-02 06:40:19 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2021-09-02 06:55:00 UTC
(In reply to David Cook from comment #8)
> Staff should be allowed to change at least the following: gonenoaddress,
> lost, relationship directly through the memberentry.pl script. We'll need to
> remove those from here and add them to the public denylist. 

Good point!
Comment 12 Marcel de Rooy 2021-09-02 06:56:31 UTC
(In reply to Jonathan Druart from comment #10)
> Created attachment 124416 [details] [review] [review]
> Bug 28935: Allow gonenoaddress and lost for staff

And relationship?
You will see patron guarantor if the patron type is Child etc.
Comment 13 Jonathan Druart 2021-09-02 07:16:25 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2021-09-02 08:35:08 UTC
So... Opened bug 28941. And we need to reuse Koha::Patron::Allowlist earlier than expected, unfortunately.

It must be moved to be reused, suggestions:

1. Koha::Allowlist::{Patron,Suggestion}
Seems weird but maybe the best place

2. Koha::Object::{Patron,Suggestion}
Seems bad, we could use it for something else than object. But we could have the lists in/under Koha::{Patron,Suggestion}, which seems convenient and correct.

3. Koha::Util::Hashref
Didn't we ban 'Util'?

4. Koha::Filter::Hashref
Could do the logic of filtering, and we would pass the list of fields from Koha::Patron->columns_editable_{staff,opac}

5. Something else to suggest?
Comment 15 Marcel de Rooy 2021-09-02 08:49:39 UTC
(In reply to Jonathan Druart from comment #14)
> So... Opened bug 28941. And we need to reuse Koha::Patron::Allowlist earlier
> than expected, unfortunately.
> 
> It must be moved to be reused, suggestions:
> 
> 1. Koha::Allowlist::{Patron,Suggestion}
> Seems weird but maybe the best place
> 
> 2. Koha::Object::{Patron,Suggestion}
> Seems bad, we could use it for something else than object. But we could have
> the lists in/under Koha::{Patron,Suggestion}, which seems convenient and
> correct.
> 
> 3. Koha::Util::Hashref
> Didn't we ban 'Util'?
> 
> 4. Koha::Filter::Hashref
> Could do the logic of filtering, and we would pass the list of fields from
> Koha::Patron->columns_editable_{staff,opac}
> 
> 5. Something else to suggest?

Koha::Filter as the parent class?
Subclass it for patrons under Koha::Patron::Filter ?
Koha::Suggestion::Filter 
etc

Filter might be just a bit too vague.
What you want, is filter or validate parameters from outside.
Something with taint?
Comment 16 Marcel de Rooy 2021-09-02 08:51:40 UTC
Note that I would not mind merging opac and staff in the same class. The current distinction is a bit articifial?
Comment 17 Jonathan Druart 2021-09-02 09:39:21 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2021-09-02 09:48:04 UTC
I think that this is not enough. See comment7.
Could be mistaken. But I needed that change to get my minimalistic version of this report working on 19.11.. Since we clear borrowernumber.

diff --git a/members/memberentry.pl b/members/memberentry.pl
index 6b63ee7650b..0fc3d2b022b 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -48,7 +48,7 @@ use Email::Valid;
 use Koha::SMS::Providers;
 use Koha::Patron::Allowlist::Staff;
 
-my $ui_allowlist = Koha::Patron::Allowlist::Staff->new();
+my $ui_allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' });
 
 
 my $input = CGI->new;
Comment 19 Jonathan Druart 2021-09-02 09:58:41 UTC
(In reply to Marcel de Rooy from comment #18)
> I think that this is not enough. See comment7.
> Could be mistaken. But I needed that change to get my minimalistic version
> of this report working on 19.11.. Since we clear borrowernumber.

Sorry, I thought I fixed it yesterday.

We moved the patches from bug 28929 to let it be as minimalist as possible. The intention of this bug was to deal correctly with the different fields, not to still be minimalist.
Comment 20 Jonathan Druart 2021-09-02 10:00:37 UTC
Created attachment 124425 [details] [review]
Bug 28935: Take borrowernumber from the original input

newdata is been modified, and filtered. We want to pick the original
value of borrowernumber.
Comment 21 Marcel de Rooy 2021-09-02 10:00:48 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Marcel de Rooy from comment #18)
> > I think that this is not enough. See comment7.
> > Could be mistaken. But I needed that change to get my minimalistic version
> > of this report working on 19.11.. Since we clear borrowernumber.
> 
> Sorry, I thought I fixed it yesterday.
> 
> We moved the patches from bug 28929 to let it be as minimalist as possible.
> The intention of this bug was to deal correctly with the different fields,
> not to still be minimalist.

No problem with that, although I preferred to backport rightaway in a minimalistic approach locally.
Comment 22 Jonathan Druart 2021-09-02 10:02:17 UTC
(In reply to Jonathan Druart from comment #17)
> Created attachment 124422 [details] [review] [review]
> Bug 28935: Move logic to Koha::Allowlist for reusability

Note that I finally didn't use Koha::Allowlist on bug 28941. But this is still needed in my opinion.
Comment 23 David Cook 2021-09-03 06:08:20 UTC
Koha::Allowlist

If we're going to go with "Koha::Allowlist", we should probably rename "get_ui_fields" to something more generic like "get_entries". My intent around "Koha::Patron::Allowlist" was to keep the scope of the allowlist concept very constrained, as I didn't necessarily want to apply it to other parts of Koha without further consideration.

Calling get_ui_fields() in apply() could be inefficient if used in a loop. (I know it's not right now, but this module has potential to be used elsewhere in the future.) I think that we should generate our lists, and then add them to "Koha::Allowlist" objects via mutator methods like "add_allow_entries" and "add_deny_entries". I think that would be more in-line with OOP. Then apply() would be much more efficient when applied against N inputs.

If we're going to warn each key/value in %blocked, it probably makes more sense to put the warning in the foreach my $key (@key) loop, rather than double-handling the data. 

The perldoc for get_ui_fields in "Koha::Allowlist" also mentions "Koha::Patron::Allowlist::Public". 

--

Koha::Patron::Allowlist

Why would we want to include all fields via _get_all_fields()? Doesn't that include more information than we need to and just ultimately increase the maintenance burden?
Comment 24 David Cook 2021-09-03 06:09:58 UTC
(In reply to Marcel de Rooy from comment #16)
> Note that I would not mind merging opac and staff in the same class. The
> current distinction is a bit articifial?

The distinction is very real, as we do have two separate interfaces, but I don't have any feelings about how/where that distinction is implemented.
Comment 25 Marcel de Rooy 2021-09-03 06:17:54 UTC
(In reply to David Cook from comment #24)
> (In reply to Marcel de Rooy from comment #16)
> > Note that I would not mind merging opac and staff in the same class. The
> > current distinction is a bit articifial?
> 
> The distinction is very real, as we do have two separate interfaces, but I
> don't have any feelings about how/where that distinction is implemented.

Hahaha
Comment 26 Marcel de Rooy 2021-09-03 06:23:06 UTC
 FAIL   members/memberentry.pl
   FAIL   valid
                Can't locate Koha/Patron/Allowlist/Staff.pm in @INC (you may need to install the Koha::Patron::Allowlist::Staff module) (@INC contains: /usr/share/koha /usr/share/koha/lib /app/qatools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/myclone/plugins)

 FAIL   t/Koha/Patron/Allowlist.t
   FAIL   file permissions
                File must have the exec flag

And some warns about adding and removing modules.
Indeed we should squash this stuff.
Comment 27 Marcel de Rooy 2021-09-03 06:46:40 UTC
(In reply to David Cook from comment #23)

> If we're going to go with "Koha::Allowlist", we should probably rename
> "get_ui_fields" to something more generic like "get_entries". My intent
> around "Koha::Patron::Allowlist" was to keep the scope of the allowlist
> concept very constrained, as I didn't necessarily want to apply it to other
> parts of Koha without further consideration.

+1 for get_entries

> Calling get_ui_fields() in apply() could be inefficient if used in a loop.
> (I know it's not right now, but this module has potential to be used
> elsewhere in the future.) I think that we should generate our lists, and
> then add them to "Koha::Allowlist" objects via mutator methods like
> "add_allow_entries" and "add_deny_entries". I think that would be more
> in-line with OOP. Then apply() would be much more efficient when applied
> against N inputs.

Yes, we could generate already in new. Not sure what you mean here when you want to use add_entry or deny_entry? Instead of the parameter being used now? additional_deny_list
The earlier remarks about only having an allow list are still valid too? The current exercise of all fields minus deny1 and deny2 could be simpler by having one allowlist with an additional optional attribute for the interface?

$allow_patron = { fieldname => 1  | { interface => 1 } , .. }

> If we're going to warn each key/value in %blocked, it probably makes more
> sense to put the warning in the foreach my $key (@key) loop, rather than
> double-handling the data. 

One warn should be enough to trigger a dev to add a new field. And if someone hacks three columns, I only need one warn. Or still an exception?

> Why would we want to include all fields via _get_all_fields()? Doesn't that
> include more information than we need to and just ultimately increase the
> maintenance burden?

I think you are right. See previous remark.

Will give it a go now.
Comment 28 Marcel de Rooy 2021-09-03 06:53:38 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2021-09-03 06:53:42 UTC Comment hidden (obsolete)
Comment 30 Marcel de Rooy 2021-09-03 06:54:07 UTC
Further changes forthcoming
Comment 31 Marcel de Rooy 2021-09-03 08:26:19 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2021-09-03 08:26:51 UTC
(In reply to Marcel de Rooy from comment #31)
> Created attachment 124448 [details] [review] [review]
> Bug 28935: Rename get_ui_fields, introduce add and remove

Submitted as concept of proposed changes
Comment 33 David Cook 2021-09-03 09:01:22 UTC
(In reply to Marcel de Rooy from comment #27)
> > Calling get_ui_fields() in apply() could be inefficient if used in a loop.
> > (I know it's not right now, but this module has potential to be used
> > elsewhere in the future.) I think that we should generate our lists, and
> > then add them to "Koha::Allowlist" objects via mutator methods like
> > "add_allow_entries" and "add_deny_entries". I think that would be more
> > in-line with OOP. Then apply() would be much more efficient when applied
> > against N inputs.
> 
> Yes, we could generate already in new. Not sure what you mean here when you
> want to use add_entry or deny_entry? Instead of the parameter being used
> now? additional_deny_list
> The earlier remarks about only having an allow list are still valid too? The
> current exercise of all fields minus deny1 and deny2 could be simpler by
> having one allowlist with an additional optional attribute for the interface?
> 
> $allow_patron = { fieldname => 1  | { interface => 1 } , .. }
> 

I mean something like the following:

my $allowlist = Koha::Allowlist->new();

my $allow_entries = Koha::Patron::Allowlist->allow_entries({ interface => 'opac' });
my $deny_entries = Koha::Patron::Allowlist->deny_entries({ interface => 'opac' });

$allowlist->add_allow_entries($allow_entries);
$allowlist->add_deny_entries($deny_entries);

if ($additional_deny_entries){
  $allowlist->add_deny_entries($additional_deny_entries);
}

foreach my $borrower ( @borrowers ) {
   $allowlist->apply({ input => $borrower });
}

--

I know it's a lot longer, but it's more flexible and efficient. 

> > If we're going to warn each key/value in %blocked, it probably makes more
> > sense to put the warning in the foreach my $key (@key) loop, rather than
> > double-handling the data. 
> 
> One warn should be enough to trigger a dev to add a new field. And if
> someone hacks three columns, I only need one warn. Or still an exception?

I mean originally I thought about only dumping out %blocked via a DEBUG log level in the controller script rather than the module.  

I don't know if anyone else has looked, but with the status quo %blocked will have at least a few entries (like $borrowernumber) because we often send more data than we need. So raising an exception currently would break everything. 

Raising an exception might also be undesirable because it would signify to an attacker that they weren't successful. We don't necessarily want an attacker to know at attack time whether or not their attack worked. But that's just my 2 cents. I don't mind silently filtering in this context.

But I think the logging isn't too important. Maybe we could wrap it in a DEBUG log level condition and move on?
Comment 34 Marcel de Rooy 2021-09-03 09:11:29 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2021-09-03 09:13:46 UTC
my $deny_entries = Koha::Patron::Allowlist->deny_entries({ interface => 'opac' });

Hmm Reads a bit odd.
Comment 36 Marcel de Rooy 2021-09-03 09:14:57 UTC
(In reply to David Cook from comment #33)
> I know it's a lot longer, but it's more flexible and efficient. 

Dont really see that. You have ->add and ->remove. So you can do whatever you want..
Comment 37 Marcel de Rooy 2021-09-03 09:45:26 UTC
Created attachment 124449 [details] [review]
Bug 28935: Rename get_ui_fields, introduce add and remove

Also move test to t/Koha/Allowlist.t
Comment 38 Marcel de Rooy 2021-09-03 09:54:30 UTC
Created attachment 124450 [details] [review]
Bug 28935: Apply modules change to controller scripts
Comment 39 Marcel de Rooy 2021-09-03 09:56:39 UTC
(In reply to David Cook from comment #33)
> my $allowlist = Koha::Allowlist->new();
> 
> my $allow_entries = Koha::Patron::Allowlist->allow_entries({ interface =>
> 'opac' });

> $allowlist->add_allow_entries($allow_entries);
> 

Did add a load and unload method to more or less implement this.
Is that much more efficient? Not sure.
We should now always ->new AND ->load at least..
Comment 40 Marcel de Rooy 2021-09-03 10:03:20 UTC
NOTE: Lets decide over the weekend if we stick to the first 6 patches or continue with the LAST 4 patches. Patch 7 is a squash of the previous 6.
Comment 41 Marcel de Rooy 2021-09-03 13:11:39 UTC
Created attachment 124461 [details] [review]
Bug 28935: Rename get_ui_fields, introduce add and remove

Also move test to t/Koha/Allowlist.t
Comment 42 Marcel de Rooy 2021-09-03 13:11:44 UTC
Created attachment 124462 [details] [review]
Bug 28935: Apply modules change to controller scripts
Comment 43 Martin Renvoize 2021-09-03 15:25:04 UTC
Might be worth taking a look at bug 28948 where we add an allow list for fields that should be available on the public API..  I was envisaging similar methods for public_write_list, and staff_write_list to denote fields allowed to be read or written to given various interfaces.
Comment 44 Marcel de Rooy 2021-09-03 17:24:24 UTC
Created attachment 124499 [details] [review]
Bug 28935: Filter user input through allow list

This is a squashed commit of earlier work and a starting
point for some further proposed adjustments.

Bug 28935: Filter user input through allowlist for patrons

This patch creates public and staff allowlists that are used
to filter the user input from the Staff Interface, OPAC, and
OPAC self-registration.

The lists are hard-coded into modules, but they could be generated
by other means in the future to make them more suited to
user context.

Test plan:
0. The following should be done for the Staff Interface,
a logged in OPAC user, and a OPAC self-registration user
1. Go to a patron create/edit screen
2. Using F12 dev tools in your browser, add an input for "flags"
or "borrower_flags" depending on the other inputs available
on that screen. Give it a value of "1".
3. Submit the change
4. Check the Staff Interface or database to confirm that the flags
have not been set to 1 for that user

Additional test plan:
1. prove t/Koha/Patron/Allowlist.t
2. Note that all tests pass

Bug 28935: Centralize the allow list

We want to make sure developpers won't forget to update the allow list
and that we get discrepancy between the list.

This patch suggests to have:
1. A global "all fields" list which must match the borrowers table
columns
2. A global deny list to apply on top of this list. This deny list is
the list of attributes we want to reject from patron's edition from both
staff and OPAC interfaces
3. A specific deny list per interface

Moreover there is now a warning when a user edit an attribute part of
the deny list.

Bug 28935: Enforce *UnwantedField sysprefs

I didn't recreate the problem on selfmod but it existed for selfreg.

Create 2 attributes, 2 visible at the OPAC but only 1 editable.
Self reg and modify the input hidden value.

Bug 28935: Allow gonenoaddress, lost and relationship for staff

Bug 28935: Move logic to Koha::Allowlist for reusability

Bug 28935: Take borrowernumber from the original input

newdata is been modified, and filtered. We want to pick the original
value of borrowernumber.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2021-09-03 17:24:29 UTC
Created attachment 124500 [details] [review]
Bug 28935: Fix module in memberentry and chmod Allowlist.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2021-09-03 17:24:34 UTC
Created attachment 124501 [details] [review]
Bug 28935: Rename get_ui_fields, introduce add and remove

Also move test to t/Koha/Allowlist.t
And add load, unload methods.

Test plan:
Run Allowlist.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2021-09-03 17:24:38 UTC
Created attachment 124502 [details] [review]
Bug 28935: Apply module changes to controller scripts

Test plan:
Add an unallowed field to the submit with dom inspector in opac or
staff. Check if it is ignored and if log contains one warning.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2021-09-03 17:24:42 UTC
Created attachment 124503 [details] [review]
Bug 28935: Add logging to controller scripts

Logged at DEBUG level.

Test plan:
Adjust your logging config, add forbidden field, check log.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2021-09-03 17:27:49 UTC
The last debug line shows that an opac-memberentry UPDATE operation only shows unallowed stuff, and no longer borrowernumber or anonymized etc.
FIrst I moved the merge of $borrower and %borrower out of the way.
Later I decided to add an allowlist->add(borrowernumber) to remove the false borrowernumber warning. Perhaps we can remove that if we could move the line where the borrowernumber is inserted.


[DEBUG] opac-memberentry.pl: allowlist blocked fields: anonymized=0, autorenew_checkouts=1, borrowernotes=2, borrowernumber=51, checkprevcheckout=inherit, date_renewed=, dateenrolled=2021-07-08, dateexpiry=2029-10-08, debarred=, debarredcomment=, flags=1, gonenoaddress=2, lang=default, lastseen=, login_attempts=0, lost=0, opacnote=1, overdrive_auth_token=, privacy=1, privacy_guarantor_checkouts=0, privacy_guarantor_fines=0, relationship=, sms_provider_id=, sort1=, sort2=, updated_on=2021-09-03 13:59:14, zzgonenoaddress=abc CGI::Compile::ROOT::usr_share_koha_opac_opac_2dmemberentry_2epl::__ANON__ /usr/share/koha/opac/opac-memberentry.pl (288)

[DEBUG] opac-memberentry.pl: allowlist blocked fields: borrowernumber=51, gonenoaddress=2, gonenoaddress2=3 CGI::Compile::ROOT::usr_share_koha_opac_opac_2dmemberentry_2epl::__ANON__ /usr/share/koha/opac/opac-memberentry.pl (283)

[DEBUG] opac-memberentry.pl: allowlist blocked fields: gonenoaddress=2, gonenoaddress2=3 CGI::Compile::ROOT::usr_share_koha_opac_opac_2dmemberentry_2epl::__ANON__ /usr/share/koha/opac/opac-memberentry.pl (284)
Comment 50 Marcel de Rooy 2021-09-03 17:50:28 UTC
(In reply to Martin Renvoize from comment #43)
> Might be worth taking a look at bug 28948 where we add an allow list for
> fields that should be available on the public API..  I was envisaging
> similar methods for public_write_list, and staff_write_list to denote fields
> allowed to be read or written to given various interfaces.

Yeah. Certainly interesting. Maybe not for now directly, but we should move such things closer to each other.
Comment 51 Marcel de Rooy 2021-09-06 06:43:38 UTC
Created attachment 124523 [details] [review]
Bug 28935: Filter user input through allowlist for patrons

This patch creates public and staff allowlists that are used
to filter the user input from the Staff Interface, OPAC, and
OPAC self-registration.

The lists are hard-coded into modules, but they could be generated
by other means in the future to make them more suited to
user context.

Test plan:
0. The following should be done for the Staff Interface,
a logged in OPAC user, and a OPAC self-registration user
1. Go to a patron create/edit screen
2. Using F12 dev tools in your browser, add an input for "flags"
or "borrower_flags" depending on the other inputs available
on that screen. Give it a value of "1".
3. Submit the change
4. Check the Staff Interface or database to confirm that the flags
have not been set to 1 for that user

Additional test plan:
1. prove t/Koha/Patron/Allowlist.t
2. Note that all tests pass

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2021-09-06 06:43:43 UTC
Created attachment 124524 [details] [review]
Bug 28935: Centralize the allow list

We want to make sure developpers won't forget to update the allow list
and that we get discrepancy between the list.

This patch suggests to have:
1. A global "all fields" list which must match the borrowers table
columns
2. A global deny list to apply on top of this list. This deny list is
the list of attributes we want to reject from patron's edition from both
staff and OPAC interfaces
3. A specific deny list per interface

Moreover there is now a warning when a user edit an attribute part of
the deny list.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2021-09-06 06:43:47 UTC
Created attachment 124525 [details] [review]
Bug 28935: Enforce *UnwantedField sysprefs

I didn't recreate the problem on selfmod but it existed for selfreg.

Create 2 attributes, 2 visible at the OPAC but only 1 editable.
Self reg and modify the input hidden value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2021-09-06 06:43:52 UTC
Created attachment 124526 [details] [review]
Bug 28935: Allow gonenoaddress, lost and relationship for staff

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2021-09-06 06:43:56 UTC
Created attachment 124527 [details] [review]
Bug 28935: Move logic to Koha::Allowlist for reusability

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Marcel de Rooy 2021-09-06 06:44:01 UTC
Created attachment 124528 [details] [review]
Bug 28935: Take borrowernumber from the original input

newdata is been modified, and filtered. We want to pick the original
value of borrowernumber.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2021-09-06 06:44:05 UTC
Created attachment 124529 [details] [review]
Bug 28935: Fix module in memberentry and chmod Allowlist.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Marcel de Rooy 2021-09-06 06:44:10 UTC
Created attachment 124530 [details] [review]
Bug 28935: Rename get_ui_fields, introduce add and remove

Also move test to t/Koha/Allowlist.t
And add load, unload methods.

Test plan:
Run Allowlist.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Marcel de Rooy 2021-09-06 06:44:14 UTC
Created attachment 124531 [details] [review]
Bug 28935: Apply module changes to controller scripts

Test plan:
Add an unallowed field to the submit with dom inspector in opac or
staff. Check if it is ignored and if log contains one warning.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 60 Marcel de Rooy 2021-09-06 06:44:19 UTC
Created attachment 124532 [details] [review]
Bug 28935: Add logging to controller scripts

Logged at DEBUG level.

Test plan:
Adjust your logging config, add forbidden field, check log.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2021-09-06 06:45:50 UTC
Lets see if we can get this further now
Comment 62 Nick Clemens 2021-09-08 11:34:36 UTC
Created attachment 124640 [details] [review]
Bug 28935: Filter user input through allowlist for patrons

This patch creates public and staff allowlists that are used
to filter the user input from the Staff Interface, OPAC, and
OPAC self-registration.

The lists are hard-coded into modules, but they could be generated
by other means in the future to make them more suited to
user context.

Test plan:
0. The following should be done for the Staff Interface,
a logged in OPAC user, and a OPAC self-registration user
1. Go to a patron create/edit screen
2. Using F12 dev tools in your browser, add an input for "flags"
or "borrower_flags" depending on the other inputs available
on that screen. Give it a value of "1".
3. Submit the change
4. Check the Staff Interface or database to confirm that the flags
have not been set to 1 for that user

Additional test plan:
1. prove t/Koha/Patron/Allowlist.t
2. Note that all tests pass

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Nick Clemens 2021-09-08 11:34:41 UTC
Created attachment 124641 [details] [review]
Bug 28935: Centralize the allow list

We want to make sure developpers won't forget to update the allow list
and that we get discrepancy between the list.

This patch suggests to have:
1. A global "all fields" list which must match the borrowers table
columns
2. A global deny list to apply on top of this list. This deny list is
the list of attributes we want to reject from patron's edition from both
staff and OPAC interfaces
3. A specific deny list per interface

Moreover there is now a warning when a user edit an attribute part of
the deny list.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Nick Clemens 2021-09-08 11:34:45 UTC
Created attachment 124642 [details] [review]
Bug 28935: Enforce *UnwantedField sysprefs

I didn't recreate the problem on selfmod but it existed for selfreg.

Create 2 attributes, 2 visible at the OPAC but only 1 editable.
Self reg and modify the input hidden value.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Nick Clemens 2021-09-08 11:34:49 UTC
Created attachment 124643 [details] [review]
Bug 28935: Allow gonenoaddress, lost and relationship for staff

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 66 Nick Clemens 2021-09-08 11:34:54 UTC
Created attachment 124644 [details] [review]
Bug 28935: Move logic to Koha::Allowlist for reusability

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 67 Nick Clemens 2021-09-08 11:34:58 UTC
Created attachment 124645 [details] [review]
Bug 28935: Take borrowernumber from the original input

newdata is been modified, and filtered. We want to pick the original
value of borrowernumber.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Nick Clemens 2021-09-08 11:35:03 UTC
Created attachment 124646 [details] [review]
Bug 28935: Fix module in memberentry and chmod Allowlist.t

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Nick Clemens 2021-09-08 11:35:09 UTC
Created attachment 124647 [details] [review]
Bug 28935: Rename get_ui_fields, introduce add and remove

Also move test to t/Koha/Allowlist.t
And add load, unload methods.

Test plan:
Run Allowlist.t

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Nick Clemens 2021-09-08 11:35:14 UTC
Created attachment 124648 [details] [review]
Bug 28935: Apply module changes to controller scripts

Test plan:
Add an unallowed field to the submit with dom inspector in opac or
staff. Check if it is ignored and if log contains one warning.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Nick Clemens 2021-09-08 11:35:19 UTC
Created attachment 124649 [details] [review]
Bug 28935: Add logging to controller scripts

Logged at DEBUG level.

Test plan:
Adjust your logging config, add forbidden field, check log.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Nick Clemens 2021-09-08 11:35:24 UTC
Created attachment 124650 [details] [review]
Bug 28935: (follow-up) Correct attribute test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Nick Clemens 2021-09-08 11:37:28 UTC
Couple comments:
1 - Should we remove the deletion of 'flags' in ParseCGIForBorrower and allow the allowlist to handle it?
2 - I think we could lower the logging level to 'WARN' and add info about what was attempted to be forced - logging attempts to alter data seems like it doesn't need to be 'DEBUG'
Comment 74 Marcel de Rooy 2021-09-08 12:46:07 UTC
(In reply to Nick Clemens from comment #73)

Thx for your signoff.

> 1 - Should we remove the deletion of 'flags' in ParseCGIForBorrower and
> allow the allowlist to handle it?
Could be. But I would personally leave it as is.

> 2 - I think we could lower the logging level to 'WARN' and add info about
> what was attempted to be forced - logging attempts to alter data seems like
> it doesn't need to be 'DEBUG'

DEBUG is a lower level than WARN. Normally you would set your app to WARN and not receive DEBUG info. Only if you want diagnostic info, you will lower the log level to DEBUG.
The debug info contains all fields that were attempted to change.
Comment 75 Martin Renvoize 2021-09-10 08:52:30 UTC
Created attachment 124730 [details] [review]
Bug 28935: Filter user input through allowlist for patrons

This patch creates public and staff allowlists that are used
to filter the user input from the Staff Interface, OPAC, and
OPAC self-registration.

The lists are hard-coded into modules, but they could be generated
by other means in the future to make them more suited to
user context.

Test plan:
0. The following should be done for the Staff Interface,
a logged in OPAC user, and a OPAC self-registration user
1. Go to a patron create/edit screen
2. Using F12 dev tools in your browser, add an input for "flags"
or "borrower_flags" depending on the other inputs available
on that screen. Give it a value of "1".
3. Submit the change
4. Check the Staff Interface or database to confirm that the flags
have not been set to 1 for that user

Additional test plan:
1. prove t/Koha/Patron/Allowlist.t
2. Note that all tests pass

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2021-09-10 08:52:35 UTC
Created attachment 124731 [details] [review]
Bug 28935: Centralize the allow list

We want to make sure developpers won't forget to update the allow list
and that we get discrepancy between the list.

This patch suggests to have:
1. A global "all fields" list which must match the borrowers table
columns
2. A global deny list to apply on top of this list. This deny list is
the list of attributes we want to reject from patron's edition from both
staff and OPAC interfaces
3. A specific deny list per interface

Moreover there is now a warning when a user edit an attribute part of
the deny list.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2021-09-10 08:52:40 UTC
Created attachment 124732 [details] [review]
Bug 28935: Enforce *UnwantedField sysprefs

I didn't recreate the problem on selfmod but it existed for selfreg.

Create 2 attributes, 2 visible at the OPAC but only 1 editable.
Self reg and modify the input hidden value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2021-09-10 08:52:45 UTC
Created attachment 124733 [details] [review]
Bug 28935: Allow gonenoaddress, lost and relationship for staff

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Martin Renvoize 2021-09-10 08:52:49 UTC
Created attachment 124734 [details] [review]
Bug 28935: Move logic to Koha::Allowlist for reusability

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Martin Renvoize 2021-09-10 08:52:54 UTC
Created attachment 124735 [details] [review]
Bug 28935: Take borrowernumber from the original input

newdata is been modified, and filtered. We want to pick the original
value of borrowernumber.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2021-09-10 08:53:00 UTC
Created attachment 124736 [details] [review]
Bug 28935: Fix module in memberentry and chmod Allowlist.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2021-09-10 08:53:05 UTC
Created attachment 124737 [details] [review]
Bug 28935: Rename get_ui_fields, introduce add and remove

Also move test to t/Koha/Allowlist.t
And add load, unload methods.

Test plan:
Run Allowlist.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2021-09-10 08:53:10 UTC
Created attachment 124738 [details] [review]
Bug 28935: Apply module changes to controller scripts

Test plan:
Add an unallowed field to the submit with dom inspector in opac or
staff. Check if it is ignored and if log contains one warning.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2021-09-10 08:53:15 UTC
Created attachment 124739 [details] [review]
Bug 28935: Add logging to controller scripts

Logged at DEBUG level.

Test plan:
Adjust your logging config, add forbidden field, check log.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2021-09-10 08:53:20 UTC
Created attachment 124740 [details] [review]
Bug 28935: (follow-up) Correct attribute test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2021-09-10 08:56:05 UTC
This seems pretty well thought out and I can see how it could be applied in other cases.

It all works well, has tests and qa scripts pass.

Passing QA.

My one minor comment was around the Koha::AllowList module name.. I had a few thoughts on nameing options but decided none of them really made any more sense.

Koha::Base::AllowList
Koha::Object::AllowList

Something that made it clear it's a base class.. but as we have Koha::Exception for much the same sort of idea.. just as a base.. this isn't a blocker.

PQA
Comment 87 Martin Renvoize 2021-09-10 14:19:41 UTC
This is a pain when trying to apply it to objects for the API :(

I'm still of the opinion ::Allowlist is too generic.

We have lots of differing Allowlist needs..

Read, ReadOnly and ReadWrite fields and different subsets for different access points... Intranet/OPAC, API Public/Staff.

I tried using the Allowlist approach in a follow-up on bug 28948 and really it just highlights this.

I used it to create Public vs Staff Readable field lists for the API.. that's the opposite of what you've done here as the Allowlist is listing Writable fields.  And what about the UnwantedFields stuff.. should we really be doing that at the controller level instead of in the object where it will get properly tested and caught for all interfaces... try to do that in the Koha::Object base class for the to_api method and we come really unstuck.

Frankly.. whilst this passes QA in terms of the code works for the use case here and only that use case.. I feel a fundamental base like this needs some more work.... you even choose NOT to use it in clearly related bugs.
Comment 88 Marcel de Rooy 2021-09-10 14:52:01 UTC
We could push a very simple patch to remove some borrower fields from member entry in opac and staff, less than 10 lines. I already have it running.

Then we have a bit more time to customize this. Otherwise stable versions are waiting for this discussion?

It would not be hard to add an attribute to the allow list entries and have apply check that attribute. In that way you could generate various lists for borrowers based on combinations of these attributes.
Comment 89 Jonathan Druart 2021-09-10 15:05:34 UTC
Yes, Marcel, please attach the patch here!
Comment 90 Marcel de Rooy 2021-09-10 15:18:50 UTC Comment hidden (obsolete)
Comment 91 Marcel de Rooy 2021-09-10 15:19:29 UTC Comment hidden (obsolete)
Comment 92 Marcel de Rooy 2021-09-10 15:25:15 UTC
(In reply to Marcel de Rooy from comment #88)
> It would not be hard to add an attribute to the allow list entries and have
> apply check that attribute. In that way you could generate various lists for
> borrowers based on combinations of these attributes.

If the allowlist is too generic, another approach would be to use the columns from DBIx, put some extra code in the Schema modules to create various column lists from there. Even closer to the DB?
Comment 93 Marcel de Rooy 2021-09-10 17:41:15 UTC
Created attachment 124784 [details] [review]
Bug 28935: No filtering on patron's data on member entry pages

Security patch. Follow-up for 28929.
Including correction for gonenoaddress and two others.
Includes unwanted fields too now.
Comment 94 Marcel de Rooy 2021-09-10 17:41:40 UTC
Will move the new code elsewhere
Comment 100 Marcel de Rooy 2021-09-13 06:17:32 UTC
(In reply to Marcel de Rooy from comment #94)
> Will move the new code elsewhere

See bug 28999. Removed the patron related stuff. So only generic code.

But this patch still needs a signoff. Activity enough on Bugzilla, but this security patch got no attention ;)
Comment 101 Nick Clemens 2021-09-13 12:57:58 UTC
Created attachment 124832 [details] [review]
Bug 28935: No filtering on patron's data on member entry pages

Security patch. Follow-up for 28929.
Including correction for gonenoaddress and two others.
Includes unwanted fields too now.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 102 Nick Clemens 2021-09-13 12:58:03 UTC
Created attachment 124833 [details] [review]
Bug 28935: (follow-up) Use BorrowerUnwantedField on staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 104 Owen Leonard 2021-09-13 13:19:57 UTC
Created attachment 124837 [details] [review]
Bug 28935: No filtering on patron's data on member entry pages

Security patch. Follow-up for 28929.
Including correction for gonenoaddress and two others.
Includes unwanted fields too now.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 105 Owen Leonard 2021-09-13 13:20:02 UTC
Created attachment 124838 [details] [review]
Bug 28935: (follow-up) Use BorrowerUnwantedField on staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 106 Marcel de Rooy 2021-09-13 13:31:44 UTC
Created attachment 124839 [details] [review]
Bug 28935: No filtering on patron's data on member entry pages

Security patch. Follow-up for 28929.
Including correction for gonenoaddress and two others.
Includes unwanted fields too now.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 107 Marcel de Rooy 2021-09-13 13:31:49 UTC
Created attachment 124840 [details] [review]
Bug 28935: (QA follow-up) Use BorrowerUnwantedField on staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 108 Marcel de Rooy 2021-09-13 13:32:48 UTC
Exchanged signoffs. Thanks Owen and Nick ! So passed QA now.
Comment 109 Victor Grousset/tuxayo 2021-09-16 07:31:47 UTC
Works on 20.05.x here was my test plan:

1. log into the OPAC
2. modify your personal details
3. right click on a field => inspect
4. edit as html (wording depends on the web browser)
5. add: <input type="text" name="borrower_dateexpiry" value="2034-03-03">
6. submit the form
7. log into the staff interface
8. review patron updates
9. see that date expiry would have been changed
10. deny
11. apply patches
12. switch to the patron and resubmit a fraudulent update request
13. switch to the librarian and see that the date expiry isn't changed anymore
Comment 110 Victor Grousset/tuxayo 2021-09-16 07:35:55 UTC
Backported: Pushed to 20.05.x security branch for 20.05.16
Comment 111 Jonathan Druart 2021-09-30 07:22:57 UTC
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Comment 112 Jonathan Druart 2021-09-30 07:52:04 UTC
Pushed to master for 21.11, thanks to everybody involved!