Skip to content

Commit d28bbe6

Browse files
facundo-herodevsfacundo-herodevs
authored
refactor(auth): better UX for OAuth error messages (#479)
1 parent dd24b14 commit d28bbe6

3 files changed

Lines changed: 108 additions & 7 deletions

File tree

‎src/commands/auth/login.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { createInterface } from 'node:readline';
44
import { URL } from 'node:url';
55
import { Command } from '@oclif/core';
66
import { ensureUserSetup } from '../../api/user-setup.client.ts';
7+
import { OAUTH_CALLBACK_ERROR_CODES } from '../../config/constants.ts';
78
import { persistTokenResponse } from '../../service/auth.svc.ts';
89
import { getClientId, getRealmUrl } from '../../service/auth-config.svc.ts';
910
import { debugLogger, getErrorMessage } from '../../service/log.svc.ts';
@@ -80,6 +81,8 @@ export default class AuthLogin extends Command {
8081
if (parsedUrl.pathname === '/oauth2/callback') {
8182
const code = parsedUrl.searchParams.get('code');
8283
const state = parsedUrl.searchParams.get('state');
84+
const oauthError = parsedUrl.searchParams.get('error');
85+
const oauthErrorDescription = parsedUrl.searchParams.get('error_description');
8386

8487
if (!state) {
8588
res.writeHead(400, { 'Content-Type': 'text/plain' });
@@ -95,6 +98,38 @@ export default class AuthLogin extends Command {
9598
return reject(new Error('State verification failed'));
9699
}
97100

101+
if (oauthError) {
102+
const isAlreadyLoggedIn = oauthError === OAUTH_CALLBACK_ERROR_CODES.ALREADY_LOGGED_IN;
103+
const isDifferentUserAuthenticated = oauthError === OAUTH_CALLBACK_ERROR_CODES.DIFFERENT_USER_AUTHENTICATED;
104+
debugLogger(
105+
'OAuth callback returned error: %s (%s)',
106+
oauthError,
107+
oauthErrorDescription ?? 'no description',
108+
);
109+
let browserMessage: string;
110+
let cliErrorMessage: string;
111+
112+
if (isAlreadyLoggedIn) {
113+
browserMessage = "You're already signed in. We'll continue for you. Return to the terminal.";
114+
cliErrorMessage = `You're already signed in. Run "hd auth login" again to continue.`;
115+
} else if (isDifferentUserAuthenticated) {
116+
browserMessage =
117+
"You're signed in with a different account than this sign-in attempt. Return to the terminal.";
118+
cliErrorMessage =
119+
`You're signed in with a different account than this sign-in attempt. ` +
120+
`Choose another account, or reset this sign-in session and try again. ` +
121+
`If needed, run "hd auth logout" and then "hd auth login".`;
122+
} else {
123+
browserMessage = "We couldn't complete sign-in. Return to the terminal and try again.";
124+
cliErrorMessage = `We couldn't complete sign-in. Please run "hd auth login" again.`;
125+
}
126+
127+
res.writeHead(400, { 'Content-Type': 'text/plain' });
128+
res.end(browserMessage);
129+
this.stopServer();
130+
return reject(new Error(cliErrorMessage));
131+
}
132+
98133
if (code) {
99134
res.writeHead(200, { 'Content-Type': 'text/plain' });
100135
res.end('Login successful. You can close this window.');

‎src/config/constants.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ export const DEFAULT_DATE_COMMIT_MONTH_FORMAT = 'MMMM yyyy';
1616
// Trackers - Constants
1717
export const DEFAULT_TRACKER_RUN_DATA_FILE = 'data.json';
1818
export const TRACKER_GIT_OUTPUT_FORMAT = `"${['%H', '%an', '%ad'].join('|')}"`;
19+
export const OAUTH_CALLBACK_ERROR_CODES = {
20+
ALREADY_LOGGED_IN: 'already_logged_in',
21+
DIFFERENT_USER_AUTHENTICATED: 'different_user_authenticated',
22+
} as const;
23+
24+
export type OAuthCallbackErrorCode = (typeof OAUTH_CALLBACK_ERROR_CODES)[keyof typeof OAUTH_CALLBACK_ERROR_CODES];
1925

2026
let concurrentPageRequests = CONCURRENT_PAGE_REQUESTS;
2127
const parsed = Number.parseInt(process.env.CONCURRENT_PAGE_REQUESTS ?? '0', 10);

‎test/commands/auth/login.test.ts‎

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -209,13 +209,73 @@ describe('AuthLogin', () => {
209209
expect(server.close).toHaveBeenCalledTimes(1);
210210
});
211211

212-
it('rejects when the callback omits the authorization code', async () => {
212+
it('rejects with guidance when callback returns already_logged_in', async () => {
213213
const command = createCommand(basePort + 3);
214214
const pendingCode = (
215215
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
216216
).startServerAndAwaitCode(authUrl, 'expected-state');
217217
const server = getLatestServer();
218218

219+
await flushAsync();
220+
const response = sendCallbackThroughStub({ error: 'already_logged_in', state: 'expected-state' });
221+
222+
expect(response.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'text/plain' });
223+
expect(response.end).toHaveBeenCalledWith(
224+
"You're already signed in. We'll continue for you. Return to the terminal.",
225+
);
226+
await expect(pendingCode).rejects.toThrow(`You're already signed in. Run "hd auth login" again to continue.`);
227+
expect(server.close).toHaveBeenCalledTimes(1);
228+
});
229+
230+
it('rejects when callback returns a generic OAuth error', async () => {
231+
const command = createCommand(basePort + 4);
232+
const pendingCode = (
233+
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
234+
).startServerAndAwaitCode(authUrl, 'expected-state');
235+
const server = getLatestServer();
236+
237+
await flushAsync();
238+
const response = sendCallbackThroughStub({
239+
error: 'access_denied',
240+
error_description: 'User denied access',
241+
state: 'expected-state',
242+
});
243+
244+
expect(response.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'text/plain' });
245+
expect(response.end).toHaveBeenCalledWith("We couldn't complete sign-in. Return to the terminal and try again.");
246+
await expect(pendingCode).rejects.toThrow(`We couldn't complete sign-in. Please run "hd auth login" again.`);
247+
expect(server.close).toHaveBeenCalledTimes(1);
248+
});
249+
250+
it('rejects with guidance when callback returns different_user_authenticated', async () => {
251+
const command = createCommand(basePort + 5);
252+
const pendingCode = (
253+
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
254+
).startServerAndAwaitCode(authUrl, 'expected-state');
255+
const server = getLatestServer();
256+
257+
await flushAsync();
258+
const response = sendCallbackThroughStub({ error: 'different_user_authenticated', state: 'expected-state' });
259+
260+
expect(response.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'text/plain' });
261+
expect(response.end).toHaveBeenCalledWith(
262+
"You're signed in with a different account than this sign-in attempt. Return to the terminal.",
263+
);
264+
await expect(pendingCode).rejects.toThrow(
265+
`You're signed in with a different account than this sign-in attempt. ` +
266+
`Choose another account, or reset this sign-in session and try again. ` +
267+
`If needed, run "hd auth logout" and then "hd auth login".`,
268+
);
269+
expect(server.close).toHaveBeenCalledTimes(1);
270+
});
271+
272+
it('rejects when the callback omits the authorization code', async () => {
273+
const command = createCommand(basePort + 6);
274+
const pendingCode = (
275+
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
276+
).startServerAndAwaitCode(authUrl, 'expected-state');
277+
const server = getLatestServer();
278+
219279
await flushAsync();
220280
sendCallbackThroughStub({ state: 'expected-state' });
221281

@@ -224,7 +284,7 @@ describe('AuthLogin', () => {
224284
});
225285

226286
it('rejects when the callback URL is invalid', async () => {
227-
const command = createCommand(basePort + 4);
287+
const command = createCommand(basePort + 7);
228288
const pendingCode = (
229289
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
230290
).startServerAndAwaitCode(authUrl, 'expected-state');
@@ -240,7 +300,7 @@ describe('AuthLogin', () => {
240300
});
241301

242302
it('returns a 400 response when the incoming request is missing a URL', async () => {
243-
const command = createCommand(basePort + 4);
303+
const command = createCommand(basePort + 8);
244304
const pendingCode = (
245305
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
246306
).startServerAndAwaitCode(authUrl, 'expected-state');
@@ -259,7 +319,7 @@ describe('AuthLogin', () => {
259319
});
260320

261321
it('responds with not found for unrelated paths', async () => {
262-
const command = createCommand(basePort + 5);
322+
const command = createCommand(basePort + 9);
263323
const pendingCode = (
264324
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
265325
).startServerAndAwaitCode(authUrl, 'expected-state');
@@ -278,7 +338,7 @@ describe('AuthLogin', () => {
278338
});
279339

280340
it('rejects when the local HTTP server emits an error', async () => {
281-
const command = createCommand(basePort + 6);
341+
const command = createCommand(basePort + 10);
282342
const pendingCode = (
283343
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
284344
).startServerAndAwaitCode(authUrl, 'expected-state');
@@ -294,7 +354,7 @@ describe('AuthLogin', () => {
294354

295355
it('warns and allows manual navigation when browser launch fails', async () => {
296356
openMock.mockRejectedValueOnce(new Error('browser unavailable'));
297-
const command = createCommand(basePort + 7);
357+
const command = createCommand(basePort + 11);
298358
const warnSpy = vi
299359
.spyOn(command as unknown as { warn: (...args: unknown[]) => unknown }, 'warn')
300360
.mockImplementation(() => {});
@@ -319,7 +379,7 @@ describe('AuthLogin', () => {
319379
});
320380

321381
it('deduplicates shutdown when callback success and server error race', async () => {
322-
const command = createCommand(basePort + 8);
382+
const command = createCommand(basePort + 12);
323383
const state = 'expected-state';
324384
const pendingCode = (
325385
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }

0 commit comments

Comments
 (0)