From dae5b1a571f0a300166f5763538646a8143c6ac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 31 Jul 2023 15:28:07 +0200 Subject: [PATCH] Fix #104237: fix issue with drive-only paths on Windows Fix an issue where a shared storage path on Linux, that maps via two-way variables to a drive root on Windows, caused problems with the path translation system. Windows paths that consist only of a drive letter (`F:`) cannot just be concatenated to a relative path, as that will result in `F:path\to\file`, which is still a relative path of sorts. This is now handled correctly, and should result in `F:\path\to\file`. This fixes #104237. --- internal/manager/api_impl/meta_test.go | 57 ++++++++++++++++++++++++++ internal/manager/config/variables.go | 17 +++++++- pkg/crosspath/crosspath.go | 29 +++++++++++++ pkg/crosspath/crosspath_test.go | 41 +++++++++++++++++- 4 files changed, 142 insertions(+), 2 deletions(-) diff --git a/internal/manager/api_impl/meta_test.go b/internal/manager/api_impl/meta_test.go index 2a1599ad..ab9f013e 100644 --- a/internal/manager/api_impl/meta_test.go +++ b/internal/manager/api_impl/meta_test.go @@ -143,6 +143,63 @@ func TestGetSharedStorage(t *testing.T) { } +// Test shared storage sitting on /mnt/flamenco, where that's mapped to F:\ for Windows. +func TestGetSharedStorageDriveLetterRoot(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mf := newMockedFlamenco(mockCtrl) + + conf := config.GetTestConfig(func(c *config.Conf) { + // Test with a Manager on Linux. + c.MockCurrentGOOSForTests("linux") + + // Set up a two-way variable to do the mapping. + c.Variables["shared_storage_mapping"] = config.Variable{ + IsTwoWay: true, + Values: []config.VariableValue{ + {Value: "/mnt/flamenco", Platform: config.VariablePlatformLinux, Audience: config.VariableAudienceAll}, + {Value: `F:\`, Platform: config.VariablePlatformWindows, Audience: config.VariableAudienceAll}, + }, + } + }) + mf.config.EXPECT().Get().Return(&conf).AnyTimes() + mf.config.EXPECT().EffectiveStoragePath().Return(`/mnt/flamenco`).AnyTimes() + + { // Test user client on Linux. + mf.config.EXPECT(). + NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformLinux). + DoAndReturn(conf.NewVariableExpander) + mf.shaman.EXPECT().IsEnabled().Return(false) + + echoCtx := mf.prepareMockedRequest(nil) + err := mf.flamenco.GetSharedStorage(echoCtx, api.ManagerVariableAudienceUsers, "linux") + require.NoError(t, err) + assertResponseJSON(t, echoCtx, http.StatusOK, api.SharedStorageLocation{ + Location: "/mnt/flamenco", + Audience: api.ManagerVariableAudienceUsers, + Platform: "linux", + }) + } + + { // Test user client on Windows. + mf.config.EXPECT(). + NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformWindows). + DoAndReturn(conf.NewVariableExpander) + mf.shaman.EXPECT().IsEnabled().Return(false) + + echoCtx := mf.prepareMockedRequest(nil) + err := mf.flamenco.GetSharedStorage(echoCtx, api.ManagerVariableAudienceUsers, "windows") + require.NoError(t, err) + assertResponseJSON(t, echoCtx, http.StatusOK, api.SharedStorageLocation{ + Location: `F:\`, + Audience: api.ManagerVariableAudienceUsers, + Platform: "windows", + }) + } + +} + func TestCheckSharedStoragePath(t *testing.T) { mf, finish := metaTestFixtures(t) defer finish() diff --git a/internal/manager/config/variables.go b/internal/manager/config/variables.go index f347c027..6cf78ec1 100644 --- a/internal/manager/config/variables.go +++ b/internal/manager/config/variables.go @@ -126,7 +126,7 @@ func (ve *VariableExpander) Expand(valueToExpand string) string { if !isValueMatch(expanded, managerValue) { continue } - expanded = targetValue + expanded[len(managerValue):] + expanded = ve.join(targetValue, expanded[len(managerValue):], ve.targetPlatform) // Since two-way variables are meant for path replacement, we know this // should be a path. @@ -136,5 +136,20 @@ func (ve *VariableExpander) Expand(valueToExpand string) string { if isPathValue { expanded = crosspath.ToPlatform(expanded, string(ve.targetPlatform)) } + return expanded } + +func (ve *VariableExpander) join(valueFromVariable, suffix string, platform VariablePlatform) string { + result := valueFromVariable + suffix + + if platform == VariablePlatformWindows { + // 'result' may now be of the form `F:some\path\to\file`, where `F:` comes + // from `valueFromVariable` and the rest is the suffix. This is not an + // absolute path, and needs a separator between the drive letter and the + // rest of the path. + return crosspath.EnsureDriveAbsolute(result) + } + + return result +} diff --git a/pkg/crosspath/crosspath.go b/pkg/crosspath/crosspath.go index 7203ef4a..d8337552 100644 --- a/pkg/crosspath/crosspath.go +++ b/pkg/crosspath/crosspath.go @@ -131,6 +131,10 @@ func ToPlatform(path, platform string) string { pathsep := pathSepForPlatform(platform) translated := strings.Join(components, pathsep) + if platform == "windows" { + return EnsureDriveAbsolute(translated) + } + return translated } @@ -162,3 +166,28 @@ func TrimTrailingSep(path string) string { } return trimmed } + +// EnsureDriveAbsolute ensures that a Windows path that starts with a drive +// letter also has an absolute path on that drive. For example, turns +// `F:path\to\file` into `F:\path\to\file`. +func EnsureDriveAbsolute(windowsPath string) string { + runes := []rune(windowsPath) + numRunes := len(runes) + if numRunes < 2 { + return windowsPath + } + + if !validDriveLetter(runes[0]) || runes[1] != ':' { + return windowsPath + } + + pathSep := pathSepForPlatform("windows") + if numRunes == 2 { + return windowsPath + pathSep + } + if string(runes[2]) == pathSep { + return windowsPath // Already F:\blabla + } + + return string(runes[:2]) + pathSep + string(runes[2:]) +} diff --git a/pkg/crosspath/crosspath_test.go b/pkg/crosspath/crosspath_test.go index 7ca992a2..b59a8253 100644 --- a/pkg/crosspath/crosspath_test.go +++ b/pkg/crosspath/crosspath_test.go @@ -185,6 +185,7 @@ func TestIsRoot(t *testing.T) { {"just letters", "just letters", false}, {"subdir of root", "/subdir", false}, {"subdir of drive", "C:\\subdir", false}, + {"relative subdir of drive", "C:subdir", false}, {"indirectly root", "/subdir/..", false}, {"UNC notation", `\\NAS\Share\`, false}, @@ -221,7 +222,14 @@ func TestToPlatform(t *testing.T) { {"mixed-lnx", args{`F:/mixed/path\to\file.blend`, "linux"}, `F:/mixed/path/to/file.blend`}, {"absolute-win", args{`F:/absolute/path`, "windows"}, `F:\absolute\path`}, {"absolute-lnx", args{`/absolute/path`, "linux"}, `/absolute/path`}, - {"drive-relative-win", args{`/absolute/path`, "windows"}, `\absolute\path`}, + {"relative-win", args{`/absolute/path`, "windows"}, `\absolute\path`}, + + // Trailing path separators should not be removed if it's only a drive + // letter, as concatenation rules are tricky there. `F:path` is not the same + // as `F:\path`. + {"drive-root-win", args{`F:\`, "windows"}, `F:\`}, + {"trailing-win", args{`F:\directory\`, "linux"}, `F:/directory`}, + {"trailing-lnx", args{`/dir/path/`, "windows"}, `\dir\path`}, // UNC notation should survive, even when it no longer makes sense (like on Linux). {"unc-win", args{`\\NAS\share\path`, "windows"}, `\\NAS\share\path`}, @@ -267,3 +275,34 @@ func TestTrimTrailingSep(t *testing.T) { }) } } + +func TestEnsureDriveAbsolute(t *testing.T) { + tests := []struct { + name string + inputPath string + want string + }{ + // Windows paths expected to change: + {"windows-drive-relative", `F:path\to\file`, `F:\path\to\file`}, + {"windows-drive-relative-mixed", "F:path/to/file", `F:\path/to/file`}, + {"windows-drive-only", "F:", `F:\`}, + + // No-op paths: + {"empty", "", ""}, + {"linux-root", "/", "/"}, + {"linux-path", "/some/path", "/some/path"}, + {"linux-unicode-path", "/söme/path", "/söme/path"}, + {"one-letter", "F", "F"}, + {"windows-drive-invalid", `©:path\to\thing`, `©:path\to\thing`}, + {"windows-unc", `\\NAS\Flamenco\path`, `\\NAS\Flamenco\path`}, + {"unicode-one-letter", "€", "€"}, + {"windows-drive-absolute", `F:\some\path`, `F:\some\path`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := EnsureDriveAbsolute(tt.inputPath); got != tt.want { + t.Errorf("EnsureDriveAbsolute() = %v, want %v", got, tt.want) + } + }) + } +}