Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library
Summary: Checkouts ( and possibly checkins and other actions ) will use the patron hom...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 14507
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-10 16:36 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library (3.08 KB, patch)
2016-05-10 16:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library (3.08 KB, patch)
2016-05-10 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16492 [QA Followup] (758 bytes, patch)
2016-05-24 18:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library (5.27 KB, patch)
2016-05-24 18:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library (5.33 KB, patch)
2016-05-25 12:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16492: Test that no_set_userenv will not set userenv if not exist yet (3.43 KB, patch)
2016-05-25 16:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library (5.33 KB, patch)
2016-06-08 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16492: Test that no_set_userenv will not set userenv if not exist yet (3.43 KB, patch)
2016-06-08 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16492: Add a FIXME to explain the possible problem with no_set_userenv (860 bytes, patch)
2016-06-08 14:33 UTC, Jonathan Druart
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 2016-05-10 16:36:15 UTC
Bug 14507 introduced the use of checkpw in C4::SIP::ILS::Patron so that non-Koha internal authentication processes would be able to function via SIP ( LDAP et al ).

The problem is that checkpw changes the userenv to that of the patron! This is not usually an issue in Koha because most of the time that patron running through checkpw is the one to be logged in.

Aside from SIP2 the only other area where this may be an issue is in SCO when using SelfCheckoutByLogin.
Comment 1 Kyle M Hall 2016-05-10 16:39:21 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-05-10 16:41:07 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2016-05-16 06:14:21 UTC
I think that we need a test for this one.
Comment 4 Colin Campbell 2016-05-16 08:28:31 UTC
The call to checkpw in Patron.pm is weird and will probably make people think some magic is being invoked. the parameters should be more explicit i.e. :

checkpw($dvh, $self->{userid}, $pwd, undef, undef, 1);

with a comment to document
Comment 5 Kyle M Hall 2016-05-24 18:05:21 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2016-05-24 18:31:50 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2016-05-24 18:32:45 UTC
(In reply to Srdjan Jankovic from comment #3)
> I think that we need a test for this one.

Unit test added!
Comment 8 Kyle M Hall 2016-05-24 18:33:06 UTC
(In reply to Colin Campbell from comment #4)
> The call to checkpw in Patron.pm is weird and will probably make people
> think some magic is being invoked. the parameters should be more explicit
> i.e. :
> 
> checkpw($dvh, $self->{userid}, $pwd, undef, undef, 1);
> 
> with a comment to document

Fixed in new patch!
Comment 9 Chris Cormack 2016-05-25 12:10:43 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-05-25 16:37:27 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-05-25 16:39:16 UTC
Should not we just set a correct userenv instead of not creating at all?
There are several places where an error could occur if not set.
Comment 12 Kyle M Hall 2016-06-07 16:30:29 UTC
(In reply to Jonathan Druart from comment #11)
> Should not we just set a correct userenv instead of not creating at all?
> There are several places where an error could occur if not set.

That's a good idea. Would you be able to add that as a followup?
Comment 13 Jonathan Druart 2016-06-08 14:33:32 UTC
Created attachment 52188 [details] [review]
Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library

Bug 14507 introduced the use of checkpw in C4::SIP::ILS::Patron so that
non-Koha internal authentication processes would be able to function via
SIP ( LDAP et al ).

The problem is that checkpw changes the userenv to that of the patron!
This is not usually an issue in Koha because most of the time that
patron running through checkpw is the one to be logged in.

Aside from SIP2 the only other area where this may be an issue is in SCO
when using SelfCheckoutByLogin.

Test Plan:
1) On master, check out an item to a patron via SIP2
2) Note the checkout lists the item as having been checked out
   from the patron's home library not matter which library is was
   supposed to be checked out from.
3) Apply this patch
4) Re-checkout the item
5) The item should now be checked out as if it was checked out from
   the library as defined in the SIP configuration file.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 14 Jonathan Druart 2016-06-08 14:33:40 UTC
Created attachment 52189 [details] [review]
Bug 16492: Test that no_set_userenv will not set userenv if not exist yet
Comment 15 Jonathan Druart 2016-06-08 14:33:44 UTC
Created attachment 52190 [details] [review]
Bug 16492: Add a FIXME to explain the possible problem with no_set_userenv
Comment 16 Jonathan Druart 2016-06-08 14:34:42 UTC
(In reply to Kyle M Hall from comment #12)
> (In reply to Jonathan Druart from comment #11)
> > Should not we just set a correct userenv instead of not creating at all?
> > There are several places where an error could occur if not set.
> 
> That's a good idea. Would you be able to add that as a followup?

I do not have the time right now. Let's see later, when we will need to reuse again this parameter.
Comment 17 Kyle M Hall 2016-06-10 17:32:54 UTC
Pushed to master for 16.11, thanks for the followups Jonathan!
Comment 18 Frédéric Demians 2016-06-15 07:45:17 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 19 Julian Maurice 2016-06-16 09:13:17 UTC
Patch pushed to 3.22.x, will be in 3.22.8