From 6d981c25a472b371c407dc6b3dd46ad3df4ea8f4 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 2 Jan 2025 18:16:08 +0100 Subject: [PATCH 1/2] Set the write bit for files written during template initialization This used to work because the permission bits for builtin templates were hardcoded to 0644 for files and 0755 for directories. As of #1912 (and the PRs it depends on), builtin templates are no longer pre-materialized to a temporary directory and read directly from the embedded filesystem. It turns out that this builtin filesystem returns 0444 as the permission bits for the files it contains. These bits are carried over to the destination filesystem. This change updates template materialization to always set the owner's write bit. It doesn't really make sense to write read-only files and expect users to work with these files in a VCS (note: Git only stores the executable bit). The regression shipped as part of v0.235.0 and will be fixed as of v0.238.0. --- internal/testutil/file.go | 29 +++++++++++++++++++++++++++++ libs/template/file_test.go | 9 +++++---- libs/template/renderer.go | 4 ++++ libs/template/renderer_test.go | 28 ++++++++++------------------ 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/internal/testutil/file.go b/internal/testutil/file.go index 538a3c20ad..476c4123a3 100644 --- a/internal/testutil/file.go +++ b/internal/testutil/file.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -52,3 +53,31 @@ func ReadFile(t TestingT, path string) string { return string(b) } + +// StatFile returns the file info for a file. +func StatFile(t TestingT, path string) os.FileInfo { + fi, err := os.Stat(path) + require.NoError(t, err) + + return fi +} + +// AssertFileContents asserts that the file at path has the expected content. +func AssertFileContents(t TestingT, path, expected string) bool { + actual := ReadFile(t, path) + return assert.Equal(t, expected, actual) +} + +// AssertFilePermissions asserts that the file at path has the expected permissions. +func AssertFilePermissions(t TestingT, path string, expected os.FileMode) bool { + fi := StatFile(t, path) + assert.False(t, fi.Mode().IsDir(), "expected a file, got a directory") + return assert.Equal(t, expected, fi.Mode().Perm(), "expected 0%o, got 0%o", expected, fi.Mode().Perm()) +} + +// AssertDirPermissions asserts that the file at path has the expected permissions. +func AssertDirPermissions(t TestingT, path string, expected os.FileMode) bool { + fi := StatFile(t, path) + assert.True(t, fi.Mode().IsDir(), "expected a directory, got a file") + return assert.Equal(t, expected, fi.Mode().Perm(), "expected 0%o, got 0%o", expected, fi.Mode().Perm()) +} diff --git a/libs/template/file_test.go b/libs/template/file_test.go index ced38c2848..f4bf5652c8 100644 --- a/libs/template/file_test.go +++ b/libs/template/file_test.go @@ -8,6 +8,7 @@ import ( "runtime" "testing" + "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/filer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -27,8 +28,8 @@ func testInMemoryFile(t *testing.T, ctx context.Context, perm fs.FileMode) { err = f.Write(ctx, out) assert.NoError(t, err) - assertFileContent(t, filepath.Join(tmpDir, "a/b/c"), "123") - assertFilePermissions(t, filepath.Join(tmpDir, "a/b/c"), perm) + testutil.AssertFileContents(t, filepath.Join(tmpDir, "a/b/c"), "123") + testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "a/b/c"), perm) } func testCopyFile(t *testing.T, ctx context.Context, perm fs.FileMode) { @@ -48,8 +49,8 @@ func testCopyFile(t *testing.T, ctx context.Context, perm fs.FileMode) { err = f.Write(ctx, out) assert.NoError(t, err) - assertFileContent(t, filepath.Join(tmpDir, "a/b/c"), "qwerty") - assertFilePermissions(t, filepath.Join(tmpDir, "a/b/c"), perm) + testutil.AssertFileContents(t, filepath.Join(tmpDir, "source"), "qwerty") + testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "source"), perm) } func TestTemplateInMemoryFilePersistToDisk(t *testing.T) { diff --git a/libs/template/renderer.go b/libs/template/renderer.go index 5030cd9df3..679b7d8b79 100644 --- a/libs/template/renderer.go +++ b/libs/template/renderer.go @@ -150,6 +150,10 @@ func (r *renderer) computeFile(relPathTemplate string) (file, error) { } perm := info.Mode().Perm() + // Always include the write bit for the owner of the file. + // It does not make sense to have a file that is not writable by the owner. + perm |= 0o200 + // Execute relative path template to get destination path for the file relPath, err := r.executeTemplate(relPathTemplate) if err != nil { diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 2c14009ff3..39c85c178a 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -27,18 +27,6 @@ import ( "github.com/stretchr/testify/require" ) -func assertFileContent(t *testing.T, path, content string) { - b, err := os.ReadFile(path) - require.NoError(t, err) - assert.Equal(t, content, string(b)) -} - -func assertFilePermissions(t *testing.T, path string, perm fs.FileMode) { - info, err := os.Stat(path) - require.NoError(t, err) - assert.Equal(t, perm, info.Mode().Perm()) -} - func assertBuiltinTemplateValid(t *testing.T, template string, settings map[string]any, target string, isServicePrincipal, build bool, tempDir string) { ctx := context.Background() @@ -69,6 +57,10 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri err = renderer.persistToDisk(ctx, out) require.NoError(t, err) + // Verify permissions on file and directory + testutil.AssertFilePermissions(t, filepath.Join(tempDir, "my_project/README.md"), fs.FileMode(0o644)) + testutil.AssertDirPermissions(t, filepath.Join(tempDir, "my_project/resources"), fs.FileMode(0o755)) + b, err := bundle.Load(ctx, filepath.Join(tempDir, "my_project")) require.NoError(t, err) diags := bundle.Apply(ctx, b, phases.LoadNamedTarget(target)) @@ -347,10 +339,10 @@ func TestRendererPersistToDisk(t *testing.T) { assert.NoFileExists(t, filepath.Join(tmpDir, "a", "b", "c")) assert.NoFileExists(t, filepath.Join(tmpDir, "mno")) - assertFileContent(t, filepath.Join(tmpDir, "a", "b", "d"), "123") - assertFilePermissions(t, filepath.Join(tmpDir, "a", "b", "d"), 0o444) - assertFileContent(t, filepath.Join(tmpDir, "mmnn"), "456") - assertFilePermissions(t, filepath.Join(tmpDir, "mmnn"), 0o444) + testutil.AssertFileContents(t, filepath.Join(tmpDir, "a/b/d"), "123") + testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "a/b/d"), fs.FileMode(0o444)) + testutil.AssertFileContents(t, filepath.Join(tmpDir, "mmnn"), "456") + testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "mmnn"), fs.FileMode(0o444)) } func TestRendererWalk(t *testing.T) { @@ -617,8 +609,8 @@ func TestRendererFileTreeRendering(t *testing.T) { require.NoError(t, err) // Assert files and directories are correctly materialized. - assert.DirExists(t, filepath.Join(tmpDir, "my_directory")) - assert.FileExists(t, filepath.Join(tmpDir, "my_directory", "my_file")) + testutil.AssertDirPermissions(t, filepath.Join(tmpDir, "my_directory"), fs.FileMode(0o755)) + testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "my_directory", "my_file"), fs.FileMode(0o644)) } func TestRendererSubTemplateInPath(t *testing.T) { From 663bb0ac95d6fafc6446f4aa7d16a11808ad15ef Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 8 Jan 2025 13:57:47 +0100 Subject: [PATCH 2/2] Initialize default permission bits by operating system --- libs/template/renderer_test.go | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 39c85c178a..eb208ca7b4 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -27,6 +27,21 @@ import ( "github.com/stretchr/testify/require" ) +var ( + defaultFilePermissions fs.FileMode + defaultDirPermissions fs.FileMode +) + +func init() { + if runtime.GOOS == "windows" { + defaultFilePermissions = fs.FileMode(0o666) + defaultDirPermissions = fs.FileMode(0o777) + } else { + defaultFilePermissions = fs.FileMode(0o644) + defaultDirPermissions = fs.FileMode(0o755) + } +} + func assertBuiltinTemplateValid(t *testing.T, template string, settings map[string]any, target string, isServicePrincipal, build bool, tempDir string) { ctx := context.Background() @@ -58,8 +73,8 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri require.NoError(t, err) // Verify permissions on file and directory - testutil.AssertFilePermissions(t, filepath.Join(tempDir, "my_project/README.md"), fs.FileMode(0o644)) - testutil.AssertDirPermissions(t, filepath.Join(tempDir, "my_project/resources"), fs.FileMode(0o755)) + testutil.AssertFilePermissions(t, filepath.Join(tempDir, "my_project/README.md"), defaultFilePermissions) + testutil.AssertDirPermissions(t, filepath.Join(tempDir, "my_project/resources"), defaultDirPermissions) b, err := bundle.Load(ctx, filepath.Join(tempDir, "my_project")) require.NoError(t, err) @@ -609,8 +624,8 @@ func TestRendererFileTreeRendering(t *testing.T) { require.NoError(t, err) // Assert files and directories are correctly materialized. - testutil.AssertDirPermissions(t, filepath.Join(tmpDir, "my_directory"), fs.FileMode(0o755)) - testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "my_directory", "my_file"), fs.FileMode(0o644)) + testutil.AssertDirPermissions(t, filepath.Join(tmpDir, "my_directory"), defaultDirPermissions) + testutil.AssertFilePermissions(t, filepath.Join(tmpDir, "my_directory", "my_file"), defaultFilePermissions) } func TestRendererSubTemplateInPath(t *testing.T) {