From 74dcc91ea7a4f5a4d110c157d8c079ecc2ecb70a Mon Sep 17 00:00:00 2001
From: Ainar Garipov <a.garipov@adguard.com>
Date: Tue, 28 Dec 2021 17:00:52 +0300
Subject: [PATCH] cherry-pick: all: imp uniq validation err msgs

Updates #3975.

Squashed commit of the following:

commit f8578c2afb1bb5786e7b855a1715e0757bc08510
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Dec 28 16:39:13 2021 +0300

    aghalgo: imp docs

commit d9fc625f7c4ede2cf4b0683ad5efd0ddf9b966b1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Dec 28 16:21:24 2021 +0300

    all: imp uniq validation err msgs
---
 CHANGELOG.md                     |  6 +++
 internal/aghalgo/aghalgo.go      | 75 ++++++++++++++++++++++++++++++++
 internal/dnsforward/access.go    | 68 +++++++++++------------------
 internal/home/config.go          | 24 +++++++---
 internal/home/controlinstall.go  | 18 +++++---
 internal/home/home.go            | 15 ++++---
 internal/home/portsmap.go        | 63 ---------------------------
 internal/home/service.go         |  2 +-
 internal/home/service_openbsd.go |  2 +-
 internal/home/tls.go             | 21 ++++++---
 10 files changed, 166 insertions(+), 128 deletions(-)
 create mode 100644 internal/aghalgo/aghalgo.go
 delete mode 100644 internal/home/portsmap.go

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b42f1372..5d0b2ee5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,11 @@ and this project adheres to
 
 - `windows/arm64` support ([#3057]).
 
+### Changed
+
+- The validation error message for duplicated allow- and blocklists in DNS
+  settings now shows the duplicated elements ([#3975]).
+
 ### Deprecated
 
 <!--
@@ -50,6 +55,7 @@ and this project adheres to
 - Panic on port availability check during installation ([#3987]).
 
 [#3868]: https://github.com/AdguardTeam/AdGuardHome/issues/3868
+[#3975]: https://github.com/AdguardTeam/AdGuardHome/issues/3975
 [#3987]: https://github.com/AdguardTeam/AdGuardHome/issues/3987
 [#4008]: https://github.com/AdguardTeam/AdGuardHome/issues/4008
 [#4016]: https://github.com/AdguardTeam/AdGuardHome/issues/4016
diff --git a/internal/aghalgo/aghalgo.go b/internal/aghalgo/aghalgo.go
new file mode 100644
index 00000000..3d937301
--- /dev/null
+++ b/internal/aghalgo/aghalgo.go
@@ -0,0 +1,75 @@
+// Package aghalgo contains common generic algorithms and data structures.
+//
+// TODO(a.garipov): Update to use type parameters in Go 1.18.
+package aghalgo
+
+import (
+	"fmt"
+	"sort"
+)
+
+// comparable is an alias for interface{}.  Values passed as arguments of this
+// type alias must be comparable.
+//
+// TODO(a.garipov): Remove in Go 1.18.
+type comparable = interface{}
+
+// UniquenessValidator allows validating uniqueness of comparable items.
+type UniquenessValidator map[comparable]int64
+
+// Add adds a value to the validator.  v must not be nil.
+func (v UniquenessValidator) Add(elems ...comparable) {
+	for _, e := range elems {
+		v[e]++
+	}
+}
+
+// Merge returns a validator containing data from both v and other.
+func (v UniquenessValidator) Merge(other UniquenessValidator) (merged UniquenessValidator) {
+	merged = make(UniquenessValidator, len(v)+len(other))
+	for elem, num := range v {
+		merged[elem] += num
+	}
+
+	for elem, num := range other {
+		merged[elem] += num
+	}
+
+	return merged
+}
+
+// Validate returns an error enumerating all elements that aren't unique.
+// isBefore is an optional sorting function to make the error message
+// deterministic.
+func (v UniquenessValidator) Validate(isBefore func(a, b comparable) (less bool)) (err error) {
+	var dup []comparable
+	for elem, num := range v {
+		if num > 1 {
+			dup = append(dup, elem)
+		}
+	}
+
+	if len(dup) == 0 {
+		return nil
+	}
+
+	if isBefore != nil {
+		sort.Slice(dup, func(i, j int) (less bool) {
+			return isBefore(dup[i], dup[j])
+		})
+	}
+
+	return fmt.Errorf("duplicated values: %v", dup)
+}
+
+// IntIsBefore is a helper sort function for UniquenessValidator.Validate.
+// a and b must be of type int.
+func IntIsBefore(a, b comparable) (less bool) {
+	return a.(int) < b.(int)
+}
+
+// StringIsBefore is a helper sort function for UniquenessValidator.Validate.
+// a and b must be of type string.
+func StringIsBefore(a, b comparable) (less bool) {
+	return a.(string) < b.(string)
+}
diff --git a/internal/dnsforward/access.go b/internal/dnsforward/access.go
index 79f6c2b7..f2f2df4a 100644
--- a/internal/dnsforward/access.go
+++ b/internal/dnsforward/access.go
@@ -7,8 +7,8 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghalgo"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
-	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/stringutil"
@@ -194,62 +194,46 @@ func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func isUniq(slice []string) (ok bool, uniqueMap map[string]unit) {
-	exists := make(map[string]unit)
-	for _, key := range slice {
-		if _, has := exists[key]; has {
-			return false, nil
-		}
-		exists[key] = unit{}
-	}
-	return true, exists
-}
-
-func intersect(mapA, mapB map[string]unit) bool {
-	for key := range mapA {
-		if _, has := mapB[key]; has {
-			return true
-		}
-	}
-	return false
-}
-
 // validateAccessSet checks the internal accessListJSON lists.  To search for
 // duplicates, we cannot compare the new stringutil.Set and []string, because
 // creating a set for a large array can be an unnecessary algorithmic complexity
-func validateAccessSet(list accessListJSON) (err error) {
-	const (
-		errAllowedDup    errors.Error = "duplicates in allowed clients"
-		errDisallowedDup errors.Error = "duplicates in disallowed clients"
-		errBlockedDup    errors.Error = "duplicates in blocked hosts"
-		errIntersect     errors.Error = "some items in allowed and " +
-			"disallowed lists at the same time"
-	)
-
-	ok, allowedClients := isUniq(list.AllowedClients)
-	if !ok {
-		return errAllowedDup
+func validateAccessSet(list *accessListJSON) (err error) {
+	allowed, err := validateStrUniq(list.AllowedClients)
+	if err != nil {
+		return fmt.Errorf("validating allowed clients: %w", err)
 	}
 
-	ok, disallowedClients := isUniq(list.DisallowedClients)
-	if !ok {
-		return errDisallowedDup
+	disallowed, err := validateStrUniq(list.DisallowedClients)
+	if err != nil {
+		return fmt.Errorf("validating disallowed clients: %w", err)
 	}
 
-	ok, _ = isUniq(list.BlockedHosts)
-	if !ok {
-		return errBlockedDup
+	_, err = validateStrUniq(list.BlockedHosts)
+	if err != nil {
+		return fmt.Errorf("validating blocked hosts: %w", err)
 	}
 
-	if intersect(allowedClients, disallowedClients) {
-		return errIntersect
+	merged := allowed.Merge(disallowed)
+	err = merged.Validate(aghalgo.StringIsBefore)
+	if err != nil {
+		return fmt.Errorf("items in allowed and disallowed clients intersect: %w", err)
 	}
 
 	return nil
 }
 
+// validateStrUniq returns an informative error if clients are not unique.
+func validateStrUniq(clients []string) (uv aghalgo.UniquenessValidator, err error) {
+	uv = make(aghalgo.UniquenessValidator, len(clients))
+	for _, c := range clients {
+		uv.Add(c)
+	}
+
+	return uv, uv.Validate(aghalgo.StringIsBefore)
+}
+
 func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
-	list := accessListJSON{}
+	list := &accessListJSON{}
 	err := json.NewDecoder(r.Body).Decode(&list)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusBadRequest, "decoding request: %s", err)
diff --git a/internal/home/config.go b/internal/home/config.go
index 55d884c8..d036a085 100644
--- a/internal/home/config.go
+++ b/internal/home/config.go
@@ -1,11 +1,13 @@
 package home
 
 import (
+	"fmt"
 	"net"
 	"os"
 	"path/filepath"
 	"sync"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghalgo"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
@@ -286,22 +288,25 @@ func parseConfig() (err error) {
 		return err
 	}
 
-	pm := portsMap{}
-	pm.add(
+	uv := aghalgo.UniquenessValidator{}
+	addPorts(
+		uv,
 		config.BindPort,
 		config.BetaBindPort,
 		config.DNS.Port,
 	)
+
 	if config.TLS.Enabled {
-		pm.add(
+		addPorts(
+			uv,
 			config.TLS.PortHTTPS,
 			config.TLS.PortDNSOverTLS,
 			config.TLS.PortDNSOverQUIC,
 			config.TLS.PortDNSCrypt,
 		)
 	}
-	if err = pm.validate(); err != nil {
-		return err
+	if err = uv.Validate(aghalgo.IntIsBefore); err != nil {
+		return fmt.Errorf("validating ports: %w", err)
 	}
 
 	if !checkFiltersUpdateIntervalHours(config.DNS.FiltersUpdateIntervalHours) {
@@ -315,6 +320,15 @@ func parseConfig() (err error) {
 	return nil
 }
 
+// addPorts is a helper for ports validation.  It skips zero ports.
+func addPorts(uv aghalgo.UniquenessValidator, ports ...int) {
+	for _, p := range ports {
+		if p != 0 {
+			uv.Add(p)
+		}
+	}
+}
+
 // readConfigFile reads configuration file contents.
 func readConfigFile() (fileData []byte, err error) {
 	if len(config.fileData) > 0 {
diff --git a/internal/home/controlinstall.go b/internal/home/controlinstall.go
index cc01255f..13d870da 100644
--- a/internal/home/controlinstall.go
+++ b/internal/home/controlinstall.go
@@ -14,6 +14,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghalgo"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/golibs/errors"
@@ -102,9 +103,15 @@ func (web *Web) handleInstallCheckConfig(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	pm := portsMap{}
-	pm.add(config.BindPort, config.BetaBindPort, reqData.Web.Port)
-	if err = pm.validate(); err != nil {
+	uv := aghalgo.UniquenessValidator{}
+	addPorts(
+		uv,
+		config.BindPort,
+		config.BetaBindPort,
+		reqData.Web.Port,
+	)
+	if err = uv.Validate(aghalgo.IntIsBefore); err != nil {
+		err = fmt.Errorf("validating ports: %w", err)
 		respData.Web.Status = err.Error()
 	} else if reqData.Web.Port != 0 {
 		err = aghnet.CheckPort("tcp", reqData.Web.IP, reqData.Web.Port)
@@ -113,8 +120,9 @@ func (web *Web) handleInstallCheckConfig(w http.ResponseWriter, r *http.Request)
 		}
 	}
 
-	pm.add(reqData.DNS.Port)
-	if err = pm.validate(); err != nil {
+	addPorts(uv, reqData.DNS.Port)
+	if err = uv.Validate(aghalgo.IntIsBefore); err != nil {
+		err = fmt.Errorf("validating ports: %w", err)
 		respData.DNS.Status = err.Error()
 	} else if reqData.DNS.Port != 0 {
 		err = aghnet.CheckPort("udp", reqData.DNS.IP, reqData.DNS.Port)
diff --git a/internal/home/home.go b/internal/home/home.go
index fe11a8d3..853940e2 100644
--- a/internal/home/home.go
+++ b/internal/home/home.go
@@ -19,6 +19,7 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghalgo"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
@@ -295,22 +296,24 @@ func setupConfig(args options) (err error) {
 	Context.clients.Init(config.Clients, Context.dhcpServer, Context.etcHosts)
 
 	if args.bindPort != 0 {
-		pm := portsMap{}
-		pm.add(
+		uv := aghalgo.UniquenessValidator{}
+		addPorts(
+			uv,
 			args.bindPort,
 			config.BetaBindPort,
 			config.DNS.Port,
 		)
 		if config.TLS.Enabled {
-			pm.add(
+			addPorts(
+				uv,
 				config.TLS.PortHTTPS,
 				config.TLS.PortDNSOverTLS,
 				config.TLS.PortDNSOverQUIC,
 				config.TLS.PortDNSCrypt,
 			)
 		}
-		if err = pm.validate(); err != nil {
-			return err
+		if err = uv.Validate(aghalgo.IntIsBefore); err != nil {
+			return fmt.Errorf("validating ports: %w", err)
 		}
 
 		config.BindPort = args.bindPort
@@ -374,7 +377,7 @@ func fatalOnError(err error) {
 	}
 }
 
-// run performs configurating and starts AdGuard Home.
+// run configures and starts AdGuard Home.
 func run(args options, clientBuildFS fs.FS) {
 	var err error
 
diff --git a/internal/home/portsmap.go b/internal/home/portsmap.go
deleted file mode 100644
index cfbfcde9..00000000
--- a/internal/home/portsmap.go
+++ /dev/null
@@ -1,63 +0,0 @@
-package home
-
-import (
-	"fmt"
-	"strconv"
-	"strings"
-
-	"github.com/AdguardTeam/golibs/errors"
-	"github.com/AdguardTeam/golibs/stringutil"
-)
-
-// portsMap is a helper type for mapping a network port to the number of its
-// users.
-type portsMap map[int]int
-
-// add binds each of ps.  Zeroes are skipped.
-func (pm portsMap) add(ps ...int) {
-	for _, p := range ps {
-		if p == 0 {
-			continue
-		}
-
-		pm[p]++
-	}
-}
-
-// validate returns an error about all the ports bound several times.
-func (pm portsMap) validate() (err error) {
-	overbound := []int{}
-	for p, num := range pm {
-		if num > 1 {
-			overbound = append(overbound, p)
-			pm[p] = 1
-		}
-	}
-
-	switch len(overbound) {
-	case 0:
-		return nil
-	case 1:
-		return fmt.Errorf("port %d is already used", overbound[0])
-	default:
-		b := &strings.Builder{}
-
-		// TODO(e.burkov, a.garipov):  Add JoinToBuilder helper to stringutil.
-		stringutil.WriteToBuilder(b, "ports ", strconv.Itoa(overbound[0]))
-		for _, p := range overbound[1:] {
-			stringutil.WriteToBuilder(b, ", ", strconv.Itoa(p))
-		}
-		stringutil.WriteToBuilder(b, " are already used")
-
-		return errors.Error(b.String())
-	}
-}
-
-// validatePorts is a helper function for a single-step ports binding
-// validation.
-func validatePorts(ps ...int) (err error) {
-	pm := portsMap{}
-	pm.add(ps...)
-
-	return pm.validate()
-}
diff --git a/internal/home/service.go b/internal/home/service.go
index c2073e6c..fc34fd67 100644
--- a/internal/home/service.go
+++ b/internal/home/service.go
@@ -157,7 +157,7 @@ func sendSigReload() {
 // it is specified when we register a service, and it indicates to the app
 // that it is being run as a service/daemon.
 func handleServiceControlAction(opts options, clientBuildFS fs.FS) {
-	// Call chooseSystem expicitly to introduce OpenBSD support for service
+	// Call chooseSystem explicitly to introduce OpenBSD support for service
 	// package.  It's a noop for other GOOS values.
 	chooseSystem()
 
diff --git a/internal/home/service_openbsd.go b/internal/home/service_openbsd.go
index 521e6287..679a7437 100644
--- a/internal/home/service_openbsd.go
+++ b/internal/home/service_openbsd.go
@@ -173,7 +173,7 @@ func (s *openbsdRunComService) template() (t *template.Template) {
 	)))
 }
 
-// execPath returns the absolute path to the excutable to be run as a service.
+// execPath returns the absolute path to the executable to be run as a service.
 func (s *openbsdRunComService) execPath() (path string, err error) {
 	if c := s.cfg; c != nil && len(c.Executable) != 0 {
 		return filepath.Abs(c.Executable)
diff --git a/internal/home/tls.go b/internal/home/tls.go
index 35138c34..37e03b0b 100644
--- a/internal/home/tls.go
+++ b/internal/home/tls.go
@@ -20,6 +20,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghalgo"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/golibs/errors"
@@ -250,7 +251,9 @@ func (t *TLSMod) handleTLSValidate(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if setts.Enabled {
-		if err = validatePorts(
+		uv := aghalgo.UniquenessValidator{}
+		addPorts(
+			uv,
 			config.BindPort,
 			config.BetaBindPort,
 			config.DNS.Port,
@@ -258,8 +261,11 @@ func (t *TLSMod) handleTLSValidate(w http.ResponseWriter, r *http.Request) {
 			setts.PortDNSOverTLS,
 			setts.PortDNSOverQUIC,
 			setts.PortDNSCrypt,
-		); err != nil {
-			aghhttp.Error(r, w, http.StatusBadRequest, "%s", err)
+		)
+
+		err = uv.Validate(aghalgo.IntIsBefore)
+		if err != nil {
+			aghhttp.Error(r, w, http.StatusBadRequest, "validating ports: %s", err)
 
 			return
 		}
@@ -338,7 +344,9 @@ func (t *TLSMod) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if data.Enabled {
-		if err = validatePorts(
+		uv := aghalgo.UniquenessValidator{}
+		addPorts(
+			uv,
 			config.BindPort,
 			config.BetaBindPort,
 			config.DNS.Port,
@@ -346,7 +354,10 @@ func (t *TLSMod) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
 			data.PortDNSOverTLS,
 			data.PortDNSOverQUIC,
 			data.PortDNSCrypt,
-		); err != nil {
+		)
+
+		err = uv.Validate(aghalgo.IntIsBefore)
+		if err != nil {
 			aghhttp.Error(r, w, http.StatusBadRequest, "%s", err)
 
 			return