fix(auth): make logout reject only the credential tea stored

Signed-off-by: bircni <bircni@icloud.com>

internal/app/auth.go

@@ -709,7 +709,7 @@ instance, such as a copy of its token that a keychain kept after a push.`,
 			if err != nil {
 				return err
 			}
-			account, exists := t.host.Users[t.User]
+			_, exists := t.host.Users[t.User]
 			switch {
 			case len(t.host.Users) == 0:
 				return clierr.Wrap(clierr.Usage, fmt.Errorf("not logged in to %s", t.Host))
@@ -725,15 +725,15 @@ instance, such as a copy of its token that a keychain kept after a push.`,
 			if err != nil {
 				return err
 			}
-			if err := manager.Logout(t.Host, t.User); err != nil {
+			removed, err := manager.Logout(t.Host, t.User)
+			if err != nil {
 				return err
 			}
 			a.done(cmd, "Logged out of %s (%s)", t.Host, t.User)
-			if !account.Tokenless() {
-				if err := git.RejectCredentials(cmd.Context(), a.gitProbe(), a.options.cwd, t.Host, t.User); err != nil {
+			// Helpers erase only the copies that hold the removed token, not the user's own credentials.
+			if err := git.RejectCredentials(cmd.Context(), a.gitProbe(), a.options.cwd, t.Host, t.User, removed.AccessToken); err != nil {
 				a.warn(cmd, "Could not ask git's credential helpers to forget %s on %s", t.User, t.Host)
 			}
-			}
 			if s, err := a.settings(); err == nil {
 				if active := s.Hosts.Hosts[t.Host].User; active != "" && active != t.host.User {
 					a.done(cmd, "Switched active account for %s to %s", t.Host, active)

internal/app/auth_test.go

@@ -777,7 +777,7 @@ func TestLogoutAsksGitToForgetTheAccount(t *testing.T) {
 	deps.Git = runner
 	configureAccounts(t, deps, instance, "alice", "alice", "bob")
 	require.NoError(t, Execute(context.Background(), deps, []string{"auth", "logout", "--yes"}))
-	require.Equal(t, []string{"credential reject\nprotocol=https\nhost=example.org:3000\nusername=alice\n\n"}, runner.inputs)
+	require.Equal(t, []string{"credential reject\nprotocol=https\nhost=example.org:3000\nusername=alice\npassword=alice-token\n\n"}, runner.inputs)
 	require.Equal(t, "✓ Logged out of "+instance+" (alice)\n✓ Switched active account for "+instance+" to bob\n", withoutANSI(diagnostics.String()))
 	err := Execute(context.Background(), deps, []string{"auth", "logout", "carol", "--yes"})
 	require.EqualError(t, err, `account "carol" is not logged in on `+instance+" (accounts: bob)")

internal/auth/manager.go

@@ -98,10 +98,11 @@ func (m Manager) register(instance, user string, account settings.Account, optio
 	})
 }
 
-// Logout removes one account and restores its credential if publication fails.
-// An unreadable credential does not prevent removing the account.
-func (m Manager) Logout(instance, user string) error {
-	return m.settings.Transaction(settings.TargetHosts, func(s *settings.Snapshot) (func() error, error) {
+// Logout removes one account, restores its credential if publication fails and returns the
+// removed credential. An unreadable credential does not prevent removing the account.
+func (m Manager) Logout(instance, user string) (Credential, error) {
+	var removed Credential
+	err := m.settings.Transaction(settings.TargetHosts, func(s *settings.Snapshot) (func() error, error) {
 		host := s.Hosts.Hosts[instance]
 		account, exists := host.Users[user]
 		if !exists {
@@ -111,6 +112,7 @@ func (m Manager) Logout(instance, user string) error {
 		if !account.Tokenless() {
 			credential, err := m.credentials.Load(instance, user, account.CredentialStore)
 			if err == nil {
+				removed = credential
 				// Preserve a compensating copy until the account file is successfully published.
 				restore = func() error { return m.credentials.Save(instance, user, account.CredentialStore, credential) }
 			}
@@ -132,4 +134,8 @@ func (m Manager) Logout(instance, user string) error {
 		s.Hosts.Hosts[instance] = host
 		return restore, nil
 	})
+	if err != nil {
+		return Credential{}, err
+	}
+	return removed, nil
 }

internal/auth/manager_test.go

@@ -70,7 +70,9 @@ func TestRegisterAndLogoutIsolateAccounts(t *testing.T) {
 	for _, user := range []string{"alice", "bob", "charlie"} {
 		require.NoError(t, manager.Register("https://example.org", user, "keyring", "token", HostOptions{}, Credential{AccessToken: "secret-" + user}))
 	}
-	require.NoError(t, manager.Logout("https://example.org", "charlie"))
+	removed, err := manager.Logout("https://example.org", "charlie")
+	require.NoError(t, err)
+	require.Equal(t, "secret-charlie", removed.AccessToken)
 	s, err := config.Load()
 	require.NoError(t, err)
 	require.Empty(t, s.Hosts.Hosts["https://example.org"].User)
@@ -154,14 +156,17 @@ func TestLogoutRemovesAccountWithMissingCredential(t *testing.T) {
 	manager := newTestManager(t, config, &Store{Dir: config.Dir, Keyring: keyring})
 	require.NoError(t, manager.Register("https://example.org", "alice", settings.StoreKeyring, settings.AuthToken, HostOptions{}, Credential{AccessToken: "secret"}))
 	clear(keyring.values)
-	require.NoError(t, manager.Logout("https://example.org", "alice"))
+	removed, err := manager.Logout("https://example.org", "alice")
+	require.NoError(t, err)
+	require.Empty(t, removed.AccessToken)
 	s, err := config.Load()
 	require.NoError(t, err)
 	require.Empty(t, s.Hosts.Hosts["https://example.org"].Users)
 	require.NoError(t, manager.Register("https://example.org", "alice", settings.StoreKeyring, settings.AuthToken, HostOptions{}, Credential{AccessToken: "secret"}))
 	// An unavailable keyring keeps the account rather than orphaning its token.
 	keyring.fail = true
-	require.Error(t, manager.Logout("https://example.org", "alice"))
+	_, err = manager.Logout("https://example.org", "alice")
+	require.Error(t, err)
 	s, err = config.Load()
 	require.NoError(t, err)
 	require.Contains(t, s.Hosts.Hosts["https://example.org"].Users, "alice")
@@ -175,7 +180,8 @@ func TestRegisterHTTPSignRejectsTokenAccounts(t *testing.T) {
 	s, err := config.Load()
 	require.NoError(t, err)
 	require.Equal(t, settings.Account{AuthMethod: settings.AuthSSH, SSHKey: "/keys/id"}, s.Hosts.Hosts["https://example.org"].Users["alice"])
-	require.NoError(t, manager.Logout("https://example.org", "alice"))
+	_, err = manager.Logout("https://example.org", "alice")
+	require.NoError(t, err)
 }
 
 func TestRegisterReplacesAnAccountOnlyWhenAsked(t *testing.T) {

internal/git/credentials.go

@@ -153,25 +153,23 @@ func (s System) Input(ctx context.Context, dir, input string, args ...string) (s
 	return s.Output(ctx, dir, true, args...)
 }
 
-// RejectCredentials asks Git's credential helpers to erase the credentials they stored for user on
-// instance, such as a copy of tea's token that another helper kept after a push. Runners that cannot
-// send input reject nothing.
-func RejectCredentials(ctx context.Context, runner Runner, dir, instance, user string) error {
+// RejectCredentials asks Git's credential helpers to erase the copies of token they stored for user on
+// instance, such as one that a keychain kept after a push. Helpers keep entries with another password,
+// such as the user's own. Runners that cannot send input reject nothing.
+func RejectCredentials(ctx context.Context, runner Runner, dir, instance, user, token string) error {
 	input, ok := runner.(InputRunner)
-	if !ok {
+	if !ok || user == "" || token == "" {
 		return nil
 	}
 	u, err := url.Parse(instance)
 	if err != nil || u.Host == "" {
 		return fmt.Errorf("invalid instance URL %q", instance)
 	}
-	if strings.ContainsAny(user, "\x00\r\n") {
-		return errors.New("invalid account name")
-	}
-	request := "protocol=" + u.Scheme + "\nhost=" + u.Host + "\n"
-	if user != "" {
-		request += "username=" + user + "\n"
+	if strings.ContainsAny(user+token, "\x00\r\n") {
+		return errors.New("invalid account name or token")
 	}
+	// No path: copies are stored per repository when useHttpPath is set, and a path must match exactly.
+	request := "protocol=" + u.Scheme + "\nhost=" + u.Host + "\nusername=" + user + "\npassword=" + token + "\n"
 	_, err = input.Input(ctx, dir, request+"\n", "credential", "reject")
 	return err
 }

internal/git/credentials_test.go

@@ -102,11 +102,23 @@ func TestRejectCredentialsAsksHelpersToEraseTheAccount(t *testing.T) {
 	runner := System{}
 	_, err := runner.Output(ctx, home, false, "config", "--global", "credential.helper", "!f() { echo \"op=$1\" >> '"+log+"'; cat >> '"+log+"'; }; f")
 	require.NoError(t, err)
+	store := filepath.Join(home, "git-credentials")
+	_, err = runner.Output(ctx, home, false, "config", "--global", "--add", "credential.helper", "store --file="+store)
+	require.NoError(t, err)
+	own := "http://alice:own-password@localhost:3001\n"
+	require.NoError(t, os.WriteFile(store, []byte(own+"http://alice:tea-token@localhost:3001/gitea/alice/project.git\n"), 0o600))
 	require.Empty(t, HelperScope(ctx, runner, home, "http://localhost:3001"))
-	require.NoError(t, RejectCredentials(ctx, runner, home, "http://localhost:3001/gitea", "alice"))
+	require.NoError(t, RejectCredentials(ctx, runner, home, "http://localhost:3001/gitea", "alice", "tea-token"))
 	data, err := os.ReadFile(log)
 	require.NoError(t, err)
-	require.Equal(t, "op=erase\nprotocol=http\nhost=localhost:3001\nusername=alice\n", string(data))
+	require.Equal(t, "op=erase\nprotocol=http\nhost=localhost:3001\nusername=alice\npassword=tea-token\n", string(data))
+	data, err = os.ReadFile(store)
+	require.NoError(t, err)
+	require.Equal(t, own, string(data), "only the copy of tea's token is erased")
+	require.NoError(t, RejectCredentials(ctx, runner, home, "http://localhost:3001", "alice", ""))
+	data, err = os.ReadFile(store)
+	require.NoError(t, err)
+	require.Equal(t, own, string(data), "without a token nothing is erased")
 	executable, err := os.Executable()
 	require.NoError(t, err)
 	require.NoError(t, SetupCredentials(ctx, runner, CredentialSetup{Directory: home, Instance: "http://localhost:3001", Executable: executable}))