Pull request: 2875 fix client filtering settings

Merge in DNS/adguard-home from 2875-client-filtering to master Updates AdguardTeam#2875. Squashed commit of the following: commit b3b9582 Author: Eugene Burkov <[email protected]> Date: Mon May 24 14:22:29 2021 +0300 all: use atomic, log changes commit 9304d8b Author: Eugene Burkov <[email protected]> Date: Mon May 24 13:43:22 2021 +0300 all: fix client filtering settings
ZSKYC · May 24, 2021 · 1425082 · 1425082
1 parent 52e6a63
commit 1425082
Show file tree

Hide file tree

Showing 9 changed files with 57 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,10 @@ released by then.
 
 - Go 1.16 support.  v0.108.0 will require at least Go 1.17 to build.
 
+### Fixed
+
+- Incorrect client-based filtering applying logic ([#2875]).
+
 ### Removed
 
 - Go 1.15 support.

diff --git a/README.md b/README.md
@@ -317,7 +317,7 @@ Here is a link to AdGuard Home project: https://crowdin.com/project/adguard-appl
 Here's what you can also do to contribute:
 
 1. [Look for issues](https://github.com/AdguardTeam/AdGuardHome/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22+) marked as "help wanted".
-2. Actualize the list of *Blocked services*. It it can be found in [filtering/blocked.go](https://github.com/AdguardTeam/AdGuardHome/blob/master/internal/filtering/blocked.go).
+2. Actualize the list of *Blocked services*. It can be found in [filtering/blocked.go](https://github.com/AdguardTeam/AdGuardHome/blob/master/internal/filtering/blocked.go).
 3. Actualize the list of known *trackers*. It it can be found in [client/src/helpers/trackers/adguard.json](https://github.com/AdguardTeam/AdGuardHome/blob/master/client/src/helpers/trackers/adguard.json).
 4. Actualize the list of vetted *blocklists*. It it can be found in [client/src/helpers/filters/filters.json](https://github.com/AdguardTeam/AdGuardHome/blob/master/client/src/helpers/filters/filters.json).
 

diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go
@@ -68,6 +68,7 @@ func createTestServer(
 	}}
 
 	f := filtering.New(filterConf, filters)
+	f.SetEnabled(true)
 
 	snd, err := aghnet.NewSubnetDetector()
 	require.NoError(t, err)
@@ -734,10 +735,11 @@ func TestBlockedCustomIP(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, snd)
 
+	f := filtering.New(&filtering.Config{}, filters)
 	var s *Server
 	s, err = NewServer(DNSCreateParams{
 		DHCPServer:     &testDHCP{},
-		DNSFilter:      filtering.New(&filtering.Config{}, filters),
+		DNSFilter:      f,
 		SubnetDetector: snd,
 	})
 	require.NoError(t, err)
@@ -763,6 +765,7 @@ func TestBlockedCustomIP(t *testing.T) {
 	err = s.Prepare(conf)
 	require.NoError(t, err)
 
+	f.SetEnabled(true)
 	startDeferStop(t, s)
 
 	addr := s.dnsProxy.Addr(proxy.ProtoUDP)
@@ -798,6 +801,7 @@ func TestBlockedByHosts(t *testing.T) {
 			ProtectionEnabled: true,
 		},
 	}
+
 	s := createTestServer(t, &filtering.Config{}, forwardConf, nil)
 	startDeferStop(t, s)
 	addr := s.dnsProxy.Addr(proxy.ProtoUDP)

diff --git a/internal/dnsforward/filter.go b/internal/dnsforward/filter.go
@@ -38,7 +38,6 @@ func (s *Server) beforeRequestHandler(_ *proxy.Proxy, d *proxy.DNSContext) (bool
 // the client's IP address and ID, if any, from ctx.
 func (s *Server) getClientRequestFilteringSettings(ctx *dnsContext) *filtering.Settings {
 	setts := s.dnsFilter.GetConfig()
-	setts.FilteringEnabled = true
 	if s.conf.FilterHandler != nil {
 		s.conf.FilterHandler(IPFromAddr(ctx.proxyCtx.Addr), ctx.clientID, &setts)
 	}

diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go
@@ -11,6 +11,7 @@ import (
 	"runtime/debug"
 	"strings"
 	"sync"
+	"sync/atomic"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghstrings"
@@ -50,6 +51,11 @@ type Resolver interface {
 
 // Config allows you to configure DNS filtering with New() or just change variables directly.
 type Config struct {
+	// enabled is used to be returned within Settings.
+	//
+	// It is of type uint32 to be accessed by atomic.
+	enabled uint32
+
 	ParentalEnabled     bool `yaml:"parental_enabled"`
 	SafeSearchEnabled   bool `yaml:"safesearch_enabled"`
 	SafeBrowsingEnabled bool `yaml:"safebrowsing_enabled"`
@@ -118,7 +124,8 @@ type DNSFilter struct {
 	parentalUpstream     upstream.Upstream
 	safeBrowsingUpstream upstream.Upstream
 
-	Config   // for direct access by library users, even a = assignment
+	Config // for direct access by library users, even a = assignment
+	// confLock protects Config.
 	confLock sync.RWMutex
 
 	// Channel for passing data to filters-initializer goroutine
@@ -223,15 +230,26 @@ func (r Reason) In(reasons ...Reason) bool {
 	return false
 }
 
+// SetEnabled sets the status of the *DNSFilter.
+func (d *DNSFilter) SetEnabled(enabled bool) {
+	var i int32
+	if enabled {
+		i = 1
+	}
+	atomic.StoreUint32(&d.enabled, uint32(i))
+}
+
 // GetConfig - get configuration
-func (d *DNSFilter) GetConfig() Settings {
-	c := Settings{}
-	// d.confLock.RLock()
-	c.SafeSearchEnabled = d.Config.SafeSearchEnabled
-	c.SafeBrowsingEnabled = d.Config.SafeBrowsingEnabled
-	c.ParentalEnabled = d.Config.ParentalEnabled
-	// d.confLock.RUnlock()
-	return c
+func (d *DNSFilter) GetConfig() (s Settings) {
+	d.confLock.RLock()
+	defer d.confLock.RUnlock()
+
+	return Settings{
+		FilteringEnabled:    atomic.LoadUint32(&d.Config.enabled) == 1,
+		SafeSearchEnabled:   d.Config.SafeSearchEnabled,
+		SafeBrowsingEnabled: d.Config.SafeBrowsingEnabled,
+		ParentalEnabled:     d.Config.ParentalEnabled,
+	}
 }
 
 // WriteDiskConfig - write configuration

diff --git a/internal/home/control.go b/internal/home/control.go
@@ -134,7 +134,6 @@ func handleStatus(w http.ResponseWriter, _ *http.Request) {
 	}
 
 	var resp statusResponse
-
 	func() {
 		config.RLock()
 		defer config.RUnlock()

diff --git a/internal/home/controlfiltering.go b/internal/home/controlfiltering.go
@@ -351,8 +351,14 @@ func (f *Filtering) handleFilteringConfig(w http.ResponseWriter, r *http.Request
 		return
 	}
 
-	config.DNS.FilteringEnabled = req.Enabled
-	config.DNS.FiltersUpdateIntervalHours = req.Interval
+	func() {
+		config.Lock()
+		defer config.Unlock()
+
+		config.DNS.FilteringEnabled = req.Enabled
+		config.DNS.FiltersUpdateIntervalHours = req.Interval
+	}()
+
 	onConfigModified()
 	enableFilters(true)
 }
@@ -364,7 +370,6 @@ type checkHostRespRule struct {
 
 type checkHostResp struct {
 	Reason string `json:"reason"`
-
 	// FilterID is the ID of the rule's filter list.
 	//
 	// Deprecated: Use Rules[*].FilterListID.

diff --git a/internal/home/dns.go b/internal/home/dns.go
@@ -307,7 +307,6 @@ func applyAdditionalFiltering(clientAddr net.IP, clientID string, setts *filteri
 
 	setts.ClientName = c.Name
 	setts.ClientTags = c.Tags
-
 	if !c.UseOwnSettings {
 		return
 	}
@@ -319,14 +318,14 @@ func applyAdditionalFiltering(clientAddr net.IP, clientID string, setts *filteri
 }
 
 func startDNSServer() error {
-	config.Lock()
-	defer config.Unlock()
+	config.RLock()
+	defer config.RUnlock()
 
 	if isRunning() {
 		return fmt.Errorf("unable to start forwarding DNS server: Already running")
 	}
 
-	enableFilters(false)
+	enableFiltersLocked(false)
 
 	Context.clients.Start()
 

diff --git a/internal/home/filter.go b/internal/home/filter.go
@@ -664,6 +664,13 @@ func (filter *filter) Path() string {
 }
 
 func enableFilters(async bool) {
+	config.RLock()
+	defer config.RUnlock()
+
+	enableFiltersLocked(async)
+}
+
+func enableFiltersLocked(async bool) {
 	var whiteFilters []filtering.Filter
 	filters := []filtering.Filter{{
 		Data: []byte(strings.Join(config.UserRules, "\n")),
@@ -693,4 +700,6 @@ func enableFilters(async bool) {
 	if err := Context.dnsFilter.SetFilters(filters, whiteFilters, async); err != nil {
 		log.Debug("enabling filters: %s", err)
 	}
+
+	Context.dnsFilter.SetEnabled(config.DNS.FilteringEnabled)
 }