luci-app-upnp: Complete plugin wording/UI revision

[Self-Hosting-Group]

• Remove clean_ruleset_interval/threshold UI options as no function when compiled with IGDv2
• Hide dependent UI fields if UPnP IGD protocol is disabled
• Complete plugin wording/UI revision
  Add missing protocol term in status/overview UI and shorten menu option to UPnP IGD & PCP, otherwise two lines, rearrange table columns, improve wording and add help texts and note upstream change miniupnp/miniupnp@9339f0e
• Require client address and improve defaults in ACL form
• Simplify UI by moving rarely used options to advanced settings and the rest to general setup and rearrangement
• Manual adaption of added non-translatable protocol acronyms in translation files, see Integrity of translation source texts and changes. #7175

This completes PR #6863, merged in #6975.

Proposed service UI

Full screenshot

Proposed status/overview UI

Proposed advanced settings UI

Port Control Protocol (PCP) is the successor to NAT-PMP and has similar protocol concepts and packet formats, but adds IPv6 support. For more information, see Port Mapping Protocols Overview and Comparison 2024:
https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview

Maintainer: @jow-

[systemcrash]

So, what we typically find in the luci repo, in GUI, is that the first or second instance where space allows (and isn't in a heading) is to clarify acronyms and abbreviations. e.g. DNS = Domain Name System.

The best practice, in order to save inundating i18n peeps with needless html guff in text which can be error prone, is to use the .format() directive, and if the string is unclear when out of context, add a translator comment.

e.g. this is OK tho it's shit for translators:

			_('Dnsmasq is a lightweight <abbr title="Dynamic Host Configuration Protocol">DHCP</abbr> server and <abbr title="Domain Name System">DNS</abbr> forwarder.'));

but this is better so i18n peeps don't also need to copy and write html:

			_('Dnsmasq is a lightweight %s server and %s forwarder.').format('<abbr title="Dynamic Host Configuration Protocol">DHCP</abbr>', '<abbr title="Domain Name System">DNS</abbr>');

and this is even better so i18n peeps can understand what %s is. The second string in the _() is i18n explanation:

			_('Dnsmasq is a lightweight %s server and %s forwarder.', 'Dnsmasq is a lightweight %s (%s = DHCP) server and %s (%s = DNS) forwarder').format('<abbr title="Dynamic Host Configuration Protocol">DHCP</abbr>', '<abbr title="Domain Name System">DNS</abbr>');

[systemcrash]

If you must link to an acronym at a web resource, use format also to avoid link injection. Best is to link to RFCs which often clarify and explain the standard. Many acronyms ossify via English since that's what RFC are usually written in, and helps translators also.

[systemcrash]

see comments

[Self-Hosting-Group]

@systemcrash Thanks for the constructive/fast review and the suggestion to encode and link the acronyms. Linked the protocols to the standards on Wikipedia (hopefully also good), as the UPnP IGD is not from the IETF (RFC) and is only available as several separate PDFs.

For the clean_ruleset_* options, where the corresponding miniupnpd special function does not seem to work at all and therefore could not be analysed, it might be useful to wait and see if this is fixed or even removed upstream, see miniupnp/miniupnp#769. Also not sure how exactly clean_ruleset_threshold is defined when looking at the code and miniupnpd.conf. Earlier attempt at clarification: miniupnp/miniupnp#699.

Part of the advanced settings UI

[systemcrash]

.

[systemcrash]

*_threshold and *_interval are both made clear in the linked issue. What is the wording for *_threshold found there (that seems unclear)?

[systemcrash]

While it might be nice to track variable names in the miniupnpd, it's mandatory to follow the naming in its startup script. What version does openwrt use in master? Because if a setting name is not in master yet, we cannot change it here yet.

[Self-Hosting-Group]

While it might be nice to track variable names in the miniupnpd, it's mandatory to follow the naming in its startup script. What version does openwrt use in master? Because if a setting name is not in master yet, we cannot change it here yet.

The only necessary prerequisite for miniupnpd (the linked commit, included in 2.3.4) is already in the master since openwrt/packages@c9a1705, and all other configuration changes are implemented without additional prerequisites and include a uci-defaults migration.

[stangri]

@Self-Hosting-Group would it be possible to nest the .format calls to allow localization of text in things like: <abbr title="UPnP Internet Gateway Device (Control Protocol)">UPnP IGD</abbr></a>?

[systemcrash]

Nesting is an entirely reasonable use case. I've done similar but not for the purpose of i18n. Most of the abbreviations and acronyms are hardened around English in any case.

[Self-Hosting-Group]

These are the previously mentioned proposed changes to the package:

miniupnpd: Revise several upnpd UCI configuration options and defaults

• Remove clean_ruleset_interval/threshold UCI config options as not working
• Rename UCI config option enable_nat_pmp to enable_pcp_pmp as upstream, see miniupnp/miniupnp@02da705
• Allow third-party PCP (daemon/non-UCI) config option when secure_mode UCI config option is disabled
• Add (one-line) patch to use secure_mode UCI config also for UPnP IGD with IPv6, previously it was always enabled and the behaviour is undocumented. See miniupnp/miniupnp@c79e25a
• Convert download/upload UCI config option from KByte/s to kbit/s and rename to *_kbps, and update defaults to 100/50 Mbit/s (informational only)
• New/clearer UPnP IGD compatibility mode upnp_igd_compat UCI config option accepts igdv1/igdv2, replacing the current igdv1 boolean option, allowing future compatibility modes
• Rename and invert UCI config option secure_mode to allow_third_party_maps
• Better document and reformat default upnpd UCI config file and add (template) ACL entry for low ports (<1024) denied by default, current behaviour

Open questions

1. What do you think of the package proposal for easier and more predictable setup?
2. What do you think of the phrase UPnP IGD compatibility mode and the option name upnp_igd_compat or more clearly as upnp_igd_compat_mode or with compatiblity or other suggestions?
3. Now that we have a more complete secure_mode option, perhaps it is the right time to consider renaming/inverting it to allow_third_party_maps/Allow third-party port maps and adding a help text of e.g. Allow adding port maps for non-requesting IP addresses [and allow third-party PCP option], with the option disabled by default. This would be the more meaningful/security-by-default wording, the current secure mode has been enabled by default in OpenWrt for a long time (around 2018), and the non-specific secure mode does not say much, possibly with a hint to the former name.
4. Should the UI options for download/upload speed state in the title or in the help that it is the maximum or max. speed?
5. Some of the help texts in the advanced settings are currently long. Do you have any suggestions for shortening them so that they do not break into two lines at low resolutions?

[systemcrash]

At a minimum, if you want to make changes to the GUI, they must first exist in miniupnpd.init.

I'm going to close this PR since it includes new settings which don't exist there, but you're welcome to ping this PR when necessary changes are integrated there. Otherwise, change this PR to include GUI changes only. There are some acceptable changes here.

1. Open a PR to the packages repo (and therein...)
2. Include a newer version of the daemon (and possibly only do that).
3. Include new settings in the init script
4. Include any 'changes' with backwards compatible helpers. (possibly in an entirely separate PR)

Be aware of what this entails:

• a working build environment
• you verify that the new daemon compiles
• any new patches for it work, and old patches still apply and work
• the newer daemon and new settings have been tested and verified in a working openwrt environment.

Before undertaking this venture, I advise you to review previous packages repo PR to change variable names in the packages repo. Review other PRs that update daemons (there are tons of them both open and merged). If you try and do too much at once, your chances of success there fall drastically, will take a longer time, and have a higher chance of stalling and rotting.

[Self-Hosting-Group]

The idea was a linked PR in the package repository. And I have already prepared the PR for the package. I was just interested in feedback on the idea of the package update and feedback on the wording for the next step. There is also a prepared uci-defaults script.

[systemcrash]

Please submit the packages PR.

[Self-Hosting-Group]

@systemcrash Thank you for your constructive reviews. I am especially grateful for the much improved English wording. Since OpenWrt has the most comprehensive UI for autonomous port mapping compared to other router OSs, and since I am also updating the UI of some of the others, I plan to include the improved wording there later. Hence the questions above about perfecting the wording. I am very open to even the smallest text improvements and other suggestions from you, as I can see that you take the wording very seriously, and also see that you have extensive knowledge.

[systemcrash]

Was this your PR? openwrt/packages#24988 In which case there are a bunch of red flags there that need to be addressed for it to get approved.

I'll help with the wording, when the changes they're intended for go in. 👍

[systemcrash]

Here we took into account e.g. Chinese names which have no spaces (single name), and we see a few more of those PRs here than there. If you want to pass the checks, add some fake name like Joe Bloggs.

[Self-Hosting-Group]

@systemcrash I have reworked this PR, now without dependencies, because I would like to have the most important UI fixes/improvements in the upcoming OpenWrt 24.10 release before the OpenWrt package PR has a chance to be carefully reviewed (positive comment exists).

Is it possible for you to reopen and review this now independent PR first? The screenshots have been updated to reflect the simpler UI, and only a few minor changes will be needed later when it comes time for the package PR to be approved.

I still offer to incorporate the changes into the OpenWrt wiki (account already set up) for the 24.10 release, and I have already promised the translation corrections, but I would like to be able to finish this sooner rather than later if possible.

[systemcrash]

Push your current changes.

[systemcrash]

Let me try

[systemcrash]

OK

[systemcrash]

No, cannot reopen. Make a new PR I guess...