Bug 28657 - Expand branches.branchip to allow for multiple space separated IP ranges
Summary: Expand branches.branchip to allow for multiple space separated IP ranges
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Michael Hafen
QA Contact: Testopia
URL:
Keywords:
Depends on: 36028
Blocks: 28658
  Show dependency treegraph
 
Reported: 2021-07-02 17:01 UTC by Michael Hafen
Modified: 2024-02-07 23:49 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (6.64 KB, patch)
2021-07-02 17:04 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28657: expand branches.branchip to allow for multiple ip ranges. (7.18 KB, patch)
2022-04-27 16:17 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28657: expand branches.branchip to allow for multiple ip ranges. (7.33 KB, patch)
2022-04-27 17:23 UTC, Michael Hafen
Details | Diff | Splinter Review
ug 28657: expand branches.branchip to allow for multiple ip ranges. (7.00 KB, patch)
2023-04-10 21:43 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28657: expand branches.branchip to allow for multiple ip ranges. (7.01 KB, patch)
2023-05-19 16:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28657: (QA follow-up) Return 1 if no range, improve unit tests (3.25 KB, patch)
2023-05-19 16:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28657: expand branches.branchip to allow for multiple ip ranges (13.24 KB, patch)
2023-10-05 19:43 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28657: expand branches.branchip to allow for multiple ip ranges. (13.29 KB, patch)
2024-02-07 23:49 UTC, Michael Hafen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Hafen 2021-07-02 17:01:12 UTC
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.
Comment 1 Michael Hafen 2021-07-02 17:04:30 UTC
Created attachment 122572 [details] [review]
Proposed patch

Proposed patch
Comment 2 Lucas Gass 2022-04-27 15:16:50 UTC
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?
Comment 3 Michael Hafen 2022-04-27 16:17:25 UTC
Created attachment 134126 [details] [review]
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.
Comment 4 Michael Hafen 2022-04-27 17:23:14 UTC
Created attachment 134173 [details] [review]
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.
Comment 5 George Veranis 2022-05-12 09:45:08 UTC
Almost the same is that Bug11180 , with a little bit more better approach to be compatible CIDR notation .
Comment 6 Marcel de Rooy 2022-05-12 16:34:30 UTC
Please fill assignee field
Comment 7 Michael Hafen 2023-04-10 21:43:01 UTC
Created attachment 149412 [details] [review]
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.
Comment 8 Kyle M Hall 2023-05-19 16:03:43 UTC
Created attachment 151484 [details] [review]
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>
Comment 9 Kyle M Hall 2023-05-19 16:03:49 UTC
Created attachment 151485 [details] [review]
Bug 28657: (QA follow-up) Return 1 if no range, improve unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Martin Renvoize 2023-09-11 14:04:02 UTC
(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 ?
Comment 11 George Veranis 2023-09-14 11:49:42 UTC
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 .
Comment 12 Marcel de Rooy 2023-09-29 08:43:03 UTC
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.
Comment 13 Michael Hafen 2023-10-05 19:43:55 UTC
Created attachment 156582 [details] [review]
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.
Comment 14 Matthias Le Gac 2024-01-10 19:01:15 UTC
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.
Comment 15 Michael Hafen 2024-02-07 22:49:35 UTC
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.
Comment 16 Michael Hafen 2024-02-07 23:49:41 UTC
Created attachment 161872 [details] [review]
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.