Pull request: 3419 client allowlist collision

Updates #3419.

Squashed commit of the following:

commit 370094c00d9c15b1336fbedb1e233bd4436c9898
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Fri Sep 10 17:31:16 2021 +0300

    added link to github issue

commit 407ba9b2db46b887a30ddb081bd37c56e56b0496
Merge: 426c8146 80548233
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Fri Sep 10 17:29:52 2021 +0300

    Merge branch 'master' into 3419-client-allowlist-collision

commit 426c8146cff5c112ebb25192af276c6601200528
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Fri Sep 10 16:28:11 2021 +0300

    fix en

commit d28c6022321828c6bdc55c3f9a4f655b26d146d2
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Fri Sep 10 15:49:12 2021 +0300

    added missing space

commit b374a09327968ca5343c1595d1ab8cf317c15ffe
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Fri Sep 10 15:43:55 2021 +0300

    fixes after review

commit 2be629d66e4703e2f5a85615bf1eaaa92e03c6fd
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Thu Sep 9 14:17:19 2021 +0300

    fixes

commit 5c2aa6201cc0ecf404d4057e354fbb0bdadcdd6d
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Wed Sep 8 15:04:30 2021 +0300

    return empty line to locale file

commit 3631c3772babbd595b1c3de4a7e91be6bac3e80f
Author: Dmitriy Seregin <d.seregin@adguard.com>
Date:   Wed Sep 8 13:57:51 2021 +0300

    all: fix collisions in access lists && expand block/unblock client

CHANGELOG.md

@@ -46,6 +46,8 @@ and this project adheres to
 
 ### Changed
 
+- Items in allowed clients, disallowed clients, and blocked hosts lists must
+  be unique ([#3419]).
 - The TLS private key previously saved as a string isn't shown in API responses
   any more ([#1898]).
 - Better OpenWrt detection ([#3435]).
@@ -178,6 +180,7 @@ In this release, the schema version has changed from 10 to 12.
 [#3351]: https://github.com/AdguardTeam/AdGuardHome/issues/3351
 [#3372]: https://github.com/AdguardTeam/AdGuardHome/issues/3372
 [#3417]: https://github.com/AdguardTeam/AdGuardHome/issues/3417
+[#3419]: https://github.com/AdguardTeam/AdGuardHome/issues/3419
 [#3435]: https://github.com/AdguardTeam/AdGuardHome/issues/3435
 [#3437]: https://github.com/AdguardTeam/AdGuardHome/issues/3437
 [#3443]: https://github.com/AdguardTeam/AdGuardHome/issues/3443

client/src/__locales/en.json

@@ -613,7 +613,8 @@
     "click_to_view_queries": "Click to view queries",
     "port_53_faq_link": "Port 53 is often occupied by \"DNSStubListener\" or \"systemd-resolved\" services. Please read <0>this instruction</0> on how to resolve this.",
     "adg_will_drop_dns_queries": "AdGuard Home will be dropping all DNS queries from this client.",
-    "client_not_in_allowed_clients": "The client is not allowed because it is not in the \"Allowed clients\" list.",
+    "filter_allowlist": "WARNING: This action also will exclude the rule \"{{disallowed_rule}}\" from the list of allowed clients.",
+    "last_rule_in_allowlist": "Cannot disallow this client because excluding the rule \"{{disallowed_rule}}\" will DISABLE \"Allowed clients\" list.",
     "experimental": "Experimental",
     "use_saved_key": "Use the previously saved key"
 }

client/src/actions/access.js

@@ -52,25 +52,34 @@ export const toggleClientBlock = (ip, disallowed, disallowed_rule) => async (dis
     dispatch(toggleClientBlockRequest());
     try {
         const accessList = await apiClient.getAccessList();
-        const allowed_clients = accessList.allowed_clients ?? [];
         const blocked_hosts = accessList.blocked_hosts ?? [];
-        const disallowed_clients = accessList.disallowed_clients ?? [];
+        let allowed_clients = accessList.allowed_clients ?? [];
-
+        let disallowed_clients = accessList.disallowed_clients ?? [];
-        const updatedDisallowedClients = disallowed
-            ? disallowed_clients.filter((client) => client !== disallowed_rule)
-            : disallowed_clients.concat(ip);
 
+        if (disallowed) {
+            if (!disallowed_rule) {
+                allowed_clients = allowed_clients.concat(ip);
+            } else {
+                disallowed_clients = disallowed_clients
+                    .filter((client) => client !== disallowed_rule);
+            }
+        } else if (allowed_clients.length > 1) {
+            allowed_clients = allowed_clients
+                .filter((client) => client !== disallowed_rule);
+        } else {
+            disallowed_clients = disallowed_clients.concat(ip);
+        }
         const values = {
             allowed_clients,
             blocked_hosts,
-            disallowed_clients: updatedDisallowedClients,
+            disallowed_clients,
         };
 
         await apiClient.setAccessList(values);
         dispatch(toggleClientBlockSuccess(values));
 
         if (disallowed) {
-            dispatch(addSuccessToast(i18next.t('client_unblocked', { ip: disallowed_rule })));
+            dispatch(addSuccessToast(i18next.t('client_unblocked', { ip: disallowed_rule || ip })));
         } else {
             dispatch(addSuccessToast(i18next.t('client_blocked', { ip })));
         }

client/src/components/Dashboard/Clients.js

@@ -38,15 +38,23 @@ const renderBlockingButton = (ip, disallowed, disallowed_rule) => {
     const dispatch = useDispatch();
     const { t } = useTranslation();
     const processingSet = useSelector((state) => state.access.processingSet);
+    const allowedСlients = useSelector((state) => state.access.allowed_clients, shallowEqual);
 
     const buttonClass = classNames('button-action button-action--main', {
         'button-action--unblock': disallowed,
     });
 
     const toggleClientStatus = async (ip, disallowed, disallowed_rule) => {
-        const confirmMessage = disallowed
+        let confirmMessage;
-            ? t('client_confirm_unblock', { ip: disallowed_rule })
+
-            : `${t('adg_will_drop_dns_queries')} ${t('client_confirm_block', { ip })}`;
+        if (disallowed) {
+            confirmMessage = t('client_confirm_unblock', { ip: disallowed_rule || ip });
+        } else {
+            confirmMessage = `${t('adg_will_drop_dns_queries')} ${t('client_confirm_block', { ip })}`;
+            if (allowedСlients.length > 0) {
+                confirmMessage = confirmMessage.concat(`\n\n${t('filter_allowlist', { disallowed_rule })}`);
+            }
+        }
 
         if (window.confirm(confirmMessage)) {
             await dispatch(toggleClientBlock(ip, disallowed, disallowed_rule));
@@ -58,15 +66,16 @@ const renderBlockingButton = (ip, disallowed, disallowed_rule) => {
 
     const text = disallowed ? BLOCK_ACTIONS.UNBLOCK : BLOCK_ACTIONS.BLOCK;
 
-    const isNotInAllowedList = disallowed && disallowed_rule === '';
+    const lastRuleInAllowlist = !disallowed && allowedСlients === disallowed_rule;
+    const disabled = processingSet || lastRuleInAllowlist;
     return (
         <div className="table__action pl-4">
             <button
                 type="button"
                 className={buttonClass}
-                onClick={isNotInAllowedList ? undefined : onClick}
+                onClick={onClick}
-                disabled={isNotInAllowedList || processingSet}
+                disabled={disabled}
-                title={t(isNotInAllowedList ? 'client_not_in_allowed_clients' : text)}
+                title={lastRuleInAllowlist ? t('last_rule_in_allowlist', { disallowed_rule }) : ''}
             >
                 <Trans>{text}</Trans>
             </button>

client/src/components/Logs/Cells/ClientCell.js

@@ -28,6 +28,8 @@ const ClientCell = ({
     const autoClients = useSelector((state) => state.dashboard.autoClients, shallowEqual);
     const processingRules = useSelector((state) => state.filtering.processingRules);
     const isDetailed = useSelector((state) => state.queryLogs.isDetailed);
+    const processingSet = useSelector((state) => state.access.processingSet);
+    const allowedСlients = useSelector((state) => state.access.allowed_clients, shallowEqual);
     const [isOptionsOpened, setOptionsOpened] = useState(false);
 
     const autoClient = autoClients.find((autoClient) => autoClient.name === client);
@@ -71,11 +73,12 @@ const ClientCell = ({
         const {
             confirmMessage,
             buttonKey: blockingClientKey,
-            isNotInAllowedList,
+            lastRuleInAllowlist,
         } = getBlockClientInfo(
             client,
             client_info?.disallowed || false,
             client_info?.disallowed_rule || '',
+            allowedСlients,
         );
 
         const blockingForClientKey = isFiltered ? 'unblock_for_this_client_only' : 'block_for_this_client_only';
@@ -100,7 +103,7 @@ const ClientCell = ({
                         await dispatch(updateLogs());
                     }
                 },
-                disabled: isNotInAllowedList,
+                disabled: processingSet || lastRuleInAllowlist,
             },
         ];
 

client/src/components/Logs/Cells/helpers/index.js

@@ -2,17 +2,24 @@ import i18next from 'i18next';
 
 export const BUTTON_PREFIX = 'btn_';
 
-export const getBlockClientInfo = (ip, disallowed, disallowed_rule) => {
+export const getBlockClientInfo = (ip, disallowed, disallowed_rule, allowedСlients) => {
-    const confirmMessage = disallowed
+    let confirmMessage;
-        ? i18next.t('client_confirm_unblock', { ip: disallowed_rule })
+
-        : `${i18next.t('adg_will_drop_dns_queries')} ${i18next.t('client_confirm_block', { ip })}`;
+    if (disallowed) {
+        confirmMessage = i18next.t('client_confirm_unblock', { ip: disallowed_rule || ip });
+    } else {
+        confirmMessage = `${i18next.t('adg_will_drop_dns_queries')} ${i18next.t('client_confirm_block', { ip })}`;
+        if (allowedСlients.length > 0) {
+            confirmMessage = confirmMessage.concat(`\n\n${i18next.t('filter_allowlist', { disallowed_rule })}`);
+        }
+    }
 
     const buttonKey = i18next.t(disallowed ? 'allow_this_client' : 'disallow_this_client');
-    const isNotInAllowedList = disallowed && disallowed_rule === '';
+    const lastRuleInAllowlist = !disallowed && allowedСlients === disallowed_rule;
 
     return {
         confirmMessage,
         buttonKey,
-        isNotInAllowedList,
+        lastRuleInAllowlist,
     };
 };

client/src/components/Logs/Cells/index.js

@@ -50,6 +50,8 @@ const Row = memo(({
     const filters = useSelector((state) => state.filtering.filters, shallowEqual);
     const whitelistFilters = useSelector((state) => state.filtering.whitelistFilters, shallowEqual);
     const autoClients = useSelector((state) => state.dashboard.autoClients, shallowEqual);
+    const processingSet = useSelector((state) => state.access.processingSet);
+    const allowedСlients = useSelector((state) => state.access.allowed_clients, shallowEqual);
 
     const clients = useSelector((state) => state.dashboard.clients);
 
@@ -104,11 +106,12 @@ const Row = memo(({
         const {
             confirmMessage,
             buttonKey: blockingClientKey,
-            isNotInAllowedList,
+            lastRuleInAllowlist,
         } = getBlockClientInfo(
             client,
             client_info?.disallowed || false,
             client_info?.disallowed_rule || '',
+            allowedСlients,
         );
 
         const blockingForClientKey = isFiltered ? 'unblock_for_this_client_only' : 'block_for_this_client_only';
@@ -147,7 +150,7 @@ const Row = memo(({
         const blockClientButton = <button
                 className='text-center font-weight-bold py-2 button-action--arrow-option'
                 onClick={onBlockingClientClick}
-                disabled={isNotInAllowedList}>
+                disabled={processingSet || lastRuleInAllowlist}>
             {t(blockingClientKey)}
         </button>;
 

client/src/components/Logs/index.js

@@ -15,6 +15,7 @@ import Disabled from './Disabled';
 import { getFilteringStatus } from '../../actions/filtering';
 import { getClients } from '../../actions';
 import { getDnsConfig } from '../../actions/dnsConfig';
+import { getAccessList } from '../../actions/access';
 import {
     getLogsConfig,
     resetFilteredLogs,
@@ -126,6 +127,7 @@ const Logs = () => {
                 await Promise.all([
                     dispatch(getLogsConfig()),
                     dispatch(getDnsConfig()),
+                    dispatch(getAccessList()),
                 ]);
             } catch (err) {
                 console.error(err);

client/src/components/Settings/Dns/Access/Form.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import { Field, reduxForm } from 'redux-form';
+import { connect } from 'react-redux';
+import { Field, reduxForm, formValueSelector } from 'redux-form';
 import { Trans, withTranslation } from 'react-i18next';
 import flow from 'lodash/flow';
 import { renderTextareaField } from '../../../../helpers/form';
@@ -31,16 +32,20 @@ const fields = [
     },
 ];
 
-const Form = (props) => {
+let Form = (props) => {
     const {
-        handleSubmit, submitting, invalid, processingSet,
+        allowedClients, handleSubmit, submitting, invalid, processingSet,
     } = props;
 
     const renderField = ({
-        id, title, subtitle, disabled = processingSet, normalizeOnBlur,
+        id, title, subtitle, disabled = false, processingSet, normalizeOnBlur,
     }) => <div key={id} className="form__group mb-5">
         <label className="form__label form__label--with-desc" htmlFor={id}>
             <Trans>{title}</Trans>
+            {disabled && <>
+                <span> </span>
+                (<Trans>disabled</Trans>)
+            </>}
         </label>
         <div className="form__desc form__desc--top">
             <Trans>{subtitle}</Trans>
@@ -51,7 +56,7 @@ const Form = (props) => {
             component={renderTextareaField}
             type="text"
             className="form-control form-control--textarea font-monospace"
-            disabled={disabled}
+            disabled={disabled || processingSet}
             normalizeOnBlur={normalizeOnBlur}
         />
     </div>;
@@ -66,7 +71,15 @@ const Form = (props) => {
 
     return (
         <form onSubmit={handleSubmit}>
-            {fields.map(renderField)}
+            {
+                fields.map((f) => {
+                    const props = { ...f };
+                    if (allowedClients && f.id === 'disallowed_clients') {
+                        props.disabled = true;
+                    }
+                    return renderField(props);
+                })
+            }
             <div className="card-actions">
                 <div className="btn-list">
                     <button
@@ -90,6 +103,21 @@ Form.propTypes = {
     processingSet: PropTypes.bool.isRequired,
     t: PropTypes.func.isRequired,
     textarea: PropTypes.bool,
+    allowedClients: PropTypes.string,
 };
 
-export default flow([withTranslation(), reduxForm({ form: FORM_NAME.ACCESS })])(Form);
+const selector = formValueSelector(FORM_NAME.ACCESS);
+
+Form = connect((state) => {
+    const allowedClients = selector(state, 'allowed_clients');
+    return {
+        allowedClients,
+    };
+})(Form);
+
+export default flow([
+    withTranslation(),
+    reduxForm({
+        form: FORM_NAME.ACCESS,
+    }),
+])(Form);

internal/dnsforward/access.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/stringutil"
@@ -192,6 +193,60 @@ 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
+	}
+
+	ok, disallowedClients := isUniq(list.DisallowedClients)
+	if !ok {
+		return errDisallowedDup
+	}
+
+	ok, _ = isUniq(list.BlockedHosts)
+	if !ok {
+		return errBlockedDup
+	}
+
+	if intersect(allowedClients, disallowedClients) {
+		return errIntersect
+	}
+
+	return nil
+}
+
 func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
 	list := accessListJSON{}
 	err := json.NewDecoder(r.Body).Decode(&list)
@@ -201,6 +256,13 @@ func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	err = validateAccessSet(list)
+	if err != nil {
+		httpError(r, w, http.StatusBadRequest, err.Error())
+
+		return
+	}
+
 	var a *accessCtx
 	a, err = newAccessCtx(list.AllowedClients, list.DisallowedClients, list.BlockedHosts)
 	if err != nil {

internal/home/clients.go

@@ -285,39 +285,70 @@ func toQueryLogWHOIS(wi *RuntimeClientWHOISInfo) (cw *querylog.ClientWHOIS) {
 	}
 }
 
-// findMultiple is a wrapper around Find to make it a valid client finder for
+// findMultiple returns info about client.  If no information about the client
-// the query log.  err is always nil.
+// is found, it sends the client by default only with the "Disallowed" field
+// filled in.  err is always nil.
 func (clients *clientsContainer) findMultiple(ids []string) (c *querylog.Client, err error) {
-	for _, id := range ids {
+	var emptyClient *querylog.Client
-		var name string
+
-		whois := &querylog.ClientWHOIS{}
+	for _, id := range ids {
-		ip := net.ParseIP(id)
+		ip := net.ParseIP(id)
+		disallowed, disallowedRule := clients.dnsServer.IsBlockedClient(ip, id)
+
+		client := clients.clientInfo(ip, id, disallowed, disallowedRule)
+
+		if client.Name == "" && client.DisallowedRule == "" {
+			emptyClient = client
 
-		c, ok := clients.Find(id)
-		if ok {
-			name = c.Name
-		} else if ip != nil {
-			var rc *RuntimeClient
-			rc, ok = clients.FindRuntimeClient(ip)
-			if !ok {
 			continue
 		}
 
-			name = rc.Host
+		return client, nil
-			whois = toQueryLogWHOIS(rc.WHOISInfo)
 	}
 
-		disallowed, disallowedRule := clients.dnsServer.IsBlockedClient(ip, id)
+	return emptyClient, nil
+}
 
+// clientInfo is a wrapper around Find to make it a valid client finder for
+// the query log.
+func (clients *clientsContainer) clientInfo(
+	ip net.IP,
+	id string,
+	disallowed bool,
+	rule string,
+) (c *querylog.Client) {
+	whois := &querylog.ClientWHOIS{}
+	client, ok := clients.Find(id)
+	if ok {
 		return &querylog.Client{
-			Name:           name,
+			Name:           client.Name,
-			DisallowedRule: disallowedRule,
+			DisallowedRule: rule,
 			WHOIS:          whois,
 			Disallowed:     disallowed,
-		}, nil
+		}
 	}
 
-	return nil, nil
+	if ip == nil {
+		return nil
+	}
+
+	var rc *RuntimeClient
+	rc, ok = clients.FindRuntimeClient(ip)
+	if ok {
+		return &querylog.Client{
+			Name:           rc.Host,
+			DisallowedRule: rule,
+			WHOIS:          toQueryLogWHOIS(rc.WHOISInfo),
+			Disallowed:     disallowed,
+		}
+	}
+
+	return &querylog.Client{
+		Name:           "",
+		DisallowedRule: rule,
+		WHOIS:          &querylog.ClientWHOIS{},
+		Disallowed:     disallowed,
+	}
 }
 
 func (clients *clientsContainer) Find(id string) (c *Client, ok bool) {

openapi/CHANGELOG.md

@@ -4,12 +4,25 @@
 
 ## v0.107: API changes
 
+### New possible value of `"name"` field in `QueryLogItemClient`
+
+* The value of `"name"` field in `GET /control/querylog` method is never empty:
+  either persistent client's name or runtime client's hostname.
+
+### Lists in `AccessList`
+
+* Fields `"allowed_clients"`, `"disallowed_clients"` and `"blocked_hosts"` in
+  `POST /access/set` now should contain only unique elements.
+
+* Fields `"allowed_clients"` and `"disallowed_clients"` cannot contain the same
+  elements.
+
 ### The new field `"private_key_saved"` in `TlsConfig`
 
 * The new field `"private_key_saved"` in `POST /control/tls/configure`,
-`POST /control/tls/validate` and `GET /control/tls/status` is true if the
+  `POST /control/tls/validate` and `GET /control/tls/status` is true if the
-private key was previously saved as a string and now the private key omitted
+  private key was previously saved as a string and now the private key omitted
-from communication between server and client due to security issues.
+  from communication between server and client due to security issues.
 
 ### The new field `"cache_optimistic"` in DNS configuration
 

openapi/openapi.yaml

@@ -1962,8 +1962,7 @@
             The rule due to which the client is allowed or blocked.
         'name':
           'description': >
-            Persistent client's name or an empty string if this is a runtime
+            Persistent client's name or runtime client's hostname.
-            client.
           'type': 'string'
         'whois':
           '$ref': '#/components/schemas/QueryLogItemClientWhois'
@@ -2363,7 +2362,10 @@
     'AccessSetRequest':
       '$ref': '#/components/schemas/AccessList'
     'AccessList':
-      'description': 'Client and host access list'
+      'description': >
+        Client and host access list.  Each of the lists should contain only
+        unique elements.  In addition, allowed and disallowed lists cannot
+        contain the same elements.
       'properties':
         'allowed_clients':
           'description': >