Bug 24754 - UserEnv not set for ISLDI requests
Summary: UserEnv not set for ISLDI requests
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24759
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-28 01:42 UTC by Nick Clemens
Modified: 2020-04-03 22:04 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24754: Set userenv for ilsdi requests (1.71 KB, patch)
2020-02-28 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24754: Set userenv for ilsdi requests (1.78 KB, patch)
2020-02-28 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24754: (QA follow-up) opac -> api for 'interface' (855 bytes, patch)
2020-02-28 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23165: Set branch for renewal (1.65 KB, patch)
2020-02-28 15:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24754: Simplify tests (5.14 KB, patch)
2020-03-03 13:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24754: Test intranet at the same time (3.34 KB, patch)
2020-03-03 13:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24754: Set branch for renewal (1.60 KB, patch)
2020-03-03 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24754: Set userenv for ilsdi requests (2.88 KB, patch)
2020-03-03 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24754: Set userenv for ilsdi requests (2.30 KB, patch)
2020-03-06 12:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24754: Set userenv for ilsdi requests (2.34 KB, patch)
2020-03-07 05:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 24754: (QA follow-up) Add timestamp, remove 11th parameter (946 bytes, patch)
2020-03-20 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24754: Set userenv for ilsdi requests (2.44 KB, patch)
2020-03-24 13:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24754: (QA follow-up) Add timestamp, remove 11th parameter (946 bytes, patch)
2020-03-24 13:58 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 Nick Clemens 2020-02-28 01:42:20 UTC
This was discovered via Renewals where no branch of the wrong branch was set in statistics.

When making ILSDI requests the scripts do no set the userenv.
Comment 1 Nick Clemens 2020-02-28 14:59:03 UTC
Created attachment 99768 [details] [review]
Bug 24754: Set userenv for ilsdi requests

To test:
1 - Enable ILSDI in systempreferences
2 - Checkout an item to a patron
3 - Enable Renewal logs
4 - Ensure you will be able to renew the item several times
5 - Open an incognito/private browser
6 - Renew item via ilsdi:
    http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128
7 - Check action logs - note renewal is indicated as opac
8 - In private browser open a new tab
9 - Sign in to staff client
10 - Return to other tab
11 - Refresh page to repeat renewal
12 - Check action_logs
13 - The second renewal indicates it was renewed via intranet by logged in staff member
14 - Apply patch
15 - restart_all
16 - Ensure staff client still signed in
17 - Repeate renewal again
18 - Check logs, renewal now indicates opac again (as it should)
Comment 2 Martin Renvoize 2020-02-28 15:31:16 UTC
Comment on attachment 99768 [details] [review]
Bug 24754: Set userenv for ilsdi requests

Review of attachment 99768 [details] [review]:
-----------------------------------------------------------------

::: opac/ilsdi.pl
@@ +129,5 @@
>  
> +# Set the userenv
> +C4::Context->_new_userenv(1);
> +C4::Context->set_userenv(
> +    undef, undef, undef, 'CRON', 'CRON', undef,

Those 'CRON' values stand out as a little weird here to me.
Comment 3 Martin Renvoize 2020-02-28 15:35:31 UTC
Comment on attachment 99768 [details] [review]
Bug 24754: Set userenv for ilsdi requests

Review of attachment 99768 [details] [review]:
-----------------------------------------------------------------

::: opac/ilsdi.pl
@@ +129,5 @@
>  
> +# Set the userenv
> +C4::Context->_new_userenv(1);
> +C4::Context->set_userenv(
> +    undef, undef, undef, 'CRON', 'CRON', undef,

OK.. so those are being used as 'firstname' and 'surname'.. weird..  

CRON is probably OK

@@ +132,5 @@
> +C4::Context->set_userenv(
> +    undef, undef, undef, 'CRON', 'CRON', undef,
> +    undef, undef, undef, undef,  undef
> +);
> +C4::Context->interface('opac'); #FIXME 'ilsdi' should be a valid value?

Valid values are currently 'api', 'opac', 'intranet', 'sip', 'cron' and 'commandline'..

I reckon 'api' would fit reasonably well here?
Comment 4 Martin Renvoize 2020-02-28 15:37:54 UTC
Created attachment 99772 [details] [review]
Bug 24754: Set userenv for ilsdi requests

To test:
1 - Enable ILSDI in systempreferences
2 - Checkout an item to a patron
3 - Enable Renewal logs
4 - Ensure you will be able to renew the item several times
5 - Open an incognito/private browser
6 - Renew item via ilsdi:
    http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128
7 - Check action logs - note renewal is indicated as opac
8 - In private browser open a new tab
9 - Sign in to staff client
10 - Return to other tab
11 - Refresh page to repeat renewal
12 - Check action_logs
13 - The second renewal indicates it was renewed via intranet by logged in staff member
14 - Apply patch
15 - restart_all
16 - Ensure staff client still signed in
17 - Repeate renewal again
18 - Check logs, renewal now indicates opac again (as it should)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-02-28 15:37:58 UTC
Created attachment 99773 [details] [review]
Bug 24754: (QA follow-up) opac -> api for 'interface'
Comment 6 Nick Clemens 2020-02-28 15:56:46 UTC
Created attachment 99778 [details] [review]
Bug 23165: Set branch for renewal

Originally I used 'opac' as the interface so that we would follow
opac rules for setting the branches for renewals.

If using api we must explicitly state the branch - falling back to
the checkout branch is the simplest fix as we don't have a user here.

This also corrects typo if setting surname/firstname to CRON instead of ILSDI
Comment 7 Jonathan Druart 2020-03-03 13:26:43 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2020-03-03 13:26:46 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2020-03-03 13:32:26 UTC
Created attachment 100006 [details] [review]
Bug 24754: Set branch for renewal

Originally I used 'opac' as the interface so that we would follow
opac rules for setting the branches for renewals.

If using api we must explicitly state the branch - falling back to
the checkout branch is the simplest fix as we don't have a user here.

This also corrects typo if setting surname/firstname to CRON instead of ILSDI
Comment 10 Jonathan Druart 2020-03-03 14:11:17 UTC
Note: If I tested well, on top of bug 24759:
Without the patch, step 7 indicates "intranet" (test plan expects opac)

Also, test plan is wrong at 18, as the patch now put "api", not "opac".
However I think we should record opac, and depends on bug 24759.

It could help to have an updated test plan, as well as a quick message in the git body explaining what we really want to achieve with this patch.
Comment 11 Nick Clemens 2020-03-03 14:52:27 UTC
Created attachment 100035 [details] [review]
Bug 24754: Set userenv for ilsdi requests

Currently ILSDI scripts do not have a userenv set. This affects both
action_logs and statistics.

For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded

For statistics we need to speify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there.

To test:
1 - Enable ILSDI in systempreferences
2 - Checkout an item to a patron
3 - Enable Renewal logs
4 - Ensure you will be able to renew the item several times
5 - Open an incognito/private browser
6 - Renew item via ilsdi:
    http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128
7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one
8 - In private browser open a new tab
9 - Sign in to staff client
10 - Return to other tab
11 - Refresh page to repeat renewal
12 - Check action_logs
13 - The second renewal indicates it was renewed via intranet by logged in staff member
14 - Apply patch
15 - restart_all
16 - Ensure staff client still signed in
17 - Repeate renewal again
18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab
Comment 12 Jonathan Druart 2020-03-04 12:09:53 UTC
Why do you pass the branchcode's checkout and do not let renewal_branchcode/OpacRenewalBranch does the job?
Comment 13 Marcel de Rooy 2020-03-06 09:07:20 UTC
(In reply to Jonathan Druart from comment #12)
> Why do you pass the branchcode's checkout and do not let
> renewal_branchcode/OpacRenewalBranch does the job?

Please change status if there is something with the patch and it needs an answer. This helps us to skip such patches. It is the responsibility of the author to respond.
Comment 14 Nick Clemens 2020-03-06 12:16:53 UTC
Created attachment 100269 [details] [review]
Bug 24754: Set userenv for ilsdi requests

Currently ILSDI scripts do not have a userenv set. This affects both
action_logs and statistics.

For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded

For statistics we need to specify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there.

To test:
1 - Enable ILSDI in systempreferences
2 - Checkout an item to a patron
3 - Enable Renewal logs
4 - Ensure you will be able to renew the item several times
5 - Open an incognito/private browser
6 - Renew item via ilsdi:
    http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128
7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one
8 - In private browser open a new tab
9 - Sign in to staff client
10 - Return to other tab
11 - Refresh page to repeat renewal
12 - Check action_logs
13 - The second renewal indicates it was renewed via intranet by logged in staff member
14 - Apply patch
15 - restart_all
16 - Ensure staff client still signed in
17 - Repeate renewal again
18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab
Comment 15 Nick Clemens 2020-03-06 12:17:24 UTC
(In reply to Jonathan Druart from comment #12)
> Why do you pass the branchcode's checkout and do not let
> renewal_branchcode/OpacRenewalBranch does the job?

Old code, was before dependency.

Patch updated, back to NSO
Comment 16 Jonathan Druart 2020-03-06 12:35:13 UTC
Ok but then it will explode in some cases Nick. For instance the logged in user does not exist.
Comment 17 Jonathan Druart 2020-03-06 13:10:15 UTC
(In reply to Jonathan Druart from comment #16)
> Ok but then it will explode in some cases Nick. For instance the logged in
> user does not exist.

Scratch that, it won't reach that code for "opac"
Comment 18 David Nind 2020-03-07 05:22:35 UTC
Created attachment 100305 [details] [review]
Bug 24754: Set userenv for ilsdi requests

Currently ILSDI scripts do not have a userenv set. This affects both
action_logs and statistics.

For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded

For statistics we need to specify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there.

To test:
1 - Enable ILS-DI in systempreferences
2 - Checkout an item to a patron
3 - Enable Renewal logs (RenewalLog system preference)
4 - Ensure you will be able to renew the item several times
5 - Open an incognito/private browser
6 - Renew item via ilsdi:
    http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128
7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one
8 - In private browser open a new tab
9 - Sign in to staff client
10 - Return to other tab
11 - Refresh page to repeat renewal
12 - Check action_logs
13 - The second renewal indicates it was renewed via intranet by logged in staff member
14 - Apply patch
15 - restart_all
16 - Ensure staff client still signed in
17 - Repeate renewal again
18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Marcel de Rooy 2020-03-13 14:43:20 UTC
Looking here
Comment 20 Marcel de Rooy 2020-03-13 15:06:22 UTC
The patch does this:
+# Set the userenv
+C4::Context->_new_userenv(1);
+C4::Context->set_userenv(
+    undef, undef, undef, 'ILSDI', 'ILSDI', undef,
+    undef, undef, undef, undef,  undef
+);
+C4::Context->interface('opac');

For comparison, misc/commit_file.pl does this:
# FIXME dummy user so that logging won't fail
# in future, probably should tie to a real user account
C4::Context->set_userenv(0, 'batch', 0, 'batch', 'batch', 'batch', 'batch');

And Koha::Script does this:
        C4::Context->_new_userenv(1);
        C4::Context->set_userenv(
            undef, undef, undef, 'CRON', 'CRON',
            undef, undef, undef, undef,  undef
        );
Might be that the author copied this part of the code? (I think it should be adapted; see below.)

The sub _new_userenv stores a sessionID in activeuser (which is quite confusing). I would not recommend to always use session 1. (Session id is a varchar(32) btw.) Why not use "ILSDI_session", possibly followed by a few digits or so?

Another note: 11 parameters are passed in ilsdi.pl and Koha::Script. I only count 10. We're copy&pasting an error here.
    shift @_; # this is the object
    my ($usernum, $userid, $usercnum, $userfirstname, $usersurname, $userbranch, $branchname, $userflags, $emailaddress, $shibboleth)= [etc]  # these are 10
Comment 21 Nick Clemens 2020-03-13 16:32:31 UTC
(In reply to Marcel de Rooy from comment #20)
> Koha::Script does this:
> Might be that the author copied this part of the code?

Yes, copied from there

> The sub _new_userenv stores a sessionID in activeuser (which is quite
> confusing). I would not recommend to always use session 1. (Session id is a
> varchar(32) btw.) Why not use "ILSDI_session", possibly followed by a few
> digits or so?
Any recommendation on the digits? Use a timestamp or the same for each?


> Another note: 11 parameters are passed in ilsdi.pl and Koha::Script. I only
> count 10. We're copy&pasting an error here.
>     shift @_; # this is the object
>     my ($usernum, $userid, $usercnum, $userfirstname, $usersurname,
> $userbranch, $branchname, $userflags, $emailaddress, $shibboleth)= [etc]  #
> these are 10

Koha Script appears to be right, just a typo when changing to cron I think
Comment 22 Marcel de Rooy 2020-03-20 10:38:49 UTC
Coming back here now
Comment 23 Jonathan Druart 2020-03-20 11:11:16 UTC
(In reply to Marcel de Rooy from comment #22)
> Coming back here now

Unassigning me as QA contact then. I was waiting for dependent bug to be ready actually.
Comment 24 Marcel de Rooy 2020-03-20 12:35:58 UTC
(In reply to Jonathan Druart from comment #23)
> (In reply to Marcel de Rooy from comment #22)
> > Coming back here now
> 
> Unassigning me as QA contact then. I was waiting for dependent bug to be
> ready actually.

Sorry for that. If you want to, go ahead. But I was wondering if we need to define these two as dependent actually.
Comment 25 Marcel de Rooy 2020-03-20 12:39:59 UTC
Created attachment 101094 [details] [review]
Bug 24754: (QA follow-up) Add timestamp, remove 11th parameter

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2020-03-20 13:28:46 UTC
Having doubts if we should add userenv if there is already a login. If there is no login, the current approach seems fine to me. But if we have a login (by cookie from opac or intranet login), we loose the information which user triggered the action. Currently, such actions are saved into action_logs with the current user.

|      2646 | 2020-03-20 13:05:58 |    0 | HOLDS            | CANCEL |    181 |
=> This patch causes 0
|      2648 | 2020-03-20 13:17:12 |   51 | HOLDS            | CANCEL |    182 | 
=> Without patch, login from intranet
|      2652 | 2020-03-20 13:23:29 |   51 | HOLDS            | CANCEL |    183 | 
=> Without patch, login from opac

Keeping in FQA. Needs feedback

Please add why we need the dependency. Only for the test plan?
Comment 27 Nick Clemens 2020-03-20 13:52:36 UTC
(In reply to Marcel de Rooy from comment #26)
> Having doubts if we should add userenv if there is already a login. If there
> is no login, the current approach seems fine to me. But if we have a login
> (by cookie from opac or intranet login), we loose the information which user
> triggered the action. Currently, such actions are saved into action_logs
> with the current user.

ILSDI does not depend on user authentication, the permissions of the user to edit  items or place holds etc do not match. ILSDI is its own way to interact with KOha - I think in these instances it is more correct to indicate a system user as opposed to sign in user.

The trigger for this patch was actually the Aspen discovery system. The ILSDI calls are routed from a central server which is also passing other calls to Koha. We began to see holds being manipulated by random patrons - because without a session set explicitly for ILSDI any other session from the server was being picked. It was causing logging issues and confusion

> Please add why we need the dependency. Only for the test plan?

Yes, I think there were small differences in logs with those patches, so they are related
Comment 28 Marcel de Rooy 2020-03-23 12:50:27 UTC
And would it be interesting to differentiate between no context (from that server) and with context from a logged-in user?
Comment 29 Tomás Cohen Arazi 2020-03-23 14:59:13 UTC
I agree ILSDI should set its own userenv as it is really not dependent on the current cookie. This is a nasty side effect of our coding practices.
Comment 30 Marcel de Rooy 2020-03-24 13:58:38 UTC
Created attachment 101584 [details] [review]
Bug 24754: Set userenv for ilsdi requests

Currently ILSDI scripts do not have a userenv set. This affects both
action_logs and statistics.

For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded

For statistics we need to specify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there.

To test:
1 - Enable ILSDI in systempreferences
2 - Checkout an item to a patron
3 - Enable Renewal logs
4 - Ensure you will be able to renew the item several times
5 - Open an incognito/private browser
6 - Renew item via ilsdi:
    http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128
7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one
8 - In private browser open a new tab
9 - Sign in to staff client
10 - Return to other tab
11 - Refresh page to repeat renewal
12 - Check action_logs
13 - The second renewal indicates it was renewed via intranet by logged in staff member
14 - Apply patch
15 - restart_all
16 - Ensure staff client still signed in
17 - Repeate renewal again
18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2020-03-24 13:58:42 UTC
Created attachment 101585 [details] [review]
Bug 24754: (QA follow-up) Add timestamp, remove 11th parameter

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2020-03-24 13:59:30 UTC
@RM: There is a dependency ! But note that it is imo not strictly needed to wait for it..
Comment 33 Martin Renvoize 2020-03-25 09:43:40 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 34 Joy Nelson 2020-04-03 22:04:28 UTC
not backported to 19.11. due to dependencies