Bug 30744 - Use RecordProcessor in get_marc_notes to ensure non-public notes do not leak
Summary: Use RecordProcessor in get_marc_notes to ensure non-public notes do not leak
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords: regression, rel_21_05_candidate, rel_21_11_candidate
Depends on: 30848 30918
Blocks: 29652 30921
  Show dependency treegraph
 
Reported: 2022-05-12 22:31 UTC by Aleisha Amohia
Modified: 2023-12-28 20:42 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.04, 21.11.11


Attachments
Bug 30744: Use RecordProcessor in get_marc_notes (2.29 KB, patch)
2022-05-25 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.39 KB, patch)
2022-05-25 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.40 KB, patch)
2022-05-26 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.45 KB, patch)
2022-05-26 23:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.44 KB, patch)
2022-06-07 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30744: Optionally allow passing record to prevent refetch (1.80 KB, patch)
2022-06-07 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.48 KB, patch)
2022-06-07 12:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.54 KB, patch)
2022-06-30 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30744: Use RecordProcessor in get_marc_notes (5.59 KB, patch)
2022-06-30 16:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2022-05-12 22:31:29 UTC
To reproduce:

1. Go to Koha Administration -> Koha bibliographic frameworks
2. View the MARC structure for your BKS framework (or something else)
3. Search for tag 583, edit subfields
4. Go to subfield 'x' - nonpublic note. Confirm the OPAC visibility checkbox is UNCHECKED.
5. Edit or create a record using the BKS framework. Put a note in the 583$x.
6. View this record in the OPAC
7. Go to the Title notes tab. Confirm the non-public note is showing, even though the framework says it should not be visible via the OPAC.
Comment 1 David Cook 2022-05-13 00:12:18 UTC
I haven't verified yet but that's interesting...

Ah because of this sucker:

my $marcnotesarray = $biblio->get_marc_notes({ marcflavour => $marcflavour, opac => 1 });

That function should have a "record" parameter. It would solve the problem (and improve efficiency by reducing the number of unnecessary database calls).
Comment 2 David Cook 2022-05-13 00:18:10 UTC
(In reply to David Cook from comment #1)
> I haven't verified yet but that's interesting...
> 
> Ah because of this sucker:
> 
> my $marcnotesarray = $biblio->get_marc_notes({ marcflavour => $marcflavour,
> opac => 1 });
> 
> That function should have a "record" parameter. It would solve the problem
> (and improve efficiency by reducing the number of unnecessary database
> calls).

In fact, this could be a class method rather than an object method... for loops (like basket.pl and opac-basket.pl) that could add up...
Comment 3 Martin Renvoize 2022-05-25 14:57:08 UTC
Created attachment 135339 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.
Comment 4 Martin Renvoize 2022-05-25 14:58:36 UTC
We had customers starting to notice this, so I've taken a pragmatic approach here for now.. 

I think this all needs a little more thought as to what level it should all sit at as fetching and processing the MARC data twice is a bit silly.
Comment 5 Martin Renvoize 2022-05-25 16:19:39 UTC
Created attachment 135354 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.
Comment 6 David Cook 2022-05-26 00:01:43 UTC
(In reply to Martin Renvoize from comment #4)
> We had customers starting to notice this, so I've taken a pragmatic approach
> here for now.. 
> 
> I think this all needs a little more thought as to what level it should all
> sit at as fetching and processing the MARC data twice is a bit silly.

I don't like it but I do see the pragmatism...

Yet, if we do this, I don't think we'll ever fix the performance of get_marc_notes() :/. 

Would a compromise be to add a "record" parameter to "get_marc_notes", and just update the call in opac/opac-detail.pl for now (we could open other tickets for the other Perl scripts)? 

That would fix Bug 30744 without duplicating processing and with less code.
Comment 7 Martin Renvoize 2022-05-26 06:34:40 UTC
I was actually just seeing this as a stepping stone.. I intend to work on bug 29652 and bug 29697 next and as part of those I'm thinking we cache filtered records much lower in the stack so we're not passing around a record but rather any method we call already has a shared copy to refer to.
Comment 8 Martin Renvoize 2022-05-26 10:07:25 UTC
Created attachment 135365 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.
Comment 9 David Nind 2022-05-26 23:53:27 UTC
Created attachment 135415 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2022-05-27 00:04:30 UTC
Testing notes (koha-testing-docker):

1. I followed the steps in comment #1
2. After applying the patch, I refreshed the OPAC page and the nonpublic note was no longer visible.
3. I changed the framework so 583$x should be visible in the OPAC, and this works as expected.
Comment 11 Katrin Fischer 2022-06-06 13:06:49 UTC
I am a little worried that we won't be able to backport this looking at the dependency tree. Would it be possible to create a simple fix for the older versions in addition to this?

I wonder if it might have been better to deal with expanding authorised values separately? 

Detail page explodes:

Can't locate object method "search" via package "Koha::ClassSources" (perhaps you forgot to load "Koha::ClassSources"?) at /kohadevbox/koha/C4/Biblio.pm line 1510

Tests fail:

t/db_dependent/Koha/Biblio.t .. 13/21 
    #   Failed test 'Authorised value is correctly parsed to show description rather than code'
    #   at t/db_dependent/Koha/Biblio.t line 719.
    #          got: 'CODE'
    #     expected: 'Description should show'

    #   Failed test 'Authorised value is correctly parsed for OPAC to show description rather than code'
    #   at t/db_dependent/Koha/Biblio.t line 727.
    #          got: 'CODE'
    #     expected: 'Description should show OPAC'
    # Looks like you failed 2 tests of 14.
t/db_dependent/Koha/Biblio.t .. 15/21 
#   Failed test 'get_marc_notes() MARC21 tests'
#   at t/db_dependent/Koha/Biblio.t line 736.
t/db_dependent/Koha/Biblio.t .. 21/21 # Looks like you failed 1 test of 21.
t/db_dependent/Koha/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/21 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Biblio.t (Wstat: 256 Tests: 21 Failed: 1)
  Failed test:  15
  Non-zero exit status: 1
Files=1, Tests=21, 14 wallclock secs ( 0.05 usr  0.01 sys + 11.36 cusr  1.68 csys = 13.10 CPU)
Result: FAIL
Comment 12 Martin Renvoize 2022-06-07 12:18:44 UTC
Created attachment 135757 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Martin Renvoize 2022-06-07 12:18:49 UTC
Created attachment 135758 [details] [review]
Bug 30744: Optionally allow passing record to prevent refetch

This patch adds the option to pass the marcrecord into the call to
prevent the extra DB fetch of the record.
Comment 14 Martin Renvoize 2022-06-07 12:20:53 UTC
* Minor rebase for change of filter name in bug 30848 (to fix Katrins issue)
* Minor follow-up to allow for David's request that we can pass a record along rather than refetching it.
Comment 15 Martin Renvoize 2022-06-07 12:21:25 UTC
I can create a backportable version of this easily enough too
Comment 16 Martin Renvoize 2022-06-07 12:55:30 UTC
Created attachment 135762 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Martin Renvoize 2022-06-07 12:58:13 UTC
OK, after discussing with Tomas, I've split the bug.. the simple quick fix can now be found in bug 30918 and the work here can now be seen as the next steps to reduce the chance of leaking data in other displays.
Comment 18 David Nind 2022-06-08 16:47:31 UTC
What's required to test this, since the quick fix is in another bug? Or is more work still required?
Comment 19 Martin Renvoize 2022-06-10 11:00:40 UTC
Moved the original bugfix to it's own bug and changed this to an architectural one.

I'm still working through this tree at the moment really so perhaps not worth testing.

If you were to test, it's basically to confirm that there are no user facing changes when this bug is applied atop bug 30198
Comment 20 David Nind 2022-06-10 14:49:19 UTC
Thanks Martin. I'll leave for a bit and come back to this one later.
Comment 21 Jonathan Druart 2022-06-13 09:54:01 UTC
There is something wrong in the design here.
I really don't think it's correct to pass the marc to Koha::Biblio->get_marc_notes

I feel like we need a Koha::MARC class that would have the 'get_notes' method.

Koha::Biblio->get_marc_notes fetches the marc and call it
If you have a marc already you could call it directly.
Comment 22 Martin Renvoize 2022-06-13 10:24:06 UTC
(In reply to Jonathan Druart from comment #21)
> There is something wrong in the design here.
> I really don't think it's correct to pass the marc to
> Koha::Biblio->get_marc_notes
> 
> I feel like we need a Koha::MARC class that would have the 'get_notes'
> method.
> 
> Koha::Biblio->get_marc_notes fetches the marc and call it
> If you have a marc already you could call it directly.

We allow for optionally passing the marc (at Davids request)... but yes, I generally agree.. there's a bug 'move get_marc_* methods to Koha::Biblio::Metadata.. which Tomas raised and I've been trying to think of how to achieve.. this is one of the stepping stones towards that end.
Comment 23 Tomás Cohen Arazi 2022-06-30 14:09:08 UTC
Created attachment 136783 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Kyle M Hall 2022-06-30 16:03:20 UTC
Created attachment 136793 [details] [review]
Bug 30744: Use RecordProcessor in get_marc_notes

This patch utilises RecordProcessor to filter the MARC::Record for the
right interface prior to constructing the marc notes array.  We also
remove the use of C4::XSLT for replacing AV values in the MARC fields in
preference to using the RecordProcessor filter.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Tomás Cohen Arazi 2022-07-01 12:20:09 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 26 Tomás Cohen Arazi 2022-07-29 00:22:19 UTC
@RMAints please backport this bugs. If they don't apply we'll provide version-specific patches. But please try ASAP.
Comment 27 Lucas Gass 2022-07-29 17:22:23 UTC
Backported to 22.05.x for 22.05.04
Comment 28 Arthur Suzuki 2022-08-12 06:47:29 UTC
applied to 21.11.x for 21.11.11
Comment 29 Victor Grousset/tuxayo 2022-08-17 02:01:48 UTC
Can't backport a dependency to 21.05.x so can't backport this.

Send help to Bug 30918 if this one (30744) is really needed.



(In reply to Martin Renvoize from comment #15)
> I can create a backportable version of this easily enough too

That's another solution.
Comment 30 Victor Grousset/tuxayo 2022-10-21 22:37:51 UTC
Last month to get an opportunity to have this backported to 21.05.x if that is still needed and if someone can make a patch for 21.05.x