Skip to content
New issue

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

#

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

Already on GitHub? # to your account

fix(connection): getConnectionCredentials should return credentials even on error #2870

Merged
merged 4 commits into from
Oct 22, 2024
Merged

fix(connection): getConnectionCredentials should return credentials even on error #2870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6080b57
fix(connection): getConnectionCredentials should return credentials e…
bodinsamuel Oct 22, 2024
db355bf
better fix
bodinsamuel Oct 22, 2024
0a83295
Merge branch 'master' into sam/fix/get-connection-error
bodinsamuel Oct 22, 2024
6ab3ead
review
bodinsamuel Oct 22, 2024
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
25 changes: 21 additions & 4 deletions packages/server/lib/controllers/connection.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,29 @@ class ConnectionController {
metrics.increment(metrics.Types.GET_CONNECTION, 1, { accountId: account.id });
}

const credentialResponse = await connectionService.getConnectionCredentials({
const integration = await configService.getProviderConfig(providerConfigKey, environment.id);
if (!integration) {
res.status(404).send({
error: {
code: 'unknown_provider_config',
message:
'Provider config not found for the given provider config key. Please make sure the provider config exists in the Nango dashboard.'
}
});
return;
}

const connectionRes = await connectionService.getConnection(connectionId, providerConfigKey, environment.id);
if (connectionRes.error || !connectionRes.response) {
errorManager.errResFromNangoErr(res, connectionRes.error);
return;
}

const credentialResponse = await connectionService.refreshOrTestCredentials({
account,
environment,
connectionId,
providerConfigKey,
connection: connectionRes.response,
integration,
logContextGetter,
instantRefresh,
onRefreshSuccess: connectionRefreshSuccessHook,
Expand All @@ -60,7 +78,6 @@ class ConnectionController {

if (credentialResponse.isErr()) {
errorManager.errResFromNangoErr(res, credentialResponse.error);

return;
}

Expand Down
54 changes: 36 additions & 18 deletions packages/server/lib/controllers/proxy.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import querystring from 'querystring';
import type { AxiosError, AxiosRequestConfig, AxiosResponse } from 'axios';
import { backOff } from 'exponential-backoff';
import type { HTTP_VERB, UserProvidedProxyConfiguration, InternalProxyConfiguration, ApplicationConstructedProxyConfiguration, File } from '@nangohq/shared';
import { NangoError, LogActionEnum, errorManager, ErrorSourceEnum, proxyService, connectionService, configService, featureFlags } from '@nangohq/shared';
import { LogActionEnum, errorManager, ErrorSourceEnum, proxyService, connectionService, configService, featureFlags } from '@nangohq/shared';
import { metrics, getLogger, axiosInstance as axios, getHeaders } from '@nangohq/utils';
import { logContextGetter } from '@nangohq/logs';
import { connectionRefreshFailed as connectionRefreshFailedHook, connectionRefreshSuccess as connectionRefreshSuccessHook } from '../hooks/hooks.js';
Expand Down Expand Up @@ -84,11 +84,34 @@ class ProxyController {
retryOn
};

const credentialResponse = await connectionService.getConnectionCredentials({
const integration = await configService.getProviderConfig(providerConfigKey, environment.id);
if (!integration) {
await logCtx.error('Provider configuration not found');
await logCtx.failed();
metrics.increment(metrics.Types.PROXY_FAILURE);
res.status(404).send({
error: {
code: 'unknown_provider_config',
message:
'Provider config not found for the given provider config key. Please make sure the provider config exists in the Nango dashboard.'
}
});
return;
}

const connectionRes = await connectionService.getConnection(connectionId, providerConfigKey, environment.id);
if (connectionRes.error || !connectionRes.response) {
await logCtx.error('Failed to get connection', { error: connectionRes.error });
await logCtx.failed();
errorManager.errResFromNangoErr(res, connectionRes.error);
return;
}

const credentialResponse = await connectionService.refreshOrTestCredentials({
account,
environment,
connectionId,
providerConfigKey,
connection: connectionRes.response,
integration,
logContextGetter,
instantRefresh: false,
onRefreshSuccess: connectionRefreshSuccessHook,
Expand All @@ -99,32 +122,26 @@ class ProxyController {
await logCtx.error('Failed to get connection credentials', { error: credentialResponse.error });
await logCtx.failed();
metrics.increment(metrics.Types.PROXY_FAILURE);
throw new Error(`Failed to get connection credentials: '${credentialResponse.error.message}'`);
res.status(400).send({
error: { code: 'server_error', message: `Failed to get connection credentials: '${credentialResponse.error.message}'` }
});
return;
}

const { value: connection } = credentialResponse;

const providerConfig = await configService.getProviderConfig(providerConfigKey, environment.id);

if (!providerConfig) {
await logCtx.error('Provider configuration not found');
await logCtx.failed();
metrics.increment(metrics.Types.PROXY_FAILURE);

throw new NangoError('unknown_provider_config');
}
await logCtx.enrichOperation({
integrationId: providerConfig.id!,
integrationName: providerConfig.unique_key,
providerName: providerConfig.provider,
integrationId: integration.id!,
integrationName: integration.unique_key,
providerName: integration.provider,
connectionId: connection.id!,
connectionName: connection.connection_id
});

const internalConfig: InternalProxyConfiguration = {
existingActivityLogId: logCtx.id,
connection,
providerName: providerConfig.provider
providerName: integration.provider
};

const { success, error, response: proxyConfig, logs } = proxyService.configure(externalConfig, internalConfig);
Expand All @@ -143,6 +160,7 @@ class ProxyController {
errorManager.errResFromNangoErr(res, error);
await logCtx.failed();
metrics.increment(metrics.Types.PROXY_FAILURE);
res.status(400).send({ error: { code: 'server_error', message: 'failed to configure proxy' } });
return;
}

Expand Down
116 changes: 53 additions & 63 deletions packages/server/lib/controllers/v1/connection/get.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { z } from 'zod';
import { asyncWrapper } from '../../../utils/asyncWrapper.js';
import { requireEmptyBody, zodErrorToHTTP } from '@nangohq/utils';
import type { Connection, GetConnection, IntegrationConfig } from '@nangohq/types';
import type { GetConnection, IntegrationConfig } from '@nangohq/types';
import { connectionService, configService, errorNotificationService } from '@nangohq/shared';
import { connectionRefreshFailed as connectionRefreshFailedHook, connectionRefreshSuccess as connectionRefreshSuccessHook } from '../../../hooks/hooks.js';
import { logContextGetter } from '@nangohq/logs';
Expand Down Expand Up @@ -53,87 +53,77 @@ export const getConnection = asyncWrapper<GetConnection>(async (req, res) => {
const instantRefresh = force_refresh === 'true';
const { connectionId } = params;

const credentialResponse = await connectionService.getConnectionCredentials({
const integration: IntegrationConfig | null = await configService.getProviderConfig(providerConfigKey, environment.id);
if (!integration) {
res.status(404).send({
error: {
code: 'unknown_provider_config',
message: 'Provider config not found for the given provider config key. Please make sure the provider config exists in the Nango dashboard.'
}
});
return;
}

const connectionRes = await connectionService.getConnection(connectionId, providerConfigKey, environment.id);
if (connectionRes.error || !connectionRes.response) {
switch (connectionRes.error?.type) {
case 'missing_connection':
res.status(400).send({
error: { code: 'missing_connection', message: connectionRes.error.message }
});
break;
case 'missing_provider_config':
res.status(400).send({
error: { code: 'missing_provider_config', message: connectionRes.error.message }
});
break;
case 'unknown_connection':
res.status(404).send({
error: { code: 'unknown_connection', message: connectionRes.error.message }
});
break;
case 'unknown_provider_config':
res.status(404).send({
error: { code: 'unknown_provider_config', message: connectionRes.error.message }
});
break;
default:
res.status(500).send({ error: { code: 'server_error' } });
}
return;
}

const credentialResponse = await connectionService.refreshOrTestCredentials({
account,
environment,
connectionId,
providerConfigKey,
connection: connectionRes.response,
integration,
logContextGetter,
instantRefresh,
onRefreshSuccess: connectionRefreshSuccessHook,
onRefreshFailed: connectionRefreshFailedHook
});

if (credentialResponse.isErr()) {
if (credentialResponse.error.payload && credentialResponse.error.payload['id']) {
const errorConnection = credentialResponse.error.payload as unknown as Connection;
const errorLog = await errorNotificationService.auth.get(errorConnection.id as number);

res.status(400).send({
errorLog,
provider: null, // TODO: fix this
connection: errorConnection
});
} else {
switch (credentialResponse.error.type) {
case 'missing_connection':
res.status(400).send({
error: {
code: 'missing_connection',
message: credentialResponse.error.message
}
});
break;
case 'missing_provider_config':
res.status(400).send({
error: {
code: 'missing_provider_config',
message: credentialResponse.error.message
}
});
break;
case 'unknown_connection':
res.status(404).send({
error: {
code: 'unknown_connection',
message: credentialResponse.error.message
}
});
break;
case 'unknown_provider_config':
res.status(404).send({
error: {
code: 'unknown_provider_config',
message: credentialResponse.error.message
}
});
break;
}
}
return;
}

const { value: connection } = credentialResponse;
const errorLog = await errorNotificationService.auth.get(connectionRes.response.id as number);

const config: IntegrationConfig | null = await configService.getProviderConfig(connection.provider_config_key, environment.id);
// When we failed to refresh we still return a 200 because the connection is used in the UI
// Ultimately this could be a second endpoint so the UI displays faster and no confusion between error code
res.status(200).send({ errorLog, provider: integration.provider, connection: connectionRes.response });
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we return something else than 200 in case of error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thing is in the UI it's still used to display the page, before that we returned an error by did not respected that and still displayed the page. My take on this: it should be a totally different endpoint.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. can you add a comment?


if (!config) {
res.status(404).send({
error: {
code: 'unknown_provider_config',
message: 'Provider config not found for the given provider config key. Please make sure the provider config exists in the Nango dashboard.'
}
});
return;
}

const connection = credentialResponse.value;

if (instantRefresh) {
// If we force the refresh we also specifically log a success operation (we usually only log error)
const logCtx = await logContextGetter.create(
{ operation: { type: 'auth', action: 'refresh_token' } },
{
account,
environment,
integration: { id: config.id!, name: config.unique_key, provider: config.provider },
integration: { id: integration.id!, name: integration.unique_key, provider: integration.provider },
connection: { id: connection.id!, name: connection.connection_id }
}
);
Expand All @@ -142,7 +132,7 @@ export const getConnection = asyncWrapper<GetConnection>(async (req, res) => {
}

res.status(200).send({
provider: config.provider,
provider: integration.provider,
connection,
errorLog: null
});
Expand Down
17 changes: 3 additions & 14 deletions packages/server/lib/hooks/connection/post-connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { LogActionEnum, LogTypes, proxyService, connectionService, telemetry, ge
import * as postConnectionHandlers from './index.js';
import type { LogContext, LogContextGetter } from '@nangohq/logs';
import { stringifyError } from '@nangohq/utils';
import { connectionRefreshFailed as connectionRefreshFailedHook, connectionRefreshSuccess as connectionRefreshSuccessHook } from '../hooks.js';
import type { InternalProxyConfiguration } from '@nangohq/types';

type PostConnectionHandler = (internalNango: InternalNango) => Promise<void>;
Expand All @@ -23,22 +22,12 @@ async function execute(createdConnection: RecentlyCreatedConnection, providerNam
const { connection: upsertedConnection, environment, account } = createdConnection;
let logCtx: LogContext | undefined;
try {
const credentialResponse = await connectionService.getConnectionCredentials({
account,
environment,
connectionId: upsertedConnection.connection_id,
providerConfigKey: upsertedConnection.provider_config_key,
logContextGetter,
instantRefresh: false,
onRefreshSuccess: connectionRefreshSuccessHook,
onRefreshFailed: connectionRefreshFailedHook
});

if (credentialResponse.isErr()) {
const connectionRes = await connectionService.getConnection(upsertedConnection.connection_id, upsertedConnection.provider_config_key, environment.id);
if (connectionRes.error || !connectionRes.response) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On post connection I don't think we need to test again the credentials since we just fetched them, but maybe I forgot an edge case? @khaliqgant

Copy link
Member

Choose a reason for hiding this comment

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

No, should be fine to just use getConnection here

return;
}

const { value: connection } = credentialResponse;
const connection = connectionRes.response;

const internalConfig: InternalProxyConfiguration = {
connection,
Expand Down
17 changes: 9 additions & 8 deletions packages/server/lib/refreshConnections.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function refreshConnectionsCron(): void {
}

export async function exec(): Promise<void> {
return await tracer.trace<Promise<void>>('nango.server.cron.refreshConnections', async (span) => {
await tracer.trace<Promise<void>>('nango.server.cron.refreshConnections', async (span) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

exec is itself awaited above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it's just eslint best practice rules when you return void https://typescript-eslint.io/rules/no-confusing-void-expression/

let lock: Lock | undefined;
try {
logger.info(`${cronName} starting`);
Expand All @@ -56,21 +56,22 @@ export async function exec(): Promise<void> {
const limit = 1000;
// eslint-disable-next-line no-constant-condition
while (true) {
const staleConnections = await connectionService.getStaleConnections({ days: 0, limit, cursor });
const staleConnections = await connectionService.getStaleConnections({ days: 1, limit, cursor });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it was an error, let me know @TBonnin

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, that's a test leftover. Thank you for fixing

logger.info(`${cronName} found ${staleConnections.length} stale connections`);
for (const staleConnection of staleConnections) {
if (Date.now() - startTimestamp > ttlMs) {
logger.info(`${cronName} time limit reached, stopping`);
return;
}
const { connection_id, environment, provider_config_key, account } = staleConnection;
logger.info(`${cronName} refreshing connection '${connection_id}' for accountId '${account.id}'`);

const { connection, account, environment, integration } = staleConnection;
logger.info(`${cronName} refreshing connection '${connection.connection_id}' for accountId '${account.id}'`);
try {
const credentialResponse = await connectionService.getConnectionCredentials({
const credentialResponse = await connectionService.refreshOrTestCredentials({
account,
environment,
connectionId: connection_id,
providerConfigKey: provider_config_key,
integration,
connection,
logContextGetter,
instantRefresh: false,
onRefreshSuccess: connectionRefreshSuccessHook,
Expand All @@ -83,7 +84,7 @@ export async function exec(): Promise<void> {
metrics.increment(metrics.Types.REFRESH_CONNECTIONS_FAILED);
}
} catch (err) {
logger.error(`${cronName} failed to refresh connection '${connection_id}' ${stringifyError(err)}`);
logger.error(`${cronName} failed to refresh connection '${connection.connection_id}' ${stringifyError(err)}`);
metrics.increment(metrics.Types.REFRESH_CONNECTIONS_FAILED);
}
cursor = staleConnection.cursor;
Expand Down
Loading
Loading