Merge branch '37371-git-clone-on-secondary-geo-node-fetches-lfs-files-from-primary' into 'master'

Use correct git-lfs download or upload operation names

See merge request gitlab-org/gitlab-shell!353

4 files changed, 65 insertions, 41 deletions

diff --git a/internal/command/lfsauthenticate/lfsauthenticate.go b/internal/command/lfsauthenticate/lfsauthenticate.go
index 1b2a742..2aaac2a 100644
--- a/internal/command/lfsauthenticate/lfsauthenticate.go
+++ b/internal/command/lfsauthenticate/lfsauthenticate.go
@@ -14,8 +14,8 @@ import (
 )
 
 const (
-	downloadAction = "download"
-	uploadAction   = "upload"
+	downloadOperation = "download"
+	uploadOperation   = "upload"
 )
 
 type Command struct {
@@ -40,8 +40,11 @@ func (c *Command) Execute() error {
 		return disallowedcommand.Error
 	}
 
+	// e.g. git-lfs-authenticate user/repo.git download
 	repo := args[1]
-	action, err := actionToCommandType(args[2])
+	operation := args[2]
+
+	action, err := actionFromOperation(operation)
 	if err != nil {
 		return err
 	}
@@ -51,7 +54,7 @@ func (c *Command) Execute() error {
 		return err
 	}
 
-	payload, err := c.authenticate(action, repo, accessResponse.UserId)
+	payload, err := c.authenticate(operation, repo, accessResponse.UserId)
 	if err != nil {
 		// return nothing just like Ruby's GitlabShell#lfs_authenticate does
 		return nil
@@ -62,18 +65,19 @@ func (c *Command) Execute() error {
 	return nil
 }
 
-func actionToCommandType(action string) (commandargs.CommandType, error) {
-	var accessAction commandargs.CommandType
-	switch action {
-	case downloadAction:
-		accessAction = commandargs.UploadPack
-	case uploadAction:
-		accessAction = commandargs.ReceivePack
+func actionFromOperation(operation string) (commandargs.CommandType, error) {
+	var action commandargs.CommandType
+
+	switch operation {
+	case downloadOperation:
+		action = commandargs.UploadPack
+	case uploadOperation:
+		action = commandargs.ReceivePack
 	default:
 		return "", disallowedcommand.Error
 	}
 
-	return accessAction, nil
+	return action, nil
 }
 
 func (c *Command) verifyAccess(action commandargs.CommandType, repo string) (*accessverifier.Response, error) {
@@ -82,13 +86,13 @@ func (c *Command) verifyAccess(action commandargs.CommandType, repo string) (*ac
 	return cmd.Verify(action, repo)
 }
 
-func (c *Command) authenticate(action commandargs.CommandType, repo, userId string) ([]byte, error) {
+func (c *Command) authenticate(operation string, repo, userId string) ([]byte, error) {
 	client, err := lfsauthenticate.NewClient(c.Config, c.Args)
 	if err != nil {
 		return nil, err
 	}
 
-	response, err := client.Authenticate(action, repo, userId)
+	response, err := client.Authenticate(operation, repo, userId)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/command/lfsauthenticate/lfsauthenticate_test.go b/internal/command/lfsauthenticate/lfsauthenticate_test.go
index 22e151a..c2f0fd3 100644
--- a/internal/command/lfsauthenticate/lfsauthenticate_test.go
+++ b/internal/command/lfsauthenticate/lfsauthenticate_test.go
@@ -64,6 +64,7 @@ func TestFailedRequests(t *testing.T) {
 
 func TestLfsAuthenticateRequests(t *testing.T) {
 	userId := "123"
+	operation := "upload"
 
 	requests := []testserver.TestRequestHandler{
 		{
@@ -75,6 +76,7 @@ func TestLfsAuthenticateRequests(t *testing.T) {
 
 				var request *lfsauthenticate.Request
 				require.NoError(t, json.Unmarshal(b, &request))
+				require.Equal(t, request.Operation, operation)
 
 				if request.UserId == userId {
 					body := map[string]interface{}{
@@ -140,7 +142,7 @@ func TestLfsAuthenticateRequests(t *testing.T) {
 			output := &bytes.Buffer{}
 			cmd := &Command{
 				Config:     &config.Config{GitlabUrl: url},
-				Args:       &commandargs.Shell{GitlabUsername: tc.username, SshArgs: []string{"git-lfs-authenticate", "group/repo", "upload"}},
+				Args:       &commandargs.Shell{GitlabUsername: tc.username, SshArgs: []string{"git-lfs-authenticate", "group/repo", operation}},
 				ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output},
 			}
 
diff --git a/internal/gitlabnet/lfsauthenticate/client.go b/internal/gitlabnet/lfsauthenticate/client.go
index d7469dd..d797321 100644
--- a/internal/gitlabnet/lfsauthenticate/client.go
+++ b/internal/gitlabnet/lfsauthenticate/client.go
@@ -17,10 +17,10 @@ type Client struct {
 }
 
 type Request struct {
-	Action commandargs.CommandType `json:"operation"`
-	Repo   string                  `json:"project"`
-	KeyId  string                  `json:"key_id,omitempty"`
-	UserId string                  `json:"user_id,omitempty"`
+	Operation string `json:"operation"`
+	Repo      string `json:"project"`
+	KeyId     string `json:"key_id,omitempty"`
+	UserId    string `json:"user_id,omitempty"`
 }
 
 type Response struct {
@@ -39,8 +39,8 @@ func NewClient(config *config.Config, args *commandargs.Shell) (*Client, error)
 	return &Client{config: config, client: client, args: args}, nil
 }
 
-func (c *Client) Authenticate(action commandargs.CommandType, repo, userId string) (*Response, error) {
-	request := &Request{Action: action, Repo: repo}
+func (c *Client) Authenticate(operation, repo, userId string) (*Response, error) {
+	request := &Request{Operation: operation, Repo: repo}
 	if c.args.GitlabKeyId != "" {
 		request.KeyId = c.args.GitlabKeyId
 	} else {
diff --git a/internal/gitlabnet/lfsauthenticate/client_test.go b/internal/gitlabnet/lfsauthenticate/client_test.go
index 6faaa63..0a06960 100644
--- a/internal/gitlabnet/lfsauthenticate/client_test.go
+++ b/internal/gitlabnet/lfsauthenticate/client_test.go
@@ -14,9 +14,8 @@ import (
 )
 
 const (
-	keyId  = "123"
-	repo   = "group/repo"
-	action = commandargs.UploadPack
+	keyId = "123"
+	repo  = "group/repo"
 )
 
 func setup(t *testing.T) []testserver.TestRequestHandler {
@@ -64,17 +63,17 @@ func TestFailedRequests(t *testing.T) {
 	}{
 		{
 			desc:           "With bad response",
-			args:           &commandargs.Shell{GitlabKeyId: "-1", CommandType: commandargs.UploadPack},
+			args:           &commandargs.Shell{GitlabKeyId: "-1", CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, "download"}},
 			expectedOutput: "Parsing failed",
 		},
 		{
 			desc:           "With API returns an error",
-			args:           &commandargs.Shell{GitlabKeyId: "forbidden", CommandType: commandargs.UploadPack},
+			args:           &commandargs.Shell{GitlabKeyId: "forbidden", CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, "download"}},
 			expectedOutput: "Internal API error (403)",
 		},
 		{
 			desc:           "With API fails",
-			args:           &commandargs.Shell{GitlabKeyId: "broken", CommandType: commandargs.UploadPack},
+			args:           &commandargs.Shell{GitlabKeyId: "broken", CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, "download"}},
 			expectedOutput: "Internal API error (500)",
 		},
 	}
@@ -84,9 +83,9 @@ func TestFailedRequests(t *testing.T) {
 			client, err := NewClient(&config.Config{GitlabUrl: url}, tc.args)
 			require.NoError(t, err)
 
-			repo := "group/repo"
+			operation := tc.args.SshArgs[2]
 
-			_, err = client.Authenticate(tc.args.CommandType, repo, "")
+			_, err = client.Authenticate(operation, repo, "")
 			require.Error(t, err)
 
 			require.Equal(t, tc.expectedOutput, err.Error())
@@ -99,19 +98,38 @@ func TestSuccessfulRequests(t *testing.T) {
 	url, cleanup := testserver.StartHttpServer(t, requests)
 	defer cleanup()
 
-	args := &commandargs.Shell{GitlabKeyId: keyId, CommandType: commandargs.LfsAuthenticate}
-	client, err := NewClient(&config.Config{GitlabUrl: url}, args)
-	require.NoError(t, err)
+	testCases := []struct {
+		desc      string
+		operation string
+	}{
+		{
+			desc:      "For download",
+			operation: "download",
+		},
+		{
+			desc:      "For upload",
+			operation: "upload",
+		},
+	}
 
-	response, err := client.Authenticate(action, repo, "")
-	require.NoError(t, err)
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			operation := tc.operation
+			args := &commandargs.Shell{GitlabKeyId: keyId, CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, operation}}
+			client, err := NewClient(&config.Config{GitlabUrl: url}, args)
+			require.NoError(t, err)
 
-	expectedResponse := &Response{
-		Username:  "john",
-		LfsToken:  "sometoken",
-		RepoPath:  "https://gitlab.com/repo/path",
-		ExpiresIn: 1800,
-	}
+			response, err := client.Authenticate(operation, repo, "")
+			require.NoError(t, err)
 
-	require.Equal(t, expectedResponse, response)
+			expectedResponse := &Response{
+				Username:  "john",
+				LfsToken:  "sometoken",
+				RepoPath:  "https://gitlab.com/repo/path",
+				ExpiresIn: 1800,
+			}
+
+			require.Equal(t, expectedResponse, response)
+		})
+	}
 }