Bug 35518 - Call to C4::Context->userenv happens before it's gets populated breaks code logic in circulation
Summary: Call to C4::Context->userenv happens before it's gets populated breaks code l...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 17798
Blocks: 36139
  Show dependency treegraph
 
Reported: 2023-12-07 21:04 UTC by Slava Shishkin
Modified: 2024-02-26 22:23 UTC (History)
14 users (show)

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


Attachments
Bug 35518: Check authentication and set userenv before fetching userenv variables (5.17 KB, patch)
2024-01-26 14:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35518: Check authentication and set userenv before fetching userenv variables (5.22 KB, patch)
2024-01-31 23:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 35518: Check authentication and set userenv before fetching userenv variables (5.28 KB, patch)
2024-02-08 15:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35518: Tidy the moved blocks (1.95 KB, patch)
2024-02-08 15:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35518: follow up - clear the barcode list in AutoSwitchPatron block (922 bytes, patch)
2024-02-15 17:35 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 35518: Follow-up for AutoSwitchPatron - clear $patron variable too (996 bytes, patch)
2024-02-15 18:24 UTC, Michael Hafen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Slava Shishkin 2023-12-07 21:04:01 UTC
We getting results from `C4::Context->userenv` on line 72:

    my $userenv = C4::Context->userenv;

but `C4::Context->userenv` gets populated only far below in `get_template_and_user` call, line 142,
this means that everything that we getting further from `$userenv` is empty.
And more, because autovivification occurs for the `$userenv` at line 73:

    my $branch = $userenv->{'branch'} // ''; 

that does not cause any visible errors, but it causes malfunctions at places where $branch, $desk_id is used, and also where $userenv is accessed, at line 153:

    if (!C4::Auth::haspermission( $userenv->{id} , { circulate => 'force_checkout' } ) ). 


To check that we have undef and it gets autovivificated, let's add some dumps into code when we open any borrower, e.g. /cgi-bin/koha/circ/circulation.pl?borrowernumber=19,

Let's dump $userenv before and after `$branch = $userenv->{'branch'} // '';` line:

    72 my $userenv = C4::Context->userenv;
    73 use Data::Dumper;
    74 warn __LINE__, ' ', Dumper($userenv);
    75 my $branch  = $userenv->{'branch'} // '';
    76 warn __LINE__, ' ', Dumper($userenv);

this gives in server logs:

    74 $VAR1 = undef;
    76 $VAR1 = {};


and also to check when `C4::Context->userenv` gets its real values, let's dump before and after the `get_template_and_user()` call at line 142 (line 145 if to continue above dumps code):

    145 warn __LINE__, ' ', Dumper([C4::Context->userenv]);
    146 my ( $template, $loggedinuser, $cookie ) = get_template_and_user (
        ...
    153 );
    154 warn __LINE__, ' ', Dumper([C4::Context->userenv]);

this gives in server logs:

    145 $VAR1 = [];
    154 $VAR1 = [
              {
                ...
                'id' => 'admin',
                ...
              }
            ];

what depicts the whole bug.
Comment 1 Nick Clemens 2024-01-26 14:15:04 UTC
Created attachment 161539 [details] [review]
Bug 35518: Check authentication and set userenv before fetching userenv variables

Currently we get the userenv before we have set it correctly for the session

To test:
 1 - Sign in as a user with fast cataloging permission
 2 - Bring up a patron, type gibberish into barcode field to get a fast cataloging link
 3 - Check the link, it should have your current signed in barcode
 4 - Sign in to a different browser with a different user and at a different branch
 5 - Bring up a aptron in circulation and type gibberish into barcode field to get a fast cataloging link
 6 - It may have your branch, but it may also have the other user's branch from the other window
 7 - Keep entering gibberish to get a link until one user has the correct branch
 8 - Then switch to the other browser, and keep entering gibberish, watch the branchcode change
 9 - Apply patch, restart all
10 - Test switching between browsers. generating fast cataloging links
11 - Users should now consistently have the correct branch
Comment 2 David Nind 2024-01-31 23:07:16 UTC
Created attachment 161707 [details] [review]
Bug 35518: Check authentication and set userenv before fetching userenv variables

Currently we get the userenv before we have set it correctly for the session

To test:
 1 - Sign in as a user with fast cataloging permission
 2 - Bring up a patron, type gibberish into barcode field to get a fast cataloging link
 3 - Check the link, it should have your current signed in barcode
 4 - Sign in to a different browser with a different user and at a different branch
 5 - Bring up a aptron in circulation and type gibberish into barcode field to get a fast cataloging link
 6 - It may have your branch, but it may also have the other user's branch from the other window
 7 - Keep entering gibberish to get a link until one user has the correct branch
 8 - Then switch to the other browser, and keep entering gibberish, watch the branchcode change
 9 - Apply patch, restart all
10 - Test switching between browsers. generating fast cataloging links
11 - Users should now consistently have the correct branch

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Martin Renvoize 2024-02-08 15:57:22 UTC
Created attachment 161929 [details] [review]
Bug 35518: Check authentication and set userenv before fetching userenv variables

Currently we get the userenv before we have set it correctly for the session

To test:
 1 - Sign in as a user with fast cataloging permission
 2 - Bring up a patron, type gibberish into barcode field to get a fast cataloging link
 3 - Check the link, it should have your current signed in barcode
 4 - Sign in to a different browser with a different user and at a different branch
 5 - Bring up a aptron in circulation and type gibberish into barcode field to get a fast cataloging link
 6 - It may have your branch, but it may also have the other user's branch from the other window
 7 - Keep entering gibberish to get a link until one user has the correct branch
 8 - Then switch to the other browser, and keep entering gibberish, watch the branchcode change
 9 - Apply patch, restart all
10 - Test switching between browsers. generating fast cataloging links
11 - Users should now consistently have the correct branch

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2024-02-08 15:57:24 UTC
Created attachment 161930 [details] [review]
Bug 35518: Tidy the moved blocks

This patch just tidies the moved blocks to get us past the QA script
check.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2024-02-08 15:57:49 UTC
No regressions found, QA script happy.

Passing QA
Comment 6 Andrii Nugged 2024-02-12 22:02:18 UTC
this comment is not related to the code/solution, but to the problem as before the ticket: we recently found that before this solution Koha brings the wrong returning branches when the item is returned on circ/circulation.pl page

(moreover, it looks like (unsure) that it reads branch data of another user from the Plack session because the current wasn't populated properly)

so this bug should have "increased" priority, IMO.
Comment 7 Martin Renvoize 2024-02-13 07:02:00 UTC
It's already PQA, we're in a security freeze right now. It'll be pushed as soon as that freeze is unlocked.
Comment 8 Martin Renvoize 2024-02-14 13:31:20 UTC
Pushed to master for 24.05.00, thanks for the hard work everyone.
Comment 9 Fridolin Somers 2024-02-15 09:50:23 UTC
Pushed to 23.11.x for 23.11.03
Comment 10 Michael Hafen 2024-02-15 17:32:31 UTC
I found a bug with this patch.  Because the AutoSwitch check has been moved to after the @$barcodes variable is filled, if I scan a patron barcode into the item barcode field (as my librarians do all the time), the @$barcodes variable gets filled, but not emptied, and the 'Barcode not found' error surfaces.  I'll be submitting a (trivial) patch to correct that in a moment.
Comment 11 Michael Hafen 2024-02-15 17:35:40 UTC
Created attachment 162216 [details] [review]
Bug 35518: follow up - clear the barcode list in  AutoSwitchPatron block

The AutoSwitchPatron block got moved, and now the @$barcodes variable gets
filled and not cleared.  Leading to a 'Barcode not found' error when the
patron is auto switched.
Comment 12 Michael Hafen 2024-02-15 18:24:10 UTC
Created attachment 162218 [details] [review]
Bug 35518: Follow-up for AutoSwitchPatron - clear $patron variable too

Another follow up:

The AutoSwitchPatron clears the $borrowernumber variable to switch patrons.
With the AuthSwitchPatron block moved, the $patron variable still gets set,
and the patron doesn't get switched.  The clears the $patron variable too.
Comment 13 Fridolin Somers 2024-02-19 10:43:05 UTC
Hi Michael
Thanks for that.

You should create a new bug report with those patches since the previous ones have already been pushed.
And beware of commit messages for follow-ups :
https://wiki.koha-community.org/wiki/Commit_messages#Follow-ups
Comment 14 Eric 2024-02-21 21:00:53 UTC
Would it be possible for this to be backported to 23.05?
Comment 15 Michael Adamyk 2024-02-26 21:06:50 UTC
+1 to Eric's question. This is causing significant confusion in several different contexts in our consortium.
Comment 16 Lucas Gass 2024-02-26 22:23:31 UTC
Backported to 23.05.x for upcoming 23.05.09