Improve fileshare monitoring job

daemon/jobs_test.go

@@ -36,6 +36,7 @@ func (failingLoginChecker) IsMFAEnabled() (bool, error) { return false, nil }
 func (failingLoginChecker) IsVPNExpired() (bool, error) {
 	return true, errors.New("IsVPNExpired error")
 }
+
 func (failingLoginChecker) GetDedicatedIPServices() ([]auth.DedicatedIPService, error) {
 	return nil, fmt.Errorf("Not implemented")
 }
@@ -128,6 +129,10 @@ func (n *meshNetworker) AllowFileshare(address meshnet.UniqueAddress) error {
 	return nil
 }
 
+func (n *meshNetworker) PermitFileshare() error {
+	return nil
+}
+
 func (n *meshNetworker) AllowIncoming(address meshnet.UniqueAddress, lanAllowed bool) error {
 	n.allowedIncoming = append(n.allowedIncoming, address)
 	return nil
@@ -138,6 +143,10 @@ func (n *meshNetworker) BlockIncoming(address meshnet.UniqueAddress) error {
 	return nil
 }
 
+func (n *meshNetworker) ForbidFileshare() error {
+	return nil
+}
+
 func (n *meshNetworker) BlockFileshare(address meshnet.UniqueAddress) error {
 	n.blockedFileshare = append(n.blockedFileshare, address)
 	return nil

go.sum

@@ -12,8 +12,6 @@ github.com/NordSecurity/gopenvpn v0.0.0-20230117114932-2252c52984b4 h1:2ozEjYEw4
 github.com/NordSecurity/gopenvpn v0.0.0-20230117114932-2252c52984b4/go.mod h1:tguhorMSnkMcQExNIHWBX6TRhFeGYlERzbeAWZ4j9Uw=
 github.com/NordSecurity/libdrop-go/v8 v8.0.0-20241017064027-670787595588 h1:L/nAbQXJGCOFqw1eTTRYEBKmiuaQQeS7b863+0Ifevw=
 github.com/NordSecurity/libdrop-go/v8 v8.0.0-20241017064027-670787595588/go.mod h1:SRYI0D0K6hSMBskvcB2/t/5ktSTNLPGbOvLaQ5p/sAE=
-github.com/NordSecurity/libtelio-go/v5 v5.1.4 h1:o2JbYad8sdRsljFAMZRVmkXGQ7kTVbS32P6TTDOTuL0=
-github.com/NordSecurity/libtelio-go/v5 v5.1.4/go.mod h1:mnoTGgXOu8dBQgPxG8MBju4d9C+ljKIT2p8OX5GFom4=
 github.com/NordSecurity/libtelio-go/v5 v5.1.5 h1:lwP7m2GJ+GkO1EDaRqm5ymDT/CtjIBC/1bN2CL55mnY=
 github.com/NordSecurity/libtelio-go/v5 v5.1.5/go.mod h1:mnoTGgXOu8dBQgPxG8MBju4d9C+ljKIT2p8OX5GFom4=
 github.com/NordSecurity/systray v0.0.0-20240327004800-3e3b59c1b83d h1:oUEFXgFRa9Svcjr+O1stzR3vEXZ5OfQxLUcDjqFcOuo=

meshnet/jobs.go

@@ -15,7 +15,7 @@ func (s *Server) StartJobs() {
 	}
 
 	if _, err := s.scheduler.NewJob(
-		gocron.DurationJob(5*time.Second),
+		gocron.DurationJob(1*time.Second),
 		gocron.NewTask(JobMonitorFileshareProcess(s)),
 		gocron.WithName("job monitor fileshare process")); err != nil {
 		log.Println(internal.WarningPrefix, "job monitor fileshare process schedule error:", err)
@@ -39,33 +39,58 @@ func JobRefreshMeshnet(s *Server) func() error {
 }
 
 func JobMonitorFileshareProcess(s *Server) func() error {
-	oldState := false
-	return func() error {
-		if !s.isMeshOn() {
-			return nil
-		}
-		newState := internal.IsProcessRunning(internal.FileshareBinaryPath)
-		if newState == oldState {
-			// only state change triggers the modifications
-			return nil
-		}
-
-		log.Println(internal.InfoPrefix, "fileshare change to running", newState)
-		peers, err := s.listPeers()
-		if err != nil {
-			return err
-		}
+	job := monitorFileshareProcessJob{
+		isFileshareAllowed: false,
+		meshChecker:        s,
+		rulesController:    s.netw,
+		processChecker:     defaultProcessChecker{},
+	}
+	return job.run
+}
 
-		isFileshareUp := newState
-		for _, peer := range peers {
-			if !isFileshareUp {
-				s.netw.BlockFileshare(UniqueAddress{UID: peer.PublicKey, Address: peer.Address})
-			} else {
-				s.netw.AllowFileshare(UniqueAddress{UID: peer.PublicKey, Address: peer.Address})
+func (j *monitorFileshareProcessJob) run() error {
+	if !j.meshChecker.isMeshOn() {
+		if j.isFileshareAllowed {
+			if err := j.rulesController.ForbidFileshare(); err == nil {
+				j.isFileshareAllowed = false
 			}
 		}
-		oldState = newState
-
 		return nil
 	}
+
+	if j.processChecker.isFileshareRunning() {
+		j.rulesController.PermitFileshare()
+		j.isFileshareAllowed = true
+	} else {
+		j.rulesController.ForbidFileshare()
+		j.isFileshareAllowed = false
+	}
+
+	return nil
+}
+
+type defaultProcessChecker struct{}
+
+func (defaultProcessChecker) isFileshareRunning() bool {
+	return internal.IsProcessRunning(internal.FileshareBinaryPath)
+}
+
+type monitorFileshareProcessJob struct {
+	isFileshareAllowed bool
+	meshChecker        meshChecker
+	rulesController    rulesController
+	processChecker     processChecker
+}
+
+type meshChecker interface {
+	isMeshOn() bool
+}
+
+type rulesController interface {
+	ForbidFileshare() error
+	PermitFileshare() error
+}
+
+type processChecker interface {
+	isFileshareRunning() bool
 }

meshnet/jobs_test.go

@@ -0,0 +1,126 @@
+package meshnet
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/NordSecurity/nordvpn-linux/test/category"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestJobMonitorFileshare(t *testing.T) {
+	category.Set(t, category.Unit)
+
+	tests := []struct {
+		name                        string
+		isFileshareInitiallyAllowed bool
+		meshChecker                 meshChecker
+		processChecker              processChecker
+		wasForbidCalled             bool
+		wasPermitCalled             bool
+		shouldRulesChangeFail       bool
+		isFileshareFinallyAllowed   bool
+	}{
+		{
+			name:                        "nothing happens with disabled meshnet",
+			meshChecker:                 meshCheckerStub{isMeshnetOn: false},
+			wasForbidCalled:             false,
+			wasPermitCalled:             false,
+			isFileshareInitiallyAllowed: false,
+			isFileshareFinallyAllowed:   false,
+		},
+		{
+			name:                        "fileshare is allowed when meshnet is on and process is running",
+			meshChecker:                 meshCheckerStub{isMeshnetOn: true},
+			processChecker:              processCheckerStub{isFileshareUp: true},
+			wasForbidCalled:             false,
+			wasPermitCalled:             true,
+			isFileshareInitiallyAllowed: false,
+			isFileshareFinallyAllowed:   true,
+		},
+		{
+			name:                        "fileshare is forbidden when meshnet is on process was stopped",
+			meshChecker:                 meshCheckerStub{isMeshnetOn: true},
+			processChecker:              processCheckerStub{isFileshareUp: false},
+			wasForbidCalled:             true,
+			wasPermitCalled:             false,
+			isFileshareInitiallyAllowed: true,
+			isFileshareFinallyAllowed:   false,
+		},
+		{
+			name:                        "when mesh is off, but fileshare was allowed in previous run, now it gets blocked",
+			meshChecker:                 meshCheckerStub{isMeshnetOn: false},
+			wasForbidCalled:             true,
+			wasPermitCalled:             false,
+			isFileshareInitiallyAllowed: true,
+			isFileshareFinallyAllowed:   false,
+		},
+		{
+			name:                        "when mesh is off, fileshare was allowed in previous run, the state does not change until block succeeds",
+			meshChecker:                 meshCheckerStub{isMeshnetOn: false},
+			wasForbidCalled:             true,
+			wasPermitCalled:             false,
+			shouldRulesChangeFail:       true,
+			isFileshareInitiallyAllowed: true,
+			isFileshareFinallyAllowed:   true,
+		},
+	}
+
+	for _, tt := range tests {
+		rulesController := rulesControllerSpy{shouldFail: tt.shouldRulesChangeFail}
+		job := monitorFileshareProcessJob{
+			isFileshareAllowed: tt.isFileshareInitiallyAllowed,
+			meshChecker:        tt.meshChecker,
+			rulesController:    &rulesController,
+			processChecker:     tt.processChecker,
+		}
+		assert.Equal(t, tt.isFileshareInitiallyAllowed, job.isFileshareAllowed)
+		assert.False(t, rulesController.wasForbidCalled)
+		assert.False(t, rulesController.wasPermitCalled)
+
+		err := job.run()
+
+		assert.Nil(t, err)
+		assert.Equal(t, tt.isFileshareFinallyAllowed, job.isFileshareAllowed)
+		assert.Equal(t, tt.wasForbidCalled, rulesController.wasForbidCalled)
+		assert.Equal(t, tt.wasPermitCalled, rulesController.wasPermitCalled)
+	}
+}
+
+type meshCheckerStub struct {
+	isMeshnetOn bool
+}
+
+func (m meshCheckerStub) isMeshOn() bool {
+	return m.isMeshnetOn
+}
+
+type rulesControllerSpy struct {
+	wasForbidCalled bool
+	wasPermitCalled bool
+	shouldFail      bool
+}
+
+func (rc *rulesControllerSpy) ForbidFileshare() error {
+	rc.wasForbidCalled = true
+	if rc.shouldFail {
+		return errors.New("intentional failure for testing")
+	}
+	return nil
+}
+
+func (rc *rulesControllerSpy) PermitFileshare() error {
+	rc.wasPermitCalled = true
+	if rc.shouldFail {
+		return errors.New("intentional failure for testing")
+	}
+	return nil
+}
+
+type processCheckerStub struct {
+	isFileshareUp bool
+}
+
+func (m processCheckerStub) isFileshareRunning() bool {
+	return m.isFileshareUp
+}

meshnet/networker.go

@@ -28,8 +28,12 @@ type Networker interface {
 	BlockIncoming(UniqueAddress) error
 	// AllowFileshare creates a rule enabling fileshare port for the given address
 	AllowFileshare(UniqueAddress) error
+	// PermitFileshare creates a rules enabling fileshare port for all available peers and sets fileshare as permitted
+	PermitFileshare() error
 	// BlockFileshare removes a rule enabling fileshare port for the given address if it exists
 	BlockFileshare(UniqueAddress) error
+	// ForbidFileshare removes a rules enabling fileshare port for all available peers and sets fileshare as forbidden
+	ForbidFileshare() error
 	// ResetRouting is used when there are routing setting changes,
 	// except when routing is denied - then BlockRouting must be used. changedPeer is the peer whose routing settings
 	// changed, peers is the map of all the machine peers(including the changed peer).

meshnet/server_test.go

@@ -44,6 +44,7 @@ type meshRenewChecker struct {
 func (m meshRenewChecker) IsLoggedIn() bool {
 	return !m.IsNotLoggedIn
 }
+
 func (m meshRenewChecker) IsMFAEnabled() (bool, error) {
 	return false, nil
 }
@@ -92,6 +93,10 @@ func (n *workingNetworker) AllowFileshare(address UniqueAddress) error {
 	return nil
 }
 
+func (n *workingNetworker) PermitFileshare() error {
+	return nil
+}
+
 func (n *workingNetworker) AllowIncoming(address UniqueAddress, lanAllowed bool) error {
 	n.allowedIncoming = append(n.allowedIncoming, allowedIncoming{
 		address:    address,
@@ -111,6 +116,10 @@ func (n *workingNetworker) BlockFileshare(address UniqueAddress) error {
 	return nil
 }
 
+func (n *workingNetworker) ForbidFileshare() error {
+	return nil
+}
+
 func (n *workingNetworker) ResetRouting(changedPeer mesh.MachinePeer, peer mesh.MachinePeers) error {
 	n.resetPeers = append(n.resetPeers, changedPeer.PublicKey)