From 6d357211b2809df2e021364843b36c195799fe41 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sat, 2 Jul 2016 19:38:39 +0000 Subject: [PATCH] lib/config: Remove "Invalid" attribute (fixes #2471) This contains the following behavioral changes: - Duplicate folder IDs is now fatal during startup - Invalid folder flags in the ClusterConfig is fatal for the connection (this will go away soon with the proto changes, as we won't have any unknown flags any more then) - Empty path is a folder error reported at runtime GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3370 --- cmd/syncthing/gui.go | 4 +- cmd/syncthing/gui_test.go | 53 +++++++++++++++++++ cmd/syncthing/main.go | 1 - lib/config/config.go | 39 ++++++++------ lib/config/config_test.go | 40 +++++++++----- lib/config/folderconfiguration.go | 40 +++++++------- .../{duplicates.xml => dupdevices.xml} | 9 ---- lib/config/testdata/dupfolders.xml | 6 +++ lib/config/testdata/nopath.xml | 4 ++ lib/model/model.go | 26 ++++----- lib/model/model_test.go | 14 ++--- 11 files changed, 156 insertions(+), 80 deletions(-) rename lib/config/testdata/{duplicates.xml => dupdevices.xml} (70%) create mode 100644 lib/config/testdata/dupfolders.xml create mode 100644 lib/config/testdata/nopath.xml diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index d6d6d4c3b..aac6511d7 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -577,7 +577,7 @@ func (s *apiService) getDBStatus(w http.ResponseWriter, r *http.Request) { func folderSummary(cfg configIntf, m modelIntf, folder string) map[string]interface{} { var res = make(map[string]interface{}) - res["invalid"] = cfg.Folders()[folder].Invalid + res["invalid"] = "" // Deprecated, retains external API for now globalFiles, globalDeleted, globalBytes := m.GlobalSize(folder) res["globalFiles"], res["globalDeleted"], res["globalBytes"] = globalFiles, globalDeleted, globalBytes @@ -690,7 +690,7 @@ func (s *apiService) postSystemConfig(w http.ResponseWriter, r *http.Request) { to, err := config.ReadJSON(r.Body, myID) r.Body.Close() if err != nil { - l.Warnln("decoding posted config:", err) + l.Warnln("Decoding posted config:", err) http.Error(w, err.Error(), http.StatusBadRequest) return } diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index 4dbf76497..bdbf58a16 100644 --- a/cmd/syncthing/gui_test.go +++ b/cmd/syncthing/gui_test.go @@ -11,6 +11,7 @@ import ( "compress/gzip" "encoding/json" "fmt" + "io" "io/ioutil" "net" "net/http" @@ -613,3 +614,55 @@ func TestRandomString(t *testing.T) { t.Errorf("Expected 27 random characters, got %q of length %d", res["random"], len(res["random"])) } } + +func TestConfigPostOK(t *testing.T) { + cfg := bytes.NewBuffer([]byte(`{ + "version": 15, + "folders": [ + {"id": "foo"} + ] + }`)) + + resp, err := testConfigPost(cfg) + if err != nil { + t.Fatal(err) + } + if resp.StatusCode != http.StatusOK { + t.Error("Expected 200 OK, not", resp.Status) + } +} + +func TestConfigPostDupFolder(t *testing.T) { + cfg := bytes.NewBuffer([]byte(`{ + "version": 15, + "folders": [ + {"id": "foo"}, + {"id": "foo"} + ] + }`)) + + resp, err := testConfigPost(cfg) + if err != nil { + t.Fatal(err) + } + if resp.StatusCode != http.StatusBadRequest { + t.Error("Expected 400 Bad Request, not", resp.Status) + } +} + +func testConfigPost(data io.Reader) (*http.Response, error) { + const testAPIKey = "foobarbaz" + cfg := new(mockedConfig) + cfg.gui.APIKey = testAPIKey + baseURL, err := startHTTP(cfg) + if err != nil { + return nil, err + } + cli := &http.Client{ + Timeout: time.Second, + } + + req, _ := http.NewRequest("POST", baseURL+"/rest/system/config", data) + req.Header.Set("X-API-Key", testAPIKey) + return cli.Do(req) +} diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 794fa2399..6130bc28a 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -863,7 +863,6 @@ func loadConfig() (*config.Wrapper, error) { cfg, err := config.Load(cfgFile, myID) if err != nil { - l.Infoln("Error loading config file; using defaults for now") myName, _ := os.Hostname() newCfg := defaultConfig(myName) cfg = config.Wrap(cfgFile, newCfg) diff --git a/lib/config/config.go b/lib/config/config.go index aa8c12794..97e72c93a 100644 --- a/lib/config/config.go +++ b/lib/config/config.go @@ -10,6 +10,7 @@ package config import ( "encoding/json" "encoding/xml" + "fmt" "io" "io/ioutil" "net/url" @@ -81,11 +82,15 @@ func ReadXML(r io.Reader, myID protocol.DeviceID) (Configuration, error) { util.SetDefaults(&cfg.Options) util.SetDefaults(&cfg.GUI) - err := xml.NewDecoder(r).Decode(&cfg) + if err := xml.NewDecoder(r).Decode(&cfg); err != nil { + return Configuration{}, err + } cfg.OriginalVersion = cfg.Version - cfg.prepare(myID) - return cfg, err + if err := cfg.prepare(myID); err != nil { + return Configuration{}, err + } + return cfg, nil } func ReadJSON(r io.Reader, myID protocol.DeviceID) (Configuration, error) { @@ -97,14 +102,16 @@ func ReadJSON(r io.Reader, myID protocol.DeviceID) (Configuration, error) { bs, err := ioutil.ReadAll(r) if err != nil { - return cfg, err + return Configuration{}, err } err = json.Unmarshal(bs, &cfg) cfg.OriginalVersion = cfg.Version - cfg.prepare(myID) - return cfg, err + if err := cfg.prepare(myID); err != nil { + return Configuration{}, err + } + return cfg, nil } type Configuration struct { @@ -154,7 +161,7 @@ func (cfg *Configuration) WriteXML(w io.Writer) error { return err } -func (cfg *Configuration) prepare(myID protocol.DeviceID) { +func (cfg *Configuration) prepare(myID protocol.DeviceID) error { util.FillNilSlices(&cfg.Options) // Initialize any empty slices @@ -168,19 +175,19 @@ func (cfg *Configuration) prepare(myID protocol.DeviceID) { cfg.Options.AlwaysLocalNets = []string{} } - // Check for missing, bad or duplicate folder ID:s - var seenFolders = map[string]*FolderConfiguration{} + // Prepare folders and check for duplicates. Duplicates are bad and + // dangerous, can't currently be resolved in the GUI, and shouldn't + // happen when configured by the GUI. We return with an error in that + // situation. + seenFolders := make(map[string]struct{}) for i := range cfg.Folders { folder := &cfg.Folders[i] folder.prepare() - if seen, ok := seenFolders[folder.ID]; ok { - l.Warnf("Multiple folders with ID %q; disabling", folder.ID) - seen.Invalid = "duplicate folder ID" - folder.Invalid = "duplicate folder ID" - } else { - seenFolders[folder.ID] = folder + if _, ok := seenFolders[folder.ID]; ok { + return fmt.Errorf("duplicate folder ID %q in configuration", folder.ID) } + seenFolders[folder.ID] = struct{}{} } cfg.Options.ListenAddresses = util.UniqueStrings(cfg.Options.ListenAddresses) @@ -257,6 +264,8 @@ func (cfg *Configuration) prepare(myID protocol.DeviceID) { if cfg.GUI.APIKey == "" { cfg.GUI.APIKey = rand.String(32) } + + return nil } func convertV14V15(cfg *Configuration) { diff --git a/lib/config/config_test.go b/lib/config/config_test.go index f2288ee37..a4802ca44 100644 --- a/lib/config/config_test.go +++ b/lib/config/config_test.go @@ -595,22 +595,14 @@ func TestGUIConfigURL(t *testing.T) { } } -func TestRemoveDuplicateDevicesFolders(t *testing.T) { - wrapper, err := Load("testdata/duplicates.xml", device1) +func TestDuplicateDevices(t *testing.T) { + // Duplicate devices should be removed + + wrapper, err := Load("testdata/dupdevices.xml", device1) if err != nil { t.Fatal(err) } - // All folders are loaded, but the duplicate ones are disabled. - if l := len(wrapper.Raw().Folders); l != 3 { - t.Errorf("Incorrect number of folders, %d != 3", l) - } - for i, f := range wrapper.Raw().Folders { - if f.ID == "f1" && f.Invalid == "" { - t.Errorf("Folder %d (%q) is not set invalid", i, f.ID) - } - } - if l := len(wrapper.Raw().Devices); l != 3 { t.Errorf("Incorrect number of devices, %d != 3", l) } @@ -621,6 +613,30 @@ func TestRemoveDuplicateDevicesFolders(t *testing.T) { } } +func TestDuplicateFolders(t *testing.T) { + // Duplicate folders are a loading error + + _, err := Load("testdata/dupfolders.xml", device1) + if err == nil || !strings.HasPrefix(err.Error(), "duplicate folder ID") { + t.Fatal(`Expected error to mention "duplicate folder ID":`, err) + } +} + +func TestEmptyFolderPaths(t *testing.T) { + // Empty folder paths are allowed at the loading stage, and should not + // get messed up by the prepare steps (e.g., become the current dir or + // get a slash added so that it becomes the root directory or similar). + + wrapper, err := Load("testdata/nopath.xml", device1) + if err != nil { + t.Fatal(err) + } + folder := wrapper.Folders()["f1"] + if folder.Path() != "" { + t.Errorf("Expected %q to be empty", folder.Path()) + } +} + func TestV14ListenAddressesMigration(t *testing.T) { tcs := [][3][]string{ diff --git a/lib/config/folderconfiguration.go b/lib/config/folderconfiguration.go index 5782d0ed6..15c426fd9 100644 --- a/lib/config/folderconfiguration.go +++ b/lib/config/folderconfiguration.go @@ -39,7 +39,6 @@ type FolderConfiguration struct { DisableSparseFiles bool `xml:"disableSparseFiles" json:"disableSparseFiles"` DisableTempIndexes bool `xml:"disableTempIndexes" json:"disableTempIndexes"` - Invalid string `xml:"-" json:"invalid"` // Set at runtime when there is an error, not saved cachedPath string DeprecatedReadOnly bool `xml:"ro,attr,omitempty" json:"-"` @@ -70,7 +69,7 @@ func (f FolderConfiguration) Path() string { // This is intentionally not a pointer method, because things like // cfg.Folders["default"].Path() should be valid. - if f.cachedPath == "" { + if f.cachedPath == "" && f.RawPath != "" { l.Infoln("bug: uncached path call (should only happen in tests)") return f.cleanedPath() } @@ -108,31 +107,24 @@ func (f *FolderConfiguration) DeviceIDs() []protocol.DeviceID { } func (f *FolderConfiguration) prepare() { - if len(f.RawPath) == 0 { - f.Invalid = "no directory configured" - return - } + if f.RawPath != "" { + // The reason it's done like this: + // C: -> C:\ -> C:\ (issue that this is trying to fix) + // C:\somedir -> C:\somedir\ -> C:\somedir + // C:\somedir\ -> C:\somedir\\ -> C:\somedir + // This way in the tests, we get away without OS specific separators + // in the test configs. + f.RawPath = filepath.Dir(f.RawPath + string(filepath.Separator)) - // The reason it's done like this: - // C: -> C:\ -> C:\ (issue that this is trying to fix) - // C:\somedir -> C:\somedir\ -> C:\somedir - // C:\somedir\ -> C:\somedir\\ -> C:\somedir - // This way in the tests, we get away without OS specific separators - // in the test configs. - f.RawPath = filepath.Dir(f.RawPath + string(filepath.Separator)) - - // If we're not on Windows, we want the path to end with a slash to - // penetrate symlinks. On Windows, paths must not end with a slash. - if runtime.GOOS != "windows" && f.RawPath[len(f.RawPath)-1] != filepath.Separator { - f.RawPath = f.RawPath + string(filepath.Separator) + // If we're not on Windows, we want the path to end with a slash to + // penetrate symlinks. On Windows, paths must not end with a slash. + if runtime.GOOS != "windows" && f.RawPath[len(f.RawPath)-1] != filepath.Separator { + f.RawPath = f.RawPath + string(filepath.Separator) + } } f.cachedPath = f.cleanedPath() - if f.ID == "" { - f.ID = "default" - } - if f.RescanIntervalS > MaxRescanIntervalS { f.RescanIntervalS = MaxRescanIntervalS } else if f.RescanIntervalS < 0 { @@ -145,6 +137,10 @@ func (f *FolderConfiguration) prepare() { } func (f *FolderConfiguration) cleanedPath() string { + if f.RawPath == "" { + return "" + } + cleaned := f.RawPath // Attempt tilde expansion; leave unchanged in case of error diff --git a/lib/config/testdata/duplicates.xml b/lib/config/testdata/dupdevices.xml similarity index 70% rename from lib/config/testdata/duplicates.xml rename to lib/config/testdata/dupdevices.xml index a6fbbd15e..2eb2718d9 100644 --- a/lib/config/testdata/duplicates.xml +++ b/lib/config/testdata/dupdevices.xml @@ -15,15 +15,6 @@
192.0.2.5
- - - - - - - - - diff --git a/lib/config/testdata/dupfolders.xml b/lib/config/testdata/dupfolders.xml new file mode 100644 index 000000000..cc2cc39c7 --- /dev/null +++ b/lib/config/testdata/dupfolders.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/lib/config/testdata/nopath.xml b/lib/config/testdata/nopath.xml new file mode 100644 index 000000000..706f93f27 --- /dev/null +++ b/lib/config/testdata/nopath.xml @@ -0,0 +1,4 @@ + + + + diff --git a/lib/model/model.go b/lib/model/model.go index 0e055514c..d4e874066 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -110,6 +110,7 @@ var ( // errors returned by the CheckFolderHealth method var ( + errFolderPathEmpty = errors.New("folder path empty") errFolderPathMissing = errors.New("folder path missing") errFolderMarkerMissing = errors.New("folder marker missing") errHomeDiskNoSpace = errors.New("home disk has insufficient free space") @@ -658,23 +659,19 @@ func (m *Model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterCon tempIndexFolders := make([]string, 0, len(cm.Folders)) - m.fmut.Lock() -nextFolder: for _, folder := range cm.Folders { - cfg := m.folderCfgs[folder.ID] - if folder.Flags&^protocol.FlagFolderAll != 0 { - // There are flags set that we don't know what they mean. Scary! + // There are flags set that we don't know what they mean. Fatal! l.Warnf("Device %v: unknown flags for folder %s", deviceID, folder.ID) - cfg.Invalid = fmt.Sprintf("Unknown flags from device %v", deviceID) - m.cfg.SetFolder(cfg) - if srv := m.folderRunners[folder.ID]; srv != nil { - srv.setError(fmt.Errorf(cfg.Invalid)) - } - continue nextFolder + m.fmut.Unlock() + m.Close(deviceID, fmt.Errorf("Unknown folder flags from device %v", deviceID)) + return } - if !m.folderSharedWithUnlocked(folder.ID, deviceID) { + m.fmut.Lock() + shared := m.folderSharedWithUnlocked(folder.ID, deviceID) + m.fmut.Unlock() + if !shared { events.Default.Log(events.FolderRejected, map[string]string{ "folder": folder.ID, "folderLabel": folder.Label, @@ -687,7 +684,6 @@ nextFolder: tempIndexFolders = append(tempIndexFolders, folder.ID) } } - m.fmut.Unlock() // This breaks if we send multiple CM messages during the same connection. if len(tempIndexFolders) > 0 { @@ -1953,6 +1949,10 @@ func (m *Model) CheckFolderHealth(id string) error { // checkFolderPath returns nil if the folder path exists and has the marker file. func (m *Model) checkFolderPath(folder config.FolderConfiguration) error { + if folder.Path() == "" { + return errFolderPathEmpty + } + if fi, err := os.Stat(folder.Path()); err != nil || !fi.IsDir() { return errFolderPathMissing } diff --git a/lib/model/model_test.go b/lib/model/model_test.go index 7e3356035..baab865b1 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -642,9 +642,6 @@ func TestROScanRecovery(t *testing.T) { waitFor := func(status string) error { timeout := time.Now().Add(2 * time.Second) for { - if time.Now().After(timeout) { - return fmt.Errorf("Timed out waiting for status: %s, current status: %s", status, m.cfg.Folders()["default"].Invalid) - } _, _, err := m.State("default") if err == nil && status == "" { return nil @@ -652,6 +649,10 @@ func TestROScanRecovery(t *testing.T) { if err != nil && err.Error() == status { return nil } + + if time.Now().After(timeout) { + return fmt.Errorf("Timed out waiting for status: %s, current status: %v", status, err) + } time.Sleep(10 * time.Millisecond) } } @@ -727,9 +728,6 @@ func TestRWScanRecovery(t *testing.T) { waitFor := func(status string) error { timeout := time.Now().Add(2 * time.Second) for { - if time.Now().After(timeout) { - return fmt.Errorf("Timed out waiting for status: %s, current status: %s", status, m.cfg.Folders()["default"].Invalid) - } _, _, err := m.State("default") if err == nil && status == "" { return nil @@ -737,6 +735,10 @@ func TestRWScanRecovery(t *testing.T) { if err != nil && err.Error() == status { return nil } + + if time.Now().After(timeout) { + return fmt.Errorf("Timed out waiting for status: %s, current status: %v", status, err) + } time.Sleep(10 * time.Millisecond) } }