Merge pull request #1475 from eyaltoledano/mfa-fix-2

This commit is contained in:
Ralph Khreish
2025-12-02 19:45:10 +01:00
committed by GitHub
10 changed files with 313 additions and 285 deletions

View File

@@ -4,7 +4,6 @@
*/
import {
AUTH_TIMEOUT_MS,
type AuthCredentials,
AuthManager,
AuthenticationError
@@ -12,14 +11,8 @@ import {
import chalk from 'chalk';
import { Command } from 'commander';
import inquirer from 'inquirer';
import open from 'open';
import ora from 'ora';
import {
AuthCountdownTimer,
displayAuthInstructions,
displayWaitingForAuth,
handleMFAFlow
} from '../utils/auth-ui.js';
import { authenticateWithBrowserMFA, handleMFAFlow } from '../utils/auth-ui.js';
import { displayError } from '../utils/error-handler.js';
import * as ui from '../utils/ui.js';
import { ContextCommand } from './context.command.js';
@@ -434,7 +427,7 @@ Examples:
// Direct browser authentication - no menu needed
const credentials = await this.authenticateWithBrowser();
ui.displaySuccess('Authentication successful!');
// Display user info (auth success message is already shown by authenticateWithBrowserMFA)
console.log(
chalk.gray(` Logged in as: ${credentials.email || credentials.userId}`)
);
@@ -497,70 +490,11 @@ Examples:
/**
* Authenticate with browser using OAuth 2.0 with PKCE
* Uses shared countdown timer from auth-ui.ts
* Includes MFA handling if user has MFA enabled
* Uses shared authenticateWithBrowserMFA for consistent login UX
* across all commands (auth login, parse-prd, export, etc.)
*/
private async authenticateWithBrowser(): Promise<AuthCredentials> {
const countdownTimer = new AuthCountdownTimer(AUTH_TIMEOUT_MS);
try {
// Use AuthManager's new unified OAuth flow method with callbacks
const credentials = await this.authManager.authenticateWithOAuth({
// Callback to handle browser opening
openBrowser: async (authUrl) => {
await open(authUrl);
},
timeout: AUTH_TIMEOUT_MS,
// Callback when auth URL is ready
onAuthUrl: (authUrl) => {
displayAuthInstructions(authUrl);
},
// Callback when waiting for authentication
onWaitingForAuth: () => {
displayWaitingForAuth();
countdownTimer.start();
},
// Callback on success
onSuccess: () => {
countdownTimer.stop('success');
},
// Callback on error
onError: () => {
countdownTimer.stop('failure');
}
});
return credentials;
} catch (error) {
// Check if MFA is required BEFORE showing failure message
if (
error instanceof AuthenticationError &&
error.code === 'MFA_REQUIRED'
) {
// Stop spinner without showing failure - MFA is required, not a failure
countdownTimer.stop('mfa');
if (!error.mfaChallenge?.factorId) {
throw new AuthenticationError(
'MFA challenge information missing',
'MFA_VERIFICATION_FAILED'
);
}
// Use shared MFA flow handler
return this.handleMFAVerification(error);
}
countdownTimer.stop('failure');
throw error;
} finally {
// Ensure cleanup
countdownTimer.cleanup();
}
return authenticateWithBrowserMFA(this.authManager);
}
/**
@@ -635,7 +569,7 @@ Examples:
// Authenticate with the token
const credentials = await this.authenticateWithToken(token);
ui.displaySuccess('Authentication successful!');
// Display user info (auth success message is already shown by authenticateWithToken spinner)
console.log(
chalk.gray(` Logged in as: ${credentials.email || credentials.userId}`)
);

View File

@@ -6,6 +6,7 @@
import fs from 'node:fs/promises';
import path from 'node:path';
import {
AuthManager,
FileStorage,
type GenerateBriefResult,
type InvitationResult,
@@ -28,6 +29,7 @@ import { createUrlLink } from '../ui/index.js';
import { ensureAuthenticated } from '../utils/auth-guard.js';
import { selectBriefFromInput } from '../utils/brief-selection.js';
import { displayError } from '../utils/error-handler.js';
import { ensureOrgSelected } from '../utils/org-selection.js';
import { getProjectRoot } from '../utils/project-root.js';
/**
@@ -334,6 +336,23 @@ export class ExportCommand extends Command {
return;
}
// Force org selection after tag selection
// User can choose which org to export to, with current org pre-selected
const authManager = AuthManager.getInstance();
const orgResult = await ensureOrgSelected(authManager, {
promptMessage: 'Select an organization to export to:',
forceSelection: true
});
if (!orgResult.success) {
console.log(chalk.gray('\n Export cancelled.\n'));
this.lastResult = {
success: false,
action: 'cancelled',
message: orgResult.message || 'Organization selection cancelled'
};
return;
}
// Handle multiple tags export
if (selectedTags.length > 1) {
await this.executeExportMultipleTags(selectedTags, options);
@@ -972,9 +991,8 @@ export class ExportCommand extends Command {
try {
if (!this.taskMasterCore) return;
// Get AuthManager from TmCore
const authManager = (this.taskMasterCore.auth as any).authManager;
if (!authManager) return;
// Get AuthManager singleton
const authManager = AuthManager.getInstance();
// Use the selectBriefFromInput utility which properly resolves
// the brief and sets all context fields (org, brief details, etc.)

View File

@@ -3,24 +3,17 @@
* Provides reusable authentication checking and OAuth flow triggering
* for commands that require authentication.
*
* Includes MFA (Multi-Factor Authentication) support.
* Uses the shared authenticateWithBrowserMFA utility for consistent
* login UX across all commands (auth login, parse-prd, export, etc.)
*
* After successful authentication, ensures org selection is completed.
*/
import {
AUTH_TIMEOUT_MS,
type AuthCredentials,
AuthDomain,
AuthenticationError
} from '@tm/core';
import { type AuthCredentials, AuthDomain, AuthManager } from '@tm/core';
import chalk from 'chalk';
import inquirer from 'inquirer';
import open from 'open';
import {
AuthCountdownTimer,
displayAuthInstructions,
displayWaitingForAuth,
handleMFAFlow
} from './auth-ui.js';
import { authenticateWithBrowserMFA } from './auth-ui.js';
import { ensureOrgSelected } from './org-selection.js';
/**
* Options for the auth guard
@@ -81,6 +74,28 @@ export async function ensureAuthenticated(
// Check if already authenticated
const hasSession = await authDomain.hasValidSession();
if (hasSession) {
// Only get AuthManager when we need to check org selection
const authManager = AuthManager.getInstance();
// Check if org is already selected (quick check before any API calls)
const context = authManager.getContext();
if (context?.orgId) {
// Org already selected, return immediately without further API calls
return { authenticated: true };
}
// Org not selected, need to prompt
const orgResult = await ensureOrgSelected(authManager, {
promptMessage: 'Select an organization to continue:'
});
if (!orgResult.success) {
return {
authenticated: true,
error: orgResult.message || 'Organization selection required'
};
}
return { authenticated: true };
}
@@ -112,9 +127,31 @@ export async function ensureAuthenticated(
}
}
// Trigger OAuth flow
// Trigger OAuth flow using shared browser auth with MFA support
try {
const credentials = await authenticateWithBrowser(authDomain);
const credentials = await authenticateWithBrowserMFA(authDomain);
// Display user info (auth success message is already shown by authenticateWithBrowserMFA)
if (credentials.email) {
console.log(chalk.gray(` Logged in as: ${credentials.email}`));
}
console.log('');
// After successful authentication, ensure org is selected
// This is REQUIRED for all Hamster operations
const authManager = AuthManager.getInstance();
const orgResult = await ensureOrgSelected(authManager, {
promptMessage: 'Select an organization to continue:'
});
if (!orgResult.success) {
return {
authenticated: true, // Auth succeeded, but org selection failed
credentials,
error: orgResult.message || 'Organization selection required'
};
}
return {
authenticated: true,
credentials
@@ -127,74 +164,6 @@ export async function ensureAuthenticated(
}
}
/**
* Authenticate with browser using OAuth 2.0 with PKCE
* Includes MFA handling if the user has MFA enabled.
*/
async function authenticateWithBrowser(
authDomain: AuthDomain
): Promise<AuthCredentials> {
const countdownTimer = new AuthCountdownTimer(AUTH_TIMEOUT_MS);
try {
const credentials = await authDomain.authenticateWithOAuth({
// Callback to handle browser opening
openBrowser: async (authUrl: string) => {
await open(authUrl);
},
timeout: AUTH_TIMEOUT_MS,
// Callback when auth URL is ready
onAuthUrl: (authUrl: string) => {
displayAuthInstructions(authUrl);
},
// Callback when waiting for authentication
onWaitingForAuth: () => {
displayWaitingForAuth();
countdownTimer.start();
},
// Callback on success
onSuccess: () => {
countdownTimer.stop('success');
},
// Callback on error
onError: () => {
countdownTimer.stop('failure');
}
});
return credentials;
} catch (error: unknown) {
// Check if MFA is required BEFORE showing failure message
if (error instanceof AuthenticationError && error.code === 'MFA_REQUIRED') {
// Stop spinner without showing failure - MFA is required, not a failure
countdownTimer.stop('mfa');
if (!error.mfaChallenge?.factorId) {
throw new AuthenticationError(
'MFA challenge information missing',
'MFA_VERIFICATION_FAILED'
);
}
// Use shared MFA flow handler
return handleMFAFlow(
authDomain.verifyMFAWithRetry.bind(authDomain),
error.mfaChallenge.factorId
);
}
countdownTimer.stop('failure');
throw error;
} finally {
// Ensure cleanup
countdownTimer.cleanup();
}
}
/**
* Higher-order function that wraps a command action with auth checking.
* Use this to easily protect any command that requires authentication.

View File

@@ -11,10 +11,12 @@ import {
AUTH_TIMEOUT_MS,
type AuthCredentials,
AuthenticationError,
MFA_MAX_ATTEMPTS
MFA_MAX_ATTEMPTS,
type OAuthFlowOptions
} from '@tm/core';
import chalk from 'chalk';
import inquirer from 'inquirer';
import open from 'open';
import ora, { type Ora } from 'ora';
import * as ui from './ui.js';
@@ -267,3 +269,111 @@ export async function handleMFAFlow(
'MFA_VERIFICATION_FAILED'
);
}
/**
* Authentication provider interface for browser OAuth with MFA
* Can be satisfied by either AuthManager or AuthDomain
*/
export interface BrowserAuthProvider {
authenticateWithOAuth(options?: OAuthFlowOptions): Promise<AuthCredentials>;
verifyMFAWithRetry(
factorId: string,
codeProvider: () => Promise<string>,
options?: {
maxAttempts?: number;
onInvalidCode?: (attempt: number, remaining: number) => void;
}
): Promise<{
success: boolean;
credentials?: AuthCredentials;
attemptsUsed: number;
}>;
}
/**
* Shared browser authentication with MFA support
*
* This is the SINGLE implementation of browser-based OAuth login with MFA handling.
* Used by both the auth login command and any protected commands that trigger login
* (like parse-prd when "Bring it to Hamster" is selected).
*
* @param authProvider - AuthManager or AuthDomain instance
* @returns AuthCredentials on success
* @throws AuthenticationError on failure
*
* @example
* ```typescript
* // From auth command
* const authManager = AuthManager.getInstance();
* const credentials = await authenticateWithBrowserMFA(authManager);
*
* // From auth-guard (for protected commands)
* const authDomain = new AuthDomain();
* const credentials = await authenticateWithBrowserMFA(authDomain);
* ```
*/
export async function authenticateWithBrowserMFA(
authProvider: BrowserAuthProvider
): Promise<AuthCredentials> {
const countdownTimer = new AuthCountdownTimer(AUTH_TIMEOUT_MS);
try {
const credentials = await authProvider.authenticateWithOAuth({
// Callback to handle browser opening
openBrowser: async (authUrl: string) => {
await open(authUrl);
},
timeout: AUTH_TIMEOUT_MS,
// Callback when auth URL is ready
onAuthUrl: (authUrl: string) => {
displayAuthInstructions(authUrl);
},
// Callback when waiting for authentication
onWaitingForAuth: () => {
displayWaitingForAuth();
countdownTimer.start();
},
// Callback on success
onSuccess: () => {
countdownTimer.stop('success');
},
// Don't handle onError here - we need to check error type in catch block
// to differentiate between MFA_REQUIRED (not a failure) and actual failures
onError: () => {
// Timer will be stopped in catch block with appropriate status
}
});
return credentials;
} catch (error: unknown) {
// Check if MFA is required BEFORE showing failure message
if (error instanceof AuthenticationError && error.code === 'MFA_REQUIRED') {
// Stop spinner without showing failure - MFA is required, not a failure
countdownTimer.stop('mfa');
if (!error.mfaChallenge?.factorId) {
throw new AuthenticationError(
'MFA challenge information missing',
'MFA_VERIFICATION_FAILED'
);
}
// Use shared MFA flow handler
return handleMFAFlow(
authProvider.verifyMFAWithRetry.bind(authProvider),
error.mfaChallenge.factorId
);
}
// Only show failure for actual errors, not MFA requirement
countdownTimer.stop('failure');
throw error;
} finally {
// Ensure cleanup
countdownTimer.cleanup();
}
}

View File

@@ -20,6 +20,12 @@ export {
type AuthGuardResult
} from './auth-guard.js';
// Shared browser authentication with MFA support
export { authenticateWithBrowserMFA } from './auth-ui.js';
// Organization selection utility
export { ensureOrgSelected, type OrgSelectionResult } from './org-selection.js';
// Command guard for local-only commands
export {
checkAndBlockIfAuthenticated,

View File

@@ -3,7 +3,7 @@
* Provides reusable org selection flow for commands that require org context.
*/
import { AuthManager } from '@tm/core';
import type { AuthManager } from '@tm/core';
import chalk from 'chalk';
import inquirer from 'inquirer';
import * as ui from './ui.js';
@@ -27,6 +27,8 @@ export interface EnsureOrgOptions {
silent?: boolean;
/** Custom message to show when prompting */
promptMessage?: string;
/** If true, always prompt for org selection even if one is already set */
forceSelection?: boolean;
}
/**
@@ -55,13 +57,13 @@ export async function ensureOrgSelected(
authManager: AuthManager,
options: EnsureOrgOptions = {}
): Promise<OrgSelectionResult> {
const { silent = false, promptMessage } = options;
const { silent = false, promptMessage, forceSelection = false } = options;
try {
const context = authManager.getContext();
// If org is already selected, return it
if (context?.orgId) {
// If org is already selected and we're not forcing selection, return it
if (context?.orgId && !forceSelection) {
return {
success: true,
orgId: context.orgId,
@@ -70,7 +72,7 @@ export async function ensureOrgSelected(
};
}
// No org selected - check if we can auto-select
// Fetch available orgs
const orgs = await authManager.getOrganizations();
if (orgs.length === 0) {
@@ -104,19 +106,25 @@ export async function ensureOrgSelected(
}
// Multiple orgs - prompt for selection
if (!silent) {
if (!silent && !context?.orgId) {
console.log(chalk.yellow('No organization selected.'));
}
// Set default to current org if one is selected
const defaultOrg = context?.orgId
? orgs.findIndex((o) => o.id === context.orgId)
: 0;
const response = await inquirer.prompt<{ orgId: string }>([
{
type: 'list',
name: 'orgId',
message: promptMessage || 'Select an organization:',
choices: orgs.map((org) => ({
name: org.name,
name: org.id === context?.orgId ? `${org.name} (current)` : org.name,
value: org.id
}))
})),
default: defaultOrg >= 0 ? defaultOrg : 0
}
]);

View File

@@ -114,8 +114,9 @@ export class OAuthService {
error
);
// Notify error
if (onError) {
// Only notify error for actual failures, NOT for MFA_REQUIRED
// MFA requirement is a continuation of the auth flow, not an error
if (onError && authError.code !== 'MFA_REQUIRED') {
onError(authError);
}

View File

@@ -1534,14 +1534,46 @@ export class ExportService {
};
}
const jsonData = (await response.json()) as SendTeamInvitationsResponse;
const jsonData = (await response.json()) as Record<string, unknown>;
if (!response.ok || !jsonData.success) {
// Check if all users are already members - this is not an error
const invitations = jsonData.invitations || (jsonData as any)?.data;
const jsonError = (jsonData as any)?.error;
// Extract invitations from various possible response structures
const dataField = jsonData.data as Record<string, unknown> | undefined;
const invitations =
jsonData.invitations ||
dataField?.invitations ||
dataField ||
(Array.isArray(jsonData) ? jsonData : null);
const jsonError = jsonData.error as
| { code?: string; message?: string }
| string
| undefined;
// Handle "already member" as success - check both invitations array and error message
// Success case: 200 OK with invitations array
if (response.ok && invitations && Array.isArray(invitations)) {
return {
success: true,
invitations: invitations.map(
(inv: { email?: string; status?: string }) => ({
email: inv.email || '',
status: (inv.status || 'sent') as
| 'sent'
| 'already_member'
| 'already_invited'
| 'failed'
})
)
};
}
// Error case: check for specific error conditions
if (!response.ok || jsonError) {
// Parse error object safely
const errorObj =
typeof jsonError === 'object' && jsonError !== null
? (jsonError as { code?: string; message?: string })
: null;
// Handle "already member" as success
const isAlreadyMember =
(invitations &&
Array.isArray(invitations) &&
@@ -1549,17 +1581,24 @@ export class ExportService {
invitations.every(
(inv: { status: string }) => inv.status === 'already_member'
)) ||
(jsonError?.code === 'invitation_failed' &&
jsonError?.message?.toLowerCase().includes('already member'));
(errorObj?.code === 'invitation_failed' &&
errorObj?.message?.toLowerCase().includes('already member'));
if (isAlreadyMember) {
// Return success with synthetic invitations if we only got an error
const resultInvitations =
invitations ||
emails.map((email) => ({
email,
status: 'already_member' as const
}));
const resultInvitations = Array.isArray(invitations)
? (invitations as Array<{
email: string;
status:
| 'sent'
| 'already_member'
| 'already_invited'
| 'failed';
}>)
: emails.map((email) => ({
email,
status: 'already_member' as const
}));
return {
success: true,
invitations: resultInvitations
@@ -1567,10 +1606,10 @@ export class ExportService {
}
const errorMessage =
(jsonData as any)?.message ||
(jsonData.message as string) ||
(typeof jsonError === 'string'
? jsonError
: jsonError?.message || JSON.stringify(jsonError)) ||
: errorObj?.message || JSON.stringify(jsonError)) ||
`Failed to send invitations: ${response.status}`;
return {
@@ -1582,9 +1621,13 @@ export class ExportService {
};
}
// Fallback: no invitations in response
return {
success: true,
invitations: jsonData.invitations
success: false,
error: {
code: 'INVALID_RESPONSE',
message: 'No invitations in response'
}
};
} catch (error) {
const errorMessage =

View File

@@ -17,8 +17,8 @@ import { randomUUID } from 'crypto';
import fs from 'fs';
import path from 'path';
import readline from 'readline';
import { ui } from '@tm/cli';
import { AuthManager, AUTH_TIMEOUT_MS } from '@tm/core';
import { authenticateWithBrowserMFA, ensureOrgSelected, ui } from '@tm/cli';
import { AuthManager } from '@tm/core';
import boxen from 'boxen';
import chalk from 'chalk';
import figlet from 'figlet';
@@ -419,7 +419,8 @@ async function initializeProject(options = {}) {
log('success', 'Already authenticated with Hamster');
authCredentials = existingCredentials;
} else {
// Trigger OAuth flow
// Use shared browser auth with MFA support
// This is the SAME auth flow used by 'tm auth login' and 'tm parse-prd'
log('info', 'Starting authentication flow...');
console.log(chalk.blue('\n🔐 Authentication Required\n'));
console.log(
@@ -430,94 +431,23 @@ async function initializeProject(options = {}) {
console.log(
chalk.gray(' This enables sync across devices with Hamster.\n')
);
let countdownInterval = null;
let authSpinner = null;
const startCountdown = (totalMs) => {
const startTime = Date.now();
const endTime = startTime + totalMs;
const updateCountdown = () => {
const remaining = Math.max(0, endTime - Date.now());
const mins = Math.floor(remaining / 60000);
const secs = Math.floor((remaining % 60000) / 1000);
const timeStr = `${mins}:${secs.toString().padStart(2, '0')}`;
if (authSpinner) {
authSpinner.text = `Waiting for authentication... ${chalk.cyan(timeStr)} remaining`;
}
if (remaining <= 0 && countdownInterval) {
clearInterval(countdownInterval);
}
};
authSpinner = ora({
text: `Waiting for authentication... ${chalk.cyan('10:00')} remaining`,
spinner: 'dots'
}).start();
countdownInterval = setInterval(updateCountdown, 1000);
};
const stopCountdown = (success) => {
if (countdownInterval) {
clearInterval(countdownInterval);
countdownInterval = null;
}
if (authSpinner) {
if (success) {
authSpinner.succeed('Authentication successful!');
} else {
authSpinner.fail('Authentication failed');
}
authSpinner = null;
}
};
authCredentials = await authManager.authenticateWithOAuth({
openBrowser: async (authUrl) => {
await open(authUrl);
},
timeout: AUTH_TIMEOUT_MS,
onAuthUrl: (authUrl) => {
console.log(
chalk.blue.bold('\n[auth] Browser Authentication\n')
);
console.log(
chalk.white(' Opening your browser to authenticate...')
);
console.log(
chalk.gray(" If the browser doesn't open, visit:")
);
console.log(chalk.cyan.underline(` ${authUrl}\n`));
},
onWaitingForAuth: () => {
console.log(
chalk.dim(
' If you signed up, check your email to confirm your account.'
)
);
console.log(
chalk.dim(
' The CLI will automatically detect when you log in.\n'
)
);
startCountdown(AUTH_TIMEOUT_MS);
},
onSuccess: () => {
stopCountdown(true);
},
onError: (error) => {
stopCountdown(false);
log('error', `Authentication failed: ${error.message}`);
}
});
// Use shared auth utility - handles MFA automatically
authCredentials = await authenticateWithBrowserMFA(authManager);
// Track auth_completed event
// TODO: Send to Segment telemetry when implemented
log('debug', `Auth completed - taskmaster_id: ${taskmasterId}`);
}
// Ensure org is selected (required for all Hamster operations)
// This runs for both new auth AND existing auth
// Uses shared utility from @tm/cli
const orgResult = await ensureOrgSelected(authManager, {
promptMessage: 'Select an organization to continue:'
});
if (!orgResult.success) {
log('warn', orgResult.message || 'Organization selection required');
}
} catch (authError) {
log(
'error',
@@ -815,20 +745,14 @@ async function promptStorageSelection() {
try {
// Display header
console.log(
'\n' +
chalk.bold.cyan('You need a plan before you execute.') +
' ' +
chalk.white('How do you want to build it?\n')
);
console.log(chalk.bold.cyan('You need a plan before you execute.\n'));
const { storageType } = await inquirer.prompt([
{
type: 'list',
name: 'storageType',
message: chalk.cyan('Choose one:'),
message: chalk.white('How do you want to build it?\n'),
choices: [
'\n',
{
name: [
chalk.bold('Solo (Taskmaster)'),

View File

@@ -63,7 +63,7 @@ import {
validateDependenciesCommand
} from './dependency-manager.js';
import { checkAndBlockIfAuthenticated } from '@tm/cli';
import { checkAndBlockIfAuthenticated, ensureOrgSelected } from '@tm/cli';
import { LOCAL_ONLY_COMMANDS } from '@tm/core';
import {
@@ -203,9 +203,8 @@ async function promptHamsterCollaboration() {
{
type: 'list',
name: 'choice',
message: chalk.cyan('How would you like to parse your PRD?'),
message: chalk.cyan('How would you like to parse your PRD?\n'),
choices: [
'\n',
{
name: [
chalk.bold('Parse locally'),
@@ -384,6 +383,22 @@ async function handleParsePrdToHamster(prdPath) {
const authManager = AuthManager.getInstance();
// Always prompt for organization selection for parse-prd
// This allows users to choose which org to create the brief in
// even if they have one already selected in context
const orgResult = await ensureOrgSelected(authManager, {
promptMessage: 'Select an organization to create the brief in:',
forceSelection: true
});
if (!orgResult.success) {
console.error(
chalk.red(
`\n ${orgResult.message || 'Organization selection cancelled.'}\n`
)
);
return;
}
// Read PRD file content
const prdContent = fs.readFileSync(prdPath, 'utf-8');
if (!prdContent.trim()) {