feature/taildrop: do not use m.opts.Dir for Android (tailscale#16316)

In Android, we are prompting the user to select a Taildrop directory when they first receive a Taildrop: we block writes on Taildrop dir selection. This means that we cannot use Dir inside managerOptions, since the http request would not get the new Taildrop extension. This PR removes, in the Android case, the reliance on m.opts.Dir, and instead has FileOps hold the correct directory.

This expands FileOps to be the Taildrop interface for all file system operations.

Updates tailscale/corp#29211

Signed-off-by: kari-ts <[email protected]>

restore tstest

Author: kari-ts

feature/taildrop/delete.go

@@ -6,9 +6,7 @@ package taildrop
 import (
 	"container/list"
 	"context"
-	"io/fs"
 	"os"
-	"path/filepath"
 	"strings"
 	"sync"
 	"time"
@@ -28,7 +26,6 @@ const deleteDelay = time.Hour
 type fileDeleter struct {
 	logf  logger.Logf
 	clock tstime.DefaultClock
-	dir   string
 	event func(string) // called for certain events; for testing only
 
 	mu     sync.Mutex
@@ -39,6 +36,7 @@ type fileDeleter struct {
 	group       syncs.WaitGroup
 	shutdownCtx context.Context
 	shutdown    context.CancelFunc
+	fs          FileOps // must be used for all filesystem operations
 }
 
 // deleteFile is a specific file to delete after deleteDelay.
@@ -50,15 +48,14 @@ type deleteFile struct {
 func (d *fileDeleter) Init(m *manager, eventHook func(string)) {
 	d.logf = m.opts.Logf
 	d.clock = m.opts.Clock
-	d.dir = m.opts.Dir
 	d.event = eventHook
+	d.fs = m.opts.fileOps
 
 	d.byName = make(map[string]*list.Element)
 	d.emptySignal = make(chan struct{})
 	d.shutdownCtx, d.shutdown = context.WithCancel(context.Background())
 
 	// From a cold-start, load the list of partial and deleted files.
-	//
 	// Only run this if we have ever received at least one file
 	// to avoid ever touching the taildrop directory on systems (e.g., MacOS)
 	// that pop up a security dialog window upon first access.
@@ -71,38 +68,45 @@ func (d *fileDeleter) Init(m *manager, eventHook func(string)) {
 	d.group.Go(func() {
 		d.event("start full-scan")
 		defer d.event("end full-scan")
-		rangeDir(d.dir, func(de fs.DirEntry) bool {
+
+		if d.fs == nil {
+			d.logf("deleter: nil FileOps")
+		}
+
+		files, err := d.fs.ListFiles()
+		if err != nil {
+			d.logf("deleter: ListDir error: %v", err)
+			return
+		}
+		for _, filename := range files {
 			switch {
 			case d.shutdownCtx.Err() != nil:
-				return false // terminate early
-			case !de.Type().IsRegular():
-				return true
-			case strings.HasSuffix(de.Name(), partialSuffix):
+				return // terminate early
+			case strings.HasSuffix(filename, partialSuffix):
 				// Only enqueue the file for deletion if there is no active put.
-				nameID := strings.TrimSuffix(de.Name(), partialSuffix)
+				nameID := strings.TrimSuffix(filename, partialSuffix)
 				if i := strings.LastIndexByte(nameID, '.'); i > 0 {
 					key := incomingFileKey{clientID(nameID[i+len("."):]), nameID[:i]}
 					m.incomingFiles.LoadFunc(key, func(_ *incomingFile, loaded bool) {
 						if !loaded {
-							d.Insert(de.Name())
+							d.Insert(filename)
 						}
 					})
 				} else {
-					d.Insert(de.Name())
+					d.Insert(filename)
 				}
-			case strings.HasSuffix(de.Name(), deletedSuffix):
+			case strings.HasSuffix(filename, deletedSuffix):
 				// Best-effort immediate deletion of deleted files.
-				name := strings.TrimSuffix(de.Name(), deletedSuffix)
-				if os.Remove(filepath.Join(d.dir, name)) == nil {
-					if os.Remove(filepath.Join(d.dir, de.Name())) == nil {
-						break
+				name := strings.TrimSuffix(filename, deletedSuffix)
+				if d.fs.Remove(name) == nil {
+					if d.fs.Remove(filename) == nil {
+						continue
 					}
 				}
-				// Otherwise, enqueue the file for later deletion.
-				d.Insert(de.Name())
+				// Otherwise enqueue for later deletion.
+				d.Insert(filename)
 			}
-			return true
-		})
+		}
 	})
 }
 
@@ -149,13 +153,13 @@ func (d *fileDeleter) waitAndDelete(wait time.Duration) {
 
 			// Delete the expired file.
 			if name, ok := strings.CutSuffix(file.name, deletedSuffix); ok {
-				if err := os.Remove(filepath.Join(d.dir, name)); err != nil && !os.IsNotExist(err) {
+				if err := d.fs.Remove(name); err != nil && !os.IsNotExist(err) {
 					d.logf("could not delete: %v", redactError(err))
 					failed = append(failed, elem)
 					continue
 				}
 			}
-			if err := os.Remove(filepath.Join(d.dir, file.name)); err != nil && !os.IsNotExist(err) {
+			if err := d.fs.Remove(file.name); err != nil && !os.IsNotExist(err) {
 				d.logf("could not delete: %v", redactError(err))
 				failed = append(failed, elem)
 				continue

feature/taildrop/delete_test.go

@@ -5,7 +5,6 @@ package taildrop
 
 import (
 	"os"
-	"path/filepath"
 	"slices"
 	"testing"
 	"time"
@@ -20,11 +19,20 @@ import (
 
 func TestDeleter(t *testing.T) {
 	dir := t.TempDir()
-	must.Do(touchFile(filepath.Join(dir, "foo.partial")))
-	must.Do(touchFile(filepath.Join(dir, "bar.partial")))
-	must.Do(touchFile(filepath.Join(dir, "fizz")))
-	must.Do(touchFile(filepath.Join(dir, "fizz.deleted")))
-	must.Do(touchFile(filepath.Join(dir, "buzz.deleted"))) // lacks a matching "buzz" file
+	var m manager
+	var fd fileDeleter
+	m.opts.Logf = t.Logf
+	m.opts.Clock = tstime.DefaultClock{Clock: tstest.NewClock(tstest.ClockOpts{
+		Start: time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC),
+	})}
+	m.opts.State = must.Get(mem.New(nil, ""))
+	m.opts.fileOps, _ = newFileOps(dir)
+
+	must.Do(m.touchFile("foo.partial"))
+	must.Do(m.touchFile("bar.partial"))
+	must.Do(m.touchFile("fizz"))
+	must.Do(m.touchFile("fizz.deleted"))
+	must.Do(m.touchFile("buzz.deleted")) // lacks a matching "buzz" file
 
 	checkDirectory := func(want ...string) {
 		t.Helper()
@@ -69,12 +77,10 @@ func TestDeleter(t *testing.T) {
 	}
 	eventHook := func(event string) { eventsChan <- event }
 
-	var m manager
-	var fd fileDeleter
 	m.opts.Logf = t.Logf
 	m.opts.Clock = tstime.DefaultClock{Clock: clock}
-	m.opts.Dir = dir
 	m.opts.State = must.Get(mem.New(nil, ""))
+	m.opts.fileOps, _ = newFileOps(dir)
 	must.Do(m.opts.State.WriteState(ipn.TaildropReceivedKey, []byte{1}))
 	fd.Init(&m, eventHook)
 	defer fd.Shutdown()
@@ -100,17 +106,17 @@ func TestDeleter(t *testing.T) {
 	checkEvents("end waitAndDelete")
 	checkDirectory()
 
-	must.Do(touchFile(filepath.Join(dir, "one.partial")))
+	must.Do(m.touchFile("one.partial"))
 	insert("one.partial")
 	checkEvents("start waitAndDelete")
 	advance(deleteDelay / 4)
-	must.Do(touchFile(filepath.Join(dir, "two.partial")))
+	must.Do(m.touchFile("two.partial"))
 	insert("two.partial")
 	advance(deleteDelay / 4)
-	must.Do(touchFile(filepath.Join(dir, "three.partial")))
+	must.Do(m.touchFile("three.partial"))
 	insert("three.partial")
 	advance(deleteDelay / 4)
-	must.Do(touchFile(filepath.Join(dir, "four.partial")))
+	must.Do(m.touchFile("four.partial"))
 	insert("four.partial")
 
 	advance(deleteDelay / 4)
@@ -145,8 +151,8 @@ func TestDeleterInitWithoutTaildrop(t *testing.T) {
 	var m manager
 	var fd fileDeleter
 	m.opts.Logf = t.Logf
-	m.opts.Dir = t.TempDir()
 	m.opts.State = must.Get(mem.New(nil, ""))
+	m.opts.fileOps, _ = newFileOps(t.TempDir())
 	fd.Init(&m, func(event string) { t.Errorf("unexpected event: %v", event) })
 	fd.Shutdown()
 }

feature/taildrop/ext.go

@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"io"
 	"maps"
-	"os"
 	"path/filepath"
 	"runtime"
 	"slices"
@@ -75,7 +74,7 @@ type Extension struct {
 
 	// FileOps abstracts platform-specific file operations needed for file transfers.
 	// This is currently being used for Android to use the Storage Access Framework.
-	FileOps FileOps
+	fileOps FileOps
 
 	nodeBackendForTest ipnext.NodeBackend // if non-nil, pretend we're this node state for tests
 
@@ -89,30 +88,6 @@ type Extension struct {
 	outgoingFiles map[string]*ipn.OutgoingFile
 }
 
-// safDirectoryPrefix is used to determine if the directory is managed via SAF.
-const SafDirectoryPrefix = "content://"
-
-// PutMode controls how Manager.PutFile writes files to storage.
-//
-//	PutModeDirect    – write files directly to a filesystem path (default).
-//	PutModeAndroidSAF – use Android’s Storage Access Framework (SAF), where
-//	                      the OS manages the underlying directory permissions.
-type PutMode int
-
-const (
-	PutModeDirect PutMode = iota
-	PutModeAndroidSAF
-)
-
-// FileOps defines platform-specific file operations.
-type FileOps interface {
-	OpenFileWriter(filename string) (io.WriteCloser, string, error)
-
-	// RenamePartialFile finalizes a partial file.
-	// It returns the new SAF URI as a string and an error.
-	RenamePartialFile(partialUri, targetDirUri, targetName string) (string, error)
-}
-
 func (e *Extension) Name() string {
 	return "taildrop"
 }
@@ -176,23 +151,34 @@ func (e *Extension) onChangeProfile(profile ipn.LoginProfileView, _ ipn.PrefsVie
 		return
 	}
 
-	// If we have a netmap, create a taildrop manager.
-	fileRoot, isDirectFileMode := e.fileRoot(uid, activeLogin)
-	if fileRoot == "" {
-		e.logf("no Taildrop directory configured")
-	}
-	mode := PutModeDirect
-	if e.directFileRoot != "" && strings.HasPrefix(e.directFileRoot, SafDirectoryPrefix) {
-		mode = PutModeAndroidSAF
+	// Use the provided [FileOps] implementation (typically for SAF access on Android),
+	// or create an [fsFileOps] instance rooted at fileRoot.
+	//
+	// A non-nil [FileOps] also implies that we are in DirectFileMode.
+	fops := e.fileOps
+	isDirectFileMode := fops != nil
+	if fops == nil {
+		var fileRoot string
+		if fileRoot, isDirectFileMode = e.fileRoot(uid, activeLogin); fileRoot == "" {
+			e.logf("no Taildrop directory configured")
+			e.setMgrLocked(nil)
+			return
+		}
+
+		var err error
+		if fops, err = newFileOps(fileRoot); err != nil {
+			e.logf("taildrop: cannot create FileOps: %v", err)
+			e.setMgrLocked(nil)
+			return
+		}
 	}
+
 	e.setMgrLocked(managerOptions{
 		Logf:           e.logf,
 		Clock:          tstime.DefaultClock{Clock: e.sb.Clock()},
 		State:          e.stateStore,
-		Dir:            fileRoot,
 		DirectFileMode: isDirectFileMode,
-		FileOps:        e.FileOps,
-		Mode:           mode,
+		fileOps:        fops,
 		SendFileNotify: e.sendFileNotify,
 	}.New())
 }
@@ -221,12 +207,7 @@ func (e *Extension) fileRoot(uid tailcfg.UserID, activeLogin string) (root strin
 	baseDir := fmt.Sprintf("%s-uid-%d",
 		strings.ReplaceAll(activeLogin, "@", "-"),
 		uid)
-	dir := filepath.Join(varRoot, "files", baseDir)
-	if err := os.MkdirAll(dir, 0700); err != nil {
-		e.logf("Taildrop disabled; error making directory: %v", err)
-		return "", false
-	}
-	return dir, false
+	return filepath.Join(varRoot, "files", baseDir), false
 }
 
 // hasCapFileSharing reports whether the current node has the file sharing

feature/taildrop/fileops.go

@@ -0,0 +1,41 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package taildrop
+
+import (
+	"io"
+	"io/fs"
+	"os"
+)
+
+// FileOps abstracts over both local‐FS paths and Android SAF URIs.
+type FileOps interface {
+	// OpenWriter creates or truncates a file named relative to the receiver's root,
+	// seeking to the specified offset. If the file does not exist, it is created with mode perm
+	// on platforms that support it.
+	//
+	// It returns an [io.WriteCloser] and the file's absolute path, or an error.
+	// This call may block. Callers should avoid holding locks when calling OpenWriter.
+	OpenWriter(name string, offset int64, perm os.FileMode) (wc io.WriteCloser, path string, err error)
+
+	// Remove deletes a file or directory relative to the receiver's root.
+	// It returns [io.ErrNotExist] if the file or directory does not exist.
+	Remove(name string) error
+
+	// Rename atomically renames oldPath to a new file named newName,
+	// returning the full new path or an error.
+	Rename(oldPath, newName string) (newPath string, err error)
+
+	// ListFiles returns just the basenames of all regular files
+	// in the root directory.
+	ListFiles() ([]string, error)
+
+	// Stat returns the FileInfo for the given name or an error.
+	Stat(name string) (fs.FileInfo, error)
+
+	// OpenReader opens the given basename for the given name or an error.
+	OpenReader(name string) (io.ReadCloser, error)
+}
+
+var newFileOps func(dir string) (FileOps, error)