Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-app-acme: improve UI for inexperienced users #7147

Draft
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Jun 1, 2024

Maintainer: @tohojo

  1. A cleanup of translations.
  2. Autodetect of domain when adding
  3. Import domains from DDNS
  4. Logging tab

luci-app-acme screenshot

CC @hgl @stangri @systemcrash

stokito added 12 commits June 1, 2024 15:39
The dns_wait is supported by the acme but wasn't yet configurable on UI.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Use the %s placeholder instead of URLs.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
The introduction is confusing because the acme.sh use by default ZeroSSL.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
By default it's ZeroSSL.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Also change days placeholder to 60 which is a default for acme.sh

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
The article is "Get a free HTTPS certificate from LetsEncrypt for OpenWrt with ACME.sh"

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
A user must specify the validation_method first.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
We can't just use the datatype = "list(hostname)" because a domain may have a wildcard.
So check the domain by a simple regexp.
Check that DNS mode is used for wildcard.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
…S mode is used

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Check if the hostname is FQDN (e.g. has least one dot).
Check if the domain in the browser is not an IP and FQDN.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Many users already have a DDNS configured e.g. DuckDNS.org or Cloudflare.
We can import the configurations to simplify configurations and avoid mistakes.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@stokito stokito changed the title Luci app acme luci-app-acme: improve UI for inexperienced users Jun 1, 2024
We must simplify for a user to understand what happened.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
In the 585df1d I changed the validation_method to webroot by mistake because I thought this is a default in the acme.sh.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Copy link
Contributor

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally applaud the effort to make things more user friendly, but have some comments, see below.

Also a general one: You're adding new reads to the file system but not updating the acl config accordingly. So will things actually work correctly? How did you test all the permutations?

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Also use L.resolveDefault() to not fail if the ACL is not present.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Make the wildcard allowed only the beginning.
Add lowercase requirement.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@stokito stokito requested a review from tohojo June 3, 2024 11:20
@stokito
Copy link
Contributor Author

stokito commented Jun 3, 2024

Please squash before merging, I made many commits to just simplify the review.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@stokito
Copy link
Contributor Author

stokito commented Jun 7, 2024 via email

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Comment on lines +61 to +64
let ddnsDomainsList = [];
for (let ddnsDomain of ddnsDomains) {
ddnsDomainsList.push(ddnsDomain.domains[0]);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to clean it up, I'm fine with either tho.

let ddnsDomainsList = ddnsDomains.map(d => d.domains[0]);

@tohojo
Copy link
Contributor

tohojo commented Jun 10, 2024

Chrome console:
_isFqdn('0a.net')
true _isFqdn('0a')

Right, but:
_isFqdn("a0.net")
false

Maybe something like:

function _isIP(str) {
  return /^[0-9\.]+$/.test(str) || /[\[\]:]/.test(str);
}

function _isFqdn(str) {
  return !_isIP(str) && /\./.test(str);
}

@@ -648,7 +648,7 @@ function _collectDdnsDomains() {
if (credentials.length > 0) {
ddnsDomains.push({
sectionId: ddnsService['.name'],
domains: [ddnsService['domain']],
domains: [ddnsService['domain'], '*.' + ddnsService['domain']],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I don't think we should be encouraging people to randomly issue wildcard certs...

@@ -665,6 +657,45 @@ function _collectDdnsDomains() {
return ddnsDomains;
}

function _importDdns(ddnsDomains) {
alert(_('After import check the added domain certificate configurations.'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alert() is not a good way to send messages to the uses. Pretty sure luci has some way of adding messages, please use that instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this alert()

for (let d of ddnsDomain.domains) {
let dupDomainSection = certSectionDomains.get(d);
if (dupDomainSection) {
alert(_('The domain %s in DDNS %s already was configured in %s. Please check it after the import.').format(d, sectionId, dupDomainSection));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's already configured we should just skip it instead of bugging the user...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this alert()

@stokito
Copy link
Contributor Author

stokito commented Jun 10, 2024 via email

@stokito
Copy link
Contributor Author

stokito commented Jun 10, 2024 via email

@stokito
Copy link
Contributor Author

stokito commented Jun 10, 2024 via email

@tohojo
Copy link
Contributor

tohojo commented Jun 10, 2024 via email

@tohojo
Copy link
Contributor

tohojo commented Jun 10, 2024 via email

@systemcrash
Copy link
Contributor

systemcrash commented Jun 10, 2024 via email

@systemcrash systemcrash marked this pull request as draft June 24, 2024 12:21
@systemcrash
Copy link
Contributor

Please remove alert and sync i18n.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants