Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/main/ipc/githubIpc.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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
Expand Down
48 changes: 45 additions & 3 deletions src/main/services/GitHubService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,18 @@ 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;
private pollingInterval: NodeJS.Timeout | null = null;
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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Authenticate with GitHub using Device Flow
* Returns device code info for the UI to display to the user
Expand Down Expand Up @@ -1263,9 +1268,11 @@ export class GitHubService {
*/
async logout(): Promise<void> {
this.stopPolling();
this.migrationAttempted = true;
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');
Expand All @@ -1275,27 +1282,62 @@ export class GitHubService {
/**
* Store authentication token securely
*/
private async storeToken(token: string): Promise<void> {
private async storeToken(token: string, source: 'user' | 'migration' = 'user'): Promise<void> {
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;
}
}

/**
* 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<string | null> {
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;
} catch (error) {
console.error('Failed to retrieve token:', error);
return null;
}

return this.migrateTokenFromGHCLI();
}

private async migrateTokenFromGHCLI(): Promise<string | null> {
if (this.migrationAttempted) return null;
this.migrationAttempted = true;

try {
const { stdout } = await execAsync('gh auth token', { encoding: 'utf8' });
const token = String(stdout).trim();
if (!token) 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
Expand Down
61 changes: 54 additions & 7 deletions src/test/main/GitHubService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,21 @@ 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<string, string>();

const keyFor = (serviceName: string, accountName: string) => `${serviceName}:${accountName}`;

const setPasswordMock = vi.fn(
async (serviceName: string, accountName: string, password: string) => {
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', () => {
Expand All @@ -98,10 +110,21 @@ describe('GitHubService.isAuthenticated', () => {
prListStdout = '[]';
repoViewStdout = 'generalaction/emdash';
prCountStdout = '0';
keychain.clear();
setPasswordMock.mockClear();
getPasswordMock.mockClear();
deletePasswordMock.mockClear();
getPasswordMock.mockResolvedValue(null);
setPasswordMock.mockImplementation(
async (serviceName: string, accountName: string, password: string) => {
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,
Expand All @@ -121,14 +144,37 @@ 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 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();
});

Expand All @@ -155,6 +201,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([]);
});

Expand Down
Loading