Bug 28478 - MARC detail and ISBD pages still show suppressed records
Summary: MARC detail and ISBD pages still show suppressed records
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 10195
Blocks: 38936
  Show dependency treegraph
 
Reported: 2021-05-28 07:11 UTC by Katrin Fischer
Modified: 2025-03-06 12:57 UTC (History)
15 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:
25.05.00,24.11.02,24.05.07
Circulation function:


Attachments
Bug 28478: Add get_opac_suppression extractor (3.09 KB, patch)
2024-12-18 14:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28478: Add Koha::Biblio->opac_suppressed() (3.02 KB, patch)
2024-12-18 14:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed() (6.55 KB, patch)
2024-12-18 14:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28478: Add get_opac_suppression extractor (3.15 KB, patch)
2025-01-14 08:17 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 28478: Add Koha::Biblio->opac_suppressed() (3.07 KB, patch)
2025-01-14 08:17 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed() (6.68 KB, patch)
2025-01-14 08:17 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 28478: Add get_opac_suppression extractor (3.20 KB, patch)
2025-01-21 13:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 28478: Add Koha::Biblio->opac_suppressed() (3.13 KB, patch)
2025-01-21 13:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed() (6.75 KB, patch)
2025-01-21 13:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 28478: Add get_opac_suppression extractor (3.28 KB, patch)
2025-01-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28478: Add Koha::Biblio->opac_suppressed() (3.20 KB, patch)
2025-01-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed() (6.82 KB, patch)
2025-01-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
[24.05.x] Bug 28478: Add get_opac_suppression extractor (3.94 KB, patch)
2025-02-24 13:49 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 28478: Add Koha::Biblio->opac_suppressed() (3.21 KB, patch)
2025-02-24 13:49 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed() (6.88 KB, patch)
2025-02-24 13:49 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-05-28 07:11:49 UTC
I couldn't verify on master yet, but tested on 20.11.05:

When a record is suppressed in the catalog using 942$n=1 and OpacSuppression a direct link to the MARCdetail page will still work:

/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=x

This is problematic as some search bots could still index the records and someone clever would also be able to guess it.

I think we need to add checks for suppression on these pages and redirect according to OpacSuppressionRedirect.
Comment 1 Fridolin Somers 2021-12-08 21:35:17 UTC
Arf indeed, code in only in opac-detail.pl :

https://git.koha-community.org/Koha-community/Koha/src/commit/3ba74940ee730e92ad7061f27f18390cc6308364/opac/opac-detail.pl#L147

This needs to be in a module, and if possible with a Koha to MARC mapping instead of hardcoded 942$n.
Comment 2 Katrin Fischer 2021-12-12 17:17:06 UTC
(In reply to Fridolin Somers from comment #1)
> Arf indeed, code in only in opac-detail.pl :
> 
> https://git.koha-community.org/Koha-community/Koha/src/commit/
> 3ba74940ee730e92ad7061f27f18390cc6308364/opac/opac-detail.pl#L147
> 
> This needs to be in a module, and if possible with a Koha to MARC mapping
> instead of hardcoded 942$n.

I think adding a Koha to MARC mapping could be a different bug, also to keep this as easy as possible for backporting.

Possibly the code for the check should also be in a routine somewhere?

For MARC21 942$n has been established as standard and has a matching index. I am not sure if UNIMARC has a standard mapping at the moment?
Comment 3 Fridolin Somers 2021-12-14 02:37:51 UTC
(In reply to Katrin Fischer from comment #2)
> (In reply to Fridolin Somers from comment #1)
> > Arf indeed, code in only in opac-detail.pl :
> > 
> > https://git.koha-community.org/Koha-community/Koha/src/commit/
> > 3ba74940ee730e92ad7061f27f18390cc6308364/opac/opac-detail.pl#L147
> > 
> > This needs to be in a module, and if possible with a Koha to MARC mapping
> > instead of hardcoded 942$n.
> 
> I think adding a Koha to MARC mapping could be a different bug, also to keep
> this as easy as possible for backporting.
> 
Sure.
Comment 4 Fridolin Somers 2021-12-14 02:42:10 UTC
Should we add to Koha::Biblio hidden_in_opac() ?
Maybe with a param 'context' = 'search' or 'details' ?
Comment 5 David Cook 2024-12-12 05:33:09 UTC
I reckon this is actually a security bug.
Comment 6 Fridolin Somers 2024-12-13 06:47:17 UTC
Maybe it is time to fix this :

FIXME hardcoded; the suppression flag ought to be materialized
as a column on biblio or the like
Comment 7 David Cook 2024-12-15 23:45:03 UTC
(In reply to Fridolin Somers from comment #6)
> Maybe it is time to fix this :
> 
> FIXME hardcoded; the suppression flag ought to be materialized
> as a column on biblio or the like

No better time than the start of a cycle. Let's do it!

If you write the code, I'm sure someone can test it, and I can QA it.
Comment 8 Tomás Cohen Arazi (tcohen) 2024-12-18 14:43:18 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi (tcohen) 2024-12-18 14:43:21 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi (tcohen) 2024-12-18 14:43:23 UTC Comment hidden (obsolete)
Comment 11 Magnus Enger 2025-01-14 08:17:07 UTC
Created attachment 176490 [details] [review]
Bug 28478: Add get_opac_suppression extractor

This patch adds an extractor method for the historically hardcoded field
942$n. This way we have a single place in which we code the extraction
and sanitization of its value.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass! All use cases are covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tests pass
Comment 12 Magnus Enger 2025-01-14 08:17:11 UTC
Created attachment 176491 [details] [review]
Bug 28478: Add Koha::Biblio->opac_suppressed()

This patch adds a convenient method at the Koha::Biblio level, as a
wrapper for the extrator added on this bug. Following the established
pattern we adopted a while back.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass! All use cases covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tests pass
Comment 13 Magnus Enger 2025-01-14 08:17:14 UTC
Created attachment 176492 [details] [review]
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed()

This patch makes the following scripts use the new method for checking
suppression:

* opac/opac-ISBDdetail.pl
* opac/opac-MARCdetail.pl
* opac/opac-detail.pl

In the `opac-detail.pl` case, it is a simple change by removing MARC
data traversal in favor of the new method. The code checking
suppression gets moved up so we don't process or make any other
calculations if the record is suppressed.

The other two scripts where completely missing the check and thus
leaking suppressed records.

To test:
1. Pick two records, one marked as suppressed, and the other not
   suppressed.
2. Try acessing them in the OPAC detail page.
=> SUCCESS: Suppressed records are suppressed, and not suppressed ones
are not.
3. Try the same records on the ISBD and MARC view
=> FAIL: They are not suppressed!
4. Apply this patch
5. Repeat 2
=> SUCCESS: Suppression is still respected
6. Repeat 3
=> SUCCESS: Suppression is respected on the ISBD and MARC views
7. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Remember to activate OPAC suppression with
OpacSuppression.
Comment 14 Jonathan Druart 2025-01-21 08:59:16 UTC
The code is duplicated in 3 different places. We need to move it to a module.

C4::Output::redirect_if_opac_suppressed or something?
It's ugly but better than duplication of code.
Comment 15 Tomás Cohen Arazi (tcohen) 2025-01-21 11:01:54 UTC
(In reply to Jonathan Druart from comment #14)
> The code is duplicated in 3 different places. We need to move it to a module.
> 
> C4::Output::redirect_if_opac_suppressed or something?
> It's ugly but better than duplication of code.

I like your idea, but can we do it on a separate report? This patch actually introduces a high-level method that encapsulates some logic and solves a long standing issue.
Comment 16 Tomás Cohen Arazi (tcohen) 2025-01-21 12:46:31 UTC
(In reply to Jonathan Druart from comment #14)
> The code is duplicated in 3 different places. We need to move it to a module.
> 
> C4::Output::redirect_if_opac_suppressed or something?
> It's ugly but better than duplication of code.

Done on bug 38926!
Comment 17 Martin Renvoize (ashimema) 2025-01-21 13:25:23 UTC
Created attachment 176861 [details] [review]
Bug 28478: Add get_opac_suppression extractor

This patch adds an extractor method for the historically hardcoded field
942$n. This way we have a single place in which we code the extraction
and sanitization of its value.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass! All use cases are covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize (ashimema) 2025-01-21 13:25:26 UTC
Created attachment 176862 [details] [review]
Bug 28478: Add Koha::Biblio->opac_suppressed()

This patch adds a convenient method at the Koha::Biblio level, as a
wrapper for the extrator added on this bug. Following the established
pattern we adopted a while back.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass! All use cases covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize (ashimema) 2025-01-21 13:25:29 UTC
Created attachment 176863 [details] [review]
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed()

This patch makes the following scripts use the new method for checking
suppression:

* opac/opac-ISBDdetail.pl
* opac/opac-MARCdetail.pl
* opac/opac-detail.pl

In the `opac-detail.pl` case, it is a simple change by removing MARC
data traversal in favor of the new method. The code checking
suppression gets moved up so we don't process or make any other
calculations if the record is suppressed.

The other two scripts where completely missing the check and thus
leaking suppressed records.

To test:
1. Pick two records, one marked as suppressed, and the other not
   suppressed.
2. Try acessing them in the OPAC detail page.
=> SUCCESS: Suppressed records are suppressed, and not suppressed ones
are not.
3. Try the same records on the ISBD and MARC view
=> FAIL: They are not suppressed!
4. Apply this patch
5. Repeat 2
=> SUCCESS: Suppression is still respected
6. Repeat 3
=> SUCCESS: Suppression is respected on the ISBD and MARC views
7. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Remember to activate OPAC suppression with
OpacSuppression.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize (ashimema) 2025-01-21 13:27:16 UTC
Was just looking at this and spotted Joubu commenting, so attached my signoff but leaving the final QA to him..

In my opinion, this is a pragmatic approach and is good to go and the follow-up bug is a great further enhancement on top to dry out code in the next bug.
Comment 21 Jonathan Druart 2025-01-22 10:30:12 UTC
Created attachment 176898 [details] [review]
Bug 28478: Add get_opac_suppression extractor

This patch adds an extractor method for the historically hardcoded field
942$n. This way we have a single place in which we code the extraction
and sanitization of its value.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass! All use cases are covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2025-01-22 10:30:15 UTC
Created attachment 176899 [details] [review]
Bug 28478: Add Koha::Biblio->opac_suppressed()

This patch adds a convenient method at the Koha::Biblio level, as a
wrapper for the extrator added on this bug. Following the established
pattern we adopted a while back.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass! All use cases covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2025-01-22 10:30:18 UTC
Created attachment 176900 [details] [review]
Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed()

This patch makes the following scripts use the new method for checking
suppression:

* opac/opac-ISBDdetail.pl
* opac/opac-MARCdetail.pl
* opac/opac-detail.pl

In the `opac-detail.pl` case, it is a simple change by removing MARC
data traversal in favor of the new method. The code checking
suppression gets moved up so we don't process or make any other
calculations if the record is suppressed.

The other two scripts where completely missing the check and thus
leaking suppressed records.

To test:
1. Pick two records, one marked as suppressed, and the other not
   suppressed.
2. Try acessing them in the OPAC detail page.
=> SUCCESS: Suppressed records are suppressed, and not suppressed ones
are not.
3. Try the same records on the ISBD and MARC view
=> FAIL: They are not suppressed!
4. Apply this patch
5. Repeat 2
=> SUCCESS: Suppression is still respected
6. Repeat 3
=> SUCCESS: Suppression is respected on the ISBD and MARC views
7. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Remember to activate OPAC suppression with
OpacSuppression.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Alex Buckley 2025-02-05 02:05:33 UTC
Hi there, 

This patchset doesn't apply cleanly on 24.05.x-security branch.

This is because it is built on top of Bug 37380 which is only on 24.11

Can you please let us know if bug 28478 is needed in 24.05?
Comment 25 Paul Derscheid 2025-02-22 18:17:09 UTC
Applied to 24.11.x-security
Comment 26 Alex Buckley 2025-02-23 22:48:14 UTC
(In reply to Alex Buckley from comment #24)
> Hi there, 
> 
> This patchset doesn't apply cleanly on 24.05.x-security branch.
> 
> This is because it is built on top of Bug 37380 which is only on 24.11
> 
> Can you please let us know if bug 28478 is needed in 24.05?

Following up on this.

Can we please have a rebase for 24.05?

Because it does not apply cleanly when we apply it on that branch.
Comment 27 Tomás Cohen Arazi (tcohen) 2025-02-24 13:49:04 UTC
Created attachment 178572 [details] [review]
[24.05.x] Bug 28478: Add get_opac_suppression extractor

This patch adds an extractor method for the historically hardcoded field
942$n. This way we have a single place in which we code the extraction
and sanitization of its value.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass! All use cases are covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Tomás Cohen Arazi (tcohen) 2025-02-24 13:49:08 UTC
Created attachment 178573 [details] [review]
[24.05.x] Bug 28478: Add Koha::Biblio->opac_suppressed()

This patch adds a convenient method at the Koha::Biblio level, as a
wrapper for the extrator added on this bug. Following the established
pattern we adopted a while back.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass! All use cases covered!
3. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Tomás Cohen Arazi (tcohen) 2025-02-24 13:49:11 UTC
Created attachment 178574 [details] [review]
[24.05.x] Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed()

This patch makes the following scripts use the new method for checking
suppression:

* opac/opac-ISBDdetail.pl
* opac/opac-MARCdetail.pl
* opac/opac-detail.pl

In the `opac-detail.pl` case, it is a simple change by removing MARC
data traversal in favor of the new method. The code checking
suppression gets moved up so we don't process or make any other
calculations if the record is suppressed.

The other two scripts where completely missing the check and thus
leaking suppressed records.

To test:
1. Pick two records, one marked as suppressed, and the other not
   suppressed.
2. Try acessing them in the OPAC detail page.
=> SUCCESS: Suppressed records are suppressed, and not suppressed ones
are not.
3. Try the same records on the ISBD and MARC view
=> FAIL: They are not suppressed!
4. Apply this patch
5. Repeat 2
=> SUCCESS: Suppression is still respected
6. Repeat 3
=> SUCCESS: Suppression is respected on the ISBD and MARC views
7. Sign off :-D

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Remember to activate OPAC suppression with
OpacSuppression.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Tomás Cohen Arazi (tcohen) 2025-02-24 13:54:23 UTC
(In reply to Alex Buckley from comment #26)
¡> Following up on this.
> 
> Can we please have a rebase for 24.05?

Done! I've tested it myself. Works as advertised on the original patchset. (amended) tests pass too.
Comment 31 Alex Buckley 2025-02-24 21:21:58 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #30)
> (In reply to Alex Buckley from comment #26)
> ¡> Following up on this.
> > 
> > Can we please have a rebase for 24.05?
> 
> Done! I've tested it myself. Works as advertised on the original patchset.
> (amended) tests pass too.

Thank you very much Tomás!
Comment 32 Alex Buckley 2025-02-24 21:22:37 UTC
Applied to 24.05.x-security branch.

Test plans in 24.05 patches all worked as expected.
Comment 33 Fridolin Somers 2025-02-26 12:55:19 UTC
Applied to 23.11.x-security
Comment 34 Katrin Fischer 2025-03-06 12:55:54 UTC
Pushed for 25.05!

Well done everyone, thank you!