From 613cdd2ba2c0f224c4857a5fc3eea36dbd683049 Mon Sep 17 00:00:00 2001 From: Val <68596159+valya@users.noreply.github.com> Date: Thu, 15 Aug 2024 09:39:48 +0100 Subject: [PATCH] fix: Project Viewer always seeing a connection error when testing credentials (#10417) --- .../src/credentials/credentials.controller.ts | 6 +- .../src/credentials/credentials.service.ts | 29 ++++---- .../credentials/credentials.service.test.ts | 66 ++++++++++++++++++- 3 files changed, 79 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 40f2aa8d43219..ba965a1adde64 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -127,11 +127,11 @@ export class CredentialsController { const mergedCredentials = deepCopy(credentials); const decryptedData = this.credentialsService.decrypt(storedCredential); - // When a sharee opens a credential, the fields and the credential data are missing - // so the payload will be empty + // When a sharee (or project viewer) opens a credential, the fields and the + // credential data are missing so the payload will be empty // We need to replace the credential contents with the db version if that's the case // So the credential can be tested properly - this.credentialsService.replaceCredentialContentsForSharee( + await this.credentialsService.replaceCredentialContentsForSharee( req.user, storedCredential, decryptedData, diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 01517c960adad..03ea13efe1a3e 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -38,6 +38,7 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error'; import type { ProjectRelation } from '@/databases/entities/ProjectRelation'; import { RoleService } from '@/services/role.service'; import { UserRepository } from '@/databases/repositories/user.repository'; +import { userHasScope } from '@/permissions/checkAccess'; export type CredentialsGetSharedOptions = | { allowGlobalScope: true; globalScope: Scope } @@ -599,28 +600,20 @@ export class CredentialsService { ); } - replaceCredentialContentsForSharee( + async replaceCredentialContentsForSharee( user: User, credential: CredentialsEntity, decryptedData: ICredentialDataDecryptedObject, mergedCredentials: ICredentialsDecrypted, ) { - credential.shared.forEach((sharedCredentials) => { - if (sharedCredentials.role === 'credential:owner') { - if (sharedCredentials.project.type === 'personal') { - // Find the owner of this personal project - sharedCredentials.project.projectRelations.forEach((projectRelation) => { - if ( - projectRelation.role === 'project:personalOwner' && - projectRelation.user.id !== user.id - ) { - // If we realize that the current user does not own this credential - // We replace the payload with the stored decrypted data - mergedCredentials.data = decryptedData; - } - }); - } - } - }); + // We may want to change this to 'credential:decrypt' if that gets added, but this + // works for now. The only time we wouldn't want to do this is if the user + // could actually be testing the credential before saving it, so this should cover + // the cases we need it for. + if ( + !(await userHasScope(user, ['credential:update'], false, { credentialId: credential.id })) + ) { + mergedCredentials.data = decryptedData; + } } } diff --git a/packages/cli/test/integration/credentials/credentials.service.test.ts b/packages/cli/test/integration/credentials/credentials.service.test.ts index 95aea5cbf8b7c..827ec5fdba4dd 100644 --- a/packages/cli/test/integration/credentials/credentials.service.test.ts +++ b/packages/cli/test/integration/credentials/credentials.service.test.ts @@ -7,6 +7,7 @@ import { SharedCredentialsRepository } from '@/databases/repositories/sharedCred import Container from 'typedi'; import { CredentialsService } from '@/credentials/credentials.service'; import * as testDb from '../shared/testDb'; +import { createTeamProject, linkUserToProject } from '@test-integration/db/projects'; const credentialPayload = randomCredentialPayload(); let memberWhoOwnsCredential: User; @@ -42,7 +43,7 @@ describe('credentials service', () => { data: { accessToken: '' }, }; - Container.get(CredentialsService).replaceCredentialContentsForSharee( + await Container.get(CredentialsService).replaceCredentialContentsForSharee( memberWhoDoesNotOwnCredential, storedCredential!, decryptedData, @@ -51,5 +52,68 @@ describe('credentials service', () => { expect(mergedCredentials.data).toEqual({ accessToken: credentialPayload.data.accessToken }); }); + + it('should replace the contents of the credential for project viewer', async () => { + const [project, viewerMember] = await Promise.all([createTeamProject(), createMember()]); + await linkUserToProject(viewerMember, project, 'project:viewer'); + const projectCredential = await saveCredential(credentialPayload, { + project, + role: 'credential:owner', + }); + + const storedProjectCredential = await Container.get( + SharedCredentialsRepository, + ).findCredentialForUser(projectCredential.id, viewerMember, ['credential:read']); + + const decryptedData = Container.get(CredentialsService).decrypt(storedProjectCredential!); + + const mergedCredentials = { + id: projectCredential.id, + name: projectCredential.name, + type: projectCredential.type, + data: { accessToken: '' }, + }; + + await Container.get(CredentialsService).replaceCredentialContentsForSharee( + viewerMember, + storedProjectCredential!, + decryptedData, + mergedCredentials, + ); + + expect(mergedCredentials.data).toEqual({ accessToken: credentialPayload.data.accessToken }); + }); + + it('should not replace the contents of the credential for project editor', async () => { + const [project, editorMember] = await Promise.all([createTeamProject(), createMember()]); + await linkUserToProject(editorMember, project, 'project:editor'); + const projectCredential = await saveCredential(credentialPayload, { + project, + role: 'credential:owner', + }); + + const storedProjectCredential = await Container.get( + SharedCredentialsRepository, + ).findCredentialForUser(projectCredential.id, editorMember, ['credential:read']); + + const decryptedData = Container.get(CredentialsService).decrypt(storedProjectCredential!); + + const originalData = { accessToken: '' }; + const mergedCredentials = { + id: projectCredential.id, + name: projectCredential.name, + type: projectCredential.type, + data: originalData, + }; + + await Container.get(CredentialsService).replaceCredentialContentsForSharee( + editorMember, + storedProjectCredential!, + decryptedData, + mergedCredentials, + ); + + expect(mergedCredentials.data).toBe(originalData); + }); }); });