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
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
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
Failed the PODcoverage in the qa-tools tests.
False positive on the POD issue, we remove a whole routine and as such remove the corresponding POD too.
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>
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>
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.
(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..
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 !
Lowering the severity to Major
I do expect additional tests here too. Your test should show that the sensitive information is no longer in the output of GetRecords etc.
Created attachment 95178 [details] [review] Bug 21301: (QA follow-up) Fix number of tests
Did not we display old issues prior to this patch?
(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 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.
(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
(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?
(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 ?
(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.
Created attachment 101818 [details] [review] Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service
Created attachment 109646 [details] [review] Bug 21301: add unit tests Run t/db_dependent/ILSDI_Services.t with and without previous patches
Here are UT to check data removed from GetRecords ILS-DI service
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
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>
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>
Created attachment 113063 [details] [review] Bug 21301: (QA follow-up) Fix number of tests
Created attachment 113064 [details] [review] Bug 21301: (follow-up) Remove borrowernumber in issues for GetRecords ILS-DI service
Created attachment 113065 [details] [review] Bug 21301: add unit tests Run t/db_dependent/ILSDI_Services.t with and without previous patches
trivial conflict on Bug 21301: (QA follow-up) Fix number of tests
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>
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>
Created attachment 113070 [details] [review] Bug 21301: (QA follow-up) Fix number of tests Signed-off-by: David Nind <david@davidnind.com>
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>
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>
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>
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>
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>
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>
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>
Please add a release note about the behaviour change.
Pushed to master for 21.05, thanks to everybody involved!
(In reply to Jonathan Druart from comment #40) > Please add a release note about the behaviour change. Done
Pushed to 20.11.x for 20.11.02
Pushed to 20.05.x for 20.05.08
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.