From da8f0d1c31ebb1329cdfb11aa1fdb604f010e333 Mon Sep 17 00:00:00 2001
From: Matt Visnovsky <matt@visnovsky.us>
Date: Wed, 8 May 2024 10:06:20 -0600
Subject: [PATCH] Apply suggestions from code review

Co-authored-by: Frank Elsinga <frank@elsinga.de>
---
 .../2024-04-26-0000-snmp-monitor.js           | 10 ++++----
 server/monitor-types/snmp.js                  |  6 ++---
 src/pages/EditMonitor.vue                     | 24 +++++++++----------
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/db/knex_migrations/2024-04-26-0000-snmp-monitor.js b/db/knex_migrations/2024-04-26-0000-snmp-monitor.js
index 8e553de7..988828c0 100644
--- a/db/knex_migrations/2024-04-26-0000-snmp-monitor.js
+++ b/db/knex_migrations/2024-04-26-0000-snmp-monitor.js
@@ -1,11 +1,11 @@
 exports.up = function (knex) {
     return knex.schema
         .alterTable("monitor", function (table) {
-            table.string("snmp_community_string", 255).defaultTo("public"); // Add snmp_community_string column
-            table.string("snmp_oid").defaultTo(null); // Add oid column
-            table.enum("snmp_version", [ "0", "1", "3" ]).defaultTo("1"); // Add snmp_version column with enum values (0: SNMPv1, 1: SNMPv2c, 3: SNMPv3)
-            table.float("snmp_control_value").defaultTo(null); // Add control_value column as float
-            table.string("snmp_condition").defaultTo(null); // Add oid column
+            table.string("snmp_community_string", 255).defaultTo("public");
+            table.string("snmp_oid").defaultTo(null);
+            table.enum("snmp_version", [ "1", "2c", "3" ]).defaultTo("2c");
+            table.float("snmp_control_value").defaultTo(null);
+            table.string("snmp_condition").defaultTo(null);
         });
 };
 
diff --git a/server/monitor-types/snmp.js b/server/monitor-types/snmp.js
index ab14bb07..920e1fe2 100644
--- a/server/monitor-types/snmp.js
+++ b/server/monitor-types/snmp.js
@@ -14,7 +14,7 @@ class SNMPMonitorType extends MonitorType {
             port: monitor.port || "161",
             retries: monitor.maxretries,
             timeout: monitor.timeout * 1000,
-            version: parseInt(monitor.snmpVersion),
+            version: snmp.Version[monitor.snmpVersion],
         };
 
         let session;
@@ -37,14 +37,12 @@ class SNMPMonitorType extends MonitorType {
             });
             log.debug("monitor", `SNMP: Received varbinds (Type: ${snmp.ObjectType[varbinds[0].type]} Value: ${varbinds[0].value}`);
 
-            // Verify if any varbinds were returned from the SNMP session.
             if (varbinds.length === 0) {
                 throw new Error(`No varbinds returned from SNMP session (OID: ${monitor.snmpOid})`);
             }
 
-            // Check if the varbind type indicates a non-existent instance.
             if (varbinds[0].type === snmp.ObjectType.NoSuchInstance) {
-                throw new Error(`The SNMP query was successful but no varbinds returned for OID: ${monitor.snmpOid}`);
+                throw new Error(`The SNMP query returned that no instance exists for OID ${monitor.snmpOid}`);
             }
 
             // We restrict querying to one OID per monitor, therefore `varbinds[0]` will always contain the value we're interested in.
diff --git a/src/pages/EditMonitor.vue b/src/pages/EditMonitor.vue
index b8b79324..83e38356 100644
--- a/src/pages/EditMonitor.vue
+++ b/src/pages/EditMonitor.vue
@@ -265,18 +265,18 @@
                             <!-- SNMP Monitor Type -->
                             <div v-if="monitor.type === 'snmp'" class="my-3">
                                 <label for="snmp_community_string" class="form-label">{{ $t("Community String") }}</label>
-                                <input id="snmp_community_string" v-model="monitor.snmpCommunityString" type="text" class="form-control" required placeholder="public">
+                                <!-- TODO: Rename monitor.radiusPassword to monitor.password for general use -->
+                                <HiddenInput id="snmp_community_string" v-model="monitor.radiusPassword" autocomplete="false" required="true" placeholder="public"></HiddenInput>
 
-                                <!-- eslint-disable-next-line vue/no-v-html -->
-                                <div class="form-text" v-html="$t('snmpCommunityStringHelptext')"></div>
+                                <div class="form-text">{{ $t('snmpCommunityStringHelptext')
+ }}</div>
                             </div>
 
                             <div v-if="monitor.type === 'snmp'" class="my-3">
                                 <label for="snmp_oid" class="form-label">{{ $t("OID (Object Identifier)") }}</label>
                                 <input id="snmp_oid" v-model="monitor.snmpOid" :title="$t('Please enter a valid OID.') + ' ' + $t('Example:', ['1.3.6.1.4.1.9.6.1.101'])" type="text" class="form-control" pattern="^([0-2])((\.0)|(\.[1-9][0-9]*))*$" placeholder="1.3.6.1.4.1.9.6.1.101" required>
-
-                                <!-- eslint-disable-next-line vue/no-v-html -->
-                                <div class="form-text" v-html="$t('snmpOIDHelptext')"></div>
+                                <div class="form-text">{{ 
+$t('snmpOIDHelptext') }}</div>
                             </div>
 
                             <div v-if="monitor.type === 'snmp'" class="my-3">
@@ -303,10 +303,10 @@
                             <div v-if="monitor.type === 'snmp'" class="my-3">
                                 <label for="snmp_version" class="form-label">{{ $t("SNMP Version") }}</label>
                                 <select id="snmp_version" v-model="monitor.snmpVersion" class="form-select">
-                                    <option value="0">
+                                    <option value="1">
                                         SNMPv1
                                     </option>
-                                    <option value="1">
+                                    <option value="2c">
                                         SNMPv2c
                                     </option>
                                 </select>
@@ -1323,13 +1323,11 @@ message HealthCheckResponse {
                 }
             }
 
-            // Set a default timeout of 1 second for SNMP monitors when querying a single OID.
-            // Since we're only querying a single OID, a shorter timeout is sufficient to ensure timely responses
-            // without unnecessary delays. This helps keep the monitoring process lightweight and efficient.
             if (this.monitor.type === "snmp") {
-                this.monitor.timeout = 1;
+                // snmp is not expected to be executed via the internet => we can choose a lower default timeout
+                this.monitor.timeout = 5;
             } else {
-                this.monitor.timeout = 48; // Default timeout for other monitor types
+                this.monitor.timeout = 48;
             }
 
             // Set default SNMP version