diff --git a/src/commands/accounts/add.ts b/src/commands/accounts/add.ts index e9aadc0b99..b5bb542d36 100644 --- a/src/commands/accounts/add.ts +++ b/src/commands/accounts/add.ts @@ -30,7 +30,14 @@ export default class Add extends Command { } const token = this.heroku.auth! - - AccountsModule.add(name, email, token) + const config = AccountsModule.getStorageConfig() + + if (config.credentialStore) { + // Keychain-mode: don't save token to cache file + AccountsModule.add(name, email) + } else { + // Netrc-mode: save token to cache file + AccountsModule.add(name, email, token) + } } } diff --git a/src/lib/accounts/accounts.ts b/src/lib/accounts/accounts.ts index d51dced93d..a3d487f4ae 100644 --- a/src/lib/accounts/accounts.ts +++ b/src/lib/accounts/accounts.ts @@ -16,7 +16,7 @@ export interface AccountEntry { } export interface IAccountsWrapper { - add(name: string, username: string, password: string): void + add(name: string, username: string, password?: string): void current(heroku: APIClient): Promise currentNetrc(): Promise getStorageConfig(): ReturnType @@ -29,11 +29,14 @@ export interface IAccountsWrapper { export class AccountsWrapper implements IAccountsWrapper { private netrc: any - add(name: string, username: string, password: string): void { + add(name: string, username: string, password?: string): void { fs.mkdirSync(this.accountsDir(), {recursive: true}) - // eslint-disable-next-line perfectionist/sort-objects - this.writeAccountFile(name, {username, password}) + const content: Record = password === undefined + ? {username} + : {username, password} // eslint-disable-line perfectionist/sort-objects + + this.writeAccountFile(name, content) } async current(heroku: APIClient): Promise { @@ -92,12 +95,17 @@ export class AccountsWrapper implements IAccountsWrapper { if (account.name) { if (config.credentialStore) { + // Keychain mode: only update login state, skip netrc const email = this.getAliasEmail(account.name) - if (email) { - await this.writeLoginState(dataDir, email) + if (!email) { + throw new Error(`We can't find the alias file for ${account.name}.`) } + + await this.writeLoginState(dataDir, email) + return } + // Netrc mode: update both login state and netrc files const netrcInstance = await this.initNetrc() let current try { diff --git a/test/unit/commands/accounts/add.unit.test.ts b/test/unit/commands/accounts/add.unit.test.ts index ead9eef57f..6dc7d1503d 100644 --- a/test/unit/commands/accounts/add.unit.test.ts +++ b/test/unit/commands/accounts/add.unit.test.ts @@ -26,7 +26,7 @@ describe('accounts:add', function () { }) describe('when the user is logged in', function () { - it('should call the accounts.add function with the account name, user email, and auth token', async function () { + it('should call the accounts.add function with the account name and user email', async function () { stubCredentialManager({ getAuth: async () => ({account: 'testEmail', token: 'testHerokuAPIKey'}), }) @@ -38,6 +38,43 @@ describe('accounts:add', function () { expect(addStub.calledOnce).to.equal(true) expect(addStub.args[0][0]).to.equal('testAccountName') expect(addStub.args[0][1]).to.equal('testEmail') + }) + + it('should not pass token to add() when credentialStore is active (keychain-mode)', async function () { + stubCredentialManager({ + getAuth: async () => ({account: 'testEmail', token: 'testHerokuAPIKey'}), + }) + + const getStorageConfigStub = stub(AccountsModule, 'getStorageConfig') + getStorageConfigStub.returns({credentialStore: 'keychain' as any, useNetrc: false}) + + api.get('/account') + .reply(200, {email: 'testEmail'}) + + await runCommand(Cmd, ['testAccountName']) + + expect(addStub.calledOnce).to.equal(true) + expect(addStub.args[0][0]).to.equal('testAccountName') + expect(addStub.args[0][1]).to.equal('testEmail') + expect(addStub.args[0][2]).to.be.undefined + }) + + it('should pass token to add() when credentialStore is not active (netrc-mode)', async function () { + stubCredentialManager({ + getAuth: async () => ({account: 'testEmail', token: 'testHerokuAPIKey'}), + }) + + const getStorageConfigStub = stub(AccountsModule, 'getStorageConfig') + getStorageConfigStub.returns({credentialStore: null, useNetrc: true}) + + api.get('/account') + .reply(200, {email: 'testEmail'}) + + await runCommand(Cmd, ['testAccountName']) + + expect(addStub.calledOnce).to.equal(true) + expect(addStub.args[0][0]).to.equal('testAccountName') + expect(addStub.args[0][1]).to.equal('testEmail') expect(addStub.args[0][2]).to.equal('testHerokuAPIKey') }) diff --git a/test/unit/lib/accounts/accounts.unit.test.ts b/test/unit/lib/accounts/accounts.unit.test.ts index f9f2a4388f..dd6e33a6c5 100644 --- a/test/unit/lib/accounts/accounts.unit.test.ts +++ b/test/unit/lib/accounts/accounts.unit.test.ts @@ -119,6 +119,22 @@ describe('accounts', function () { expect(() => AccountsModule.add('test-user', 'username123', 'password123')).to.throw() }) + + it('should write only username when password is undefined (keychain-mode)', function () { + AccountsModule.add('test-user', 'username123') + + expect(writeFileSyncStub.calledOnce).to.be.true + expect(writeFileSyncStub.firstCall.args[1]).to.equal('username: username123\n') + expect(writeFileSyncStub.firstCall.args[2]).to.equal('utf8') + }) + + it('should write username and password when password is provided (netrc-mode)', function () { + AccountsModule.add('test-user', 'username123', 'password123') + + expect(writeFileSyncStub.calledOnce).to.be.true + expect(writeFileSyncStub.firstCall.args[1]).to.equal('username: username123\npassword: password123\n') + expect(writeFileSyncStub.firstCall.args[2]).to.equal('utf8') + }) }) describe('set()', function () { @@ -145,7 +161,7 @@ describe('accounts', function () { setNetrc(null as unknown as typeof fakeNetrc) }) - it('calls writeLoginState with email from alias file and writes to netrc', async function () { + it('calls writeLoginState with email from alias file but does not write to netrc', async function () { const account = {name: 'production', username: 'prod@example.com'} existsSyncStub.withArgs('/user/home/.config/heroku').returns(false) existsSyncStub.withArgs(match(/production$/)).returns(true) @@ -157,9 +173,9 @@ describe('accounts', function () { expect(writeLoginStateStub.calledOnce).to.be.true expect(writeLoginStateStub.firstCall.args[0]).to.equal('/data/heroku') expect(writeLoginStateStub.firstCall.args[1]).to.equal('prod@example.com') - expect(fakeNetrc.machines['api.heroku.com']).to.deep.equal({login: 'prod@example.com', password: 'secret'}) - expect(fakeNetrc.machines['git.heroku.com']).to.deep.equal({login: 'prod@example.com', password: 'secret'}) - expect(fakeNetrc.save.calledOnce).to.be.true + expect(fakeNetrc.machines['api.heroku.com']).to.be.undefined + expect(fakeNetrc.machines['git.heroku.com']).to.be.undefined + expect(fakeNetrc.save.called).to.be.false }) it('throws error when alias file does not exist', async function () {