Skip to content

Commit 2121def

Browse files
eduardoRotheduardoRoth
authored
fix(auth): update code/token generation from CLI and redirect to successful page (#518)
* fix(auth): env variables not used * chore(auth): add new env variable for CLI login completed page * feat(auth): refactor auth generation to wait for tokens and redirect to `cli-log-in` page * fix(auth): fix tests * fix(auth): fix undefined during tests * chore(auth): remove unused async * fix(auth): update EOL_LOG_IN_URL to right URL * fix(auth): typo * chore(auth): remove constants in favor of using config object
1 parent 8569ec1 commit 2121def

4 files changed

Lines changed: 103 additions & 62 deletions

File tree

‎.envrc.example‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ export GRAPHQL_HOST='https://api.nes.herodevs.com';
55
export GRAPHQL_PATH='/graphql';
66
export EOL_REPORT_URL='https://eol-report-card.apps.herodevs.com/reports';
77
export ANALYTICS_URL='https://eol-api.herodevs.com/track';
8+
export EOL_LOG_IN_URL='https://apps.herodevs.com/eol/api/auth/cli-log-in';
89

910
# OAuth (for hd auth login)
1011
export OAUTH_CONNECT_URL='';

‎src/commands/auth/login.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +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';
7+
import { config, OAUTH_CALLBACK_ERROR_CODES } from '../../config/constants.ts';
88
import { refreshIdentityFromStoredToken } from '../../service/analytics.svc.ts';
99
import { persistTokenResponse } from '../../service/auth.svc.ts';
1010
import { getClientId, getRealmUrl } from '../../service/auth-config.svc.ts';
@@ -40,8 +40,7 @@ export default class AuthLogin extends Command {
4040
`&code_challenge_method=S256` +
4141
`&state=${state}`;
4242

43-
const code = await this.startServerAndAwaitCode(authUrl, state);
44-
const token = await this.exchangeCodeForToken(code, codeVerifier);
43+
const token = await this.startServerAndAwaitToken(authUrl, state, codeVerifier);
4544

4645
try {
4746
await persistTokenResponse(token);
@@ -65,7 +64,11 @@ export default class AuthLogin extends Command {
6564
this.log('\nLogin completed successfully.');
6665
}
6766

68-
private startServerAndAwaitCode(authUrl: string, expectedState: string): Promise<string> {
67+
private startServerAndAwaitToken(
68+
authUrl: string,
69+
expectedState: string,
70+
codeVerifier: string,
71+
): Promise<TokenResponse> {
6972
return new Promise((resolve, reject) => {
7073
this.server = http.createServer((req, res) => {
7174
if (!req.url) {
@@ -138,10 +141,18 @@ export default class AuthLogin extends Command {
138141
}
139142

140143
if (code) {
141-
res.writeHead(200, { 'Content-Type': 'text/plain' });
142-
res.end('Login successful. You can close this window.');
143-
this.stopServer();
144-
resolve(code);
144+
this.exchangeCodeForToken(code, codeVerifier)
145+
.then((token) => {
146+
res.writeHead(302, {
147+
Location: `${config.eolLogInUrl}`,
148+
});
149+
res.end();
150+
resolve(token);
151+
})
152+
.catch((error) => reject(error))
153+
.finally(() => {
154+
this.stopServer();
155+
});
145156
} else {
146157
res.writeHead(400, { 'Content-Type': 'text/plain' });
147158
res.end('No authorization code returned. Please try again.');

‎src/config/constants.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,3 @@
1-
export const EOL_REPORT_URL = 'https://apps.herodevs.com/eol/reports';
2-
export const GRAPHQL_HOST = 'https://gateway.prod.apps.herodevs.io';
3-
export const GRAPHQL_PATH = '/graphql';
4-
export const ANALYTICS_URL = 'https://apps.herodevs.com/api/eol/track';
51
export const CONCURRENT_PAGE_REQUESTS = 3;
62
export const PAGE_SIZE = 500;
73
export const GIT_OUTPUT_FORMAT = `"${['%h', '%an', '%ad'].join('|')}"`;
@@ -34,10 +30,11 @@ if (parsedPageSize > 0) {
3430
}
3531

3632
export const config = {
37-
eolReportUrl: process.env.EOL_REPORT_URL || EOL_REPORT_URL,
38-
graphqlHost: process.env.GRAPHQL_HOST || GRAPHQL_HOST,
39-
graphqlPath: process.env.GRAPHQL_PATH || GRAPHQL_PATH,
40-
analyticsUrl: process.env.ANALYTICS_URL || ANALYTICS_URL,
33+
eolReportUrl: process.env.EOL_REPORT_URL || 'https://apps.herodevs.com/eol/reports',
34+
graphqlHost: process.env.GRAPHQL_HOST || 'https://gateway.prod.apps.herodevs.io',
35+
graphqlPath: process.env.GRAPHQL_PATH || '/graphql',
36+
analyticsUrl: process.env.ANALYTICS_URL || 'https://apps.herodevs.com/api/eol/track',
37+
eolLogInUrl: process.env.EOL_LOG_IN_URL || 'https://apps.herodevs.com/eol/api/auth/cli-log-in',
4138
concurrentPageRequests,
4239
pageSize,
4340
ciTokenFromEnv: process.env.HD_CI_CREDENTIAL?.trim() || undefined,

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

Lines changed: 78 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { ensureUserSetup } from '../../../src/api/user-setup.client.ts';
44
import AuthLogin from '../../../src/commands/auth/login.ts';
55
import { refreshIdentityFromStoredToken } from '../../../src/service/analytics.svc.ts';
66
import { persistTokenResponse } from '../../../src/service/auth.svc.ts';
7+
import type { TokenResponse } from '../../../src/types/auth.ts';
78
import { openInBrowser } from '../../../src/utils/open-in-browser.ts';
89

910
type ServerRequest = { url?: string };
@@ -167,22 +168,30 @@ describe('AuthLogin', () => {
167168
persistTokenResponseMock.mockClear();
168169
});
169170

170-
describe('startServerAndAwaitCode', () => {
171+
describe('startServerAndAwaitToken', () => {
171172
const authUrl = 'https://login.example/auth';
172173
const basePort = 4900;
173174

174-
it('resolves with the authorization code when the callback is valid', async () => {
175+
it('resolves with the token response when the callback is valid', async () => {
175176
const command = createCommand(basePort);
176177
const state = 'expected-state';
177-
const pendingCode = (
178-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
179-
).startServerAndAwaitCode(authUrl, state);
178+
const codeVerifier = 'verifier-123';
179+
180+
const commandWithInternals = command as unknown as {
181+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<TokenResponse>;
182+
exchangeCodeForToken: (...args: unknown[]) => Promise<TokenResponse>;
183+
};
184+
const tokenResponse = { access_token: 'access', refresh_token: 'refresh' };
185+
vi.spyOn(commandWithInternals, 'exchangeCodeForToken').mockResolvedValue(tokenResponse);
186+
187+
const pendingCode = commandWithInternals.startServerAndAwaitToken(authUrl, state, codeVerifier);
188+
180189
const server = getLatestServer();
181190

182191
await flushAsync();
183192
sendCallbackThroughStub({ code: 'test-code', state });
184193

185-
await expect(pendingCode).resolves.toBe('test-code');
194+
await expect(pendingCode).resolves.toBe(tokenResponse);
186195
expect(questionMock).toHaveBeenCalledWith(expect.stringContaining(authUrl), expect.any(Function));
187196
expect(closeMock).toHaveBeenCalledTimes(1);
188197
expect(openMock).toHaveBeenCalledWith(authUrl);
@@ -192,8 +201,10 @@ describe('AuthLogin', () => {
192201
it('rejects when the callback is missing the state parameter', async () => {
193202
const command = createCommand(basePort + 1);
194203
const pendingCode = (
195-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
196-
).startServerAndAwaitCode(authUrl, 'expected-state');
204+
command as unknown as {
205+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
206+
}
207+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
197208
const server = getLatestServer();
198209

199210
await flushAsync();
@@ -206,8 +217,10 @@ describe('AuthLogin', () => {
206217
it('rejects when the callback state does not match', async () => {
207218
const command = createCommand(basePort + 2);
208219
const pendingCode = (
209-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
210-
).startServerAndAwaitCode(authUrl, 'expected-state');
220+
command as unknown as {
221+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
222+
}
223+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
211224
const server = getLatestServer();
212225

213226
await flushAsync();
@@ -220,8 +233,10 @@ describe('AuthLogin', () => {
220233
it('rejects with guidance when callback returns already_logged_in', async () => {
221234
const command = createCommand(basePort + 3);
222235
const pendingCode = (
223-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
224-
).startServerAndAwaitCode(authUrl, 'expected-state');
236+
command as unknown as {
237+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
238+
}
239+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
225240
const server = getLatestServer();
226241

227242
await flushAsync();
@@ -238,8 +253,10 @@ describe('AuthLogin', () => {
238253
it('rejects when callback returns a generic OAuth error', async () => {
239254
const command = createCommand(basePort + 4);
240255
const pendingCode = (
241-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
242-
).startServerAndAwaitCode(authUrl, 'expected-state');
256+
command as unknown as {
257+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
258+
}
259+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
243260
const server = getLatestServer();
244261

245262
await flushAsync();
@@ -258,8 +275,10 @@ describe('AuthLogin', () => {
258275
it('rejects with guidance when callback returns different_user_authenticated', async () => {
259276
const command = createCommand(basePort + 5);
260277
const pendingCode = (
261-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
262-
).startServerAndAwaitCode(authUrl, 'expected-state');
278+
command as unknown as {
279+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
280+
}
281+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
263282
const server = getLatestServer();
264283

265284
await flushAsync();
@@ -280,8 +299,10 @@ describe('AuthLogin', () => {
280299
it('rejects when the callback omits the authorization code', async () => {
281300
const command = createCommand(basePort + 6);
282301
const pendingCode = (
283-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
284-
).startServerAndAwaitCode(authUrl, 'expected-state');
302+
command as unknown as {
303+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
304+
}
305+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
285306
const server = getLatestServer();
286307

287308
await flushAsync();
@@ -294,8 +315,10 @@ describe('AuthLogin', () => {
294315
it('rejects when the callback URL is invalid', async () => {
295316
const command = createCommand(basePort + 7);
296317
const pendingCode = (
297-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
298-
).startServerAndAwaitCode(authUrl, 'expected-state');
318+
command as unknown as {
319+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
320+
}
321+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
299322
const server = getLatestServer();
300323

301324
await flushAsync();
@@ -310,8 +333,10 @@ describe('AuthLogin', () => {
310333
it('returns a 400 response when the incoming request is missing a URL', async () => {
311334
const command = createCommand(basePort + 8);
312335
const pendingCode = (
313-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
314-
).startServerAndAwaitCode(authUrl, 'expected-state');
336+
command as unknown as {
337+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
338+
}
339+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
315340
const server = getLatestServer();
316341

317342
await flushAsync();
@@ -329,8 +354,10 @@ describe('AuthLogin', () => {
329354
it('responds with not found for unrelated paths', async () => {
330355
const command = createCommand(basePort + 9);
331356
const pendingCode = (
332-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
333-
).startServerAndAwaitCode(authUrl, 'expected-state');
357+
command as unknown as {
358+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
359+
}
360+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
334361
const server = getLatestServer();
335362

336363
await flushAsync();
@@ -348,8 +375,10 @@ describe('AuthLogin', () => {
348375
it('rejects when the local HTTP server emits an error', async () => {
349376
const command = createCommand(basePort + 10);
350377
const pendingCode = (
351-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
352-
).startServerAndAwaitCode(authUrl, 'expected-state');
378+
command as unknown as {
379+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<string>;
380+
}
381+
).startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
353382
const server = getLatestServer();
354383

355384
await flushAsync();
@@ -369,17 +398,23 @@ describe('AuthLogin', () => {
369398
const state = 'expected-state';
370399

371400
try {
372-
const pendingCode = (
373-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
374-
).startServerAndAwaitCode(authUrl, state);
401+
const commandWithInternals = command as unknown as {
402+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<TokenResponse>;
403+
exchangeCodeForToken: (...args: unknown[]) => Promise<TokenResponse>;
404+
};
405+
const tokenResponse = { access_token: 'access', refresh_token: 'refresh' };
406+
vi.spyOn(commandWithInternals, 'exchangeCodeForToken').mockResolvedValue(tokenResponse);
407+
408+
const pendingCode = commandWithInternals.startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
409+
375410
const server = getLatestServer();
376411

377412
await flushAsync();
378413

379414
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Failed to open browser automatically'));
380415

381416
sendCallbackThroughStub({ code: 'manual-code', state });
382-
await expect(pendingCode).resolves.toBe('manual-code');
417+
await expect(pendingCode).resolves.toBe(tokenResponse);
383418
expect(server.close).toHaveBeenCalledTimes(1);
384419
} finally {
385420
warnSpy.mockRestore();
@@ -389,9 +424,12 @@ describe('AuthLogin', () => {
389424
it('deduplicates shutdown when callback success and server error race', async () => {
390425
const command = createCommand(basePort + 12);
391426
const state = 'expected-state';
392-
const pendingCode = (
393-
command as unknown as { startServerAndAwaitCode: (url: string, state: string) => Promise<string> }
394-
).startServerAndAwaitCode(authUrl, state);
427+
428+
const commandWithInternals = command as unknown as {
429+
startServerAndAwaitToken: (url: string, state: string, codeVerifier: string) => Promise<TokenResponse>;
430+
};
431+
const pendingCode = commandWithInternals.startServerAndAwaitToken(authUrl, 'expected-state', 'code-verifier');
432+
395433
const server = getLatestServer();
396434
const warnSpy = vi
397435
.spyOn(command as unknown as { warn: (...args: unknown[]) => unknown }, 'warn')
@@ -402,7 +440,7 @@ describe('AuthLogin', () => {
402440
sendCallbackThroughStub({ code: 'race-code', state });
403441
server.emitError(new Error('late listener error'));
404442

405-
await expect(pendingCode).resolves.toBe('race-code');
443+
await expect(pendingCode).rejects.toThrow('late listener error');
406444
expect(server.close).toHaveBeenCalledTimes(1);
407445
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('Failed to stop local OAuth callback server'));
408446
} finally {
@@ -470,11 +508,9 @@ describe('AuthLogin', () => {
470508
const command = createCommand(6000);
471509
const tokenResponse = { access_token: 'access', refresh_token: 'refresh' };
472510
const commandWithInternals = command as unknown as {
473-
startServerAndAwaitCode: (...args: unknown[]) => Promise<string>;
474-
exchangeCodeForToken: (...args: unknown[]) => Promise<unknown>;
511+
startServerAndAwaitToken: (...args: unknown[]) => Promise<unknown>;
475512
};
476-
vi.spyOn(commandWithInternals, 'startServerAndAwaitCode').mockResolvedValue('code-123');
477-
vi.spyOn(commandWithInternals, 'exchangeCodeForToken').mockResolvedValue(tokenResponse);
513+
vi.spyOn(commandWithInternals, 'startServerAndAwaitToken').mockResolvedValue(tokenResponse);
478514

479515
await command.run();
480516

@@ -488,11 +524,9 @@ describe('AuthLogin', () => {
488524
const command = createCommand(6001);
489525
const tokenResponse = { access_token: 'access', refresh_token: 'refresh' };
490526
const commandWithInternals = command as unknown as {
491-
startServerAndAwaitCode: (...args: unknown[]) => Promise<string>;
492-
exchangeCodeForToken: (...args: unknown[]) => Promise<unknown>;
527+
startServerAndAwaitToken: (...args: unknown[]) => Promise<unknown>;
493528
};
494-
vi.spyOn(commandWithInternals, 'startServerAndAwaitCode').mockResolvedValue('code-123');
495-
vi.spyOn(commandWithInternals, 'exchangeCodeForToken').mockResolvedValue(tokenResponse);
529+
vi.spyOn(commandWithInternals, 'startServerAndAwaitToken').mockResolvedValue(tokenResponse);
496530

497531
await command.run();
498532

@@ -506,11 +540,9 @@ describe('AuthLogin', () => {
506540
const command = createCommand(6002);
507541
const tokenResponse = { access_token: 'access', refresh_token: 'refresh' };
508542
const commandWithInternals = command as unknown as {
509-
startServerAndAwaitCode: (...args: unknown[]) => Promise<string>;
510-
exchangeCodeForToken: (...args: unknown[]) => Promise<unknown>;
543+
startServerAndAwaitToken: (...args: unknown[]) => Promise<unknown>;
511544
};
512-
vi.spyOn(commandWithInternals, 'startServerAndAwaitCode').mockResolvedValue('code-123');
513-
vi.spyOn(commandWithInternals, 'exchangeCodeForToken').mockResolvedValue(tokenResponse);
545+
vi.spyOn(commandWithInternals, 'startServerAndAwaitToken').mockResolvedValue(tokenResponse);
514546

515547
await expect(command.run()).rejects.toThrow('User setup failed');
516548
expect(refreshIdentityFromStoredTokenMock).not.toHaveBeenCalled();

0 commit comments

Comments
 (0)