fix(core): Use cached value in retrieval of personal project owner (#11533)

Co-authored-by: Danny Martini <danny@n8n.io>

packages/cli/src/services/__tests__/ownership.service.test.ts

@@ -1,4 +1,5 @@
 import { mock } from 'jest-mock-extended';
+import { v4 as uuid } from 'uuid';
 
 import { Project } from '@/databases/entities/project';
 import { ProjectRelation } from '@/databases/entities/project-relation';
@@ -13,12 +14,15 @@ import { OwnershipService } from '@/services/ownership.service';
 import { mockCredential, mockProject } from '@test/mock-objects';
 import { mockInstance } from '@test/mocking';
 
+import { CacheService } from '../cache/cache.service';
+
 describe('OwnershipService', () => {
 	const userRepository = mockInstance(UserRepository);
 	const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository);
 	const projectRelationRepository = mockInstance(ProjectRelationRepository);
+	const cacheService = mockInstance(CacheService);
 	const ownershipService = new OwnershipService(
-		mock(),
+		cacheService,
 		userRepository,
 		mock(),
 		projectRelationRepository,
@@ -52,22 +56,22 @@ describe('OwnershipService', () => {
 		});
 	});
 
-	describe('getProjectOwnerCached()', () => {
+	describe('getPersonalProjectOwnerCached()', () => {
 		test('should retrieve a project owner', async () => {
-			const mockProject = new Project();
-			const mockOwner = new User();
-
-			const projectRelation = Object.assign(new ProjectRelation(), {
-				role: 'project:personalOwner',
-				project: mockProject,
-				user: mockOwner,
-			});
+			// ARRANGE
+			const project = new Project();
+			const owner = new User();
+			const projectRelation = new ProjectRelation();
+			projectRelation.role = 'project:personalOwner';
+			(projectRelation.project = project), (projectRelation.user = owner);
 
 			projectRelationRepository.getPersonalProjectOwners.mockResolvedValueOnce([projectRelation]);
 
+			// ACT
 			const returnedOwner = await ownershipService.getPersonalProjectOwnerCached('some-project-id');
 
-			expect(returnedOwner).toBe(mockOwner);
+			// ASSERT
+			expect(returnedOwner).toBe(owner);
 		});
 
 		test('should not throw if no project owner found, should return null instead', async () => {
@@ -77,6 +81,29 @@ describe('OwnershipService', () => {
 
 			expect(owner).toBeNull();
 		});
+
+		test('should not use the repository if the owner was found in the cache', async () => {
+			// ARRANGE
+			const project = new Project();
+			project.id = uuid();
+			const owner = new User();
+			owner.id = uuid();
+			const projectRelation = new ProjectRelation();
+			projectRelation.role = 'project:personalOwner';
+			(projectRelation.project = project), (projectRelation.user = owner);
+
+			cacheService.getHashValue.mockResolvedValueOnce(owner);
+			userRepository.create.mockReturnValueOnce(owner);
+
+			// ACT
+			const foundOwner = await ownershipService.getPersonalProjectOwnerCached(project.id);
+
+			// ASSERT
+			expect(cacheService.getHashValue).toHaveBeenCalledTimes(1);
+			expect(cacheService.getHashValue).toHaveBeenCalledWith('project-owner', project.id);
+			expect(projectRelationRepository.getPersonalProjectOwners).not.toHaveBeenCalled();
+			expect(foundOwner).toEqual(owner);
+		});
 	});
 
 	describe('getProjectOwnerCached()', () => {

packages/cli/src/services/ownership.service.ts

@@ -45,13 +45,9 @@ export class OwnershipService {
 	 * Personal project ownership is **immutable**.
 	 */
 	async getPersonalProjectOwnerCached(projectId: string): Promise<User | null> {
-		const cachedValue = await this.cacheService.getHashValue<User | null>(
-			'project-owner',
-			projectId,
-		);
+		const cachedValue = await this.cacheService.getHashValue<User>('project-owner', projectId);
 
-		if (cachedValue) this.userRepository.create(cachedValue);
-		if (cachedValue === null) return null;
+		if (cachedValue) return this.userRepository.create(cachedValue);
 
 		const ownerRel = await this.projectRelationRepository.getPersonalProjectOwners([projectId]);
 		const owner = ownerRel[0]?.user ?? null;

packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts

@@ -1,6 +1,5 @@
 import { GlobalConfig } from '@n8n/config';
 import { type Workflow, type INode, type WorkflowSettings } from 'n8n-workflow';
-import { strict as assert } from 'node:assert';
 import { Service } from 'typedi';
 
 import type { Project } from '@/databases/entities/project';
@@ -68,11 +67,9 @@ export class SubworkflowPolicyChecker {
 
 		const owner = await this.ownershipService.getPersonalProjectOwnerCached(subworkflowProject.id);
 
-		assert(owner !== null); // only `null` if not personal
-
 		return {
 			hasReadAccess,
-			ownerName: owner.firstName + ' ' + owner.lastName,
+			ownerName: owner ? owner.firstName + ' ' + owner.lastName : 'No owner (team project)',
 		};
 	}