refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage #9979

emlun · 2025-03-01T23:47:50Z

(This is based on PR #9977 since it uses a test helper from there, and on PR #9980 because the test needs a longer shutdown timeout on GitHub Actions. This PR description excludes commits 1f01d7e and 4e21c53.)

This is extracted from PR #9175. This deduplicates SetPassword calls and makes postAdjustGui a single place where PR #9175 can add another adjustment step for sanitizing changes to WebAuthn credentials.

This also adds tests to validate that the refactored logic was not broken.

Purpose

Dedupliate logic in confighandler.go and make PR #9175 easier to review by reducing its diff volume.

Testing

Before this PR, the following patch still passes the tests:

diff --git a/lib/api/confighandler.go b/lib/api/confighandler.go
index fc3e73a59..29ac6bd23 100644
--- a/lib/api/confighandler.go
+++ b/lib/api/confighandler.go
@@ -319,7 +319,7 @@ func (c *configMuxBuilder) adjustConfig(w http.ResponseWriter, r *http.Request)
 	var status int
 	waiter, err := c.cfg.Modify(func(cfg *config.Configuration) {
 		if to.GUI.Password != cfg.GUI.Password {
-			if err := to.GUI.SetPassword(to.GUI.Password); err != nil {
+			if err := to.GUI.SetPassword(cfg.GUI.Password); err != nil {
 				l.Warnln("hashing password:", err)
 				errMsg = err.Error()
 				status = http.StatusInternalServerError
@@ -401,7 +401,7 @@ func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui
 	var status int
 	waiter, err := c.cfg.Modify(func(cfg *config.Configuration) {
 		if gui.Password != oldPassword {
-			if err := gui.SetPassword(gui.Password); err != nil {
+			if err := gui.SetPassword(oldPassword); err != nil {
 				l.Warnln("hashing password:", err)
 				errMsg = err.Error()
 				status = http.StatusInternalServerError

After this PR, the corresponding change fails both of the new tests.

These refactorizations were made in [PR syncthing#9175][1] to accommodate a few new variants of authentication method and body content. On request from reviewers, this PR extracts it as a smaller refactorization to review in isolation. This extracts a shared `httpRequest` base function from `httpGet` and `httpPost`, which will be used in PR syncthing#9175 for new helper functions `httpGetCsrf` (hiding all optional parameters except the CSRF token), `httpPostCsrf` (same) and `httpPostCsrfAuth` (hiding basic auth parameters). A `getSessionCookie` function is also extracted from `hasSessionCookie` and will be used to test that concurrent WebAuthn authentications result in separate sessions (indicated by different session cookies). [1]: syncthing#9175

Before this commit, the following patch still passes the tests: ```diff diff --git a/lib/api/confighandler.go b/lib/api/confighandler.go index fc3e73a..29ac6bd23 100644 --- a/lib/api/confighandler.go +++ b/lib/api/confighandler.go @@ -319,7 +319,7 @@ func (c *configMuxBuilder) adjustConfig(w http.ResponseWriter, r *http.Request) var status int waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { if to.GUI.Password != cfg.GUI.Password { - if err := to.GUI.SetPassword(to.GUI.Password); err != nil { + if err := to.GUI.SetPassword(cfg.GUI.Password); err != nil { l.Warnln("hashing password:", err) errMsg = err.Error() status = http.StatusInternalServerError @@ -401,7 +401,7 @@ func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui var status int waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { if gui.Password != oldPassword { - if err := gui.SetPassword(gui.Password); err != nil { + if err := gui.SetPassword(oldPassword); err != nil { l.Warnln("hashing password:", err) errMsg = err.Error() status = http.StatusInternalServerError ``` After this commit, each hunk of the above patch fails one of the new tests.

This is extracted from [PR syncthing#9175][1]. This deduplicates `SetPassword` calls and makes `postAdjustGui` a single place where PR syncthing#9175 can add another adjustment step for sanitizing changes to WebAuthn credentials. [1]: syncthing#9175

[PR syncthing#9175][1] adds some tests that seem to need a longer shutdown timeout when running on GitHub Actions. The problem manifests in errors like this in `TestWebauthnConfigChanges`: ``` 2024-05-05T17:00:45.0503602Z api_test.go:2919: TestWebauthnConfigChanges/Can_edit_GUIConfiguration.WebauthnUserId Put "http://127.0.0.1:37585/rest/config/gui": EOF [] 2024-05-05T17:00:45.0566336Z --- FAIL: TestWebauthnConfigChanges/Can_edit_GUIConfiguration.WebauthnUserId (0.52s) ``` indicating that the server was forcefully shut down (or panicked, but that was ruled out in these cases) before the request was fully written. [1]: syncthing#9175

emlun · 2025-03-07T10:22:39Z

As I had somewhat expected, the new test seems to have the timeout problem described in #9980 since it depends on updating config via the API and then verifying that behaviour changed as expected after the config-change restart. So this seems like it will need #9980 as a prerequisite; converting to draft until that is merged.

…adjust-gui

acolomb · 2025-03-07T11:58:33Z

Looks alright from my side. Let's have someone else take another look though. Especially regarding the PR title, we may want to focus more on the added test?

lib/api/api_test.go

As suggested in syncthing#9979 (comment)

* main: chore(gui, man, authors): update docs, translations, and contributors refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage (syncthing#9979)

* main: fix(syncthing): don't auto upgrade to higher major on startup (#9989) build(deps): update dependencies (#9988) chore(gui, man, authors): update docs, translations, and contributors refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage (#9979)

* main: (175 commits) build: move nightly build schedule to separate workflow (syncthing#10000) chore(syncthing): use file lock on certificate to prevent multiple instances (syncthing#10003) chore(ur): add RSS to reported stats (syncthing#10002) chore(gui, man, authors): update docs, translations, and contributors chore(gui, man, authors): update docs, translations, and contributors fix(api): prevent tilde expansion in path suggestions (fixes syncthing#9990) (syncthing#9992) fix(syncthing): don't auto upgrade to higher major on startup (syncthing#9989) build(deps): update dependencies (syncthing#9988) chore(gui, man, authors): update docs, translations, and contributors refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage (syncthing#9979) refactor(api): make shutdown timeout configurable for tests (syncthing#9980) refactor(api): deduplicate HTTP test helpers and allow session cookie access (syncthing#9977) build: correct API call for Weblate statistics build(deps): update dependencies (syncthing#9978) chore(etc): remove /usr/bin prefix from Linux .desktop files (syncthing#9966) build: use Go 1.24, minimum is Go 1.23 (syncthing#9960) fix(policy): do not require multiple maintainers for build changes chore(gui, man, authors): update docs, translations, and contributors chore(fs): build kqueue instead of fsevents watcher on iOS (syncthing#9950) build(deps): update dependencies (syncthing#9951) ...

…-is-disabled-for-Send-Only-folders * main: (266 commits) build: move nightly build schedule to separate workflow (syncthing#10000) chore(syncthing): use file lock on certificate to prevent multiple instances (syncthing#10003) chore(ur): add RSS to reported stats (syncthing#10002) chore(gui, man, authors): update docs, translations, and contributors chore(gui, man, authors): update docs, translations, and contributors fix(api): prevent tilde expansion in path suggestions (fixes syncthing#9990) (syncthing#9992) fix(syncthing): don't auto upgrade to higher major on startup (syncthing#9989) build(deps): update dependencies (syncthing#9988) chore(gui, man, authors): update docs, translations, and contributors refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage (syncthing#9979) refactor(api): make shutdown timeout configurable for tests (syncthing#9980) refactor(api): deduplicate HTTP test helpers and allow session cookie access (syncthing#9977) build: correct API call for Weblate statistics build(deps): update dependencies (syncthing#9978) chore(etc): remove /usr/bin prefix from Linux .desktop files (syncthing#9966) build: use Go 1.24, minimum is Go 1.23 (syncthing#9960) fix(policy): do not require multiple maintainers for build changes chore(gui, man, authors): update docs, translations, and contributors chore(fs): build kqueue instead of fsevents watcher on iOS (syncthing#9950) build(deps): update dependencies (syncthing#9951) ...

emlun added 6 commits March 1, 2025 21:47

Extract function postAdjustGui

e4673a2

This is extracted from [PR syncthing#9175][1]. This deduplicates `SetPassword` calls and makes `postAdjustGui` a single place where PR syncthing#9175 can add another adjustment step for sanitizing changes to WebAuthn credentials. [1]: syncthing#9175

fixup! Add test that password changes after PUTing new config

3aefa16

fixup! fixup! Add test that password changes after PUTing new config

7755f8a

acolomb approved these changes Mar 2, 2025

View reviewed changes

acolomb changed the title ~~Extract method configMuxBuilder.postAdjustGui and add test coverage~~ refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage Mar 2, 2025

rasa and others added 3 commits March 3, 2025 07:55

Merge branch 'main' into configurable-shutdown-timeout

9b28024

Merge branch 'main' into confighandler-post-adjust-gui

fda817d

Merge branch 'main' into confighandler-post-adjust-gui

3d255e4

emlun marked this pull request as draft March 7, 2025 10:22

emlun added 3 commits March 7, 2025 11:23

Merge branch 'main' into configurable-shutdown-timeout

03d140f

Merge branch 'configurable-shutdown-timeout' into confighandler-post-…

c9a172c

…adjust-gui

Use longer shutdown timeout for password change test in CI

6785596

emlun mentioned this pull request Mar 7, 2025

refactor(api): make shutdown timeout configurable for tests #9980

Merged

Merge branch 'main' into confighandler-post-adjust-gui

4ccbd77

emlun marked this pull request as ready for review March 7, 2025 12:01

calmh reviewed Mar 7, 2025

View reviewed changes

lib/api/api_test.go Outdated Show resolved Hide resolved

Use longer shutdown timeout not only on GitHub Actions

09ec194

emlun requested a review from calmh March 7, 2025 23:18

acolomb merged commit 893071d into syncthing:main Mar 9, 2025
24 checks passed

emlun deleted the confighandler-post-adjust-gui branch March 9, 2025 20:39

emlun added a commit to emlun/syncthing that referenced this pull request Mar 9, 2025

Use longer shutdown timeout not only on GitHub Actions

671283f

As suggested in syncthing#9979 (comment)

calmh modified the milestone: v1.29.3 Mar 18, 2025

calmh added this to the v1.29.4 milestone Mar 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage #9979

refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage #9979

Uh oh!

emlun commented Mar 1, 2025 •

edited

Loading

Uh oh!

emlun commented Mar 7, 2025

Uh oh!

acolomb commented Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage #9979

refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage #9979

Uh oh!

Conversation

emlun commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Testing

Uh oh!

emlun commented Mar 7, 2025

Uh oh!

acolomb commented Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emlun commented Mar 1, 2025 •

edited

Loading