fix(connection): getConnectionCredentials should return credentials even on error #2870
Conversation
| @@ -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 }); | |||
There was a problem hiding this comment.
I think it was an error, let me know @TBonnin
There was a problem hiding this comment.
yes, that's a test leftover. Thank you for fixing
|
|
||
| if (credentialResponse.isErr()) { | ||
| const connectionRes = await connectionService.getConnection(upsertedConnection.connection_id, upsertedConnection.provider_config_key, environment.id); | ||
| if (connectionRes.error || !connectionRes.response) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
No, should be fine to just use getConnection here
| @@ -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) => { | |||
There was a problem hiding this comment.
exec is itself awaited above.
There was a problem hiding this comment.
yes, it's just eslint best practice rules when you return void https://typescript-eslint.io/rules/no-confusing-void-expression/
| if (!provider) { | ||
| const error = new NangoError('unknown_provider_config'); | ||
| return Err(error); | ||
| } | ||
|
|
||
| const copy = { ...connection }; |
There was a problem hiding this comment.
it's modifying the credentials by reference since we don't fetch anymore inside, it's best practice to copy (even though it's a shallow copy)
| } | ||
| } | ||
| const errorLog = await errorNotificationService.auth.get(connectionRes.response.id as number); | ||
| res.status(200).send({ errorLog, provider: integration.provider, connection: connectionRes.response }); |
There was a problem hiding this comment.
should we return something else than 200 in case of error?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok. can you add a comment?
…on (#3472) ## Changes Noticed something weird while improving the type for Connection in #3470 - Automatic refresh token was not passing decrypted connection When doing this refactor #2870 I split the refresh and the getConnection because it was inefficient. I forgot to decrypt the connection, and due to bad typing, there was no difference between Connection and (decrypted)Connection. The really permissive code completely allowed this to pass, and still mark connection has refreshed while not being refreshed. NB: this was not impacting the regular refresh on fetch
Describe your changes
Fixes https://linear.app/nango/issue/NAN-1933/fix-ui-crash-when-loading-a-connection-with-token-refresh-error
For the /connections/:id endpoint we still want to return the connection even if the refresh failed but if there was an error we stripped the credentials to avoid leaking stuff. Initially, I had just reverted this and stripped the credentials outside of the function at multiple places but the code was even more crappier.
So I split the function, this allows us to not repeat call to fetch the integration on the API and correctly output the errors. It's a bit less straightforward but more granular