Bug 21301 - Restriction of the informations given by GetRecords ILS-DI service
Summary: Restriction of the informations given by GetRecords ILS-DI service
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 18081
  Show dependency treegraph
 
Reported: 2018-08-31 09:42 UTC by abernaud
Modified: 2021-12-13 21:09 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
For privacy protection, ILS-DI webservice GetRecords will not give patron information anymore. Also old issues are not given anymore. This removes method C4::Circulation::GetBiblioIssues().
Version(s) released in:
21.05.00,20.11.02,20.05.08


Attachments
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service (4.99 KB, patch)
2019-10-02 15:03 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service (1.56 KB, patch)
2019-10-03 13:45 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service (5.05 KB, patch)
2019-11-07 14:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service (1.62 KB, patch)
2019-11-07 14:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21301: (QA follow-up) Fix number of tests (714 bytes, patch)
2019-11-08 07:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service (820 bytes, patch)
2020-03-26 12:02 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: add unit tests (2.41 KB, patch)
2020-09-04 08:02 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service (5.06 KB, patch)
2020-11-05 10:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service (1.62 KB, patch)
2020-11-05 10:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: (QA follow-up) Fix number of tests (674 bytes, patch)
2020-11-05 10:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service (820 bytes, patch)
2020-11-05 10:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: add unit tests (2.41 KB, patch)
2020-11-05 10:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service (5.12 KB, patch)
2020-11-05 10:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service (1.67 KB, patch)
2020-11-05 10:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 21301: (QA follow-up) Fix number of tests (722 bytes, patch)
2020-11-05 10:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service (868 bytes, patch)
2020-11-05 10:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 21301: add unit tests (2.46 KB, patch)
2020-11-05 10:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service (5.18 KB, patch)
2021-01-11 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service (1.72 KB, patch)
2021-01-11 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21301: (QA follow-up) Fix number of tests (779 bytes, patch)
2021-01-11 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service (925 bytes, patch)
2021-01-11 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21301: add unit tests (2.51 KB, patch)
2021-01-11 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description abernaud 2018-08-31 09:42:59 UTC
Actually, the GetRecords service gives many information :
bibliographic data (marcxml field)
item data (items field)
issue data (issues field)

The issue data should not be sent by these service.
For example, this service is actually used by EDS clients to display real-time item informations on EDS. Ebsco should not have access to the issues information, even if they are partially anonymised (only the firstname of the patron is sent) in 16.05 and maybe in the later versions of Koha.

This would be more compliant with GDPR (see bug 18081).
Thanks,
Anne-Claire
Comment 1 Fridolin Somers 2019-10-02 15:03:06 UTC
Created attachment 93495 [details] [review]
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
bibliographic data (marcxml field)
item data (items field)
issue data (issues field)

The issue data should not be sent by these service, its a privacy problem.
Biblio title and author are redundant.
This patch changes to use Koha::Checkouts to get issues informations with only the addition of item barcode.

Test plan :
1) Enable ILS-DI webservice
2) Checkout an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the issues tag content
Comment 2 Fridolin Somers 2019-10-03 13:45:55 UTC
Created attachment 93603 [details] [review]
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
borrowernumber (reserves field)

The issue data should not be sent by these service, its a privacy problem.

Test plan :
1) Enable ILS-DI webservice
2) Place hold on an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the reserves tag content
Comment 3 Brendan Gallagher 2019-10-04 10:42:27 UTC
Failed the PODcoverage in the qa-tools tests.
Comment 4 Martin Renvoize 2019-11-07 14:28:05 UTC
False positive on the POD issue, we remove a whole routine and as such remove the corresponding POD too.
Comment 5 Martin Renvoize 2019-11-07 14:28:37 UTC
Created attachment 95158 [details] [review]
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
bibliographic data (marcxml field)
item data (items field)
issue data (issues field)

The issue data should not be sent by these service, its a privacy problem.
Biblio title and author are redundant.
This patch changes to use Koha::Checkouts to get issues informations with only the addition of item barcode.

Test plan :
1) Enable ILS-DI webservice
2) Checkout an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the issues tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-11-07 14:28:41 UTC
Created attachment 95159 [details] [review]
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
borrowernumber (reserves field)

The issue data should not be sent by these service, its a privacy problem.

Test plan :
1) Enable ILS-DI webservice
2) Place hold on an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the reserves tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-11-07 14:39:03 UTC
Works as expected, checked if the online documentation needed any updating to reflect the change, but neither issues nor reservations have any details in the examples.

Looks good to me, signing off.
Comment 8 Marcel de Rooy 2019-11-08 07:17:59 UTC
(In reply to Fridolin SOMERS from comment #2)
> The issue data should not be sent by these service, its a privacy problem.

<reserve>
<suspend>0</suspend>
<biblionumber>633</biblionumber>
<timestamp>2019-11-08 07:06:05</timestamp>
<borrowernumber>5</borrowernumber>
<lowestPriority>0</lowestPriority>
<reservedate>2019-11-08</reservedate>
<priority>1</priority>
<reserve_id>110</reserve_id>
<branchcode>CPL</branchcode>
<item_level_hold>0</item_level_hold>
</reserve>

Is this really a privacy issue? In order to identify the person associated with borrowernumber 5, you'll need additional information.
So if this is just theoretical, then removing the borrowernumber might not be enough ;) If I have biblionumber and reservedata or timestamp, I might also be able to find the associated borrower?

No blocker, just pushing a bit..
Comment 9 Marcel de Rooy 2019-11-08 07:23:36 UTC
More serious:

<issue>
<issuedate>2019-11-08 07:21:08</issuedate>
<onsite_checkout>0</onsite_checkout>
<itemnumber>598</itemnumber>
<branchcode>CPL</branchcode>
<date_due>2019-11-22 23:59:00</date_due>
<barcode>perl01</barcode>
<issue_id>690</issue_id>
<renewals>0</renewals>
<timestamp>2019-11-08 07:21:08</timestamp>
<auto_renew>0</auto_renew>
<borrowernumber>5</borrowernumber>
</issue>

If you are concerned about borrowernumber in hold, then you should be for issue too. Inconsistent !
Comment 10 Marcel de Rooy 2019-11-08 07:26:14 UTC
Lowering the severity to Major
Comment 11 Marcel de Rooy 2019-11-08 07:27:10 UTC
I do expect additional tests here too. Your test should show that the sensitive information is no longer in the output of GetRecords etc.
Comment 12 Marcel de Rooy 2019-11-08 07:31:02 UTC
Created attachment 95178 [details] [review]
Bug 21301: (QA follow-up) Fix number of tests
Comment 13 Jonathan Druart 2019-12-27 16:31:40 UTC
Did not we display old issues prior to this patch?
Comment 14 Fridolin Somers 2020-03-25 12:16:11 UTC
(In reply to Jonathan Druart from comment #13)
> Did not we display old issues prior to this patch?

Yes indeed, C4::Circulation::GetBiblioIssues() makes issues UNION old_issues.

This is not the purpose of this patch, but isn't it a performance killer ?
Comment 15 Fridolin Somers 2020-03-25 12:34:36 UTC
(In reply to Marcel de Rooy from comment #8)
> 
> Is this really a privacy issue? In order to identify the person associated
> with borrowernumber 5, you'll need additional information.
> So if this is just theoretical, then removing the borrowernumber might not
> be enough ;) If I have biblionumber and reservedata or timestamp, I might
> also be able to find the associated borrower?
> 
> No blocker, just pushing a bit..

Valid question yep.

In my opinion removing borrowernumber is also to avoid using some scripts vulnerability with a valid DB id that may allow SQL injection or data leaking.
Comment 16 Fridolin Somers 2020-03-25 12:35:26 UTC
(In reply to Marcel de Rooy from comment #9)
> 
> If you are concerned about borrowernumber in hold, then you should be for
> issue too. Inconsistent !

Oh good catch, I'll fix
Comment 17 Jonathan Druart 2020-03-25 12:53:45 UTC
(In reply to Fridolin SOMERS from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > Did not we display old issues prior to this patch?
> 
> Yes indeed, C4::Circulation::GetBiblioIssues() makes issues UNION old_issues.
> 
> This is not the purpose of this patch, but isn't it a performance killer ?

Yes, but it's a change that is not advertised by the patch. What is the expected behavior?
Comment 18 Fridolin Somers 2020-03-25 13:47:53 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Fridolin SOMERS from comment #14)
> > (In reply to Jonathan Druart from comment #13)
> > > Did not we display old issues prior to this patch?
> > 
> > Yes indeed, C4::Circulation::GetBiblioIssues() makes issues UNION old_issues.
> > 
> > This is not the purpose of this patch, but isn't it a performance killer ?
> 
> Yes, but it's a change that is not advertised by the patch. What is the
> expected behavior?

Do we create another patch and this one will depend on it ?
Comment 19 Arthur Suzuki 2020-03-25 13:58:59 UTC
(In reply to Fridolin SOMERS from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > Did not we display old issues prior to this patch?
> 
> Yes indeed, C4::Circulation::GetBiblioIssues() makes issues UNION old_issues.
> 
> This is not the purpose of this patch, but isn't it a performance killer ?

In my opinion it's useful to get current issues but if we want previous checkout there are Rest API routes to query patrons checkouts.

I'd say, we don't need nor want GetRecords to show previous issues.
Also, removing the borrowernumber is still very useful, any script, having the borrowernumber could easily grab borrower information from GetPatron.
Comment 20 Fridolin Somers 2020-03-26 12:02:48 UTC
Created attachment 101818 [details] [review]
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service
Comment 21 Fridolin Somers 2020-09-04 08:02:34 UTC
Created attachment 109646 [details] [review]
Bug 21301: add unit tests

Run t/db_dependent/ILSDI_Services.t with and without previous patches
Comment 22 Fridolin Somers 2020-09-04 08:04:39 UTC
Here are UT to check data removed from GetRecords ILS-DI service
Comment 23 David Nind 2020-11-05 10:10:28 UTC
Patch no longer applies 8-(..

Applying: Bug 21301: (QA follow-up) Fix number of tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Circulation/issue.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Circulation/issue.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation/issue.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 21301: (QA follow-up) Fix number of tests
Comment 24 Fridolin Somers 2020-11-05 10:20:12 UTC
Created attachment 113061 [details] [review]
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
bibliographic data (marcxml field)
item data (items field)
issue data (issues field)

The issue data should not be sent by these service, its a privacy problem.
Biblio title and author are redundant.
This patch changes to use Koha::Checkouts to get issues informations with only the addition of item barcode.

Test plan :
1) Enable ILS-DI webservice
2) Checkout an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the issues tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Fridolin Somers 2020-11-05 10:20:32 UTC
Created attachment 113062 [details] [review]
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
borrowernumber (reserves field)

The issue data should not be sent by these service, its a privacy problem.

Test plan :
1) Enable ILS-DI webservice
2) Place hold on an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the reserves tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Fridolin Somers 2020-11-05 10:20:43 UTC
Created attachment 113063 [details] [review]
Bug 21301: (QA follow-up) Fix number of tests
Comment 27 Fridolin Somers 2020-11-05 10:20:56 UTC
Created attachment 113064 [details] [review]
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service
Comment 28 Fridolin Somers 2020-11-05 10:21:19 UTC
Created attachment 113065 [details] [review]
Bug 21301: add unit tests

Run t/db_dependent/ILSDI_Services.t with and without previous patches
Comment 29 Fridolin Somers 2020-11-05 10:24:09 UTC
trivial conflict on  Bug 21301: (QA follow-up) Fix number of tests
Comment 30 David Nind 2020-11-05 10:43:35 UTC
Created attachment 113068 [details] [review]
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
bibliographic data (marcxml field)
item data (items field)
issue data (issues field)

The issue data should not be sent by these service, its a privacy problem.
Biblio title and author are redundant.
This patch changes to use Koha::Checkouts to get issues informations with only the addition of item barcode.

Test plan :
1) Enable ILS-DI webservice
2) Checkout an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the issues tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 31 David Nind 2020-11-05 10:43:39 UTC
Created attachment 113069 [details] [review]
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
borrowernumber (reserves field)

The issue data should not be sent by these service, its a privacy problem.

Test plan :
1) Enable ILS-DI webservice
2) Place hold on an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the reserves tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 David Nind 2020-11-05 10:43:44 UTC
Created attachment 113070 [details] [review]
Bug 21301: (QA follow-up) Fix number of tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 David Nind 2020-11-05 10:43:48 UTC
Created attachment 113071 [details] [review]
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service

Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2020-11-05 10:43:52 UTC
Created attachment 113072 [details] [review]
Bug 21301: add unit tests

Run t/db_dependent/ILSDI_Services.t with and without previous patches

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 Nick Clemens 2021-01-11 13:24:40 UTC
Created attachment 115022 [details] [review]
Bug 21301: Remove patron informations in issues for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
bibliographic data (marcxml field)
item data (items field)
issue data (issues field)

The issue data should not be sent by these service, its a privacy problem.
Biblio title and author are redundant.
This patch changes to use Koha::Checkouts to get issues informations with only the addition of item barcode.

Test plan :
1) Enable ILS-DI webservice
2) Checkout an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the issues tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Nick Clemens 2021-01-11 13:24:43 UTC
Created attachment 115023 [details] [review]
Bug 21301: Remove patron informations in reserves for GetRecords ILS-DI service

Actually, the GetRecords service gives many information :
borrowernumber (reserves field)

The issue data should not be sent by these service, its a privacy problem.

Test plan :
1) Enable ILS-DI webservice
2) Place hold on an item
3) Go to {opac}/cgi-bin/koha/ilsdi.pl?service=GetRecords&id={biblionumber}
4) Check the reserves tag content

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 Nick Clemens 2021-01-11 13:24:47 UTC
Created attachment 115024 [details] [review]
Bug 21301: (QA follow-up) Fix number of tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Nick Clemens 2021-01-11 13:24:51 UTC
Created attachment 115025 [details] [review]
Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Nick Clemens 2021-01-11 13:24:57 UTC
Created attachment 115026 [details] [review]
Bug 21301: add unit tests

Run t/db_dependent/ILSDI_Services.t with and without previous patches

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Jonathan Druart 2021-01-12 10:38:04 UTC
Please add a release note about the behaviour change.
Comment 41 Jonathan Druart 2021-01-12 10:42:50 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 42 Fridolin Somers 2021-01-14 16:15:01 UTC
(In reply to Jonathan Druart from comment #40)
> Please add a release note about the behaviour change.

Done
Comment 43 Fridolin Somers 2021-01-14 16:17:56 UTC
Pushed to 20.11.x for 20.11.02
Comment 44 Andrew Fuerste-Henry 2021-01-20 01:52:39 UTC
Pushed to 20.05.x for 20.05.08
Comment 45 Victor Grousset/tuxayo 2021-01-22 19:49:09 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.