-
Notifications
You must be signed in to change notification settings - Fork 52.3k
feat: Support dynamic credentials in oauth refresh #23225
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
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
|
E2E Tests: n8n tests passed after 8m 34.3s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
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.
2 issues found across 3 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/src/credentials-helper.ts">
<violation number="1" location="packages/cli/src/credentials-helper.ts:549">
P2: Rule violated: **Prefer Typeguards over Type casting**
Unnecessary type cast using `as`. The `toCredentialContext` function already returns a properly typed `ICredentialContext`. Instead of casting, use the `ICredentialContext` type directly or import and use `ICredentialContextV1` from 'n8n-workflow' if the specific V1 shape is needed.</violation>
</file>
<file name="packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts">
<violation number="1" location="packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts:1633">
P2: Rule violated: **Prefer Typeguards over Type casting**
Use a type guard instead of `as` for type narrowing. Since `contentBody` has type `Exclude<IN8nHttpResponse, Buffer>`, using `contentBody as string` violates the rule against using `as` for type narrowing. Consider adding a type guard check or refactoring the variable declaration to have the correct type from assignment.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts
Outdated
Show resolved
Hide resolved
guillaumejacquart
left a comment
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 to me
Summary
This PR allows dynamic credentials resolvers to be used for storing a refreshed token assuming the credential is resolver capable and has a resolver id.
This allows refreshing of tokens, and also avoids the risk of a dynamic credential overwriting static credential data
Related Linear tickets, Github issues, and Community forum posts
PAY-4235
PAY-4317
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)