Bug 35890 - AutoLocation system preference + setting the library IP field - can still login and unexpected results
Summary: AutoLocation system preference + setting the library IP field - can still log...
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 critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 35904
Blocks: 34478 35918 36092
  Show dependency treegraph
 
Reported: 2024-01-24 11:55 UTC by David Nind
Modified: 2024-04-22 12:37 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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 35890: Remove var loggedin (1.35 KB, patch)
2024-01-24 15:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Reject login if IP is not valid (910 bytes, patch)
2024-01-24 15:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Reject login if IP is not valid (688 bytes, patch)
2024-01-25 08:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Reject login if IP is not valid (688 bytes, patch)
2024-01-25 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Add tests for AutoLocation (2.90 KB, patch)
2024-01-25 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Remove var loggedin (1.43 KB, patch)
2024-01-31 16:51 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35890: Reject login if IP is not valid (769 bytes, patch)
2024-01-31 16:51 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35890: Add tests for AutoLocation (2.98 KB, patch)
2024-01-31 16:51 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35890: Reject login if IP is not valid (753 bytes, patch)
2024-02-14 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Add tests for AutoLocation (2.92 KB, patch)
2024-02-14 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35890: Remove var loggedin (1.47 KB, patch)
2024-02-14 16:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35890: Reject login if IP is not valid (809 bytes, patch)
2024-02-14 16:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35890: Add tests for AutoLocation (2.98 KB, patch)
2024-02-14 16:17 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 David Nind 2024-01-24 11:55:28 UTC
The AutoLocation system preference says "Require staff to log in from a computer in the IP address range specified by their library (if any): No/Yes Link to library administration" Options are No (the default) and Yes.

If you add an IP address to the IP field for a library (Administration > Libraries > [Library name] > IP (hint is 'Can be entered as a single IP, or a subnet such as 192.168.1.*')) and then set AutoLocation to 'Yes', it doesn't seem to work as expected. 

Expected behavour: you should only be able to login from that IP address or IP address range. If you aren't from that IP address or range, you shouldn't be able to login at all.

Unexpected results include:
1. You can login, but if you try to browse to a page (for example Koha Administration) you are logged out.
2. Login again, do a catalog search - you are logged out.
3. Login again, and you get the search results from the previous step.

To replicate (using KTD):
1. Go to Koha Administration > Libraries
2. Edit Centerville (CPL) and add an IP address to the IP field, for example: 169.58.2.1
3. Enable the AutoLocation system preference (set to Yes).
4. Log out.
5. Login again, then attempt to browse to the staff interface home page - you are logged out.
6. Login again and do a catalog search - you are logged out.
7. Login again - the search results from your search before are shown.
8. Attempt to browse to the staff interface home page - you are logged out.
9. Login again and go to Koha Administration - you are logged out.
10. Login again and try to access the Libraries page - you are logged out.
11. Login again and you are on the libraries page - a pop-up window says:
"Something went wrong when loading the table.
401: Unauthorized."

Other information:
1. Troubleshooting issue on IRC (it is quite long): https://irc.koha-community.org/koha/2024-01-24#i_2532968
2. Bug 26176 - AutoLocation is badly named (NEW)
3. Bug 1442 - AutoLocation does not actually work as advertised (CLOSED FIXED)
Comment 1 Jonathan Druart 2024-01-24 15:31:41 UTC
Created attachment 161347 [details] [review]
Bug 35890: Remove var loggedin

It is never used and add confusion
Comment 2 Jonathan Druart 2024-01-24 15:31:44 UTC
Created attachment 161348 [details] [review]
Bug 35890: Reject login if IP is not valid
Comment 3 Jonathan Druart 2024-01-24 15:32:00 UTC
This is the fix, now we need Selenium tests.
Comment 4 David Nind 2024-01-24 22:30:48 UTC
Thanks Jonathan!

I can confirm that this fixes the issue, as far as my basic testing can tell.

One thing I noted (see steps 13 and 14):
- I had mixed results when changing the library in the login form dropdown list and trying to log in. 
- Expected result: You should be able to log in to a library without IP restrictions, regardless of the patron's home library.
- Actual results: sometimes it worked, sometimes it didn't, got session errors rather than the AutoLocation error.
- I ended up confusing myself, and haven't tried again!


Testing notes (using KTD):

1. Replicate the issue: see the description for the bug.

2. One issue I didn't cover in the bug description was that if you manually disable the AutoLocation system preference in the database (SQL query: update systempreferences set value='0' where variable='AutoLocation';), the issue still occurs and seems to ignore the system preference value. To be able to log in again, you needed to remove the values from the branchip field for the libraries in the branches table (SQL query: update branches set branchip=NULL where branchcode='CPL'). With the patches, disabling AutoLocation works as expected - you can log in again.

3. Apply the patch, flush_memcached, restart_all, start with a fresh browser with cache cleared.

4. Using the koha user, attempt to log in - there are two error messages displayed 
on the login screen: 

    Error: Session timed out.
    Please log in again

    Error: Autolocation is switched on and you are logging in with an IP address that doesn't match your library. 

5. Manually disable the AutoLocation system preference in the database:
   - koha-mysql kohadev
   - SQL query: update systempreferences set value='0' where variable='AutoLocation';
   - flush_memcached and restart_all (I think this is required)

6. You should be able to log in again.

7. Edit patron Henry Acevedo so that they have a user name and password that you can log in with, and set their permissions so that they are a super librarian (Access to all librarian functions).

9. Re-enable the AutoLocation system preference.

10. Log out of the staff interface.

11. I ran flush_memcached and restart_all again as well, and used a browser with the cache cleared.

12. Try to log in as the koha user - you should not be able to.

13. Try to log in as the koha user again BUT change the library - you are not able to log in. I'm not sure whether this needs fixing or not. If the library you are trying to log in has no IP restrictions, then you should be able to log in. Maybe it is using the patrons home library?

14. I tried step 13 with Henry Acevedo and changing the library. I had mixed results, sometimes I could and sometimes I couldn't - mostly session error, rather the AutoLocation one. Maybe I need to clear the browser cache.
Comment 5 Jonathan Druart 2024-01-25 08:08:08 UTC
Created attachment 161375 [details] [review]
Bug 35890: Reject login if IP is not valid
Comment 6 Jonathan Druart 2024-01-25 09:43:56 UTC
Created attachment 161385 [details] [review]
Bug 35890: Reject login if IP is not valid
Comment 7 Jonathan Druart 2024-01-25 09:43:59 UTC
Created attachment 161386 [details] [review]
Bug 35890: Add tests for AutoLocation
Comment 8 Jonathan Druart 2024-01-25 09:44:25 UTC
(In reply to David Nind from comment #4)
> Thanks Jonathan!
> 
> I can confirm that this fixes the issue, as far as my basic testing can tell.
> 
> One thing I noted (see steps 13 and 14):
> - I had mixed results when changing the library in the login form dropdown
> list and trying to log in. 
> - Expected result: You should be able to log in to a library without IP
> restrictions, regardless of the patron's home library.
> - Actual results: sometimes it worked, sometimes it didn't, got session
> errors rather than the AutoLocation error.
> - I ended up confusing myself, and haven't tried again!

Yes sorry, the patch was buggy!
Comment 9 Matt Blenkinsop 2024-01-25 15:26:44 UTC
Just tested this and I still get a behaviour similar to David where it seems to be the patron's home library that governs whether or not they can sign in. I assigned IPs to both the Koha user and Henry Acevedo's home libraries and could not log in with either of them when Autolocation was turned on. With no IP assigned to Henry's home library (Midway) I could log into all branches except Centerville.

Otherwise everything works as described but is this the desired behaviour? The unit tests also pass.
Comment 10 Jonathan Druart 2024-01-26 07:33:00 UTC
Sorry, not sure I understand.

What I have tried:

Login with koha/koha branchcode=CPL

Tests with CPL, FFL and FPL:

branchcode: CPL
branchname: Centerville
  branchip: 172.18.0.1

branchcode: FFL
branchname: Fairfield
  branchip: 172.18.0.1

branchcode: FPL
branchname: Fairview
  branchip: 172.18.0.4


My IP is 172.18.0.1 so I should be able to connect to CPL and FFL, not FPL.

I have added a warn 

diff --git a/C4/Auth.pm b/C4/Auth.pm
index 1d31c5b7466..35c0119ea61 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -1196,6 +1196,7 @@ sub checkauth {
                     if ( $type ne 'opac' and C4::Context->preference('AutoLocation') ) {
 
                         # we have to check they are coming from the right ip range
+                        warn $branchcode;
                         my $domain = $branches->{$branchcode}->{'branchip'};
                         $domain =~ s|\.\*||g;
                         if ( $ip !~ /^$domain/ ) {



1. Login form, koha/koha, "my library"
=> OK, "CPL" in log

2. Logout, login form, "Centerville"
=> OK, "CPL" in log

3. Logout, login form, "Fairfield"
=> OK, "FFL" in log

4. Logout, login form, "Fairview"
=> KO, "FPL" in log, seeing the "AutoLocation" error on the screen.
Comment 11 Jonathan Druart 2024-01-26 07:36:41 UTC
I found another bug however... See bug 35918.
Comment 12 Matt Blenkinsop 2024-01-29 09:47:17 UTC
The issue I saw is replicated as follows:

1) Log in as Koha user
2) Add an IP that is different to yours to Centerville (Home library) and Fairfield
3) Turn Autolocation on
4) Log out
5) Clear browsing data, flush_memcached, restart_all
6) Log into Centerville - blocked correctly for incorrect IP
7) Clear browsing data, flush_memcached, restart_al
8) Log into Fairfield - blocked correctly for incorrect IP
9) Clear browsing data, flush_memcached, restart_al
10) Log into any other library - there is no IP restriction so login should be possible but login will fail with the Autolocation warning

It feels like you should be able to log into other libraries that have no IP restriction
Comment 13 Matt Blenkinsop 2024-01-29 09:48:10 UTC
Testing again with 35918 applied
Comment 14 Matt Blenkinsop 2024-01-29 09:51:16 UTC
Still seems to be happening with 35918 applied - let me know if I'm missing any testing steps
Comment 15 Jonathan Druart 2024-01-29 16:51:48 UTC
(In reply to Matt Blenkinsop from comment #12)
> The issue I saw is replicated as follows:
> 
> 1) Log in as Koha user
> 2) Add an IP that is different to yours to Centerville (Home library) and
> Fairfield
> 3) Turn Autolocation on
> 4) Log out
> 5) Clear browsing data, flush_memcached, restart_all
> 6) Log into Centerville - blocked correctly for incorrect IP
> 7) Clear browsing data, flush_memcached, restart_al
> 8) Log into Fairfield - blocked correctly for incorrect IP
> 9) Clear browsing data, flush_memcached, restart_al
> 10) Log into any other library - there is no IP restriction so login should
> be possible but login will fail with the Autolocation warning
> 
> It feels like you should be able to log into other libraries that have no IP
> restriction

Congrats Matt, you found yet another C4::Auth bug!
This is in master as well, try this (no need to play with AutoLocation):
* login with foo/bar, select CPL => FAIL
* login with koha/koha, select another branch => OK but CPL is picked!

You earned the privilege to open a new bug :D
Comment 16 Jonathan Druart 2024-01-30 08:02:04 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Matt Blenkinsop from comment #12)
> > The issue I saw is replicated as follows:
> > 
> > 1) Log in as Koha user
> > 2) Add an IP that is different to yours to Centerville (Home library) and
> > Fairfield
> > 3) Turn Autolocation on
> > 4) Log out
> > 5) Clear browsing data, flush_memcached, restart_all
> > 6) Log into Centerville - blocked correctly for incorrect IP
> > 7) Clear browsing data, flush_memcached, restart_al
> > 8) Log into Fairfield - blocked correctly for incorrect IP
> > 9) Clear browsing data, flush_memcached, restart_al
> > 10) Log into any other library - there is no IP restriction so login should
> > be possible but login will fail with the Autolocation warning
> > 
> > It feels like you should be able to log into other libraries that have no IP
> > restriction
> 
> Congrats Matt, you found yet another C4::Auth bug!
> This is in master as well, try this (no need to play with AutoLocation):
> * login with foo/bar, select CPL => FAIL
> * login with koha/koha, select another branch => OK but CPL is picked!
> 
> You earned the privilege to open a new bug :D

I've opened bug 35935.
Comment 17 Matt Blenkinsop 2024-01-31 16:47:22 UTC
You were too quick with the new bug! I'll sign this off on the basis that the issue is now fixed in bug 35935
Comment 18 Matt Blenkinsop 2024-01-31 16:51:32 UTC
Created attachment 161701 [details] [review]
Bug 35890: Remove var loggedin

It is never used and add confusion

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 19 Matt Blenkinsop 2024-01-31 16:51:35 UTC
Created attachment 161702 [details] [review]
Bug 35890: Reject login if IP is not valid

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 20 Matt Blenkinsop 2024-01-31 16:51:38 UTC
Created attachment 161703 [details] [review]
Bug 35890: Add tests for AutoLocation

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 21 Jonathan Druart 2024-02-14 09:18:52 UTC
Created attachment 162136 [details] [review]
Bug 35890: Reject login if IP is not valid

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 22 Jonathan Druart 2024-02-14 09:18:56 UTC
Created attachment 162137 [details] [review]
Bug 35890: Add tests for AutoLocation

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 23 Jonathan Druart 2024-02-14 09:19:08 UTC
Patches rebased.
Comment 24 Kyle M Hall 2024-02-14 16:17:42 UTC
Created attachment 162152 [details] [review]
Bug 35890: Remove var loggedin

It is never used and add confusion

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2024-02-14 16:17:49 UTC
Created attachment 162153 [details] [review]
Bug 35890: Reject login if IP is not valid

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2024-02-14 16:17:51 UTC
Created attachment 162154 [details] [review]
Bug 35890: Add tests for AutoLocation

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Victor Grousset/tuxayo 2024-02-15 03:29:17 UTC
Dependency bug 35904 not passed QA yet so this can't be passed QA, right?
(unless a big change in the dependency, the existing testing and review will still be valid)
Comment 28 Martin Renvoize 2024-02-15 09:48:53 UTC
Pushed to 24.05.00
Comment 29 Marcel de Rooy 2024-02-29 15:20:45 UTC
What about the route with an OPAC cookie? If they share the same cookie (same domain, other port), then there is no problem for switching to staff client after OPAC login.

Note that there are bugs already for not allowing opac cookies on staff.

In this case AutoLocation + branchip just gives a false sense of security.