From 893071d2ba5b31e9a19080ffb3c5832524dfb851 Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Sun, 9 Mar 2025 21:31:06 +0100 Subject: [PATCH] refactor(api): extract method configMuxBuilder.postAdjustGui and add test coverage (#9979) 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. --- lib/api/api_test.go | 123 +++++++++++++++++++++++++++++++++++++++ lib/api/confighandler.go | 33 ++++++----- 2 files changed, 141 insertions(+), 15 deletions(-) diff --git a/lib/api/api_test.go b/lib/api/api_test.go index db5df4963..96ea1d1ce 100644 --- a/lib/api/api_test.go +++ b/lib/api/api_test.go @@ -751,6 +751,129 @@ func TestHTTPLogin(t *testing.T) { testWith(false, http.StatusOK, http.StatusOK, "/") testWith(false, http.StatusOK, http.StatusForbidden, "/meta.js") testWith(false, http.StatusNotFound, http.StatusForbidden, "/any-path/that/does/nooooooot/match-any/noauth-pattern") + + t.Run("Password change invalidates old and enables new password", func(t *testing.T) { + t.Parallel() + + // This test needs a longer-than-default shutdown timeout to finish saving + // config changes when running on GitHub Actions + shutdownTimeout := time.Second + + initConfig := func(password string, t *testing.T) config.Wrapper { + gui := config.GUIConfiguration{ + RawAddress: "127.0.0.1:0", + APIKey: testAPIKey, + User: "user", + } + if err := gui.SetPassword(password); err != nil { + t.Fatal(err, "Failed to set initial password") + } + cfg := config.Configuration{ + GUI: gui, + } + + tmpFile, err := os.CreateTemp("", "syncthing-testConfig-Password-*") + if err != nil { + t.Fatal(err, "Failed to create tmpfile for test") + } + w := config.Wrap(tmpFile.Name(), cfg, protocol.LocalDeviceID, events.NoopLogger) + tmpFile.Close() + cfgCtx, cfgCancel := context.WithCancel(context.Background()) + go w.Serve(cfgCtx) + t.Cleanup(func() { + os.Remove(tmpFile.Name()) + cfgCancel() + }) + return w + } + + initialPassword := "Asdf123!" + newPassword := "123!asdF" + + t.Run("when done via /rest/config", func(t *testing.T) { + t.Parallel() + + w := initConfig(initialPassword, t) + { + baseURL, cancel, err := startHTTPWithShutdownTimeout(w, shutdownTimeout) + cfgPath := baseURL + "/rest/config" + path := baseURL + "/meta.js" + t.Cleanup(cancel) + if err != nil { + t.Fatal(err) + } + + resp := httpGetBasicAuth(path, "user", initialPassword) + if resp.StatusCode != http.StatusOK { + t.Fatalf("Unexpected non-200 return code %d for auth with initial password", resp.StatusCode) + } + + cfg := w.RawCopy() + cfg.GUI.Password = newPassword + httpRequest(http.MethodPut, cfgPath, cfg, "", "", testAPIKey, "", "", "", nil, t) + } + { + baseURL, cancel, err := startHTTP(w) + path := baseURL + "/meta.js" + t.Cleanup(cancel) + if err != nil { + t.Fatal(err) + } + + resp := httpGetBasicAuth(path, "user", initialPassword) + if resp.StatusCode != http.StatusForbidden { + t.Errorf("Unexpected non-403 return code %d for request authed with old password", resp.StatusCode) + } + + resp2 := httpGetBasicAuth(path, "user", newPassword) + if resp2.StatusCode != http.StatusOK { + t.Errorf("Unexpected non-200 return code %d for request authed with new password", resp2.StatusCode) + } + } + }) + + t.Run("when done via /rest/config/gui", func(t *testing.T) { + t.Parallel() + + w := initConfig(initialPassword, t) + { + baseURL, cancel, err := startHTTPWithShutdownTimeout(w, shutdownTimeout) + cfgPath := baseURL + "/rest/config/gui" + path := baseURL + "/meta.js" + t.Cleanup(cancel) + if err != nil { + t.Fatal(err) + } + + resp := httpGetBasicAuth(path, "user", initialPassword) + if resp.StatusCode != http.StatusOK { + t.Fatalf("Unexpected non-200 return code %d for auth with initial password", resp.StatusCode) + } + + cfg := w.RawCopy() + cfg.GUI.Password = newPassword + httpRequest(http.MethodPut, cfgPath, cfg.GUI, "", "", testAPIKey, "", "", "", nil, t) + } + { + baseURL, cancel, err := startHTTP(w) + path := baseURL + "/meta.js" + t.Cleanup(cancel) + if err != nil { + t.Fatal(err) + } + + resp := httpGetBasicAuth(path, "user", initialPassword) + if resp.StatusCode != http.StatusForbidden { + t.Errorf("Unexpected non-403 return code %d for request authed with old password", resp.StatusCode) + } + + resp2 := httpGetBasicAuth(path, "user", newPassword) + if resp2.StatusCode != http.StatusOK { + t.Errorf("Unexpected non-200 return code %d for request authed with new password", resp2.StatusCode) + } + } + }) + }) } func TestHtmlFormLogin(t *testing.T) { diff --git a/lib/api/confighandler.go b/lib/api/confighandler.go index fc3e73a59..9ca6acc32 100644 --- a/lib/api/confighandler.go +++ b/lib/api/confighandler.go @@ -318,13 +318,10 @@ func (c *configMuxBuilder) adjustConfig(w http.ResponseWriter, r *http.Request) var errMsg string 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 { - l.Warnln("hashing password:", err) - errMsg = err.Error() - status = http.StatusInternalServerError - return - } + if err := c.postAdjustGui(&cfg.GUI, &to.GUI); err != nil { + errMsg = err.Error() + status = http.StatusInternalServerError + return } *cfg = to }) @@ -391,7 +388,6 @@ func (c *configMuxBuilder) adjustOptions(w http.ResponseWriter, r *http.Request, } func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui config.GUIConfiguration) { - oldPassword := gui.Password err := unmarshalTo(r.Body, &gui) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) @@ -400,13 +396,10 @@ func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui var errMsg string var status int waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { - if gui.Password != oldPassword { - if err := gui.SetPassword(gui.Password); err != nil { - l.Warnln("hashing password:", err) - errMsg = err.Error() - status = http.StatusInternalServerError - return - } + if err := c.postAdjustGui(&cfg.GUI, &gui); err != nil { + errMsg = err.Error() + status = http.StatusInternalServerError + return } cfg.GUI = gui }) @@ -419,6 +412,16 @@ func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui c.finish(w, waiter) } +func (c *configMuxBuilder) postAdjustGui(from *config.GUIConfiguration, to *config.GUIConfiguration) error { + if to.Password != from.Password { + if err := to.SetPassword(to.Password); err != nil { + l.Warnln("hashing password:", err) + return err + } + } + return nil +} + func (c *configMuxBuilder) adjustLDAP(w http.ResponseWriter, r *http.Request, ldap config.LDAPConfiguration) { if err := unmarshalTo(r.Body, &ldap); err != nil { http.Error(w, err.Error(), http.StatusBadRequest)