ipn,cmd/tailscale,client/tailscale: add support for renaming TailFS shares

- Updates API to support renaming TailFS shares.
- Adds a CLI rename subcommand for renaming a share.
- Renames the CLI subcommand 'add' to 'set' to make it clear that
  this is an add or update.
- Adds a unit test for TailFS in ipnlocal

Updates tailscale/corp#16827

Signed-off-by: Percy Wegmann <percy@tailscale.com>

ipn/ipnlocal/local_test.go

@@ -5,16 +5,20 @@ package ipnlocal
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net"
 	"net/http"
 	"net/netip"
+	"os"
 	"reflect"
 	"slices"
+	"sync"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"go4.org/netipx"
 	"golang.org/x/net/dns/dnsmessage"
 	"tailscale.com/appc"
@@ -25,6 +29,8 @@ import (
 	"tailscale.com/net/interfaces"
 	"tailscale.com/net/tsaddr"
 	"tailscale.com/tailcfg"
+	"tailscale.com/tailfs"
+	"tailscale.com/tailfs/tailfsimpl"
 	"tailscale.com/tsd"
 	"tailscale.com/tstest"
 	"tailscale.com/types/dnstype"
@@ -34,6 +40,7 @@ import (
 	"tailscale.com/types/netmap"
 	"tailscale.com/types/opt"
 	"tailscale.com/types/ptr"
+	"tailscale.com/types/views"
 	"tailscale.com/util/dnsname"
 	"tailscale.com/util/mak"
 	"tailscale.com/util/must"
@@ -2238,3 +2245,227 @@ func TestTCPHandlerForDst(t *testing.T) {
 		})
 	}
 }
+
+func TestTailFSManageShares(t *testing.T) {
+	tests := []struct {
+		name     string
+		disabled bool
+		existing []*tailfs.Share
+		add      *tailfs.Share
+		remove   string
+		rename   [2]string
+		expect   any
+	}{
+		{
+			name: "append",
+			existing: []*tailfs.Share{
+				{Name: "b"},
+				{Name: "d"},
+			},
+			add: &tailfs.Share{Name: "  E  "},
+			expect: []*tailfs.Share{
+				{Name: "b"},
+				{Name: "d"},
+				{Name: "e"},
+			},
+		},
+		{
+			name: "prepend",
+			existing: []*tailfs.Share{
+				{Name: "b"},
+				{Name: "d"},
+			},
+			add: &tailfs.Share{Name: "  A  "},
+			expect: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "b"},
+				{Name: "d"},
+			},
+		},
+		{
+			name: "insert",
+			existing: []*tailfs.Share{
+				{Name: "b"},
+				{Name: "d"},
+			},
+			add: &tailfs.Share{Name: "  C  "},
+			expect: []*tailfs.Share{
+				{Name: "b"},
+				{Name: "c"},
+				{Name: "d"},
+			},
+		},
+		{
+			name: "replace",
+			existing: []*tailfs.Share{
+				{Name: "b", Path: "i"},
+				{Name: "d"},
+			},
+			add: &tailfs.Share{Name: "  B  ", Path: "ii"},
+			expect: []*tailfs.Share{
+				{Name: "b", Path: "ii"},
+				{Name: "d"},
+			},
+		},
+		{
+			name:   "add_bad_name",
+			add:    &tailfs.Share{Name: "$"},
+			expect: ErrInvalidShareName,
+		},
+		{
+			name:     "add_disabled",
+			disabled: true,
+			add:      &tailfs.Share{Name: "a"},
+			expect:   ErrTailFSNotEnabled,
+		},
+		{
+			name: "remove",
+			existing: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "b"},
+				{Name: "c"},
+			},
+			remove: "b",
+			expect: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "c"},
+			},
+		},
+		{
+			name: "remove_non_existing",
+			existing: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "b"},
+				{Name: "c"},
+			},
+			remove: "D",
+			expect: os.ErrNotExist,
+		},
+		{
+			name:     "remove_disabled",
+			disabled: true,
+			remove:   "b",
+			expect:   ErrTailFSNotEnabled,
+		},
+		{
+			name: "rename",
+			existing: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "b"},
+			},
+			rename: [2]string{"a", "  C  "},
+			expect: []*tailfs.Share{
+				{Name: "b"},
+				{Name: "c"},
+			},
+		},
+		{
+			name: "rename_not_exist",
+			existing: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "b"},
+			},
+			rename: [2]string{"d", "c"},
+			expect: os.ErrNotExist,
+		},
+		{
+			name: "rename_exists",
+			existing: []*tailfs.Share{
+				{Name: "a"},
+				{Name: "b"},
+			},
+			rename: [2]string{"a", "b"},
+			expect: os.ErrExist,
+		},
+		{
+			name:   "rename_bad_name",
+			rename: [2]string{"a", "$"},
+			expect: ErrInvalidShareName,
+		},
+		{
+			name:     "rename_disabled",
+			disabled: true,
+			rename:   [2]string{"a", "c"},
+			expect:   ErrTailFSNotEnabled,
+		},
+	}
+
+	tailfs.DisallowShareAs = true
+	t.Cleanup(func() {
+		tailfs.DisallowShareAs = false
+	})
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			b := newTestBackend(t)
+			b.mu.Lock()
+			if tt.existing != nil {
+				b.tailFSSetSharesLocked(tt.existing)
+			}
+			if !tt.disabled {
+				self := b.netMap.SelfNode.AsStruct()
+				self.CapMap = tailcfg.NodeCapMap{tailcfg.NodeAttrsTailFSShare: nil}
+				b.netMap.SelfNode = self.View()
+				b.sys.Set(tailfsimpl.NewFileSystemForRemote(b.logf))
+			}
+			b.mu.Unlock()
+
+			ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+			t.Cleanup(cancel)
+
+			result := make(chan views.SliceView[*tailfs.Share, tailfs.ShareView], 1)
+
+			var wg sync.WaitGroup
+			wg.Add(1)
+			go b.WatchNotifications(
+				ctx,
+				0,
+				func() { wg.Done() },
+				func(n *ipn.Notify) bool {
+					select {
+					case result <- n.TailFSShares:
+					default:
+						//
+					}
+					return false
+				},
+			)
+			wg.Wait()
+
+			var err error
+			switch {
+			case tt.add != nil:
+				err = b.TailFSSetShare(tt.add)
+			case tt.remove != "":
+				err = b.TailFSRemoveShare(tt.remove)
+			default:
+				err = b.TailFSRenameShare(tt.rename[0], tt.rename[1])
+			}
+
+			switch e := tt.expect.(type) {
+			case error:
+				if !errors.Is(err, e) {
+					t.Errorf("expected error, want: %v got: %v", e, err)
+				}
+			case []*tailfs.Share:
+				if err != nil {
+					t.Errorf("unexpected error: %v", err)
+				} else {
+					r := <-result
+
+					got, err := json.MarshalIndent(r, "", "  ")
+					if err != nil {
+						t.Fatalf("can't marshal got: %v", err)
+					}
+					want, err := json.MarshalIndent(e, "", "  ")
+					if err != nil {
+						t.Fatalf("can't marshal want: %v", err)
+					}
+					if diff := cmp.Diff(string(got), string(want)); diff != "" {
+						t.Errorf("wrong shares; (-got+want):%v", diff)
+					}
+				}
+			}
+		})
+	}
+}

ipn/ipnlocal/tailfs.go

@@ -6,7 +6,9 @@ package ipnlocal
 import (
 	"errors"
 	"fmt"
+	"os"
 	"regexp"
+	"slices"
 	"strings"
 
 	"tailscale.com/ipn"
@@ -24,7 +26,8 @@ const (
 
 var (
 	shareNameRegex      = regexp.MustCompile(`^[a-z0-9_\(\) ]+$`)
-	errInvalidShareName = errors.New("Share names may only contain the letters a-z, underscore _, parentheses (), or spaces")
+	ErrTailFSNotEnabled = errors.New("TailFS not enabled")
+	ErrInvalidShareName = errors.New("Share names may only contain the letters a-z, underscore _, parentheses (), or spaces")
 )
 
 // TailFSSharingEnabled reports whether sharing to remote nodes via tailfs is
@@ -59,18 +62,18 @@ func (b *LocalBackend) tailFSAccessEnabledLocked() bool {
 func (b *LocalBackend) TailFSSetFileServerAddr(addr string) error {
 	fs, ok := b.sys.TailFSForRemote.GetOK()
 	if !ok {
-		return errors.New("tailfs not enabled")
+		return ErrTailFSNotEnabled
 	}
 
 	fs.SetFileServerAddr(addr)
 	return nil
 }
 
-// TailFSAddShare adds the given share if no share with that name exists, or
-// replaces the existing share if one with the same name already exists.
-// To avoid potential incompatibilities across file systems, share names are
+// TailFSSetShare adds the given share if no share with that name exists, or
+// replaces the existing share if one with the same name already exists. To
+// avoid potential incompatibilities across file systems, share names are
 // limited to alphanumeric characters and the underscore _.
-func (b *LocalBackend) TailFSAddShare(share *tailfs.Share) error {
+func (b *LocalBackend) TailFSSetShare(share *tailfs.Share) error {
 	var err error
 	share.Name, err = normalizeShareName(share.Name)
 	if err != nil {
@@ -78,7 +81,7 @@ func (b *LocalBackend) TailFSAddShare(share *tailfs.Share) error {
 	}
 
 	b.mu.Lock()
-	shares, err := b.tailFSAddShareLocked(share)
+	shares, err := b.tailFSSetShareLocked(share)
 	b.mu.Unlock()
 	if err != nil {
 		return err
@@ -99,18 +102,18 @@ func normalizeShareName(name string) (string, error) {
 	name = strings.TrimSpace(name)
 
 	if !shareNameRegex.MatchString(name) {
-		return "", errInvalidShareName
+		return "", ErrInvalidShareName
 	}
 
 	return name, nil
 }
 
-func (b *LocalBackend) tailFSAddShareLocked(share *tailfs.Share) (views.SliceView[*tailfs.Share, tailfs.ShareView], error) {
+func (b *LocalBackend) tailFSSetShareLocked(share *tailfs.Share) (views.SliceView[*tailfs.Share, tailfs.ShareView], error) {
 	existingShares := b.pm.prefs.TailFSShares()
 
 	fs, ok := b.sys.TailFSForRemote.GetOK()
 	if !ok {
-		return existingShares, errors.New("tailfs not enabled")
+		return existingShares, ErrTailFSNotEnabled
 	}
 
 	addedShare := false
@@ -139,6 +142,70 @@ func (b *LocalBackend) tailFSAddShareLocked(share *tailfs.Share) (views.SliceVie
 	return b.pm.prefs.TailFSShares(), nil
 }
 
+// TailFSRenameShare renames the share at old name to new name. To avoid
+// potential incompatibilities across file systems, the new share name is
+// limited to alphanumeric characters and the underscore _.
+// Any of the following will result in an error.
+// - no share found under old name
+// - new share name contains disallowed characters
+// - share already exists under new name
+func (b *LocalBackend) TailFSRenameShare(oldName, newName string) error {
+	var err error
+	newName, err = normalizeShareName(newName)
+	if err != nil {
+		return err
+	}
+
+	b.mu.Lock()
+	shares, err := b.tailFSRenameShareLocked(oldName, newName)
+	b.mu.Unlock()
+	if err != nil {
+		return err
+	}
+
+	b.tailFSNotifyShares(shares)
+	return nil
+}
+
+func (b *LocalBackend) tailFSRenameShareLocked(oldName, newName string) (views.SliceView[*tailfs.Share, tailfs.ShareView], error) {
+	existingShares := b.pm.prefs.TailFSShares()
+
+	fs, ok := b.sys.TailFSForRemote.GetOK()
+	if !ok {
+		return existingShares, ErrTailFSNotEnabled
+	}
+
+	found := false
+	var shares []*tailfs.Share
+	for i := 0; i < existingShares.Len(); i++ {
+		existing := existingShares.At(i)
+		if existing.Name() == newName {
+			return existingShares, os.ErrExist
+		}
+		if existing.Name() == oldName {
+			share := existing.AsStruct()
+			share.Name = newName
+			shares = append(shares, share)
+			found = true
+		} else {
+			shares = append(shares, existing.AsStruct())
+		}
+	}
+
+	if !found {
+		return existingShares, os.ErrNotExist
+	}
+
+	slices.SortFunc(shares, tailfs.CompareShares)
+	err := b.tailFSSetSharesLocked(shares)
+	if err != nil {
+		return existingShares, err
+	}
+	fs.SetShares(shares)
+
+	return b.pm.prefs.TailFSShares(), nil
+}
+
 // TailFSRemoveShare removes the named share. Share names are forced to
 // lowercase.
 func (b *LocalBackend) TailFSRemoveShare(name string) error {
@@ -166,17 +233,24 @@ func (b *LocalBackend) tailFSRemoveShareLocked(name string) (views.SliceView[*ta
 
 	fs, ok := b.sys.TailFSForRemote.GetOK()
 	if !ok {
-		return existingShares, errors.New("tailfs not enabled")
+		return existingShares, ErrTailFSNotEnabled
 	}
 
+	found := false
 	var shares []*tailfs.Share
 	for i := 0; i < existingShares.Len(); i++ {
 		existing := existingShares.At(i)
 		if existing.Name() != name {
 			shares = append(shares, existing.AsStruct())
+		} else {
+			found = true
 		}
 	}
 
+	if !found {
+		return existingShares, os.ErrNotExist
+	}
+
 	err := b.tailFSSetSharesLocked(shares)
 	if err != nil {
 		return existingShares, err

ipn/ipnlocal/tailfs_test.go

@@ -20,11 +20,11 @@ func TestNormalizeShareName(t *testing.T) {
 		},
 		{
 			name: "",
-			err:  errInvalidShareName,
+			err:  ErrInvalidShareName,
 		},
 		{
 			name: "generally good except for .",
-			err:  errInvalidShareName,
+			err:  ErrInvalidShareName,
 		},
 	}
 	for _, tt := range tests {

ipn/localapi/localapi.go

@@ -2571,9 +2571,14 @@ func (h *Handler) serveTailFSFileServerAddr(w http.ResponseWriter, r *http.Reque
 }
 
 // serveShares handles the management of tailfs shares.
+//
+// PUT - adds or updates an existing share
+// DELETE - removes a share
+// GET - gets a list of all shares, sorted by name
+// POST - renames an existing share
 func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) {
 	if !h.b.TailFSSharingEnabled() {
-		http.Error(w, `tailfs sharing not enabled, please add the attribute "tailfs:share" to this node in your ACLs' "nodeAttrs" section`, http.StatusInternalServerError)
+		http.Error(w, `tailfs sharing not enabled, please add the attribute "tailfs:share" to this node in your ACLs' "nodeAttrs" section`, http.StatusForbidden)
 		return
 	}
 	switch r.Method {
@@ -2603,20 +2608,23 @@ func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) {
 			}
 			share.As = username
 		}
-		err = h.b.TailFSAddShare(&share)
+		err = h.b.TailFSSetShare(&share)
 		if err != nil {
+			if errors.Is(err, ipnlocal.ErrInvalidShareName) {
+				http.Error(w, "invalid share name", http.StatusBadRequest)
+				return
+			}
 			http.Error(w, err.Error(), http.StatusInternalServerError)
 			return
 		}
 		w.WriteHeader(http.StatusCreated)
 	case "DELETE":
-		var share tailfs.Share
-		err := json.NewDecoder(r.Body).Decode(&share)
+		b, err := io.ReadAll(r.Body)
 		if err != nil {
 			http.Error(w, err.Error(), http.StatusBadRequest)
 			return
 		}
-		err = h.b.TailFSRemoveShare(share.Name)
+		err = h.b.TailFSRemoveShare(string(b))
 		if err != nil {
 			if os.IsNotExist(err) {
 				http.Error(w, "share not found", http.StatusNotFound)
@@ -2626,6 +2634,31 @@ func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 		w.WriteHeader(http.StatusNoContent)
+	case "POST":
+		var names [2]string
+		err := json.NewDecoder(r.Body).Decode(&names)
+		if err != nil {
+			http.Error(w, err.Error(), http.StatusBadRequest)
+			return
+		}
+		err = h.b.TailFSRenameShare(names[0], names[1])
+		if err != nil {
+			if os.IsNotExist(err) {
+				http.Error(w, "share not found", http.StatusNotFound)
+				return
+			}
+			if os.IsExist(err) {
+				http.Error(w, "share name already used", http.StatusBadRequest)
+				return
+			}
+			if errors.Is(err, ipnlocal.ErrInvalidShareName) {
+				http.Error(w, "invalid share name", http.StatusBadRequest)
+				return
+			}
+			http.Error(w, err.Error(), http.StatusInternalServerError)
+			return
+		}
+		w.WriteHeader(http.StatusNoContent)
 	case "GET":
 		shares := h.b.TailFSGetShares()
 		err := json.NewEncoder(w).Encode(shares)