diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 6b4fd8472a..e6fc3d141a 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -130,7 +130,7 @@ export class CredentialsController { } const mergedCredentials = deepCopy(credentials); - const decryptedData = this.credentialsService.decrypt(storedCredential); + const decryptedData = this.credentialsService.decrypt(storedCredential, true); // When a sharee (or project viewer) opens a credential, the fields and the // credential data are missing so the payload will be empty @@ -143,14 +143,14 @@ export class CredentialsController { mergedCredentials, ); - if (mergedCredentials.data && storedCredential) { + if (mergedCredentials.data) { mergedCredentials.data = this.credentialsService.unredact( mergedCredentials.data, decryptedData, ); } - return await this.credentialsService.test(req.user, mergedCredentials); + return await this.credentialsService.test(req.user.id, mergedCredentials); } @Post('/') @@ -176,18 +176,22 @@ export class CredentialsController { @Patch('/:credentialId') @ProjectScope('credential:update') async updateCredentials(req: CredentialRequest.Update) { - const { credentialId } = req.params; + const { + body, + user, + params: { credentialId }, + } = req; const credential = await this.sharedCredentialsRepository.findCredentialForUser( credentialId, - req.user, + user, ['credential:update'], ); if (!credential) { this.logger.info('Attempt to update credential blocked due to lack of permissions', { credentialId, - userId: req.user.id, + userId: user.id, }); throw new NotFoundError( 'Credential to be updated not found. You can only update credentials owned by you', @@ -199,6 +203,8 @@ export class CredentialsController { } const decryptedData = this.credentialsService.decrypt(credential, true); + // We never want to allow users to change the oauthTokenData + delete body.data?.oauthTokenData; const preparedCredentialData = await this.credentialsService.prepareUpdateData( req.body, decryptedData, diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index b183e20431..5f94312e94 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/packages/cli/src/credentials/credentials.service.ee.ts @@ -87,7 +87,7 @@ export class EnterpriseCredentialsService { if (credential) { // Decrypt the data if we found the credential with the `credential:update` // scope. - decryptedData = this.credentialsService.decrypt(credential, true); + decryptedData = this.credentialsService.decrypt(credential); } else { // Otherwise try to find them with only the `credential:read` scope. In // that case we return them without the decrypted data. @@ -109,6 +109,11 @@ export class EnterpriseCredentialsService { const { data: _, ...rest } = credential; if (decryptedData) { + // We never want to expose the oauthTokenData to the frontend, but it + // expects it to check if the credential is already connected. + if (decryptedData?.oauthTokenData) { + decryptedData.oauthTokenData = true; + } return { data: decryptedData, ...rest }; } diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index ac0711e51b..bfcce78481 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -112,9 +112,16 @@ export class CredentialsService { if (includeData) { credentials = credentials.map((c: CredentialsEntity & ScopesField) => { + const data = c.scopes.includes('credential:update') ? this.decrypt(c) : undefined; + // We never want to expose the oauthTokenData to the frontend, but it + // expects it to check if the credential is already connected. + if (data?.oauthTokenData) { + data.oauthTokenData = true; + } + return { ...c, - data: c.scopes.includes('credential:update') ? this.decrypt(c) : undefined, + data, } as unknown as CredentialsEntity; }); } @@ -441,8 +448,8 @@ export class CredentialsService { await this.credentialsRepository.remove(credential); } - async test(user: User, credentials: ICredentialsDecrypted) { - return await this.credentialsTester.testCredentials(user, credentials.type, credentials); + async test(userId: User['id'], credentials: ICredentialsDecrypted) { + return await this.credentialsTester.testCredentials(userId, credentials.type, credentials); } // Take data and replace all sensitive values with a sentinel value. @@ -553,7 +560,7 @@ export class CredentialsService { if (sharing) { // Decrypt the data if we found the credential with the `credential:update` // scope. - decryptedData = this.decrypt(sharing.credentials, true); + decryptedData = this.decrypt(sharing.credentials); } else { // Otherwise try to find them with only the `credential:read` scope. In // that case we return them without the decrypted data. @@ -569,6 +576,11 @@ export class CredentialsService { const { data: _, ...rest } = credential; if (decryptedData) { + // We never want to expose the oauthTokenData to the frontend, but it + // expects it to check if the credential is already connected. + if (decryptedData?.oauthTokenData) { + decryptedData.oauthTokenData = true; + } return { data: decryptedData, ...rest }; } return { ...rest }; diff --git a/packages/cli/src/services/credentials-tester.service.ts b/packages/cli/src/services/credentials-tester.service.ts index a45443a3bc..46d4456403 100644 --- a/packages/cli/src/services/credentials-tester.service.ts +++ b/packages/cli/src/services/credentials-tester.service.ts @@ -172,7 +172,7 @@ export class CredentialsTester { // eslint-disable-next-line complexity async testCredentials( - user: User, + userId: User['id'], credentialType: string, credentialsDecrypted: ICredentialsDecrypted, ): Promise { @@ -186,7 +186,7 @@ export class CredentialsTester { if (credentialsDecrypted.data) { try { - const additionalData = await WorkflowExecuteAdditionalData.getBase(user.id); + const additionalData = await WorkflowExecuteAdditionalData.getBase(userId); credentialsDecrypted.data = this.credentialsHelper.applyDefaultsAndOverwrites( additionalData, credentialsDecrypted.data, @@ -292,7 +292,7 @@ export class CredentialsTester { }, }; - const additionalData = await WorkflowExecuteAdditionalData.getBase(user.id, node.parameters); + const additionalData = await WorkflowExecuteAdditionalData.getBase(userId, node.parameters); const executeData: IExecuteData = { node, data: {}, source: null }; const executeFunctions = new ExecuteContext( diff --git a/packages/cli/test/integration/credentials/credentials.api.ee.test.ts b/packages/cli/test/integration/credentials/credentials.api.ee.test.ts index 98cecb6664..f6a5c1cfd5 100644 --- a/packages/cli/test/integration/credentials/credentials.api.ee.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.ee.test.ts @@ -28,7 +28,10 @@ import { createUser, createUserShell, } from '../shared/db/users'; -import { randomCredentialPayload } from '../shared/random'; +import { + randomCredentialPayload, + randomCredentialPayloadWithOauthTokenData, +} from '../shared/random'; import * as testDb from '../shared/test-db'; import type { SaveCredentialFunction } from '../shared/types'; import type { SuperAgentTest } from '../shared/types'; @@ -556,10 +559,11 @@ describe('GET /credentials/:id', () => { expect(secondCredential.data).toBeDefined(); }); - test('should not redact the data when `includeData:true` is passed', async () => { + test('should redact the data when `includeData:true` is passed', async () => { const credentialService = Container.get(CredentialsService); const redactSpy = jest.spyOn(credentialService, 'redact'); - const savedCredential = await saveCredential(randomCredentialPayload(), { + const credential = randomCredentialPayloadWithOauthTokenData(); + const savedCredential = await saveCredential(credential, { user: owner, }); @@ -569,7 +573,8 @@ describe('GET /credentials/:id', () => { validateMainCredentialData(response.body.data); expect(response.body.data.data).toBeDefined(); - expect(redactSpy).not.toHaveBeenCalled(); + expect(response.body.data.data.oauthTokenData).toBe(true); + expect(redactSpy).toHaveBeenCalled(); }); test('should retrieve non-owned cred for owner', async () => { diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index ece8340e83..860c36af9a 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.test.ts @@ -1,7 +1,9 @@ import { GlobalConfig } from '@n8n/config'; import { Container } from '@n8n/di'; import type { Scope } from '@sentry/node'; +import { mock } from 'jest-mock-extended'; import { Credentials } from 'n8n-core'; +import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; import { randomString } from 'n8n-workflow'; import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; @@ -12,8 +14,10 @@ import { CredentialsRepository } from '@/databases/repositories/credentials.repo import { ProjectRepository } from '@/databases/repositories/project.repository'; import { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository'; import type { ListQuery } from '@/requests'; +import { CredentialsTester } from '@/services/credentials-tester.service'; import { + getCredentialById, saveCredential, shareCredentialWithProjects, shareCredentialWithUsers, @@ -23,6 +27,7 @@ import { createManyUsers, createMember, createOwner } from '../shared/db/users'; import { randomCredentialPayload as payload, randomCredentialPayload, + randomCredentialPayloadWithOauthTokenData, randomName, } from '../shared/random'; import * as testDb from '../shared/test-db'; @@ -319,7 +324,7 @@ describe('GET /credentials', () => { ] = await Promise.all([ saveCredential(randomCredentialPayload(), { user: owner, role: 'credential:owner' }), saveCredential(randomCredentialPayload(), { user: member, role: 'credential:owner' }), - saveCredential(randomCredentialPayload(), { + saveCredential(randomCredentialPayloadWithOauthTokenData(), { project: teamProjectViewer, role: 'credential:owner', }), @@ -370,6 +375,9 @@ describe('GET /credentials', () => { expect(teamCredAsViewer.id).toBe(teamCredentialAsViewer.id); expect(teamCredAsViewer.data).toBeDefined(); + expect( + (teamCredAsViewer.data as unknown as ICredentialDataDecryptedObject).oauthTokenData, + ).toBe(true); expect(teamCredAsViewer.scopes).toEqual( [ 'credential:move', @@ -1165,55 +1173,19 @@ describe('PATCH /credentials/:id', () => { expect(shellCredential.name).toBe(patchPayload.name); // updated }); - test('should not store redacted value in the db for oauthTokenData', async () => { - // ARRANGE - const credentialService = Container.get(CredentialsService); - const redactSpy = jest.spyOn(credentialService, 'redact').mockReturnValueOnce({ - accessToken: CREDENTIAL_BLANKING_VALUE, - oauthTokenData: CREDENTIAL_BLANKING_VALUE, - }); - - const payload = randomCredentialPayload(); - payload.data.oauthTokenData = { tokenData: true }; - const savedCredential = await saveCredential(payload, { - user: owner, - role: 'credential:owner', - }); - - // ACT - const patchPayload = { ...payload, data: { foo: 'bar' } }; - await authOwnerAgent.patch(`/credentials/${savedCredential.id}`).send(patchPayload).expect(200); - - // ASSERT - const response = await authOwnerAgent - .get(`/credentials/${savedCredential.id}`) - .query({ includeData: true }) - .expect(200); - - const { id, data } = response.body.data; - - expect(id).toBe(savedCredential.id); - expect(data).toEqual({ - ...patchPayload.data, - // should be the original - oauthTokenData: payload.data.oauthTokenData, - }); - expect(redactSpy).not.toHaveBeenCalled(); - }); - test('should not allow to overwrite oauthTokenData', async () => { // ARRANGE - const payload = randomCredentialPayload(); - payload.data.oauthTokenData = { tokenData: true }; - const savedCredential = await saveCredential(payload, { + const credential = randomCredentialPayload(); + credential.data.oauthTokenData = { access_token: 'foo' }; + const savedCredential = await saveCredential(credential, { user: owner, role: 'credential:owner', }); // ACT const patchPayload = { - ...payload, - data: { accessToken: 'new', oauthTokenData: { tokenData: false } }, + ...credential, + data: { accessToken: 'new', oauthTokenData: { access_token: 'bar' } }, }; await authOwnerAgent.patch(`/credentials/${savedCredential.id}`).send(patchPayload).expect(200); @@ -1229,7 +1201,9 @@ describe('PATCH /credentials/:id', () => { // was overwritten expect(data.accessToken).toBe(patchPayload.data.accessToken); // was not overwritten - expect(data.oauthTokenData).toEqual(payload.data.oauthTokenData); + const dbCredential = await getCredentialById(savedCredential.id); + const unencryptedData = Container.get(CredentialsService).decrypt(dbCredential!); + expect(unencryptedData.oauthTokenData).toEqual(credential.data.oauthTokenData); }); test('should fail with invalid inputs', async () => { @@ -1341,7 +1315,7 @@ describe('GET /credentials/:id', () => { expect(secondResponse.body.data.data).toBeDefined(); }); - test('should not redact the data when `includeData:true` is passed', async () => { + test('should redact the data when `includeData:true` is passed', async () => { const credentialService = Container.get(CredentialsService); const redactSpy = jest.spyOn(credentialService, 'redact'); const savedCredential = await saveCredential(randomCredentialPayload(), { @@ -1355,7 +1329,23 @@ describe('GET /credentials/:id', () => { validateMainCredentialData(response.body.data); expect(response.body.data.data).toBeDefined(); - expect(redactSpy).not.toHaveBeenCalled(); + expect(redactSpy).toHaveBeenCalled(); + }); + + test('should omit oauth data when `includeData:true` is passed', async () => { + const credential = randomCredentialPayloadWithOauthTokenData(); + const savedCredential = await saveCredential(credential, { + user: owner, + role: 'credential:owner', + }); + + const response = await authOwnerAgent + .get(`/credentials/${savedCredential.id}`) + .query({ includeData: true }); + + validateMainCredentialData(response.body.data); + expect(response.body.data.data).toBeDefined(); + expect(response.body.data.data.oauthTokenData).toBe(true); }); test('should retrieve owned cred for member', async () => { @@ -1425,6 +1415,73 @@ describe('GET /credentials/:id', () => { }); }); +describe('POST /credentials/test', () => { + const mockCredentialsTester = mock(); + Container.set(CredentialsTester, mockCredentialsTester); + + afterEach(() => { + mockCredentialsTester.testCredentials.mockClear(); + }); + + test('should test a credential with unredacted data', async () => { + mockCredentialsTester.testCredentials.mockResolvedValue({ + status: 'OK', + message: 'Credential tested successfully', + }); + const credential = randomCredentialPayload(); + const savedCredential = await saveCredential(credential, { + user: owner, + role: 'credential:owner', + }); + + const response = await authOwnerAgent.post('/credentials/test').send({ + credentials: { + id: savedCredential.id, + type: savedCredential.type, + data: credential.data, + }, + }); + expect(response.statusCode).toBe(200); + expect(mockCredentialsTester.testCredentials.mock.calls[0][0]).toEqual(owner.id); + expect(mockCredentialsTester.testCredentials.mock.calls[0][1]).toBe(savedCredential.type); + expect(mockCredentialsTester.testCredentials.mock.calls[0][2]).toEqual({ + id: savedCredential.id, + type: savedCredential.type, + data: credential.data, + }); + }); + + test('should test a credential with redacted data', async () => { + mockCredentialsTester.testCredentials.mockResolvedValue({ + status: 'OK', + message: 'Credential tested successfully', + }); + const credential = randomCredentialPayload(); + const savedCredential = await saveCredential(credential, { + user: owner, + role: 'credential:owner', + }); + + const response = await authOwnerAgent.post('/credentials/test').send({ + credentials: { + id: savedCredential.id, + type: savedCredential.type, + data: { + accessToken: CREDENTIAL_BLANKING_VALUE, + }, + }, + }); + expect(response.statusCode).toBe(200); + expect(mockCredentialsTester.testCredentials.mock.calls[0][0]).toEqual(owner.id); + expect(mockCredentialsTester.testCredentials.mock.calls[0][1]).toBe(savedCredential.type); + expect(mockCredentialsTester.testCredentials.mock.calls[0][2]).toEqual({ + id: savedCredential.id, + type: savedCredential.type, + data: credential.data, + }); + }); +}); + const INVALID_PAYLOADS = [ { type: randomName(), diff --git a/packages/cli/test/integration/shared/random.ts b/packages/cli/test/integration/shared/random.ts index e556c4f512..bc55b72d18 100644 --- a/packages/cli/test/integration/shared/random.ts +++ b/packages/cli/test/integration/shared/random.ts @@ -46,4 +46,13 @@ export const randomCredentialPayload = ({ isManaged, }); +export const randomCredentialPayloadWithOauthTokenData = ({ + isManaged = false, +}: { isManaged?: boolean } = {}): CredentialPayload => ({ + name: randomName(), + type: randomName(), + data: { accessToken: randomString(6, 16), oauthTokenData: { access_token: randomString(6, 16) } }, + isManaged, +}); + export const uniqueId = () => uuid();