This routine seems to go looking to see if the passed-in interface is PPP-style. At the end, if it is not PPP-style then it calls pfsense_ngctl_detach.
This foreach loop in its current state will always exit after the first iteration that is not mode "server". But it looks like it should look through all the 'pppoe' entries until it finds the interface or gets to the end.
In theory the code will sometimes miss setting $found = true when it should have. And thus pfsense_ngctl_detach would get called later for a PPP-style interface.
I noticed this while reviewing for code style guide - it is an example where the indenting shows the intention but there are no curlies to implement it.
Forum: https://forum.pfsense.org/index.php?topic=89302.0
If the user put "none" in the 'gateway' field for the DHCP settings of an interface, that would set $add_routers to false at line 742. Coming around the loop again for a subsequent interface, and going through the else line 744, nothing would set $add_routers back to true (actually back to the value originally calculated at line 461).
Use a different var to remember the boolean value calculated at line 461 so it can be remembered and used as needed each time through the loop.
I set Gateway "none" on my LAN and left it blank on OPT1 and came up with the same problem as the forum post - OPT1 got no " option routers" statement in dhcpd.conf
After this code fix it looks good.
Currently if the user is clicking around while they are setting up an OpenVPN server, they can do stuff like this:
a) Select Server Mode - Remote Access (SSL/TLS + User Auth)
b) Select something in Backend for authentication
c) Change their mind and select Server Mode - Peer to Peer (SSL/TLS)
d) Enter the other settings and Save
Now the OpenVPN server config has an 'authmode' key in it.
Probably does no harm, I suspect it will simply not be used when building the server.conf for Peer to Peer, but it looks a bit odd when analysing/diagnosing a config for problems.
Other fields that are mode-specific have tests to only save the values at the end if the appropriate mode is the one finally selected.
While I am here, I also constantly forget to click on "Local Database" authmode when setting up a new server. It gives the validation error message, then I click on "Local Database" again and save. Seems unnecessary - when defining a new OpenVPN server why not default this to have the first entry in the list be the one selected? So I did that. What do you think? 1 place less for many users to need to click.
