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: DNS API store all params #7510

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Dec 26, 2024

Fix #7504

@systemcrash
Copy link
Contributor

systemcrash commented Dec 26, 2024

This, in its present form, does not fix the bug. The data race is still present.

Test:

CF Key: 1
Save
Edit entry again. CF Key empty. Credentials empty.

@systemcrash
Copy link
Contributor

When handling credentials, you’ll need to pull apart the variable and value, to determine whether an incoming variable already exists, and if so, replace it.

@stokito
Copy link
Contributor Author

stokito commented Dec 29, 2024

The _parseKeyValueListToMap() does this

@systemcrash
Copy link
Contributor

Not quite. It checks to see whether an identical k+v exist in the set at add time, but not whether an identical k already exists with a different v. Such a check is necessary if a user changes their credentials for a provider.

@stokito
Copy link
Contributor Author

stokito commented Dec 29, 2024

it splits the K=V

let parts = paramKeyVal.split('=');
let name = parts[0];
let val = parts[1];

And saves to a map map.set(name, unquotedVal);

Then when any field was changed, we get the creds values credentialsDynList.getValue(), convert to a map, update the map :

if (newVal) {
	credsMap.set(optName, newVal);
} else {
	credsMap.delete(optName);
}

And and put the updated list back to the credentialsDynList:

credentialsDynList.setValue(newCreds);

@stokito stokito force-pushed the luci-app-acme_creds_fix branch 2 times, most recently from f0c3df4 to b715976 Compare December 29, 2024 16:58
Previously only one DNS API field was stored.
Get the current creds list, alter it and save.
Fix openwrt#7504

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@stokito stokito force-pushed the luci-app-acme_creds_fix branch from b715976 to 0aa3836 Compare December 29, 2024 17:01
@stokito
Copy link
Contributor Author

stokito commented Dec 29, 2024

Please merge it, I made another PR on top of the branch.

@systemcrash systemcrash merged commit f843014 into openwrt:master Dec 30, 2024
5 checks passed
@systemcrash
Copy link
Contributor

Merged. Good work!

@stokito stokito deleted the luci-app-acme_creds_fix branch December 30, 2024 07:16
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.

luci-app-acme: variables don't save using dns-api
2 participants