From 280fbb550646f887a2397ad6db60af7f9d63939f Mon Sep 17 00:00:00 2001 From: Will Browne Date: Tue, 16 Sep 2025 16:26:34 +0100 Subject: [PATCH] adjust tests + fix import --- pkg/plugins/storage/ownership_nop.go | 5 +- pkg/plugins/storage/ownership_unix.go | 25 +++- pkg/plugins/storage/ownership_unix_test.go | 151 +++++++++++++++++---- 3 files changed, 146 insertions(+), 35 deletions(-) diff --git a/pkg/plugins/storage/ownership_nop.go b/pkg/plugins/storage/ownership_nop.go index 965c8b5546f..fec98143fb0 100644 --- a/pkg/plugins/storage/ownership_nop.go +++ b/pkg/plugins/storage/ownership_nop.go @@ -1,8 +1,9 @@ //go:build windows +// +build windows package storage // no-op for Windows -func matchOwnershipToParent(_ , _ string) error { - return nil +func matchOwnershipToParent(_, _ string) error { + return nil } diff --git a/pkg/plugins/storage/ownership_unix.go b/pkg/plugins/storage/ownership_unix.go index 80f722c95b5..102e43ffb10 100644 --- a/pkg/plugins/storage/ownership_unix.go +++ b/pkg/plugins/storage/ownership_unix.go @@ -3,13 +3,30 @@ package storage import ( - "fmt" - "os" - "syscall" + "fmt" + "os" + "path/filepath" + "syscall" ) +// chownFunc is a function type for changing file ownership +type chownFunc func(name string, uid, gid int) error + +// ownershipChanger handles changing file ownership +type ownershipChanger struct { + chown chownFunc +} + +// defaultOwnershipChanger uses the real os.Chown function +var defaultOwnershipChanger = &ownershipChanger{chown: os.Chown} + // matchOwnershipToParent ensures child directory and all its contents inherit ownership from parent dir (UID/GID). func matchOwnershipToParent(child, parent string) error { + return matchOwnershipToParentWithChanger(child, parent, defaultOwnershipChanger) +} + +// matchOwnershipToParentWithChanger is the testable version that accepts a custom ownership changer +func matchOwnershipToParentWithChanger(child, parent string, changer *ownershipChanger) error { parentInfo, err := os.Stat(parent) if err != nil { return err @@ -30,6 +47,6 @@ func matchOwnershipToParent(child, parent string) error { if err != nil { return err } - return os.Chown(path, uid, gid) + return changer.chown(path, uid, gid) }) } diff --git a/pkg/plugins/storage/ownership_unix_test.go b/pkg/plugins/storage/ownership_unix_test.go index 5f1a9b71dd7..df3620b4826 100644 --- a/pkg/plugins/storage/ownership_unix_test.go +++ b/pkg/plugins/storage/ownership_unix_test.go @@ -1,47 +1,140 @@ //go:build !windows -// +build !windows package storage import ( + "errors" "os" "path/filepath" + "syscall" "testing" + + "github.com/stretchr/testify/require" ) -// Basic tests for matchOwnershipToParent on Unix-like systems. -// These avoid requiring root and are safe to run in CI. +func TestMatchOwnershipToParent(t *testing.T) { + t.Run("Parent dir does not exist", func(t *testing.T) { + childDir := t.TempDir() + err := matchOwnershipToParent(childDir, filepath.Join(childDir, "no-such-parent")) + require.ErrorIs(t, err, os.ErrNotExist) + }) -func TestMatchOwnershipToParent_NoErrorAndRecurses(t *testing.T) { - parent := t.TempDir() - childParent := t.TempDir() - child := filepath.Join(childParent, "childdir") - if err := os.MkdirAll(filepath.Join(child, "nested"), 0o755); err != nil { - t.Fatalf("failed to create child nested dir: %v", err) - } + t.Run("Child directory does not exist", func(t *testing.T) { + parent := t.TempDir() + nonExistentChild := filepath.Join(parent, "does-not-exist") + err := matchOwnershipToParent(nonExistentChild, parent) + require.ErrorIs(t, err, os.ErrNotExist) + }) - fpath := filepath.Join(child, "nested", "file.txt") - if err := os.WriteFile(fpath, []byte("hello"), 0o644); err != nil { - t.Fatalf("failed to write file: %v", err) - } + t.Run("Should call chown for all files and directories", func(t *testing.T) { + parent := t.TempDir() + parentUID, parentGID := getOwnership(t, parent) - // Should not error and must recurse into child directories. - if err := matchOwnershipToParent(child, parent); err != nil { - t.Fatalf("matchOwnershipToParent returned error: %v", err) - } + // Create test structure + child := filepath.Join(parent, "child") + err := os.MkdirAll(child, 0o750) + require.NoError(t, err) - // File must still exist after call. - if _, err := os.Stat(fpath); err != nil { - t.Fatalf("expected file to exist after ownership change, got: %v", err) - } + nestedDir := filepath.Join(child, "nested") + err = os.MkdirAll(nestedDir, 0o750) + require.NoError(t, err) + + file1 := filepath.Join(child, "file1.txt") + err = os.WriteFile(file1, []byte("test"), 0o644) + require.NoError(t, err) + + file2 := filepath.Join(nestedDir, "file2.txt") + err = os.WriteFile(file2, []byte("test"), 0o644) + require.NoError(t, err) + + mock := &mockChowner{} + err = matchOwnershipToParentWithChanger(child, parent, &ownershipChanger{chown: mock.chown}) + require.NoError(t, err) + + // Verify chown was called for all paths with correct UID/GID + require.Len(t, mock.calls, 4) + + expectedPaths := []string{child, file1, nestedDir, file2} + actualPaths := make([]string, len(mock.calls)) + for i, call := range mock.calls { + actualPaths[i] = call.path + require.Equal(t, parentUID, call.uid) + require.Equal(t, parentGID, call.gid) + } + + require.ElementsMatch(t, expectedPaths, actualPaths) + }) + + t.Run("Should handle chown errors", func(t *testing.T) { + parent := t.TempDir() + + child := filepath.Join(parent, "child") + err := os.MkdirAll(child, 0o750) + require.NoError(t, err) + + // Mock chown to return an error + chownErr := errors.New("permission denied") + mock := &mockChowner{err: chownErr} + err = matchOwnershipToParentWithChanger(child, parent, &ownershipChanger{chown: mock.chown}) + require.ErrorIs(t, err, chownErr) + }) + + t.Run("Should handle single file", func(t *testing.T) { + parent := t.TempDir() + parentUID, parentGID := getOwnership(t, parent) + + singleFile := filepath.Join(parent, "single-file.txt") + err := os.WriteFile(singleFile, []byte("content"), 0o644) + require.NoError(t, err) + + mock := &mockChowner{} + err = matchOwnershipToParentWithChanger(singleFile, parent, &ownershipChanger{chown: mock.chown}) + require.NoError(t, err) + + // Should call chown once for the single file + require.Len(t, mock.calls, 1) + require.Equal(t, singleFile, mock.calls[0].path) + require.Equal(t, parentUID, mock.calls[0].uid) + require.Equal(t, parentGID, mock.calls[0].gid) + }) + + t.Run("Should not error with non mock", func(t *testing.T) { + parent := t.TempDir() + + singleFile := filepath.Join(parent, "single-file.txt") + err := os.WriteFile(singleFile, []byte("content"), 0o644) + require.NoError(t, err) + + err = matchOwnershipToParent(singleFile, parent) + require.NoError(t, err) + }) } -func TestMatchOwnershipToParent_MissingParentReturnsError(t *testing.T) { - child := t.TempDir() - missingParent := filepath.Join(child, "no-such-parent") +// getOwnership returns the UID and GID of a file or directory +func getOwnership(t *testing.T, path string) (uid, gid int) { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) - // Should error if parent does not exist. - if err := matchOwnershipToParent(child, missingParent); err == nil { - t.Fatalf("expected an error when parent does not exist, got nil") - } + stat, ok := info.Sys().(*syscall.Stat_t) + require.True(t, ok) + + return int(stat.Uid), int(stat.Gid) +} + +// mockChowner records calls to chown for testing +type mockChowner struct { + calls []chownCall + err error // error to return from chown calls +} + +type chownCall struct { + path string + uid int + gid int +} + +func (m *mockChowner) chown(path string, uid, gid int) error { + m.calls = append(m.calls, chownCall{path: path, uid: uid, gid: gid}) + return m.err }