luci-app-radsecproxy: new LuCI app for radsecproxy#8862
Conversation
This app provides a GUI for configuring radsecproxy Signed-off-by: Jan-Frederik Rieckers <rieckers@dfn.de>
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Fresh review of this new luci-app-radsecproxy app (1 commit). The frontend/backend coupling looks sound: the UCI options map to the radsecproxy init script's config generation, and the dependency on openwrt/packages#30044 is declared in the PR description. Inline comments cover a wrong datatype, an inverted Enabled/Disabled dropdown mapping (cross-checked against the init script's bool handling), a missing translation wrapper, and a couple of robustness nits. Left as review comments since the PR is still a draft.
Generated by Claude Code
| o.modalonly = true; | ||
|
|
||
| o = s.taboption('advanced', form.Value, 'CACertificatePath', _('CA Certificate path'), _('Path to directory containing CA certificate(s) and CRLs.')); | ||
| o.datatype = 'integer'; |
There was a problem hiding this comment.
CACertificatePath is a filesystem path (the description says "Path to directory containing CA certificate(s) and CRLs"), but datatype = 'integer' will reject any path in the client-side validation, making the field unusable. It should be a string.
| o.datatype = 'integer'; | |
| o.datatype = 'string'; |
Generated by Claude Code
| o.value('0', 'Enabled'); | ||
| o.value('1', 'Disabled'); |
There was a problem hiding this comment.
The Enabled/Disabled labels look inverted. The selected value is written verbatim to UCI, and the init script's append_bools maps UCI 0 → off and 1 → on (radsecproxy.init:42-43). So value 0 here disables SNI and 1 enables it — the opposite of these labels. This is also consistent with the global SNI Flag where enabled = 1.
| o.value('0', 'Enabled'); | |
| o.value('1', 'Disabled'); | |
| o.value('0', 'Disabled'); | |
| o.value('1', 'Enabled'); |
Same inversion applies to the per-server LoopPrevention dropdown at lines 619-620 below.
Generated by Claude Code
| o.depends('type', 'dtls'); | ||
| o.modalonly = true; | ||
|
|
||
| o = s.taboption('tls', form.Flag, 'certificateNameCheck', _('CertificateNameCheck'), ('Check the name in the Certificate against the configured name / host. Disable this to use a custom name check.')); |
There was a problem hiding this comment.
nit: the help text is passed as a bare string ('...') instead of _('...'), so this description won't be translatable. The identical client-side option at line 403 correctly wraps it in _().
| o = s.taboption('tls', form.Flag, 'certificateNameCheck', _('CertificateNameCheck'), ('Check the name in the Certificate against the configured name / host. Disable this to use a custom name check.')); | |
| o = s.taboption('tls', form.Flag, 'certificateNameCheck', _('CertificateNameCheck'), _('Check the name in the Certificate against the configured name / host. Disable this to use a custom name check.')); |
Generated by Claude Code
| o.rows = 25; | ||
| o.rmempty = true; | ||
| o.cfgvalue = function(section_id) { | ||
| return fs.read(config_file); |
There was a problem hiding this comment.
fs.read rejects if /etc/radsecproxy.conf does not exist (or is unreadable). On a fresh install where the file has not been created yet, this rejected promise will make the Advanced config-file field fail to load. Wrap it in L.resolveDefault so it falls back to empty content.
| return fs.read(config_file); | |
| return L.resolveDefault(fs.read(config_file), ''); |
Generated by Claude Code
| 'require ui'; | ||
|
|
||
| function writeFile(path, content) { | ||
| if (content) { |
There was a problem hiding this comment.
The if (content) guard skips the write when content is an empty string, so a user who clears the Advanced config-file textarea (the TextValue has rmempty = true) can never empty /etc/radsecproxy.conf through the UI — the old contents persist. Consider guarding on content != null instead so an intentional clear is written through.
Generated by Claude Code
| LUCI_DEPENDS:=+luci-base +radsecproxy | ||
|
|
||
| PKG_MAINTAINER:=Jan-Frederik Rieckers <rieckers@dfn.de> | ||
| PKG_LICENSE:=BSD-3-CLAUSE |
There was a problem hiding this comment.
nit: SPDX license identifiers are case-sensitive; the canonical form is BSD-3-Clause, not BSD-3-CLAUSE. All other in-tree luci-app-* Makefiles use the exact SPDX casing (e.g. Apache-2.0, GPL-2.0).
| PKG_LICENSE:=BSD-3-CLAUSE | |
| PKG_LICENSE:=BSD-3-Clause |
Generated by Claude Code
This change implements (hopefully all) suggestions from openwrt-ai in PR openwrt#8862 Signed-off-by: Jan-Frederik Rieckers <rieckers@dfn.de>
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The new commit correctly addresses all six points from the prior review (datatype string fix, both inverted Enabled/Disabled dropdowns, the _() translation wrapper, L.resolveDefault around fs.read, the content != null write guard, and the BSD-3-Clause SPDX casing).
Generated by Claude Code
2 participants
This app provides a GUI for configuring radsecproxy
Pull request details
Description
This adds a new LuCI interface for radsecproxy.
It allows to configure (almost) every radsecproxy functionality. Only exception are config options deprecated in the upstream radsecproxy.
It also includes the option to add custom configuration via a file that is included in the final configuration.
This depends on openwrt/packages#30044 for full functionality (since not all config options are yet included in the init file that builds the actual radsecproxy configuration).
Screenshot or video of changes (if applicable)
Maintainer (preferred)
No one, since this is a new LuCI app packet.
Tested on
OpenWrt version: OpenWrt 25.12.5 (r33051-f5dae5ece4)
LuCI version: LuCI openwrt-25.12 branch (26.187.49110~99464ec)
Web browser(s): Mozilla Firefox 140.12.0esr
Checklist