Project

General

Profile

Bug #5188

vpn_ipsec_settings.php "Auto-exclude LAN address" changes lost

Added by Chris Buechler over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Target version:
Start date:
09/22/2015
Due date:
% Done:

100%

Estimated time:

Description

The changes to rename field, update description, and fix check/uncheck of "Auto-exclude LAN address" field (as labeled in 2.2.4, still former "Bypass LAN address" in 2.3), got lost in the merge.

The text needs to be replaced with what's in 2.2.4, and the check/uncheck behavior fixed to work correctly. Right now checking it never gets saved.

Associated revisions

History

#1 Updated by Steve Beaver over 3 years ago

  • Status changed from Confirmed to Assigned

Three new fields need to be added:

  1. CRL Checking
  2. Make before Break
  3. Auto-exclude LAN address

#2 Updated by Steve Beaver over 3 years ago

  • Status changed from Assigned to Feedback
  • Assignee changed from Steve Beaver to Chris Buechler

New fields added and tested

#3 Updated by Steve Beaver over 3 years ago

  • % Done changed from 0 to 100

#4 Updated by Chris Buechler over 3 years ago

  • Status changed from Feedback to Assigned
  • Assignee changed from Chris Buechler to Steve Beaver

most of this is good, and good catch on the 2 missing fields.

One issue still: The setting/unsetting of the config value for "Auto-exclude LAN address" is backwards from what it should be. It's checked if that config setting doesn't exist, and un-checked if the config setting does exist. That was a bit of a saga before when this was implemented, if that creates a mess for some reason we can reconsider that behavior.

#5 Updated by Steve Beaver over 3 years ago

Yeah. The code contains this:

// The logic value sent by $POST is opposite to the way it is stored in the config.
// Reset the $pconfig value so it reflects the opposite of what was $POSTed.
if ($_POST['noshuntlaninterfaces'] == "yes") {
$pconfig['noshuntlaninterfaces'] = false;
} else {
$pconfig['noshuntlaninterfaces'] = true;
}

So the behavior was obviously required at some point. Easy to remove of course.

#6 Updated by Phillip Davis over 3 years ago

The UI box "Auto-exclude LAN address" asks for "Enable bypass for LAN interface IP". That is the reverse question to the name of the setting "noshuntlaninterfaces".

1) In a default config that has no mention of "noshuntlaninterfaces" then the checkbox "Enable bypass for LAN interface IP" should be checked on first display.

2) If the checkbox is unchecked and save is pressed, then the config gets "noshuntlaninterfaces" put in it (and that then flows through the back-end code to implement the settings)

3) If the checkbox is unchecked and save is pressed, then the config gets "noshuntlaninterfaces" removed (there should be no mention of it any more in the config)

As long as it works like the above, then do whatever is needed in the underlying code to make it so.

#7 Updated by Phillip Davis over 3 years ago

I wish I could edit my posts on Redmine!!!

The UI box "Auto-exclude LAN address" asks for "Enable bypass for LAN interface IP". That is the reverse question to the name of the setting "noshuntlaninterfaces".

1) In a default config that has no mention of "noshuntlaninterfaces" then the checkbox "Enable bypass for LAN interface IP" should be checked on first display.

2) If the checkbox is unchecked and save is pressed, then the config gets "noshuntlaninterfaces" put in it (and that then flows through the back-end code to implement the setting)

3) If the checkbox is checked and save is pressed, then the config gets "noshuntlaninterfaces" removed (there should be no mention of it any more in the config)

As long as it works like the above, then do whatever is needed in the underlying code to make it so.

#8 Updated by Phillip Davis over 3 years ago

The latest code in pfsense/master looks like it should be working.
I made pull request https://github.com/pfsense/pfsense/pull/1931 - that changes some variable names and comment text that might help future maintainers to understand this beast. Take it if you like it, close it if you don't.

#9 Updated by Steve Beaver over 3 years ago

  • Status changed from Assigned to Resolved

P Davis clarifications merged. Thanks!

Also available in: Atom PDF