From baa715b67383e9d930c5b079c8be0ba3ed11d105 Mon Sep 17 00:00:00 2001 From: Facundo Rodriguez Date: Sat, 21 Feb 2026 12:49:15 -0300 Subject: [PATCH] refactor(auth): better UX for OAuth error messages --- src/commands/auth/login.ts | 35 +++++++++++++++ src/config/constants.ts | 6 +++ test/commands/auth/login.test.ts | 74 +++++++++++++++++++++++++++++--- 3 files changed, 108 insertions(+), 7 deletions(-) diff --git a/src/commands/auth/login.ts b/src/commands/auth/login.ts index 0ea96711..7140c7f3 100644 --- a/src/commands/auth/login.ts +++ b/src/commands/auth/login.ts @@ -4,6 +4,7 @@ import { createInterface } from 'node:readline'; import { URL } from 'node:url'; import { Command } from '@oclif/core'; import { ensureUserSetup } from '../../api/user-setup.client.ts'; +import { OAUTH_CALLBACK_ERROR_CODES } from '../../config/constants.ts'; import { persistTokenResponse } from '../../service/auth.svc.ts'; import { getClientId, getRealmUrl } from '../../service/auth-config.svc.ts'; import { debugLogger, getErrorMessage } from '../../service/log.svc.ts'; @@ -80,6 +81,8 @@ export default class AuthLogin extends Command { if (parsedUrl.pathname === '/oauth2/callback') { const code = parsedUrl.searchParams.get('code'); const state = parsedUrl.searchParams.get('state'); + const oauthError = parsedUrl.searchParams.get('error'); + const oauthErrorDescription = parsedUrl.searchParams.get('error_description'); if (!state) { res.writeHead(400, { 'Content-Type': 'text/plain' }); @@ -95,6 +98,38 @@ export default class AuthLogin extends Command { return reject(new Error('State verification failed')); } + if (oauthError) { + const isAlreadyLoggedIn = oauthError === OAUTH_CALLBACK_ERROR_CODES.ALREADY_LOGGED_IN; + const isDifferentUserAuthenticated = oauthError === OAUTH_CALLBACK_ERROR_CODES.DIFFERENT_USER_AUTHENTICATED; + debugLogger( + 'OAuth callback returned error: %s (%s)', + oauthError, + oauthErrorDescription ?? 'no description', + ); + let browserMessage: string; + let cliErrorMessage: string; + + if (isAlreadyLoggedIn) { + browserMessage = "You're already signed in. We'll continue for you. Return to the terminal."; + cliErrorMessage = `You're already signed in. Run "hd auth login" again to continue.`; + } else if (isDifferentUserAuthenticated) { + browserMessage = + "You're signed in with a different account than this sign-in attempt. Return to the terminal."; + cliErrorMessage = + `You're signed in with a different account than this sign-in attempt. ` + + `Choose another account, or reset this sign-in session and try again. ` + + `If needed, run "hd auth logout" and then "hd auth login".`; + } else { + browserMessage = "We couldn't complete sign-in. Return to the terminal and try again."; + cliErrorMessage = `We couldn't complete sign-in. Please run "hd auth login" again.`; + } + + res.writeHead(400, { 'Content-Type': 'text/plain' }); + res.end(browserMessage); + this.stopServer(); + return reject(new Error(cliErrorMessage)); + } + if (code) { res.writeHead(200, { 'Content-Type': 'text/plain' }); res.end('Login successful. You can close this window.'); diff --git a/src/config/constants.ts b/src/config/constants.ts index 954f5342..98f5f75e 100644 --- a/src/config/constants.ts +++ b/src/config/constants.ts @@ -18,6 +18,12 @@ export const ENABLE_USER_SETUP = false; // Trackers - Constants export const DEFAULT_TRACKER_RUN_DATA_FILE = 'data.json'; export const TRACKER_GIT_OUTPUT_FORMAT = `"${['%H', '%an', '%ad'].join('|')}"`; +export const OAUTH_CALLBACK_ERROR_CODES = { + ALREADY_LOGGED_IN: 'already_logged_in', + DIFFERENT_USER_AUTHENTICATED: 'different_user_authenticated', +} as const; + +export type OAuthCallbackErrorCode = (typeof OAUTH_CALLBACK_ERROR_CODES)[keyof typeof OAUTH_CALLBACK_ERROR_CODES]; let concurrentPageRequests = CONCURRENT_PAGE_REQUESTS; const parsed = Number.parseInt(process.env.CONCURRENT_PAGE_REQUESTS ?? '0', 10); diff --git a/test/commands/auth/login.test.ts b/test/commands/auth/login.test.ts index 89c6af1e..db5052a0 100644 --- a/test/commands/auth/login.test.ts +++ b/test/commands/auth/login.test.ts @@ -207,13 +207,73 @@ describe('AuthLogin', () => { expect(server.close).toHaveBeenCalledTimes(1); }); - it('rejects when the callback omits the authorization code', async () => { + it('rejects with guidance when callback returns already_logged_in', async () => { const command = createCommand(basePort + 3); const pendingCode = ( command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } ).startServerAndAwaitCode(authUrl, 'expected-state'); const server = getLatestServer(); + await flushAsync(); + const response = sendCallbackThroughStub({ error: 'already_logged_in', state: 'expected-state' }); + + expect(response.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'text/plain' }); + expect(response.end).toHaveBeenCalledWith( + "You're already signed in. We'll continue for you. Return to the terminal.", + ); + await expect(pendingCode).rejects.toThrow(`You're already signed in. Run "hd auth login" again to continue.`); + expect(server.close).toHaveBeenCalledTimes(1); + }); + + it('rejects when callback returns a generic OAuth error', async () => { + const command = createCommand(basePort + 4); + const pendingCode = ( + command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } + ).startServerAndAwaitCode(authUrl, 'expected-state'); + const server = getLatestServer(); + + await flushAsync(); + const response = sendCallbackThroughStub({ + error: 'access_denied', + error_description: 'User denied access', + state: 'expected-state', + }); + + expect(response.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'text/plain' }); + expect(response.end).toHaveBeenCalledWith("We couldn't complete sign-in. Return to the terminal and try again."); + await expect(pendingCode).rejects.toThrow(`We couldn't complete sign-in. Please run "hd auth login" again.`); + expect(server.close).toHaveBeenCalledTimes(1); + }); + + it('rejects with guidance when callback returns different_user_authenticated', async () => { + const command = createCommand(basePort + 5); + const pendingCode = ( + command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } + ).startServerAndAwaitCode(authUrl, 'expected-state'); + const server = getLatestServer(); + + await flushAsync(); + const response = sendCallbackThroughStub({ error: 'different_user_authenticated', state: 'expected-state' }); + + expect(response.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'text/plain' }); + expect(response.end).toHaveBeenCalledWith( + "You're signed in with a different account than this sign-in attempt. Return to the terminal.", + ); + await expect(pendingCode).rejects.toThrow( + `You're signed in with a different account than this sign-in attempt. ` + + `Choose another account, or reset this sign-in session and try again. ` + + `If needed, run "hd auth logout" and then "hd auth login".`, + ); + expect(server.close).toHaveBeenCalledTimes(1); + }); + + it('rejects when the callback omits the authorization code', async () => { + const command = createCommand(basePort + 6); + const pendingCode = ( + command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } + ).startServerAndAwaitCode(authUrl, 'expected-state'); + const server = getLatestServer(); + await flushAsync(); sendCallbackThroughStub({ state: 'expected-state' }); @@ -222,7 +282,7 @@ describe('AuthLogin', () => { }); it('rejects when the callback URL is invalid', async () => { - const command = createCommand(basePort + 4); + const command = createCommand(basePort + 7); const pendingCode = ( command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } ).startServerAndAwaitCode(authUrl, 'expected-state'); @@ -238,7 +298,7 @@ describe('AuthLogin', () => { }); it('returns a 400 response when the incoming request is missing a URL', async () => { - const command = createCommand(basePort + 4); + const command = createCommand(basePort + 8); const pendingCode = ( command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } ).startServerAndAwaitCode(authUrl, 'expected-state'); @@ -257,7 +317,7 @@ describe('AuthLogin', () => { }); it('responds with not found for unrelated paths', async () => { - const command = createCommand(basePort + 5); + const command = createCommand(basePort + 9); const pendingCode = ( command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } ).startServerAndAwaitCode(authUrl, 'expected-state'); @@ -276,7 +336,7 @@ describe('AuthLogin', () => { }); it('rejects when the local HTTP server emits an error', async () => { - const command = createCommand(basePort + 6); + const command = createCommand(basePort + 10); const pendingCode = ( command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise } ).startServerAndAwaitCode(authUrl, 'expected-state'); @@ -292,7 +352,7 @@ describe('AuthLogin', () => { it('warns and allows manual navigation when browser launch fails', async () => { openMock.mockRejectedValueOnce(new Error('browser unavailable')); - const command = createCommand(basePort + 7); + const command = createCommand(basePort + 11); const warnSpy = vi .spyOn(command as unknown as { warn: (...args: unknown[]) => unknown }, 'warn') .mockImplementation(() => {}); @@ -317,7 +377,7 @@ describe('AuthLogin', () => { }); it('deduplicates shutdown when callback success and server error race', async () => { - const command = createCommand(basePort + 8); + const command = createCommand(basePort + 12); const state = 'expected-state'; const pendingCode = ( command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise }