Bug 36665 - Auto location and IP recognition
Summary: Auto location and IP recognition
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords: regression
Depends on: 35918
Blocks: 36908
  Show dependency treegraph
 
Reported: 2024-04-22 12:35 UTC by olivier.delangle
Modified: 2024-05-23 15:56 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch adds a new system preference "StaffLoginLibraryBasedOnIP" which, when enabled, will set the logged in library to the library with an IP setting matching the current users IP. This preference will be overridden if "AutoLocation" is enabled, as that preference will enforce the user selecting a library that matches their current IP or signing into their home library from the correct IP.
Version(s) released in:
24.05.00


Attachments
Bug 36665: Add option to set the staff user's logged in branch based on their current ip (9.92 KB, patch)
2024-05-17 12:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36665: Add option to set the staff user's logged in branch based on their current ip (9.98 KB, patch)
2024-05-17 13:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36665: Add option to set the staff user's logged in branch based on their current ip (10.04 KB, patch)
2024-05-20 17:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36665: Add option to set the staff user's logged in branch based on their current ip (10.04 KB, patch)
2024-05-21 11:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36665: (follow-up) Wrap code block in type ne 'opac' conditional (4.76 KB, patch)
2024-05-21 11:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation (2.56 KB, patch)
2024-05-22 13:09 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation (2.56 KB, patch)
2024-05-22 13:11 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36665: Add option to set the staff user's logged in branch based on their current ip (10.11 KB, patch)
2024-05-22 13:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36665: (follow-up) Wrap code block in type ne 'opac' conditional (4.84 KB, patch)
2024-05-22 13:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation (2.63 KB, patch)
2024-05-22 13:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36665: (follow-up) Default preference to enabled (1.75 KB, patch)
2024-05-23 15:16 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description olivier.delangle 2024-04-22 12:35:58 UTC
Since the modification of the Autolocation syspref functionality, the previous feature is no longer possible. 

A new syspref should be implemented to restore it, meaning: 

for terminals with IPs declared for the branch, the connection to the professional interface defaults to the IP's branch rather than the member's affiliated branch.
Comment 1 David Cook 2024-04-22 23:39:38 UTC
At a glance, it looks like AutoLocation is working the same. Can you be more specific about the issue you're having?
Comment 2 David Cook 2024-04-22 23:46:43 UTC
Wait... I think I understand you. You mean you weren't using AutoLocation to try to restrict logins?

You were just trying to use AutoLocation to force the branch of the session to the branch of the physical location (as designated by the IP address).

So arguably that desired functionality would be AutoLocation and the restricting by IP address would be renamed as per bug 26176.

--

That's an interesting idea.
Comment 3 olivier.delangle 2024-04-23 06:04:09 UTC
That's right. 

This feature allowed individuals not affiliated with the library to work there without having to switch branch with each session. 
This is particularly useful, of course, in networks composed of multiple libraries. 

It was indeed very useful and practical.
Comment 4 Nick Clemens (kidclamp) 2024-05-17 11:12:57 UTC
I am raising the severity here. Before bug 35918 if a library specified IPs in the branches table a user would be logged in to the correct branch based on their IP when AutoLocation was disabled

Now, this does not happen.

If AutoLocation is enabled, the user can only log in if their current IP matches the branch in their account.

This is a huge behavior change and negatively affects large consortia where staff travel between branches
Comment 5 Nick Clemens (kidclamp) 2024-05-17 12:14:20 UTC
Created attachment 166889 [details] [review]
Bug 36665: Add option to set the staff user's logged in branch based on their current ip

This patch adds a new system preference StaffLoginBranchBasedOnIP which restores the behaviour before bug 35918
of using the current IP to determine the user's logged in branchcode

To test:
 1 - Get your current ip
 2 - Set that IP for a library in the administration section
 3 - Find a user account assigned to a different library that can login to staff side
 4 - Login to staff as that user, select 'My library'
 5 - You are logged in to the user's branch
 6 - Apply patch, restart all
 7 - Log out and back in, selecting 'My library'
 8 - You are logged in to the user's branch
 9 - Enable new system preference StaffLoginBranchBasedOnIP
 9 - Log out and back in, selecting a different branch, noting the new warning below the library selection
10 - You are logged in to the branch with the matching IP
11 - Log out and back in, selecting 'My library'
10 - You are logged in to the branch with the matching IP
11 - Change your logged in branch
12 - Verify the selection sticks and you can perform staff actions in the chosen branch
13 - Change the IP of the library to one that doesn't match yours
14 - Verify you can log out and log back in and that selected branch is respected when your IP doesn't match library IP
Comment 6 Martin Renvoize 2024-05-17 13:12:38 UTC
Created attachment 166904 [details] [review]
Bug 36665: Add option to set the staff user's logged in branch based on their current ip

This patch adds a new system preference StaffLoginBranchBasedOnIP which restores the behaviour before bug 35918
of using the current IP to determine the user's logged in branchcode

To test:
 1 - Get your current ip
 2 - Set that IP for a library in the administration section
 3 - Find a user account assigned to a different library that can login to staff side
 4 - Login to staff as that user, select 'My library'
 5 - You are logged in to the user's branch
 6 - Apply patch, restart all
 7 - Log out and back in, selecting 'My library'
 8 - You are logged in to the user's branch
 9 - Enable new system preference StaffLoginBranchBasedOnIP
 9 - Log out and back in, selecting a different branch, noting the new warning below the library selection
10 - You are logged in to the branch with the matching IP
11 - Log out and back in, selecting 'My library'
10 - You are logged in to the branch with the matching IP
11 - Change your logged in branch
12 - Verify the selection sticks and you can perform staff actions in the chosen branch
13 - Change the IP of the library to one that doesn't match yours
14 - Verify you can log out and log back in and that selected branch is respected when your IP doesn't match library IP

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Jonathan Druart 2024-05-20 11:54:25 UTC
Just quickly looked at the code but are you sure the auth failed condition ($auth_state ne 'failed') shouldn't be included as well for StaffLoginBranchBasedOnIP?
Comment 8 Nick Clemens (kidclamp) 2024-05-20 13:55:26 UTC
(In reply to Jonathan Druart from comment #7)
> Just quickly looked at the code but are you sure the auth failed condition
> ($auth_state ne 'failed') shouldn't be included as well for
> StaffLoginBranchBasedOnIP?

I don't think it matters -  at this point in the code valid credentials were passed ( $return > 0 )

We would prevent the login if permissions are wrong , or for autlocation - for wrong permissions I think it is correct to still set the branch
Comment 9 Nick Clemens (kidclamp) 2024-05-20 17:42:16 UTC
Created attachment 166941 [details] [review]
Bug 36665: Add option to set the staff user's logged in branch based on their current ip

This patch adds a new system preference StaffLoginBranchBasedOnIP which restores the behaviour before bug 35918
of using the current IP to determine the user's logged in branchcode

To test:
 1 - Get your current ip
 2 - Set that IP for a library in the administration section
 3 - Find a user account assigned to a different library that can login to staff side
 4 - Login to staff as that user, select 'My library'
 5 - You are logged in to the user's branch
 6 - Apply patch, restart all
 7 - Log out and back in, selecting 'My library'
 8 - You are logged in to the user's branch
 9 - Enable new system preference StaffLoginBranchBasedOnIP
 9 - Log out and back in, selecting a different branch, noting the new warning below the library selection
10 - You are logged in to the branch with the matching IP
11 - Log out and back in, selecting 'My library'
10 - You are logged in to the branch with the matching IP
11 - Change your logged in branch
12 - Verify the selection sticks and you can perform staff actions in the chosen branch
13 - Change the IP of the library to one that doesn't match yours
14 - Verify you can log out and log back in and that selected branch is respected when your IP doesn't match library IP

Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Jonathan Druart 2024-05-21 07:26:24 UTC
1. At least since bug 35918 we took care of having the selected library at the beginning on the array and so this library will be picked first if matched. I think we should improve the syspref's description to clarify what "first" really means here.
For discussion: shouldn't we list patron's library as well? uniq(selected_library, patron's library, other libraries)

2. I think AutoLocation should be renamed, we are adding more confusion about its purpose with this new syspref.

3. We should have all this code inside a `if ($type ne 'opac')` block. It seems that we don't use this branch or branchname TT var in the OPAC but it would be better to not pass it at all it seems (existed prior to this patch).

I understand that you consider this critical and that you even bypass proper QA but maybe those remarks can be addressed on a separate bug, or at least we need plans for the next steps?
Comment 11 Katrin Fischer 2024-05-21 11:09:14 UTC
I have to admit, I am not sure I understand all the comments. So I will just ask:

Nick and Joubu - is this ready for push?
Comment 12 Nick Clemens (kidclamp) 2024-05-21 11:17:00 UTC
(In reply to Jonathan Druart from comment #10)
> 1. At least since bug 35918 we took care of having the selected library at
> the beginning on the array and so this library will be picked first if
> matched. I think we should improve the syspref's description to clarify what
> "first" really means here.
> For discussion: shouldn't we list patron's library as well?
> uniq(selected_library, patron's library, other libraries)

Looking into the code, it seems 35918 uses the patron's library OR the selected library first, but it looks like the rest of the array is still taken unordered from the hash? I will file a follow-up bug for this

> 
> 2. I think AutoLocation should be renamed, we are adding more confusion
> about its purpose with this new syspref.

We have bug 26176 open for this, I will look into supplying a patch


> 3. We should have all this code inside a `if ($type ne 'opac')` block. It
> seems that we don't use this branch or branchname TT var in the OPAC but it
> would be better to not pass it at all it seems (existed prior to this patch).

That makes sense, I went about restoring the previous functionality as closely as possible, but can provide a follow-up here to wrap the opac test around all the code
 
> I understand that you consider this critical and that you even bypass proper
> QA but maybe those remarks can be addressed on a separate bug, or at least
> we need plans for the next steps?

I am not sure what steps here bypassed proper QA? A QA team member signed off the bug - we then got a sign off from a community member (we attached the sign off as they had issues using the sandboxes) and upgraded, with their permission, the QA team members SO to QA. That discussion could have been included on the bug, but I thought it was straightforward enough to simply reorder the sign offs. I will file to follow-ups, but if you think this needs more attention feel free to change the status.
Comment 13 Nick Clemens (kidclamp) 2024-05-21 11:21:37 UTC
Created attachment 166963 [details] [review]
Bug 36665: Add option to set the staff user's logged in branch based on their current ip

This patch adds a new system preference StaffLoginBranchBasedOnIP which restores the behaviour before bug 35918
of using the current IP to determine the user's logged in branchcode

To test:
 1 - Get your current ip
 2 - Set that IP for a library in the administration section
 3 - Find a user account assigned to a different library that can login to staff side
 4 - Login to staff as that user, select 'My library'
 5 - You are logged in to the user's branch
 6 - Apply patch, restart all
 7 - Log out and back in, selecting 'My library'
 8 - You are logged in to the user's branch
 9 - Enable new system preference StaffLoginBranchBasedOnIP
 9 - Log out and back in, selecting a different branch, noting the new warning below the library selection
10 - You are logged in to the branch with the matching IP
11 - Log out and back in, selecting 'My library'
10 - You are logged in to the branch with the matching IP
11 - Change your logged in branch
12 - Verify the selection sticks and you can perform staff actions in the chosen branch
13 - Change the IP of the library to one that doesn't match yours
14 - Verify you can log out and log back in and that selected branch is respected when your IP doesn't match library IP

Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Nick Clemens (kidclamp) 2024-05-21 11:21:39 UTC
Created attachment 166964 [details] [review]
Bug 36665: (follow-up) Wrap code block in type ne 'opac' conditional
Comment 15 Katrin Fischer 2024-05-21 11:34:36 UTC
This adds strings as it adds a new system preference - I'd still make an exception, but since it's a bit of a "risky area" and we don't want to break something else - could we get another sign-off? (must not be from QA team, but testing the different pref combinations would be great)
Comment 16 Jonathan Druart 2024-05-21 13:31:15 UTC
(In reply to Nick Clemens (kidclamp) from comment #12)
> (In reply to Jonathan Druart from comment #10)
> > I understand that you consider this critical and that you even bypass proper
> > QA but maybe those remarks can be addressed on a separate bug, or at least
> > we need plans for the next steps?
> 
> I am not sure what steps here bypassed proper QA? A QA team member signed
> off the bug - we then got a sign off from a community member (we attached
> the sign off as they had issues using the sandboxes) and upgraded, with
> their permission, the QA team members SO to QA. That discussion could have
> been included on the bug, but I thought it was straightforward enough to
> simply reorder the sign offs. I will file to follow-ups, but if you think
> this needs more attention feel free to change the status.

Personally if I add my signoff on a bug I don't necessarily do a proper code review. If I do signoff bug, don't swap it for a QA stamp :)

Thanks for the follow-up bugs!
Comment 17 Jonathan Druart 2024-05-22 06:51:45 UTC
What's now the expected behaviour for AutoLocation? In my understanding AutoLocation is now only used for IP restriction.

So I think there is a bug.

* AutoLocation = yes,  StaffLoginBranchBasedOnIP = no
* set CPL's IPto your IP and use koha/CPL to login but select another library
=> CPL is used (expected: the selected library)
Comment 18 Nick Clemens (kidclamp) 2024-05-22 11:12:23 UTC
(In reply to Jonathan Druart from comment #17)
> What's now the expected behaviour for AutoLocation? In my understanding
> AutoLocation is now only used for IP restriction.
> 
> So I think there is a bug.
> 
> * AutoLocation = yes,  StaffLoginBranchBasedOnIP = no
> * set CPL's IPto your IP and use koha/CPL to login but select another library
> => CPL is used (expected: the selected library)

Does the user have 'loggedinlibrary' permission? Without that the chosen branch is not respected
Comment 19 Nick Clemens (kidclamp) 2024-05-22 11:24:07 UTC
(In reply to Jonathan Druart from comment #17)
> What's now the expected behaviour for AutoLocation? In my understanding
> AutoLocation is now only used for IP restriction.

If AutoLocation is enabled:
  If the user has loggedinlibrary permission:
      If the chosen branch has an ip:
          the user will be denied login unless IP matches
          they will be logged in to chosen library if IP matches
      If the chosen branch has no ip:
          the user will be logged in at chosen branch
      If no library is chosen:
          If the user's branch has an ip:
            the user will be denied login unless IP matches
            they will be logged in to homebranch if matches
          If the user's branch has no IP:
            the user will be signed in to their homebranch
  If the user does not have loggedinlibrary permission:
    Their chosen branch will be ignored - i.e. choosing or not, homebranch will be used
    If their homebranch has an IP:
        they will be denied login unless IP matches
        they will be logged in to their homebranch if IP matches
    If their homebranch has no IP:
        they will be logged in to their homebranch
Comment 20 Jonathan Druart 2024-05-22 11:32:35 UTC
(In reply to Nick Clemens (kidclamp) from comment #19)
> (In reply to Jonathan Druart from comment #17)
> > What's now the expected behaviour for AutoLocation? In my understanding
> > AutoLocation is now only used for IP restriction.

I am using koha/superlibrarian/CPL
CPL has an IP, IPT has not.
I select IPT on the login screen.

> If AutoLocation is enabled: # Yes, turned ON
>   If the user has loggedinlibrary permission: # Yes, koha is superlibrarian
>       If the chosen branch has no ip: # IPT has no IP
>           the user will be logged in at chosen branch

=> KO I am logged in at CPL
Comment 21 Katrin Fischer 2024-05-22 11:40:13 UTC
We are running out of time and it feels like there are still some question marks. Could we aim for this and the follow-ups to make it into 24.05.01 maybe? (if RMaint agrees?)
Comment 22 Katrin Fischer 2024-05-22 11:41:21 UTC
Set to Failed QA, to keep overview of my queue. Waiting for Nick.
Comment 23 Nick Clemens (kidclamp) 2024-05-22 12:13:44 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to Nick Clemens (kidclamp) from comment #19)
> > (In reply to Jonathan Druart from comment #17)
> > > What's now the expected behaviour for AutoLocation? In my understanding
> > > AutoLocation is now only used for IP restriction.
> 
> I am using koha/superlibrarian/CPL
> CPL has an IP, IPT has not.
> I select IPT on the login screen.
> 
> > If AutoLocation is enabled: # Yes, turned ON
> >   If the user has loggedinlibrary permission: # Yes, koha is superlibrarian
> >       If the chosen branch has no ip: # IPT has no IP
> >           the user will be logged in at chosen branch
> 
> => KO I am logged in at CPL

I had that wrong:
You aren't blocked from signing in because IPT has no IP, but when AutoLocation is checked then you are assigned to the matching branch - this is true before and after my patch - i.e. I am not changing behaviour.

I think we should expand this case into tests on bug 36908
Comment 24 Jonathan Druart 2024-05-22 12:23:07 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2024-05-22 12:47:33 UTC
I am not blocking this.
Comment 26 Nick Clemens (kidclamp) 2024-05-22 13:09:05 UTC
Created attachment 167035 [details] [review]
Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation
Comment 27 Nick Clemens (kidclamp) 2024-05-22 13:11:22 UTC
Created attachment 167036 [details] [review]
Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation
Comment 28 Jonathan Druart 2024-05-22 13:18:22 UTC
Created attachment 167037 [details] [review]
Bug 36665: Add option to set the staff user's logged in branch based on their current ip

This patch adds a new system preference StaffLoginBranchBasedOnIP which restores the behaviour before bug 35918
of using the current IP to determine the user's logged in branchcode

To test:
 1 - Get your current ip
 2 - Set that IP for a library in the administration section
 3 - Find a user account assigned to a different library that can login to staff side
 4 - Login to staff as that user, select 'My library'
 5 - You are logged in to the user's branch
 6 - Apply patch, restart all
 7 - Log out and back in, selecting 'My library'
 8 - You are logged in to the user's branch
 9 - Enable new system preference StaffLoginBranchBasedOnIP
 9 - Log out and back in, selecting a different branch, noting the new warning below the library selection
10 - You are logged in to the branch with the matching IP
11 - Log out and back in, selecting 'My library'
10 - You are logged in to the branch with the matching IP
11 - Change your logged in branch
12 - Verify the selection sticks and you can perform staff actions in the chosen branch
13 - Change the IP of the library to one that doesn't match yours
14 - Verify you can log out and log back in and that selected branch is respected when your IP doesn't match library IP

Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2024-05-22 13:18:25 UTC
Created attachment 167038 [details] [review]
Bug 36665: (follow-up) Wrap code block in type ne 'opac' conditional

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2024-05-22 13:18:28 UTC
Created attachment 167039 [details] [review]
Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Katrin Fischer 2024-05-22 14:20:03 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 32 Laura O'Neil 2024-05-22 18:38:26 UTC
Can this be backported to 23.11?
Comment 33 Nick Clemens (kidclamp) 2024-05-23 15:16:19 UTC
Created attachment 167117 [details] [review]
Bug 36665: (follow-up) Default preference to enabled

While this is a new preference, this was the standard behavior for a long time. I think we should
default to enabled to restore previous behavior.
Comment 34 Nick Clemens (kidclamp) 2024-05-23 15:16:58 UTC
Moving to PQA for attention on follow-up
Comment 35 Katrin Fischer 2024-05-23 15:56:58 UTC
Pushed for 24.05!

Well done everyone, thank you!