Skip to content

refactor(auth): better UX for OAuth error messages #479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
facundo-herodevs wants to merge 1 commit into
base: main
Choose a base branch
from
+108 −7
Open

refactor(auth): better UX for OAuth error messages #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 35 additions & 0 deletions src/commands/auth/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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' });
Expand All @@ -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.');
Expand Down
6 changes: 6 additions & 0 deletions src/config/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
74 changes: 67 additions & 7 deletions test/commands/auth/login.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string> }
).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<string> }
).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<string> }
).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<string> }
).startServerAndAwaitCode(authUrl, 'expected-state');
const server = getLatestServer();

await flushAsync();
sendCallbackThroughStub({ state: 'expected-state' });

Expand All @@ -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<string> }
).startServerAndAwaitCode(authUrl, 'expected-state');
Expand All @@ -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<string> }
).startServerAndAwaitCode(authUrl, 'expected-state');
Expand All @@ -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<string> }
).startServerAndAwaitCode(authUrl, 'expected-state');
Expand All @@ -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<string> }
).startServerAndAwaitCode(authUrl, 'expected-state');
Expand All @@ -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(() => {});
Expand All @@ -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<string> }
Expand Down