diff options
author | Nick Thomas <nick@gitlab.com> | 2019-05-22 12:28:25 +0000 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-05-22 12:28:25 +0000 |
commit | 48142b98ca9565698632c44e72e29e22b9c923c3 (patch) | |
tree | f2d007cb411667045ffc26278d7345bf19f362e9 | |
parent | 58d8c7691ac52c00dfebe2154e793c8fccc46aa0 (diff) | |
parent | b77a375205af394945de99c3f7318292510c3245 (diff) | |
download | gitlab-shell-48142b98ca9565698632c44e72e29e22b9c923c3.tar.gz |
Merge branch 'id-go-refactorings' into 'master'
Refactor execution and parsing logic in Go's implementation
See merge request gitlab-org/gitlab-shell!302
-rw-r--r-- | go/cmd/gitlab-shell/main.go | 6 | ||||
-rw-r--r-- | go/internal/command/command.go | 12 | ||||
-rw-r--r-- | go/internal/command/command_test.go | 4 | ||||
-rw-r--r-- | go/internal/command/discover/discover.go | 11 | ||||
-rw-r--r-- | go/internal/command/discover/discover_test.go | 16 | ||||
-rw-r--r-- | go/internal/command/fallback/fallback.go | 4 | ||||
-rw-r--r-- | go/internal/command/fallback/fallback_test.go | 6 | ||||
-rw-r--r-- | go/internal/command/twofactorrecover/twofactorrecover.go | 25 | ||||
-rw-r--r-- | go/internal/command/twofactorrecover/twofactorrecover_test.go | 8 | ||||
-rw-r--r-- | go/internal/gitlabnet/client.go | 12 | ||||
-rw-r--r-- | go/internal/gitlabnet/discover/client.go | 48 | ||||
-rw-r--r-- | go/internal/gitlabnet/discover/client_test.go | 17 | ||||
-rw-r--r-- | go/internal/gitlabnet/twofactorrecover/client.go | 31 | ||||
-rw-r--r-- | spec/gitlab_shell_two_factor_recovery_spec.rb | 2 |
14 files changed, 101 insertions, 101 deletions
diff --git a/go/cmd/gitlab-shell/main.go b/go/cmd/gitlab-shell/main.go index 6e39d8b..a8aef75 100644 --- a/go/cmd/gitlab-shell/main.go +++ b/go/cmd/gitlab-shell/main.go @@ -29,7 +29,7 @@ func findRootDir() (string, error) { func execRuby(rootDir string, readWriter *readwriter.ReadWriter) { cmd := &fallback.Command{RootDir: rootDir, Args: os.Args} - if err := cmd.Execute(readWriter); err != nil { + if err := cmd.Execute(); err != nil { fmt.Fprintf(readWriter.ErrOut, "Failed to exec: %v\n", err) os.Exit(1) } @@ -56,7 +56,7 @@ func main() { execRuby(rootDir, readWriter) } - cmd, err := command.New(os.Args, config) + cmd, err := command.New(os.Args, config, readWriter) if err != nil { // For now this could happen if `SSH_CONNECTION` is not set on // the environment @@ -66,7 +66,7 @@ func main() { // The command will write to STDOUT on execution or replace the current // process in case of the `fallback.Command` - if err = cmd.Execute(readWriter); err != nil { + if err = cmd.Execute(); err != nil { fmt.Fprintf(readWriter.ErrOut, "%v\n", err) os.Exit(1) } diff --git a/go/internal/command/command.go b/go/internal/command/command.go index 560e0b2..0ceb7fc 100644 --- a/go/internal/command/command.go +++ b/go/internal/command/command.go @@ -10,10 +10,10 @@ import ( ) type Command interface { - Execute(*readwriter.ReadWriter) error + Execute() error } -func New(arguments []string, config *config.Config) (Command, error) { +func New(arguments []string, config *config.Config, readWriter *readwriter.ReadWriter) (Command, error) { args, err := commandargs.Parse(arguments) if err != nil { @@ -21,18 +21,18 @@ func New(arguments []string, config *config.Config) (Command, error) { } if config.FeatureEnabled(string(args.CommandType)) { - return buildCommand(args, config), nil + return buildCommand(args, config, readWriter), nil } return &fallback.Command{RootDir: config.RootDir, Args: arguments}, nil } -func buildCommand(args *commandargs.CommandArgs, config *config.Config) Command { +func buildCommand(args *commandargs.CommandArgs, config *config.Config, readWriter *readwriter.ReadWriter) Command { switch args.CommandType { case commandargs.Discover: - return &discover.Command{Config: config, Args: args} + return &discover.Command{Config: config, Args: args, ReadWriter: readWriter} case commandargs.TwoFactorRecover: - return &twofactorrecover.Command{Config: config, Args: args} + return &twofactorrecover.Command{Config: config, Args: args, ReadWriter: readWriter} } return nil diff --git a/go/internal/command/command_test.go b/go/internal/command/command_test.go index 42c5112..228dc7a 100644 --- a/go/internal/command/command_test.go +++ b/go/internal/command/command_test.go @@ -65,7 +65,7 @@ func TestNew(t *testing.T) { restoreEnv := testhelper.TempEnv(tc.environment) defer restoreEnv() - command, err := New(tc.arguments, tc.config) + command, err := New(tc.arguments, tc.config, nil) assert.NoError(t, err) assert.IsType(t, tc.expectedType, command) @@ -78,7 +78,7 @@ func TestFailingNew(t *testing.T) { restoreEnv := testhelper.TempEnv(map[string]string{}) defer restoreEnv() - _, err := New([]string{}, &config.Config{}) + _, err := New([]string{}, &config.Config{}, nil) assert.Error(t, err, "Only ssh allowed") }) diff --git a/go/internal/command/discover/discover.go b/go/internal/command/discover/discover.go index 9bb442f..7d4ad2b 100644 --- a/go/internal/command/discover/discover.go +++ b/go/internal/command/discover/discover.go @@ -10,20 +10,21 @@ import ( ) type Command struct { - Config *config.Config - Args *commandargs.CommandArgs + Config *config.Config + Args *commandargs.CommandArgs + ReadWriter *readwriter.ReadWriter } -func (c *Command) Execute(readWriter *readwriter.ReadWriter) error { +func (c *Command) Execute() error { response, err := c.getUserInfo() if err != nil { return fmt.Errorf("Failed to get username: %v", err) } if response.IsAnonymous() { - fmt.Fprintf(readWriter.Out, "Welcome to GitLab, Anonymous!\n") + fmt.Fprintf(c.ReadWriter.Out, "Welcome to GitLab, Anonymous!\n") } else { - fmt.Fprintf(readWriter.Out, "Welcome to GitLab, @%s!\n", response.Username) + fmt.Fprintf(c.ReadWriter.Out, "Welcome to GitLab, @%s!\n", response.Username) } return nil diff --git a/go/internal/command/discover/discover_test.go b/go/internal/command/discover/discover_test.go index f85add8..40c0d4e 100644 --- a/go/internal/command/discover/discover_test.go +++ b/go/internal/command/discover/discover_test.go @@ -78,10 +78,14 @@ func TestExecute(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - cmd := &Command{Config: &config.Config{GitlabUrl: url}, Args: tc.arguments} buffer := &bytes.Buffer{} + cmd := &Command{ + Config: &config.Config{GitlabUrl: url}, + Args: tc.arguments, + ReadWriter: &readwriter.ReadWriter{Out: buffer}, + } - err := cmd.Execute(&readwriter.ReadWriter{Out: buffer}) + err := cmd.Execute() assert.NoError(t, err) assert.Equal(t, tc.expectedOutput, buffer.String()) @@ -118,10 +122,14 @@ func TestFailingExecute(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - cmd := &Command{Config: &config.Config{GitlabUrl: url}, Args: tc.arguments} buffer := &bytes.Buffer{} + cmd := &Command{ + Config: &config.Config{GitlabUrl: url}, + Args: tc.arguments, + ReadWriter: &readwriter.ReadWriter{Out: buffer}, + } - err := cmd.Execute(&readwriter.ReadWriter{Out: buffer}) + err := cmd.Execute() assert.Empty(t, buffer.String()) assert.EqualError(t, err, tc.expectedError) diff --git a/go/internal/command/fallback/fallback.go b/go/internal/command/fallback/fallback.go index 71e2a98..f525a57 100644 --- a/go/internal/command/fallback/fallback.go +++ b/go/internal/command/fallback/fallback.go @@ -4,8 +4,6 @@ import ( "os" "path/filepath" "syscall" - - "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/readwriter" ) type Command struct { @@ -22,7 +20,7 @@ const ( RubyProgram = "gitlab-shell-ruby" ) -func (c *Command) Execute(*readwriter.ReadWriter) error { +func (c *Command) Execute() error { rubyCmd := filepath.Join(c.RootDir, "bin", RubyProgram) // Ensure rubyArgs[0] is the full path to gitlab-shell-ruby diff --git a/go/internal/command/fallback/fallback_test.go b/go/internal/command/fallback/fallback_test.go index 2d67b14..afd752b 100644 --- a/go/internal/command/fallback/fallback_test.go +++ b/go/internal/command/fallback/fallback_test.go @@ -49,7 +49,7 @@ func TestExecuteExecsCommandSuccesfully(t *testing.T) { fake.Setup() defer fake.Cleanup() - require.NoError(t, cmd.Execute(nil)) + require.NoError(t, cmd.Execute()) require.True(t, fake.Called) require.Equal(t, fake.Filename, "/tmp/bin/gitlab-shell-ruby") require.Equal(t, fake.Args, []string{"/tmp/bin/gitlab-shell-ruby", "foo", "bar"}) @@ -64,12 +64,12 @@ func TestExecuteExecsCommandOnError(t *testing.T) { fake.Setup() defer fake.Cleanup() - require.Error(t, cmd.Execute(nil)) + require.Error(t, cmd.Execute()) require.True(t, fake.Called) } func TestExecuteGivenNonexistentCommand(t *testing.T) { cmd := &Command{RootDir: "/tmp/does/not/exist", Args: fakeArgs} - require.Error(t, cmd.Execute(nil)) + require.Error(t, cmd.Execute()) } diff --git a/go/internal/command/twofactorrecover/twofactorrecover.go b/go/internal/command/twofactorrecover/twofactorrecover.go index e77a334..faa35db 100644 --- a/go/internal/command/twofactorrecover/twofactorrecover.go +++ b/go/internal/command/twofactorrecover/twofactorrecover.go @@ -11,33 +11,34 @@ import ( ) type Command struct { - Config *config.Config - Args *commandargs.CommandArgs + Config *config.Config + Args *commandargs.CommandArgs + ReadWriter *readwriter.ReadWriter } -func (c *Command) Execute(readWriter *readwriter.ReadWriter) error { - if c.canContinue(readWriter) { - c.displayRecoveryCodes(readWriter) +func (c *Command) Execute() error { + if c.canContinue() { + c.displayRecoveryCodes() } else { - fmt.Fprintln(readWriter.Out, "\nNew recovery codes have *not* been generated. Existing codes will remain valid.") + fmt.Fprintln(c.ReadWriter.Out, "\nNew recovery codes have *not* been generated. Existing codes will remain valid.") } return nil } -func (c *Command) canContinue(readWriter *readwriter.ReadWriter) bool { +func (c *Command) canContinue() bool { question := "Are you sure you want to generate new two-factor recovery codes?\n" + "Any existing recovery codes you saved will be invalidated. (yes/no)" - fmt.Fprintln(readWriter.Out, question) + fmt.Fprintln(c.ReadWriter.Out, question) var answer string - fmt.Fscanln(readWriter.In, &answer) + fmt.Fscanln(c.ReadWriter.In, &answer) return answer == "yes" } -func (c *Command) displayRecoveryCodes(readWriter *readwriter.ReadWriter) { +func (c *Command) displayRecoveryCodes() { codes, err := c.getRecoveryCodes() if err == nil { @@ -47,9 +48,9 @@ func (c *Command) displayRecoveryCodes(readWriter *readwriter.ReadWriter) { "\n\nDuring sign in, use one of the codes above when prompted for\n" + "your two-factor code. Then, visit your Profile Settings and add\n" + "a new device so you do not lose access to your account again.\n" - fmt.Fprint(readWriter.Out, messageWithCodes) + fmt.Fprint(c.ReadWriter.Out, messageWithCodes) } else { - fmt.Fprintf(readWriter.Out, "\nAn error occurred while trying to generate new recovery codes.\n%v\n", err) + fmt.Fprintf(c.ReadWriter.Out, "\nAn error occurred while trying to generate new recovery codes.\n%v\n", err) } } diff --git a/go/internal/command/twofactorrecover/twofactorrecover_test.go b/go/internal/command/twofactorrecover/twofactorrecover_test.go index be76520..bcca12a 100644 --- a/go/internal/command/twofactorrecover/twofactorrecover_test.go +++ b/go/internal/command/twofactorrecover/twofactorrecover_test.go @@ -122,9 +122,13 @@ func TestExecute(t *testing.T) { output := &bytes.Buffer{} input := bytes.NewBufferString(tc.answer) - cmd := &Command{Config: &config.Config{GitlabUrl: url}, Args: tc.arguments} + cmd := &Command{ + Config: &config.Config{GitlabUrl: url}, + Args: tc.arguments, + ReadWriter: &readwriter.ReadWriter{Out: output, In: input}, + } - err := cmd.Execute(&readwriter.ReadWriter{Out: output, In: input}) + err := cmd.Execute() assert.NoError(t, err) assert.Equal(t, tc.expectedOutput, output.String()) diff --git a/go/internal/gitlabnet/client.go b/go/internal/gitlabnet/client.go index c0f7f97..86add04 100644 --- a/go/internal/gitlabnet/client.go +++ b/go/internal/gitlabnet/client.go @@ -17,6 +17,10 @@ const ( secretHeaderName = "Gitlab-Shared-Secret" ) +var ( + ParsingError = fmt.Errorf("Parsing failed") +) + type ErrorResponse struct { Message string `json:"message"` } @@ -120,3 +124,11 @@ func (c *GitlabClient) doRequest(method, path string, data interface{}) (*http.R return response, nil } + +func ParseJSON(hr *http.Response, response interface{}) error { + if err := json.NewDecoder(hr.Body).Decode(response); err != nil { + return ParsingError + } + + return nil +} diff --git a/go/internal/gitlabnet/discover/client.go b/go/internal/gitlabnet/discover/client.go index 1266379..675670d 100644 --- a/go/internal/gitlabnet/discover/client.go +++ b/go/internal/gitlabnet/discover/client.go @@ -1,7 +1,6 @@ package discover import ( - "encoding/json" "fmt" "net/http" "net/url" @@ -32,56 +31,39 @@ func NewClient(config *config.Config) (*Client, error) { } func (c *Client) GetByCommandArgs(args *commandargs.CommandArgs) (*Response, error) { - if args.GitlabKeyId != "" { - return c.GetByKeyId(args.GitlabKeyId) - } else if args.GitlabUsername != "" { - return c.GetByUsername(args.GitlabUsername) + params := url.Values{} + if args.GitlabUsername != "" { + params.Add("username", args.GitlabUsername) + } else if args.GitlabKeyId != "" { + params.Add("key_id", args.GitlabKeyId) } else { // There was no 'who' information, this matches the ruby error // message. return nil, fmt.Errorf("who='' is invalid") } -} - -func (c *Client) GetByKeyId(keyId string) (*Response, error) { - params := url.Values{} - params.Add("key_id", keyId) - - return c.getResponse(params) -} - -func (c *Client) GetByUsername(username string) (*Response, error) { - params := url.Values{} - params.Add("username", username) return c.getResponse(params) } -func (c *Client) parseResponse(resp *http.Response) (*Response, error) { - parsedResponse := &Response{} - - if err := json.NewDecoder(resp.Body).Decode(parsedResponse); err != nil { - return nil, err - } else { - return parsedResponse, nil - } -} - func (c *Client) getResponse(params url.Values) (*Response, error) { path := "/discover?" + params.Encode() - response, err := c.client.Get(path) + response, err := c.client.Get(path) if err != nil { return nil, err } - defer response.Body.Close() - parsedResponse, err := c.parseResponse(response) - if err != nil { - return nil, fmt.Errorf("Parsing failed") + + return parse(response) +} + +func parse(hr *http.Response) (*Response, error) { + response := &Response{} + if err := gitlabnet.ParseJSON(hr, response); err != nil { + return nil, err } - return parsedResponse, nil + return response, nil } func (r *Response) IsAnonymous() bool { diff --git a/go/internal/gitlabnet/discover/client_test.go b/go/internal/gitlabnet/discover/client_test.go index 006568a..8eabdd7 100644 --- a/go/internal/gitlabnet/discover/client_test.go +++ b/go/internal/gitlabnet/discover/client_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" "testing" "gitlab.com/gitlab-org/gitlab-shell/go/internal/config" @@ -59,7 +60,9 @@ func TestGetByKeyId(t *testing.T) { client, cleanup := setup(t) defer cleanup() - result, err := client.GetByKeyId("1") + params := url.Values{} + params.Add("key_id", "1") + result, err := client.getResponse(params) assert.NoError(t, err) assert.Equal(t, &Response{UserId: 2, Username: "alex-doe", Name: "Alex Doe"}, result) } @@ -68,7 +71,9 @@ func TestGetByUsername(t *testing.T) { client, cleanup := setup(t) defer cleanup() - result, err := client.GetByUsername("jane-doe") + params := url.Values{} + params.Add("username", "jane-doe") + result, err := client.getResponse(params) assert.NoError(t, err) assert.Equal(t, &Response{UserId: 1, Username: "jane-doe", Name: "Jane Doe"}, result) } @@ -77,7 +82,9 @@ func TestMissingUser(t *testing.T) { client, cleanup := setup(t) defer cleanup() - result, err := client.GetByUsername("missing") + params := url.Values{} + params.Add("username", "missing") + result, err := client.getResponse(params) assert.NoError(t, err) assert.True(t, result.IsAnonymous()) } @@ -110,7 +117,9 @@ func TestErrorResponses(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - resp, err := client.GetByUsername(tc.fakeUsername) + params := url.Values{} + params.Add("username", tc.fakeUsername) + resp, err := client.getResponse(params) assert.EqualError(t, err, tc.expectedError) assert.Nil(t, resp) diff --git a/go/internal/gitlabnet/twofactorrecover/client.go b/go/internal/gitlabnet/twofactorrecover/client.go index d26b141..f90a85c 100644 --- a/go/internal/gitlabnet/twofactorrecover/client.go +++ b/go/internal/gitlabnet/twofactorrecover/client.go @@ -1,10 +1,8 @@ package twofactorrecover import ( - "encoding/json" "errors" "fmt" - "io/ioutil" "net/http" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" @@ -46,38 +44,25 @@ func (c *Client) GetRecoveryCodes(args *commandargs.CommandArgs) ([]string, erro } response, err := c.client.Post("/two_factor_recovery_codes", requestBody) - if err != nil { return nil, err } - defer response.Body.Close() - parsedResponse, err := c.parseResponse(response) - - if err != nil { - return nil, fmt.Errorf("Parsing failed") - } - if parsedResponse.Success { - return parsedResponse.RecoveryCodes, nil - } else { - return nil, errors.New(parsedResponse.Message) - } + return parse(response) } -func (c *Client) parseResponse(resp *http.Response) (*Response, error) { - parsedResponse := &Response{} - body, err := ioutil.ReadAll(resp.Body) - - if err != nil { +func parse(hr *http.Response) ([]string, error) { + response := &Response{} + if err := gitlabnet.ParseJSON(hr, response); err != nil { return nil, err } - if err := json.Unmarshal(body, parsedResponse); err != nil { - return nil, err - } else { - return parsedResponse, nil + if !response.Success { + return nil, errors.New(response.Message) } + + return response.RecoveryCodes, nil } func (c *Client) getRequestBody(args *commandargs.CommandArgs) (*RequestBody, error) { diff --git a/spec/gitlab_shell_two_factor_recovery_spec.rb b/spec/gitlab_shell_two_factor_recovery_spec.rb index 19999e5..a7fa909 100644 --- a/spec/gitlab_shell_two_factor_recovery_spec.rb +++ b/spec/gitlab_shell_two_factor_recovery_spec.rb @@ -114,7 +114,7 @@ describe 'bin/gitlab-shell 2fa_recovery_codes' do it_behaves_like 'dialog for regenerating recovery keys' end - describe 'with go features' do + describe 'with go features', :go do before(:context) do write_config( "gitlab_url" => "http+unix://#{CGI.escape(tmp_socket_path)}", |