Bug 20292 - Filter/censor info sent via SIP
Summary: Filter/censor info sent via SIP
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-24 08:00 UTC by Kyle M Hall
Modified: 2019-06-07 07:56 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


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?