[Bug 28657] New: Expand branches.branchip to allow for multiple space separated ip ranges
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Bug ID: 28657 Summary: Expand branches.branchip to allow for multiple space separated ip ranges Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: System Administration Assignee: koha-bugs@lists.koha-community.org Reporter: michael.hafen@washk12.org QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #1 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 122572 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122572&action=edit Proposed patch Proposed patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28658 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28658 [Bug 28658] With SearchMyLibraryFirst if library isn't set for the opac, try to guess it by checking branchip -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |lucas@bywatersolutions.com --- Comment #2 from Lucas Gass <lucas@bywatersolutions.com> --- Hi Micheal, This patchset no longer applies cleanly to current master. Can you rebase it? Also, would it be possible to include a test plan? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122572|0 |1 is obsolete| | --- Comment #3 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 134126 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134126&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134126|0 |1 is obsolete| | --- Comment #4 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 134173 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134173&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. Rebased on Current (2022-04-27) master change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 George Veranis <gveranis@dataly.gr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gveranis@dataly.gr --- Comment #5 from George Veranis <gveranis@dataly.gr> --- Almost the same is that Bug11180 , with a little bit more better approach to be compatible CIDR notation . -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=11180 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Please fill assignee field -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |michael.hafen@washk12.org |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134173|0 |1 is obsolete| | --- Comment #7 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 149412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149412&action=edit ug 28657: expand branches.branchip to allow for multiple ip ranges. [rebased 2023-04-10] change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149412|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 151484 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151484&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 151485 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151485&action=edit Bug 28657: (QA follow-up) Return 1 if no range, improve unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|kyle@bywatersolutions.com |testopia@bugs.koha-communit | |y.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to George Veranis from comment #5)
Almost the same is that Bug11180 , with a little bit more better approach to be compatible CIDR notation .
I'm not sure I understand this comment.. are you suggesting you prefer the approach here or the approach on bug 11180 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #11 from George Veranis <gveranis@dataly.gr> --- both approaches are similar I don't prefer one instead of other. My concern was about CIDR notation but as I see it is ok also on that solution. so it is ok . -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |m.de.rooy@rijksmuseum.nl --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Thanks for your patch, Michael. It still needs a bit of attention: WARN tidiness The file is less tidy than before (bad/messy lines before: 446, now: 447) + $dbh->do( "ALTER TABLE `branches` MODIFY `branchip` mediumtext COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'the IP address(s) for your library or branch'" ); + `branchip` mediumtext DEFAULT NULL COMMENT 'the IP address for your library or branch', These are not identical. Look at collation and comment. installer/data/mysql/atomicupdate/bug_28657.perl Please adjust to "new" style. We are doing that for quite some time already. Extension should be pl now See skeleton file. + # FIXME remove '*' for backwards compatibility in branchip settings Why FIXME? You remove it one line later.. koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt If we do want * in the IP field, we should check here. And server side probably too (library->store or so). t/Auth.t Show that * does no longer work. And show that 1.2.3. as range does work. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151484|0 |1 is obsolete| | Attachment #151485|0 |1 is obsolete| | --- Comment #13 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 156582 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156582&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges [squashed follow-up for QA 2023-10-05] Bug 28657: expand branches.branchip to allow for multiple ip ranges. change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() shouldn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Bug 28657: (QA follow-up) Return 1 if no range, improve unit tests Bug 28657: (Follow-up for QA) Tidy the code and make it more consident remove branches.tt branchip input maxlength attribute. Have the db_rev match the kohastructure.sql for column collation and comment. Use new style db_rev skeleton. Fix comment C4::Auth. Have Koha::Library->store() watch for and remove '*' from branchip. Add a couple more test for full coverage. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Expand branches.branchip to |Expand branches.branchip to |allow for multiple space |allow for multiple space |separated ip ranges |separated IP ranges -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Matthias Le Gac <matthias.le-gac@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matthias.le-gac@inlibro.com --- Comment #14 from Matthias Le Gac <matthias.le-gac@inlibro.com> --- All it's work if I put the PC ip address in the list of ip for a a branches branchip. But if I don't put the PC ip address I can login but every time I click on a button I need to login again and again. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36028 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #15 from Michael Hafen <michael.hafen@washk12.org> --- I think the behavior of getting past the login screen, but being directed back to it is a different bug. I have opened #36028 (now in the 'see also') with a patch to fix that. With that patch and this one you should get an error when there is no ip, or you are are an ip not covered by your branch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |36028 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36028 [Bug 36028] When AutoLocation is on there is no error message when the user is at the wrong branch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156582|0 |1 is obsolete| | --- Comment #16 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 161872 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161872&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. [ conflicts with patch for bug 36028, so this is now dependent on that ] change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address ag ainst branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do ). Change note in admin/branches.tt under branchip field to reflect changes to bran chip handling. Bug 28657: (QA follow-up) Return 1 if no range, improve unit tests Bug 28657: (Follow-up for QA) Tidy the code and make it more consident remove branches.tt branchip input maxlength attribute. Have the db_rev match the kohastructure.sql for column collation and comment. Use new style db_rev skeleton. Fix comment C4::Auth. Have Koha::Library->store() watch for and remove '*' from branchip. Add a couple more test for full coverage. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |olivier.hubert@inlibro.com --- Comment #17 from Olivier Hubert <olivier.hubert@inlibro.com> --- As far as I can tell, the AutoLocation system preference was removed. So unfortunately this patch cannot be tested anymore. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #18 from Lucas Gass <lucas@bywatersolutions.com> --- (In reply to Olivier Hubert from comment #17)
As far as I can tell, the AutoLocation system preference was removed. So unfortunately this patch cannot be tested anymore.
It has been renamed as StaffLoginBranchBasedOnIP. See Bug 26176 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #19 from Olivier Hubert <olivier.hubert@inlibro.com> --- Yes, I was just about to write that I was able to find the updated preference name. I tried applying the patch and hit a conflict: git bz apply 28657 Bug 28657 - Expand branches.branchip to allow for multiple space separated IP ranges 161872 - Bug 28657: expand branches.branchip to allow for multiple ip ranges. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28657: expand branches.branchip to allow for multiple ip ranges. error: sha1 information is lacking or useless (C4/Auth.pm). error: could not build fake ancestor Patch failed at 0001 Bug 28657: expand branches.branchip to allow for multiple ip ranges. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161872|0 |1 is obsolete| | --- Comment #20 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 172149 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172149&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. [Rebased on main ~ 2024-09-08] Bug 28657: expand branches.branchip to allow for multiple ip ranges. change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on StaffLoginBranchBasedOnIP system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #21 from Olivier Hubert <olivier.hubert@inlibro.com> --- Thanks for the quick turnaround. The patch now applies properly. However, I am unable to use the test plan as designed. Before the patch is applied, the branchip field only allows 15 characters so it is impossible to enter a single IP range, much less several of them. If you reset things to before the patch, are you able to follow the test plan? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #22 from Michael Hafen <michael.hafen@washk12.org> --- There is an atomic database update in the patch which adjusts the database column for branch ip up to a mediumtext field. That should allow you to continue the test plan. I didn't think of mentioning applying that database change in the test plan, sorry. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #23 from Olivier Hubert <olivier.hubert@inlibro.com> --- Thanks for pointing out the atomic update. I was able to save IP ranges in the branchid for the CPL branch. However, whatever I put in StaffLoginRestrictLibraryByIP, I am either always or never able to have the Centerville library show up in the Library dropdown list. Applying the patch or removing it does not seem to change the behaviour. I will have to take a look at it again at some later date. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #24 from Michael Hafen <michael.hafen@washk12.org> --- I missed the template plugin, that's a new code block for me. I'll upload another patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #172149|0 |1 is obsolete| | --- Comment #25 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 172197 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172197&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. . Change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. . Change C4/Auth.pm and Koha/Template/Plugin/Branches.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) . Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. . Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on StaffLoginRestrictLibraryByIp or StaffLoginBranchBasedOnIP system preference 2. Change a libraries branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The library won't appear in the select list. The location check will fail. 4. Apply patch and database update in patch. Restart plack if necessary. 5. Try logging in again. The library will appear in the select list and the location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #172197|0 |1 is obsolete| | --- Comment #26 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 184061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184061&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. [ updated to cover maxLength in /api/v1/swagger/definitions/library.yaml ] change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 184061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184061 Bug 28657: expand branches.branchip to allow for multiple ip ranges. Review of attachment 184061: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=28657&attachment=184061) ----------------------------------------------------------------- ::: C4/Auth.pm @@ +2474,5 @@
sub in_iprange { my ($iprange) = @_; + my $result = 0; + # FIXME remove '*' for backwards compatibility in branchip settings + $iprange =~ s|\*||g;
Since you're changing in_iprange, you should update the unit test in t/Auth.t accordingly. That is, you should add a test with at least one * in it. Fair enough changing initial $result to 0. That "in_iprange" is some funky looking code... ::: installer/data/mysql/atomicupdate/bug_28657.perl @@ +1,3 @@
+$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do( "ALTER TABLE `branches` MODIFY `branchip` mediumtext COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'the IP address(s) for your library or branch'" );
Why are you adding the collation there? From memory, I think that will make an upgraded DB vary slightly from a new DB. ::: installer/data/mysql/kohastructure.sql @@ +1670,4 @@
`branchreturnpath` longtext DEFAULT NULL COMMENT 'the email to be used as Return-Path', `branchurl` longtext DEFAULT NULL COMMENT 'the URL for your library or branch''s website', `issuing` tinyint(4) DEFAULT NULL COMMENT 'unused in Koha', + `branchip` mediumtext DEFAULT NULL COMMENT 'the IP address for your library or branch',
The comments in kohastructure.sql and bug_28657.perl are different. They should be the same. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |dcook@prosentient.com.au --- Comment #28 from David Cook <dcook@prosentient.com.au> --- Marking Failed QA due to missing unit test and inconsistent DB comments. I'm looking into the collation thing. I don't know if we documented that anywhere. I can see an example in the coding guidelines where a column level collation is used (https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL13:_Modifying_colu...) but I'm pretty sure that's wrong. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184061|0 |1 is obsolete| | --- Comment #29 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 184063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184063&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. [Updated to pass QA] change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change tests to account for new default, and add a test for '*'. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #30 from Michael Hafen <michael.hafen@washk12.org> --- Added updated patch due to QA comments, and setting back to Needs Signoff. Thanks. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #31 from David Cook <dcook@prosentient.com.au> --- (In reply to Michael Hafen from comment #30)
Added updated patch due to QA comments, and setting back to Needs Signoff. Thanks.
Thanks for that. That's looking much better. Although you've got a typo in the DB comments: 'address(s)' should be 'address(es)' -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184063|0 |1 is obsolete| | --- Comment #32 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 184064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184064&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. [ QA follow up, now with more typo fixes ;) ] change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address ag ainst branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do ). Also, have it trim '*' in case someone adds some back later. Change tests to account for new default, and add a test for '*'. Change note in admin/branches.tt under branchip field to reflect changes to bran chip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #33 from Chris Cormack <chris@bigballofwax.co.nz> --- Automated patch testing results: [PASS] ALL PATCHES APPLY CLEANLY All patches apply successfully to the current codebase. **Patch Results:** [PASS] 0001-Bug-28657-expand-branches.branchip-to-allow-for-mult.patch (ID: 184064) Status: SUCCESS Dependencies failed: 36028 (failed to apply) + Patch applies cleanly **Dependencies:** 36028 --- This is an automated message from the patch testing system. Tested against branch: main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA Version|unspecified |Main --- Comment #34 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- This patch set needs a little work: 1) There is a small conflict in the template, I resolved it for some further testing. I believe size should be removed with the new grid layouts. 2) The database update is "old style" and requires updating using the current template. See: https://wiki.koha-community.org/wiki/Database_updates#How_to_write_an_atomic... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #35 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- There also is a dependency on 36028 which is NEW, but has a patch. Can you please unravel? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Bug 28657 depends on bug 36028, which changed state. Bug 36028 Summary: When AutoLocation is on there is no error message when the user is at the wrong branch https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36028 What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |DUPLICATE -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|36028 | See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36028 | --- Comment #36 from Michael Hafen <michael.hafen@washk12.org> --- Resolving half of QA comment: Bug 36028 has been closed as a duplicate since the issue has now been fixed by another bug. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36028 [Bug 36028] When AutoLocation is on there is no error message when the user is at the wrong branch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184064|0 |1 is obsolete| | --- Comment #37 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 200871 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200871&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. [ 2026-06-22 : change db update to new template and resolved conflict in template ] change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change tests to account for new default, and add a test for '*'. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #38 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Michael, I think you might need to update your setup - the files are not tidy. This should now happen automatically when using ktd on commit. [FAIL] C4/Auth.pm FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl C4/Auth.pm` [FAIL] Koha/Template/Plugin/Branches.pm SKIP pod_coverage No POD exists FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl Koha/Template/Plugin/Branches.pm` [PASS] api/v1/swagger/definitions/library.yaml [FAIL] installer/data/mysql/atomicupdate/bug_28657.pl FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl installer/data/mysql/atomicupdate/bug_28657.pl` I've amended the patch so everything is tidy. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28657 --- Comment #39 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 201120 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201120&action=edit Bug 28657: expand branches.branchip to allow for multiple ip ranges. change branches.branchip to a mediumtext, and remove '*' from existing branchip settings. Change C4/Auth.pm from a regexp to using in_iprange() to check client address against branchip. (in_iprange() doesn't work with '*' in branchip.) Change in_iprange() to default to false (which is what the POD says it should do). Also, have it trim '*' in case someone adds some back later. Change tests to account for new default, and add a test for '*'. Change note in admin/branches.tt under branchip field to reflect changes to branchip handling. Test plan: 1. Turn on AutoLocation system preference 2. Change a branches branchip to multiple ip ranges seperated by a space, making sure one of the ranges includes your computers address. 3. Log out and try logging in. The location check will fail. 4. Apply patch. Restart plack if necessary. 5. Try logging in again. The location check will succeed. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Tidied some files and amended the patch. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org