Bug 20292

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

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

(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

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

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

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

I witnessed this software patch in action and it affects Koha as described. I signoff this patch.

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>

+            $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 ?

(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.

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>

(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

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?

Nice work!

Pushed to master for 19.11.00

Enhancement not pushed to 19.05.x

(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.

(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.

this applied clean, passes all test, so I will go ahead and backport to 19.05.x for 19.05.08

Enhancement will not be backported to 18.11.x.