diff --git a/pkg/cmd/auth/gitcredential/helper.go b/pkg/cmd/auth/gitcredential/helper.go index dfa5690c1..9b64f74b9 100644 --- a/pkg/cmd/auth/gitcredential/helper.go +++ b/pkg/cmd/auth/gitcredential/helper.go @@ -55,7 +55,6 @@ func NewCmdCredential(f *cmdutil.Factory, runF func(*CredentialOptions) error) * return cmd } -// TODO: In multi-account we should use active user token only if the username is not passed in. func helperRun(opts *CredentialOptions) error { if opts.Operation == "store" { // We pretend to implement the "store" operation, but do nothing since we already have a cached token. diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 2c978f854..7be4bf91a 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -29,24 +29,18 @@ type RefreshOptions struct { Scopes []string RemoveScopes []string ResetScopes bool - AuthFlow func(*config.AuthConfig, *iostreams.IOStreams, string, []string, bool, bool) error + AuthFlow func(*iostreams.IOStreams, string, []string, bool) (string, string, error) Interactive bool InsecureStorage bool } -// TODO: Determine if this is super wonky in multi-account world. Do we need a --user flag? func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra.Command { opts := &RefreshOptions{ IO: f.IOStreams, Config: f.Config, - AuthFlow: func(authCfg *config.AuthConfig, io *iostreams.IOStreams, hostname string, scopes []string, interactive, secureStorage bool) error { - token, username, err := authflow.AuthFlow(hostname, io, "", scopes, interactive, f.Browser) - if err != nil { - return err - } - _, loginErr := authCfg.Login(hostname, username, token, "", secureStorage) - return loginErr + AuthFlow: func(io *iostreams.IOStreams, hostname string, scopes []string, interactive bool) (string, string, error) { + return authflow.AuthFlow(hostname, io, "", scopes, interactive, f.Browser) }, HttpClient: &http.Client{}, GitClient: f.GitClient, @@ -187,7 +181,15 @@ func refreshRun(opts *RefreshOptions) error { additionalScopes.RemoveValues(opts.RemoveScopes) - if err := opts.AuthFlow(authCfg, opts.IO, hostname, additionalScopes.ToSlice(), opts.Interactive, !opts.InsecureStorage); err != nil { + username, token, err := opts.AuthFlow(opts.IO, hostname, additionalScopes.ToSlice(), opts.Interactive) + if err != nil { + return err + } + activeUser, _ := authCfg.ActiveUser(hostname) + if activeUser != "" && activeUser != username { + return fmt.Errorf("error refreshing credentials for %s received credentials for %s", activeUser, username) + } + if _, err := authCfg.Login(hostname, username, token, "", !opts.InsecureStorage); err != nil { return err } diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 97c3bff11..cf5b9d524 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -176,12 +176,19 @@ type authArgs struct { secureStorage bool } +type authOut struct { + username string + token string + err error +} + func Test_refreshRun(t *testing.T) { tests := []struct { name string opts *RefreshOptions prompterStubs func(*prompter.PrompterMock) cfgHosts []string + authOut authOut oldScopes string wantErr string nontty bool @@ -193,7 +200,7 @@ func Test_refreshRun(t *testing.T) { wantErr: `not logged in to any hosts`, }, { - name: "hostname given but dne", + name: "hostname given but not previously authenticated with it", cfgHosts: []string{ "github.com", "aline.cedrac", @@ -403,16 +410,30 @@ func Test_refreshRun(t *testing.T) { secureStorage: true, }, }, + { + name: "errors when active user does not match user returned by auth flow", + cfgHosts: []string{ + "github.com", + }, + authOut: authOut{ + username: "not-test-user", + token: "xyz456", + }, + opts: &RefreshOptions{}, + wantErr: "error refreshing credentials for test-user received credentials for not-test-user", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { aa := authArgs{} - tt.opts.AuthFlow = func(_ *config.AuthConfig, _ *iostreams.IOStreams, hostname string, scopes []string, interactive, secureStorage bool) error { + tt.opts.AuthFlow = func(_ *iostreams.IOStreams, hostname string, scopes []string, interactive bool) (string, string, error) { aa.hostname = hostname aa.scopes = scopes aa.interactive = interactive - aa.secureStorage = secureStorage - return nil + if tt.authOut != (authOut{}) { + return tt.authOut.username, tt.authOut.token, tt.authOut.err + } + return "test-user", "xyz456", nil } cfg, _ := config.NewIsolatedTestConfig(t) @@ -458,14 +479,20 @@ func Test_refreshRun(t *testing.T) { err := refreshRun(tt.opts) if tt.wantErr != "" { require.Contains(t, err.Error(), tt.wantErr) - } else { - require.NoError(t, err) + return } + require.NoError(t, err) + require.Equal(t, tt.wantAuthArgs.hostname, aa.hostname) require.Equal(t, tt.wantAuthArgs.scopes, aa.scopes) require.Equal(t, tt.wantAuthArgs.interactive, aa.interactive) - require.Equal(t, tt.wantAuthArgs.secureStorage, aa.secureStorage) + + authCfg := cfg.Authentication() + activeUser, _ := authCfg.ActiveUser(aa.hostname) + activeToken, _ := authCfg.ActiveToken(aa.hostname) + require.Equal(t, "test-user", activeUser) + require.Equal(t, "xyz456", activeToken) }) } }