Description
olivier.delangle
2024-04-22 12:35:58 UTC
At a glance, it looks like AutoLocation is working the same. Can you be more specific about the issue you're having? 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. 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. 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 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 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> 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? (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 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> 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? 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? (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. 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> Created attachment 166964 [details] [review] Bug 36665: (follow-up) Wrap code block in type ne 'opac' conditional 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) (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! 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) (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 (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 (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 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?) Set to Failed QA, to keep overview of my queue. Waiting for Nick. (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 As we are splitting the 2 "features" on this one (36665) I think we should deal with that here. I am not blocking this. Created attachment 167035 [details] [review] Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation Created attachment 167036 [details] [review] Bug 36665: (follow-up) Allow choosing a branch with no IP when using AutoLocation 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> 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> 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> Pushed for 24.05! Well done everyone, thank you! Can this be backported to 23.11? 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. Moving to PQA for attention on follow-up Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.06 I had to remove from DBRev : -use Koha::Installer::Output qw(say_warning say_failure say_success say_info); Backported to 23.05.x for upcoming 23.05.12 |