luci-mod-system: improve Dropbear interface binding selection - #8948
luci-mod-system: improve Dropbear interface binding selection#8948Ser9ei wants to merge 1 commit into
Conversation
|
@systemcrash & @jow-, could you please take a look when you have a moment? This builds on the previous dropbear listen options UI (boolean "Bind to Interface") and replaces it with an explicit three-way selector. |
00ac738 to
301bf5e
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. The three-way selector approach looks sound — _bind_to is correctly kept out of UCI (the write override is needed, since the default write() would happily persist _bind_to just like the old _direct did), the DirectInterface-before-Interface precedence in cfgvalue() matches the dropbear init script's own precedence, and mode switching clears the unused option through the normal inactive-option removal path. No new UCI options are introduced, so there is no backend-coupling gap.
Three inline notes, mainly about the interaction between the new radio and the interface dropdowns.
Generated by Claude Code
| o.validate = function(section, value) { | ||
| return value ? true : _('Please select an interface'); | ||
| }; |
There was a problem hiding this comment.
The dropdown still offers an explicit unspecified choice, which now duplicates the new All interfaces (unspecified) radio and can only ever produce this validation error. widgets.NetworkSelect adds that entry whenever rmempty or optional is set (widgets.js:507-508),`` and rmempty defaults to `true` (form.js:1431) — neither option sets it here.
Setting rmempty = false drops the entry from the choice list and makes the option required, so the custom validate() becomes redundant: parse() already rejects an empty active value with Option "Interface" must not be empty. (form.js:2152-2161). Clearing on mode switch is unaffected — an inactive option is removed via the !this.retain branch (form.js:2167-2169) regardless of rmempty.
| o.validate = function(section, value) { | |
| return value ? true : _('Please select an interface'); | |
| }; | |
| o.rmempty = false; |
Same applies to lines 50-52 for Interface.
Generated by Claude Code
| this.remove(section); | ||
| }; | ||
|
|
||
| o = s.option(widgets.NetworkSelect, 'DirectInterface', _('Interface'), _('Listen only on the given interface or, if unspecified, on all')); |
There was a problem hiding this comment.
The , if unspecified, on all clause no longer describes reachable behaviour after this change: "unspecified" is now the separate All interfaces (unspecified) radio, and the validate() added below turns an empty selection into an error rather than a bind-to-all fallback. Leaving the old wording keeps the ambiguity the PR is trying to remove.
| o = s.option(widgets.NetworkSelect, 'DirectInterface', _('Interface'), _('Listen only on the given interface or, if unspecified, on all')); | |
| o = s.option(widgets.NetworkSelect, 'DirectInterface', _('Interface'), _('Listen only on the given interface')); |
Line 47 has the same stale clause — _('Listen on up to 10 IPs on the given interface or, if unspecified, on all interfaces') should become _('Listen on up to 10 IPs on the given interface').
Generated by Claude Code
| o.default = o.disabled; | ||
| // Virtual option: derives UI mode from Interface/DirectInterface, | ||
| // is not stored in UCI and clears the real options on change | ||
| o = s.option(form.ListValue, '_bind_to', _('Bind to'), _('Select how the SSH service should be bound to network interfaces or IP addresses')); |
There was a problem hiding this comment.
nit: the replaced _direct flag was actually persisted to /etc/config/dropbear, unlike the new _bind_to. form.Flag.parse() writes any non-default value (form.js:5153-5156) and uci.set() only filters option names starting with ., not _ (uci.js:607). So every instance where a user had ticked "Bind to Interface" carries a stale option _direct '1' that nothing reads any more (the dropbear init script only consumes Interface/DirectInterface), and this PR never removes it.
Would it be worth clearing it here, e.g. this.map.data.unset('dropbear', section, '_direct') alongside the this.remove(section) in the write handler below, so configs get tidied up on the next save?
Unrelated to that: the comment says the virtual option "clears the real options on change", but the clearing actually comes from the framework removing Interface/DirectInterface once their depends() stop matching — write() here only suppresses _bind_to itself.
Generated by Claude Code
301bf5e to
af34b76
Compare
|
Updated:
|
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push (301bf5e → af34b76). The two earlier notes about the stale or, if unspecified, on all clauses and the misleading code comment are addressed, and the rmempty = false suggestion is fairly answered in your comment — dropping the unspecified entry would make NetworkSelect preselect whatever network happens to come first, so keeping the custom validate() is the better trade. I traced the empty-value path to confirm the custom validator actually fires: Validator.validate() only short-circuits on an empty value when the field is not optional, otherwise it falls through to vfunc (validation.js:208-236),`` so "Please select an interface" does get raised.
The new uci-defaults migration is correct as written. Worth recording for whoever merges: _direct did ship in a release — it is on openwrt-25.12 via a cherry-pick of 37aa280 — so the migration is not dead code. It also reaches users on both paths (sysupgrade first boot sees the new file since it has no overlay whiteout, and an apk/opkg upgrade of luci-mod-system alone gets it through default_postinst). config_load handles the case where /etc/config/dropbear is absent without erroring, _direct survives the uci export → config_set round-trip fine despite the leading underscore, and the 90_ prefix orders it after 50-dropbear.
Two nits inline, both on the new script; nothing blocking.
Generated by Claude Code
There was a problem hiding this comment.
nit: /etc/uci-defaults/ is a single flat namespace shared by every installed package, and this filename doesn't say which package owns it — it reads as if it belongs to the dropbear package, which already installs its own /etc/uci-defaults/50-dropbear from openwrt.git.
Every other uci-defaults file shipped by a LuCI package names its LuCI package: 50_luci-mod-admin-full, 40_luci-statistics, 95-luci-app-banip-housekeeping, 30_luci-theme-*, etc. Something like 90_luci-mod-system-dropbear would keep that pattern and make the owner obvious when the file shows up in luci-mod-system.list.
Generated by Claude Code
| is_commit=0 | ||
|
|
||
| dropbear_update() { | ||
| local _direct | ||
|
|
||
| config_get _direct "$1" _direct | ||
| [ -n "$_direct" ] || return 0 | ||
|
|
||
| if uci -q delete dropbear."$1"._direct; then | ||
| is_commit=1 | ||
| fi | ||
| } | ||
|
|
||
| config_load "dropbear" | ||
| config_foreach dropbear_update dropbear | ||
| [ $is_commit -eq 0 ] || uci commit dropbear |
There was a problem hiding this comment.
nit: the is_commit bookkeeping doesn't buy anything — both runners of uci-defaults scripts already uci commit unconditionally right after sourcing them: uci_apply_defaults() in /etc/init.d/boot on the boot path and default_postinst() on the package-install path. That's why the in-tree precedents for exactly this kind of stale-option cleanup just stage the delete and stop — 95-luci-app-attendedsysupgrade-housekeeping`` does uci -q delete with no commit at all, and openwrt's own `dropbear.defaults` relies on the same thing.
An unconditional uci -q commit dropbear is a no-op when the delta is empty, so the flag can go (also drops the unquoted $is_commit in [ ]). Using config_foreach is still right here, since s.addremove = true means there can be several dropbear sections.
| is_commit=0 | |
| dropbear_update() { | |
| local _direct | |
| config_get _direct "$1" _direct | |
| [ -n "$_direct" ] || return 0 | |
| if uci -q delete dropbear."$1"._direct; then | |
| is_commit=1 | |
| fi | |
| } | |
| config_load "dropbear" | |
| config_foreach dropbear_update dropbear | |
| [ $is_commit -eq 0 ] || uci commit dropbear | |
| dropbear_update() { | |
| local _direct | |
| config_get _direct "$1" _direct | |
| [ -n "$_direct" ] || return 0 | |
| uci -q delete dropbear."$1"._direct | |
| } | |
| config_load "dropbear" | |
| config_foreach dropbear_update dropbear | |
| uci -q commit dropbear |
Generated by Claude Code
Replace the boolean "Bind to Interface" option with a three-way "Bind to" selector to make the distinction between Interface and DirectInterface explicit. The virtual _bind_to option is not stored in UCI; existing Interface/DirectInterface options and the init script are unchanged. Legacy _direct UI state is removed from existing configurations by a one-time uci-defaults migration. Signed-off-by: Sergei S. <it4notice@proton.me>
af34b76 to
c10f903
Compare
|
Update: Renamed the uci-defaults file to 90_luci-mod-system-dropbear. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The force-push (af34b76 → c10f903) contains only the rename 90_dropbear_remove_direct → 90_luci-mod-system-dropbear, which addresses the naming nit; the script body and dropbear.js are byte-identical to what I reviewed before. The commit message still matches the diff. Nothing references the old filename, and luci.mk installs everything under root/ automatically, so no .list/Makefile update is needed. The 0644 mode is fine — both runners source the file rather than exec it (uci_apply_defaults() in /etc/init.d/boot and default_postinst()) — and matches the majority of in-tree LuCI uci-defaults files.
The is_commit nit is answered in your comment and left for a human reviewer to call, so I'm not re-raising it. FormalityCheck is green on c10f903.
Generated by Claude Code
|
CI is waiting for approval (first-time contributor). @hnyman would you |
Motivation
The current Dropbear LuCI configuration exposes interface binding through a boolean
"Bind to Interface" option and two different Interface fields:
network interface;
This works correctly internally, but the UI is confusing because the same-looking Interface selector represents different Dropbear configuration options depending on the state of the flag.
The current UI can therefore be ambiguous:
versus:
Although both configurations display an Interface selector, they result in different Dropbear behaviour.
Additionally, the UI flag was persisted as “option _direct” in UCI, although the dropbear service never uses it.
Description
New UI
Replace the boolean "Bind to Interface" option with a three-way "Bind to" selection:
The three modes map to the existing Dropbear configuration:
All interfaces
Neither Interface nor DirectInterface is configured.
IP addresses of interface
The existing Interface option is used.
Network interface
The existing DirectInterface option is used.
Implementation
The change is limited to the LuCI dropbear configuration.
A virtual "_bind_to" ListValue controls the UI. Its value is derived from the existing Interface/DirectInterface options and is not persisted.
Added a one-time "uci-defaults" migration to remove the legacy "_direct" UI state from existing Dropbear configs.
Screenshot or video of changes
Maintainer (preferred)
Tested on
OpenWrt version: OpenWrt 25.12.5 (r33051-f5dae5ece4)
LuCI version: LuCI openwrt-25.12 branch (26.209.73834~b61f907)
Web browser(s): Firefox 153.0.4
Checklist