Bug 28999 - Add Koha::AllowList and DenyList
Summary: Add Koha::AllowList and DenyList
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-10 17:46 UTC by Marcel de Rooy
Modified: 2021-12-17 10:00 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 28999: Introduce Allowlist (4.76 KB, patch)
2021-09-13 06:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28999: Rename to AllowList (761 bytes, patch)
2021-09-13 06:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28999: Additional check method, some parameter changes (6.43 KB, patch)
2021-09-13 06:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28999: Remove (un)load, add reset; apply returns result now (9.33 KB, patch)
2021-09-13 06:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28999: Add Koha::DenyList as counterpart (3.73 KB, patch)
2021-09-13 06:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28999: {CONCEPT] Introduce Koha::Object::ColumnSet (5.57 KB, patch)
2021-09-13 08:06 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 Marcel de Rooy 2021-09-10 17:46:46 UTC
Idea is to have a module that helps you filter columns of a certain table for various use cases. Like checking parameters from user interface, or in the API, etc.
Comment 1 Marcel de Rooy 2021-09-13 06:15:33 UTC
Created attachment 124820 [details] [review]
Bug 28999: Introduce Allowlist

Based on work of David Cook and Jonathan Druart.
Removed the patron example here.
Comment 2 Marcel de Rooy 2021-09-13 06:15:37 UTC
Created attachment 124821 [details] [review]
Bug 28999: Rename to AllowList
Comment 3 Marcel de Rooy 2021-09-13 06:15:40 UTC
Created attachment 124822 [details] [review]
Bug 28999: Additional check method, some parameter changes

License for module.
Remove interface parameter in new.
Add dry_run parameter to apply.
Add arrayref as input for apply.
Add check as shortcut for apply with dry_run.
Add tests.
Comment 4 Marcel de Rooy 2021-09-13 06:15:44 UTC
Created attachment 124823 [details] [review]
Bug 28999: Remove (un)load, add reset; apply returns result now

Remove load but add $defaults in new.
Replace unload by reset, restoring defaults.
Replace warn en verbose by a dump variable in apply.
Do not return $blocked anymore but return the filtered input in apply.
New check method returns true when no entries were blocked.
New keys method to list current allow list entries.
Comment 5 Marcel de Rooy 2021-09-13 06:15:47 UTC
Created attachment 124824 [details] [review]
Bug 28999: Add Koha::DenyList as counterpart
Comment 6 Marcel de Rooy 2021-09-13 06:27:01 UTC
The code for AllowList is generic now. But we need the ColumnSet to apply it to Koha objects in various ways.


This is the idea:

Say $city is a Koha object.
$city->filter('opac_ui') returns city data through the opac_ui filter which is just a set of columns for some purpose.
$city->filter('staff') could return some other subset or even nothing when the set does not exist.

The sub filter calls Koha::Object::ColumnSet with the requested column set name. The module calls the corresponding DBIx schema where we add a sub called column_set_info containing the lists we need like a list for opac_ui.
ColumnSet runs a Koha::AllowList based on that list over the object data.

Hang on a little bit. Submitting some concept code shortly.
Comment 7 David Cook 2021-09-13 06:49:44 UTC
I spent a lot of time thinking this morning, and I'm not sure I see a purpose for filtering outbound data besides for the public API. 

And on the flipside the API has its own input validation so it wouldn't need an inbound data filter whereas the HTML UI does. 

I just wonder if we're trying to make this all too generic/comprehensive. The original premise was just filtering input from HTML forms. 

Maybe it makes more sense to do a RFC? I think this might be the 3rd or 4th report now for allow lists.
Comment 8 Marcel de Rooy 2021-09-13 08:06:37 UTC
Created attachment 124828 [details] [review]
Bug 28999: {CONCEPT] Introduce Koha::Object::ColumnSet
Comment 9 Marcel de Rooy 2021-09-13 08:08:10 UTC
Please add feedback on especially the last patch.
Comment 10 Marcel de Rooy 2021-09-15 12:33:00 UTC
Moving the ColumnSet to its own report. Bug 29029
Comment 11 Marcel de Rooy 2021-09-15 12:35:16 UTC
(In reply to Marcel de Rooy from comment #9)
> Please add feedback on especially the last patch.

Here is my current stand: These two objects are so generic and close to hash slice and invert that I removed them from ColumnSet. They are real nice objects, but we can live without them?
Comment 12 Marcel de Rooy 2021-12-17 10:00:02 UTC
Waste of time?