Browse Source

Merge branch 'main' into v2

* main:
  fix(syncthing): ensure both config and data dirs exist at startup (fixes #10126) (#10127)
  fix(versioner): fix perms of created folders (fixes #9626) (#10105)
  refactor: use slices.Contains to simplify code (#10121)
Jakob Borg 4 months ago
parent
commit
39d6692109

+ 6 - 4
cmd/syncthing/main.go

@@ -295,10 +295,12 @@ func (c *serveCmd) Run() error {
 		}
 		}
 	}
 	}
 
 
-	// Ensure that our home directory exists.
-	if err := syncthing.EnsureDir(locations.GetBaseDir(locations.ConfigBaseDir), 0o700); err != nil {
-		l.Warnln("Failure on home directory:", err)
-		os.Exit(svcutil.ExitError.AsInt())
+	// Ensure that our config and data directories exist.
+	for _, loc := range []locations.BaseDirEnum{locations.ConfigBaseDir, locations.DataBaseDir} {
+		if err := syncthing.EnsureDir(locations.GetBaseDir(loc), 0o700); err != nil {
+			l.Warnln("Failed to ensure directory exists:", err)
+			os.Exit(svcutil.ExitError.AsInt())
+		}
 	}
 	}
 
 
 	if c.InternalInnerProcess {
 	if c.InternalInnerProcess {

+ 3 - 11
lib/model/folder_test.go

@@ -8,6 +8,7 @@ package model
 
 
 import (
 import (
 	"path/filepath"
 	"path/filepath"
+	"slices"
 	"testing"
 	"testing"
 
 
 	"github.com/d4l3k/messagediff"
 	"github.com/d4l3k/messagediff"
@@ -117,20 +118,11 @@ func unifySubsCases() []unifySubsCase {
 	return cases
 	return cases
 }
 }
 
 
-func unifyExists(f string, tc unifySubsCase) bool {
-	for _, e := range tc.exists {
-		if f == e {
-			return true
-		}
-	}
-	return false
-}
-
 func TestUnifySubs(t *testing.T) {
 func TestUnifySubs(t *testing.T) {
 	cases := unifySubsCases()
 	cases := unifySubsCases()
 	for i, tc := range cases {
 	for i, tc := range cases {
 		exists := func(f string) bool {
 		exists := func(f string) bool {
-			return unifyExists(f, tc)
+			return slices.Contains(tc.exists, f)
 		}
 		}
 		out := unifySubs(tc.in, exists)
 		out := unifySubs(tc.in, exists)
 		if diff, equal := messagediff.PrettyDiff(tc.out, out); !equal {
 		if diff, equal := messagediff.PrettyDiff(tc.out, out); !equal {
@@ -146,7 +138,7 @@ func BenchmarkUnifySubs(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 	for i := 0; i < b.N; i++ {
 		for _, tc := range cases {
 		for _, tc := range cases {
 			exists := func(f string) bool {
 			exists := func(f string) bool {
-				return unifyExists(f, tc)
+				return slices.Contains(tc.exists, f)
 			}
 			}
 			unifySubs(tc.in, exists)
 			unifySubs(tc.in, exists)
 		}
 		}

+ 3 - 5
lib/model/model.go

@@ -1817,11 +1817,9 @@ func (m *model) handleAutoAccepts(deviceID protocol.DeviceID, folder protocol.Fo
 		l.Infof("Failed to auto-accept folder %s from %s due to path conflict", folder.Description(), deviceID)
 		l.Infof("Failed to auto-accept folder %s from %s due to path conflict", folder.Description(), deviceID)
 		return config.FolderConfiguration{}, false
 		return config.FolderConfiguration{}, false
 	} else {
 	} else {
-		for _, device := range cfg.DeviceIDs() {
-			if device == deviceID {
-				// Already shared nothing todo.
-				return config.FolderConfiguration{}, false
-			}
+		if slices.Contains(cfg.DeviceIDs(), deviceID) {
+			// Already shared nothing todo.
+			return config.FolderConfiguration{}, false
 		}
 		}
 		if cfg.Type == config.FolderTypeReceiveEncrypted {
 		if cfg.Type == config.FolderTypeReceiveEncrypted {
 			if len(ccDeviceInfos.remote.EncryptionPasswordToken) == 0 && len(ccDeviceInfos.local.EncryptionPasswordToken) == 0 {
 			if len(ccDeviceInfos.remote.EncryptionPasswordToken) == 0 && len(ccDeviceInfos.local.EncryptionPasswordToken) == 0 {

+ 5 - 8
lib/model/model_test.go

@@ -18,6 +18,7 @@ import (
 	"os"
 	"os"
 	"path/filepath"
 	"path/filepath"
 	"runtime/pprof"
 	"runtime/pprof"
+	"slices"
 	"sort"
 	"sort"
 	"strconv"
 	"strconv"
 	"strings"
 	"strings"
@@ -1254,10 +1255,8 @@ func TestAutoAcceptPausedWhenFolderConfigChanged(t *testing.T) {
 	} else if fcfg.Path != idOther {
 	} else if fcfg.Path != idOther {
 		t.Error("folder path changed")
 		t.Error("folder path changed")
 	} else {
 	} else {
-		for _, dev := range fcfg.DeviceIDs() {
-			if dev == device1 {
-				return
-			}
+		if slices.Contains(fcfg.DeviceIDs(), device1) {
+			return
 		}
 		}
 		t.Error("device missing")
 		t.Error("device missing")
 	}
 	}
@@ -1303,10 +1302,8 @@ func TestAutoAcceptPausedWhenFolderConfigNotChanged(t *testing.T) {
 	} else if fcfg.Path != idOther {
 	} else if fcfg.Path != idOther {
 		t.Error("folder path changed")
 		t.Error("folder path changed")
 	} else {
 	} else {
-		for _, dev := range fcfg.DeviceIDs() {
-			if dev == device1 {
-				return
-			}
+		if slices.Contains(fcfg.DeviceIDs(), device1) {
+			return
 		}
 		}
 		t.Error("device missing")
 		t.Error("device missing")
 	}
 	}

+ 100 - 0
lib/versioner/simple_test.go

@@ -13,6 +13,7 @@ import (
 	"testing"
 	"testing"
 	"time"
 	"time"
 
 
+	"github.com/syncthing/syncthing/lib/build"
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/config"
 )
 )
 
 
@@ -156,3 +157,102 @@ func TestPathTildes(t *testing.T) {
 		t.Fatalf("found versioned file %q, want one that begins with %q", got, testPath)
 		t.Fatalf("found versioned file %q, want one that begins with %q", got, testPath)
 	}
 	}
 }
 }
+
+func TestArchiveFoldersCreationPermission(t *testing.T) {
+	if build.IsWindows {
+		t.Skip("Skipping on Windows")
+		return
+	}
+	dir := t.TempDir()
+	versionsDir := t.TempDir()
+
+	cfg := config.FolderConfiguration{
+		FilesystemType: config.FilesystemTypeBasic,
+		Path:           dir,
+		Versioning: config.VersioningConfiguration{
+			FSPath: versionsDir,
+			FSType: config.FilesystemTypeBasic,
+			Params: map[string]string{
+				"keep": "2",
+			},
+		},
+	}
+	vfs := cfg.Filesystem(nil)
+	v := newSimple(cfg)
+
+	// Create two folders and set their permissions
+	folder1Path := filepath.Join(dir, "folder1")
+	folder1Perms := os.FileMode(0o777)
+	folder1VersionsPath := filepath.Join(versionsDir, "folder1")
+	err := os.Mkdir(folder1Path, folder1Perms)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// chmod incase umask changes the create permissions
+	err = os.Chmod(folder1Path, folder1Perms)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	folder2Path := filepath.Join(folder1Path, "földer2")
+	folder2VersionsPath := filepath.Join(folder1VersionsPath, "földer2")
+	folder2Perms := os.FileMode(0o744)
+	err = os.Mkdir(folder2Path, folder2Perms)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// chmod incase umask changes the create permissions
+	err = os.Chmod(folder2Path, folder2Perms)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// create a file
+	filePath := filepath.Join("folder1", "földer2", "testFile")
+	f, err := vfs.Create(filePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	f.Close()
+
+	if err := v.Archive(filePath); err != nil {
+		t.Error(err)
+	}
+
+	// check permissions of the created version folders
+	folder1VersionsInfo, err := os.Stat(folder1VersionsPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if folder1VersionsInfo.Mode().Perm() != folder1Perms {
+		t.Errorf("folder1 permissions %v, want %v", folder1VersionsInfo.Mode(), folder1Perms)
+	}
+
+	folder2VersionsInfo, err := os.Stat(folder2VersionsPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if folder2VersionsInfo.Mode().Perm() != folder2Perms {
+		t.Errorf("földer2 permissions %v, want %v", folder2VersionsInfo.Mode(), folder2Perms)
+	}
+
+	// Archive again to test that archiving doesn't fail if the versioned folders already exist
+	if err := v.Archive(filePath); err != nil {
+		t.Error(err)
+	}
+	folder1VersionsInfo, err = os.Stat(folder1VersionsPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if folder1VersionsInfo.Mode().Perm() != folder1Perms {
+		t.Errorf("folder1 permissions %v, want %v", folder1VersionsInfo.Mode(), folder1Perms)
+	}
+
+	folder2VersionsInfo, err = os.Stat(folder2VersionsPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if folder2VersionsInfo.Mode().Perm() != folder2Perms {
+		t.Errorf("földer2 permissions %v, want %v", folder2VersionsInfo.Mode(), folder2Perms)
+	}
+}

+ 47 - 3
lib/versioner/util.go

@@ -10,6 +10,7 @@ import (
 	"context"
 	"context"
 	"errors"
 	"errors"
 	"fmt"
 	"fmt"
+	"os"
 	"path/filepath"
 	"path/filepath"
 	"regexp"
 	"regexp"
 	"sort"
 	"sort"
@@ -164,9 +165,8 @@ func archiveFile(method fs.CopyRangeMethod, srcFs, dstFs fs.Filesystem, filePath
 
 
 	file := filepath.Base(filePath)
 	file := filepath.Base(filePath)
 	inFolderPath := filepath.Dir(filePath)
 	inFolderPath := filepath.Dir(filePath)
-
-	err = dstFs.MkdirAll(inFolderPath, 0o755)
-	if err != nil && !fs.IsExist(err) {
+	err = dupDirTree(srcFs, dstFs, inFolderPath)
+	if err != nil {
 		l.Debugln("archiving", filePath, err)
 		l.Debugln("archiving", filePath, err)
 		return err
 		return err
 	}
 	}
@@ -190,6 +190,50 @@ func archiveFile(method fs.CopyRangeMethod, srcFs, dstFs fs.Filesystem, filePath
 	return err
 	return err
 }
 }
 
 
+func dupDirTree(srcFs, dstFs fs.Filesystem, folderPath string) error {
+	// Return early if the folder already exists.
+	_, err := dstFs.Stat(folderPath)
+	if err == nil || !fs.IsNotExist(err) {
+		return err
+	}
+	hadParent := true
+	for i := range folderPath {
+		if os.IsPathSeparator(folderPath[i]) {
+			// If the parent folder didn't exist, then this folder doesn't exist
+			// so we can skip the check
+			if hadParent {
+				_, err := dstFs.Stat(folderPath[:i])
+				if err == nil {
+					continue
+				}
+				if !fs.IsNotExist(err) {
+					return err
+				}
+			}
+			hadParent = false
+			err := dupDirWithPerms(srcFs, dstFs, folderPath[:i])
+			if err != nil {
+				return err
+			}
+		}
+	}
+	return dupDirWithPerms(srcFs, dstFs, folderPath)
+}
+
+func dupDirWithPerms(srcFs, dstFs fs.Filesystem, folderPath string) error {
+	srcStat, err := srcFs.Stat(folderPath)
+	if err != nil {
+		return err
+	}
+	// If we call Mkdir with srcStat.Mode(), we won't get the expected perms because of umask
+	// So, we create the folder with 0700, and then change the perms to the srcStat.Mode()
+	err = dstFs.Mkdir(folderPath, 0o700)
+	if err != nil {
+		return err
+	}
+	return dstFs.Chmod(folderPath, srcStat.Mode())
+}
+
 func restoreFile(method fs.CopyRangeMethod, src, dst fs.Filesystem, filePath string, versionTime time.Time, tagger fileTagger) error {
 func restoreFile(method fs.CopyRangeMethod, src, dst fs.Filesystem, filePath string, versionTime time.Time, tagger fileTagger) error {
 	tag := versionTime.In(time.Local).Truncate(time.Second).Format(TimeFormat)
 	tag := versionTime.In(time.Local).Truncate(time.Second).Format(TimeFormat)
 	taggedFilePath := tagger(filePath, tag)
 	taggedFilePath := tagger(filePath, tag)