-
Notifications
You must be signed in to change notification settings - Fork 464
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.
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(webhooks): send end user on connection creation #3065
Conversation
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
8205899 | Triggered | Generic Password | 533ea50 | packages/database/lib/getDbConfig.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
packages/database/lib/getDbConfig.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a leftover
// account?: DBTeam; | ||
// environment?: DBEnvironment; | ||
// user: Pick<DBUser, 'id' | 'email' | 'name'>; | ||
// }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to fix this type but opened a trap door (unfortunately I have wrote this type so I can only blame myself)
Will fix later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I follow but I trust you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is too loose meaning we can have connectSession with auth mode basic, which could lead to mistakes. Ideally, we have something stricter but when I wrote it I didn't anticipate this issue and now it seems some middleware does not enjoy the strictness when I tried to change it
@@ -98,7 +98,8 @@ async function handleCreateWebhook(integration: ProviderConfig, body: any, logCo | |||
environment, | |||
account, | |||
auth_mode: 'APP', | |||
operation: res.operation | |||
operation: res.operation, | |||
endUser: undefined // TODO fix this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github App webhook routing is weird, but it's an edge case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -156,7 +157,8 @@ class ApiAuthController { | |||
environment, | |||
account, | |||
auth_mode: 'API_KEY', | |||
operation: updatedConnection.operation | |||
operation: updatedConnection.operation, | |||
endUser: isConnectSession ? res.locals['endUser'] : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not directly res.locals['endUser']
? is it not undefined
if not isConnectSession
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, I think I like the explicitness here and if (big if) I manage to fix the locals types endUser
will be never in most case.
const { providerConfigKey } = req.params; | ||
const receivedConnectionId = req.query['connection_id'] as string | undefined; | ||
const connectionConfig = req.query['params'] != null ? getConnectionConfig(req.query['params']) : {}; | ||
const isConnectSession = res.locals['authType'] === 'connectSession'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isConnectSession
could almost be an attribute of RequestLocals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe indeed
// account?: DBTeam; | ||
// environment?: DBEnvironment; | ||
// user: Pick<DBUser, 'id' | 'email' | 'name'>; | ||
// }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I follow but I trust you
🤓☝️ Changes
Fixes https://linear.app/nango/issue/NAN-1880/send-back-the-end-user-profile-in-the-webhook
endUser
in webhook on connection creationWhen we use a session token, we pass metadata, but when we receive a webhook, we get a random connectionId that is impossible to link to an actual user. Sending back the userId (or orgId) will allow customers to link this connectionId to an actual user without needing to do any additional work.
NB: we don't send it in any other webhook since the connectionId should have been linked already
🧪 How to tests?