feat: update accounts:set to work with keychain managers (#3696)

* feat: update accounts:set to allow for keychains

* test: update tests for accounts:set and set function

* test: update tests to work with Windows

* test: fix git credentials tests

* refactor: update logic to be more specific about keychain vs netrc accounts

* test: update accounts:set tests

* fix: revert changes to the accounts wrapper

Author: k80bowman

src/commands/accounts/set.ts

@@ -6,21 +6,27 @@ import AccountsModule from '../../lib/accounts/accounts.js'
 
 export default class Set extends Command {
   static args = {
-    name: Args.string({description: 'name of account to set', required: true}),
+    name: Args.string({description: 'name or username of account to set', required: true}),
   }
 
-  static description = 'set the current Heroku account from your cache'
+  static description = 'set the current Heroku account from your accounts cache or system keychain'
 
   static example = `${color.command('heroku accounts:set my-account')}`
 
   async run() {
     const {args} = await this.parse(Set)
     const {name} = args
 
-    if (!(await AccountsModule.list()).some(account => account.name === name)) {
-      ux.error(`${name} does not exist in your accounts cache.`)
+    const accounts = await AccountsModule.list()
+    const netrcAccount = accounts.find(account => account.name === name)
+    const keychainAccount = accounts.find(account => !account.name && account.username === name)
+
+    if (!netrcAccount && !keychainAccount) {
+      ux.error(`${name} does not exist in your accounts cache or system keychain.`)
     }
 
-    AccountsModule.set(name)
+    const account = netrcAccount ?? keychainAccount
+
+    await AccountsModule.set(account!, this.config.dataDir)
   }
 }

src/commands/git/credentials.ts

@@ -38,6 +38,7 @@ export class GitCredentials extends Command {
       })
 
       rl.on('close', () => {
+        process.stdin.pause()
         resolve(input)
       })
     })

src/lib/accounts/accounts.ts

@@ -1,4 +1,4 @@
-import {APIClient, listKeychainAccounts, getStorageConfig} from '@heroku-cli/command'
+import {APIClient, listKeychainAccounts, getStorageConfig, writeLoginState} from '@heroku-cli/command'
 import {removeAuth} from '@heroku-cli/command/lib/credential-manager.js'
 import * as Heroku from '@heroku-cli/schema'
 import fs from 'node:fs'
@@ -16,7 +16,9 @@ export interface IAccountsWrapper {
   current(heroku: APIClient): Promise<string | null>
   add(name: string, username: string, password: string): void
   remove(name: string): void
-  set(name: string): Promise<void>
+  set(account: AccountEntry, dataDir: string): Promise<void>
+  getStorageConfig(): ReturnType<typeof getStorageConfig>
+  writeLoginState(dataDir: string, name: string): Promise<void>
 }
 
 export class AccountsWrapper implements IAccountsWrapper {
@@ -65,6 +67,10 @@ export class AccountsWrapper implements IAccountsWrapper {
     return getStorageConfig()
   }
 
+  async writeLoginState(dataDir: string, name: string): Promise<void> {
+    return writeLoginState(dataDir, name)
+  }
+
   async list(): Promise<AccountEntry[]> {
     const config = this.getStorageConfig()
     if (config.credentialStore) {
@@ -130,12 +136,20 @@ export class AccountsWrapper implements IAccountsWrapper {
     fs.unlinkSync(path.join(basedir, name))
   }
 
-  async set(name: string): Promise<void> {
-    const netrcInstance = await this.initNetrc()
-    const current = this.account(name)
-    netrcInstance.machines['git.heroku.com'] = {login: current.username, password: current.password}
-    netrcInstance.machines['api.heroku.com'] = {login: current.username, password: current.password}
-    await netrcInstance.save()
+  async set(account: AccountEntry, dataDir: string): Promise<void> {
+    const config = this.getStorageConfig()
+    if (config.credentialStore && !account.name) {
+      await this.writeLoginState(dataDir, account.username)
+      return
+    }
+
+    if (config.useNetrc && account.name) {
+      const netrcInstance = await this.initNetrc()
+      const current = this.account(account.name)
+      netrcInstance.machines['git.heroku.com'] = {login: current.username, password: current.password}
+      netrcInstance.machines['api.heroku.com'] = {login: current.username, password: current.password}
+      await netrcInstance.save()
+    }
   }
 }
 

test/unit/commands/accounts/set.unit.test.ts

@@ -1,33 +1,49 @@
 import {expect} from 'chai'
-import runCommand from '../../../helpers/runCommand.js'
-import * as sinon from 'sinon'
+import {restore, SinonStub, stub} from 'sinon'
+
 import Cmd from '../../../../src/commands/accounts/set.js'
 import AccountsModule from '../../../../src/lib/accounts/accounts.js'
+import runCommand from '../../../helpers/runCommand.js'
 
 describe('accounts:set', function () {
-  let listStub: sinon.SinonStub
-  let setStub: sinon.SinonStub
+  let listStub: SinonStub
+  let setStub: SinonStub
 
   beforeEach(function () {
-    listStub = sinon.stub(AccountsModule, 'list')
-    setStub = sinon.stub(AccountsModule, 'set')
+    listStub = stub(AccountsModule, 'list')
+    setStub = stub(AccountsModule, 'set').resolves()
   })
 
   afterEach(function () {
-    sinon.restore()
+    restore()
   })
 
-  it('calls the set function with the account name when the account exists', async function () {
+  it('calls set with the account name and dataDir when matched by name', async function () {
     listStub.resolves([{name: 'test-account', username: 'user1'}, {name: 'test-account-2', username: 'user2'}])
     await runCommand(Cmd, ['test-account-2'])
-    expect(setStub.calledWith('test-account-2'))
+    expect(setStub.calledOnce).to.be.true
+    expect(setStub.firstCall.args[0]).to.deep.equal({name: 'test-account-2', username: 'user2'})
+    expect(setStub.firstCall.args[1].toLowerCase()).to.contain('local')
+    expect(setStub.firstCall.args[1]).to.contain('heroku')
   })
 
-  it('should return an error if the selected account name is not included in the account list', async function () {
+  it('calls set with the account name and dataDir when matched by username', async function () {
+    listStub.resolves([{username: 'user1@example.com'}, {username: 'user2@example.com'}])
+    await runCommand(Cmd, ['user1@example.com'])
+    expect(setStub.calledOnce).to.be.true
+    expect(setStub.firstCall.args[0]).to.deep.equal({username: 'user1@example.com'})
+    expect(setStub.firstCall.args[1].toLowerCase()).to.contain('local')
+    expect(setStub.firstCall.args[1]).to.contain('heroku')
+  })
+
+  it('returns an error if the account is not in the list', async function () {
     listStub.resolves([{name: 'test-account', username: 'user1'}, {name: 'test-account-2', username: 'user2'}])
-    await runCommand(Cmd, ['test-account-3'])
-      .catch((error: Error) => {
-        expect(error.message).to.contain('test-account-3 does not exist in your accounts cache.')
-      })
+
+    try {
+      await runCommand(Cmd, ['test-account-3'])
+      expect.fail('Expected command to throw error')
+    } catch (error: any) {
+      expect(error.message).to.contain('test-account-3 does not exist in your accounts cache or system keychain.')
+    }
   })
 })

test/unit/lib/accounts/accounts.unit.test.ts

@@ -163,6 +163,75 @@ describe('accounts', function () {
     })
   })
 
+  describe('set()', function () {
+    describe('with credentialStore and no account name', function () {
+      let writeLoginStateStub: sinon.SinonStub
+
+      beforeEach(function () {
+        sinon.stub(AccountsModule, 'getStorageConfig').returns({credentialStore: 'keychain' as any, useNetrc: false})
+        writeLoginStateStub = sinon.stub(AccountsModule, 'writeLoginState').resolves()
+      })
+
+      it('calls writeLoginState with the dataDir and account username', async function () {
+        const account = {username: 'user@example.com'}
+        await AccountsModule.set(account, '/data/heroku')
+
+        expect(writeLoginStateStub.calledOnce).to.be.true
+        expect(writeLoginStateStub.firstCall.args[0]).to.equal('/data/heroku')
+        expect(writeLoginStateStub.firstCall.args[1]).to.equal('user@example.com')
+      })
+
+      it('does not call writeLoginState when account has a name', async function () {
+        const account = {name: 'my-account', username: 'user@example.com'}
+        await AccountsModule.set(account, '/data/heroku')
+
+        expect(writeLoginStateStub.called).to.be.false
+      })
+    })
+
+    describe('with useNetrc and account name', function () {
+      let fakeNetrc: {machines: Record<string, {login: string, password: string}>, save: sinon.SinonStub}
+
+      function setNetrc(value: typeof fakeNetrc | undefined) {
+        (AccountsModule as unknown as {netrc: typeof fakeNetrc | undefined}).netrc = value
+      }
+
+      beforeEach(function () {
+        sinon.stub(AccountsModule, 'getStorageConfig').returns({credentialStore: null, useNetrc: true})
+        fakeNetrc = {machines: {}, save: sinon.stub().resolves()}
+        setNetrc(fakeNetrc)
+        fsReadFileStub.withArgs(sinon.match(/my-account$/), 'utf8')
+          .returns('username: user@example.com\npassword: secret\n')
+      })
+
+      afterEach(function () {
+        setNetrc(null as unknown as typeof fakeNetrc)
+      })
+
+      it('writes credentials to api.heroku.com and git.heroku.com machines', async function () {
+        const account = {name: 'my-account', username: 'user@example.com'}
+        await AccountsModule.set(account, '/data/heroku')
+
+        expect(fakeNetrc.machines['api.heroku.com']).to.deep.equal({login: 'user@example.com', password: 'secret'})
+        expect(fakeNetrc.machines['git.heroku.com']).to.deep.equal({login: 'user@example.com', password: 'secret'})
+      })
+
+      it('saves the netrc file', async function () {
+        const account = {name: 'my-account', username: 'user@example.com'}
+        await AccountsModule.set(account, '/data/heroku')
+
+        expect(fakeNetrc.save.calledOnce).to.be.true
+      })
+
+      it('does not update netrc when account has no name', async function () {
+        const account = {username: 'user@example.com'}
+        await AccountsModule.set(account, '/data/heroku')
+
+        expect(fakeNetrc.save.called).to.be.false
+      })
+    })
+  })
+
   describe('remove', function () {
     let unlinkStub: sinon.SinonStub
     let osHomeStub: sinon.SinonStub