storage: Remove Git-specific knowledge from the locator interface

The `storage.Locator` interface provides functions to locate specific
Git data structures in a repository. This knowledge is specific to Git
though and doesn't have anything to do with what the locator should do,
which is to provide information about Gitaly-specific locations.

Move this Git-specific knowledge into the localrepo package and remove
those functions from the interface.

5 files changed, 157 insertions, 174 deletions

diff --git a/internal/git/localrepo/paths.go b/internal/git/localrepo/paths.go
index 8efbe79fe..0c6e32ada 100644
--- a/internal/git/localrepo/paths.go
+++ b/internal/git/localrepo/paths.go
@@ -1,5 +1,14 @@
 package localrepo
 
+import (
+	"os"
+	"path/filepath"
+	"strings"
+
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+)
+
 // Path returns the on-disk path of the repository.
 func (repo *Repo) Path() (string, error) {
 	return repo.locator.GetRepoPath(repo)
@@ -8,10 +17,51 @@ func (repo *Repo) Path() (string, error) {
 // ObjectDirectoryPath returns the full path of the object directory. The errors returned are gRPC
 // errors with relevant error codes and should be passed back to gRPC without further decoration.
 func (repo *Repo) ObjectDirectoryPath() (string, error) {
-	return repo.locator.GetObjectDirectoryPath(repo)
+	repoPath, err := repo.Path()
+	if err != nil {
+		return "", err
+	}
+
+	objectDirectoryPath := repo.GetGitObjectDirectory()
+	if objectDirectoryPath == "" {
+		return "", helper.ErrInvalidArgumentf("object directory path is not set")
+	}
+
+	// We need to check whether the relative object directory as given by the repository is
+	// a valid path. This may either be a path in the Git repository itself, where it may either
+	// point to the main object directory storage or to an object quarantine directory as
+	// created by git-receive-pack(1). Alternatively, if that is not the case, then it may be a
+	// manual object quarantine directory located in the storage's temporary directory. These
+	// have a repository-specific prefix which we must check in order to determine whether the
+	// quarantine directory does in fact belong to the repo at hand.
+	if _, origError := storage.ValidateRelativePath(repoPath, objectDirectoryPath); origError != nil {
+		tempDir, err := repo.locator.TempDir(repo.GetStorageName())
+		if err != nil {
+			return "", helper.ErrInvalidArgumentf("getting storage's temporary directory: %s", err)
+		}
+
+		expectedQuarantinePrefix := filepath.Join(tempDir, storage.QuarantineDirectoryPrefix(repo))
+		absoluteObjectDirectoryPath := filepath.Join(repoPath, objectDirectoryPath)
+
+		if !strings.HasPrefix(absoluteObjectDirectoryPath, expectedQuarantinePrefix) {
+			return "", helper.ErrInvalidArgumentf("not a valid relative path: %s", origError)
+		}
+	}
+
+	fullPath := filepath.Join(repoPath, objectDirectoryPath)
+	if _, err := os.Stat(fullPath); os.IsNotExist(err) {
+		return "", helper.ErrNotFoundf("object directory does not exist: %q", fullPath)
+	}
+
+	return fullPath, nil
 }
 
 // InfoAlternatesPath returns the full path of the alternates file.
 func (repo *Repo) InfoAlternatesPath() (string, error) {
-	return repo.locator.InfoAlternatesPath(repo)
+	repoPath, err := repo.Path()
+	if err != nil {
+		return "", err
+	}
+
+	return filepath.Join(repoPath, "objects", "info", "alternates"), nil
 }
diff --git a/internal/git/localrepo/paths_test.go b/internal/git/localrepo/paths_test.go
index 8c0a21450..290690a08 100644
--- a/internal/git/localrepo/paths_test.go
+++ b/internal/git/localrepo/paths_test.go
@@ -2,13 +2,20 @@ package localrepo_test
 
 import (
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
+	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
 )
 
 func TestRepo_Path(t *testing.T) {
@@ -44,3 +51,101 @@ func TestRepo_Path(t *testing.T) {
 		require.Equal(t, codes.NotFound, helper.GrpcCode(err))
 	})
 }
+
+func TestRepo_ObjectDirectoryPath(t *testing.T) {
+	cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
+	locator := config.NewLocator(cfg)
+
+	ctx := testhelper.Context(t)
+
+	quarantine, err := quarantine.New(ctx, repoProto, locator)
+	require.NoError(t, err)
+	quarantinedRepo := quarantine.QuarantinedRepo()
+
+	repoWithGitObjDir := func(repo *gitalypb.Repository, dir string) *gitalypb.Repository {
+		repo = proto.Clone(repo).(*gitalypb.Repository)
+		repo.GitObjectDirectory = dir
+		return repo
+	}
+
+	testCases := []struct {
+		desc string
+		repo *gitalypb.Repository
+		path string
+		err  codes.Code
+	}{
+		{
+			desc: "storages configured",
+			repo: repoWithGitObjDir(repoProto, "objects/"),
+			path: filepath.Join(repoPath, "objects/"),
+		},
+		{
+			desc: "no GitObjectDirectoryPath",
+			repo: repoProto,
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "with directory traversal",
+			repo: repoWithGitObjDir(repoProto, "../bazqux.git"),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "valid path but doesn't exist",
+			repo: repoWithGitObjDir(repoProto, "foo../bazqux.git"),
+			err:  codes.NotFound,
+		},
+		{
+			desc: "with sneaky directory traversal",
+			repo: repoWithGitObjDir(repoProto, "/../bazqux.git"),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "with traversal outside repository",
+			repo: repoWithGitObjDir(repoProto, "objects/../.."),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "with traversal outside repository with trailing separator",
+			repo: repoWithGitObjDir(repoProto, "objects/../../"),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "with deep traversal at the end",
+			repo: repoWithGitObjDir(repoProto, "bazqux.git/../.."),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "quarantined repo",
+			repo: quarantinedRepo,
+			path: filepath.Join(repoPath, quarantinedRepo.GetGitObjectDirectory()),
+		},
+		{
+			desc: "quarantined repo with parent directory",
+			repo: repoWithGitObjDir(quarantinedRepo, quarantinedRepo.GetGitObjectDirectory()+"/.."),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "quarantined repo with directory traversal",
+			repo: repoWithGitObjDir(quarantinedRepo, quarantinedRepo.GetGitObjectDirectory()+"/../foobar.git"),
+			err:  codes.InvalidArgument,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			repo := localrepo.NewTestRepo(t, cfg, tc.repo)
+
+			path, err := repo.ObjectDirectoryPath()
+
+			if tc.err != codes.OK {
+				st, ok := status.FromError(err)
+				require.True(t, ok)
+				require.Equal(t, tc.err, st.Code())
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tc.path, path)
+		})
+	}
+}
diff --git a/internal/gitaly/config/locator.go b/internal/gitaly/config/locator.go
index 27b8329dd..15b7376ab 100644
--- a/internal/gitaly/config/locator.go
+++ b/internal/gitaly/config/locator.go
@@ -3,7 +3,6 @@ package config
 import (
 	"os"
 	"path/filepath"
-	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
@@ -97,59 +96,6 @@ func (l *configLocator) GetStorageByName(storageName string) (string, error) {
 	return storagePath, nil
 }
 
-// GetObjectDirectoryPath returns the full path of the object directory in a
-// repository referenced by an RPC Repository message. The errors returned are
-// gRPC errors with relevant error codes and should be passed back to gRPC
-// without further decoration.
-func (l *configLocator) GetObjectDirectoryPath(repo repository.GitRepo) (string, error) {
-	repoPath, err := l.GetRepoPath(repo)
-	if err != nil {
-		return "", err
-	}
-
-	objectDirectoryPath := repo.GetGitObjectDirectory()
-	if objectDirectoryPath == "" {
-		return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: empty directory")
-	}
-
-	// We need to check whether the relative object directory as given by the repository is
-	// a valid path. This may either be a path in the Git repository itself, where it may either
-	// point to the main object directory storage or to an object quarantine directory as
-	// created by git-receive-pack(1). Alternatively, if that is not the case, then it may be a
-	// manual object quarantine directory located in the storage's temporary directory. These
-	// have a repository-specific prefix which we must check in order to determine whether the
-	// quarantine directory does in fact belong to the repo at hand.
-	if _, origError := storage.ValidateRelativePath(repoPath, objectDirectoryPath); origError != nil {
-		tempDir, err := l.TempDir(repo.GetStorageName())
-		if err != nil {
-			return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: %s", err)
-		}
-
-		expectedQuarantinePrefix := filepath.Join(tempDir, storage.QuarantineDirectoryPrefix(repo))
-		absoluteObjectDirectoryPath := filepath.Join(repoPath, objectDirectoryPath)
-
-		if !strings.HasPrefix(absoluteObjectDirectoryPath, expectedQuarantinePrefix) {
-			return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: %s", origError)
-		}
-	}
-
-	fullPath := filepath.Join(repoPath, objectDirectoryPath)
-	if _, err := os.Stat(fullPath); os.IsNotExist(err) {
-		return "", status.Errorf(codes.NotFound, "GetObjectDirectoryPath: does not exist: %q", fullPath)
-	}
-
-	return fullPath, nil
-}
-
-func (l *configLocator) InfoAlternatesPath(repo repository.GitRepo) (string, error) {
-	repoPath, err := l.GetRepoPath(repo)
-	if err != nil {
-		return "", err
-	}
-
-	return filepath.Join(repoPath, "objects", "info", "alternates"), nil
-}
-
 // CacheDir returns the path to the cache dir for a storage.
 func (l *configLocator) CacheDir(storageName string) (string, error) {
 	return l.getPath(storageName, cachePrefix)
diff --git a/internal/gitaly/config/locator_test.go b/internal/gitaly/config/locator_test.go
deleted file mode 100644
index c006ff648..000000000
--- a/internal/gitaly/config/locator_test.go
+++ /dev/null
@@ -1,111 +0,0 @@
-package config_test
-
-import (
-	"path/filepath"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
-	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
-	"google.golang.org/protobuf/proto"
-)
-
-func TestConfigLocator_GetObjectDirectoryPath(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
-	locator := config.NewLocator(cfg)
-	ctx := testhelper.Context(t)
-
-	quarantine, err := quarantine.New(ctx, repo, locator)
-	require.NoError(t, err)
-	quarantinedRepo := quarantine.QuarantinedRepo()
-
-	repoWithGitObjDir := func(repo *gitalypb.Repository, dir string) *gitalypb.Repository {
-		repo = proto.Clone(repo).(*gitalypb.Repository)
-		repo.GitObjectDirectory = dir
-		return repo
-	}
-
-	testCases := []struct {
-		desc string
-		repo *gitalypb.Repository
-		path string
-		err  codes.Code
-	}{
-		{
-			desc: "storages configured",
-			repo: repoWithGitObjDir(repo, "objects/"),
-			path: filepath.Join(repoPath, "objects/"),
-		},
-		{
-			desc: "no GitObjectDirectoryPath",
-			repo: repo,
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "with directory traversal",
-			repo: repoWithGitObjDir(repo, "../bazqux.git"),
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "valid path but doesn't exist",
-			repo: repoWithGitObjDir(repo, "foo../bazqux.git"),
-			err:  codes.NotFound,
-		},
-		{
-			desc: "with sneaky directory traversal",
-			repo: repoWithGitObjDir(repo, "/../bazqux.git"),
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "with traversal outside repository",
-			repo: repoWithGitObjDir(repo, "objects/../.."),
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "with traversal outside repository with trailing separator",
-			repo: repoWithGitObjDir(repo, "objects/../../"),
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "with deep traversal at the end",
-			repo: repoWithGitObjDir(repo, "bazqux.git/../.."),
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "quarantined repo",
-			repo: quarantinedRepo,
-			path: filepath.Join(repoPath, quarantinedRepo.GetGitObjectDirectory()),
-		},
-		{
-			desc: "quarantined repo with parent directory",
-			repo: repoWithGitObjDir(quarantinedRepo, quarantinedRepo.GetGitObjectDirectory()+"/.."),
-			err:  codes.InvalidArgument,
-		},
-		{
-			desc: "quarantined repo with directory traversal",
-			repo: repoWithGitObjDir(quarantinedRepo, quarantinedRepo.GetGitObjectDirectory()+"/../foobar.git"),
-			err:  codes.InvalidArgument,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			path, err := locator.GetObjectDirectoryPath(tc.repo)
-
-			if tc.err != codes.OK {
-				st, ok := status.FromError(err)
-				require.True(t, ok)
-				require.Equal(t, tc.err, st.Code())
-				return
-			}
-
-			require.NoError(t, err)
-			require.Equal(t, tc.path, path)
-		})
-	}
-}
diff --git a/internal/gitaly/storage/locator.go b/internal/gitaly/storage/locator.go
index 70e3031ca..a7d5f60da 100644
--- a/internal/gitaly/storage/locator.go
+++ b/internal/gitaly/storage/locator.go
@@ -25,13 +25,6 @@ type Locator interface {
 	// GetStorageByName will return the path for the storage, which is fetched by
 	// its key. An error is return if it cannot be found.
 	GetStorageByName(storageName string) (string, error)
-	// GetObjectDirectoryPath returns the full path of the object directory in a
-	// repository referenced by an RPC Repository message. The errors returned are
-	// gRPC errors with relevant error codes and should be passed back to gRPC
-	// without further decoration.
-	GetObjectDirectoryPath(repo repository.GitRepo) (string, error)
-	// InfoAlternatesPath finds the fully qualified path for the alternates file.
-	InfoAlternatesPath(repo repository.GitRepo) (string, error)
 
 	// CacheDir returns the path to the cache dir for a storage.
 	CacheDir(storageName string) (string, error)