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

11 files changed, 157 insertions, 81 deletions

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
-	}
-
-	// 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 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))
+
+		// 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
index a6fbbd15e..2eb2718d9 100644
--- a/lib/config/testdata/duplicates.xml
+++ b/lib/config/testdata/dupdevices.xml
@@ -15,15 +15,6 @@
         <!-- duplicate, will be removed -->
         <address>192.0.2.5</address>
     </device>
-    <folder id="f1" directory="testdata/">
-        <!-- duplicate, will be disabled -->
-        <device id="AIR6LPZ-7K4PTTV-UXQSMUU-CPQ5YWH-OEDFIIQ-JUG777G-2YQXXR5-YD6AWQR"></device>
-        <device id="GYRZZQBIRNPV4T7TC52WEQYJ3TFDQW6MWDFLMU4SSSU6EMFBK2VA"></device>
-    </folder>
-    <folder id="f1" directory="testdata/">
-        <!-- duplicate, will be disabled -->
-        <device id="AIR6LPZ-7K4PTTV-UXQSMUU-CPQ5YWH-OEDFIIQ-JUG777G-2YQXXR5-YD6AWQR"></device>
-    </folder>
     <folder id="f2" directory="testdata/">
         <device id="AIR6LPZ-7K4PTTV-UXQSMUU-CPQ5YWH-OEDFIIQ-JUG777G-2YQXXR5-YD6AWQR"></device>
         <device id="GYRZZQBIRNPV4T7TC52WEQYJ3TFDQW6MWDFLMU4SSSU6EMFBK2VA"></device>
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 @@
+<configuration version="15">
+    <folder id="f1" directory="testdata/">
+    </folder>
+    <folder id="f1" directory="testdata/">
+    </folder>
+</configuration>
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 @@
+<configuration version="15">
+    <folder id="f1">
+    </folder>
+</configuration>
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)
 		}
 	}