Merge pull request #4653 from pi-hole/refactor-utils-redux

Tweaks to functions in utils.sh and refactored usages in webpage.sh

advanced/Scripts/utils.sh

@@ -1,4 +1,6 @@
-#!/usr/bin/env bash
+#!/usr/bin/env sh
+# shellcheck disable=SC3043 #https://github.com/koalaman/shellcheck/wiki/SC3043#exceptions
+
 # Pi-hole: A black hole for Internet advertisements
 # (c) 2017 Pi-hole, LLC (https://pi-hole.net)
 # Network-wide ad blocking via your own hardware.
@@ -15,46 +17,74 @@
 #  - New functions must have a test added for them in test/test_any_utils.py
 
 #######################
-# Takes three arguments key, value, and file.
+# Takes either
+#   - Three arguments: file, key, and value.
+#   - Two arguments: file, and key.
+#
 # Checks the target file for the existence of the key
 #   - If it exists, it changes the value
 #   - If it does not exist, it adds the value
 #
 # Example usage:
-# addOrEditKeyValuePair "BLOCKING_ENABLED" "true" "/etc/pihole/setupVars.conf"
+# addOrEditKeyValuePair "/etc/pihole/setupVars.conf" "BLOCKING_ENABLED" "true"
 #######################
 addOrEditKeyValPair() {
-  local key="${1}"
+  local file="${1}"
-  local value="${2}"
+  local key="${2}"
-  local file="${3}"
+  local value="${3}"
+
+  if [ "${value}" != "" ]; then
+    # value has a value, so it is a key-value pair
     if grep -q "^${key}=" "${file}"; then
+      # Key already exists in file, modify the value
       sed -i "/^${key}=/c\\${key}=${value}" "${file}"
     else
+      # Key does not already exist, add it and it's value
       echo "${key}=${value}" >> "${file}"
     fi
+  else
+    # value has no value, so it is just a key. Add it if it does not already exist
+    if ! grep -q "^${key}" "${file}"; then
+      # Key does not exist, add it.
+      echo "${key}" >> "${file}"
+    fi
+  fi
+}
+
+#######################
+# Takes two arguments file, and key.
+# Deletes a key from target file
+#
+# Example usage:
+# removeKey "/etc/pihole/setupVars.conf" "PIHOLE_DNS_1"
+#######################
+removeKey() {
+  local file="${1}"
+  local key="${2}"
+  sed -i "/^${key}/d" "${file}"
 }
 
 #######################
 # returns FTL's current telnet API port
 #######################
 getFTLAPIPort(){
-  local -r FTLCONFFILE="/etc/pihole/pihole-FTL.conf"
+  local FTLCONFFILE="/etc/pihole/pihole-FTL.conf"
-  local -r DEFAULT_PORT_FILE="/run/pihole-FTL.port"
+  local DEFAULT_PORT_FILE="/run/pihole-FTL.port"
-  local -r DEFAULT_FTL_PORT=4711
+  local DEFAULT_FTL_PORT=4711
   local PORTFILE
   local ftl_api_port
 
-  if [[ -f "$FTLCONFFILE" ]]; then
+  if [ -f "$FTLCONFFILE" ]; then
     # if PORTFILE is not set in pihole-FTL.conf, use the default path
     PORTFILE="$( (grep "^PORTFILE=" $FTLCONFFILE || echo "$DEFAULT_PORT_FILE") | cut -d"=" -f2-)"
   fi
 
-  if [[ -s "$PORTFILE" ]]; then
+  if [ -s "$PORTFILE" ]; then
     # -s: FILE exists and has a size greater than zero
-    ftl_api_port=$(<"$PORTFILE")
+    ftl_api_port=$(cat "${PORTFILE}")
     # Exploit prevention: unset the variable if there is malicious content
     # Verify that the value read from the file is numeric    
-    [[ "$ftl_api_port" =~ [^[:digit:]] ]] && unset ftl_api_port
+    expr "$ftl_api_port" : "[^[:digit:]]" > /dev/null && unset ftl_api_port
   fi
 
   # echo the port found in the portfile or default to the default port

advanced/Scripts/webpage.sh

@@ -1,5 +1,7 @@
 #!/usr/bin/env bash
 # shellcheck disable=SC1090
+# shellcheck disable=SC2154
+
 
 # Pi-hole: A black hole for Internet advertisements
 # (c) 2017 Pi-hole, LLC (https://pi-hole.net)
@@ -26,6 +28,9 @@ readonly PI_HOLE_FILES_DIR="/etc/.pihole"
 PH_TEST="true"
 source "${PI_HOLE_FILES_DIR}/automated install/basic-install.sh"
 
+readonly utilsfile="/opt/pihole/utils.sh"
+source "${utilsfile}"
+
 coltable="/opt/pihole/COL_TABLE"
 if [[ -f ${coltable} ]]; then
     source ${coltable}
@@ -51,41 +56,35 @@ Options:
 }
 
 add_setting() {
-    echo "${1}=${2}" >> "${setupVars}"
+    addOrEditKeyValPair "${setupVars}" "${1}" "${2}"
 }
 
 delete_setting() {
-    sed -i "/^${1}/d" "${setupVars}"
+    removeKey "${setupVars}" "${1}"
 }
 
 change_setting() {
-    delete_setting "${1}"
+    addOrEditKeyValPair "${setupVars}" "${1}" "${2}"
-    add_setting "${1}" "${2}"
 }
 
 addFTLsetting() {
-    echo "${1}=${2}" >> "${FTLconf}"
+    addOrEditKeyValPair "${FTLconf}" "${1}" "${2}"
 }
 
 deleteFTLsetting() {
-    sed -i "/^${1}/d" "${FTLconf}"
+    removeKey "${FTLconf}" "${1}"
 }
 
 changeFTLsetting() {
-    deleteFTLsetting "${1}"
+    addOrEditKeyValPair "${FTLconf}" "${1}" "${2}"
-    addFTLsetting "${1}" "${2}"
 }
 
 add_dnsmasq_setting() {
-    if [[ "${2}" != "" ]]; then
+    addOrEditKeyValPair "${dnsmasqconfig}" "${1}" "${2}"
-        echo "${1}=${2}" >> "${dnsmasqconfig}"
-    else
-        echo "${1}" >> "${dnsmasqconfig}"
-    fi
 }
 
 delete_dnsmasq_setting() {
-    sed -i "/^${1}/d" "${dnsmasqconfig}"
+    removeKey "${dnsmasqconfig}" "${1}"
 }
 
 SetTemperatureUnit() {
@@ -183,7 +182,7 @@ ProcessDNSSettings() {
     fi
 
     delete_dnsmasq_setting "dnssec"
-    delete_dnsmasq_setting "trust-anchor="
+    delete_dnsmasq_setting "trust-anchor"
 
     if [[ "${DNSSEC}" == true ]]; then
         echo "dnssec

pihole

@@ -226,7 +226,7 @@ Time:
       fi
 
       local str="Pi-hole Disabled"
-      addOrEditKeyValPair "BLOCKING_ENABLED" "false" "${setupVars}"
+      addOrEditKeyValPair "${setupVars}" "BLOCKING_ENABLED" "false"
     fi
   else
     # Enable Pi-hole
@@ -238,7 +238,7 @@ Time:
     echo -e "  ${INFO} Enabling blocking"
     local str="Pi-hole Enabled"
 
-    addOrEditKeyValPair "BLOCKING_ENABLED" "true" "${setupVars}"
+    addOrEditKeyValPair "${setupVars}" "BLOCKING_ENABLED" "true"
   fi
 
   restartDNS reload-lists
@@ -260,8 +260,8 @@ Options:
     exit 0
   elif [[ "${1}" == "off" ]]; then
     # Disable logging
-    sed -i 's/^log-queries/#log-queries/' /etc/dnsmasq.d/01-pihole.conf
+    addOrEditKeyValPair /etc/dnsmasq.d/01-pihole.conf "log-queries"
-    addOrEditKeyValPair "QUERY_LOGGING" "false" "${setupVars}"
+    addOrEditKeyValPair "${setupVars}" "QUERY_LOGGING" "false"
     if [[ "${2}" != "noflush" ]]; then
       # Flush logs
       "${PI_HOLE_BIN_DIR}"/pihole -f
@@ -270,8 +270,8 @@ Options:
     local str="Logging has been disabled!"
   elif [[ "${1}" == "on" ]]; then
     # Enable logging
-    sed -i 's/^#log-queries/log-queries/' /etc/dnsmasq.d/01-pihole.conf
+    removeKey /etc/dnsmasq.d/01-pihole.conf "log-queries"
-    addOrEditKeyValPair "QUERY_LOGGING" "true" "${setupVars}"
+    addOrEditKeyValPair "${setupVars}" "QUERY_LOGGING" "true"
     echo -e "  ${INFO} Enabling logging..."
     local str="Logging has been enabled!"
   else

test/test_any_utils.py

@@ -2,15 +2,33 @@ def test_key_val_replacement_works(host):
     ''' Confirms addOrEditKeyValPair provides the expected output '''
     host.run('''
     source /opt/pihole/utils.sh
-    addOrEditKeyValPair "KEY_ONE" "value1" "./testoutput"
+    addOrEditKeyValPair "./testoutput" "KEY_ONE" "value1"
-    addOrEditKeyValPair "KEY_TWO" "value2" "./testoutput"
+    addOrEditKeyValPair "./testoutput" "KEY_TWO" "value2"
-    addOrEditKeyValPair "KEY_ONE" "value3" "./testoutput"
+    addOrEditKeyValPair "./testoutput" "KEY_ONE" "value3"
-    addOrEditKeyValPair "KEY_FOUR" "value4" "./testoutput"
+    addOrEditKeyValPair "./testoutput" "KEY_FOUR" "value4"
+    addOrEditKeyValPair "./testoutput" "KEY_FIVE_NO_VALUE"
+    addOrEditKeyValPair "./testoutput" "KEY_FIVE_NO_VALUE"
     ''')
     output = host.run('''
     cat ./testoutput
     ''')
-    expected_stdout = 'KEY_ONE=value3\nKEY_TWO=value2\nKEY_FOUR=value4\n'
+    expected_stdout = 'KEY_ONE=value3\nKEY_TWO=value2\nKEY_FOUR=value4\nKEY_FIVE_NO_VALUE\n'
+    assert expected_stdout == output.stdout
+
+
+def test_key_val_removal_works(host):
+    ''' Confirms removeKey provides the expected output '''
+    host.run('''
+    source /opt/pihole/utils.sh
+    addOrEditKeyValPair "./testoutput" "KEY_ONE" "value1"
+    addOrEditKeyValPair "./testoutput" "KEY_TWO" "value2"
+    addOrEditKeyValPair "./testoutput" "KEY_THREE" "value3"
+    removeKey "./testoutput" "KEY_TWO"
+    ''')
+    output = host.run('''
+    cat ./testoutput
+    ''')
+    expected_stdout = 'KEY_ONE=value1\nKEY_THREE=value3\n'
     assert expected_stdout == output.stdout