Remove the Path references in code

This used to be in the original proto, but than got removed as me might
not need it.

3 files changed, 64 insertions, 81 deletions

diff --git a/internal/helper/security.go b/internal/helper/security.go
index 5742ed4ec..8dae4f9c5 100644
--- a/internal/helper/security.go
+++ b/internal/helper/security.go
@@ -9,11 +9,7 @@ import (
 func ContainsPathTraversal(path string) bool {
 	// Disallow directory traversal for security
 	separator := string(os.PathSeparator)
-	if strings.HasPrefix(path, ".."+separator) ||
+	return strings.HasPrefix(path, ".."+separator) ||
 		strings.Contains(path, separator+".."+separator) ||
-		strings.HasSuffix(path, separator+"..") {
-		return true
-	}
-
-	return false
+		strings.HasSuffix(path, separator+"..")
 }
diff --git a/internal/service/storage/listdirectories.go b/internal/service/storage/listdirectories.go
index 6e51e2390..9d29c05e8 100644
--- a/internal/service/storage/listdirectories.go
+++ b/internal/service/storage/listdirectories.go
@@ -2,7 +2,8 @@ package storage
 
 import (
 	"os"
-	"path"
+	"path/filepath"
+	"strings"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -10,67 +11,52 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-// This function won't scale, as it walks the dir structure in lexical order,
-// which means that it first will
 func (s *server) ListDirectories(req *pb.ListDirectoriesRequest, stream pb.StorageService_ListDirectoriesServer) error {
-	if helper.ContainsPathTraversal(req.GetPath()) {
-		return status.Error(codes.InvalidArgument, "ListDirectories: path traversal is not allowed")
-	}
-
 	storageDir, err := helper.GetStorageByName(req.StorageName)
 	if err != nil {
 		return status.Errorf(codes.InvalidArgument, "storage lookup failed: %v", err)
 	}
 
-	fullPath := path.Join(storageDir, req.GetPath())
-	fi, err := os.Stat(fullPath)
-	if os.IsNotExist(err) || !fi.IsDir() {
-		return status.Errorf(codes.NotFound, "ListDirectories: %v", err)
-	}
+	storageDir = storageDir + "/"
 
 	maxDepth := dirDepth(storageDir) + req.GetDepth()
-	for _, dir := range directoriesInPath(fullPath) {
-		stream.Send(&pb.ListDirectoriesResponse{Paths: recursiveDirs(dir, maxDepth)})
-	}
 
-	return nil
-}
-
-// Depends on the fact that input is always a path to a dir, not a file
-func dirDepth(dir string) uint32 {
-	dirs, _ := path.Split(dir)
+	var dirs []string
+	err = filepath.Walk(storageDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
 
-	return uint32(len(dirs) + 1)
-}
+		if info.IsDir() {
+			relPath := strings.TrimPrefix(path, storageDir)
+			if relPath == "" {
+				return nil
+			}
 
-func recursiveDirs(dir string, maxDepth uint32) []string {
-	var queue, dirs []string
+			dirs = append(dirs, relPath)
 
-	for len(queue) > 0 {
-		dir := queue[0]
-		queue = queue[1:]
+			if len(dirs) > 100 {
+				stream.Send(&pb.ListDirectoriesResponse{Paths: dirs})
+				dirs = dirs[:]
+			}
 
-		subDirs := directoriesInPath(dir)
+			if dirDepth(path)+1 > maxDepth {
+				return filepath.SkipDir
+			}
 
-		if dirDepth(dir)+1 <= maxDepth {
-			queue = append(queue, subDirs...)
+			return nil
 		}
 
-		dirs = append(dirs, subDirs...)
-	}
+		return nil
+	})
 
-	return dirs
-}
-
-func directoriesInPath(path string) []string {
-	fi, err := os.Open(path)
-	if os.IsNotExist(err) || err != nil {
-		return []string{}
+	if len(dirs) > 0 {
+		stream.Send(&pb.ListDirectoriesResponse{Paths: dirs})
 	}
 
-	// Readdirnames returns an empty slice if an error occurs. Given we can't
-	// recover, we ignore possible errors here
-	dirs, _ := fi.Readdirnames(0)
+	return err
+}
 
-	return dirs
+func dirDepth(dir string) uint32 {
+	return uint32(len(strings.Split(dir, string(os.PathSeparator)))) + 1
 }
diff --git a/internal/service/storage/listdirectories_test.go b/internal/service/storage/listdirectories_test.go
index c4178087b..b98495dbf 100644
--- a/internal/service/storage/listdirectories_test.go
+++ b/internal/service/storage/listdirectories_test.go
@@ -2,34 +2,40 @@ package storage
 
 import (
 	"io"
+	"io/ioutil"
 	"os"
-	"os/exec"
 	"path"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
 func TestListDirectories(t *testing.T) {
 	testDir := path.Join(testStorage.Path, t.Name())
 	require.NoError(t, os.MkdirAll(path.Dir(testDir), 0755))
+	defer os.RemoveAll(testDir)
 
-	repoPaths := []string{
-		"foo/bar1.git",
-		"foo/bar2.git",
-		"baz/foo/qux3.git",
-		"baz/foo/bar1.git",
-	}
+	// Mock the storage dir being our test dir, so results aren't influenced
+	// by other tests.
+	testStorages := []config.Storage{{Name: "default", Path: testDir}}
+
+	defer func(oldStorages []config.Storage) {
+		config.Config.Storages = oldStorages
+	}(config.Config.Storages)
+	config.Config.Storages = testStorages
 
+	repoPaths := []string{"foo", "bar", "bar/baz", "bar/baz/foo/buz"}
 	for _, p := range repoPaths {
-		fullPath := path.Join(testStorage.Path, p)
-		require.NoError(t, os.MkdirAll(fullPath, 0755))
-		require.NoError(t, exec.Command("git", "init", "--bare", fullPath).Run())
+		dirPath := filepath.Join(testDir, p)
+		require.NoError(t, os.MkdirAll(dirPath, 0755))
+		require.NoError(t, ioutil.WriteFile(filepath.Join(dirPath, "file"), []byte("Hello"), 0644))
 	}
-	defer os.RemoveAll(testStorage.Path)
 
 	server, socketPath := runStorageServer(t)
 	defer server.Stop()
@@ -41,34 +47,25 @@ func TestListDirectories(t *testing.T) {
 	defer cancel()
 
 	testCases := []struct {
-		path   string
-		depth  uint32
-		subset []string
+		depth uint32
+		dirs  []string
 	}{
 		{
-			path:   "",
-			depth:  1,
-			subset: []string{"foo", "baz"},
-		},
-		{
-			path:   "",
-			depth:  0,
-			subset: []string{"foo", "baz"},
+			depth: 0,
+			dirs:  []string{"bar", "foo"},
 		},
 		{
-			path:   "foo",
-			depth:  0,
-			subset: []string{"bar1.git", "foo/bar2.git"},
+			depth: 1,
+			dirs:  []string{"bar", "bar/baz", "foo"},
 		},
 		{
-			path:   "",
-			depth:  5,
-			subset: repoPaths,
+			depth: 3,
+			dirs:  []string{"bar", "bar/baz", "bar/baz/foo", "bar/baz/foo/buz", "foo"},
 		},
 	}
 
 	for _, tc := range testCases {
-		stream, err := client.ListDirectories(ctx, &pb.ListDirectoriesRequest{StorageName: testStorage.Name, Path: tc.path, Depth: tc.depth})
+		stream, err := client.ListDirectories(ctx, &pb.ListDirectoriesRequest{StorageName: "default", Depth: tc.depth})
 
 		var dirs []string
 		for {
@@ -82,7 +79,11 @@ func TestListDirectories(t *testing.T) {
 		}
 
 		require.NoError(t, err)
-		assert.NotContains(t, dirs, "HEAD")
-		assert.Subset(t, tc.subset, dirs)
+		require.NotEmpty(t, dirs)
+		assert.Equal(t, tc.dirs, dirs)
+
+		for _, dir := range dirs {
+			assert.False(t, strings.HasSuffix(dir, "file"))
+		}
 	}
 }