From ab47d2a4bfec808405d0a97d84423af681d195a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 8 Sep 2026 21:18:55 +0200 Subject: [PATCH] [client] Apply the file drop mode and destination as one change FileDropSetSettings committed the mode first, through a Set that persists it and fires the handler binding the receiver, and only then validated the destination. A destination the caller may not use returned an error the UI read as "nothing changed", while the profile had already moved to auto-accept with the receiver listening, so the next offer from any peer was taken without a prompt and delivered to the old directory. Both halves are now settled before either is applied, and applied in the one Set that already carries them: it validates the mode before touching anything, so a refusal on either half leaves the policy alone. --- client/server/filedrop.go | 15 +++-- client/server/filedrop_test.go | 102 +++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 6 deletions(-) create mode 100644 client/server/filedrop_test.go diff --git a/client/server/filedrop.go b/client/server/filedrop.go index 861975f61..2fcd60142 100644 --- a/client/server/filedrop.go +++ b/client/server/filedrop.go @@ -267,19 +267,22 @@ func (s *Server) FileDropSetSettings(ctx context.Context, req *proto.FileDropSet return nil, err } + // Both halves are settled before either is applied, and then applied in one + // write: committing the mode first left a refused destination behind a + // profile already switched to auto-accept, with the receiver bound, while + // the caller was told the whole request had failed. policy := mgr.Policy().Get() policy.Mode = filedrop.Mode(req.GetMode()) - if err := mgr.Policy().Set(policy); err != nil { - return nil, err - } if dir := req.GetDestinationDir(); dir != mgr.DestinationDir() { if err := s.validateFileDropDestination(ctx, dir); err != nil { return nil, err } - if err := mgr.SetDestinationDir(dir); err != nil { - return nil, err - } + policy.DestinationDir = dir + } + + if err := mgr.Policy().Set(policy); err != nil { + return nil, err } return &proto.FileDropSetSettingsResponse{}, nil } diff --git a/client/server/filedrop_test.go b/client/server/filedrop_test.go new file mode 100644 index 000000000..c03adacdd --- /dev/null +++ b/client/server/filedrop_test.go @@ -0,0 +1,102 @@ +package server + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/peer" + + "github.com/netbirdio/netbird/client/internal/filedrop" + "github.com/netbirdio/netbird/client/internal/ipcauth" + "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/proto" +) + +func fileDropTestServer(t *testing.T) *Server { + t.Helper() + + dir := t.TempDir() + + prevOverride := profilemanager.ConfigDirOverride + prevPathDir := profilemanager.DefaultConfigPathDir + prevPath := profilemanager.DefaultConfigPath + prevActive := profilemanager.ActiveProfileStatePath + t.Cleanup(func() { + profilemanager.ConfigDirOverride = prevOverride + profilemanager.DefaultConfigPathDir = prevPathDir + profilemanager.DefaultConfigPath = prevPath + profilemanager.ActiveProfileStatePath = prevActive + }) + + profilemanager.ConfigDirOverride = dir + profilemanager.DefaultConfigPathDir = dir + profilemanager.DefaultConfigPath = filepath.Join(dir, "default.json") + profilemanager.ActiveProfileStatePath = filepath.Join(dir, "active_profile.json") + + sm := profilemanager.NewServiceManager(profilemanager.DefaultConfigPath) + require.NoError(t, sm.SetActiveProfileStateToDefault()) + + return &Server{profileManager: sm} +} + +func callerContext(t *testing.T) context.Context { + t.Helper() + + id, err := ipcauth.CurrentProcessIdentity() + require.NoError(t, err) + + return peer.NewContext(context.Background(), &peer.Peer{ + AuthInfo: ipcauth.AuthInfo{Identity: id}, + }) +} + +func TestFileDropSetSettingsKeepsTheModeWhenTheDestinationIsRefused(t *testing.T) { + s := fileDropTestServer(t) + ctx := callerContext(t) + + mgr, err := s.fileDropManager(ctx) + require.NoError(t, err) + require.NoError(t, mgr.Policy().Set(filedrop.Policy{Mode: filedrop.ModeOff})) + + bound := 0 + mgr.Policy().SetChangeHandler(func() { bound++ }) + + // A path that is not a directory is refused by validateFileDropDestination + // whatever the caller's privileges, so the refusal is the same everywhere. + notADir := filepath.Join(t.TempDir(), "file.txt") + require.NoError(t, os.WriteFile(notADir, []byte("x"), 0o600)) + + _, err = s.FileDropSetSettings(ctx, &proto.FileDropSetSettingsRequest{ + Mode: proto.FileDropMode(filedrop.ModeAutoAccept), + DestinationDir: notADir, + }) + require.Error(t, err, "a destination that is not a directory must be refused") + + assert.Equal(t, filedrop.ModeOff, mgr.Policy().Get().Mode, + "the refused request must not leave the mode switched to auto-accept") + assert.False(t, mgr.Policy().Receiving(), "the receiver must not have been turned on") + assert.Zero(t, bound, "the change handler must not have fired") +} + +func TestFileDropSetSettingsAppliesModeAndDestinationTogether(t *testing.T) { + s := fileDropTestServer(t) + ctx := callerContext(t) + + mgr, err := s.fileDropManager(ctx) + require.NoError(t, err) + require.NoError(t, mgr.Policy().Set(filedrop.Policy{Mode: filedrop.ModeOff})) + + dest := t.TempDir() + _, err = s.FileDropSetSettings(ctx, &proto.FileDropSetSettingsRequest{ + Mode: proto.FileDropMode(filedrop.ModeAsk), + DestinationDir: dest, + }) + require.NoError(t, err) + + assert.Equal(t, filedrop.ModeAsk, mgr.Policy().Get().Mode) + assert.Equal(t, dest, mgr.Policy().DestinationDir()) +}