Skip to content
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

feat: start new auth flow on 401 responses #339

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: start new auth flow on 401 responses #339

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
45 changes: 44 additions & 1 deletion src/commands/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Api } from '@neondatabase/api-client';
import axios from 'axios';

import {
existsSync,
mkdtempSync,
Expand All @@ -16,7 +17,7 @@ import { OAuth2Server } from 'oauth2-mock-server';
import * as authModule from '../auth';
import { test } from '../test_utils/fixtures';
import { startOauthServer } from '../test_utils/oauth_server';
import { authFlow, ensureAuth } from './auth';
import { authFlow, ensureAuth, deleteCredentials } from './auth';

vi.mock('open', () => ({ default: vi.fn((url: string) => axios.get(url)) }));
vi.mock('../pkg.ts', () => ({ default: { version: '0.0.0' } }));
Expand Down Expand Up @@ -254,3 +255,45 @@ describe('ensureAuth', () => {
expect(props.apiKey).toBe('new-token');
});
});

describe('deleteCredentials', () => {
let configDir = '';

beforeAll(() => {
configDir = mkdtempSync('test-config-delete');
});

afterAll(() => {
rmSync(configDir, { recursive: true });
});

test('should successfully delete credentials file', () => {
const credentialsPath = join(configDir, 'credentials.json');
writeFileSync(credentialsPath, 'test-content', { mode: 0o700 });

expect(existsSync(credentialsPath)).toBe(true);

deleteCredentials(configDir);

expect(existsSync(credentialsPath)).toBe(false);
});

test('should handle non-existent file gracefully', () => {
const nonExistentDir = mkdtempSync('test-config-nonexistent');

// Ensure the file doesn't exist
const credentialsPath = join(nonExistentDir, 'credentials.json');
if (existsSync(credentialsPath)) {
rmSync(credentialsPath);
}

expect(existsSync(credentialsPath)).toBe(false);

// Should not throw an error
expect(() => {
deleteCredentials(nonExistentDir);
}).not.toThrow();

rmSync(nonExistentDir, { recursive: true });
});
});
22 changes: 21 additions & 1 deletion src/commands/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { existsSync, readFileSync, writeFileSync } from 'node:fs';
import { existsSync, readFileSync, writeFileSync, unlinkSync } from 'node:fs';
import { join } from 'node:path';
import { createHash } from 'node:crypto';
import { TokenSet } from 'openid-client';
Expand Down Expand Up @@ -227,4 +227,24 @@ export const ensureAuth = async (
});
};

/**
* Deletes the credentials file at the specified path
* @param configDir Directory where credentials file is stored
*/
export const deleteCredentials = (configDir: string): void => {
const credentialsPath = join(configDir, CREDENTIALS_FILE);
try {
if (existsSync(credentialsPath)) {
unlinkSync(credentialsPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to use rmSync just like everywhere else.

log.info('Deleted credentials from %s', credentialsPath);
} else {
log.debug('Credentials file %s does not exist', credentialsPath);
}
} catch (err) {
const typedErr = err instanceof Error ? err : new Error('Unknown error');
log.error('Failed to delete credentials: %s', typedErr.message);
throw new Error('CREDENTIALS_DELETE_FAILED');
}
};

const md5hash = (s: string) => createHash('md5').update(s).digest('hex');
1 change: 1 addition & 0 deletions src/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export type ErrorCode =
| 'API_ERROR'
| 'UNKNOWN_COMMAND'
| 'MISSING_ARGUMENT'
| 'CREDENTIALS_DELETE_FAILED'
| 'UNKNOWN_ERROR';

const ERROR_MATCHERS = [
Expand Down
98 changes: 64 additions & 34 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ axiosDebug({
});
import { Api } from '@neondatabase/api-client';

import { ensureAuth } from './commands/auth.js';
import { ensureAuth, deleteCredentials } from './commands/auth.js';
import { defaultDir, ensureConfigDir } from './config.js';
import { log } from './log.js';
import { defaultClientID } from './auth.js';
Expand Down Expand Up @@ -160,45 +160,67 @@ builder = builder
'For more information, visit https://neon.tech/docs/reference/neon-cli',
)
.wrap(null)
.fail(async (msg, err) => {
if (process.argv.some((arg) => arg === '--help' || arg === '-h')) {
await showHelp(builder);
process.exit(0);
}
.fail(false);

async function handleError(msg: string, err: unknown): Promise<boolean> {
if (process.argv.some((arg) => arg === '--help' || arg === '-h')) {
await showHelp(builder);
process.exit(0);
}

// Log stack trace if available
if (err instanceof Error && err.stack) {
log.debug('Stack: %s', err.stack);
}

if (isAxiosError(err)) {
if (err.code === 'ECONNABORTED') {
log.error('Request timed out');
sendError(err, 'REQUEST_TIMEOUT');
} else if (err.response?.status === 401) {
sendError(err, 'AUTH_FAILED');
log.error('Authentication failed, please run `neonctl auth`');
} else {
if (err.response?.data?.message) {
log.error(err.response?.data?.message);
}
if (isAxiosError(err)) {
if (err.code === 'ECONNABORTED') {
log.error('Request timed out');
sendError(err, 'REQUEST_TIMEOUT');
return false;
} else if (err.response?.status === 401) {
sendError(err, 'AUTH_FAILED');
log.info('Authentication failed, deleting credentials...');
try {
deleteCredentials(defaultDir);
return true; // Allow retry for auth failures
} catch (deleteErr) {
log.debug(
'status: %d %s | path: %s',
err.response?.status,
err.response?.statusText,
err.request?.path,
'Failed to delete credentials: %s',
deleteErr instanceof Error ? deleteErr.message : 'unknown error',
);
sendError(err, 'API_ERROR');
return false;
}
} else {
sendError(err || new Error(msg), matchErrorCode(msg || err?.message));
log.error(msg || err?.message);
}
await closeAnalytics();
if (err?.stack) {
log.debug('Stack: %s', err.stack);
if (err.response?.data?.message) {
log.error(err.response?.data?.message);
}
log.debug(
'status: %d %s | path: %s',
err.response?.status,
err.response?.statusText,
err.request?.path,
);
sendError(err, 'API_ERROR');
return false;
}
process.exit(1);
});
} else {
const error =
err instanceof Error ? err : new Error(msg || 'Unknown error');
sendError(error, matchErrorCode(error.message));
log.error(error.message);
return false;
}
}

void (async () => {
// Main loop with max 2 attempts (initial + 1 retry):
let attempts = 0;
const MAX_ATTEMPTS = 2;

while (attempts < MAX_ATTEMPTS) {
try {
const args = await builder.argv;
// Send analytics for a successful attempt
trackEvent('cli_command_success', {
...getAnalyticsEventProperties(args),
projectId: args.projectId,
Expand All @@ -208,8 +230,16 @@ void (async () => {
await showHelp(builder);
process.exit(0);
}

await closeAnalytics();
} catch {
// noop
process.exit(0);
} catch (err) {
attempts++;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we should show number of attempts to the user somewhere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an implementation detail and not for retrying networking requests, that's why I think having a counter is unnecessary at this point. We may handle it differently in the future 🤷

Let me explain why it's there. Maybe it's not very clear from the PR.

If a user had invalid credentials:

  • ensureAuth would not error out because credentials are present on the file system
  • the execution would go to the command itself, for example, to project list
  • it'd raise an error due to 401 and yargs would call exit 1 under the hood because it has .fail(async (msg, err) => { and the rest of the error handling

This design makes it impossible to re-enter the command execution if it fails.
So, I had to change the design.

First, I removed the error handler from yargs and extracted it into a separate function called handleError.
Second, I removed the error handler from yargs with this line: .fail(false);

From the doc:

Providing false as a value for fn can be used to prevent yargs from exiting and printing a failure message. This is useful if you wish to handle failures yourself using try/catch and .getHelp().

Finally, third, I had to introduce a loop to call const args = await builder.argv;, call the handleError that handles authentication on auth error, does many other things, and indicates that the command can be retried. In our case, this is because handleError will delete invalid credentials, and on the next retry ensureAuth from the yargs pipeline will call const apiKey = await authFlow(props);.

Phew, I think I didn't forget anything.

const shouldRetry = await handleError('', err);
if (!shouldRetry || attempts >= MAX_ATTEMPTS) {
await closeAnalytics();
process.exit(1);
}
// If shouldRetry is true and we haven't hit max attempts, loop continues
}
})();
}