From aca37a38e79ba4dde5c50e834674eb892db4f333 Mon Sep 17 00:00:00 2001
From: mmetc <92726601+mmetc@users.noreply.github.com>
Date: Thu, 10 Mar 2022 13:55:25 +0100
Subject: [PATCH] return 1 with incomplete command line; always check error
 when calling cmd.Help (#1335)

---
 cmd/crowdsec-cli/alerts.go     | 10 ++++-----
 cmd/crowdsec-cli/decisions.go  |  9 ++++----
 cmd/crowdsec-cli/explain.go    |  2 +-
 cmd/crowdsec-cli/hubtest.go    |  2 +-
 cmd/crowdsec-cli/machines.go   | 10 ++-------
 cmd/crowdsec-cli/simulation.go |  4 ++--
 cmd/crowdsec-cli/utils.go      |  8 +++++++
 tests/bats/90_decisions.bats   | 39 ++++++++++++++++++++++++++++++++++
 8 files changed, 62 insertions(+), 22 deletions(-)
 create mode 100644 tests/bats/90_decisions.bats

diff --git a/cmd/crowdsec-cli/alerts.go b/cmd/crowdsec-cli/alerts.go
index 7a3316a15..dc918b0b5 100644
--- a/cmd/crowdsec-cli/alerts.go
+++ b/cmd/crowdsec-cli/alerts.go
@@ -252,7 +252,7 @@ cscli alerts list --type ban`,
 
 			if err := manageCliDecisionAlerts(alertListFilter.IPEquals, alertListFilter.RangeEquals,
 				alertListFilter.ScopeEquals, alertListFilter.ValueEquals); err != nil {
-				_ = cmd.Help()
+				printHelp(cmd)
 				log.Fatalf("%s", err)
 			}
 			if limit != nil {
@@ -267,7 +267,7 @@ cscli alerts list --type ban`,
 					realDuration := strings.TrimSuffix(*alertListFilter.Until, "d")
 					days, err := strconv.Atoi(realDuration)
 					if err != nil {
-						cmd.Help()
+						printHelp(cmd)
 						log.Fatalf("Can't parse duration %s, valid durations format: 1d, 4h, 4h15m", *alertListFilter.Until)
 					}
 					*alertListFilter.Until = fmt.Sprintf("%d%s", days*24, "h")
@@ -281,7 +281,7 @@ cscli alerts list --type ban`,
 					realDuration := strings.TrimSuffix(*alertListFilter.Since, "d")
 					days, err := strconv.Atoi(realDuration)
 					if err != nil {
-						cmd.Help()
+						printHelp(cmd)
 						log.Fatalf("Can't parse duration %s, valid durations format: 1d, 4h, 4h15m", *alertListFilter.Since)
 					}
 					*alertListFilter.Since = fmt.Sprintf("%d%s", days*24, "h")
@@ -368,7 +368,7 @@ cscli alerts delete -s crowdsecurity/ssh-bf"`,
 			if !AlertDeleteAll {
 				if err := manageCliDecisionAlerts(alertDeleteFilter.IPEquals, alertDeleteFilter.RangeEquals,
 					alertDeleteFilter.ScopeEquals, alertDeleteFilter.ValueEquals); err != nil {
-					_ = cmd.Help()
+					printHelp(cmd)
 					log.Fatalf("%s", err)
 				}
 				if ActiveDecision != nil {
@@ -424,7 +424,7 @@ cscli alerts delete -s crowdsecurity/ssh-bf"`,
 		DisableAutoGenTag: true,
 		Run: func(cmd *cobra.Command, args []string) {
 			if len(args) == 0 {
-				_ = cmd.Help()
+				printHelp(cmd)
 				return
 			}
 			for _, alertID := range args {
diff --git a/cmd/crowdsec-cli/decisions.go b/cmd/crowdsec-cli/decisions.go
index 3d200b3af..93e08df6c 100644
--- a/cmd/crowdsec-cli/decisions.go
+++ b/cmd/crowdsec-cli/decisions.go
@@ -200,7 +200,7 @@ cscli decisions list -t ban
 					realDuration := strings.TrimSuffix(*filter.Until, "d")
 					days, err := strconv.Atoi(realDuration)
 					if err != nil {
-						cmd.Help()
+						printHelp(cmd)
 						log.Fatalf("Can't parse duration %s, valid durations format: 1d, 4h, 4h15m", *filter.Until)
 					}
 					*filter.Until = fmt.Sprintf("%d%s", days*24, "h")
@@ -214,7 +214,7 @@ cscli decisions list -t ban
 					realDuration := strings.TrimSuffix(*filter.Since, "d")
 					days, err := strconv.Atoi(realDuration)
 					if err != nil {
-						cmd.Help()
+						printHelp(cmd)
 						log.Fatalf("Can't parse duration %s, valid durations format: 1d, 4h, 4h15m", *filter.Until)
 					}
 					*filter.Since = fmt.Sprintf("%d%s", days*24, "h")
@@ -325,9 +325,8 @@ cscli decisions add --scope username --value foobar
 				addValue = addRange
 				addScope = types.Range
 			} else if addValue == "" {
-				cmd.Help()
-				log.Errorf("Missing arguments, a value is required (--ip, --range or --scope and --value)")
-				return
+				printHelp(cmd)
+				log.Fatalf("Missing arguments, a value is required (--ip, --range or --scope and --value)")
 			}
 
 			if addReason == "" {
diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go
index 14c2faed7..f646ab3f5 100644
--- a/cmd/crowdsec-cli/explain.go
+++ b/cmd/crowdsec-cli/explain.go
@@ -36,7 +36,7 @@ cscli explain --dsn "file://myfile.log" --type nginx
 		Run: func(cmd *cobra.Command, args []string) {
 
 			if logType == "" || (logLine == "" && logFile == "" && dsn == "") {
-				cmd.Help()
+				printHelp(cmd)
 				fmt.Println()
 				fmt.Printf("Please provide --type flag\n")
 				os.Exit(1)
diff --git a/cmd/crowdsec-cli/hubtest.go b/cmd/crowdsec-cli/hubtest.go
index 8243a5dd9..d554172e4 100644
--- a/cmd/crowdsec-cli/hubtest.go
+++ b/cmd/crowdsec-cli/hubtest.go
@@ -165,7 +165,7 @@ cscli hubtest create my-scenario-test --parsers crowdsecurity/nginx --scenarios
 		DisableAutoGenTag: true,
 		Run: func(cmd *cobra.Command, args []string) {
 			if !runAll && len(args) == 0 {
-				cmd.Help()
+				printHelp(cmd)
 				fmt.Println("Please provide test to run or --all flag")
 				os.Exit(1)
 			}
diff --git a/cmd/crowdsec-cli/machines.go b/cmd/crowdsec-cli/machines.go
index 51120110c..1c95562a8 100644
--- a/cmd/crowdsec-cli/machines.go
+++ b/cmd/crowdsec-cli/machines.go
@@ -194,10 +194,7 @@ cscli machines add MyTestMachine --password MyPassword
 			// create machineID if not specified by user
 			if len(args) == 0 {
 				if !autoAdd {
-					err = cmd.Help()
-					if err != nil {
-						log.Fatalf("unable to print help(): %s", err)
-					}
+					printHelp(cmd)
 					return
 				}
 				machineID, err = generateID()
@@ -218,10 +215,7 @@ cscli machines add MyTestMachine --password MyPassword
 			// create password if doesn't specified by user
 			if machinePassword == "" && !interactive {
 				if !autoAdd {
-					err = cmd.Help()
-					if err != nil {
-						log.Fatalf("unable to print help(): %s", err)
-					}
+					printHelp(cmd)
 					return
 				}
 				machinePassword = generatePassword(passwordLength)
diff --git a/cmd/crowdsec-cli/simulation.go b/cmd/crowdsec-cli/simulation.go
index e40813917..eef48798c 100644
--- a/cmd/crowdsec-cli/simulation.go
+++ b/cmd/crowdsec-cli/simulation.go
@@ -180,7 +180,7 @@ cscli simulation disable crowdsecurity/ssh-bf`,
 					log.Fatalf("unable to enable global simulation mode : %s", err.Error())
 				}
 			} else {
-				cmd.Help()
+				printHelp(cmd)
 			}
 		},
 	}
@@ -224,7 +224,7 @@ cscli simulation disable crowdsecurity/ssh-bf`,
 					log.Fatalf("unable to disable global simulation mode : %s", err.Error())
 				}
 			} else {
-				cmd.Help()
+				printHelp(cmd)
 			}
 		},
 	}
diff --git a/cmd/crowdsec-cli/utils.go b/cmd/crowdsec-cli/utils.go
index 894a0ed90..dd0cd6588 100644
--- a/cmd/crowdsec-cli/utils.go
+++ b/cmd/crowdsec-cli/utils.go
@@ -20,10 +20,18 @@ import (
 	dto "github.com/prometheus/client_model/go"
 	"github.com/prometheus/prom2json"
 	log "github.com/sirupsen/logrus"
+	"github.com/spf13/cobra"
 	"golang.org/x/mod/semver"
 	"gopkg.in/yaml.v2"
 )
 
+func printHelp(cmd *cobra.Command) {
+	err := cmd.Help()
+	if err != nil {
+		log.Fatalf("uname to print help(): %s", err)
+	}
+}
+
 func inSlice(s string, slice []string) bool {
 	for _, str := range slice {
 		if s == str {
diff --git a/tests/bats/90_decisions.bats b/tests/bats/90_decisions.bats
new file mode 100644
index 000000000..ee7a58cc9
--- /dev/null
+++ b/tests/bats/90_decisions.bats
@@ -0,0 +1,39 @@
+#!/usr/bin/env bats
+# vim: ft=bats:list:ts=8:sts=4:sw=4:et:ai:si:
+
+set -u
+
+setup_file() {
+    load "../lib/setup_file.sh" >&3 2>&1
+}
+
+teardown_file() {
+    load "../lib/teardown_file.sh" >&3 2>&1
+}
+
+setup() {
+    load "../lib/setup.sh"
+    ./instance-data load
+    ./instance-crowdsec start
+}
+
+teardown() {
+    ./instance-crowdsec stop
+}
+
+declare stderr
+
+#----------
+
+@test "$FILE 'decisions add' requires parameters" {
+    run -1 --separate-stderr cscli decisions add
+    assert_line "Usage:"
+    run echo "$stderr"
+    assert_output --partial "Missing arguments, a value is required (--ip, --range or --scope and --value)"
+
+    run -1 --separate-stderr cscli decisions add -o json
+    run echo "$stderr"
+    run -0 jq -c '[ .level, .msg]' <(output)
+    assert_output '["fatal","Missing arguments, a value is required (--ip, --range or --scope and --value)"]'
+}
+