internal/vaultik/restore.go

@@ -12,6 +12,7 @@ import (
 	"path/filepath"
 	"slices"
 	"strings"
+	"syscall"
 	"time"
 
 	"filippo.io/age"
@@ -219,9 +220,7 @@ func (v *Vaultik) finishRestore(
 	)
 
 	if os.Geteuid() != 0 {
-		v.UI.Warningf("Restore did not preserve file ownership: chown(2) " +
-			"requires root. Re-run as root (e.g. with sudo) if you need " +
-			"original UID/GID preserved.")
+		v.warnIfOwnershipNotPreserved(files, opts.TargetDir)
 	}
 
 	if result.FilesFailed > 0 {
@@ -247,6 +246,43 @@ func (v *Vaultik) finishRestore(
 	return unmatchedErr
 }
 
+// warnIfOwnershipNotPreserved warns once if some entry restored under
+// targetDir has another owner or group on disk than the snapshot
+// records. It is meant for a restore by a normal user, which changes no
+// owner or group: a new entry is owned by that user, and its group is
+// that user's, or the group of the directory it is created in on macOS
+// and in a setgid directory on Linux. An entry that is not on disk, or
+// whose filesystem reports no owner, is skipped.
+func (v *Vaultik) warnIfOwnershipNotPreserved(
+	files []*database.File, targetDir string,
+) {
+	for _, file := range files {
+		targetPath, err := containedRestorePath(
+			v.Fs, targetDir, file.Path.String())
+		if err != nil {
+			continue
+		}
+
+		info, err := lstatIfPossible(v.Fs, targetPath)
+		if err != nil {
+			continue
+		}
+
+		owner, ok := info.Sys().(*syscall.Stat_t)
+		if !ok {
+			continue
+		}
+
+		if owner.Uid != file.UID || owner.Gid != file.GID {
+			v.UI.Warningf("Restore did not preserve file ownership: chown(2) " +
+				"requires root. Re-run as root (e.g. with sudo) if you need " +
+				"original UID/GID preserved.")
+
+			return
+		}
+	}
+}
+
 // reportUnmatchedPaths warns, by name, about each requested path that
 // matched nothing in the snapshot, and returns an error counting them,
 // or nil when there are none.
@@ -965,8 +1001,9 @@ type restoreSession struct {
 	// root can chown a file to an arbitrary UID/GID — non-root chown
 	// always fails with EPERM. Attempting it anyway produces N
 	// guaranteed-failed syscalls + N noisy debug lines, so we skip
-	// the call entirely as non-root and emit one warning at the end
-	// of the restore explaining that ownership was not preserved.
+	// the call entirely as non-root; warnIfOwnershipNotPreserved warns
+	// once at the end of the restore if that left any owner or group
+	// other than recorded.
 	runningAsRoot bool
 	// directories holds every restored directory, for
 	// applyDirectoryMetadata to finish after the restore loop.

internal/vaultik/restore_ownership_warning_test.go

@@ -0,0 +1,131 @@
+package vaultik //nolint:testpackage // calls the unexported ownership check
+
+import (
+	"bytes"
+	"context"
+	"os"
+	"strings"
+	"syscall"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/ui"
+)
+
+// These tests cover https://git.eeqj.de/sneak/vaultik/issues/316: a
+// restore by a normal user warns that ownership was not preserved only
+// when some restored entry has another owner or group on disk than the
+// snapshot records.
+
+const ownershipWarning = "did not preserve file ownership"
+
+// TestOwnershipWarningSilentWhenEntriesMatch checks that a restore whose
+// entries all end up with their recorded owner and group prints no
+// warning.
+func TestOwnershipWarningSilentWhenEntriesMatch(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	targetDir := t.TempDir()
+	owner := ownerOf(t, targetDir)
+
+	out := restoreAsNormalUser(t, targetDir, []*database.File{
+		{Path: "/a", Mode: plainFileMode, UID: owner.Uid, GID: owner.Gid},
+		{Path: "/b", Mode: plainFileMode, UID: owner.Uid, GID: owner.Gid},
+	})
+
+	assert.Empty(t, out)
+}
+
+// TestOwnershipWarningWhenOwnerDiffers checks that the warning is printed
+// when an entry recorded with another owner is restored owned by the
+// restoring user, even though its group matches the recorded one.
+func TestOwnershipWarningWhenOwnerDiffers(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	targetDir := t.TempDir()
+	owner := ownerOf(t, targetDir)
+
+	out := restoreAsNormalUser(t, targetDir, []*database.File{
+		{Path: "/a", Mode: plainFileMode, UID: otherOwnerID, GID: owner.Gid},
+	})
+
+	assert.Equal(t, 1, strings.Count(out, ownershipWarning), out)
+}
+
+// TestOwnershipWarningWhenDirectoryGivesItsGroup checks that the warning
+// is printed, once, when entries recorded with the restoring user's
+// owner and group take another group from the setgid directory they are
+// restored into, as every new entry takes its directory's group on
+// macOS.
+func TestOwnershipWarningWhenDirectoryGivesItsGroup(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	if os.Geteuid() != 0 {
+		t.Skip("giving a directory to another group needs root")
+	}
+
+	targetDir := t.TempDir()
+	user := ownerOf(t, targetDir)
+
+	require.NoError(t, os.Chown(targetDir, -1, int(otherOwnerID)))
+	require.NoError(t, os.Chmod(targetDir, os.ModeSetgid|writableTestMode))
+
+	out := restoreAsNormalUser(t, targetDir, []*database.File{
+		{Path: "/a", Mode: plainFileMode, UID: user.Uid, GID: user.Gid},
+		{Path: "/b", Mode: plainFileMode, UID: user.Uid, GID: user.Gid},
+	})
+
+	assert.Equal(t, 1, strings.Count(out, ownershipWarning), out)
+}
+
+// ownerOf returns the owner and group path has on disk.
+func ownerOf(t *testing.T, path string) *syscall.Stat_t {
+	t.Helper()
+
+	info, err := os.Lstat(path)
+	require.NoError(t, err)
+
+	owner, ok := info.Sys().(*syscall.Stat_t)
+	require.True(t, ok)
+
+	return owner
+}
+
+// restoreAsNormalUser restores rows, all empty regular files, into
+// targetDir without changing any owner or group, as restore does for a
+// normal user, and returns what warnIfOwnershipNotPreserved prints.
+func restoreAsNormalUser(
+	t *testing.T, targetDir string, rows []*database.File,
+) string {
+	t.Helper()
+
+	ctx := context.Background()
+	rows, repos := makeFiles(ctx, t, rows)
+
+	var out bytes.Buffer
+
+	v := newContainmentVaultik(ctx, afero.NewOsFs())
+	v.UI = ui.NewWithColor(&out, false)
+
+	session := &restoreSession{
+		v:      v,
+		ctx:    ctx,
+		repos:  repos,
+		opts:   &RestoreOptions{TargetDir: targetDir},
+		result: &RestoreResult{},
+	}
+	for _, row := range rows {
+		require.NoError(t, session.restoreFile(row))
+	}
+
+	v.warnIfOwnershipNotPreserved(rows, targetDir)
+
+	return out.String()
+}