feat (public-api): update credentials - #18082
Conversation
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
3 issues found across 9 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| return next(); | ||
| }; | ||
|
|
||
| export const validCredentialUpdate = ( |
There was a problem hiding this comment.
Middleware is named “validCredentialUpdate” but never validates the input. Regardless of whether data is present, the request proceeds unverified, so malformed or malicious credential data can reach the service layer unchecked. Implement proper schema/type validation similar to validCredentialsProperties or remove this no-op middleware.
Prompt for AI agents
Address the following comment on packages/cli/src/public-api/v1/handlers/credentials/credentials.middleware.ts at line 51:
<comment>Middleware is named “validCredentialUpdate” but never validates the input. Regardless of whether `data` is present, the request proceeds unverified, so malformed or malicious credential data can reach the service layer unchecked. Implement proper schema/type validation similar to `validCredentialsProperties` or remove this no-op middleware.</comment>
<file context>
@@ -47,3 +47,20 @@ export const validCredentialsProperties = (
return next();
};
+
+export const validCredentialUpdate = (
+ req: CredentialRequest.Update,
+ res: express.Response,
</file context>
|
|
||
| it('should throw error when credential not found', async () => { | ||
| // Mock getCredentials to return null | ||
| jest.doMock('../credentials.service', () => ({ |
There was a problem hiding this comment.
jest.doMock is called after the module has already been imported, so the intended mock for getCredentials/getSharedCredentials will not be applied and the test assertions may be invalid. (Based on your team's feedback about ensuring tests are reliable and mocks are applied correctly.)
Prompt for AI agents
Address the following comment on packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts at line 88:
<comment>jest.doMock is called after the module has already been imported, so the intended mock for getCredentials/getSharedCredentials will not be applied and the test assertions may be invalid. (Based on your team's feedback about ensuring tests are reliable and mocks are applied correctly.)</comment>
<file context>
@@ -68,4 +69,44 @@ describe('CredentialsService', () => {
).toBe(true);
});
});
+
+ describe('updateCredential', () => {
+ const mockUser: User = {
+ id: 'user1',
+ role: 'global:member',
+ } as User;
</file context>
| tempCredential.name = existingCredential.name; | ||
| tempCredential.type = existingCredential.type; | ||
| // Temporarily assign the decrypted data (will be encrypted by encryptCredential) | ||
| (tempCredential as any).data = properties.data; |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Avoid as any type assertion; it bypasses type safety and violates the "Prefer Typeguards over Type casting" rule. Declare data on CredentialsEntity (or use a properly typed helper) instead of casting.
Prompt for AI agents
Address the following comment on packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts at line 132:
<comment>Avoid `as any` type assertion; it bypasses type safety and violates the "Prefer Typeguards over Type casting" rule. Declare `data` on `CredentialsEntity` (or use a properly typed helper) instead of casting.</comment>
<file context>
@@ -96,6 +96,65 @@ export async function saveCredential(
return result;
}
+export async function updateCredential(
+ user: User,
+ credentialId: string,
+ properties: Partial<CredentialRequest.CredentialProperties>,
+): Promise<CredentialsEntity> {
+ // Get existing credential
</file context>
n8n-assistant
Bot
added
community
|
Hey @Shock3udt, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: Regarding new nodes: If your node integrates with an AI service that you own or represent, please email nodes@n8n.io and we will be happy to discuss the best approach. About review timelines: Thank you again for contributing to n8n. |
|
Hey @ericmyrem Please don't tag people to try and get something reviewed sooner, This is somewhere in the list to be reviewed by the team that look after the api which isn't myself or Jan. There are other PRs that touch the api which may be reviewed sooner so this may also end up being closed if another pr adds the same feature. This is also not related to the git feature looking at the description and we wouldn't recommend storing credentials in git because of the potential security risks. |
|
Sure thing, no more tagging - it’s probably worth some attention though since the git functionality should include updates to credentials. Enterprises use it with the external secrets feature which means credentials don’t get stored in git but the {{secrets. }} expressions do. Currently the secrets references break when they get updated in dev, the git feature doesn’t include the update, software gets released to test and entire workflows break due to the wrong / outdated secrets references. So having this update API or git supporting secret updates is essential for your enterprise customers. |
|
This is a critical item for our team as well. We intended on rotating certain credentials with an external process and that is not possible without this feature. |
|
This is hugely important for credential rotation. We manage credentials for our customers through our own front-end and this is the missing piece. |
|
This PR or something with similar functionality is essential to make credential rotation feasible in n8n. I cannot use long lived credentials an really need a way to rotate them easily. |
|
+1 This would be a great addition |
|
+1 |
|
+1 |
|
+1 on this, need the ability to programmatically:
|
|
+1, Needed for credentials rotation |
|
Hello @Shock3udt, meanwhile we added a PATCH endpoint for changing credentials so the PUT is not needed right now - thanks a lot for opening a PR though! |
|
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. After signing, please comment |
|
The cla-bot has been summoned, and re-checked this pull request! |
n8n-assistant
Bot
added
the
Needs Feedback
|
Hi @Shock3udt 👋 Some of the checks on this PR are currently failing, which is blocking review. Please take a look at the checks and fix them. If the failures don't look related to your changes, rebasing onto the latest If you're stuck on any of them, leave a comment here and we'll help. |
15 participants
Summary
This PR adds a new PUT /api/v1/credentials/{id} endpoint to the n8n public API, allowing users to update existing credentials programmatically.
🚀 Features Added:
/api/v1/credentials/{id}with support for updating bothnameanddatapropertiescredential:updatescope added to API key permissions system🔧 Implementation Details:
updateCredentialservice function with encryption supportvalidCredentialUpdatemiddleware for request validation📋 API Usage:
✅ Testing:
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
PR Labeled withrelease/backport(if the PR is an urgent fix that needs to be backported)📁 Files Modified:
Core Implementation:
packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts- AddedupdateCredentialfunctionpackages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts- Added update endpoint handlerpackages/cli/src/public-api/v1/handlers/credentials/credentials.middleware.ts- Added validation middlewarepackages/cli/src/public-api/types.ts- Added Update request typePermissions & Scopes:
packages/@n8n/permissions/src/constants.ee.ts- Addedcredential:updatescopepackages/@n8n/permissions/src/public-api-permissions.ee.ts- Added scope to role permissionsAPI Documentation:
packages/cli/src/public-api/v1/handlers/credentials/spec/paths/credentials.id.yml- Added PUT operationpackages/cli/src/public-api/v1/handlers/credentials/spec/schemas/update-credential.yml- New schema fileTests:
packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts- Added unit tests🎯 Breaking Changes:
None - this is a purely additive feature that maintains backward compatibility.
🔒 Security Considerations: