[nan-638] add in credentials override and scopes override #1906
Conversation
| username: string; | ||
| password: string; | ||
| } | ||
|
|
||
| export interface ApiKeyCredentials { | ||
| [key: string]: string; |
There was a problem hiding this comment.
Isn't this going to break some things? Don't we have weird cases of API key auth where users need to pass in more fields?
There was a problem hiding this comment.
This is just the webapp types so it won't break anything that a user is doing.
| credentials: { | ||
| oauth_client_id: '${optionalOAuthClientId}', | ||
| oauth_client_secret: '${optionalOAuthClientSecret}' | ||
| }`; |
There was a problem hiding this comment.
display: it's missing a line break here (or in the snippet)
There was a problem hiding this comment.
In the display this is what I see which is expected
Can you clarify what the issue you're seeing is?
There was a problem hiding this comment.
it's a detail, } }) line 5
There was a problem hiding this comment.
Ah gotcha. Updated with 1eff44a:
| acc[key] = connectionConfig[key] as string; | ||
| } | ||
| return acc; | ||
| }, {}); |
There was a problem hiding this comment.
maybe it's just me but I have hard time reading reducer, I don't really understand what's going on
There was a problem hiding this comment.
I don't mind reduce personally but it would be way more readable in typescript if the initial value was the first parameter instead of the last one after the function. Sometimes I have a hard time understanding language creators/maintainers.
There was a problem hiding this comment.
I would have rather just done
delete connectionConfig.oauth_client_id;
delete connectionConfig.oauth_client_secret;
but eslint was yelling at me.
There was a problem hiding this comment.
ah yes I see, I think it's fair to disable some eslint errors when we don't find them relevant.
And if it's often the case, it's always good to revisit the list of activated rules 👌🏻
packages/frontend/lib/index.ts
Outdated
| @@ -99,9 +99,14 @@ export default class Nango { | |||
| public auth( | |||
| providerConfigKey: string, | |||
| connectionId: string, | |||
| options?: (ConnectionConfig | BasicApiCredentials | ApiKeyCredentials | AppStoreCredentials) & AuthOptions | |||
| options?: (ConnectionConfig | OptionalOAuthCredentials | BasicApiCredentials | ApiKeyCredentials | AppStoreCredentials) & AuthOptions | |||
There was a problem hiding this comment.
what about OverrideOAuthCredentials instead, to make it clear they would override the integration ones
There was a problem hiding this comment.
Opted for OAuthCredentialsOverride
| const credentials = connectionConfig.credentials; | ||
| if ('oauth_client_id' in credentials && 'oauth_client_secret' in credentials) { | ||
| query.push(`credentials[oauth_client_id]=${credentials.oauth_client_id}`); | ||
| query.push(`credentials[oauth_client_secret]=${credentials.oauth_client_secret}`); |
There was a problem hiding this comment.
same here what about making it clearer with override_oauth_client_id and override_oauth_client_secret keys?
There was a problem hiding this comment.
Don't think this is necessary here if the interface is OAuthCredentialsOverride as this is just what we push to our server so it is just an internal name
| acc[key] = connectionConfig[key] as string; | ||
| } | ||
| return acc; | ||
| }, {}); |
There was a problem hiding this comment.
I don't mind reduce personally but it would be way more readable in typescript if the initial value was the first parameter instead of the last one after the function. Sometimes I have a hard time understanding language creators/maintainers.
khaliqgant
deleted the
khaliq/nan-638-handle-netsuite-per-connection-oauth-flow
branch
Describe your changes
Allow a user to override oauth credentials and scopes. Show in the UI only for netsuite
Issue ticket number and link
NAN-638
Checklist before requesting a review (skip if just adding/editing APIs & templates)