diff --git a/api/http_client.go b/api/http_client.go index fcf036008..f6e133f1f 100644 --- a/api/http_client.go +++ b/api/http_client.go @@ -13,7 +13,7 @@ import ( ) type tokenGetter interface { - Token(string) (string, string) + ActiveToken(string) (string, string) } type HTTPClientOptions struct { @@ -99,7 +99,7 @@ func AddAuthTokenHeader(rt http.RoundTripper, cfg tokenGetter) http.RoundTripper // If the host has changed during a redirect do not add the authentication token header. if !redirectHostnameChange { hostname := ghinstance.NormalizeHostname(getHost(req)) - if token, _ := cfg.Token(hostname); token != "" { + if token, _ := cfg.ActiveToken(hostname); token != "" { req.Header.Set(authorization, fmt.Sprintf("token %s", token)) } } diff --git a/api/http_client_test.go b/api/http_client_test.go index f8fe28d40..dca032e6f 100644 --- a/api/http_client_test.go +++ b/api/http_client_test.go @@ -265,7 +265,7 @@ func TestHTTPClientSanitizeControlCharactersC1(t *testing.T) { type tinyConfig map[string]string -func (c tinyConfig) Token(host string) (string, string) { +func (c tinyConfig) ActiveToken(host string) (string, string) { return c[fmt.Sprintf("%s:%s", host, "oauth_token")], "oauth_token" } diff --git a/cmd/gh/main.go b/cmd/gh/main.go index bcce3b427..46b5490b7 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -17,6 +17,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/config/migration" "github.com/cli/cli/v2/internal/update" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmd/root" @@ -56,6 +57,14 @@ func mainRun() exitCode { ctx := context.Background() + if cfg, err := cmdFactory.Config(); err == nil { + var m migration.MultiAccount + if err := cfg.Migrate(m); err != nil { + fmt.Fprintf(stderr, "failed to migrate configuration: %s\n", err) + return exitError + } + } + updateCtx, updateCancel := context.WithCancel(ctx) defer updateCancel() updateMessageChan := make(chan *update.ReleaseInfo) diff --git a/docs/multiple-accounts.md b/docs/multiple-accounts.md new file mode 100644 index 000000000..696ce52c0 --- /dev/null +++ b/docs/multiple-accounts.md @@ -0,0 +1,211 @@ +# Multiple Accounts with the CLI - v2.40.0 + +Since its creation, `gh` has enforced a mapping of one account per host. Functionally, this meant that when targeting a +single host (e.g. github.com) each `auth login` would replace the token being used for API requests, and for git +operations when `gh` was configured as a git credential manager. Removing this limitation has been a [long requested +feature](https://github.com/cli/cli/issues/326), with many community members offering workarounds for a variety of use cases. +A particular shoutout to @gabe565 and his long term community support for https://github.com/gabe565/gh-profile in this space. + +With the release of `v2.40.0`, `gh` has begun supporting multiple accounts for some use cases on github.com and +in GitHub Enterprise. We recognise that there are a number of missing quality of life features, and we've opted +not to address the use case of automatic account switching based on some context (e.g. `pwd`, `git remote`, etc). +However, we hope many of those using these custom solutions will now find it easier to obtain and update tokens (via the standard +OAuth flow rather than as a PAT), and to store them securely in the system keyring managed by `gh`. + +We are by no means excluding these things from ever being native to `gh` but we wanted to ship this MVP and get more +feedback so that we can iterate on it with the community. + +## What is in scope for this release? + +The support for multiple accounts in this release is focused around `auth login` becoming additive in behaviour. +This allows for multiple accounts to be easily switched between using the new `auth switch` command. Switching the "active" +user for a host will swap the token used by `gh` for API requests, and for git operations when `gh` was configured as a +git credential manager. + +We have extended the `auth logout` command to switch account where possible if the currently active user is the target +of the `logout`. Finally we have extended `auth token`, `auth switch`, and `auth logout` with a +`--user` flag. This new flag in combination with `--hostname` can be used to disambiguate accounts when running +non-interactively. + +Here's an example usage. First, we can see that I have a single account `wilmartin_microsoft` logged in, and +`auth status` reports that this is the active account: + +``` +➜ gh auth status +github.com + ✓ Logged in to github.com account wilmartin_microsoft (keyring) + - Active account: true + - Git operations protocol: https + - Token: gho_************************************ + - Token scopes: 'gist', 'read:org', 'repo', 'workflow' +``` + +Running `auth login` and proceeding through the browser based OAuth flow as `williammartin`, we can see that +`auth status` now reports two accounts under `github.com`, and our new account is now marked as active. + +``` +➜ gh auth login +? What account do you want to log into? GitHub.com +? What is your preferred protocol for Git operations on this host? HTTPS +? How would you like to authenticate GitHub CLI? Login with a web browser + +! First copy your one-time code: A1F4-3B3C +Press Enter to open github.com in your browser... +✓ Authentication complete. +- gh config set -h github.com git_protocol https +✓ Configured git protocol +✓ Logged in as williammartin + +➜ gh auth status +github.com + ✓ Logged in to github.com account williammartin (keyring) + - Active account: true + - Git operations protocol: https + - Token: gho_************************************ + - Token scopes: 'gist', 'read:org', 'repo', 'workflow' + + ✓ Logged in to github.com account wilmartin_microsoft (keyring) + - Active account: false + - Git operations protocol: https + - Token: gho_************************************ + - Token scopes: 'gist', 'read:org', 'repo', 'workflow' +``` + +Fetching our username from the API shows that our active token correctly corresponds to `williammartin`: + +``` +➜ gh api /user | jq .login +"williammartin" +``` + +Now we can easily switch accounts using `gh auth switch`, and hitting the API shows that the active token has been +changed: + +``` +➜ gh auth switch +✓ Switched active account for github.com to wilmartin_microsoft + +➜ gh api /user | jq .login +"wilmartin_microsoft" +``` + +We can use `gh auth token --user` to get a specific token for a user (which should be handy for automated switching +solutions): + +``` +➜ GH_TOKEN=$(gh auth token --user williammartin) gh api /user | jq .login +"williammartin" +``` + +Finally, running `gh auth logout` presents a prompt when there are multiple choices for logout, and switches account +if there are any remaining logged into the host: + +``` +➜ gh auth logout +? What account do you want to log out of? wilmartin_microsoft (github.com) +✓ Logged out of github.com account wilmartin_microsoft +✓ Switched active account for github.com to williammartin +``` + +## What is out of scope for this release? + +As mentioned above, we know that this only addreses some of the requests around supporting multiple accounts. While +these are not out of scope forever, for this release some of the big things we have intentionally not included are: + * Automatic account switching based on some context (e.g. `pwd`, `git remote`, etc) + * Automatic configuration of git config such as `user.name` and `user.email` when switching + * User level configuration e.g. `williammartin` uses `vim` but `wilmartin_microsoft` uses `emacs` + +## What are some sharp edges in this release? + +As in any MVP there are going to be some sharp edges that need to be smoothed out over time. Here are a list of known +sharp edges in this release. + +### Data Migration + +The trickiest piece of this work was that the `hosts.yml` file only supported a mapping of one-to-one in the host to +account relationship. Having persistent data on disk that required a schema change presented a compatability challenge +both backwards for those who use [`go-gh`](https://github.com/cli/go-gh/) outside of `gh`, and forward for `gh` itself +where we try to ensure that it's possible to use older versions in case we accidentally make a breaking change for users. + +As such, from this release, running any command will attempt to migrate this data into a new format, and will +additionally add a `version` field into the `config.yml` to aid in our future maintainability. While we have tried +to maintain forward compatability (except in one edge case outlined below), and in the worst case you should be able +to remove these files and start from scratch, if you are concerned about the data in these files, we advise you to take +a backup. + +#### Forward Compatability Exclusion + +There is one known case using `--insecure-storage` that we don't maintain complete forward and backward compatability. +This occurs if you `auth login --insecure-storage`, upgrade to this release (which performs the data migration), run +`auth login --insecure-storage` again on an older release, then at some time later use `auth switch` to make this +account active. The symptom here would be usage of an older token (which may for example have different scopes). + +This occurs because we will only perform the data migration once, moving the original insecure token to a place where +it would later be used by `auth switch`. + +#### Immutable Config Users + +Some of our users lean on tools to manage their application configuration in an immutable manner for example using +https://github.com/nix-community/home-manager. These users will hit an error when we attempt to persist the new +`version` field to the `config.yml`. They will need to ensure that the `home-manager` configuration scripts are updated +to add `version: 1`. + +See https://github.com/nix-community/home-manager/issues/4744 for more details. + +### Auth Refresh + +Although this has always been possible, the multi account flow increases the likelihood of doing something surprising +with `auth refresh`. This command allows for a token to be updated with additional or fewer scopes. For example, +in the following example we add the `read:project` scope to the scopes for our currently active user `williammartin`, +and proceed through the OAuth browser flow as `williammartin`: + +``` +➜ gh auth refresh -s read:project +? What account do you want to refresh auth for? github.com + +! First copy your one-time code: E79E-5FA2 +Press Enter to open github.com in your browser... +✓ Authentication complete. + +➜ gh auth status +github.com + ✓ Logged in to github.com account williammartin (keyring) + - Active account: true + - Git operations protocol: https + - Token: gho_************************************ + - Token scopes: 'gist', 'read:org', 'read:project', 'repo', 'workflow' + + ✓ Logged in to github.com account wilmartin_microsoft (keyring) + + ✓ Logged in to github.com account wilmartin_microsoft (keyring) + - Active account: false + - Git operations protocol: https + - Token: gho_************************************ + - Token scopes: 'gist', 'read:org', 'repo', 'workflow' +``` + +However, what happens if I try to remove the `workflow` scope from my active user `williammartin` but proceed through +the OAuth browser flow as `wilmartin_microsoft`? + +``` +➜ gh auth refresh -r workflow + +! First copy your one-time code: EEA3-091C +Press Enter to open github.com in your browser... +error refreshing credentials for williammartin, received credentials for wilmartin_microsoft, did you use the correct account in the browser? +``` + +When adding or removing scopes for a user, the CLI gets the scopes for the current token and then requests a new token with the requested amended scopes. Unfortunately, when we go through the account switcher flow as a different user, we end up getting a token for the wrong user with surprising scopes. We don't believe that starting and ending a `refresh` as different accounts is +a use case we wish to support and has the potential for misuse. As such, we have begun erroring in this case. + +Note that a token has still been minted on the platform but `gh` will refuse to store it. We are investigating +alternative approaches with the platform team to put some better guardrails in place earlier in the flow. + +### Account Switcher on GitHub Enterprise + +When using `auth login` with github.com, if a user has multiple accounts in the browser, they should be presented +with an interstitial page that allows for proceeding as any of their accounts. However, for Device Control Flow OAuth +flows, this feature has not yet made it into GHES. + +For the moment, if you have multiple accounts on GHES that you wish to log in as, you will need to ensure that you +are authenticated as the correct user in the browser before running `auth login`. diff --git a/internal/authflow/flow.go b/internal/authflow/flow.go index ddac948cd..370e08784 100644 --- a/internal/authflow/flow.go +++ b/internal/authflow/flow.go @@ -109,7 +109,7 @@ type cfg struct { token string } -func (c cfg) Token(hostname string) (string, string) { +func (c cfg) ActiveToken(hostname string) (string, string) { return c.token, "oauth_token" } diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index afa47fbfd..52373f375 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -4,43 +4,24 @@ import ( "errors" "testing" + "github.com/cli/cli/v2/internal/config/migration" + "github.com/cli/cli/v2/internal/keyring" ghConfig "github.com/cli/go-gh/v2/pkg/config" "github.com/stretchr/testify/require" - "github.com/zalando/go-keyring" ) +// Note that NewIsolatedTestConfig sets up a Mock keyring as well func newTestAuthConfig(t *testing.T) *AuthConfig { - authCfg := AuthConfig{ - cfg: ghConfig.ReadFromString(""), - } - - // The real implementation of config.Read uses a sync.Once - // to read config files and initialise package level variables - // that are used from then on. - // - // This means that tests can't be isolated from each other, so - // we swap out the function here to return a new config each time. - ghConfig.Read = func(_ *ghConfig.Config) (*ghConfig.Config, error) { - return authCfg.cfg, nil - } - - // The config.Write method isn't defined in the same way as Read to allow - // the function to be swapped out and it does try to write to disk. - // - // We should consider whether it makes sense to change that but in the meantime - // we can use GH_CONFIG_DIR env var to ensure the tests remain isolated. - StubWriteConfig(t) - - return &authCfg + cfg, _ := NewIsolatedTestConfig(t) + return cfg.Authentication() } func TestTokenFromKeyring(t *testing.T) { // Given a keyring that contains a token for a host - keyring.MockInit() + authCfg := newTestAuthConfig(t) require.NoError(t, keyring.Set(keyringServiceName("github.com"), "", "test-token")) // When we get the token from the auth config - authCfg := newTestAuthConfig(t) token, err := authCfg.TokenFromKeyring("github.com") // Then it returns successfully with the correct token @@ -48,6 +29,29 @@ func TestTokenFromKeyring(t *testing.T) { require.Equal(t, "test-token", token) } +func TestTokenFromKeyringForUser(t *testing.T) { + // Given a keyring that contains a token for a host with a specific user + authCfg := newTestAuthConfig(t) + require.NoError(t, keyring.Set(keyringServiceName("github.com"), "test-user", "test-token")) + + // When we get the token from the auth config + token, err := authCfg.TokenFromKeyringForUser("github.com", "test-user") + + // Then it returns successfully with the correct token + require.NoError(t, err) + require.Equal(t, "test-token", token) +} + +func TestTokenFromKeyringForUserErrorsIfUsernameIsBlank(t *testing.T) { + authCfg := newTestAuthConfig(t) + + // When we get the token from the keyring for an empty username + _, err := authCfg.TokenFromKeyringForUser("github.com", "") + + // Then it returns an error + require.ErrorContains(t, err, "username cannot be blank") +} + func TestTokenStoredInConfig(t *testing.T) { // When the user has logged in insecurely authCfg := newTestAuthConfig(t) @@ -55,13 +59,13 @@ func TestTokenStoredInConfig(t *testing.T) { require.NoError(t, err) // When we get the token - token, source := authCfg.Token("github.com") + token, source := authCfg.ActiveToken("github.com") // Then the token is successfully fetched // and the source is set to oauth_token but this isn't great: // https://github.com/cli/go-gh/issues/94 require.Equal(t, "test-token", token) - require.Equal(t, "oauth_token", source) + require.Equal(t, oauthTokenKey, source) } func TestTokenStoredInEnv(t *testing.T) { @@ -70,7 +74,7 @@ func TestTokenStoredInEnv(t *testing.T) { t.Setenv("GH_TOKEN", "test-token") // When we get the token - token, source := authCfg.Token("github.com") + token, source := authCfg.ActiveToken("github.com") // Then the token is successfully fetched // and the source is set to the name of the env var @@ -80,13 +84,12 @@ func TestTokenStoredInEnv(t *testing.T) { func TestTokenStoredInKeyring(t *testing.T) { // When the user has logged in securely - keyring.MockInit() authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) require.NoError(t, err) // When we get the token - token, source := authCfg.Token("github.com") + token, source := authCfg.ActiveToken("github.com") // Then the token is successfully fetched // and the source is set to keyring @@ -96,14 +99,13 @@ func TestTokenStoredInKeyring(t *testing.T) { func TestTokenFromKeyringNonExistent(t *testing.T) { // Given a keyring that doesn't contain any tokens - keyring.MockInit() + authCfg := newTestAuthConfig(t) // When we try to get a token from the auth config - authCfg := newTestAuthConfig(t) _, err := authCfg.TokenFromKeyring("github.com") // Then it returns failure bubbling the ErrNotFound - require.ErrorIs(t, err, keyring.ErrNotFound) + require.ErrorContains(t, err, "secret not found in keyring") } func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { @@ -151,7 +153,7 @@ func TestUserNotLoggedIn(t *testing.T) { authCfg := newTestAuthConfig(t) // When we get the user - _, err := authCfg.User("github.com") + _, err := authCfg.ActiveUser("github.com") // Then it returns failure, bubbling the KeyNotFoundError var keyNotFoundError *ghConfig.KeyNotFoundError @@ -211,24 +213,29 @@ func TestDefaultHostLoggedInToOnlyOneHost(t *testing.T) { func TestLoginSecureStorageUsesKeyring(t *testing.T) { // Given a usable keyring - keyring.MockInit() authCfg := newTestAuthConfig(t) + host := "github.com" + user := "test-user" + token := "test-token" // When we login with secure storage - insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + insecureStorageUsed, err := authCfg.Login(host, user, token, "", true) // Then it returns success, notes that insecure storage was not used, and stores the token in the keyring require.NoError(t, err) require.False(t, insecureStorageUsed, "expected to use secure storage") - token, err := keyring.Get(keyringServiceName("github.com"), "") + gotToken, err := keyring.Get(keyringServiceName(host), "") require.NoError(t, err) - require.Equal(t, "test-token", token) + require.Equal(t, token, gotToken) + + gotToken, err = keyring.Get(keyringServiceName(host), user) + require.NoError(t, err) + require.Equal(t, token, gotToken) } func TestLoginSecureStorageRemovesOldInsecureConfigToken(t *testing.T) { // Given a usable keyring and an oauth token in the config - keyring.MockInit() authCfg := newTestAuthConfig(t) authCfg.cfg.Set([]string{hostsKey, "github.com", oauthTokenKey}, "old-token") @@ -242,8 +249,8 @@ func TestLoginSecureStorageRemovesOldInsecureConfigToken(t *testing.T) { func TestLoginSecureStorageWithErrorFallsbackAndReports(t *testing.T) { // Given a keyring that errors - keyring.MockInitWithError(errors.New("test-explosion")) authCfg := newTestAuthConfig(t) + keyring.MockInitWithError(errors.New("test-explosion")) // When we login with secure storage insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", true) @@ -279,23 +286,23 @@ func TestLoginSetsUserForProvidedHost(t *testing.T) { // Then it returns success and the user is set require.NoError(t, err) - user, err := authCfg.User("github.com") + user, err := authCfg.ActiveUser("github.com") require.NoError(t, err) require.Equal(t, "test-user", user) } func TestLoginSetsGitProtocolForProvidedHost(t *testing.T) { - // Given we are loggedin + // Given we are logged in authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) require.NoError(t, err) - // When we get the git protocol - protocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) + // When we get the host git protocol + hostProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) require.NoError(t, err) // Then it returns the git protocol we provided on login - require.Equal(t, "ssh", protocol) + require.Equal(t, "ssh", hostProtocol) } func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { @@ -311,22 +318,91 @@ func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { require.Contains(t, hosts, "github.com") } +// This test mimics the behaviour of logging in with a token, not providing +// a git protocol, and using secure storage. +func TestLoginAddsUserToConfigWithoutGitProtocolAndWithSecureStorage(t *testing.T) { + // Given we are not logged in + authCfg := newTestAuthConfig(t) + + // When we log in without git protocol and with secure storage + _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + require.NoError(t, err) + + // Then the username is added under the users config + users, err := authCfg.cfg.Keys([]string{hostsKey, "github.com", usersKey}) + require.NoError(t, err) + require.Contains(t, users, "test-user") +} + func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { // Given we are logged into a host - keyring.MockInit() authCfg := newTestAuthConfig(t) - _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + host := "github.com" + user := "test-user" + token := "test-token" + + _, err := authCfg.Login(host, user, token, "ssh", true) require.NoError(t, err) // When we logout - err = authCfg.Logout("github.com") + err = authCfg.Logout(host, user) // Then we return success, and the host and token are removed from the config and keyring require.NoError(t, err) - requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com"}) - _, err = keyring.Get(keyringServiceName("github.com"), "") - require.ErrorIs(t, err, keyring.ErrNotFound) + requireNoKey(t, authCfg.cfg, []string{hostsKey, host}) + _, err = keyring.Get(keyringServiceName(host), "") + require.ErrorContains(t, err, "secret not found in keyring") + _, err = keyring.Get(keyringServiceName(host), user) + require.ErrorContains(t, err, "secret not found in keyring") +} + +func TestLogoutOfActiveUserSwitchesUserIfPossible(t *testing.T) { + // Given we have two accounts logged into a host + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "inactive-user", "test-token-1", "ssh", true) + require.NoError(t, err) + + _, err = authCfg.Login("github.com", "active-user", "test-token-2", "https", true) + require.NoError(t, err) + + // When we logout of the active user + err = authCfg.Logout("github.com", "active-user") + + // Then we return success and the inactive user is now active + require.NoError(t, err) + activeUser, err := authCfg.ActiveUser("github.com") + require.NoError(t, err) + require.Equal(t, "inactive-user", activeUser) + + token, err := authCfg.TokenFromKeyring("github.com") + require.NoError(t, err) + require.Equal(t, "test-token-1", token) + + usersForHost := authCfg.UsersForHost("github.com") + require.NotContains(t, "active-user", usersForHost) +} + +func TestLogoutOfInactiveUserDoesNotSwitchUser(t *testing.T) { + // Given we have two accounts logged into a host + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "inactive-user-1", "test-token-1.1", "ssh", true) + require.NoError(t, err) + + _, err = authCfg.Login("github.com", "inactive-user-2", "test-token-1.2", "ssh", true) + require.NoError(t, err) + + _, err = authCfg.Login("github.com", "active-user", "test-token-2", "https", true) + require.NoError(t, err) + + // When we logout of an inactive user + err = authCfg.Logout("github.com", "inactive-user-1") + + // Then we return success and the active user is still active + require.NoError(t, err) + activeUser, err := authCfg.ActiveUser("github.com") + require.NoError(t, err) + require.Equal(t, "active-user", activeUser) } // Note that I'm not sure this test enforces particularly desirable behaviour @@ -343,12 +419,229 @@ func TestLogoutIgnoresErrorsFromConfigAndKeyring(t *testing.T) { authCfg := newTestAuthConfig(t) // When we logout - err := authCfg.Logout("github.com") + err := authCfg.Logout("github.com", "test-user") // Then it returns success anyway, suppressing the errors require.NoError(t, err) } +func TestSwitchUserMakesSecureTokenActive(t *testing.T) { + // Given we have a user with a secure token + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", true) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + require.NoError(t, err) + + // When we switch to that user + require.NoError(t, authCfg.SwitchUser("github.com", "test-user-1")) + + // Their secure token is now active + token, err := authCfg.TokenFromKeyring("github.com") + require.NoError(t, err) + require.Equal(t, "test-token-1", token) +} + +func TestSwitchUserMakesInsecureTokenActive(t *testing.T) { + // Given we have a user with an insecure token + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", false) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", false) + require.NoError(t, err) + + // When we switch to that user + require.NoError(t, authCfg.SwitchUser("github.com", "test-user-1")) + + // Their insecure token is now active + token, source := authCfg.ActiveToken("github.com") + require.Equal(t, "test-token-1", token) + require.Equal(t, oauthTokenKey, source) +} + +func TestSwitchUserUpdatesTheActiveUser(t *testing.T) { + // Given we have two users logged into a host + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", false) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", false) + require.NoError(t, err) + + // When we switch to the other user + require.NoError(t, authCfg.SwitchUser("github.com", "test-user-1")) + + // Then the active user is updated + activeUser, err := authCfg.ActiveUser("github.com") + require.NoError(t, err) + require.Equal(t, "test-user-1", activeUser) +} + +func TestSwitchUserErrorsImmediatelyIfTheActiveTokenComesFromEnvironment(t *testing.T) { + // Given we have a token in the env + authCfg := newTestAuthConfig(t) + t.Setenv("GH_TOKEN", "unimportant-test-value") + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", true) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + require.NoError(t, err) + + // When we switch to a user + err = authCfg.SwitchUser("github.com", "test-user-1") + + // Then it errors immediately with an informative message + require.ErrorContains(t, err, "currently active token for github.com is from GH_TOKEN") +} + +func TestSwitchUserErrorsAndRestoresUserAndInsecureConfigUnderFailure(t *testing.T) { + // Given we have a user but no token can be found (because we deleted them, simulating an error case) + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", true) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", false) + require.NoError(t, err) + + require.NoError(t, keyring.Delete(keyringServiceName("github.com"), "test-user-1")) + + // When we switch to the user + err = authCfg.SwitchUser("github.com", "test-user-1") + + // Then it returns an error + require.EqualError(t, err, "no token found for test-user-1") + + // And restores the previous state + activeUser, err := authCfg.ActiveUser("github.com") + require.NoError(t, err) + require.Equal(t, "test-user-2", activeUser) + + token, source := authCfg.ActiveToken("github.com") + require.Equal(t, "test-token-2", token) + require.Equal(t, "oauth_token", source) +} + +func TestSwitchUserErrorsAndRestoresUserAndKeyringUnderFailure(t *testing.T) { + // Given we have a user but no token can be found (because we deleted them, simulating an error case) + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", false) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + require.NoError(t, err) + + require.NoError(t, authCfg.cfg.Remove([]string{hostsKey, "github.com", usersKey, "test-user-1", oauthTokenKey})) + + // When we switch to the user + err = authCfg.SwitchUser("github.com", "test-user-1") + + // Then it returns an error + require.EqualError(t, err, "no token found for test-user-1") + + // And restores the previous state + activeUser, err := authCfg.ActiveUser("github.com") + require.NoError(t, err) + require.Equal(t, "test-user-2", activeUser) + + token, source := authCfg.ActiveToken("github.com") + require.Equal(t, "test-token-2", token) + require.Equal(t, "keyring", source) +} + +func TestSwitchClearsActiveSecureTokenWhenSwitchingToInsecureUser(t *testing.T) { + // Given we have an active secure token + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", false) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + require.NoError(t, err) + + // When we switch to an insecure user + require.NoError(t, authCfg.SwitchUser("github.com", "test-user-1")) + + // Then the active secure token is cleared + _, err = authCfg.TokenFromKeyring("github.com") + require.Error(t, err) +} + +func TestSwitchClearsActiveInsecureTokenWhenSwitchingToSecureUser(t *testing.T) { + // Given we have an active insecure token + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", true) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", false) + require.NoError(t, err) + + // When we switch to a secure user + require.NoError(t, authCfg.SwitchUser("github.com", "test-user-1")) + + // Then the active insecure token is cleared + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com", oauthTokenKey}) +} + +func TestUsersForHostNoHost(t *testing.T) { + // Given we have a config with no hosts + authCfg := newTestAuthConfig(t) + + // When we get the users for a host that doesn't exist + users := authCfg.UsersForHost("github.com") + + // Then it returns nil + require.Nil(t, users) +} + +func TestUsersForHostWithUsers(t *testing.T) { + // Given we have a config with a host and users + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token", "ssh", false) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token", "ssh", false) + require.NoError(t, err) + + // When we get the users for that host + users := authCfg.UsersForHost("github.com") + + // Then it succeeds and returns the users + require.Equal(t, []string{"test-user-1", "test-user-2"}, users) +} + +func TestTokenForUserSecureLogin(t *testing.T) { + // Given a user has logged in securely + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token", "ssh", true) + require.NoError(t, err) + + // When we get the token + token, source, err := authCfg.TokenForUser("github.com", "test-user-1") + + // Then it returns the token and the source as keyring + require.NoError(t, err) + require.Equal(t, "test-token", token) + require.Equal(t, "keyring", source) +} + +func TestTokenForUserInsecureLogin(t *testing.T) { + // Given a user has logged in insecurely + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token", "ssh", false) + require.NoError(t, err) + + // When we get the token + token, source, err := authCfg.TokenForUser("github.com", "test-user-1") + + // Then it returns the token and the source as oauth_token + require.NoError(t, err) + require.Equal(t, "test-token", token) + require.Equal(t, "oauth_token", source) +} + +func TestTokenForUserNotFoundErrors(t *testing.T) { + // Given a user has not logged in + authCfg := newTestAuthConfig(t) + + // When we get the token + _, _, err := authCfg.TokenForUser("github.com", "test-user-1") + + // Then it returns an error + require.EqualError(t, err, "no token found for 'test-user-1'") +} + func requireKeyWithValue(t *testing.T, cfg *ghConfig.Config, keys []string, value string) { t.Helper() @@ -365,3 +658,212 @@ func requireNoKey(t *testing.T, cfg *ghConfig.Config, keys []string) { var keyNotFoundError *ghConfig.KeyNotFoundError require.ErrorAs(t, err, &keyNotFoundError) } + +// Post migration tests + +func TestUserWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + // Then we can still get the user correctly + user, err := authCfg.ActiveUser("github.com") + require.NoError(t, err) + require.Equal(t, "test-user", user) +} + +func TestGitProtocolWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration with a non-default git protocol + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + // Then we can still get the git protocol correctly + gitProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) + require.NoError(t, err) + require.Equal(t, "ssh", gitProtocol) +} + +func TestHostsWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "ghe.io", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + // Then we can still get the hosts correctly + hosts := authCfg.Hosts() + require.Contains(t, hosts, "ghe.io") +} + +func TestDefaultHostWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration to an enterprise host + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "ghe.io", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + // Then the default host is still the enterprise host + defaultHost, source := authCfg.DefaultHost() + require.Equal(t, "ghe.io", defaultHost) + require.Equal(t, hostsKey, source) +} + +func TestTokenWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + // Then we can still get the token correctly + token, source := authCfg.ActiveToken("github.com") + require.Equal(t, "test-token", token) + require.Equal(t, oauthTokenKey, source) +} + +func TestLogoutRightAfterMigrationRemovesHost(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + host := "github.com" + user := "test-user" + token := "test-token" + + _, err := preMigrationLogin(authCfg, host, user, token, "ssh", false) + require.NoError(t, err) + + // When we migrate and logout + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + require.NoError(t, authCfg.Logout(host, user)) + + // Then the host is removed from the config + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com"}) +} + +func TestLoginInsecurePostMigrationUsesConfigForToken(t *testing.T) { + // Given we have not logged in + authCfg := newTestAuthConfig(t) + + // When we migrate and login with insecure storage + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", false) + + // Then it returns success, notes that insecure storage was used, and stores the token in the config + // both under the host and under the user + require.NoError(t, err) + + require.True(t, insecureStorageUsed, "expected to use insecure storage") + requireKeyWithValue(t, authCfg.cfg, []string{hostsKey, "github.com", oauthTokenKey}, "test-token") + requireKeyWithValue(t, authCfg.cfg, []string{hostsKey, "github.com", usersKey, "test-user", oauthTokenKey}, "test-token") +} + +func TestLoginPostMigrationSetsGitProtocol(t *testing.T) { + // Given we have logged in after migration + authCfg := newTestAuthConfig(t) + + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we get the host git protocol + hostProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) + require.NoError(t, err) + + // Then it returns the git protocol we provided on login + require.Equal(t, "ssh", hostProtocol) +} + +func TestLoginPostMigrationSetsUser(t *testing.T) { + // Given we have logged in after migration + authCfg := newTestAuthConfig(t) + + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we get the user + user, err := authCfg.ActiveUser("github.com") + + // Then it returns success and the user we provided on login + require.NoError(t, err) + require.Equal(t, "test-user", user) +} + +func TestLoginSecurePostMigrationRemovesTokenFromConfig(t *testing.T) { + // Given we have logged in insecurely + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "", false) + require.NoError(t, err) + + // When we migrate and login again with secure storage + var m migration.MultiAccount + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) + + _, err = authCfg.Login("github.com", "test-user", "test-token", "", true) + + // Then it returns success, having removed the old insecure oauth token entry + require.NoError(t, err) + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com", oauthTokenKey}) + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com", usersKey, "test-user", oauthTokenKey}) +} + +// Copied and pasted directly from the trunk branch before doing any work on +// login, plus the addition of AuthConfig as the first arg since it is a method +// receiver in the real implementation. +func preMigrationLogin(c *AuthConfig, hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { + var setErr error + if secureStorage { + if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil { + // Clean up the previous oauth_token from the config file. + _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + } + } + insecureStorageUsed := false + if !secureStorage || setErr != nil { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + insecureStorageUsed = true + } + + c.cfg.Set([]string{hostsKey, hostname, userKey}, username) + + if gitProtocol != "" { + c.cfg.Set([]string{hostsKey, hostname, gitProtocolKey}, gitProtocol) + } + return insecureStorageUsed, ghConfig.Write(c.cfg) +} diff --git a/internal/config/config.go b/internal/config/config.go index 4e48c8a83..d615a57c5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,8 +1,11 @@ package config import ( + "errors" + "fmt" "os" "path/filepath" + "slices" "github.com/cli/cli/v2/internal/keyring" ghAuth "github.com/cli/go-gh/v2/pkg/auth" @@ -19,6 +22,9 @@ const ( oauthTokenKey = "oauth_token" pagerKey = "pager" promptKey = "prompt" + userKey = "user" + usersKey = "users" + versionKey = "version" ) // This interface describes interacting with some persistent configuration for gh. @@ -28,6 +34,7 @@ type Config interface { GetOrDefault(string, string) (string, error) Set(string, string, string) Write() error + Migrate(Migration) error Aliases() *AliasConfig Authentication() *AuthConfig @@ -37,6 +44,27 @@ type Config interface { HTTPUnixSocket(string) string Pager(string) string Prompt(string) string + Version() string +} + +// Migration is the interace that config migrations must implement. +// +// Migrations will receive a copy of the config, and should modify that copy +// as necessary. After migration has completed, the modified config contents +// will be used. +// +// The calling code is expected to verify that the current version of the config +// matches the PreVersion of the migration before calling Do, and will set the +// config version to the PostVersion after the migration has completed successfully. +// +//go:generate moq -rm -out migration_mock.go . Migration +type Migration interface { + // PreVersion is the required config version for this to be applied + PreVersion() string + // PostVersion is the config version that must be applied after migration + PostVersion() string + // Do is expected to apply any necessary changes to the config in place + Do(*ghConfig.Config) error } func NewConfig() (Config, error) { @@ -81,7 +109,12 @@ func (c *cfg) Set(hostname, key, value string) { c.cfg.Set([]string{key}, value) return } + c.cfg.Set([]string{hostsKey, hostname, key}, value) + + if user, _ := c.cfg.Get([]string{hostsKey, hostname, userKey}); user != "" { + c.cfg.Set([]string{hostsKey, hostname, usersKey, user, key}, value) + } } func (c *cfg) Write() error { @@ -126,6 +159,38 @@ func (c *cfg) Prompt(hostname string) string { return val } +func (c *cfg) Version() string { + val, _ := c.GetOrDefault("", versionKey) + return val +} + +func (c *cfg) Migrate(m Migration) error { + version := c.Version() + + // If migration has already occured then do not attempt to migrate again. + if m.PostVersion() == version { + return nil + } + + // If migration is incompatible with current version then return an error. + if m.PreVersion() != version { + return fmt.Errorf("failed to migrate as %q pre migration version did not match config version %q", m.PreVersion(), version) + } + + if err := m.Do(c.cfg); err != nil { + return fmt.Errorf("failed to migrate config: %s", err) + } + + c.Set("", versionKey, m.PostVersion()) + + // Then write out our migrated config. + if err := c.Write(); err != nil { + return fmt.Errorf("failed to write config after migration: %s", err) + } + + return nil +} + func defaultFor(key string) (string, bool) { for _, co := range ConfigOptions() { if co.Key == key { @@ -145,10 +210,10 @@ type AuthConfig struct { tokenOverride func(string) (string, string) } -// Token will retrieve the auth token for the given hostname, +// ActiveToken will retrieve the active auth token for the given hostname, // searching environment variables, plain text config, and // lastly encrypted storage. -func (c *AuthConfig) Token(hostname string) (string, string) { +func (c *AuthConfig) ActiveToken(hostname string) (string, string) { if c.tokenOverride != nil { return c.tokenOverride(hostname) } @@ -184,9 +249,9 @@ func (c *AuthConfig) HasEnvToken() bool { return token != "" } -// SetToken will override any token resolution and return the given -// token and source for all calls to Token. Use for testing purposes only. -func (c *AuthConfig) SetToken(token, source string) { +// SetActiveToken will override any token resolution and return the given +// token and source for all calls to ActiveToken. Use for testing purposes only. +func (c *AuthConfig) SetActiveToken(token, source string) { c.tokenOverride = func(_ string) (string, string) { return token, source } @@ -198,9 +263,24 @@ func (c *AuthConfig) TokenFromKeyring(hostname string) (string, error) { return keyring.Get(keyringServiceName(hostname), "") } -// User will retrieve the username for the logged in user at the given hostname. -func (c *AuthConfig) User(hostname string) (string, error) { - return c.cfg.Get([]string{hostsKey, hostname, "user"}) +// TokenFromKeyringForUser will retrieve the auth token for the given hostname +// and username, only searching in encrypted storage. +// +// An empty username will return an error because the potential to return +// the currently active token under surprising cases is just too high to risk +// compared to the utility of having the function being smart. +func (c *AuthConfig) TokenFromKeyringForUser(hostname, username string) (string, error) { + if username == "" { + return "", errors.New("username cannot be blank") + } + + return keyring.Get(keyringServiceName(hostname), username) +} + +// ActiveUser will retrieve the username for the active user at the given hostname. +// This will not be accurate if the oauth token is set from an environment variable. +func (c *AuthConfig) ActiveUser(hostname string) (string, error) { + return c.cfg.Get([]string{hostsKey, hostname, userKey}) } func (c *AuthConfig) Hosts() []string { @@ -237,35 +317,163 @@ func (c *AuthConfig) SetDefaultHost(host, source string) { // If the encrypt option is specified it will first try to store the auth token // in encrypted storage and will fall back to the plain text config file. func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { + // In this section we set up the users config var setErr error if secureStorage { - if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil { - // Clean up the previous oauth_token from the config file. - _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + // Try to set the token for this user in the encrypted storage for later switching + setErr = keyring.Set(keyringServiceName(hostname), username, token) + if setErr == nil { + // Clean up the previous oauth_token from the config file, if there were one + _ = c.cfg.Remove([]string{hostsKey, hostname, usersKey, username, oauthTokenKey}) } } insecureStorageUsed := false if !secureStorage || setErr != nil { - c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + // And set the oauth token under the user for later switching + c.cfg.Set([]string{hostsKey, hostname, usersKey, username, oauthTokenKey}, token) insecureStorageUsed = true } - c.cfg.Set([]string{hostsKey, hostname, "user"}, username) - if gitProtocol != "" { + // Set the host level git protocol + // Although it might be expected that this is handled by switch, git protocol + // is currently a host level config and not a user level config, so any change + // will overwrite the protocol for all users on the host. c.cfg.Set([]string{hostsKey, hostname, gitProtocolKey}, gitProtocol) } - return insecureStorageUsed, ghConfig.Write(c.cfg) + + // Create the username key with an empty value so it will be + // written even when there are no keys set under it. + if _, getErr := c.cfg.Get([]string{hostsKey, hostname, usersKey, username}); getErr != nil { + c.cfg.Set([]string{hostsKey, hostname, usersKey, username}, "") + } + + // Then we activate the new user + return insecureStorageUsed, c.activateUser(hostname, username) +} + +func (c *AuthConfig) SwitchUser(hostname, user string) error { + previouslyActiveUser, err := c.ActiveUser(hostname) + if err != nil { + return fmt.Errorf("failed to get active user: %s", err) + } + + previouslyActiveToken, previousSource := c.ActiveToken(hostname) + if previousSource != "keyring" && previousSource != "oauth_token" { + return fmt.Errorf("currently active token for %s is from %s", hostname, previousSource) + } + + err = c.activateUser(hostname, user) + if err != nil { + // Given that activateUser can only fail before the config is written, or when writing the config + // we know for sure that the config has not been written. However, we still should restore it back + // to its previous clean state just in case something else tries to make use of the config, or tries + // to write it again. + if previousSource == "keyring" { + if setErr := keyring.Set(keyringServiceName(hostname), "", previouslyActiveToken); setErr != nil { + err = errors.Join(err, setErr) + } + } + + if previousSource == "oauth_token" { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, previouslyActiveToken) + } + c.cfg.Set([]string{hostsKey, hostname, userKey}, previouslyActiveUser) + + return err + } + + return nil } // Logout will remove user, git protocol, and auth token for the given hostname. // It will remove the auth token from the encrypted storage if it exists there. -func (c *AuthConfig) Logout(hostname string) error { - _ = c.cfg.Remove([]string{hostsKey, hostname}) +func (c *AuthConfig) Logout(hostname, username string) error { + users := c.UsersForHost(hostname) + + // If there is only one (or zero) users, then we remove the host + // and unset the keyring tokens. + if len(users) < 2 { + _ = c.cfg.Remove([]string{hostsKey, hostname}) + _ = keyring.Delete(keyringServiceName(hostname), "") + _ = keyring.Delete(keyringServiceName(hostname), username) + return ghConfig.Write(c.cfg) + } + + // Otherwise, we remove the user from this host + _ = c.cfg.Remove([]string{hostsKey, hostname, usersKey, username}) + + // This error is ignorable because we already know there is an active user for the host + activeUser, _ := c.ActiveUser(hostname) + + // If the user we're removing isn't active, then we just write the config + if activeUser != username { + return ghConfig.Write(c.cfg) + } + + // Otherwise we get the first user in the slice that isn't the user we're removing + switchUserIdx := slices.IndexFunc(users, func(n string) bool { + return n != username + }) + + // And activate them + return c.activateUser(hostname, users[switchUserIdx]) +} + +func (c *AuthConfig) activateUser(hostname, user string) error { + // We first need to idempotently clear out any set tokens for the host _ = keyring.Delete(keyringServiceName(hostname), "") + _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + + // Then we'll move the keyring token or insecure token as necessary, only one of the + // following branches should be true. + + // If there is a token in the secure keyring for the user, move it to the active slot + var tokenSwitched bool + if token, err := keyring.Get(keyringServiceName(hostname), user); err == nil { + if err = keyring.Set(keyringServiceName(hostname), "", token); err != nil { + return fmt.Errorf("failed to move active token in keyring: %v", err) + } + tokenSwitched = true + } + + // If there is a token in the insecure config for the user, move it to the active field + if token, err := c.cfg.Get([]string{hostsKey, hostname, usersKey, user, oauthTokenKey}); err == nil { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + tokenSwitched = true + } + + if !tokenSwitched { + return fmt.Errorf("no token found for %s", user) + } + + // Then we'll update the active user for the host + c.cfg.Set([]string{hostsKey, hostname, userKey}, user) + return ghConfig.Write(c.cfg) } +func (c *AuthConfig) UsersForHost(hostname string) []string { + users, err := c.cfg.Keys([]string{hostsKey, hostname, usersKey}) + if err != nil { + return nil + } + + return users +} + +func (c *AuthConfig) TokenForUser(hostname, user string) (string, string, error) { + if token, err := keyring.Get(keyringServiceName(hostname), user); err == nil { + return token, "keyring", nil + } + + if token, err := c.cfg.Get([]string{hostsKey, hostname, usersKey, user, oauthTokenKey}); err == nil { + return token, "oauth_token", nil + } + + return "", "default", fmt.Errorf("no token found for '%s'", user) +} + func keyringServiceName(hostname string) string { return "gh:" + hostname } @@ -303,7 +511,12 @@ func fallbackConfig() *ghConfig.Config { return ghConfig.ReadFromString(defaultConfigStr) } +// The schema version in here should match the PostVersion of whatever the +// last migration we decided to run is. Therefore, if we run a new migration, +// this should be bumped. const defaultConfigStr = ` +# The current version of the config schema +version: 1 # What protocol to use when performing git operations. Supported values: ssh, https git_protocol: https # What editor gh should run when creating issues, pull requests, etc. If blank, will refer to environment. diff --git a/internal/config/config_mock.go b/internal/config/config_mock.go index aff4cb9d4..586078a2c 100644 --- a/internal/config/config_mock.go +++ b/internal/config/config_mock.go @@ -38,6 +38,9 @@ var _ Config = &ConfigMock{} // HTTPUnixSocketFunc: func(s string) string { // panic("mock out the HTTPUnixSocket method") // }, +// MigrateFunc: func(migration Migration) error { +// panic("mock out the Migrate method") +// }, // PagerFunc: func(s string) string { // panic("mock out the Pager method") // }, @@ -47,6 +50,9 @@ var _ Config = &ConfigMock{} // SetFunc: func(s1 string, s2 string, s3 string) { // panic("mock out the Set method") // }, +// VersionFunc: func() string { +// panic("mock out the Version method") +// }, // WriteFunc: func() error { // panic("mock out the Write method") // }, @@ -78,6 +84,9 @@ type ConfigMock struct { // HTTPUnixSocketFunc mocks the HTTPUnixSocket method. HTTPUnixSocketFunc func(s string) string + // MigrateFunc mocks the Migrate method. + MigrateFunc func(migration Migration) error + // PagerFunc mocks the Pager method. PagerFunc func(s string) string @@ -87,6 +96,9 @@ type ConfigMock struct { // SetFunc mocks the Set method. SetFunc func(s1 string, s2 string, s3 string) + // VersionFunc mocks the Version method. + VersionFunc func() string + // WriteFunc mocks the Write method. WriteFunc func() error @@ -125,6 +137,11 @@ type ConfigMock struct { // S is the s argument value. S string } + // Migrate holds details about calls to the Migrate method. + Migrate []struct { + // Migration is the migration argument value. + Migration Migration + } // Pager holds details about calls to the Pager method. Pager []struct { // S is the s argument value. @@ -144,6 +161,9 @@ type ConfigMock struct { // S3 is the s3 argument value. S3 string } + // Version holds details about calls to the Version method. + Version []struct { + } // Write holds details about calls to the Write method. Write []struct { } @@ -155,9 +175,11 @@ type ConfigMock struct { lockGetOrDefault sync.RWMutex lockGitProtocol sync.RWMutex lockHTTPUnixSocket sync.RWMutex + lockMigrate sync.RWMutex lockPager sync.RWMutex lockPrompt sync.RWMutex lockSet sync.RWMutex + lockVersion sync.RWMutex lockWrite sync.RWMutex } @@ -379,6 +401,38 @@ func (mock *ConfigMock) HTTPUnixSocketCalls() []struct { return calls } +// Migrate calls MigrateFunc. +func (mock *ConfigMock) Migrate(migration Migration) error { + if mock.MigrateFunc == nil { + panic("ConfigMock.MigrateFunc: method is nil but Config.Migrate was just called") + } + callInfo := struct { + Migration Migration + }{ + Migration: migration, + } + mock.lockMigrate.Lock() + mock.calls.Migrate = append(mock.calls.Migrate, callInfo) + mock.lockMigrate.Unlock() + return mock.MigrateFunc(migration) +} + +// MigrateCalls gets all the calls that were made to Migrate. +// Check the length with: +// +// len(mockedConfig.MigrateCalls()) +func (mock *ConfigMock) MigrateCalls() []struct { + Migration Migration +} { + var calls []struct { + Migration Migration + } + mock.lockMigrate.RLock() + calls = mock.calls.Migrate + mock.lockMigrate.RUnlock() + return calls +} + // Pager calls PagerFunc. func (mock *ConfigMock) Pager(s string) string { if mock.PagerFunc == nil { @@ -483,6 +537,33 @@ func (mock *ConfigMock) SetCalls() []struct { return calls } +// Version calls VersionFunc. +func (mock *ConfigMock) Version() string { + if mock.VersionFunc == nil { + panic("ConfigMock.VersionFunc: method is nil but Config.Version was just called") + } + callInfo := struct { + }{} + mock.lockVersion.Lock() + mock.calls.Version = append(mock.calls.Version, callInfo) + mock.lockVersion.Unlock() + return mock.VersionFunc() +} + +// VersionCalls gets all the calls that were made to Version. +// Check the length with: +// +// len(mockedConfig.VersionCalls()) +func (mock *ConfigMock) VersionCalls() []struct { +} { + var calls []struct { + } + mock.lockVersion.RLock() + calls = mock.calls.Version + mock.lockVersion.RUnlock() + return calls +} + // Write calls WriteFunc. func (mock *ConfigMock) Write() error { if mock.WriteFunc == nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6acc32245..f6832f4a4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -22,6 +22,7 @@ func TestNewConfigProvidesFallback(t *testing.T) { } _, err := NewConfig() require.NoError(t, err) + requireKeyWithValue(t, spiedCfg, []string{versionKey}, "1") requireKeyWithValue(t, spiedCfg, []string{gitProtocolKey}, "https") requireKeyWithValue(t, spiedCfg, []string{editorKey}, "") requireKeyWithValue(t, spiedCfg, []string{promptKey}, "enabled") @@ -162,3 +163,44 @@ func TestFallbackConfig(t *testing.T) { requireKeyWithValue(t, cfg, []string{browserKey}, "") requireNoKey(t, cfg, []string{"unknown"}) } + +func TestSetTopLevelKey(t *testing.T) { + c := newTestConfig() + host := "" + key := "top-level-key" + val := "top-level-value" + c.Set(host, key, val) + requireKeyWithValue(t, c.cfg, []string{key}, val) +} + +func TestSetHostSpecificKey(t *testing.T) { + c := newTestConfig() + host := "github.com" + key := "host-level-key" + val := "host-level-value" + c.Set(host, key, val) + requireKeyWithValue(t, c.cfg, []string{hostsKey, host, key}, val) +} + +func TestSetUserSpecificKey(t *testing.T) { + c := newTestConfig() + host := "github.com" + user := "test-user" + c.cfg.Set([]string{hostsKey, host, userKey}, user) + + key := "host-level-key" + val := "host-level-value" + c.Set(host, key, val) + requireKeyWithValue(t, c.cfg, []string{hostsKey, host, key}, val) + requireKeyWithValue(t, c.cfg, []string{hostsKey, host, usersKey, user, key}, val) +} + +func TestSetUserSpecificKeyNoUserPresent(t *testing.T) { + c := newTestConfig() + host := "github.com" + key := "host-level-key" + val := "host-level-value" + c.Set(host, key, val) + requireKeyWithValue(t, c.cfg, []string{hostsKey, host, key}, val) + requireNoKey(t, c.cfg, []string{hostsKey, host, usersKey}) +} diff --git a/internal/config/migrate_test.go b/internal/config/migrate_test.go new file mode 100644 index 000000000..193848558 --- /dev/null +++ b/internal/config/migrate_test.go @@ -0,0 +1,261 @@ +package config + +import ( + "bytes" + "errors" + "io" + "os" + "path/filepath" + "testing" + + ghConfig "github.com/cli/go-gh/v2/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestMigrationAppliedSuccessfully(t *testing.T) { + readConfig := StubWriteConfig(t) + + // Given we have a migrator that writes some keys to the top level config + // and hosts key + c := ghConfig.ReadFromString(testFullConfig()) + + topLevelKey := []string{"toplevelkey"} + newHostKey := []string{hostsKey, "newhost"} + + migration := mockMigration(func(config *ghConfig.Config) error { + config.Set(topLevelKey, "toplevelvalue") + config.Set(newHostKey, "newhostvalue") + return nil + }) + + // When we run the migration + conf := cfg{c} + require.NoError(t, conf.Migrate(migration)) + + // Then our original config is updated with the migration applied + requireKeyWithValue(t, c, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, c, newHostKey, "newhostvalue") + + // And our config / hosts changes are persisted to their relevant files + // Note that this is real janky. We have writers that represent the + // top level config and the hosts key but we don't merge them back together + // so when we look into the hosts data, we don't nest the key we're + // looking for under the hosts key ¯\_(ツ)_/¯ + var configBuf bytes.Buffer + var hostsBuf bytes.Buffer + readConfig(&configBuf, &hostsBuf) + persistedCfg := ghConfig.ReadFromString(configBuf.String()) + persistedHosts := ghConfig.ReadFromString(hostsBuf.String()) + + requireKeyWithValue(t, persistedCfg, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, persistedHosts, []string{"newhost"}, "newhostvalue") +} + +func TestMigrationAppliedBumpsVersion(t *testing.T) { + readConfig := StubWriteConfig(t) + + // Given we have a migration with a pre version that matches + // the version in the config + c := ghConfig.ReadFromString(testFullConfig()) + c.Set([]string{versionKey}, "expected-pre-version") + topLevelKey := []string{"toplevelkey"} + + migration := &MigrationMock{ + DoFunc: func(config *ghConfig.Config) error { + config.Set(topLevelKey, "toplevelvalue") + return nil + }, + PreVersionFunc: func() string { + return "expected-pre-version" + }, + PostVersionFunc: func() string { + return "expected-post-version" + }, + } + + // When we migrate + conf := cfg{c} + require.NoError(t, conf.Migrate(migration)) + + // Then our original config is updated with the migration applied + requireKeyWithValue(t, c, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, c, []string{versionKey}, "expected-post-version") + + // And our config / hosts changes are persisted to their relevant files + var configBuf bytes.Buffer + readConfig(&configBuf, io.Discard) + persistedCfg := ghConfig.ReadFromString(configBuf.String()) + + requireKeyWithValue(t, persistedCfg, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, persistedCfg, []string{versionKey}, "expected-post-version") +} + +func TestMigrationIsNoopWhenAlreadyApplied(t *testing.T) { + // Given we have a migration with a post version that matches + // the version in the config + c := ghConfig.ReadFromString(testFullConfig()) + c.Set([]string{versionKey}, "expected-post-version") + + migration := &MigrationMock{ + DoFunc: func(config *ghConfig.Config) error { + return errors.New("is not called") + }, + PreVersionFunc: func() string { + return "is not called" + }, + PostVersionFunc: func() string { + return "expected-post-version" + }, + } + + // When we run Migrate + conf := cfg{c} + err := conf.Migrate(migration) + + // Then there is nothing done and the config is not modified + require.NoError(t, err) + requireKeyWithValue(t, c, []string{versionKey}, "expected-post-version") +} + +func TestMigrationErrorsWhenPreVersionMismatch(t *testing.T) { + StubWriteConfig(t) + + // Given we have a migration with a pre version that does not match + // the version in the config + c := ghConfig.ReadFromString(testFullConfig()) + c.Set([]string{versionKey}, "not-expected-pre-version") + topLevelKey := []string{"toplevelkey"} + + migration := &MigrationMock{ + DoFunc: func(config *ghConfig.Config) error { + config.Set(topLevelKey, "toplevelvalue") + return nil + }, + PreVersionFunc: func() string { + return "expected-pre-version" + }, + PostVersionFunc: func() string { + return "not-expected" + }, + } + + // When we run Migrate + conf := cfg{c} + err := conf.Migrate(migration) + + // Then there is an error the migration is not applied and the version is not modified + require.ErrorContains(t, err, `failed to migrate as "expected-pre-version" pre migration version did not match config version "not-expected-pre-version"`) + requireNoKey(t, c, topLevelKey) + requireKeyWithValue(t, c, []string{versionKey}, "not-expected-pre-version") +} + +func TestMigrationErrorWritesNoFiles(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we have a migrator that errors + c := ghConfig.ReadFromString(testFullConfig()) + migration := mockMigration(func(config *ghConfig.Config) error { + return errors.New("failed to migrate in test") + }) + + // When we run the migration + conf := cfg{c} + err := conf.Migrate(migration) + + // Then the error is wrapped and bubbled + require.EqualError(t, err, "failed to migrate config: failed to migrate in test") + + // And no files are written to disk + files, err := os.ReadDir(tempDir) + require.NoError(t, err) + require.Len(t, files, 0) +} + +func TestMigrationWriteErrors(t *testing.T) { + tests := []struct { + name string + unwriteableFile string + wantErrContains string + }{ + { + name: "failure to write hosts", + unwriteableFile: "hosts.yml", + wantErrContains: "failed to write config after migration", + }, + { + name: "failure to write config", + unwriteableFile: "config.yml", + wantErrContains: "failed to write config after migration", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we error when writing the files (because we chmod the files as trickery) + makeFileUnwriteable(t, filepath.Join(tempDir, tt.unwriteableFile)) + + c := ghConfig.ReadFromString(testFullConfig()) + topLevelKey := []string{"toplevelkey"} + hostsKey := []string{hostsKey, "newhost"} + + migration := mockMigration(func(someCfg *ghConfig.Config) error { + someCfg.Set(topLevelKey, "toplevelvalue") + someCfg.Set(hostsKey, "newhostvalue") + return nil + }) + + // When we run the migration + conf := cfg{c} + err := conf.Migrate(migration) + + // Then the error is wrapped and bubbled + require.ErrorContains(t, err, tt.wantErrContains) + }) + } +} + +func makeFileUnwriteable(t *testing.T, file string) { + t.Helper() + + f, err := os.Create(file) + require.NoError(t, err) + f.Close() + + require.NoError(t, os.Chmod(file, 0000)) +} + +func mockMigration(doFunc func(config *ghConfig.Config) error) *MigrationMock { + return &MigrationMock{ + DoFunc: doFunc, + PreVersionFunc: func() string { + return "" + }, + PostVersionFunc: func() string { + return "not-expected" + }, + } + +} + +func testFullConfig() string { + var data = ` +git_protocol: ssh +editor: +prompt: enabled +pager: less +hosts: + github.com: + user: user1 + oauth_token: xxxxxxxxxxxxxxxxxxxx + git_protocol: ssh + enterprise.com: + user: user2 + oauth_token: yyyyyyyyyyyyyyyyyyyy + git_protocol: https +` + return data +} diff --git a/internal/config/migration/multi_account.go b/internal/config/migration/multi_account.go new file mode 100644 index 000000000..d4e1e7a67 --- /dev/null +++ b/internal/config/migration/multi_account.go @@ -0,0 +1,223 @@ +package migration + +import ( + "errors" + "fmt" + "net/http" + + "github.com/cli/cli/v2/internal/keyring" + ghAPI "github.com/cli/go-gh/v2/pkg/api" + "github.com/cli/go-gh/v2/pkg/config" +) + +var noTokenError = errors.New("no token found") + +type CowardlyRefusalError struct { + err error +} + +func (e CowardlyRefusalError) Error() string { + // Consider whether we should add a call to action here like "open an issue with the contents of your redacted hosts.yml" + return fmt.Sprintf("cowardly refusing to continue with multi account migration: %s", e.err.Error()) +} + +var hostsKey = []string{"hosts"} + +type tokenSource struct { + token string + inKeyring bool +} + +// This migration exists to take a hosts section of the following structure: +// +// github.com: +// user: williammartin +// git_protocol: https +// editor: vim +// github.localhost: +// user: monalisa +// git_protocol: https +// oauth_token: xyz +// +// We want this to migrate to something like: +// +// github.com: +// user: williammartin +// git_protocol: https +// editor: vim +// users: +// williammartin: +// +// github.localhost: +// user: monalisa +// git_protocol: https +// oauth_token: xyz +// users: +// monalisa: +// oauth_token: xyz +// +// For each hosts, we will create a new key `users` with the value of the host level +// `user` key as a list entry. If there is a host level `oauth_token` we will +// put that under the new user entry, otherwise there will be no value for the +// new user key. No other host level configuration will be copied to the new user. +// +// The reason for this is that we can then add new users under a host. +// Note that we are only copying the config under a new users key, and +// under a specific user. The original config is left alone. This is to +// allow forward compatability for older versions of gh and also to avoid +// breaking existing users of go-gh which looks at a specific location +// in the config for oauth tokens that are stored insecurely. + +type MultiAccount struct { + // Allow injecting a transport layer in tests. + Transport http.RoundTripper +} + +func (m MultiAccount) PreVersion() string { + // It is expected that there is no version key since this migration + // introduces it. + return "" +} + +func (m MultiAccount) PostVersion() string { + return "1" +} + +func (m MultiAccount) Do(c *config.Config) error { + hostnames, err := c.Keys(hostsKey) + // [github.com, github.localhost] + // We wouldn't expect to have a hosts key when this is the first time anyone + // is logging in with the CLI. + var keyNotFoundError *config.KeyNotFoundError + if errors.As(err, &keyNotFoundError) { + return nil + } + if err != nil { + return CowardlyRefusalError{errors.New("couldn't get hosts configuration")} + } + + // If there are no hosts then it doesn't matter whether we migrate or not, + // so lets avoid any confusion and say there's no migration required. + if len(hostnames) == 0 { + return nil + } + + // Otherwise let's get to the business of migrating! + for _, hostname := range hostnames { + tokenSource, err := getToken(c, hostname) + // If no token existed for this host we'll remove the entry from the hosts file + // by deleting it and moving on to the next one. + if errors.Is(err, noTokenError) { + // The only error that can be returned here is the key not existing, which + // we know can't be true. + _ = c.Remove(append(hostsKey, hostname)) + continue + } + // For any other error we'll error out + if err != nil { + return CowardlyRefusalError{fmt.Errorf("couldn't find oauth token for %q: %w", hostname, err)} + } + + username, err := getUsername(c, hostname, tokenSource.token, m.Transport) + if err != nil { + return CowardlyRefusalError{fmt.Errorf("couldn't get user name for %q: %w", hostname, err)} + } + + if err := migrateConfig(c, hostname, username); err != nil { + return CowardlyRefusalError{fmt.Errorf("couldn't migrate config for %q: %w", hostname, err)} + } + + if err := migrateToken(hostname, username, tokenSource); err != nil { + return CowardlyRefusalError{fmt.Errorf("couldn't migrate oauth token for %q: %w", hostname, err)} + } + } + + return nil +} + +func getToken(c *config.Config, hostname string) (tokenSource, error) { + if token, _ := c.Get(append(hostsKey, hostname, "oauth_token")); token != "" { + return tokenSource{token: token, inKeyring: false}, nil + } + token, err := keyring.Get(keyringServiceName(hostname), "") + + // If we have an error and it's not relating to there being no token + // then we'll return the error cause that's really unexpected. + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + return tokenSource{}, err + } + + // Otherwise we'll return a sentinel error + if err != nil || token == "" { + return tokenSource{}, noTokenError + } + + return tokenSource{ + token: token, + inKeyring: true, + }, nil +} + +func getUsername(c *config.Config, hostname, token string, transport http.RoundTripper) (string, error) { + username, _ := c.Get(append(hostsKey, hostname, "user")) + if username != "" && username != "x-access-token" { + return username, nil + } + opts := ghAPI.ClientOptions{ + Host: hostname, + AuthToken: token, + Transport: transport, + } + client, err := ghAPI.NewGraphQLClient(opts) + if err != nil { + return "", err + } + var query struct { + Viewer struct { + Login string + } + } + err = client.Query("CurrentUser", &query, nil) + if err != nil { + return "", err + } + return query.Viewer.Login, nil +} + +func migrateToken(hostname, username string, tokenSource tokenSource) error { + // If token is not currently stored in the keyring do not migrate it, + // as it is being stored in the config and is being handled when migrating the config. + if !tokenSource.inKeyring { + return nil + } + return keyring.Set(keyringServiceName(hostname), username, tokenSource.token) +} + +func migrateConfig(c *config.Config, hostname, username string) error { + // Set the user key incase it was previously an anonymous user. + c.Set(append(hostsKey, hostname, "user"), username) + // Create the username key with an empty value so it will be + // written even if there are no keys set under it. + c.Set(append(hostsKey, hostname, "users", username), "") + + insecureToken, err := c.Get(append(hostsKey, hostname, "oauth_token")) + var keyNotFoundError *config.KeyNotFoundError + // If there is no token then we're done here + if errors.As(err, &keyNotFoundError) { + return nil + } + + // If there's another error (current implementation doesn't have any other error but we'll be defensive) + // then bubble something up. + if err != nil { + return fmt.Errorf("couldn't get oauth token for %s: %s", hostname, err) + } + + // Otherwise we'll set the token under the new key + c.Set(append(hostsKey, hostname, "users", username, "oauth_token"), insecureToken) + return nil +} + +func keyringServiceName(hostname string) string { + return "gh:" + hostname +} diff --git a/internal/config/migration/multi_account_test.go b/internal/config/migration/multi_account_test.go new file mode 100644 index 000000000..8feb853bd --- /dev/null +++ b/internal/config/migration/multi_account_test.go @@ -0,0 +1,249 @@ +package migration_test + +import ( + "errors" + "fmt" + "testing" + + "github.com/cli/cli/v2/internal/config/migration" + "github.com/cli/cli/v2/internal/keyring" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/go-gh/v2/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestMigration(t *testing.T) { + cfg := config.ReadFromString(` +hosts: + github.com: + user: user1 + oauth_token: xxxxxxxxxxxxxxxxxxxx + git_protocol: ssh + enterprise.com: + user: user2 + oauth_token: yyyyyyyyyyyyyyyyyyyy + git_protocol: https +`) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) + + // First we'll check that the oauth tokens have been moved to their new locations + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "user1", "oauth_token"}, "xxxxxxxxxxxxxxxxxxxx") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "users", "user2", "oauth_token"}, "yyyyyyyyyyyyyyyyyyyy") + + // Then we'll check that the old data has been left alone + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "user1") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "oauth_token"}, "xxxxxxxxxxxxxxxxxxxx") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "git_protocol"}, "ssh") + + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "user"}, "user2") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "oauth_token"}, "yyyyyyyyyyyyyyyyyyyy") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "git_protocol"}, "https") +} + +func TestMigrationSecureStorage(t *testing.T) { + cfg := config.ReadFromString(` +hosts: + github.com: + user: userOne + git_protocol: ssh + enterprise.com: + user: userTwo + git_protocol: https +`) + + userOneToken := "userOne-token" + userTwoToken := "userTwo-token" + + keyring.MockInit() + require.NoError(t, keyring.Set("gh:github.com", "", userOneToken)) + require.NoError(t, keyring.Set("gh:enterprise.com", "", userTwoToken)) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) + + // Verify token gets stored with host and username + gotUserOneToken, err := keyring.Get("gh:github.com", "userOne") + require.NoError(t, err) + require.Equal(t, userOneToken, gotUserOneToken) + + // Verify token still exists with only host + gotUserOneToken, err = keyring.Get("gh:github.com", "") + require.NoError(t, err) + require.Equal(t, userOneToken, gotUserOneToken) + + // Verify token gets stored with host and username + gotUserTwoToken, err := keyring.Get("gh:enterprise.com", "userTwo") + require.NoError(t, err) + require.Equal(t, userTwoToken, gotUserTwoToken) + + // Verify token still exists with only host + gotUserTwoToken, err = keyring.Get("gh:enterprise.com", "") + require.NoError(t, err) + require.Equal(t, userTwoToken, gotUserTwoToken) + + // First we'll check that the users have been created with no config underneath them + requireKeyExists(t, cfg, []string{"hosts", "github.com", "users", "userOne"}) + requireKeyExists(t, cfg, []string{"hosts", "enterprise.com", "users", "userTwo"}) + + // Then we'll check that the old data has been left alone + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "userOne") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "git_protocol"}, "ssh") + + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "user"}, "userTwo") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "git_protocol"}, "https") +} + +func TestPreVersionIsEmptyString(t *testing.T) { + var m migration.MultiAccount + require.Equal(t, "", m.PreVersion()) +} + +func TestPostVersion(t *testing.T) { + var m migration.MultiAccount + require.Equal(t, "1", m.PostVersion()) +} + +func TestMigrationReturnsSuccessfullyWhenNoHostsEntry(t *testing.T) { + cfg := config.ReadFromString(``) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) +} + +func TestMigrationReturnsSuccessfullyWhenEmptyHosts(t *testing.T) { + cfg := config.ReadFromString(` +hosts: +`) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) +} + +func TestMigrationReturnsSuccessfullyWhenAnonymousUserExists(t *testing.T) { + // Simulates config that gets generated when a user logs + // in with a token and git protocol is not specified and + // secure storage is used. + token := "test-token" + keyring.MockInit() + require.NoError(t, keyring.Set("gh:github.com", "", token)) + + cfg := config.ReadFromString(` +hosts: + github.com: + user: x-access-token +`) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query CurrentUser\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`), + ) + + m := migration.MultiAccount{Transport: reg} + require.NoError(t, m.Do(cfg)) + + require.Equal(t, fmt.Sprintf("token %s", token), reg.Requests[0].Header.Get("Authorization")) + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "monalisa") + // monalisa key gets created with no value + users, err := cfg.Keys([]string{"hosts", "github.com", "users"}) + require.NoError(t, err) + require.Equal(t, []string{"monalisa"}, users) + + // Verify token gets stored with host and username + gotToken, err := keyring.Get("gh:github.com", "monalisa") + require.NoError(t, err) + require.Equal(t, token, gotToken) + + // Verify token still exists with only host + gotToken, err = keyring.Get("gh:github.com", "") + require.NoError(t, err) + require.Equal(t, token, gotToken) +} + +func TestMigrationReturnsSuccessfullyWhenAnonymousUserExistsAndInsecureStorage(t *testing.T) { + // Simulates config that gets generated when a user logs + // in with a token and git protocol is specified and + // secure storage is not used. + cfg := config.ReadFromString(` +hosts: + github.com: + user: x-access-token + oauth_token: test-token + git_protocol: ssh +`) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query CurrentUser\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`), + ) + + m := migration.MultiAccount{Transport: reg} + require.NoError(t, m.Do(cfg)) + + require.Equal(t, "token test-token", reg.Requests[0].Header.Get("Authorization")) + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "monalisa") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "monalisa", "oauth_token"}, "test-token") +} + +func TestMigrationRemovesHostsWithInvalidTokens(t *testing.T) { + // Simulates config when user is logged in securely + // but no token entry is in the keyring. + keyring.MockInit() + cfg := config.ReadFromString(` +hosts: + github.com: + user: user1 + git_protocol: ssh +`) + + m := migration.MultiAccount{} + require.NoError(t, m.Do(cfg)) + + requireNoKey(t, cfg, []string{"hosts", "github.com"}) +} + +func TestMigrationErrorsWhenUnableToGetExpectedSecureToken(t *testing.T) { + // Simulates config when user is logged in securely + // but no token entry is in the keyring. + keyring.MockInitWithError(errors.New("keyring test error")) + cfg := config.ReadFromString(` +hosts: + github.com: + user: user1 + git_protocol: ssh +`) + + m := migration.MultiAccount{} + err := m.Do(cfg) + + require.ErrorContains(t, err, `couldn't find oauth token for "github.com": keyring test error`) +} + +func requireKeyExists(t *testing.T, cfg *config.Config, keys []string) { + t.Helper() + + _, err := cfg.Get(keys) + require.NoError(t, err) +} + +func requireKeyWithValue(t *testing.T, cfg *config.Config, keys []string, value string) { + t.Helper() + + actual, err := cfg.Get(keys) + require.NoError(t, err) + + require.Equal(t, value, actual) +} + +func requireNoKey(t *testing.T, cfg *config.Config, keys []string) { + t.Helper() + + _, err := cfg.Get(keys) + var keyNotFoundError *config.KeyNotFoundError + require.ErrorAs(t, err, &keyNotFoundError) +} diff --git a/internal/config/migration_mock.go b/internal/config/migration_mock.go new file mode 100644 index 000000000..bf8133fb4 --- /dev/null +++ b/internal/config/migration_mock.go @@ -0,0 +1,149 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package config + +import ( + ghConfig "github.com/cli/go-gh/v2/pkg/config" + "sync" +) + +// Ensure, that MigrationMock does implement Migration. +// If this is not the case, regenerate this file with moq. +var _ Migration = &MigrationMock{} + +// MigrationMock is a mock implementation of Migration. +// +// func TestSomethingThatUsesMigration(t *testing.T) { +// +// // make and configure a mocked Migration +// mockedMigration := &MigrationMock{ +// DoFunc: func(config *ghConfig.Config) error { +// panic("mock out the Do method") +// }, +// PostVersionFunc: func() string { +// panic("mock out the PostVersion method") +// }, +// PreVersionFunc: func() string { +// panic("mock out the PreVersion method") +// }, +// } +// +// // use mockedMigration in code that requires Migration +// // and then make assertions. +// +// } +type MigrationMock struct { + // DoFunc mocks the Do method. + DoFunc func(config *ghConfig.Config) error + + // PostVersionFunc mocks the PostVersion method. + PostVersionFunc func() string + + // PreVersionFunc mocks the PreVersion method. + PreVersionFunc func() string + + // calls tracks calls to the methods. + calls struct { + // Do holds details about calls to the Do method. + Do []struct { + // Config is the config argument value. + Config *ghConfig.Config + } + // PostVersion holds details about calls to the PostVersion method. + PostVersion []struct { + } + // PreVersion holds details about calls to the PreVersion method. + PreVersion []struct { + } + } + lockDo sync.RWMutex + lockPostVersion sync.RWMutex + lockPreVersion sync.RWMutex +} + +// Do calls DoFunc. +func (mock *MigrationMock) Do(config *ghConfig.Config) error { + if mock.DoFunc == nil { + panic("MigrationMock.DoFunc: method is nil but Migration.Do was just called") + } + callInfo := struct { + Config *ghConfig.Config + }{ + Config: config, + } + mock.lockDo.Lock() + mock.calls.Do = append(mock.calls.Do, callInfo) + mock.lockDo.Unlock() + return mock.DoFunc(config) +} + +// DoCalls gets all the calls that were made to Do. +// Check the length with: +// +// len(mockedMigration.DoCalls()) +func (mock *MigrationMock) DoCalls() []struct { + Config *ghConfig.Config +} { + var calls []struct { + Config *ghConfig.Config + } + mock.lockDo.RLock() + calls = mock.calls.Do + mock.lockDo.RUnlock() + return calls +} + +// PostVersion calls PostVersionFunc. +func (mock *MigrationMock) PostVersion() string { + if mock.PostVersionFunc == nil { + panic("MigrationMock.PostVersionFunc: method is nil but Migration.PostVersion was just called") + } + callInfo := struct { + }{} + mock.lockPostVersion.Lock() + mock.calls.PostVersion = append(mock.calls.PostVersion, callInfo) + mock.lockPostVersion.Unlock() + return mock.PostVersionFunc() +} + +// PostVersionCalls gets all the calls that were made to PostVersion. +// Check the length with: +// +// len(mockedMigration.PostVersionCalls()) +func (mock *MigrationMock) PostVersionCalls() []struct { +} { + var calls []struct { + } + mock.lockPostVersion.RLock() + calls = mock.calls.PostVersion + mock.lockPostVersion.RUnlock() + return calls +} + +// PreVersion calls PreVersionFunc. +func (mock *MigrationMock) PreVersion() string { + if mock.PreVersionFunc == nil { + panic("MigrationMock.PreVersionFunc: method is nil but Migration.PreVersion was just called") + } + callInfo := struct { + }{} + mock.lockPreVersion.Lock() + mock.calls.PreVersion = append(mock.calls.PreVersion, callInfo) + mock.lockPreVersion.Unlock() + return mock.PreVersionFunc() +} + +// PreVersionCalls gets all the calls that were made to PreVersion. +// Check the length with: +// +// len(mockedMigration.PreVersionCalls()) +func (mock *MigrationMock) PreVersionCalls() []struct { +} { + var calls []struct { + } + mock.lockPreVersion.RLock() + calls = mock.calls.PreVersion + mock.lockPreVersion.RUnlock() + return calls +} diff --git a/internal/config/stub.go b/internal/config/stub.go index 143c9b870..98c1ceaba 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/cli/cli/v2/internal/keyring" ghConfig "github.com/cli/go-gh/v2/pkg/config" ) @@ -26,6 +27,9 @@ func NewFromString(cfgStr string) *ConfigMock { mock.WriteFunc = func() error { return cfg.Write() } + mock.MigrateFunc = func(m Migration) error { + return cfg.Migrate(m) + } mock.AliasesFunc = func() *AliasConfig { return &AliasConfig{cfg: c} } @@ -69,9 +73,44 @@ func NewFromString(cfgStr string) *ConfigMock { val, _ := cfg.GetOrDefault(hostname, promptKey) return val } + mock.VersionFunc = func() string { + val, _ := cfg.GetOrDefault("", versionKey) + return val + } return mock } +// NewIsolatedTestConfig sets up a Mock keyring, creates a blank config +// overwrites the ghConfig.Read function that returns a singleton config +// in the real implementation, sets the GH_CONFIG_DIR env var so that +// any call to Write goes to a different location on disk, and then returns +// the blank config and a function that reads any data written to disk. +func NewIsolatedTestConfig(t *testing.T) (Config, func(io.Writer, io.Writer)) { + keyring.MockInit() + + c := ghConfig.ReadFromString("") + cfg := cfg{c} + + // The real implementation of config.Read uses a sync.Once + // to read config files and initialise package level variables + // that are used from then on. + // + // This means that tests can't be isolated from each other, so + // we swap out the function here to return a new config each time. + ghConfig.Read = func(_ *ghConfig.Config) (*ghConfig.Config, error) { + return c, nil + } + + // The config.Write method isn't defined in the same way as Read to allow + // the function to be swapped out and it does try to write to disk. + // + // We should consider whether it makes sense to change that but in the meantime + // we can use GH_CONFIG_DIR env var to ensure the tests remain isolated. + readConfigs := StubWriteConfig(t) + + return &cfg, readConfigs +} + // StubWriteConfig stubs out the filesystem where config file are written. // It then returns a function that will read in the config files into io.Writers. // It automatically cleans up environment variables and written files. diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index afb5025e6..f873c6436 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -2,11 +2,14 @@ package keyring import ( + "errors" "time" "github.com/zalando/go-keyring" ) +var ErrNotFound = errors.New("secret not found in keyring") + type TimeoutError struct { message string } @@ -46,6 +49,9 @@ func Get(service, user string) (string, error) { }() select { case res := <-ch: + if errors.Is(res.err, keyring.ErrNotFound) { + return "", ErrNotFound + } return res.val, res.err case <-time.After(3 * time.Second): return "", &TimeoutError{"timeout while trying to get secret from keyring"} @@ -66,3 +72,11 @@ func Delete(service, user string) error { return &TimeoutError{"timeout while trying to delete secret from keyring"} } } + +func MockInit() { + keyring.MockInit() +} + +func MockInitWithError(err error) { + keyring.MockInitWithError(err) +} diff --git a/internal/prompter/test.go b/internal/prompter/test.go index af55c084d..04375ce76 100644 --- a/internal/prompter/test.go +++ b/internal/prompter/test.go @@ -138,13 +138,13 @@ func IndexFor(options []string, answer string) (int, error) { return ix, nil } } - return -1, NoSuchAnswerErr(answer) + return -1, NoSuchAnswerErr(answer, options) } func NoSuchPromptErr(prompt string) error { return fmt.Errorf("no such prompt '%s'", prompt) } -func NoSuchAnswerErr(answer string) error { - return fmt.Errorf("no such answer '%s'", answer) +func NoSuchAnswerErr(answer string, options []string) error { + return fmt.Errorf("no such answer '%s' in [%s]", answer, strings.Join(options, ", ")) } diff --git a/pkg/cmd/auth/auth.go b/pkg/cmd/auth/auth.go index c003049eb..70e01653f 100644 --- a/pkg/cmd/auth/auth.go +++ b/pkg/cmd/auth/auth.go @@ -7,6 +7,7 @@ import ( authRefreshCmd "github.com/cli/cli/v2/pkg/cmd/auth/refresh" authSetupGitCmd "github.com/cli/cli/v2/pkg/cmd/auth/setupgit" authStatusCmd "github.com/cli/cli/v2/pkg/cmd/auth/status" + authSwitchCmd "github.com/cli/cli/v2/pkg/cmd/auth/switch" authTokenCmd "github.com/cli/cli/v2/pkg/cmd/auth/token" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" @@ -28,6 +29,7 @@ func NewCmdAuth(f *cmdutil.Factory) *cobra.Command { cmd.AddCommand(gitCredentialCmd.NewCmdCredential(f, nil)) cmd.AddCommand(authSetupGitCmd.NewCmdSetupGit(f, nil)) cmd.AddCommand(authTokenCmd.NewCmdToken(f, nil)) + cmd.AddCommand(authSwitchCmd.NewCmdSwitch(f, nil)) return cmd } diff --git a/pkg/cmd/auth/gitcredential/helper.go b/pkg/cmd/auth/gitcredential/helper.go index fcceaf186..9b64f74b9 100644 --- a/pkg/cmd/auth/gitcredential/helper.go +++ b/pkg/cmd/auth/gitcredential/helper.go @@ -14,8 +14,8 @@ import ( const tokenUser = "x-access-token" type config interface { - Token(string) (string, string) - User(string) (string, error) + ActiveToken(string) (string, string) + ActiveUser(string) (string, error) } type CredentialOptions struct { @@ -112,16 +112,16 @@ func helperRun(opts *CredentialOptions) error { lookupHost := wants["host"] var gotUser string - gotToken, source := cfg.Token(lookupHost) + gotToken, source := cfg.ActiveToken(lookupHost) if gotToken == "" && strings.HasPrefix(lookupHost, "gist.") { lookupHost = strings.TrimPrefix(lookupHost, "gist.") - gotToken, source = cfg.Token(lookupHost) + gotToken, source = cfg.ActiveToken(lookupHost) } if strings.HasSuffix(source, "_TOKEN") { gotUser = tokenUser } else { - gotUser, _ = cfg.User(lookupHost) + gotUser, _ = cfg.ActiveUser(lookupHost) if gotUser == "" { gotUser = tokenUser } diff --git a/pkg/cmd/auth/gitcredential/helper_test.go b/pkg/cmd/auth/gitcredential/helper_test.go index f66df1d16..a3c6e2056 100644 --- a/pkg/cmd/auth/gitcredential/helper_test.go +++ b/pkg/cmd/auth/gitcredential/helper_test.go @@ -10,11 +10,11 @@ import ( type tinyConfig map[string]string -func (c tinyConfig) Token(host string) (string, string) { +func (c tinyConfig) ActiveToken(host string) (string, string) { return c[fmt.Sprintf("%s:%s", host, "oauth_token")], c["_source"] } -func (c tinyConfig) User(host string) (string, error) { +func (c tinyConfig) ActiveUser(host string) (string, error) { return c[fmt.Sprintf("%s:%s", host, "user")], nil } diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index ae793ae85..34a345632 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -53,14 +53,15 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm cmd := &cobra.Command{ Use: "login", Args: cobra.ExactArgs(0), - Short: "Authenticate with a GitHub host", + Short: "Log in to a GitHub account", Long: heredoc.Docf(` Authenticate with a GitHub host. The default authentication mode is a web-based browser flow. After completion, an authentication token will be stored securely in the system credential store. - If a credential store is not found or there is an issue using it gh will fallback to writing the token to a plain text file. - See %[1]sgh auth status%[1]s for its stored location. + If a credential store is not found or there is an issue using it gh will fallback + to writing the token to a plain text file. See %[1]sgh auth status%[1]s for its + stored location. Alternatively, use %[1]s--with-token%[1]s to pass in a token on standard input. The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. @@ -70,15 +71,19 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm %[1]sgh help environment%[1]s for more info. To use gh in GitHub Actions, add %[1]sGH_TOKEN: ${{ github.token }}%[1]s to %[1]senv%[1]s. + + The git protocol to use for git operations on this host can be set with %[1]s--git-protocol%[1]s, + or during the interactive prompting. Although login is for a single account on a host, setting + the git protocol will take effect for all users on the host. `, "`"), Example: heredoc.Doc(` - # start interactive setup + # Start interactive setup $ gh auth login - # authenticate against github.com by reading the token from a file + # Authenticate against github.com by reading the token from a file $ gh auth login --with-token < mytoken.txt - # authenticate with a specific GitHub instance + # Authenticate with specific host $ gh auth login --hostname enterprise.internal `), RunE: func(cmd *cobra.Command, args []string) error { @@ -125,7 +130,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes to request") cmd.Flags().BoolVar(&tokenStdin, "with-token", false, "Read token from standard input") cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open a browser to authenticate") - cmdutil.StringEnumFlag(cmd, &opts.GitProtocol, "git-protocol", "p", "", []string{"ssh", "https"}, "The protocol to use for git operations") + cmdutil.StringEnumFlag(cmd, &opts.GitProtocol, "git-protocol", "p", "", []string{"ssh", "https"}, "The protocol to use for git operations on this host") // secure storage became the default on 2023/4/04; this flag is left as a no-op for backwards compatibility var secureStorage bool @@ -173,22 +178,14 @@ func loginRun(opts *LoginOptions) error { if err := shared.HasMinimumScopes(httpClient, hostname, opts.Token); err != nil { return fmt.Errorf("error validating token: %w", err) } - // Adding a user key ensures that a nonempty host section gets written to the config file. - _, loginErr := authCfg.Login(hostname, "x-access-token", opts.Token, opts.GitProtocol, !opts.InsecureStorage) - return loginErr - } - - existingToken, _ := authCfg.Token(hostname) - if existingToken != "" && opts.Interactive { - if err := shared.HasMinimumScopes(httpClient, hostname, existingToken); err == nil { - keepGoing, err := opts.Prompter.Confirm(fmt.Sprintf("You're already logged into %s. Do you want to re-authenticate?", hostname), false) - if err != nil { - return err - } - if !keepGoing { - return nil - } + username, err := shared.GetCurrentLogin(httpClient, hostname, opts.Token) + if err != nil { + return fmt.Errorf("error retrieving current user: %w", err) } + + // Adding a user key ensures that a nonempty host section gets written to the config file. + _, loginErr := authCfg.Login(hostname, username, opts.Token, opts.GitProtocol, !opts.InsecureStorage) + return loginErr } return shared.Login(&shared.LoginOptions{ diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index 72d796a39..60c6f6f24 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -17,7 +17,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" - "github.com/zalando/go-keyring" + "github.com/stretchr/testify/require" ) func stubHomeDir(t *testing.T, dir string) { @@ -258,8 +258,9 @@ func Test_loginRun_nontty(t *testing.T) { tests := []struct { name string opts *LoginOptions + env map[string]string httpStubs func(*httpmock.Registry) - cfgStubs func(*config.ConfigMock) + cfgStubs func(*testing.T, config.Config) wantHosts string wantErr string wantStderr string @@ -274,8 +275,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc123\n user: x-access-token\n", + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc123\n oauth_token: abc123\n user: monalisa\n", }, { name: "insecure with token and https git-protocol", @@ -287,8 +291,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc123\n user: x-access-token\n git_protocol: https\n", + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc123\n git_protocol: https\n oauth_token: abc123\n user: monalisa\n", }, { name: "with token and non-default host", @@ -299,8 +306,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "albert.wesker:\n oauth_token: abc123\n user: x-access-token\n", + wantHosts: "albert.wesker:\n users:\n monalisa:\n oauth_token: abc123\n oauth_token: abc123\n user: monalisa\n", }, { name: "missing repo scope", @@ -333,8 +343,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,admin:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc456\n user: x-access-token\n", + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc456\n oauth_token: abc456\n user: monalisa\n", }, { name: "github.com token from environment", @@ -342,18 +355,12 @@ func Test_loginRun_nontty(t *testing.T) { Hostname: "github.com", Token: "abc456", }, - cfgStubs: func(c *config.ConfigMock) { - authCfg := c.Authentication() - authCfg.SetToken("value_from_env", "GH_TOKEN") - c.AuthenticationFunc = func() *config.AuthConfig { - return authCfg - } - }, + env: map[string]string{"GH_TOKEN": "value_from_env"}, wantErr: "SilentError", wantStderr: heredoc.Doc(` - The value of the GH_TOKEN environment variable is being used for authentication. - To have GitHub CLI store credentials instead, first clear the value from the environment. - `), + The value of the GH_TOKEN environment variable is being used for authentication. + To have GitHub CLI store credentials instead, first clear the value from the environment. + `), }, { name: "GHE token from environment", @@ -361,18 +368,12 @@ func Test_loginRun_nontty(t *testing.T) { Hostname: "ghe.io", Token: "abc456", }, - cfgStubs: func(c *config.ConfigMock) { - authCfg := c.Authentication() - authCfg.SetToken("value_from_env", "GH_ENTERPRISE_TOKEN") - c.AuthenticationFunc = func() *config.AuthConfig { - return authCfg - } - }, + env: map[string]string{"GH_ENTERPRISE_TOKEN": "value_from_env"}, wantErr: "SilentError", wantStderr: heredoc.Doc(` - The value of the GH_ENTERPRISE_TOKEN environment variable is being used for authentication. - To have GitHub CLI store credentials instead, first clear the value from the environment. - `), + The value of the GH_ENTERPRISE_TOKEN environment variable is being used for authentication. + To have GitHub CLI store credentials instead, first clear the value from the environment. + `), }, { name: "with token and secure storage", @@ -382,10 +383,40 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n user: x-access-token\n", + wantHosts: "github.com:\n users:\n monalisa:\n user: monalisa\n", wantSecureToken: "abc123", }, + { + name: "given we are already logged in, and log in as a new user, it is added to the config", + opts: &LoginOptions{ + Hostname: "github.com", + Token: "newUserToken", + }, + cfgStubs: func(t *testing.T, c config.Config) { + _, err := c.Authentication().Login("github.com", "monalisa", "abc123", "https", false) + require.NoError(t, err) + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"newUser"}}}`)) + }, + wantHosts: heredoc.Doc(` + github.com: + users: + monalisa: + oauth_token: abc123 + newUser: + git_protocol: https + user: newUser + `), + wantSecureToken: "newUserToken", + }, } for _, tt := range tests { @@ -395,11 +426,9 @@ func Test_loginRun_nontty(t *testing.T) { ios.SetStdoutTTY(false) tt.opts.IO = ios - keyring.MockInit() - readConfigs := config.StubWriteConfig(t) - cfg := config.NewBlankConfig() + cfg, readConfigs := config.NewIsolatedTestConfig(t) if tt.cfgStubs != nil { - tt.cfgStubs(cfg) + tt.cfgStubs(t, cfg) } tt.opts.Config = func() (config.Config, error) { return cfg, nil @@ -414,6 +443,10 @@ func Test_loginRun_nontty(t *testing.T) { tt.httpStubs(reg) } + for k, v := range tt.env { + t.Setenv(k, v) + } + _, restoreRun := run.Stub() defer restoreRun(t) @@ -446,37 +479,11 @@ func Test_loginRun_Survey(t *testing.T) { httpStubs func(*httpmock.Registry) prompterStubs func(*prompter.PrompterMock) runStubs func(*run.CommandStubber) - cfgStubs func(*config.ConfigMock) + cfgStubs func(*testing.T, config.Config) wantHosts string wantErrOut *regexp.Regexp wantSecureToken string }{ - { - name: "already authenticated", - opts: &LoginOptions{ - Interactive: true, - }, - cfgStubs: func(c *config.ConfigMock) { - authCfg := c.Authentication() - authCfg.SetToken("ghi789", "oauth_token") - c.AuthenticationFunc = func() *config.AuthConfig { - return authCfg - } - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) - }, - prompterStubs: func(pm *prompter.PrompterMock) { - pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { - if prompt == "What account do you want to log into?" { - return prompter.IndexFor(opts, "GitHub.com") - } - return -1, prompter.NoSuchPromptErr(prompt) - } - }, - wantHosts: "", - wantErrOut: nil, - }, { name: "hostname set", opts: &LoginOptions{ @@ -485,15 +492,18 @@ func Test_loginRun_Survey(t *testing.T) { InsecureStorage: true, }, wantHosts: heredoc.Doc(` - rebecca.chambers: - oauth_token: def456 - user: jillv - git_protocol: https - `), + rebecca.chambers: + users: + jillv: + oauth_token: def456 + git_protocol: https + oauth_token: def456 + user: jillv + `), prompterStubs: func(pm *prompter.PrompterMock) { pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { switch prompt { - case "What is your preferred protocol for Git operations?": + case "What is your preferred protocol for Git operations on this host?": return prompter.IndexFor(opts, "HTTPS") case "How would you like to authenticate GitHub CLI?": return prompter.IndexFor(opts, "Paste an authentication token") @@ -516,11 +526,14 @@ func Test_loginRun_Survey(t *testing.T) { { name: "choose enterprise", wantHosts: heredoc.Doc(` - brad.vickers: - oauth_token: def456 - user: jillv - git_protocol: https - `), + brad.vickers: + users: + jillv: + oauth_token: def456 + git_protocol: https + oauth_token: def456 + user: jillv + `), opts: &LoginOptions{ Interactive: true, InsecureStorage: true, @@ -530,7 +543,7 @@ func Test_loginRun_Survey(t *testing.T) { switch prompt { case "What account do you want to log into?": return prompter.IndexFor(opts, "GitHub Enterprise Server") - case "What is your preferred protocol for Git operations?": + case "What is your preferred protocol for Git operations on this host?": return prompter.IndexFor(opts, "HTTPS") case "How would you like to authenticate GitHub CLI?": return prompter.IndexFor(opts, "Paste an authentication token") @@ -556,11 +569,14 @@ func Test_loginRun_Survey(t *testing.T) { { name: "choose github.com", wantHosts: heredoc.Doc(` - github.com: - oauth_token: def456 - user: jillv - git_protocol: https - `), + github.com: + users: + jillv: + oauth_token: def456 + git_protocol: https + oauth_token: def456 + user: jillv + `), opts: &LoginOptions{ Interactive: true, InsecureStorage: true, @@ -570,7 +586,7 @@ func Test_loginRun_Survey(t *testing.T) { switch prompt { case "What account do you want to log into?": return prompter.IndexFor(opts, "GitHub.com") - case "What is your preferred protocol for Git operations?": + case "What is your preferred protocol for Git operations on this host?": return prompter.IndexFor(opts, "HTTPS") case "How would you like to authenticate GitHub CLI?": return prompter.IndexFor(opts, "Paste an authentication token") @@ -587,11 +603,14 @@ func Test_loginRun_Survey(t *testing.T) { { name: "sets git_protocol", wantHosts: heredoc.Doc(` - github.com: - oauth_token: def456 - user: jillv - git_protocol: ssh - `), + github.com: + users: + jillv: + oauth_token: def456 + git_protocol: ssh + oauth_token: def456 + user: jillv + `), opts: &LoginOptions{ Interactive: true, InsecureStorage: true, @@ -601,7 +620,7 @@ func Test_loginRun_Survey(t *testing.T) { switch prompt { case "What account do you want to log into?": return prompter.IndexFor(opts, "GitHub.com") - case "What is your preferred protocol for Git operations?": + case "What is your preferred protocol for Git operations on this host?": return prompter.IndexFor(opts, "SSH") case "How would you like to authenticate GitHub CLI?": return prompter.IndexFor(opts, "Paste an authentication token") @@ -620,7 +639,7 @@ func Test_loginRun_Survey(t *testing.T) { prompterStubs: func(pm *prompter.PrompterMock) { pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { switch prompt { - case "What is your preferred protocol for Git operations?": + case "What is your preferred protocol for Git operations on this host?": return prompter.IndexFor(opts, "HTTPS") case "How would you like to authenticate GitHub CLI?": return prompter.IndexFor(opts, "Paste an authentication token") @@ -633,13 +652,58 @@ func Test_loginRun_Survey(t *testing.T) { rs.Register(`git config credential\.helper`, 1, "") }, wantHosts: heredoc.Doc(` - github.com: - user: jillv - git_protocol: https - `), + github.com: + git_protocol: https + users: + jillv: + user: jillv + `), wantErrOut: regexp.MustCompile("Logged in as jillv"), wantSecureToken: "def456", }, + { + name: "given we log in as a user that is already in the config, we get an informational message", + opts: &LoginOptions{ + Hostname: "github.com", + Interactive: true, + InsecureStorage: true, + }, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { + switch prompt { + case "What is your preferred protocol for Git operations on this host?": + return prompter.IndexFor(opts, "HTTPS") + case "How would you like to authenticate GitHub CLI?": + return prompter.IndexFor(opts, "Paste an authentication token") + } + return -1, prompter.NoSuchPromptErr(prompt) + } + }, + cfgStubs: func(t *testing.T, c config.Config) { + _, err := c.Authentication().Login("github.com", "monalisa", "abc123", "https", false) + require.NoError(t, err) + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(`git config credential\.https:/`, 1, "") + rs.Register(`git config credential\.helper`, 1, "") + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) + }, + wantHosts: heredoc.Doc(` + github.com: + users: + monalisa: + oauth_token: def456 + git_protocol: https + user: monalisa + oauth_token: def456 + `), + wantErrOut: regexp.MustCompile(`! You were already logged in to this account`), + }, } for _, tt := range tests { @@ -655,12 +719,9 @@ func Test_loginRun_Survey(t *testing.T) { tt.opts.IO = ios - keyring.MockInit() - readConfigs := config.StubWriteConfig(t) - - cfg := config.NewBlankConfig() + cfg, readConfigs := config.NewIsolatedTestConfig(t) if tt.cfgStubs != nil { - tt.cfgStubs(cfg) + tt.cfgStubs(t, cfg) } tt.opts.Config = func() (config.Config, error) { return cfg, nil diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go index b2afc9a6c..9d7642ee9 100644 --- a/pkg/cmd/auth/logout/logout.go +++ b/pkg/cmd/auth/logout/logout.go @@ -1,11 +1,11 @@ package logout import ( + "errors" "fmt" - "net/http" + "slices" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/pkg/cmd/auth/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -14,41 +14,41 @@ import ( ) type LogoutOptions struct { - HttpClient func() (*http.Client, error) - IO *iostreams.IOStreams - Config func() (config.Config, error) - Prompter shared.Prompt - Hostname string + IO *iostreams.IOStreams + Config func() (config.Config, error) + Prompter shared.Prompt + Hostname string + Username string } func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Command { opts := &LogoutOptions{ - HttpClient: f.HttpClient, - IO: f.IOStreams, - Config: f.Config, - Prompter: f.Prompter, + IO: f.IOStreams, + Config: f.Config, + Prompter: f.Prompter, } cmd := &cobra.Command{ Use: "logout", Args: cobra.ExactArgs(0), - Short: "Log out of a GitHub host", - Long: heredoc.Docf(`Remove authentication for a GitHub host. + Short: "Log out of a GitHub account", + Long: heredoc.Doc(` + Remove authentication for a GitHub account. - This command removes the authentication configuration for a host either specified - interactively or via %[1]s--hostname%[1]s. - `, "`"), + This command removes the stored authentication configuration + for an account. The authentication configuration is only + removed locally. + + This command does not invalidate authentication tokens. + `), Example: heredoc.Doc(` + # Select what host and account to log out of via a prompt $ gh auth logout - # => select what host to log out of via a prompt - $ gh auth logout --hostname enterprise.internal - # => log out of specified host + # Log out of a specific host and specific account + $ gh auth logout --hostname enterprise.internal --user monalisa `), RunE: func(cmd *cobra.Command, args []string) error { - if opts.Hostname == "" && !opts.IO.CanPrompt() { - return cmdutil.FlagErrorf("--hostname required when not running interactively") - } if runF != nil { return runF(opts) } @@ -58,12 +58,14 @@ func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Co } cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to log out of") + cmd.Flags().StringVarP(&opts.Username, "user", "u", "", "The account to log out of") return cmd } func logoutRun(opts *LogoutOptions) error { hostname := opts.Hostname + username := opts.Username cfg, err := opts.Config() if err != nil { @@ -71,70 +73,88 @@ func logoutRun(opts *LogoutOptions) error { } authCfg := cfg.Authentication() - candidates := authCfg.Hosts() - if len(candidates) == 0 { + knownHosts := authCfg.Hosts() + if len(knownHosts) == 0 { return fmt.Errorf("not logged in to any hosts") } - if hostname == "" { - if len(candidates) == 1 { - hostname = candidates[0] - } else { - selected, err := opts.Prompter.Select( - "What account do you want to log out of?", "", candidates) - if err != nil { - return fmt.Errorf("could not prompt: %w", err) - } - hostname = candidates[selected] - } - } else { - var found bool - for _, c := range candidates { - if c == hostname { - found = true - break - } + if hostname != "" { + if !slices.Contains(knownHosts, hostname) { + return fmt.Errorf("not logged in to %s", hostname) } - if !found { - return fmt.Errorf("not logged into %s", hostname) + if username != "" { + knownUsers := cfg.Authentication().UsersForHost(hostname) + if !slices.Contains(knownUsers, username) { + return fmt.Errorf("not logged in to %s account %s", hostname, username) + } } } + type hostUser struct { + host string + user string + } + var candidates []hostUser + + for _, host := range knownHosts { + if hostname != "" && host != hostname { + continue + } + knownUsers := cfg.Authentication().UsersForHost(host) + for _, user := range knownUsers { + if username != "" && user != username { + continue + } + candidates = append(candidates, hostUser{host: host, user: user}) + } + } + + if len(candidates) == 0 { + return errors.New("no accounts matched that criteria") + } else if len(candidates) == 1 { + hostname = candidates[0].host + username = candidates[0].user + } else if !opts.IO.CanPrompt() { + return errors.New("unable to determine which account to log out of, please specify `--hostname` and `--user`") + } else { + prompts := make([]string, len(candidates)) + for i, c := range candidates { + prompts[i] = fmt.Sprintf("%s (%s)", c.user, c.host) + } + selected, err := opts.Prompter.Select( + "What account do you want to log out of?", "", prompts) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + hostname = candidates[selected].host + username = candidates[selected].user + } + if src, writeable := shared.AuthTokenWriteable(authCfg, hostname); !writeable { fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", src) fmt.Fprint(opts.IO.ErrOut, "To erase credentials stored in GitHub CLI, first clear the value from the environment.\n") return cmdutil.SilentError } - httpClient, err := opts.HttpClient() - if err != nil { + // We can ignore the error here because a host must always have an active user + preLogoutActiveUser, _ := authCfg.ActiveUser(hostname) + + if err := authCfg.Logout(hostname, username); err != nil { return err } - apiClient := api.NewClientFromHTTP(httpClient) - username, err := api.CurrentLoginName(apiClient, hostname) - if err != nil { - // suppressing; the user is trying to delete this token and it might be bad. - // we'll see if the username is in the config and fall back to that. - username, _ = authCfg.User(hostname) - } + postLogoutActiveUser, _ := authCfg.ActiveUser(hostname) + hasSwitchedToNewUser := preLogoutActiveUser != postLogoutActiveUser && + postLogoutActiveUser != "" - usernameStr := "" - if username != "" { - usernameStr = fmt.Sprintf(" account '%s'", username) - } + cs := opts.IO.ColorScheme() + fmt.Fprintf(opts.IO.ErrOut, "%s Logged out of %s account %s\n", + cs.SuccessIcon(), hostname, cs.Bold(username)) - if err := authCfg.Logout(hostname); err != nil { - return fmt.Errorf("failed to write config, authentication configuration not updated: %w", err) - } - - isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() - - if isTTY { - cs := opts.IO.ColorScheme() - fmt.Fprintf(opts.IO.ErrOut, "%s Logged out of %s%s\n", - cs.SuccessIcon(), cs.Bold(hostname), usernameStr) + if hasSwitchedToNewUser { + fmt.Fprintf(opts.IO.ErrOut, "%s Switched active account for %s to %s\n", + cs.SuccessIcon(), hostname, cs.Bold(postLogoutActiveUser)) } return nil diff --git a/pkg/cmd/auth/logout/logout_test.go b/pkg/cmd/auth/logout/logout_test.go index 75c215aaa..ca37770c3 100644 --- a/pkg/cmd/auth/logout/logout_test.go +++ b/pkg/cmd/auth/logout/logout_test.go @@ -2,58 +2,85 @@ package logout import ( "bytes" - "fmt" - "net/http" + "io" "regexp" "testing" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/cmdutil" - "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" - "github.com/stretchr/testify/assert" - "github.com/zalando/go-keyring" + "github.com/stretchr/testify/require" ) func Test_NewCmdLogout(t *testing.T) { tests := []struct { - name string - cli string - wants LogoutOptions - wantsErr bool - tty bool + name string + cli string + wants LogoutOptions + tty bool }{ { - name: "nontty no arguments", - cli: "", - wantsErr: true, + name: "nontty no arguments", + cli: "", + wants: LogoutOptions{}, }, { - name: "tty no arguments", - tty: true, - cli: "", - wants: LogoutOptions{ - Hostname: "", - }, + name: "tty no arguments", + tty: true, + cli: "", + wants: LogoutOptions{}, }, { name: "tty with hostname", tty: true, - cli: "--hostname harry.mason", + cli: "--hostname github.com", wants: LogoutOptions{ - Hostname: "harry.mason", + Hostname: "github.com", }, }, { name: "nontty with hostname", - cli: "--hostname harry.mason", + cli: "--hostname github.com", wants: LogoutOptions{ - Hostname: "harry.mason", + Hostname: "github.com", + }, + }, + { + name: "tty with user", + tty: true, + cli: "--user monalisa", + wants: LogoutOptions{ + Username: "github.com", + }, + }, + { + name: "nontty with user", + cli: "--user monalisa", + wants: LogoutOptions{ + Username: "github.com", + }, + }, + { + name: "tty with hostname and user", + tty: true, + cli: "--hostname github.com --user monalisa", + wants: LogoutOptions{ + Hostname: "github.com", + Username: "monalisa", + }, + }, + { + name: "nontty with hostname and user", + cli: "--hostname github.com --user monalisa", + wants: LogoutOptions{ + Hostname: "github.com", + Username: "monalisa", }, }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ios, _, _, _ := iostreams.Test() @@ -64,7 +91,7 @@ func Test_NewCmdLogout(t *testing.T) { ios.SetStdoutTTY(tt.tty) argv, err := shlex.Split(tt.cli) - assert.NoError(t, err) + require.NoError(t, err) var gotOpts *LogoutOptions cmd := NewCmdLogout(f, func(opts *LogoutOptions) error { @@ -80,88 +107,221 @@ func Test_NewCmdLogout(t *testing.T) { cmd.SetErr(&bytes.Buffer{}) _, err = cmd.ExecuteC() - if tt.wantsErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) + require.NoError(t, err) - assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) + require.Equal(t, tt.wants.Hostname, gotOpts.Hostname) }) - } } +type user struct { + name string + token string +} + +type hostUsers struct { + host string + users []user +} + +type tokenAssertion func(t *testing.T, cfg config.Config) + func Test_logoutRun_tty(t *testing.T) { tests := []struct { name string opts *LogoutOptions prompterStubs func(*prompter.PrompterMock) - cfgHosts []string + cfgHosts []hostUsers secureStorage bool wantHosts string + assertToken tokenAssertion wantErrOut *regexp.Regexp wantErr string }{ { - name: "no arguments, multiple hosts", - opts: &LogoutOptions{}, - cfgHosts: []string{"cheryl.mason", "github.com"}, - wantHosts: "cheryl.mason:\n oauth_token: abc123\n", + name: "logs out prompted user when multiple known hosts with one user each", + opts: &LogoutOptions{}, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + }}, + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, prompterStubs: func(pm *prompter.PrompterMock) { pm.SelectFunc = func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "github.com") + return prompter.IndexFor(opts, "monalisa (github.com)") } }, - wantErrOut: regexp.MustCompile(`Logged out of github.com account 'cybilb'`), + assertToken: hasNoToken("github.com"), + wantHosts: "ghe.io:\n users:\n monalisa-ghe:\n oauth_token: abc123\n git_protocol: ssh\n oauth_token: abc123\n user: monalisa-ghe\n", + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), }, { - name: "no arguments, one host", - opts: &LogoutOptions{}, - cfgHosts: []string{"github.com"}, - wantHosts: "{}\n", - wantErrOut: regexp.MustCompile(`Logged out of github.com account 'cybilb'`), + name: "logs out prompted user when multiple known hosts with multiple users each", + opts: &LogoutOptions{}, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + {"monalisa-ghe2", "abc123"}, + }}, + {"github.com", []user{ + {"monalisa", "monalisa-token"}, + {"monalisa2", "monalisa2-token"}, + }}, + }, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "monalisa (github.com)") + } + }, + assertToken: hasActiveToken("github.com", "monalisa2-token"), + wantHosts: "ghe.io:\n users:\n monalisa-ghe:\n oauth_token: abc123\n monalisa-ghe2:\n oauth_token: abc123\n git_protocol: ssh\n user: monalisa-ghe2\n oauth_token: abc123\ngithub.com:\n users:\n monalisa2:\n oauth_token: monalisa2-token\n git_protocol: ssh\n user: monalisa2\n oauth_token: monalisa2-token\n", + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), }, { - name: "no arguments, no hosts", + name: "logs out only logged in user", + opts: &LogoutOptions{}, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantHosts: "{}\n", + assertToken: hasNoToken("github.com"), + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), + }, + { + name: "logs out prompted user when one known host with multiple users", + opts: &LogoutOptions{}, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "monalisa-token"}, + {"monalisa2", "monalisa2-token"}, + }}, + }, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "monalisa (github.com)") + } + }, + wantHosts: "github.com:\n users:\n monalisa2:\n oauth_token: monalisa2-token\n git_protocol: ssh\n user: monalisa2\n oauth_token: monalisa2-token\n", + assertToken: hasActiveToken("github.com", "monalisa2-token"), + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), + }, + { + name: "logs out specified user when multiple known hosts with one user each", + opts: &LogoutOptions{ + Hostname: "ghe.io", + Username: "monalisa-ghe", + }, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + }}, + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc123\n git_protocol: ssh\n oauth_token: abc123\n user: monalisa\n", + assertToken: hasNoToken("ghe.io"), + wantErrOut: regexp.MustCompile(`Logged out of ghe.io account monalisa-ghe`), + }, + { + name: "logs out specified user that is using secure storage", + secureStorage: true, + opts: &LogoutOptions{ + Hostname: "github.com", + Username: "monalisa", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantHosts: "{}\n", + assertToken: hasNoToken("github.com"), + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), + }, + { + name: "errors when no known hosts", opts: &LogoutOptions{}, wantErr: `not logged in to any hosts`, }, { - name: "hostname", + name: "errors when specified host is not a known host", opts: &LogoutOptions{ - Hostname: "cheryl.mason", + Hostname: "ghe.io", + Username: "monalisa-ghe", }, - cfgHosts: []string{"cheryl.mason", "github.com"}, - wantHosts: "github.com:\n oauth_token: abc123\n", - wantErrOut: regexp.MustCompile(`Logged out of cheryl.mason account 'cybilb'`), + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantErr: "not logged in to ghe.io", }, { - name: "secure storage", - secureStorage: true, + name: "errors when specified user is not logged in on specified host", + opts: &LogoutOptions{ + Hostname: "ghe.io", + Username: "unknown-user", + }, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + }}, + }, + wantErr: "not logged in to ghe.io account unknown-user", + }, + { + name: "errors when user is specified but doesn't exist on any host", + opts: &LogoutOptions{ + Username: "unknown-user", + }, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + }}, + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantErr: "no accounts matched that criteria", + }, + { + name: "switches user if there is another one available", opts: &LogoutOptions{ Hostname: "github.com", + Username: "monalisa2", }, - cfgHosts: []string{"github.com"}, - wantHosts: "{}\n", - wantErrOut: regexp.MustCompile(`Logged out of github.com account 'cybilb'`), + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "monalisa-token"}, + {"monalisa2", "monalisa2-token"}, + }}, + }, + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: monalisa-token\n git_protocol: ssh\n user: monalisa\n oauth_token: monalisa-token\n", + assertToken: hasActiveToken("github.com", "monalisa-token"), + wantErrOut: regexp.MustCompile("✓ Switched active account for github.com to monalisa"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - keyring.MockInit() - readConfigs := config.StubWriteConfig(t) - cfg := config.NewFromString("") - for _, hostname := range tt.cfgHosts { - if tt.secureStorage { - cfg.Set(hostname, "user", "monalisa") - _ = keyring.Set(fmt.Sprintf("gh:%s", hostname), "", "abc123") - cfg.Authentication().SetToken("abc123", "keyring") - } else { - cfg.Set(hostname, "oauth_token", "abc123") + cfg, readConfigs := config.NewIsolatedTestConfig(t) + + for _, hostUsers := range tt.cfgHosts { + for _, user := range hostUsers.users { + _, _ = cfg.Authentication().Login( + string(hostUsers.host), + user.name, + user.token, "ssh", tt.secureStorage, + ) } } + tt.opts.Config = func() (config.Config, error) { return cfg, nil } @@ -171,15 +331,6 @@ func Test_logoutRun_tty(t *testing.T) { ios.SetStdoutTTY(true) tt.opts.IO = ios - reg := &httpmock.Registry{} - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"cybilb"}}}`), - ) - tt.opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - pm := &prompter.PrompterMock{} if tt.prompterStubs != nil { tt.prompterStubs(pm) @@ -188,26 +339,26 @@ func Test_logoutRun_tty(t *testing.T) { err := logoutRun(tt.opts) if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) + require.EqualError(t, err, tt.wantErr) return } else { - assert.NoError(t, err) + require.NoError(t, err) } if tt.wantErrOut == nil { - assert.Equal(t, "", stderr.String()) + require.Equal(t, "", stderr.String()) } else { - assert.True(t, tt.wantErrOut.MatchString(stderr.String())) + require.True(t, tt.wantErrOut.MatchString(stderr.String()), stderr.String()) } - mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} - readConfigs(&mainBuf, &hostsBuf) - secureToken, _ := cfg.Authentication().TokenFromKeyring(tt.opts.Hostname) + readConfigs(io.Discard, &hostsBuf) - assert.Equal(t, tt.wantHosts, hostsBuf.String()) - assert.Equal(t, "", secureToken) - reg.Verify(t) + require.Equal(t, tt.wantHosts, hostsBuf.String()) + + if tt.assertToken != nil { + tt.assertToken(t, cfg) + } }) } } @@ -216,58 +367,153 @@ func Test_logoutRun_nontty(t *testing.T) { tests := []struct { name string opts *LogoutOptions - cfgHosts []string + cfgHosts []hostUsers secureStorage bool - ghtoken string wantHosts string + assertToken tokenAssertion + wantErrOut *regexp.Regexp wantErr string }{ { - name: "hostname, one host", + name: "logs out specified user when one known host", opts: &LogoutOptions{ - Hostname: "harry.mason", + Hostname: "github.com", + Username: "monalisa", }, - cfgHosts: []string{"harry.mason"}, - wantHosts: "{}\n", + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantHosts: "{}\n", + assertToken: hasNoToken("github.com"), + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), }, { - name: "hostname, multiple hosts", + name: "logs out specified user when multiple known hosts", opts: &LogoutOptions{ - Hostname: "harry.mason", + Hostname: "github.com", + Username: "monalisa", }, - cfgHosts: []string{"harry.mason", "cheryl.mason"}, - wantHosts: "cheryl.mason:\n oauth_token: abc123\n", + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + }}, + }, + wantHosts: "ghe.io:\n users:\n monalisa-ghe:\n oauth_token: abc123\n git_protocol: ssh\n oauth_token: abc123\n user: monalisa-ghe\n", + assertToken: hasNoToken("github.com"), + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), }, { - name: "hostname, no hosts", + name: "logs out specified user that is using secure storage", + secureStorage: true, opts: &LogoutOptions{ - Hostname: "harry.mason", + Hostname: "github.com", + Username: "monalisa", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantHosts: "{}\n", + assertToken: hasNoToken("github.com"), + wantErrOut: regexp.MustCompile(`Logged out of github.com account monalisa`), + }, + { + name: "errors when no known hosts", + opts: &LogoutOptions{ + Hostname: "github.com", + Username: "monalisa", }, wantErr: `not logged in to any hosts`, }, { - name: "secure storage", - secureStorage: true, + name: "errors when specified host is not a known host", opts: &LogoutOptions{ - Hostname: "harry.mason", + Hostname: "ghe.io", + Username: "monalisa-ghe", }, - cfgHosts: []string{"harry.mason"}, - wantHosts: "{}\n", + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantErr: "not logged in to ghe.io", + }, + { + name: "errors when specified user is not logged in on specified host", + opts: &LogoutOptions{ + Hostname: "ghe.io", + Username: "unknown-user", + }, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + }}, + }, + wantErr: "not logged in to ghe.io account unknown-user", + }, + { + name: "errors when host is specified but user is ambiguous", + opts: &LogoutOptions{ + Hostname: "ghe.io", + }, + cfgHosts: []hostUsers{ + {"ghe.io", []user{ + {"monalisa-ghe", "abc123"}, + {"monalisa-ghe2", "abc123"}, + }}, + }, + wantErr: "unable to determine which account to log out of, please specify `--hostname` and `--user`", + }, + { + name: "errors when user is specified but host is ambiguous", + opts: &LogoutOptions{ + Username: "monalisa", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "abc123"}, + }}, + {"ghe.io", []user{ + {"monalisa", "abc123"}, + }}, + }, + wantErr: "unable to determine which account to log out of, please specify `--hostname` and `--user`", + }, + { + name: "switches user if there is another one available", + opts: &LogoutOptions{ + Hostname: "github.com", + Username: "monalisa2", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"monalisa", "monalisa-token"}, + {"monalisa2", "monalisa2-token"}, + }}, + }, + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: monalisa-token\n git_protocol: ssh\n user: monalisa\n oauth_token: monalisa-token\n", + assertToken: hasActiveToken("github.com", "monalisa-token"), + wantErrOut: regexp.MustCompile("✓ Switched active account for github.com to monalisa"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - keyring.MockInit() - readConfigs := config.StubWriteConfig(t) - cfg := config.NewFromString("") - for _, hostname := range tt.cfgHosts { - if tt.secureStorage { - cfg.Set(hostname, "user", "monalisa") - _ = keyring.Set(fmt.Sprintf("gh:%s", hostname), "", "abc123") - cfg.Authentication().SetToken("abc123", "keyring") - } else { - cfg.Set(hostname, "oauth_token", "abc123") + cfg, readConfigs := config.NewIsolatedTestConfig(t) + + for _, hostUsers := range tt.cfgHosts { + for _, user := range hostUsers.users { + _, _ = cfg.Authentication().Login( + string(hostUsers.host), + user.name, + user.token, "ssh", tt.secureStorage, + ) } } tt.opts.Config = func() (config.Config, error) { @@ -279,28 +525,46 @@ func Test_logoutRun_nontty(t *testing.T) { ios.SetStdoutTTY(false) tt.opts.IO = ios - reg := &httpmock.Registry{} - tt.opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - err := logoutRun(tt.opts) if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) + require.EqualError(t, err, tt.wantErr) + return } else { - assert.NoError(t, err) + require.NoError(t, err) } - assert.Equal(t, "", stderr.String()) + if tt.wantErrOut == nil { + require.Equal(t, "", stderr.String()) + } else { + require.True(t, tt.wantErrOut.MatchString(stderr.String()), stderr.String()) + } - mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} - readConfigs(&mainBuf, &hostsBuf) - secureToken, _ := cfg.Authentication().TokenFromKeyring(tt.opts.Hostname) + readConfigs(io.Discard, &hostsBuf) - assert.Equal(t, tt.wantHosts, hostsBuf.String()) - assert.Equal(t, "", secureToken) - reg.Verify(t) + require.Equal(t, tt.wantHosts, hostsBuf.String()) + + if tt.assertToken != nil { + tt.assertToken(t, cfg) + } }) } } + +func hasNoToken(hostname string) tokenAssertion { + return func(t *testing.T, cfg config.Config) { + t.Helper() + + token, _ := cfg.Authentication().ActiveToken(hostname) + require.Empty(t, token) + } +} + +func hasActiveToken(hostname string, expectedToken string) tokenAssertion { + return func(t *testing.T, cfg config.Config) { + t.Helper() + + token, _ := cfg.Authentication().ActiveToken(hostname) + require.Equal(t, expectedToken, token) + } +} diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 2335a9952..a918493e8 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -16,6 +16,9 @@ import ( "github.com/spf13/cobra" ) +type token string +type username string + type RefreshOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) @@ -29,7 +32,7 @@ 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) (token, username, error) Interactive bool InsecureStorage bool @@ -39,17 +42,9 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. opts := &RefreshOptions{ IO: f.IOStreams, Config: f.Config, - AuthFlow: func(authCfg *config.AuthConfig, io *iostreams.IOStreams, hostname string, scopes []string, interactive, secureStorage bool) error { - if secureStorage { - cs := io.ColorScheme() - fmt.Fprintf(io.ErrOut, "%s Using secure storage could break installed extensions", cs.WarningIcon()) - } - 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) (token, username, error) { + t, u, err := authflow.AuthFlow(hostname, io, "", scopes, interactive, f.Browser) + return token(t), username(u), err }, HttpClient: &http.Client{}, GitClient: f.GitClient, @@ -161,7 +156,7 @@ func refreshRun(opts *RefreshOptions) error { additionalScopes := set.NewStringSet() if !opts.ResetScopes { - if oldToken, _ := authCfg.Token(hostname); oldToken != "" { + if oldToken, _ := authCfg.ActiveToken(hostname); oldToken != "" { if oldScopes, err := shared.GetScopes(opts.HttpClient, hostname, oldToken); err == nil { for _, s := range strings.Split(oldScopes, ",") { s = strings.TrimSpace(s) @@ -190,7 +185,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 { + authedToken, authedUser, err := opts.AuthFlow(opts.IO, hostname, additionalScopes.ToSlice(), opts.Interactive) + if err != nil { + return err + } + activeUser, _ := authCfg.ActiveUser(hostname) + if activeUser != "" && username(activeUser) != authedUser { + return fmt.Errorf("error refreshing credentials for %s, received credentials for %s, did you use the correct account in the browser?", activeUser, authedUser) + } + if _, err := authCfg.Login(hostname, string(authedUser), string(authedToken), "", !opts.InsecureStorage); err != nil { return err } @@ -198,8 +201,8 @@ func refreshRun(opts *RefreshOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s Authentication complete.\n", cs.SuccessIcon()) if credentialFlow.ShouldSetup() { - username, _ := authCfg.User(hostname) - password, _ := authCfg.Token(hostname) + username, _ := authCfg.ActiveUser(hostname) + password, _ := authCfg.ActiveToken(hostname) if err := credentialFlow.Setup(hostname, username, password); err != nil { return err } diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index c87c2bcb8..51fa66bd6 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -13,7 +13,7 @@ import ( "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NewCmdRefresh(t *testing.T) { @@ -142,7 +142,7 @@ func Test_NewCmdRefresh(t *testing.T) { ios.SetNeverPrompt(tt.neverPrompt) argv, err := shlex.Split(tt.cli) - assert.NoError(t, err) + require.NoError(t, err) var gotOpts *RefreshOptions cmd := NewCmdRefresh(f, func(opts *RefreshOptions) error { @@ -159,12 +159,12 @@ func Test_NewCmdRefresh(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantsErr { - assert.Error(t, err) + require.Error(t, err) return } - assert.NoError(t, err) - assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) - assert.Equal(t, tt.wants.Scopes, gotOpts.Scopes) + require.NoError(t, err) + require.Equal(t, tt.wants.Hostname, gotOpts.Hostname) + require.Equal(t, tt.wants.Scopes, gotOpts.Scopes) }) } } @@ -176,13 +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 - config config.Config + authOut authOut oldScopes string wantErr string nontty bool @@ -194,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", @@ -404,26 +410,36 @@ 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, did you use the correct account in the browser?", + }, } 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) (token, username, error) { aa.hostname = hostname aa.scopes = scopes aa.interactive = interactive - aa.secureStorage = secureStorage - return nil + if tt.authOut != (authOut{}) { + return token(tt.authOut.token), username(tt.authOut.username), tt.authOut.err + } + return token("xyz456"), username("test-user"), nil } - var cfg config.Config - if tt.config != nil { - cfg = tt.config - } else { - cfg = config.NewFromString("") - for _, hostname := range tt.cfgHosts { - cfg.Set(hostname, "oauth_token", "abc123") - } + cfg, _ := config.NewIsolatedTestConfig(t) + for _, hostname := range tt.cfgHosts { + _, err := cfg.Authentication().Login(hostname, "test-user", "abc123", "https", false) + require.NoError(t, err) } tt.opts.Config = func() (config.Config, error) { return cfg, nil @@ -462,17 +478,21 @@ func Test_refreshRun(t *testing.T) { err := refreshRun(tt.opts) if tt.wantErr != "" { - if assert.Error(t, err) { - assert.Contains(t, err.Error(), tt.wantErr) - } - } else { - assert.NoError(t, err) + require.Contains(t, err.Error(), tt.wantErr) + return } - assert.Equal(t, tt.wantAuthArgs.hostname, aa.hostname) - assert.Equal(t, tt.wantAuthArgs.scopes, aa.scopes) - assert.Equal(t, tt.wantAuthArgs.interactive, aa.interactive) - assert.Equal(t, tt.wantAuthArgs.secureStorage, aa.secureStorage) + 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) + + authCfg := cfg.Authentication() + activeUser, _ := authCfg.ActiveUser(aa.hostname) + activeToken, _ := authCfg.ActiveToken(aa.hostname) + require.Equal(t, "test-user", activeUser) + require.Equal(t, "xyz456", activeToken) }) } } diff --git a/pkg/cmd/auth/setupgit/setupgit_test.go b/pkg/cmd/auth/setupgit/setupgit_test.go index 635cece8c..dd8b2a4d6 100644 --- a/pkg/cmd/auth/setupgit/setupgit_test.go +++ b/pkg/cmd/auth/setupgit/setupgit_test.go @@ -6,23 +6,32 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type mockGitConfigurer struct { + hosts []string setupErr error } +func (gf *mockGitConfigurer) SetupFor(hostname string) []string { + return gf.hosts +} + func (gf *mockGitConfigurer) Setup(hostname, username, authToken string) error { + gf.hosts = append(gf.hosts, hostname) return gf.setupErr } func Test_setupGitRun(t *testing.T) { tests := []struct { - name string - opts *SetupGitOptions - expectedErr string - expectedErrOut string + name string + opts *SetupGitOptions + setupErr error + cfgStubs func(*testing.T, config.Config) + expectedHostsSetup []string + expectedErr string + expectedErrOut string }{ { name: "opts.Config returns an error", @@ -34,18 +43,8 @@ func Test_setupGitRun(t *testing.T) { expectedErr: "oops", }, { - name: "no authenticated hostnames", - opts: &SetupGitOptions{ - Config: func() (config.Config, error) { - cfg := &config.ConfigMock{} - cfg.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetHosts([]string{}) - return authCfg - } - return cfg, nil - }, - }, + name: "no authenticated hostnames", + opts: &SetupGitOptions{}, expectedErr: "SilentError", expectedErrOut: "You are not logged into any GitHub hosts. Run gh auth login to authenticate.\n", }, @@ -53,79 +52,46 @@ func Test_setupGitRun(t *testing.T) { name: "not authenticated with the hostname given as flag", opts: &SetupGitOptions{ Hostname: "foo", - Config: func() (config.Config, error) { - cfg := &config.ConfigMock{} - cfg.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetHosts([]string{"bar"}) - return authCfg - } - return cfg, nil - }, + }, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) }, expectedErr: "You are not logged into the GitHub host \"foo\"\n", expectedErrOut: "", }, { - name: "error setting up git for hostname", - opts: &SetupGitOptions{ - gitConfigure: &mockGitConfigurer{ - setupErr: fmt.Errorf("broken"), - }, - Config: func() (config.Config, error) { - cfg := &config.ConfigMock{} - cfg.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetHosts([]string{"bar"}) - return authCfg - } - return cfg, nil - }, + name: "error setting up git for hostname", + opts: &SetupGitOptions{}, + setupErr: fmt.Errorf("broken"), + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) }, expectedErr: "failed to set up git credential helper: broken", expectedErrOut: "", }, { name: "no hostname option given. Setup git for each hostname in config", - opts: &SetupGitOptions{ - gitConfigure: &mockGitConfigurer{}, - Config: func() (config.Config, error) { - cfg := &config.ConfigMock{} - cfg.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetHosts([]string{"bar"}) - return authCfg - } - return cfg, nil - }, + opts: &SetupGitOptions{}, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "ghe.io", "test-user", "gho_ABCDEFG", "https", false) + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) }, + expectedHostsSetup: []string{"github.com", "ghe.io"}, }, { name: "setup git for the hostname given via options", opts: &SetupGitOptions{ - Hostname: "yes", - gitConfigure: &mockGitConfigurer{}, - Config: func() (config.Config, error) { - cfg := &config.ConfigMock{} - cfg.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetHosts([]string{"bar", "yes"}) - return authCfg - } - return cfg, nil - }, + Hostname: "ghe.io", }, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "ghe.io", "test-user", "gho_ABCDEFG", "https", false) + }, + expectedHostsSetup: []string{"ghe.io"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.opts.Config == nil { - tt.opts.Config = func() (config.Config, error) { - return &config.ConfigMock{}, nil - } - } - ios, _, _, stderr := iostreams.Test() ios.SetStdinTTY(true) @@ -133,14 +99,38 @@ func Test_setupGitRun(t *testing.T) { ios.SetStdoutTTY(true) tt.opts.IO = ios - err := setupGitRun(tt.opts) - if tt.expectedErr != "" { - assert.EqualError(t, err, tt.expectedErr) - } else { - assert.NoError(t, err) + cfg, _ := config.NewIsolatedTestConfig(t) + if tt.cfgStubs != nil { + tt.cfgStubs(t, cfg) } - assert.Equal(t, tt.expectedErrOut, stderr.String()) + if tt.opts.Config == nil { + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + } + + gcSpy := &mockGitConfigurer{setupErr: tt.setupErr} + tt.opts.gitConfigure = gcSpy + + err := setupGitRun(tt.opts) + if tt.expectedErr != "" { + require.EqualError(t, err, tt.expectedErr) + } else { + require.NoError(t, err) + } + + if tt.expectedHostsSetup != nil { + require.Equal(t, tt.expectedHostsSetup, gcSpy.hosts) + } + + require.Equal(t, tt.expectedErrOut, stderr.String()) }) } } + +func login(t *testing.T, c config.Config, hostname, username, token, gitProtocol string, secureStorage bool) { + t.Helper() + _, err := c.Authentication().Login(hostname, username, token, "https", secureStorage) + require.NoError(t, err) +} diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 5f8a524cb..7bb974547 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "os" + "slices" "strings" "github.com/MakeNowJust/heredoc" @@ -23,6 +24,7 @@ const defaultSSHKeyTitle = "GitHub CLI" type iconfig interface { Login(string, string, string, string, bool) (bool, error) + UsersForHost(string) []string } type LoginOptions struct { @@ -56,7 +58,7 @@ func Login(opts *LoginOptions) error { "SSH", } result, err := opts.Prompter.Select( - "What is your preferred protocol for Git operations?", + "What is your preferred protocol for Git operations on this host?", options[0], options) if err != nil { @@ -177,12 +179,21 @@ func Login(opts *LoginOptions) error { if username == "" { var err error - username, err = getCurrentLogin(httpClient, hostname, authToken) + username, err = GetCurrentLogin(httpClient, hostname, authToken) if err != nil { - return fmt.Errorf("error using api: %w", err) + return fmt.Errorf("error retrieving current user: %w", err) } } + // Get these users before adding the new one, so that we can + // check whether the user was already logged in later. + // + // In this case we ignore the error if the host doesn't exist + // because that can occur when the user is logging into a host + // for the first time. + usersForHost := cfg.UsersForHost(hostname) + userWasAlreadyLoggedIn := slices.Contains(usersForHost, username) + if gitProtocol != "" { fmt.Fprintf(opts.IO.ErrOut, "- gh config set -h %s git_protocol %s\n", hostname, gitProtocol) fmt.Fprintf(opts.IO.ErrOut, "%s Configured git protocol\n", cs.SuccessIcon()) @@ -217,6 +228,10 @@ func Login(opts *LoginOptions) error { } fmt.Fprintf(opts.IO.ErrOut, "%s Logged in as %s\n", cs.SuccessIcon(), cs.Bold(username)) + if userWasAlreadyLoggedIn { + fmt.Fprintf(opts.IO.ErrOut, "%s You were already logged in to this account\n", cs.WarningIcon()) + } + return nil } @@ -238,7 +253,7 @@ func sshKeyUpload(httpClient *http.Client, hostname, keyFile string, title strin return add.SSHKeyUpload(httpClient, hostname, f, title) } -func getCurrentLogin(httpClient httpClient, hostname, authToken string) (string, error) { +func GetCurrentLogin(httpClient httpClient, hostname, authToken string) (string, error) { query := `query UserCurrent{viewer{login}}` reqBody, err := json.Marshal(map[string]interface{}{"query": query}) if err != nil { diff --git a/pkg/cmd/auth/shared/login_flow_test.go b/pkg/cmd/auth/shared/login_flow_test.go index 5dbbc46c1..263ecc3d8 100644 --- a/pkg/cmd/auth/shared/login_flow_test.go +++ b/pkg/cmd/auth/shared/login_flow_test.go @@ -25,6 +25,10 @@ func (c tinyConfig) Login(host, username, token, gitProtocol string, encrypt boo return false, nil } +func (c tinyConfig) UsersForHost(hostname string) []string { + return nil +} + func TestLogin_ssh(t *testing.T) { dir := t.TempDir() ios, _, stdout, stderr := iostreams.Test() @@ -48,7 +52,7 @@ func TestLogin_ssh(t *testing.T) { pm := &prompter.PrompterMock{} pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { switch prompt { - case "What is your preferred protocol for Git operations?": + case "What is your preferred protocol for Git operations on this host?": return prompter.IndexFor(opts, "SSH") case "How would you like to authenticate GitHub CLI?": return prompter.IndexFor(opts, "Paste an authentication token") diff --git a/pkg/cmd/auth/shared/writeable.go b/pkg/cmd/auth/shared/writeable.go index cf8e678e4..e5ae91469 100644 --- a/pkg/cmd/auth/shared/writeable.go +++ b/pkg/cmd/auth/shared/writeable.go @@ -7,6 +7,6 @@ import ( ) func AuthTokenWriteable(authCfg *config.AuthConfig, hostname string) (string, bool) { - token, src := authCfg.Token(hostname) + token, src := authCfg.ActiveToken(hostname) return src, (token == "" || !strings.HasSuffix(src, "_TOKEN")) } diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index 831dfc503..fa027e193 100644 --- a/pkg/cmd/auth/status/status.go +++ b/pkg/cmd/auth/status/status.go @@ -6,6 +6,7 @@ import ( "net" "net/http" "path/filepath" + "slices" "strings" "github.com/MakeNowJust/heredoc" @@ -17,6 +18,109 @@ import ( "github.com/spf13/cobra" ) +type validEntry struct { + active bool + host string + user string + token string + tokenSource string + gitProtocol string + scopes string +} + +func (e validEntry) String(cs *iostreams.ColorScheme) string { + var sb strings.Builder + + sb.WriteString( + fmt.Sprintf(" %s Logged in to %s account %s (%s)\n", cs.SuccessIcon(), e.host, cs.Bold(e.user), e.tokenSource), + ) + activeStr := fmt.Sprintf("%v", e.active) + sb.WriteString(fmt.Sprintf(" - Active account: %s\n", cs.Bold(activeStr))) + sb.WriteString(fmt.Sprintf(" - Git operations protocol: %s\n", cs.Bold(e.gitProtocol))) + sb.WriteString(fmt.Sprintf(" - Token: %s\n", cs.Bold(e.token))) + + if expectScopes(e.token) { + sb.WriteString(fmt.Sprintf(" - Token scopes: %s\n", cs.Bold(displayScopes(e.scopes)))) + if err := shared.HeaderHasMinimumScopes(e.scopes); err != nil { + var missingScopesError *shared.MissingScopesError + if errors.As(err, &missingScopesError) { + missingScopes := strings.Join(missingScopesError.MissingScopes, ",") + sb.WriteString(fmt.Sprintf(" %s Missing required token scopes: %s\n", + cs.WarningIcon(), + cs.Bold(displayScopes(missingScopes)))) + refreshInstructions := fmt.Sprintf("gh auth refresh -h %s", e.host) + sb.WriteString(fmt.Sprintf(" - To request missing scopes, run: %s\n", cs.Bold(refreshInstructions))) + } + } + } + + return sb.String() +} + +type invalidTokenEntry struct { + active bool + host string + user string + tokenSource string + tokenIsWriteable bool +} + +func (e invalidTokenEntry) String(cs *iostreams.ColorScheme) string { + var sb strings.Builder + + if e.user != "" { + sb.WriteString(fmt.Sprintf(" %s Failed to log in to %s account %s (%s)\n", cs.Red("X"), e.host, cs.Bold(e.user), e.tokenSource)) + } else { + sb.WriteString(fmt.Sprintf(" %s Failed to log in to %s using token (%s)\n", cs.Red("X"), e.host, e.tokenSource)) + } + activeStr := fmt.Sprintf("%v", e.active) + sb.WriteString(fmt.Sprintf(" - Active account: %s\n", cs.Bold(activeStr))) + sb.WriteString(fmt.Sprintf(" - The token in %s is invalid.\n", e.tokenSource)) + if e.tokenIsWriteable { + loginInstructions := fmt.Sprintf("gh auth login -h %s", e.host) + logoutInstructions := fmt.Sprintf("gh auth logout -h %s -u %s", e.host, e.user) + sb.WriteString(fmt.Sprintf(" - To re-authenticate, run: %s\n", cs.Bold(loginInstructions))) + sb.WriteString(fmt.Sprintf(" - To forget about this account, run: %s\n", cs.Bold(logoutInstructions))) + } + + return sb.String() +} + +type timeoutErrorEntry struct { + active bool + host string + user string + tokenSource string +} + +func (e timeoutErrorEntry) String(cs *iostreams.ColorScheme) string { + var sb strings.Builder + + if e.user != "" { + sb.WriteString(fmt.Sprintf(" %s Timeout trying to log in to %s account %s (%s)\n", cs.Red("X"), e.host, cs.Bold(e.user), e.tokenSource)) + } else { + sb.WriteString(fmt.Sprintf(" %s Timeout trying to log in to %s using token (%s)\n", cs.Red("X"), e.host, e.tokenSource)) + } + activeStr := fmt.Sprintf("%v", e.active) + sb.WriteString(fmt.Sprintf(" - Active account: %s\n", cs.Bold(activeStr))) + + return sb.String() +} + +type Entry interface { + String(cs *iostreams.ColorScheme) string +} + +type Entries []Entry + +func (e Entries) Strings(cs *iostreams.ColorScheme) []string { + var out []string + for _, entry := range e { + out = append(out, entry.String(cs)) + } + return out +} + type StatusOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams @@ -64,18 +168,22 @@ func statusRun(opts *StatusOptions) error { } authCfg := cfg.Authentication() - // TODO check tty - stderr := opts.IO.ErrOut stdout := opts.IO.Out cs := opts.IO.ColorScheme() - statusInfo := map[string][]string{} + statuses := make(map[string]Entries) hostnames := authCfg.Hosts() if len(hostnames) == 0 { fmt.Fprintf(stderr, - "You are not logged into any GitHub hosts. Run %s to authenticate.\n", cs.Bold("gh auth login")) + "You are not logged into any GitHub hosts. To log in, run: %s\n", cs.Bold("gh auth login")) + return cmdutil.SilentError + } + + if opts.Hostname != "" && !slices.Contains(hostnames, opts.Hostname) { + fmt.Fprintf(stderr, + "You are not logged into any accounts on %s\n", opts.Hostname) return cmdutil.SilentError } @@ -84,115 +192,60 @@ func statusRun(opts *StatusOptions) error { return err } - var failed bool - var isHostnameFound bool - for _, hostname := range hostnames { if opts.Hostname != "" && opts.Hostname != hostname { continue } - isHostnameFound = true - token, tokenSource := authCfg.Token(hostname) - if tokenSource == "oauth_token" { - // The go-gh function TokenForHost returns this value as source for tokens read from the - // config file, but we want the file path instead. This attempts to reconstruct it. - tokenSource = filepath.Join(config.ConfigDir(), "hosts.yml") + var activeUser string + gitProtocol := cfg.GitProtocol(hostname) + activeUserToken, activeUserTokenSource := authCfg.ActiveToken(hostname) + if authTokenWriteable(activeUserTokenSource) { + activeUser, _ = authCfg.ActiveUser(hostname) } - _, tokenIsWriteable := shared.AuthTokenWriteable(authCfg, hostname) + entry := buildEntry(httpClient, buildEntryOptions{ + active: true, + gitProtocol: gitProtocol, + hostname: hostname, + showToken: opts.ShowToken, + token: activeUserToken, + tokenSource: activeUserTokenSource, + username: activeUser, + }) + statuses[hostname] = append(statuses[hostname], entry) - statusInfo[hostname] = []string{} - addMsg := func(x string, ys ...interface{}) { - statusInfo[hostname] = append(statusInfo[hostname], fmt.Sprintf(x, ys...)) + users := authCfg.UsersForHost(hostname) + for _, username := range users { + if username == activeUser { + continue + } + token, tokenSource, _ := authCfg.TokenForUser(hostname, username) + entry := buildEntry(httpClient, buildEntryOptions{ + active: false, + gitProtocol: gitProtocol, + hostname: hostname, + showToken: opts.ShowToken, + token: token, + tokenSource: tokenSource, + username: username, + }) + statuses[hostname] = append(statuses[hostname], entry) } - - scopesHeader, err := shared.GetScopes(httpClient, hostname, token) - if err != nil { - var networkError net.Error - if errors.As(err, &networkError) && networkError.Timeout() { - addMsg("%s %s: timeout trying to connect to host", cs.Red("X"), hostname) - } else { - addMsg("%s %s: authentication failed", cs.Red("X"), hostname) - addMsg("- The %s token in %s is invalid.", cs.Bold(hostname), tokenSource) - if tokenIsWriteable { - addMsg("- To re-authenticate, run: %s %s", - cs.Bold("gh auth login -h"), cs.Bold(hostname)) - addMsg("- To forget about this host, run: %s %s", - cs.Bold("gh auth logout -h"), cs.Bold(hostname)) - } - } - failed = true - continue - } - - if err := shared.HeaderHasMinimumScopes(scopesHeader); err != nil { - var missingScopes *shared.MissingScopesError - if errors.As(err, &missingScopes) { - addMsg("%s %s: the token in %s is %s", cs.Red("X"), hostname, tokenSource, err) - if tokenIsWriteable { - addMsg("- To request missing scopes, run: %s %s", - cs.Bold("gh auth refresh -h"), - cs.Bold(hostname)) - } - } - failed = true - } else { - apiClient := api.NewClientFromHTTP(httpClient) - username, err := api.CurrentLoginName(apiClient, hostname) - if err != nil { - addMsg("%s %s: api call failed: %s", cs.Red("X"), hostname, err) - failed = true - } - - addMsg("%s Logged in to %s as %s (%s)", cs.SuccessIcon(), hostname, cs.Bold(username), tokenSource) - proto := cfg.GitProtocol(hostname) - if proto != "" { - addMsg("%s Git operations for %s configured to use %s protocol.", - cs.SuccessIcon(), hostname, cs.Bold(proto)) - } - addMsg("%s Token: %s", cs.SuccessIcon(), displayToken(token, opts.ShowToken)) - - if scopesHeader != "" { - addMsg("%s Token scopes: %s", cs.SuccessIcon(), scopesHeader) - } else if expectScopes(token) { - addMsg("%s Token scopes: none", cs.Red("X")) - } - } - } - - if !isHostnameFound { - fmt.Fprintf(stderr, - "Hostname %q not found among authenticated GitHub hosts\n", opts.Hostname) - return cmdutil.SilentError } prevEntry := false for _, hostname := range hostnames { - lines, ok := statusInfo[hostname] + entries, ok := statuses[hostname] if !ok { continue } - if prevEntry && failed { - fmt.Fprint(stderr, "\n") - } else if prevEntry && !failed { + + if prevEntry { fmt.Fprint(stdout, "\n") } prevEntry = true - if failed { - fmt.Fprintf(stderr, "%s\n", cs.Bold(hostname)) - for _, line := range lines { - fmt.Fprintf(stderr, " %s\n", line) - } - } else { - fmt.Fprintf(stdout, "%s\n", cs.Bold(hostname)) - for _, line := range lines { - fmt.Fprintf(stdout, " %s\n", line) - } - } - } - - if failed { - return cmdutil.SilentError + fmt.Fprintf(stdout, "%s\n", cs.Bold(hostname)) + fmt.Fprintf(stdout, "%s", strings.Join(entries.Strings(cs), "\n")) } return nil @@ -211,6 +264,92 @@ func displayToken(token string, printRaw bool) string { return strings.Repeat("*", len(token)) } +func displayScopes(scopes string) string { + if scopes == "" { + return "none" + } + list := strings.Split(scopes, ",") + for i, s := range list { + list[i] = fmt.Sprintf("'%s'", strings.TrimSpace(s)) + } + return strings.Join(list, ", ") +} + func expectScopes(token string) bool { return strings.HasPrefix(token, "ghp_") || strings.HasPrefix(token, "gho_") } + +type buildEntryOptions struct { + active bool + gitProtocol string + hostname string + showToken bool + token string + tokenSource string + username string +} + +func buildEntry(httpClient *http.Client, opts buildEntryOptions) Entry { + tokenIsWriteable := authTokenWriteable(opts.tokenSource) + + if opts.tokenSource == "oauth_token" { + // The go-gh function TokenForHost returns this value as source for tokens read from the + // config file, but we want the file path instead. This attempts to reconstruct it. + opts.tokenSource = filepath.Join(config.ConfigDir(), "hosts.yml") + } + + // If token is not writeable, then it came from an environment variable and + // we need to fetch the username as it won't be stored in the config. + if !tokenIsWriteable { + // The httpClient will automatically use the correct token here as + // the token from the environment variable take highest precedence. + apiClient := api.NewClientFromHTTP(httpClient) + var err error + opts.username, err = api.CurrentLoginName(apiClient, opts.hostname) + if err != nil { + return invalidTokenEntry{ + active: opts.active, + host: opts.hostname, + user: opts.username, + tokenIsWriteable: tokenIsWriteable, + tokenSource: opts.tokenSource, + } + } + } + + // Get scopes for token. + scopesHeader, err := shared.GetScopes(httpClient, opts.hostname, opts.token) + if err != nil { + var networkError net.Error + if errors.As(err, &networkError) && networkError.Timeout() { + return timeoutErrorEntry{ + active: opts.active, + host: opts.hostname, + user: opts.username, + tokenSource: opts.tokenSource, + } + } + + return invalidTokenEntry{ + active: opts.active, + host: opts.hostname, + user: opts.username, + tokenIsWriteable: tokenIsWriteable, + tokenSource: opts.tokenSource, + } + } + + return validEntry{ + active: opts.active, + gitProtocol: opts.gitProtocol, + host: opts.hostname, + scopes: scopesHeader, + token: displayToken(opts.token, opts.showToken), + tokenSource: opts.tokenSource, + user: opts.username, + } +} + +func authTokenWriteable(src string) bool { + return !strings.HasSuffix(src, "_TOKEN") +} diff --git a/pkg/cmd/auth/status/status_test.go b/pkg/cmd/auth/status/status_test.go index dfebddda3..a96b36c5b 100644 --- a/pkg/cmd/auth/status/status_test.go +++ b/pkg/cmd/auth/status/status_test.go @@ -15,6 +15,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NewCmdStatus(t *testing.T) { @@ -74,130 +75,104 @@ func Test_NewCmdStatus(t *testing.T) { } func Test_statusRun(t *testing.T) { - readConfigs := config.StubWriteConfig(t) - tests := []struct { name string - opts *StatusOptions + opts StatusOptions + env map[string]string httpStubs func(*httpmock.Registry) - cfgStubs func(*config.ConfigMock) - wantErr string + cfgStubs func(*testing.T, config.Config) + wantErr error wantOut string wantErrOut string }{ { name: "timeout error", - opts: &StatusOptions{ - Hostname: "joel.miller", + opts: StatusOptions{ + Hostname: "github.com", }, - cfgStubs: func(c *config.ConfigMock) { - c.Set("joel.miller", "oauth_token", "abc123") + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "abc123", "https") }, httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", "api/v3/"), func(req *http.Request) (*http.Response, error) { + reg.Register(httpmock.REST("GET", ""), func(req *http.Request) (*http.Response, error) { // timeout error return nil, context.DeadlineExceeded }) }, - wantErr: "SilentError", - wantErrOut: heredoc.Doc(` - joel.miller - X joel.miller: timeout trying to connect to host + wantOut: heredoc.Doc(` + github.com + X Timeout trying to log in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: true `), }, { name: "hostname set", - opts: &StatusOptions{ - Hostname: "joel.miller", + opts: StatusOptions{ + Hostname: "ghe.io", }, - cfgStubs: func(c *config.ConfigMock) { - c.Set("joel.miller", "oauth_token", "abc123") - c.Set("github.com", "oauth_token", "abc123") + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "gho_abc123", "https") + login(t, c, "ghe.io", "monalisa-ghe", "gho_abc123", "https") }, httpStubs: func(reg *httpmock.Registry) { // mocks for HeaderHasMinimumScopes api requests to a non-github.com host reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo,read:org")) - // mock for CurrentLoginName - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, wantOut: heredoc.Doc(` - joel.miller - ✓ Logged in to joel.miller as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for joel.miller configured to use https protocol. - ✓ Token: ****** - ✓ Token scopes: repo,read:org + ghe.io + ✓ Logged in to ghe.io account monalisa-ghe (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo', 'read:org' `), }, { name: "missing scope", - opts: &StatusOptions{}, - cfgStubs: func(c *config.ConfigMock) { - c.Set("joel.miller", "oauth_token", "abc123") - c.Set("github.com", "oauth_token", "abc123") + opts: StatusOptions{}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "ghe.io", "monalisa-ghe", "gho_abc123", "https") }, httpStubs: func(reg *httpmock.Registry) { // mocks for HeaderHasMinimumScopes api requests to a non-github.com host reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo")) - // mocks for HeaderHasMinimumScopes api requests to github.com host - reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) - // mock for CurrentLoginName - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, - wantErr: "SilentError", - wantErrOut: heredoc.Doc(` - joel.miller - X joel.miller: the token in GH_CONFIG_DIR/hosts.yml is missing required scope 'read:org' - - To request missing scopes, run: gh auth refresh -h joel.miller - - github.com - ✓ Logged in to github.com as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for github.com configured to use https protocol. - ✓ Token: ****** - ✓ Token scopes: repo,read:org + wantOut: heredoc.Doc(` + ghe.io + ✓ Logged in to ghe.io account monalisa-ghe (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo' + ! Missing required token scopes: 'read:org' + - To request missing scopes, run: gh auth refresh -h ghe.io `), }, { name: "bad token", - opts: &StatusOptions{}, - cfgStubs: func(c *config.ConfigMock) { - c.Set("joel.miller", "oauth_token", "abc123") - c.Set("github.com", "oauth_token", "abc123") + opts: StatusOptions{}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "ghe.io", "monalisa-ghe", "gho_abc123", "https") }, httpStubs: func(reg *httpmock.Registry) { // mock for HeaderHasMinimumScopes api requests to a non-github.com host reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.StatusStringResponse(400, "no bueno")) - // mock for HeaderHasMinimumScopes api requests to github.com - reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) - // mock for CurrentLoginName - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, - wantErr: "SilentError", - wantErrOut: heredoc.Doc(` - joel.miller - X joel.miller: authentication failed - - The joel.miller token in GH_CONFIG_DIR/hosts.yml is invalid. - - To re-authenticate, run: gh auth login -h joel.miller - - To forget about this host, run: gh auth logout -h joel.miller - - github.com - ✓ Logged in to github.com as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for github.com configured to use https protocol. - ✓ Token: ****** - ✓ Token scopes: repo,read:org + wantOut: heredoc.Doc(` + ghe.io + X Failed to log in to ghe.io account monalisa-ghe (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - The token in GH_CONFIG_DIR/hosts.yml is invalid. + - To re-authenticate, run: gh auth login -h ghe.io + - To forget about this account, run: gh auth logout -h ghe.io -u monalisa-ghe `), }, { name: "all good", - opts: &StatusOptions{}, - cfgStubs: func(c *config.ConfigMock) { - c.Set("github.com", "oauth_token", "gho_abc123") - c.Set("joel.miller", "oauth_token", "gho_abc123") + opts: StatusOptions{}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "gho_abc123", "https") + login(t, c, "ghe.io", "monalisa-ghe", "gho_abc123", "ssh") }, httpStubs: func(reg *httpmock.Registry) { // mocks for HeaderHasMinimumScopes api requests to github.com @@ -208,139 +183,220 @@ func Test_statusRun(t *testing.T) { reg.Register( httpmock.REST("GET", "api/v3/"), httpmock.WithHeader(httpmock.ScopesResponder("repo,read:org"), "X-Oauth-Scopes", "")) - // mock for CurrentLoginName, one for each host - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, wantOut: heredoc.Doc(` github.com - ✓ Logged in to github.com as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for github.com configured to use https protocol. - ✓ Token: gho_****** - ✓ Token scopes: repo, read:org - - joel.miller - ✓ Logged in to joel.miller as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for joel.miller configured to use https protocol. - ✓ Token: gho_****** - X Token scopes: none + ✓ Logged in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo', 'read:org' + + ghe.io + ✓ Logged in to ghe.io account monalisa-ghe (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: ssh + - Token: gho_****** + - Token scopes: none + `), + }, + { + name: "token from env", + opts: StatusOptions{}, + env: map[string]string{"GH_TOKEN": "gho_abc123"}, + cfgStubs: func(t *testing.T, c config.Config) {}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", ""), + httpmock.ScopesResponder("")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) + }, + wantOut: heredoc.Doc(` + github.com + ✓ Logged in to github.com account monalisa (GH_TOKEN) + - Active account: true + - Git operations protocol: https + - Token: gho_****** + - Token scopes: none `), }, { name: "server-to-server token", - opts: &StatusOptions{}, - cfgStubs: func(c *config.ConfigMock) { - c.Set("github.com", "oauth_token", "ghs_xxx") + opts: StatusOptions{}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "ghs_abc123", "https") }, httpStubs: func(reg *httpmock.Registry) { // mocks for HeaderHasMinimumScopes api requests to github.com reg.Register( httpmock.REST("GET", ""), httpmock.ScopesResponder("")) - // mock for CurrentLoginName - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, wantOut: heredoc.Doc(` github.com - ✓ Logged in to github.com as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for github.com configured to use https protocol. - ✓ Token: ghs_*** + ✓ Logged in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: ghs_****** `), }, { name: "PAT V2 token", - opts: &StatusOptions{}, - cfgStubs: func(c *config.ConfigMock) { - c.Set("github.com", "oauth_token", "github_pat_xxx") + opts: StatusOptions{}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "github_pat_abc123", "https") }, httpStubs: func(reg *httpmock.Registry) { // mocks for HeaderHasMinimumScopes api requests to github.com reg.Register( httpmock.REST("GET", ""), httpmock.ScopesResponder("")) - // mock for CurrentLoginName - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, wantOut: heredoc.Doc(` github.com - ✓ Logged in to github.com as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for github.com configured to use https protocol. - ✓ Token: github_pat_*** + ✓ Logged in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: github_pat_****** `), }, { name: "show token", - opts: &StatusOptions{ + opts: StatusOptions{ ShowToken: true, }, - cfgStubs: func(c *config.ConfigMock) { - c.Set("github.com", "oauth_token", "xyz456") - c.Set("joel.miller", "oauth_token", "abc123") + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "gho_abc123", "https") + login(t, c, "ghe.io", "monalisa-ghe", "gho_xyz456", "https") }, httpStubs: func(reg *httpmock.Registry) { // mocks for HeaderHasMinimumScopes on a non-github.com host reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo,read:org")) // mocks for HeaderHasMinimumScopes on github.com reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) - // mock for CurrentLoginName, one for each host - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) }, wantOut: heredoc.Doc(` github.com - ✓ Logged in to github.com as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for github.com configured to use https protocol. - ✓ Token: xyz456 - ✓ Token scopes: repo,read:org - - joel.miller - ✓ Logged in to joel.miller as tess (GH_CONFIG_DIR/hosts.yml) - ✓ Git operations for joel.miller configured to use https protocol. - ✓ Token: abc123 - ✓ Token scopes: repo,read:org + ✓ Logged in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_abc123 + - Token scopes: 'repo', 'read:org' + + ghe.io + ✓ Logged in to ghe.io account monalisa-ghe (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_xyz456 + - Token scopes: 'repo', 'read:org' `), }, { name: "missing hostname", - opts: &StatusOptions{ + opts: StatusOptions{ Hostname: "github.example.com", }, - cfgStubs: func(c *config.ConfigMock) { - c.Set("github.com", "oauth_token", "abc123") + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "abc123", "https") }, httpStubs: func(reg *httpmock.Registry) {}, - wantErr: "SilentError", - wantErrOut: "Hostname \"github.example.com\" not found among authenticated GitHub hosts\n", + wantErr: cmdutil.SilentError, + wantErrOut: "You are not logged into any accounts on github.example.com\n", + }, + { + name: "multiple accounts on a host", + opts: StatusOptions{}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "gho_abc123", "https") + login(t, c, "github.com", "monalisa-2", "gho_abc123", "https") + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org,project:read")) + }, + wantOut: heredoc.Doc(` + github.com + ✓ Logged in to github.com account monalisa-2 (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo', 'read:org' + + ✓ Logged in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: false + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo', 'read:org', 'project:read' + `), + }, + { + name: "multiple hosts with multiple accounts with environment tokens and with errors", + opts: StatusOptions{}, + env: map[string]string{"GH_ENTERPRISE_TOKEN": "gho_abc123"}, + cfgStubs: func(t *testing.T, c config.Config) { + login(t, c, "github.com", "monalisa", "gho_def456", "https") + login(t, c, "github.com", "monalisa-2", "gho_ghi789", "https") + login(t, c, "ghe.io", "monalisa-ghe", "gho_xyz123", "ssh") + }, + httpStubs: func(reg *httpmock.Registry) { + // Get scopes for monalia-2 + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + // Get scopes for monalisa + reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo")) + // Get scopes for monalisa-ghe-2 + reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo,read:org")) + // Error getting scopes for monalisa-ghe + reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.StatusStringResponse(404, "{}")) + // Get username for monalisa-ghe-2 + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa-ghe-2"}}}`)) + }, + wantOut: heredoc.Doc(` + github.com + ✓ Logged in to github.com account monalisa-2 (GH_CONFIG_DIR/hosts.yml) + - Active account: true + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo', 'read:org' + + ✓ Logged in to github.com account monalisa (GH_CONFIG_DIR/hosts.yml) + - Active account: false + - Git operations protocol: https + - Token: gho_****** + - Token scopes: 'repo' + ! Missing required token scopes: 'read:org' + - To request missing scopes, run: gh auth refresh -h github.com + + ghe.io + ✓ Logged in to ghe.io account monalisa-ghe-2 (GH_ENTERPRISE_TOKEN) + - Active account: true + - Git operations protocol: ssh + - Token: gho_****** + - Token scopes: 'repo', 'read:org' + + X Failed to log in to ghe.io account monalisa-ghe (GH_CONFIG_DIR/hosts.yml) + - Active account: false + - The token in GH_CONFIG_DIR/hosts.yml is invalid. + - To re-authenticate, run: gh auth login -h ghe.io + - To forget about this account, run: gh auth logout -h ghe.io -u monalisa-ghe + `), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.opts == nil { - tt.opts = &StatusOptions{} - } - ios, _, stdout, stderr := iostreams.Test() ios.SetStdinTTY(true) ios.SetStderrTTY(true) ios.SetStdoutTTY(true) tt.opts.IO = ios - cfg := config.NewFromString("") + cfg, _ := config.NewIsolatedTestConfig(t) if tt.cfgStubs != nil { - tt.cfgStubs(cfg) + tt.cfgStubs(t, cfg) } tt.opts.Config = func() (config.Config, error) { return cfg, nil @@ -355,24 +411,27 @@ func Test_statusRun(t *testing.T) { tt.httpStubs(reg) } - err := statusRun(tt.opts) - if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) + for k, v := range tt.env { + t.Setenv(k, v) + } + + err := statusRun(&tt.opts) + if tt.wantErr != nil { + require.Equal(t, err, tt.wantErr) } else { - assert.NoError(t, err) + require.NoError(t, err) } output := strings.ReplaceAll(stdout.String(), config.ConfigDir()+string(filepath.Separator), "GH_CONFIG_DIR/") errorOutput := strings.ReplaceAll(stderr.String(), config.ConfigDir()+string(filepath.Separator), "GH_CONFIG_DIR/") - assert.Equal(t, tt.wantErrOut, errorOutput) - assert.Equal(t, tt.wantOut, output) - - mainBuf := bytes.Buffer{} - hostsBuf := bytes.Buffer{} - readConfigs(&mainBuf, &hostsBuf) - - assert.Equal(t, "", mainBuf.String()) - assert.Equal(t, "", hostsBuf.String()) + require.Equal(t, tt.wantErrOut, errorOutput) + require.Equal(t, tt.wantOut, output) }) } } + +func login(t *testing.T, c config.Config, hostname, username, protocol, token string) { + t.Helper() + _, err := c.Authentication().Login(hostname, username, protocol, token, false) + require.NoError(t, err) +} diff --git a/pkg/cmd/auth/switch/switch.go b/pkg/cmd/auth/switch/switch.go new file mode 100644 index 000000000..76abbe705 --- /dev/null +++ b/pkg/cmd/auth/switch/switch.go @@ -0,0 +1,171 @@ +package authswitch + +import ( + "errors" + "fmt" + "slices" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/pkg/cmd/auth/shared" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +type SwitchOptions struct { + IO *iostreams.IOStreams + Config func() (config.Config, error) + Prompter shared.Prompt + Hostname string + Username string +} + +func NewCmdSwitch(f *cmdutil.Factory, runF func(*SwitchOptions) error) *cobra.Command { + opts := SwitchOptions{ + IO: f.IOStreams, + Config: f.Config, + Prompter: f.Prompter, + } + + cmd := &cobra.Command{ + Use: "switch", + Args: cobra.ExactArgs(0), + Short: "Switch active GitHub account", + Long: heredoc.Doc(` + Switch the active account for a GitHub host. + + This command changes the authentication configuration that will + be used when running commands targeting the specified GitHub host. + `), + Example: heredoc.Doc(` + # Select what host and account to switch to via a prompt + $ gh auth switch + + # Switch to a specific host and specific account + $ gh auth logout --hostname enterprise.internal --user monalisa + `), + RunE: func(c *cobra.Command, args []string) error { + if runF != nil { + return runF(&opts) + } + + return switchRun(&opts) + }, + } + + cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to switch account for") + cmd.Flags().StringVarP(&opts.Username, "user", "u", "", "The account to switch to") + + return cmd +} + +type hostUser struct { + host string + user string + active bool +} + +type candidates []hostUser + +func switchRun(opts *SwitchOptions) error { + hostname := opts.Hostname + username := opts.Username + + cfg, err := opts.Config() + if err != nil { + return err + } + authCfg := cfg.Authentication() + + knownHosts := authCfg.Hosts() + if len(knownHosts) == 0 { + return fmt.Errorf("not logged in to any hosts") + } + + if hostname != "" { + if !slices.Contains(knownHosts, hostname) { + return fmt.Errorf("not logged in to %s", hostname) + } + + if username != "" { + knownUsers := cfg.Authentication().UsersForHost(hostname) + if !slices.Contains(knownUsers, username) { + return fmt.Errorf("not logged in to %s account %s", hostname, username) + } + } + } + + var candidates candidates + + for _, host := range knownHosts { + if hostname != "" && host != hostname { + continue + } + hostActiveUser, err := authCfg.ActiveUser(host) + if err != nil { + return err + } + knownUsers := cfg.Authentication().UsersForHost(host) + for _, user := range knownUsers { + if username != "" && user != username { + continue + } + candidates = append(candidates, hostUser{host: host, user: user, active: user == hostActiveUser}) + } + } + + if len(candidates) == 0 { + return errors.New("no accounts matched that criteria") + } else if len(candidates) == 1 { + hostname = candidates[0].host + username = candidates[0].user + } else if len(candidates) == 2 && + candidates[0].host == candidates[1].host { + // If there is a single host with two users, automatically swith to the + // inactive user without prompting. + hostname = candidates[0].host + username = candidates[0].user + if candidates[0].active { + username = candidates[1].user + } + } else if !opts.IO.CanPrompt() { + return errors.New("unable to determine which account to switch to, please specify `--hostname` and `--user`") + } else { + prompts := make([]string, len(candidates)) + for i, c := range candidates { + prompt := fmt.Sprintf("%s (%s)", c.user, c.host) + if c.active { + prompt += " - active" + } + prompts[i] = prompt + } + selected, err := opts.Prompter.Select( + "What account do you want to switch to?", "", prompts) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + hostname = candidates[selected].host + username = candidates[selected].user + } + + if src, writeable := shared.AuthTokenWriteable(authCfg, hostname); !writeable { + fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", src) + fmt.Fprint(opts.IO.ErrOut, "To have GitHub CLI manage credentials instead, first clear the value from the environment.\n") + return cmdutil.SilentError + } + + cs := opts.IO.ColorScheme() + + if err := authCfg.SwitchUser(hostname, username); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Failed to switch account for %s to %s\n", + cs.FailureIcon(), hostname, cs.Bold(username)) + + return err + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Switched active account for %s to %s\n", + cs.SuccessIcon(), hostname, cs.Bold(username)) + + return nil +} diff --git a/pkg/cmd/auth/switch/switch_test.go b/pkg/cmd/auth/switch/switch_test.go new file mode 100644 index 000000000..efb69a56b --- /dev/null +++ b/pkg/cmd/auth/switch/switch_test.go @@ -0,0 +1,439 @@ +package authswitch + +import ( + "bytes" + "errors" + "fmt" + "io" + "testing" + + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/keyring" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/require" +) + +func TestNewCmdSwitch(t *testing.T) { + tests := []struct { + name string + input string + expectedOpts SwitchOptions + expectedErrMsg string + }{ + { + name: "no flags", + input: "", + expectedOpts: SwitchOptions{}, + }, + { + name: "hostname flag", + input: "--hostname github.com", + expectedOpts: SwitchOptions{ + Hostname: "github.com", + }, + }, + { + name: "user flag", + input: "--user monalisa", + expectedOpts: SwitchOptions{ + Username: "monalisa", + }, + }, + { + name: "positional args is an error", + input: "some-positional-arg", + expectedErrMsg: "accepts 0 arg(s), received 1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &cmdutil.Factory{} + argv, err := shlex.Split(tt.input) + require.NoError(t, err) + + var gotOpts *SwitchOptions + cmd := NewCmdSwitch(f, func(opts *SwitchOptions) error { + gotOpts = opts + return nil + }) + // Override the help flag as happens in production to allow -h flag + // to be used for hostname. + cmd.Flags().BoolP("help", "x", false, "") + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.expectedErrMsg != "" { + require.ErrorContains(t, err, tt.expectedErrMsg) + return + } + + require.NoError(t, err) + require.Equal(t, &tt.expectedOpts, gotOpts) + }) + } + +} + +func TestSwitchRun(t *testing.T) { + type user struct { + name string + token string + } + + type hostUsers struct { + host string + users []user + } + + type successfulExpectation struct { + switchedHost string + activeUser string + activeToken string + hostsCfg string + stderr string + } + + type failedExpectation struct { + err error + stderr string + } + + userWithMissingToken := "user-that-is-broken-by-the-test" + + tests := []struct { + name string + opts SwitchOptions + cfgHosts []hostUsers + env map[string]string + + expectedSuccess successfulExpectation + expectedFailure failedExpectation + + prompterStubs func(*prompter.PrompterMock) + }{ + { + name: "given one host with two users, switches to the other user", + opts: SwitchOptions{}, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedSuccess: successfulExpectation{ + switchedHost: "github.com", + activeUser: "inactive-user", + activeToken: "inactive-user-token", + hostsCfg: "github.com:\n git_protocol: ssh\n users:\n inactive-user:\n active-user:\n user: inactive-user\n", + stderr: "✓ Switched active account for github.com to inactive-user", + }, + }, + { + name: "given one host, with three users, switches to the specified user", + opts: SwitchOptions{ + Username: "inactive-user-2", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user-1", "inactive-user-1-token"}, + {"inactive-user-2", "inactive-user-2-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedSuccess: successfulExpectation{ + switchedHost: "github.com", + activeUser: "inactive-user-2", + activeToken: "inactive-user-2-token", + hostsCfg: "github.com:\n git_protocol: ssh\n users:\n inactive-user-1:\n inactive-user-2:\n active-user:\n user: inactive-user-2\n", + stderr: "✓ Switched active account for github.com to inactive-user-2", + }, + }, + { + name: "given multiple hosts, with multiple users, switches to the specific user on the host", + opts: SwitchOptions{ + Hostname: "ghe.io", + Username: "inactive-user", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + {"ghe.io", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedSuccess: successfulExpectation{ + switchedHost: "ghe.io", + activeUser: "inactive-user", + activeToken: "inactive-user-token", + hostsCfg: "github.com:\n git_protocol: ssh\n users:\n inactive-user:\n active-user:\n user: active-user\nghe.io:\n git_protocol: ssh\n users:\n inactive-user:\n active-user:\n user: inactive-user\n", + stderr: "✓ Switched active account for ghe.io to inactive-user", + }, + }, + { + name: "given we're not logged into any hosts, provide an informative error", + opts: SwitchOptions{}, + cfgHosts: []hostUsers{}, + expectedFailure: failedExpectation{ + err: errors.New("not logged in to any hosts"), + }, + }, + { + name: "given we can't disambiguate users across hosts", + opts: SwitchOptions{ + Username: "inactive-user", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + {"ghe.io", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedFailure: failedExpectation{ + err: errors.New("unable to determine which account to switch to, please specify `--hostname` and `--user`"), + }, + }, + { + name: "given we can't disambiguate user on a single host", + opts: SwitchOptions{ + Hostname: "github.com", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user-1", "inactive-user-1-token"}, + {"inactive-user-2", "inactive-user-2-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedFailure: failedExpectation{ + err: errors.New("unable to determine which account to switch to, please specify `--hostname` and `--user`"), + }, + }, + { + name: "given the auth token isn't writeable (e.g. a token env var is set)", + opts: SwitchOptions{}, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + env: map[string]string{"GH_TOKEN": "unimportant-test-value"}, + expectedFailure: failedExpectation{ + err: cmdutil.SilentError, + stderr: "The value of the GH_TOKEN environment variable is being used for authentication.", + }, + }, + { + name: "specified hostname doesn't exist", + opts: SwitchOptions{ + Hostname: "ghe.io", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedFailure: failedExpectation{ + err: errors.New("not logged in to ghe.io"), + }, + }, + { + name: "specified user doesn't exist on host", + opts: SwitchOptions{ + Hostname: "github.com", + Username: "non-existent-user", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedFailure: failedExpectation{ + err: errors.New("not logged in to github.com account non-existent-user"), + }, + }, + { + name: "specified user doesn't exist on any host", + opts: SwitchOptions{ + Username: "non-existent-user", + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"active-user", "active-user-token"}, + }}, + {"ghe.io", []user{ + {"active-user", "active-user-token"}, + }}, + }, + expectedFailure: failedExpectation{ + err: errors.New("no accounts matched that criteria"), + }, + }, + { + name: "when options need to be disambiguated, the user is prompted with matrix of options including active users (if possible)", + opts: SwitchOptions{}, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + {"ghe.io", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { + require.Equal(t, "What account do you want to switch to?", prompt) + require.Equal(t, []string{ + "inactive-user (github.com)", + "active-user (github.com) - active", + "inactive-user (ghe.io)", + "active-user (ghe.io) - active", + }, opts) + + return prompter.IndexFor(opts, "inactive-user (ghe.io)") + } + }, + expectedSuccess: successfulExpectation{ + switchedHost: "ghe.io", + activeUser: "inactive-user", + activeToken: "inactive-user-token", + hostsCfg: "github.com:\n git_protocol: ssh\n users:\n inactive-user:\n active-user:\n user: active-user\nghe.io:\n git_protocol: ssh\n users:\n inactive-user:\n active-user:\n user: inactive-user\n", + stderr: "✓ Switched active account for ghe.io to inactive-user", + }, + }, + { + name: "options need to be disambiguated given two hosts, one with two users", + opts: SwitchOptions{}, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {"inactive-user", "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + {"ghe.io", []user{ + {"active-user", "active-user-token"}, + }}, + }, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { + require.Equal(t, "What account do you want to switch to?", prompt) + require.Equal(t, []string{ + "inactive-user (github.com)", + "active-user (github.com) - active", + "active-user (ghe.io) - active", + }, opts) + + return prompter.IndexFor(opts, "inactive-user (github.com)") + } + }, + expectedSuccess: successfulExpectation{ + switchedHost: "github.com", + activeUser: "inactive-user", + activeToken: "inactive-user-token", + hostsCfg: "github.com:\n git_protocol: ssh\n users:\n inactive-user:\n active-user:\n user: inactive-user\nghe.io:\n git_protocol: ssh\n users:\n active-user:\n user: active-user\n", + stderr: "✓ Switched active account for github.com to inactive-user", + }, + }, + { + name: "when switching fails due to something other than user error, an informative message is printed to explain their new state", + opts: SwitchOptions{ + Username: userWithMissingToken, + }, + cfgHosts: []hostUsers{ + {"github.com", []user{ + {userWithMissingToken, "inactive-user-token"}, + {"active-user", "active-user-token"}, + }}, + }, + expectedFailure: failedExpectation{ + err: fmt.Errorf("no token found for %s", userWithMissingToken), + stderr: fmt.Sprintf("X Failed to switch account for github.com to %s", userWithMissingToken), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg, readConfigs := config.NewIsolatedTestConfig(t) + + for k, v := range tt.env { + t.Setenv(k, v) + } + + isInteractive := tt.prompterStubs != nil + if isInteractive { + pm := &prompter.PrompterMock{} + tt.prompterStubs(pm) + tt.opts.Prompter = pm + defer func() { + require.Len(t, pm.SelectCalls(), 1) + }() + } + + for _, hostUsers := range tt.cfgHosts { + for _, user := range hostUsers.users { + _, err := cfg.Authentication().Login( + hostUsers.host, + user.name, + user.token, "ssh", true, + ) + require.NoError(t, err) + + if user.name == userWithMissingToken { + require.NoError(t, keyring.Delete(fmt.Sprintf("gh:%s", hostUsers.host), userWithMissingToken)) + } + } + } + + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + + ios, _, _, stderr := iostreams.Test() + ios.SetStdinTTY(isInteractive) + ios.SetStdoutTTY(isInteractive) + tt.opts.IO = ios + + err := switchRun(&tt.opts) + if tt.expectedFailure.err != nil { + require.Equal(t, tt.expectedFailure.err, err) + require.Contains(t, stderr.String(), tt.expectedFailure.stderr) + return + } + + require.NoError(t, err) + + activeUser, err := cfg.Authentication().ActiveUser(tt.expectedSuccess.switchedHost) + require.NoError(t, err) + require.Equal(t, tt.expectedSuccess.activeUser, activeUser) + + activeToken, _ := cfg.Authentication().TokenFromKeyring(tt.expectedSuccess.switchedHost) + require.Equal(t, tt.expectedSuccess.activeToken, activeToken) + + hostsBuf := bytes.Buffer{} + readConfigs(io.Discard, &hostsBuf) + + require.Equal(t, tt.expectedSuccess.hostsCfg, hostsBuf.String()) + + require.Contains(t, stderr.String(), tt.expectedSuccess.stderr) + }) + } +} diff --git a/pkg/cmd/auth/token/token.go b/pkg/cmd/auth/token/token.go index fee8dc638..c507645ce 100644 --- a/pkg/cmd/auth/token/token.go +++ b/pkg/cmd/auth/token/token.go @@ -3,6 +3,7 @@ package token import ( "fmt" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -14,6 +15,7 @@ type TokenOptions struct { Config func() (config.Config, error) Hostname string + Username string SecureStorage bool } @@ -25,8 +27,15 @@ func NewCmdToken(f *cmdutil.Factory, runF func(*TokenOptions) error) *cobra.Comm cmd := &cobra.Command{ Use: "token", - Short: "Print the auth token gh is configured to use", - Args: cobra.ExactArgs(0), + Short: "Print the authentication token gh uses for a hostname and account", + Long: heredoc.Docf(` + This command outputs the authentication token for an account on a given GitHub host. + + Without the %[1]s--hostname%[1]s flag, the default host is chosen. + + Without the %[1]s--user%[1]s flag, the active account for the host is chosen. + `, "`"), + Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, args []string) error { if runF != nil { return runF(opts) @@ -37,6 +46,7 @@ func NewCmdToken(f *cmdutil.Factory, runF func(*TokenOptions) error) *cobra.Comm } cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance authenticated with") + cmd.Flags().StringVarP(&opts.Username, "user", "u", "", "The account to log out of") cmd.Flags().BoolVarP(&opts.SecureStorage, "secure-storage", "", false, "Search only secure credential store for authentication token") _ = cmd.Flags().MarkHidden("secure-storage") @@ -56,17 +66,34 @@ func tokenRun(opts *TokenOptions) error { } var val string + // If this conditional logic ends up being duplicated anywhere, + // we should consider making a factory function that returns the correct + // behavior. For now, keeping it all inline is simplest. if opts.SecureStorage { - val, _ = authCfg.TokenFromKeyring(hostname) + if opts.Username == "" { + val, _ = authCfg.TokenFromKeyring(hostname) + } else { + val, _ = authCfg.TokenFromKeyringForUser(hostname, opts.Username) + } } else { - val, _ = authCfg.Token(hostname) + if opts.Username == "" { + val, _ = authCfg.ActiveToken(hostname) + } else { + val, _, _ = authCfg.TokenForUser(hostname, opts.Username) + } } + if val == "" { - return fmt.Errorf("no oauth token") + errMsg := fmt.Sprintf("no oauth token found for %s", hostname) + if opts.Username != "" { + errMsg += fmt.Sprintf(" account %s", opts.Username) + } + return fmt.Errorf(errMsg) } if val != "" { fmt.Fprintf(opts.IO.Out, "%s\n", val) } + return nil } diff --git a/pkg/cmd/auth/token/token_test.go b/pkg/cmd/auth/token/token_test.go index 03324c0b2..6e9e246af 100644 --- a/pkg/cmd/auth/token/token_test.go +++ b/pkg/cmd/auth/token/token_test.go @@ -8,8 +8,7 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" - "github.com/stretchr/testify/assert" - "github.com/zalando/go-keyring" + "github.com/stretchr/testify/require" ) func TestNewCmdToken(t *testing.T) { @@ -30,6 +29,16 @@ func TestNewCmdToken(t *testing.T) { input: "--hostname github.mycompany.com", output: TokenOptions{Hostname: "github.mycompany.com"}, }, + { + name: "with user", + input: "--user test-user", + output: TokenOptions{Username: "test-user"}, + }, + { + name: "with shorthand user", + input: "-u test-user", + output: TokenOptions{Username: "test-user"}, + }, { name: "with shorthand hostname", input: "-h github.mycompany.com", @@ -53,7 +62,7 @@ func TestNewCmdToken(t *testing.T) { }, } argv, err := shlex.Split(tt.input) - assert.NoError(t, err) + require.NoError(t, err) var cmdOpts *TokenOptions cmd := NewCmdToken(f, func(opts *TokenOptions) error { @@ -70,14 +79,14 @@ func TestNewCmdToken(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantErr { - assert.Error(t, err) - assert.EqualError(t, err, tt.wantErrMsg) + require.Error(t, err) + require.EqualError(t, err, tt.wantErrMsg) return } - assert.NoError(t, err) - assert.Equal(t, tt.output.Hostname, cmdOpts.Hostname) - assert.Equal(t, tt.output.SecureStorage, cmdOpts.SecureStorage) + require.NoError(t, err) + require.Equal(t, tt.output.Hostname, cmdOpts.Hostname) + require.Equal(t, tt.output.SecureStorage, cmdOpts.SecureStorage) }) } } @@ -86,75 +95,96 @@ func TestTokenRun(t *testing.T) { tests := []struct { name string opts TokenOptions + env map[string]string + cfgStubs func(*testing.T, config.Config) wantStdout string wantErr bool wantErrMsg string }{ { name: "token", - opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - cfg.Set("github.com", "oauth_token", "gho_ABCDEFG") - return cfg, nil - }, + opts: TokenOptions{}, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) }, wantStdout: "gho_ABCDEFG\n", }, { name: "token by hostname", opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - cfg.Set("github.com", "oauth_token", "gho_ABCDEFG") - cfg.Set("github.mycompany.com", "oauth_token", "gho_1234567") - return cfg, nil - }, Hostname: "github.mycompany.com", }, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) + login(t, cfg, "github.mycompany.com", "test-user", "gho_1234567", "https", false) + }, wantStdout: "gho_1234567\n", }, { - name: "no token", + name: "no token", + opts: TokenOptions{}, + wantErr: true, + wantErrMsg: "no oauth token found for github.com", + }, + { + name: "no token for hostname user", opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - return cfg, nil - }, + Hostname: "ghe.io", + Username: "test-user", }, wantErr: true, - wantErrMsg: "no oauth token", + wantErrMsg: "no oauth token found for ghe.io account test-user", }, { name: "uses default host when one is not provided", - opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - cfg.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetDefaultHost("github.mycompany.com", "GH_HOST") - authCfg.SetToken("gho_1234567", "default") - return authCfg - } - return cfg, nil - }, + opts: TokenOptions{}, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) + login(t, cfg, "github.mycompany.com", "test-user", "gho_1234567", "https", false) }, + env: map[string]string{"GH_HOST": "github.mycompany.com"}, wantStdout: "gho_1234567\n", }, + { + name: "token for user", + opts: TokenOptions{ + Hostname: "github.com", + Username: "test-user", + }, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", false) + login(t, cfg, "github.com", "test-user-2", "gho_1234567", "https", false) + }, + wantStdout: "gho_ABCDEFG\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ios, _, stdout, _ := iostreams.Test() tt.opts.IO = ios + + for k, v := range tt.env { + t.Setenv(k, v) + } + + cfg, _ := config.NewIsolatedTestConfig(t) + if tt.cfgStubs != nil { + tt.cfgStubs(t, cfg) + } + + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + err := tokenRun(&tt.opts) if tt.wantErr { - assert.Error(t, err) - assert.EqualError(t, err, tt.wantErrMsg) + require.Error(t, err) + require.EqualError(t, err, tt.wantErrMsg) return } - assert.NoError(t, err) - assert.Equal(t, tt.wantStdout, stdout.String()) + require.NoError(t, err) + require.Equal(t, tt.wantStdout, stdout.String()) }) } } @@ -163,60 +193,87 @@ func TestTokenRunSecureStorage(t *testing.T) { tests := []struct { name string opts TokenOptions + cfgStubs func(*testing.T, config.Config) wantStdout string wantErr bool wantErrMsg string }{ { name: "token", - opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - _ = keyring.Set("gh:github.com", "", "gho_ABCDEFG") - return cfg, nil - }, + opts: TokenOptions{}, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", true) }, wantStdout: "gho_ABCDEFG\n", }, { name: "token by hostname", opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - _ = keyring.Set("gh:mycompany.com", "", "gho_1234567") - return cfg, nil - }, Hostname: "mycompany.com", }, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "mycompany.com", "test-user", "gho_1234567", "https", true) + }, wantStdout: "gho_1234567\n", }, { - name: "no token", + name: "no token", + opts: TokenOptions{}, + wantErr: true, + wantErrMsg: "no oauth token found for github.com", + }, + { + name: "no token for hostname user", opts: TokenOptions{ - Config: func() (config.Config, error) { - cfg := config.NewBlankConfig() - return cfg, nil - }, + Hostname: "ghe.io", + Username: "test-user", }, wantErr: true, - wantErrMsg: "no oauth token", + wantErrMsg: "no oauth token found for ghe.io account test-user", + }, + { + name: "token for user", + opts: TokenOptions{ + Hostname: "github.com", + Username: "test-user", + }, + cfgStubs: func(t *testing.T, cfg config.Config) { + login(t, cfg, "github.com", "test-user", "gho_ABCDEFG", "https", true) + login(t, cfg, "github.com", "test-user-2", "gho_1234567", "https", true) + }, + wantStdout: "gho_ABCDEFG\n", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - keyring.MockInit() ios, _, stdout, _ := iostreams.Test() tt.opts.IO = ios tt.opts.SecureStorage = true + + cfg, _ := config.NewIsolatedTestConfig(t) + if tt.cfgStubs != nil { + tt.cfgStubs(t, cfg) + } + + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + err := tokenRun(&tt.opts) if tt.wantErr { - assert.Error(t, err) - assert.EqualError(t, err, tt.wantErrMsg) + require.Error(t, err) + require.EqualError(t, err, tt.wantErrMsg) return } - assert.NoError(t, err) - assert.Equal(t, tt.wantStdout, stdout.String()) + require.NoError(t, err) + require.Equal(t, tt.wantStdout, stdout.String()) }) } } + +func login(t *testing.T, c config.Config, hostname, username, token, gitProtocol string, secureStorage bool) { + t.Helper() + _, err := c.Authentication().Login(hostname, username, token, "https", secureStorage) + require.NoError(t, err) +} diff --git a/pkg/cmd/config/get/get.go b/pkg/cmd/config/get/get.go index f391decc4..b65cf6bd3 100644 --- a/pkg/cmd/config/get/get.go +++ b/pkg/cmd/config/get/get.go @@ -56,7 +56,7 @@ func NewCmdConfigGet(f *cmdutil.Factory, runF func(*GetOptions) error) *cobra.Co func getRun(opts *GetOptions) error { // search keyring storage when fetching the `oauth_token` value if opts.Hostname != "" && opts.Key == "oauth_token" { - token, _ := opts.Config.Authentication().Token(opts.Hostname) + token, _ := opts.Config.Authentication().ActiveToken(opts.Hostname) if token == "" { return errors.New(`could not find key "oauth_token"`) } diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index b58544b44..efd2f7793 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -78,7 +78,7 @@ func Test_BaseRepo(t *testing.T) { hosts = append([]string{tt.override}, hosts...) } authCfg.SetHosts(hosts) - authCfg.SetToken("", "") + authCfg.SetActiveToken("", "") authCfg.SetDefaultHost("nonsense.com", "hosts") if tt.override != "" { authCfg.SetDefaultHost(tt.override, "GH_HOST") @@ -216,7 +216,7 @@ func Test_SmartBaseRepo(t *testing.T) { hosts = append([]string{tt.override}, hosts...) } authCfg.SetHosts(hosts) - authCfg.SetToken("", "") + authCfg.SetActiveToken("", "") authCfg.SetDefaultHost("nonsense.com", "hosts") if tt.override != "" { authCfg.SetDefaultHost(tt.override, "GH_HOST") diff --git a/pkg/cmd/factory/remote_resolver_test.go b/pkg/cmd/factory/remote_resolver_test.go index a38470595..0b4447ca0 100644 --- a/pkg/cmd/factory/remote_resolver_test.go +++ b/pkg/cmd/factory/remote_resolver_test.go @@ -71,7 +71,7 @@ func Test_remoteResolver(t *testing.T) { cfg.AuthenticationFunc = func() *config.AuthConfig { authCfg := &config.AuthConfig{} authCfg.SetHosts([]string{"example.com"}) - authCfg.SetToken("", "") + authCfg.SetActiveToken("", "") authCfg.SetDefaultHost("example.com", "hosts") return authCfg } @@ -151,7 +151,7 @@ func Test_remoteResolver(t *testing.T) { cfg.AuthenticationFunc = func() *config.AuthConfig { authCfg := &config.AuthConfig{} authCfg.SetHosts([]string{"example.com", "github.com"}) - authCfg.SetToken("", "") + authCfg.SetActiveToken("", "") authCfg.SetDefaultHost("example.com", "default") return authCfg } diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index c0aa32bbd..7f37c0ae9 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -211,7 +211,7 @@ func TestRepoFork(t *testing.T) { httpStubs func(*httpmock.Registry) execStubs func(*run.CommandStubber) promptStubs func(*prompter.MockPrompter) - cfgStubs func(*config.ConfigMock) + cfgStubs func(*testing.T, config.Config) remotes []*context.Remote wantOut string wantErrOut string @@ -254,7 +254,7 @@ func TestRepoFork(t *testing.T) { Repo: ghrepo.New("OWNER", "REPO"), }, }, - cfgStubs: func(c *config.ConfigMock) { + cfgStubs: func(_ *testing.T, c config.Config) { c.Set("", "git_protocol", "") }, httpStubs: forkPost, @@ -731,9 +731,9 @@ func TestRepoFork(t *testing.T) { return &http.Client{Transport: reg}, nil } - cfg := config.NewBlankConfig() + cfg, _ := config.NewIsolatedTestConfig(t) if tt.cfgStubs != nil { - tt.cfgStubs(cfg) + tt.cfgStubs(t, cfg) } tt.opts.Config = func() (config.Config, error) { return cfg, nil diff --git a/pkg/cmdutil/auth_check_test.go b/pkg/cmdutil/auth_check_test.go index fa8bd80e2..25cbae567 100644 --- a/pkg/cmdutil/auth_check_test.go +++ b/pkg/cmdutil/auth_check_test.go @@ -4,53 +4,52 @@ import ( "testing" "github.com/cli/cli/v2/internal/config" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_CheckAuth(t *testing.T) { tests := []struct { name string - cfgStubs func(*config.ConfigMock) + env map[string]string + cfgStubs func(*testing.T, config.Config) expected bool }{ { name: "no known hosts, no env auth token", - cfgStubs: func(c *config.ConfigMock) {}, expected: false, }, { - name: "no known hosts, env auth token", - cfgStubs: func(c *config.ConfigMock) { - c.AuthenticationFunc = func() *config.AuthConfig { - authCfg := &config.AuthConfig{} - authCfg.SetToken("token", "GITHUB_TOKEN") - return authCfg - } - }, + name: "no known hosts, env auth token", + env: map[string]string{"GITHUB_TOKEN": "token"}, expected: true, }, { name: "known host", - cfgStubs: func(c *config.ConfigMock) { - c.Set("github.com", "oauth_token", "token") + cfgStubs: func(t *testing.T, c config.Config) { + _, err := c.Authentication().Login("github.com", "test-user", "test-token", "https", false) + require.NoError(t, err) }, expected: true, }, { - name: "enterprise token", - cfgStubs: func(c *config.ConfigMock) { - t.Setenv("GH_ENTERPRISE_TOKEN", "token") - }, + name: "enterprise token", + env: map[string]string{"GH_ENTERPRISE_TOKEN": "token"}, expected: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfg := config.NewBlankConfig() - tt.cfgStubs(cfg) - result := CheckAuth(cfg) - assert.Equal(t, tt.expected, result) + cfg, _ := config.NewIsolatedTestConfig(t) + if tt.cfgStubs != nil { + tt.cfgStubs(t, cfg) + } + + for k, v := range tt.env { + t.Setenv(k, v) + } + + require.Equal(t, tt.expected, CheckAuth(cfg)) }) } }