diff --git a/src/main/ipc/githubIpc.ts b/src/main/ipc/githubIpc.ts index d66a2cb65..c55cf3cfc 100644 --- a/src/main/ipc/githubIpc.ts +++ b/src/main/ipc/githubIpc.ts @@ -1,6 +1,6 @@ import { ipcMain, app } from 'electron'; import { log } from '../lib/logger'; -import { GitHubService } from '../services/GitHubService'; +import { githubService } from '../services/GitHubService'; import { worktreeService } from '../services/WorktreeService'; import { githubCLIInstaller } from '../services/GitHubCLIInstaller'; import { databaseService } from '../services/DatabaseService'; @@ -14,7 +14,6 @@ import { quoteShellArg } from '../utils/shellEscape'; import { getAppSettings } from '../settings'; const execAsync = promisify(exec); -const githubService = new GitHubService(); const slugify = (name: string) => name diff --git a/src/main/services/GitHubService.ts b/src/main/services/GitHubService.ts index cfa9cb69f..c7d290d58 100644 --- a/src/main/services/GitHubService.ts +++ b/src/main/services/GitHubService.ts @@ -97,6 +97,7 @@ export interface DeviceCodeResult { export class GitHubService { private readonly SERVICE_NAME = 'emdash-github'; private readonly ACCOUNT_NAME = 'github-token'; + private readonly MIGRATION_BLOCK_ACCOUNT = 'github-migration-blocked'; // Polling state management private isPolling = false; @@ -104,6 +105,14 @@ export class GitHubService { private currentDeviceCode: string | null = null; private currentInterval = 5; + // One-shot migration guard: try reading from `gh auth token` at most once + // per process when the Emdash keychain is empty. + private migrationAttempted = false; + private migrationInFlight: Promise | null = null; + + // Serializes auth state changes (logout + legacy token migration persistence). + private authStateLock: Promise = Promise.resolve(); + /** * Authenticate with GitHub using Device Flow * Returns device code info for the UI to display to the user @@ -1258,27 +1267,52 @@ export class GitHubService { } } + private async withAuthStateLock(operation: () => Promise): Promise { + const previousLock = this.authStateLock; + let releaseLock!: () => void; + + this.authStateLock = new Promise((resolve) => { + releaseLock = resolve; + }); + + await previousLock; + + try { + return await operation(); + } finally { + releaseLock(); + } + } + /** * Logout and clear stored token */ async logout(): Promise { this.stopPolling(); - try { - const keytar = await import('keytar'); - await keytar.deletePassword(this.SERVICE_NAME, this.ACCOUNT_NAME); - } catch (error) { - console.error('Failed to clear keychain token:', error); - throw new Error('Failed to clear keychain token'); - } + this.migrationAttempted = true; + + await this.withAuthStateLock(async () => { + try { + const keytar = await import('keytar'); + await keytar.deletePassword(this.SERVICE_NAME, this.ACCOUNT_NAME); + await keytar.setPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT, '1'); + } catch (error) { + console.error('Failed to clear keychain token:', error); + throw new Error('Failed to clear keychain token'); + } + }); } /** * Store authentication token securely */ - private async storeToken(token: string): Promise { + private async storeToken(token: string, source: 'user' | 'migration' = 'user'): Promise { try { const keytar = await import('keytar'); await keytar.setPassword(this.SERVICE_NAME, this.ACCOUNT_NAME, token); + if (source === 'user') { + await keytar.deletePassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT); + } } catch (error) { console.error('Failed to store token:', error); throw error; @@ -1286,16 +1320,83 @@ export class GitHubService { } /** - * Retrieve stored authentication token + * Retrieve stored authentication token. + * + * Migration for users authenticated before the gh-CLI decouple: their token + * only lives in the global gh CLI state. If the Emdash keychain is empty we + * try `gh auth token` once and persist the result, so PR lists and other + * gh-backed features keep working without asking the user to re-auth. */ async getStoredToken(): Promise { + if (this.migrationInFlight) { + return this.migrationInFlight; + } + try { const keytar = await import('keytar'); - return await keytar.getPassword(this.SERVICE_NAME, this.ACCOUNT_NAME); + const stored = await keytar.getPassword(this.SERVICE_NAME, this.ACCOUNT_NAME); + if (stored) return stored; + + const migrationBlocked = + (await keytar.getPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT)) === '1'; + if (migrationBlocked) return null; + + if (this.migrationInFlight) { + return this.migrationInFlight; + } + + if (this.migrationAttempted) return null; } catch (error) { console.error('Failed to retrieve token:', error); return null; } + + const inFlight = this.migrateTokenFromGHCLI(); + this.migrationInFlight = inFlight; + + try { + return await inFlight; + } finally { + if (this.migrationInFlight === inFlight) { + this.migrationInFlight = null; + } + } + } + + private async migrateTokenFromGHCLI(): Promise { + return this.withAuthStateLock(async () => { + if (this.migrationAttempted) return null; + this.migrationAttempted = true; + + try { + const keytar = await import('keytar'); + + const migrationBlockedBeforeRead = + (await keytar.getPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT)) === '1'; + if (migrationBlockedBeforeRead) return null; + + const { stdout } = await execAsync('gh auth token', { encoding: 'utf8' }); + const token = String(stdout).trim(); + if (!token) return null; + + // Re-check auth state while still serialized with logout(). + const stored = await keytar.getPassword(this.SERVICE_NAME, this.ACCOUNT_NAME); + if (stored) return stored; + + const migrationBlockedAfterRead = + (await keytar.getPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT)) === '1'; + if (migrationBlockedAfterRead) return null; + + try { + await this.storeToken(token, 'migration'); + } catch (error) { + console.warn('Failed to persist migrated gh CLI token to keychain:', error); + } + return token; + } catch { + return null; + } + }); } // ----------------------------------------------------------------------- // Repo events API — efficient polling with ETag caching diff --git a/src/test/main/GitHubService.test.ts b/src/test/main/GitHubService.test.ts index d6cd754cb..7e108e662 100644 --- a/src/test/main/GitHubService.test.ts +++ b/src/test/main/GitHubService.test.ts @@ -7,6 +7,9 @@ let issueSearchStdout = '[]'; let prListStdout = '[]'; let repoViewStdout = 'generalaction/emdash'; let prCountStdout = '0'; +let blockTokenStore = false; +let releaseTokenStore: (() => void) | null = null; +let notifyTokenStoreStarted: (() => void) | null = null; vi.mock('child_process', () => { const execImpl = (command: string, options?: any, callback?: any) => { @@ -70,9 +73,30 @@ vi.mock('child_process', () => { }; }); -const setPasswordMock = vi.fn().mockResolvedValue(undefined); -const getPasswordMock = vi.fn().mockResolvedValue(null); -const deletePasswordMock = vi.fn().mockResolvedValue(undefined); +const keychain = new Map(); + +const keyFor = (serviceName: string, accountName: string) => `${serviceName}:${accountName}`; + +const setPasswordMock = vi.fn( + async (serviceName: string, accountName: string, password: string) => { + if (accountName === 'github-token') { + notifyTokenStoreStarted?.(); + if (blockTokenStore) { + await new Promise((resolve) => { + releaseTokenStore = resolve; + }); + } + } + + keychain.set(keyFor(serviceName, accountName), password); + } +); +const getPasswordMock = vi.fn(async (serviceName: string, accountName: string) => { + return keychain.get(keyFor(serviceName, accountName)) ?? null; +}); +const deletePasswordMock = vi.fn(async (serviceName: string, accountName: string) => { + keychain.delete(keyFor(serviceName, accountName)); +}); const fetchMock = vi.fn(); vi.mock('keytar', () => { @@ -98,10 +122,33 @@ describe('GitHubService.isAuthenticated', () => { prListStdout = '[]'; repoViewStdout = 'generalaction/emdash'; prCountStdout = '0'; + blockTokenStore = false; + releaseTokenStore = null; + notifyTokenStoreStarted = null; + keychain.clear(); setPasswordMock.mockClear(); getPasswordMock.mockClear(); deletePasswordMock.mockClear(); - getPasswordMock.mockResolvedValue(null); + setPasswordMock.mockImplementation( + async (serviceName: string, accountName: string, password: string) => { + if (accountName === 'github-token') { + notifyTokenStoreStarted?.(); + if (blockTokenStore) { + await new Promise((resolve) => { + releaseTokenStore = resolve; + }); + } + } + + keychain.set(keyFor(serviceName, accountName), password); + } + ); + getPasswordMock.mockImplementation(async (serviceName: string, accountName: string) => { + return keychain.get(keyFor(serviceName, accountName)) ?? null; + }); + deletePasswordMock.mockImplementation(async (serviceName: string, accountName: string) => { + keychain.delete(keyFor(serviceName, accountName)); + }); fetchMock.mockReset(); fetchMock.mockResolvedValue({ ok: true, @@ -121,14 +168,65 @@ describe('GitHubService.isAuthenticated', () => { vi.unstubAllGlobals(); }); - it('does not treat a global gh login as authenticated without an Emdash token', async () => { + it('migrates a token from the gh CLI into the keychain when Emdash has none', async () => { const service = new GitHubService(); const result = await service.isAuthenticated(); + expect(result).toBe(true); + expect(execCalls.find((cmd) => cmd.startsWith('gh auth token'))).toBeDefined(); + expect(setPasswordMock).toHaveBeenCalledWith('emdash-github', 'github-token', 'gho_mocktoken'); + expect(fetchMock).toHaveBeenCalledWith( + 'https://api.github.com/user', + expect.objectContaining({ + headers: expect.objectContaining({ + Authorization: 'Bearer gho_mocktoken', + }), + }) + ); + }); + + it('does not persist a migrated token after logout races with migration', async () => { + const service = new GitHubService(); + + blockTokenStore = true; + const tokenStoreStarted = new Promise((resolve) => { + notifyTokenStoreStarted = () => { + notifyTokenStoreStarted = null; + resolve(); + }; + }); + + const migration = service.getStoredToken(); + await tokenStoreStarted; + + const logout = service.logout(); + + expect(deletePasswordMock).not.toHaveBeenCalled(); + expect(keychain.get('emdash-github:github-migration-blocked')).toBeUndefined(); + + releaseTokenStore?.(); + + expect(await migration).toBe('gho_mocktoken'); + await logout; + + expect(keychain.get('emdash-github:github-token') ?? null).toBeNull(); + expect(keychain.get('emdash-github:github-migration-blocked')).toBe('1'); + }); + + it('does not auto-migrate from the gh CLI after the user has logged out', async () => { + const service = new GitHubService(); + await service.logout(); + + execCalls.length = 0; + setPasswordMock.mockClear(); + + // New service instance simulates an app restart. + const serviceAfterRestart = new GitHubService(); + const result = await serviceAfterRestart.isAuthenticated(); + expect(result).toBe(false); - expect(fetchMock).not.toHaveBeenCalled(); - expect(execCalls.find((cmd) => cmd.startsWith('gh auth status'))).toBeUndefined(); + expect(execCalls.find((cmd) => cmd.startsWith('gh auth token'))).toBeUndefined(); expect(setPasswordMock).not.toHaveBeenCalled(); }); @@ -155,6 +253,7 @@ describe('GitHubService.isAuthenticated', () => { await service.logout(); expect(deletePasswordMock).toHaveBeenCalledWith('emdash-github', 'github-token'); + expect(setPasswordMock).toHaveBeenCalledWith('emdash-github', 'github-migration-blocked', '1'); expect(execCalls).toEqual([]); });