Bug 35918 - Incorrect library used when AutoLocation configured using the same IP
Summary: Incorrect library used when AutoLocation configured using the same IP
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 35890
Blocks:
  Show dependency treegraph
 
Reported: 2024-01-26 07:36 UTC by Jonathan Druart
Modified: 2024-03-04 10:38 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small 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,22.11.15,22.05.19


Attachments
Bug 35918: Add test (1.22 KB, patch)
2024-01-26 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35918: Fix auto library connect (AutoLocation) (2.68 KB, patch)
2024-01-26 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35918: Add test (1.27 KB, patch)
2024-01-26 14:37 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 35918: Fix auto library connect (AutoLocation) (3.26 KB, patch)
2024-01-26 14:37 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 35918: Add test (1.32 KB, patch)
2024-02-14 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35918: Fix auto library connect (AutoLocation) (3.32 KB, patch)
2024-02-14 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-01-26 07:36:30 UTC
If you have AutoLocation turned on and you are using the same IP for different libraries, the logged in library will be set randomly.
Comment 1 Jonathan Druart 2024-01-26 07:37:17 UTC
No idea what this code is supposed to do...

1215                     foreach my $br ( keys %$branches ) {
1216 
1217                         #     now we work with the treatment of ip
1218                         my $domain = $branches->{$br}->{'branchip'};
1219                         if ( $domain && $ip =~ /^$domain/ ) {
1220                             $branchcode = $branches->{$br}->{'branchcode'};
1221 
1222                             # new op dev : add the branchname to the cookie
1223                             $branchname    = $branches->{$br}->{'branchname'};
1224                         }
1225                     }
Comment 2 Jonathan Druart 2024-01-26 08:01:18 UTC
Created attachment 161481 [details] [review]
Bug 35918: Add test
Comment 3 Jonathan Druart 2024-01-26 08:01:20 UTC
Created attachment 161482 [details] [review]
Bug 35918: Fix auto library connect (AutoLocation)

This code is a bit weird, its purpose it to auto select the library depending on the IP.
A problem appears if the same IP is used, then the user's choice will
might be overwritten randomly by another library.

To recreate the problem:
Turn on AutoLocation
Use koha/koha @CPL for test
And the following config:
*************************** 1. row ***************************
branchcode: CPL
branchname: Centerville
  branchip: 172.18.0.1
*************************** 2. row ***************************
branchcode: FFL
branchname: Fairfield
  branchip: 172.18.0.1
*************************** 3. row ***************************
branchcode: FPL
branchname: Fairview
  branchip: 172.18.0.4

Connect and select CPL. Randomly FFL will be picked instead.
Comment 4 Jonathan Druart 2024-01-26 08:02:23 UTC
Moving to security because bug 35890 is.
Comment 5 Magnus Enger 2024-01-26 14:26:04 UTC
Tested this on top of 35890 and 35904 because git bz said they were required dependencies. 

Figured out the IP Koha was seeing me as coming from in /var/log/koha/kohadev/plack.log. 

Added that IP to the branchip for Centerville, Fairfield and Fairview. Set AutoLocation = Yes. 

After this I could recreate the problem: If i left the "Library" field in the login screen at "My Library" I got logged into a random library selected from the three i had set branchip for. Applying the patches fixed this, as expected. 

But running the changed tests gave an error, that does not seem to happen on the master branch: 

$ prove t/db_dependent/Auth.t
t/db_dependent/Auth.t .. 1/19 WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better.
Pass -nodeprecate=>1 to inhibit this message.
 at /kohadevbox/koha/Koha/Patron.pm line 2747.

        #   Failed test 'Second auth required'
        #   at t/db_dependent/Auth.t line 288.
        #          got: undef
        #     expected: '1'

        #   Failed test 'Second auth still required after wrong OTP token'
        #   at t/db_dependent/Auth.t line 296.
        #          got: undef
        #     expected: '1'

        #   Failed test 'Second auth no longer required if OTP token has been verified'
        #   at t/db_dependent/Auth.t line 302.
        #          got: undef
        #     expected: '0'

        #   Failed test 'Setup 2FA required'
        #   at t/db_dependent/Auth.t line 309.
        #          got: undef
        #     expected: '1'
        # Looks like you failed 4 tests of 18.

    #   Failed test 'Two-factor authentication'
    #   at t/db_dependent/Auth.t line 328.
    # Looks like you failed 1 test of 9.
t/db_dependent/Auth.t .. 2/19 
#   Failed test 'checkauth() tests'
#   at t/db_dependent/Auth.t line 369.
t/db_dependent/Auth.t .. 14/19 # Looks like you failed 1 test of 19.
t/db_dependent/Auth.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/19 subtests 

Test Summary Report
-------------------
t/db_dependent/Auth.t (Wstat: 256 Tests: 19 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=19,  4 wallclock secs ( 0.02 usr  0.00 sys +  3.45 cusr  0.38 csys =  3.85 CPU)
Result: FAIL

It looks like these are tests related to 2FA, and the tests related to AutoLocation look ok: 

$ prove -v t/db_dependent/Auth.t
...
# Subtest: AutoLocation
    1..7
    ok 1
    ok 2
    ok 3
    ok 4
    ok 5
    ok 6
    ok 7
ok 19 - AutoLocation

Not sure if the failing tests should result in "Failed QA", or if they should be considered irrelevant for this bug?
Comment 6 Jonathan Druart 2024-01-26 14:30:04 UTC
(In reply to Magnus Enger from comment #5)
> Not sure if the failing tests should result in "Failed QA", or if they
> should be considered irrelevant for this bug?

Turn off AutoLocation.
IIRC the tests fail if on, even before without this patch. So not relevant.
Comment 7 Magnus Enger 2024-01-26 14:37:37 UTC
Created attachment 161541 [details] [review]
Bug 35918: Add test

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 8 Magnus Enger 2024-01-26 14:37:40 UTC
Created attachment 161542 [details] [review]
Bug 35918: Fix auto library connect (AutoLocation)

This code is a bit weird, its purpose it to auto select the library depending on the IP.
A problem appears if the same IP is used, then the user's choice will
might be overwritten randomly by another library.

To recreate the problem:
Turn on AutoLocation
Use koha/koha @CPL for test
And the following config:
*************************** 1. row ***************************
branchcode: CPL
branchname: Centerville
  branchip: 172.18.0.1
*************************** 2. row ***************************
branchcode: FFL
branchname: Fairfield
  branchip: 172.18.0.1
*************************** 3. row ***************************
branchcode: FPL
branchname: Fairview
  branchip: 172.18.0.4

Connect and select CPL. Randomly FFL will be picked instead.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested this on top of 35890 and 35904 because git bz said they were required dependencies.
Figured out the IP Koha was seeing me as coming from in /var/log/koha/kohadev/plack.log.
Added that IP to the branchip for Centerville, Fairfield and Fairview. Set AutoLocation = Yes.
After this I could recreate the problem: If i left the "Library" field in the login screen
at "My Library" I got logged into a random library selected from the three i had set
branchip for. Applying the patches fixed this, as expected.
Tests pass, with AutoLocation off.
Comment 9 Kyle M Hall 2024-02-14 16:34:19 UTC
Created attachment 162155 [details] [review]
Bug 35918: Add test

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2024-02-14 16:34:28 UTC
Created attachment 162156 [details] [review]
Bug 35918: Fix auto library connect (AutoLocation)

This code is a bit weird, its purpose it to auto select the library depending on the IP.
A problem appears if the same IP is used, then the user's choice will
might be overwritten randomly by another library.

To recreate the problem:
Turn on AutoLocation
Use koha/koha @CPL for test
And the following config:
*************************** 1. row ***************************
branchcode: CPL
branchname: Centerville
  branchip: 172.18.0.1
*************************** 2. row ***************************
branchcode: FFL
branchname: Fairfield
  branchip: 172.18.0.1
*************************** 3. row ***************************
branchcode: FPL
branchname: Fairview
  branchip: 172.18.0.4

Connect and select CPL. Randomly FFL will be picked instead.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested this on top of 35890 and 35904 because git bz said they were required dependencies.
Figured out the IP Koha was seeing me as coming from in /var/log/koha/kohadev/plack.log.
Added that IP to the branchip for Centerville, Fairfield and Fairview. Set AutoLocation = Yes.
After this I could recreate the problem: If i left the "Library" field in the login screen
at "My Library" I got logged into a random library selected from the three i had set
branchip for. Applying the patches fixed this, as expected.
Tests pass, with AutoLocation off.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Martin Renvoize 2024-02-15 09:49:13 UTC
Pushed to master for 24.05.00