Fix ambiguous file issue

CHANGELOG.md

@@ -1,3 +1,9 @@
+## v0.6.3 (September 22, 2020)
+
+BUG FIXES:
+
+* Properly handle ambiguous files without read permission [#46](https://github.com/fishi0x01/vsh/pull/46) - Thank you for detailed report [agaudreault-jive](https://github.com/agaudreault-jive)
+
 ## v0.6.2 (June 18, 2020)
 
 ENHANCEMENTS:
@@ -7,7 +13,7 @@ ENHANCEMENTS:
 
 BUG FIXES:
 
-* Properly handle ambigious files ([#37](https://github.com/fishi0x01/vsh/pull/37))
+* Properly handle ambiguous files ([#37](https://github.com/fishi0x01/vsh/pull/37))
 
 ## v0.6.1 (June 15, 2020)
 

README.md

@@ -148,10 +148,18 @@ export VAULT_TOKEN=<token>
 ./vsh -c "rm secret/dir/to/remove/"
 ```
 
+## Permission requirements
+
+`vsh` requires `List` permission on the operated paths. 
+This is necessary to determine if a path points to a node or leaf in the path tree. 
+Further, it is needed to gather auto-completion data.
+
+For operations like `cp` or `mv`, `vsh` additionally requires `Read` and `Write` permissions on the operated paths.
+
 ## Stability
 
 Every command has integration tests against KV1 and KV2. 
-Every test is run against vault `1.0.0` and `1.5.0`, i.e., versions in between should also be compatible.
+Every test is run against vault `1.0.0` and `1.5.3`, i.e., versions in between should also be compatible.
 
 ## Local Development
 

cli/append.go

@@ -124,7 +124,7 @@ func (cmd *AppendCommand) Run() {
 
 	src := cmd.client.GetType(newSrcPwd)
 	if src != client.LEAF {
-		log.Error("Invalid source - must be a leaf: " + newSrcPwd)
+		log.NotAValidPath(newSrcPwd)
 		return
 	}
 

cli/cat.go

@@ -72,8 +72,7 @@ func (cmd *CatCommand) Run() {
 			}
 		}
 	} else {
-		log.Error("%s%s is not a file", cmd.client.Pwd, cmd.Path)
+		log.NotAValidPath(absPath)
 	}
-
 	return
 }

cli/cd.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"fmt"
 	"io"
+	"strings"
 
 	"github.com/fishi0x01/vsh/client"
 	"github.com/fishi0x01/vsh/log"
@@ -56,18 +57,17 @@ func (cmd *CdCommand) Run() {
 	t := cmd.client.GetType(newPwd)
 
 	if t == client.NONE {
-		log.Error("Invalid directory: %s", newPwd)
+		log.NotAValidPath(newPwd)
 		return
 	}
 
 	if t == client.LEAF {
-		log.Error("Invalid directory: %s is a file", newPwd)
+		log.NotAValidPath(newPwd)
 		return
 	}
 
-	newPwd = newPwd + "/"
-	if newPwd == "//" {
-		newPwd = "/"
+	if !strings.HasSuffix(newPwd, "/") {
+		newPwd = newPwd + "/"
 	}
 	cmd.client.Pwd = newPwd
 	return

cli/command.go

@@ -16,7 +16,6 @@ type Command interface {
 }
 
 func cmdPath(pwd string, arg string) (result string) {
-	strings.HasSuffix(arg, "/")
 	result = filepath.Clean(pwd + arg)
 
 	if strings.HasSuffix(arg, "/") {

cli/cp.go

@@ -63,7 +63,7 @@ func (cmd *CopyCommand) Run() {
 	case client.NODE:
 		runCommandWithTraverseTwoPaths(cmd.client, newSrcPwd, newTargetPwd, cmd.copySecret)
 	default:
-		log.Error("Invalid source path: %s", newSrcPwd)
+		log.NotAValidPath(newSrcPwd)
 	}
 }
 

cli/grep.go

@@ -79,7 +79,7 @@ func (cmd *GrepCommand) Run() {
 			filePaths = append(filePaths, traversedPath)
 		}
 	default:
-		log.Error("Invalid path: %s", path)
+		log.NotAValidPath(path)
 		return
 	}
 
@@ -92,6 +92,7 @@ func (cmd *GrepCommand) Run() {
 			match.print(cmd.stdout)
 		}
 	}
+	return
 }
 
 func (cmd *GrepCommand) grepFile(search string, path string) (matches []*Match, err error) {

cli/ls.go

@@ -59,7 +59,7 @@ func (cmd *ListCommand) Run() {
 	result, err := cmd.client.List(newPwd)
 
 	if err != nil {
-		log.Error("%w", err)
+		log.NotAValidPath(newPwd)
 	} else {
 		fmt.Fprintln(cmd.stdout, strings.Join(result, "\n"))
 	}

cli/mv.go

@@ -63,8 +63,9 @@ func (cmd *MoveCommand) Run() {
 	case client.NODE:
 		runCommandWithTraverseTwoPaths(cmd.client, newSrcPwd, newTargetPwd, cmd.moveSecret)
 	default:
-		log.Error("Invalid source path: %s", newSrcPwd)
+		log.NotAValidPath(newSrcPwd)
 	}
+	return
 }
 
 func (cmd *MoveCommand) moveSecret(source string, target string) error {

cli/rm.go

@@ -64,8 +64,9 @@ func (cmd *RemoveCommand) Run() {
 			}
 		}
 	default:
-		log.Error("Invalid path: %s", newPwd)
+		log.NotAValidPath(newPwd)
 	}
+	return
 }
 
 func (cmd *RemoveCommand) removeSecret(path string) error {

client/type.go

@@ -1,6 +1,9 @@
 package client
 
-import "strings"
+import (
+	"path/filepath"
+	"strings"
+)
 
 // PathKind describes the type of a path
 type PathKind int
@@ -23,39 +26,60 @@ func (client *Client) topLevelType(path string) PathKind {
 	}
 }
 
-func (client *Client) lowLevelType(path string) (result PathKind) {
-	result = NONE
-	isNode := false
-	isLeaf := false
-
-	s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path))
-	if err == nil && s != nil {
-		isNode = true
-	}
-
-	s, err = client.Vault.Logical().Read(client.getKVDataPath(path))
-	if err == nil && s != nil {
-		isLeaf = true
-	}
+var cachedPath = ""
+var cachedDirFiles = make(map[string]int)
 
-	if isLeaf && !isNode {
-		result = LEAF
+func (client *Client) isAmbiguous(path string) (result bool) {
+	// get current directory content
+	if cachedPath != path {
+		pathTrim := strings.TrimSuffix(path, "/")
+		cachedDirFiles = make(map[string]int)
+		s, err := client.Vault.Logical().List(client.getKVMetaDataPath(filepath.Dir(pathTrim)))
+		if err == nil && s != nil {
+			if keysInterface, ok := s.Data["keys"]; ok {
+				for _, valInterface := range keysInterface.([]interface{}) {
+					val := valInterface.(string)
+					cachedDirFiles[val] = 1
+				}
+			}
+		}
+		cachedPath = path
 	}
 
-	if isNode && !isLeaf {
-		result = NODE
+	// check if path exists as file and directory
+	result = false
+	if _, ok := cachedDirFiles[filepath.Base(path)]; ok {
+		if _, ok := cachedDirFiles[filepath.Base(path)+"/"]; ok {
+			result = true
+		}
 	}
+	return result
+}
 
-	if isLeaf && isNode {
-		// vault namespace path can overlap with a key, e.g.,
-		// secret/a and secret/a/b
-		// --> in that case, we have a leaf and a node when checking secret/a
+func (client *Client) lowLevelType(path string) (result PathKind) {
+	if client.isAmbiguous(path) {
 		if strings.HasSuffix(path, "/") {
 			result = NODE
 		} else {
 			result = LEAF
 		}
+	} else {
+		hasNode := false
+		s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path + "/"))
+		if err == nil && s != nil {
+			if _, ok := s.Data["keys"]; ok {
+				hasNode = true
+			}
+		}
+		if hasNode {
+			result = NODE
+		} else {
+			if _, ok := cachedDirFiles[filepath.Base(path)]; ok {
+				result = LEAF
+			} else {
+				result = NONE
+			}
+		}
 	}
-
-	return
+	return result
 }

client/util.go

@@ -1,7 +1,6 @@
 package client
 
 import (
-	"fmt"
 	"github.com/hashicorp/vault/api"
 	"strings"
 )
@@ -56,8 +55,8 @@ func (client *Client) kvPath(path string, prefix string) string {
 	case 2:
 		// https://www.vaultproject.io/docs/secrets/kv/kv-v2.html#acl-rules
 		s := strings.SplitN(path, "/", 2)
-		if len(s) != 2 {
-			panic(fmt.Errorf("Could not properly split path '%s'", path))
+		if len(s) == 1 {
+			return s[0] + prefix
 		}
 		return s[0] + prefix + s[1]
 	default:

log/log.go

@@ -1,6 +1,7 @@
 package log
 
 import (
+	"fmt"
 	au "github.com/logrusorgru/aurora"
 	"log"
 	"path/filepath"
@@ -77,3 +78,15 @@ func Warn(f string, args ...interface{}) {
 func Error(f string, args ...interface{}) {
 	logger(errorLvl, f, args...)
 }
+
+// NotAValidPath prints not a valid path message
+func NotAValidPath(f string) {
+	message := au.Red(au.Sprintf("Not a valid path for operation: %s\n", f))
+	fmt.Printf(au.Sprintf(message))
+}
+
+// NotAValidCommand prints not a valid command
+func NotAValidCommand(c string) {
+	message := au.Red(au.Sprintf("Not a valid command: %s\n", c))
+	fmt.Printf(au.Sprintf(message))
+}

main.go

@@ -66,6 +66,11 @@ func executor(in string) {
 	var cmd cli.Command
 	var run bool
 
+	if len(args) == 0 {
+		fmt.Fprint(os.Stdout, "")
+		return
+	}
+
 	// Check for built-in commands.
 	switch args[0] {
 	case commands.ls.GetName():
@@ -94,11 +99,8 @@ func executor(in string) {
 		cmd = commands.grep
 	case "exit":
 		os.Exit(0)
-	case "":
-		fmt.Fprint(os.Stdout, "")
-		return
 	default:
-		log.Error("Unknown command '%s'", args[0])
+		log.NotAValidCommand(args[0])
 		return
 	}
 

test/command-tests/append.bats

@@ -24,6 +24,14 @@ load ../bin/plugins/bats-assert/load
   assert_success
   assert_output "test"
 
+  #######################################
+  echo "==== case: append to non-existing file ===="
+  run ${APP_BIN} -c "append ${KV_BACKEND}/src/does/not/exist ${KV_BACKEND}/src/aa"
+  assert_success
+
+  echo "ensure proper error message"
+  assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist"
+
   #######################################
   echo "==== case: append value to existing destination with conflicting keys (default merge strategy) ===="
   run ${APP_BIN} -c "append ${KV_BACKEND}/src/dev/1 ${KV_BACKEND}/dest/prod/all"

test/command-tests/cat.bats

@@ -9,11 +9,19 @@ load ../bin/plugins/bats-assert/load
   assert_success
   assert_line "value = 1"
 
+  #######################################
+  echo "==== case: cat non-existing file ===="
+  run ${APP_BIN} -c "cat ${KV_BACKEND}/src/does/not/exist"
+  assert_success
+
+  echo "ensure proper error message"
+  assert_line --partial "Not a valid path: /${KV_BACKEND}/src/does/not/exist"
+
   #######################################
   echo "==== case: cat directory ===="
   run ${APP_BIN} -c "cat ${KV_BACKEND}/src/dev"
   assert_success
-  assert_output --partial "is not a file"
+  assert_line --partial "Not a valid path: /${KV_BACKEND}/src/dev"
 
   #######################################
   echo "==== case: cat ambigious file ===="
@@ -27,5 +35,5 @@ load ../bin/plugins/bats-assert/load
   echo "==== case: cat ambigious directory ===="
   run ${APP_BIN} -c "cat ${KV_BACKEND}/src/tooling/"
   assert_success
-  assert_output --partial "is not a file"
+  assert_line --partial "Not a valid path: /${KV_BACKEND}/src/tooling/"
 }

test/command-tests/cd.bats

@@ -0,0 +1,25 @@
+load ../util/util
+load ../bin/plugins/bats-support/load
+load ../bin/plugins/bats-assert/load
+
+@test "vault-${VAULT_VERSION} ${KV_BACKEND} 'cd'" {
+  #######################################
+  echo "==== case: cd to sub-sub-dir ===="
+  run ${APP_BIN} -c "cd ${KV_BACKEND}/src/dev"
+  assert_success
+
+  #######################################
+  echo "==== case: cd to KV ===="
+  run ${APP_BIN} -c "cd ${KV_BACKEND}/"
+  assert_success
+
+  #######################################
+  echo "==== case: cd to KV first level sub-dir ===="
+  run ${APP_BIN} -c "cd ${KV_BACKEND}/src/"
+  assert_success
+
+  #######################################
+  echo "==== case: cd to non-existing dir ===="
+  run ${APP_BIN} -c "cd ${KV_BACKEND}/src/does/not/exist"
+  assert_success
+}