Bug 20292 - Filter/censor info sent via SIP
Summary: Filter/censor info sent via SIP
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 25107
  Show dependency treegraph
 
Reported: 2018-02-24 08:00 UTC by Kyle M Hall
Modified: 2020-11-17 20:06 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This enhancement allows the administrator to set if, and which, fields should not be sent to third-party SIP2 clients for privacy reasons.
Version(s) released in:
19.11.00, 19.05.08


Attachments
Bug 20292: Filter/censor info sent via SIP (34.61 KB, patch)
2018-11-30 17:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20292: Filter/censor info sent via SIP (36.43 KB, patch)
2019-02-27 18:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20292: Filter/censor info sent via SIP (36.48 KB, patch)
2019-02-27 18:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20292: Filter/censor info sent via SIP (36.81 KB, patch)
2019-02-27 20:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20292: Filter/censor info sent via SIP (36.93 KB, patch)
2019-02-28 11:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20292: Filter/censor info sent via SIP (37.02 KB, patch)
2019-06-07 07:50 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 Kyle M Hall 2018-02-24 08:00:09 UTC
1) Add the ability to define a list of fields to remove from SIP2 responses on a per account basis
2) Modify C4::SIP::Sip::add_field to skip fields in that list
3) Modify C4::SIP::Sip::maybe_add to skip fields in that list
Comment 1 Kyle M Hall 2018-11-30 17:29:30 UTC
Created attachment 82802 [details] [review]
Bug 20292: Filter/censor info sent via SIP

Some libraries would like to limit the amount of personal information a SIP server sends
to arbitrary parties on a per-login basis.

Test Plan:
1) Add a new key/value pair to one of your existing login stanzas in your SIP config file
   For example: hide_fields="BD,BE,BF,PB"
2) Restart SIP
3) Send a SIP message that would normally return those fields ( in this example, a Patron Information Request )
4) Note the response has had those fields removed
Comment 2 Christopher Davis 2019-01-28 18:20:54 UTC
Greetings devs,

This bug needs some love. If my institution is the one which is sponsoring this development, am I allowed to signoff this patch?

Thank you,

Christopher Davis
Comment 3 Todd Goatley 2019-02-22 16:21:56 UTC
(In reply to Christopher Davis from comment #2)
> Greetings devs,
> 
> This bug needs some love. If my institution is the one which is sponsoring
> this development, am I allowed to signoff this patch?
> 
> Thank you,
> 
> Christopher Davis

HI Christopher!

Yes, if you're in a position to do so, please signoff this patch.

Cheers!
Todd
Comment 4 Kyle M Hall 2019-02-27 18:51:40 UTC
Created attachment 85793 [details] [review]
Bug 20292: Filter/censor info sent via SIP

Some libraries would like to limit the amount of personal information a SIP server sends
to arbitrary parties on a per-login basis.

Test Plan:
1) Add a new key/value pair to one of your existing login stanzas in your SIP config file
   For example: hide_fields="BD,BE,BF,PB"
2) Restart SIP
3) Send a SIP message that would normally return those fields ( in this example, a Patron Information Request )
4) Note the response has had those fields removed
Comment 5 Kyle M Hall 2019-02-27 18:52:35 UTC
Created attachment 85794 [details] [review]
Bug 20292: Filter/censor info sent via SIP

Some libraries would like to limit the amount of personal information a SIP server sends
to arbitrary parties on a per-login basis.

Test Plan:
1) Add a new key/value pair to one of your existing login stanzas in your SIP config file
   For example: hide_fields="BD,BE,BF,PB"
2) Restart SIP
3) Send a SIP message that would normally return those fields ( in this example, a Patron Information Request )
4) Note the response has had those fields removed
Comment 6 Kyle M Hall 2019-02-27 20:41:11 UTC
Created attachment 85799 [details] [review]
Bug 20292: Filter/censor info sent via SIP

Some libraries would like to limit the amount of personal information a SIP server sends
to arbitrary parties on a per-login basis.

Test Plan:
1) Add a new key/value pair to one of your existing login stanzas in your SIP config file
   For example: hide_fields="BD,BE,BF,PB"
2) Restart SIP
3) Send a SIP message that would normally return those fields ( in this example, a Patron Information Request )
4) Note the response has had those fields removed
Comment 7 Christopher Davis 2019-02-27 21:42:10 UTC
I witnessed this software patch in action and it affects Koha as described. I signoff this patch.
Comment 8 Kyle M Hall 2019-02-28 11:32:36 UTC
Created attachment 85849 [details] [review]
Bug 20292: Filter/censor info sent via SIP

Some libraries would like to limit the amount of personal information a SIP server sends
to arbitrary parties on a per-login basis.

Test Plan:
1) Add a new key/value pair to one of your existing login stanzas in your SIP config file
   For example: hide_fields="BD,BE,BF,PB"
2) Restart SIP
3) Send a SIP message that would normally return those fields ( in this example, a Patron Information Request )
4) Note the response has had those fields removed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Christopher Davis <tubaclarinet@protonmail.com>
Comment 9 Marcel de Rooy 2019-05-31 09:26:27 UTC
+            $resp .= add_field( FID_FEE_AMT, $status->fee_amount, $server );
+            $resp .= maybe_add( FID_CURRENCY,       $status->sip_currency, $server );
+            $resp .= maybe_add( FID_FEE_TYPE,       $status->sip_fee_type, $server );
+            $resp .= maybe_add( FID_TRANSACTION_ID, $status->transaction_id, $server );

I have some doubts here. Will this work if people add such fields to the hidden list? Or should these be added no matter what? So moved to add_field?
Maybe there are more ?
Comment 10 Kyle M Hall 2019-05-31 10:52:54 UTC
(In reply to Marcel de Rooy from comment #9)
> +            $resp .= add_field( FID_FEE_AMT, $status->fee_amount, $server );
> +            $resp .= maybe_add( FID_CURRENCY,       $status->sip_currency,
> $server );
> +            $resp .= maybe_add( FID_FEE_TYPE,       $status->sip_fee_type,
> $server );
> +            $resp .= maybe_add( FID_TRANSACTION_ID,
> $status->transaction_id, $server );
> 
> I have some doubts here. Will this work if people add such fields to the
> hidden list? Or should these be added no matter what? So moved to add_field?
> Maybe there are more ?

I see no reason to override a librarian's ability to scrub any particular SIP field. That just makes the feature inconsistent, and for all we know maybe there *will* be a scenario where a library needs to not send those fields to a particular service.
Comment 11 Marcel de Rooy 2019-06-07 07:50:42 UTC
Created attachment 90408 [details] [review]
Bug 20292: Filter/censor info sent via SIP

Some libraries would like to limit the amount of personal information a SIP server sends
to arbitrary parties on a per-login basis.

Test Plan:
1) Add a new key/value pair to one of your existing login stanzas in your SIP config file
   For example: hide_fields="BD,BE,BF,PB"
2) Restart SIP
3) Send a SIP message that would normally return those fields ( in this example, a Patron Information Request )
4) Note the response has had those fields removed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Christopher Davis <tubaclarinet@protonmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2019-06-07 07:52:41 UTC
(In reply to Kyle M Hall from comment #10)
> I see no reason to override a librarian's ability to scrub any particular
> SIP field. That just makes the feature inconsistent, and for all we know
> maybe there *will* be a scenario where a library needs to not send those
> fields to a particular service.

In the assumption that we are not stripping fields that the SIP standard dictates, I guess you are right. And if we do, we should not ;)

Passed QA
Comment 13 Marcel de Rooy 2019-06-07 07:56:16 UTC
Note to RM: We should be a bit careful with future SIP patches that still have the old maybe_add behavior in mind? In some cases they should use add_field now?
Comment 14 Martin Renvoize 2019-06-24 14:47:49 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Fridolin Somers 2019-07-24 07:20:39 UTC
Enhancement not pushed to 19.05.x
Comment 16 Christopher Davis 2020-02-12 17:07:35 UTC
(In reply to Fridolin SOMERS from comment #15)
> Enhancement not pushed to 19.05.x

Hear hear! Yes, please back-port this to 19.05.x. We need it at our site a.s.a.p. Please let me know how I can help make this happen.
Comment 17 Katrin Fischer 2020-02-12 21:50:36 UTC
(In reply to Christopher Davis from comment #16)
> (In reply to Fridolin SOMERS from comment #15)
> > Enhancement not pushed to 19.05.x
> 
> Hear hear! Yes, please back-port this to 19.05.x. We need it at our site
> a.s.a.p. Please let me know how I can help make this happen.

This is marked as an enhancement, not as a bug - enhancements are usually not backported. It's up to the Rmaints to make exceptions, but it might help to reason why this should be in an oldstable version.
Comment 18 Lucas Gass 2020-02-12 23:21:16 UTC
this applied clean, passes all test, so I will go ahead and backport to 19.05.x for 19.05.08
Comment 19 Hayley Mapley 2020-02-13 23:13:03 UTC
Enhancement will not be backported to 18.11.x.