Merge pull request #3869 from RedisInsight/bugfix/be/RI-6136

[RI-6136] Disconnect clients for the database after certificate removal

Author: mariasergeenko

redisinsight/api/src/__mocks__/common.ts

@@ -9,6 +9,7 @@ export type MockType<T> = {
 
 export const mockQueryBuilderWhere = jest.fn().mockReturnThis();
 export const mockQueryBuilderSelect = jest.fn().mockReturnThis();
+export const mockQueryBuilderLeftJoinAndSelect = jest.fn().mockReturnThis();
 export const mockQueryBuilderGetOne = jest.fn();
 export const mockQueryBuilderGetMany = jest.fn();
 export const mockQueryBuilderGetManyRaw = jest.fn();
@@ -26,7 +27,7 @@ export const mockCreateQueryBuilder = jest.fn(() => ({
   having: jest.fn().mockReturnThis(),
   limit: jest.fn().mockReturnThis(),
   leftJoin: jest.fn().mockReturnThis(),
-  leftJoinAndSelect: jest.fn().mockReturnThis(),
+  leftJoinAndSelect: mockQueryBuilderLeftJoinAndSelect,
   offset: jest.fn().mockReturnThis(),
   delete: jest.fn().mockReturnThis(),
   whereInIds: jest.fn().mockReturnThis(),

redisinsight/api/src/modules/certificate/ca-certificate.service.spec.ts

@@ -5,8 +5,10 @@ import {
   mockCaCertificate,
   mockCaCertificateRepository, mockCreateCaCertificateDto,
   MockType,
+  mockRedisClientStorage,
 } from 'src/__mocks__';
 import { CaCertificateRepository } from 'src/modules/certificate/repositories/ca-certificate.repository';
+import { RedisClientStorage } from 'src/modules/redis/redis.client.storage';
 import { pick } from 'lodash';
 import { KeytarEncryptionErrorException } from 'src/modules/encryption/exceptions';
 import { CaCertificateService } from './ca-certificate.service';
@@ -24,6 +26,10 @@ describe('CaCertificateService', () => {
           provide: CaCertificateRepository,
           useFactory: mockCaCertificateRepository,
         },
+        {
+          provide: RedisClientStorage,
+          useFactory: mockRedisClientStorage,
+        },
       ],
     }).compile();
 
@@ -87,9 +93,26 @@ describe('CaCertificateService', () => {
   });
 
   describe('delete', () => {
-    it('should delete ca certificate', async () => {
-      expect(await service.delete(mockCaCertificate.id)).toEqual(undefined);
+    const mockId = 'mock-ca-cert-id';
+    const mockAffectedDatabases = ['db1', 'db2'];
+
+    beforeEach(() => {
+      jest.clearAllMocks();
+    });
+
+    it('should delete CA certificate and remove affected database clients', async () => {
+      jest.spyOn(repository, 'delete').mockResolvedValue({ affectedDatabases: mockAffectedDatabases });
+      jest.spyOn(service['redisClientStorage'], 'removeManyByMetadata').mockResolvedValue(undefined);
+
+      await service.delete(mockId);
+
+      expect(repository.delete).toHaveBeenCalledWith(mockId);
+      expect(service['redisClientStorage'].removeManyByMetadata).toHaveBeenCalledTimes(mockAffectedDatabases.length);
+      mockAffectedDatabases.forEach((databaseId) => {
+        expect(service['redisClientStorage'].removeManyByMetadata).toHaveBeenCalledWith({ databaseId });
+      });
     });
+
     it('should throw encryption error', async () => {
       repository.delete.mockRejectedValueOnce(new KeytarEncryptionErrorException());
 

redisinsight/api/src/modules/certificate/ca-certificate.service.ts

@@ -12,13 +12,15 @@ import { CaCertificateRepository } from 'src/modules/certificate/repositories/ca
 import { CaCertificate } from 'src/modules/certificate/models/ca-certificate';
 import { CreateCaCertificateDto } from 'src/modules/certificate/dto/create.ca-certificate.dto';
 import { classToClass } from 'src/utils';
+import { RedisClientStorage } from 'src/modules/redis/redis.client.storage';
 
 @Injectable()
 export class CaCertificateService {
   private logger = new Logger('CaCertificateService');
 
   constructor(
     private readonly repository: CaCertificateRepository,
+    private redisClientStorage: RedisClientStorage,
   ) {}
 
   async get(id: string): Promise<CaCertificate> {
@@ -56,10 +58,13 @@ export class CaCertificateService {
   }
 
   async delete(id: string): Promise<void> {
-    this.logger.log(`Deleting certificate. id: ${id}`);
-
     try {
-      await this.repository.delete(id);
+      const { affectedDatabases } = await this.repository.delete(id);
+
+      await Promise.all(affectedDatabases.map(async (databaseId) => {
+        // If the certificate is used by the database, remove the client
+        await this.redisClientStorage.removeManyByMetadata({ databaseId });
+      }));
     } catch (error) {
       this.logger.error(`Failed to delete certificate ${id}`, error);
 

redisinsight/api/src/modules/certificate/client-certificate.service.spec.ts

@@ -6,10 +6,12 @@ import {
   mockClientCertificate,
   mockClientCertificateRepository, mockCreateClientCertificateDto,
   MockType,
+  mockRedisClientStorage,
 } from 'src/__mocks__';
 import { KeytarEncryptionErrorException } from 'src/modules/encryption/exceptions';
 import { ClientCertificateService } from 'src/modules/certificate/client-certificate.service';
 import { ClientCertificateRepository } from 'src/modules/certificate/repositories/client-certificate.repository';
+import { RedisClientStorage } from 'src/modules/redis/redis.client.storage';
 
 describe('ClientCertificateService', () => {
   let service: ClientCertificateService;
@@ -24,6 +26,10 @@ describe('ClientCertificateService', () => {
           provide: ClientCertificateRepository,
           useFactory: mockClientCertificateRepository,
         },
+        {
+          provide: RedisClientStorage,
+          useFactory: mockRedisClientStorage,
+        },
       ],
     }).compile();
 
@@ -87,9 +93,26 @@ describe('ClientCertificateService', () => {
   });
 
   describe('delete', () => {
-    it('should delete client certificate', async () => {
-      expect(await service.delete(mockClientCertificate.id)).toEqual(undefined);
+    const mockId = 'mock-client-cert-id';
+    const mockAffectedDatabases = ['db1', 'db2'];
+
+    beforeEach(() => {
+      jest.clearAllMocks();
+    });
+
+    it('should delete client certificate and remove affected database clients', async () => {
+      jest.spyOn(repository, 'delete').mockResolvedValue({ affectedDatabases: mockAffectedDatabases });
+      jest.spyOn(service['redisClientStorage'], 'removeManyByMetadata').mockResolvedValue(undefined);
+
+      await service.delete(mockId);
+
+      expect(repository.delete).toHaveBeenCalledWith(mockId);
+      expect(service['redisClientStorage'].removeManyByMetadata).toHaveBeenCalledTimes(mockAffectedDatabases.length);
+      mockAffectedDatabases.forEach((databaseId) => {
+        expect(service['redisClientStorage'].removeManyByMetadata).toHaveBeenCalledWith({ databaseId });
+      });
     });
+
     it('should throw encryption error', async () => {
       repository.delete.mockRejectedValueOnce(new KeytarEncryptionErrorException());
 

redisinsight/api/src/modules/certificate/client-certificate.service.ts

@@ -12,13 +12,15 @@ import { ClientCertificateRepository } from 'src/modules/certificate/repositorie
 import { ClientCertificate } from 'src/modules/certificate/models/client-certificate';
 import { CreateClientCertificateDto } from 'src/modules/certificate/dto/create.client-certificate.dto';
 import { classToClass } from 'src/utils';
+import { RedisClientStorage } from 'src/modules/redis/redis.client.storage';
 
 @Injectable()
 export class ClientCertificateService {
   private logger = new Logger('ClientCertificateService');
 
   constructor(
     private readonly repository: ClientCertificateRepository,
+    private redisClientStorage: RedisClientStorage,
   ) {}
 
   /**
@@ -67,7 +69,12 @@ export class ClientCertificateService {
     this.logger.log(`Deleting client certificate. id: ${id}`);
 
     try {
-      await this.repository.delete(id);
+      const { affectedDatabases } = await this.repository.delete(id);
+
+      await Promise.all(affectedDatabases.map(async (databaseId) => {
+        // If the certificate is used by the database, remove the client
+        await this.redisClientStorage.removeManyByMetadata({ databaseId });
+      }));
     } catch (error) {
       this.logger.error(`Failed to delete certificate ${id}`, error);
 

redisinsight/api/src/modules/certificate/repositories/ca-certificate.repository.ts

@@ -26,5 +26,5 @@ export abstract class CaCertificateRepository {
    * @param id
    * @throws NotFoundException in case when try to delete not existing cert (?)
    */
-  abstract delete(id: string): Promise<void>;
+  abstract delete(id: string): Promise<{ affectedDatabases: string[] }>;
 }

redisinsight/api/src/modules/certificate/repositories/client-certificate.repository.ts

@@ -24,5 +24,5 @@ export abstract class ClientCertificateRepository {
    * @param id
    * @throws NotFoundException in case when try to delete not existing cert (?)
    */
-  abstract delete(id: string): Promise<void>;
+  abstract delete(id: string): Promise<{ affectedDatabases: string[] }>;
 }

redisinsight/api/src/modules/certificate/repositories/local.ca-certificate.repository.spec.ts

@@ -5,7 +5,8 @@ import { getRepositoryToken } from '@nestjs/typeorm';
 import { Repository } from 'typeorm';
 import {
   mockCaCertificate, mockCaCertificateCertificateEncrypted, mockCaCertificateCertificatePlain, mockCaCertificateEntity,
-  mockCaCertificateId, mockEncryptionService,
+  mockCaCertificateId,
+  mockEncryptionService,
   mockRepository,
   MockType,
 } from 'src/__mocks__';
@@ -14,11 +15,13 @@ import { CaCertificateEntity } from 'src/modules/certificate/entities/ca-certifi
 import { EncryptionService } from 'src/modules/encryption/encryption.service';
 import { BadRequestException, NotFoundException } from '@nestjs/common';
 import ERROR_MESSAGES from 'src/constants/error-messages';
+import { DatabaseEntity } from 'src/modules/database/entities/database.entity';
 
 describe('LocalCaCertificateRepository', () => {
   let service: LocalCaCertificateRepository;
   let encryptionService: MockType<EncryptionService>;
   let repository: MockType<Repository<CaCertificateEntity>>;
+  let databaseRepository: MockType<Repository<DatabaseEntity>>;
 
   beforeEach(async () => {
     jest.clearAllMocks();
@@ -30,6 +33,10 @@ describe('LocalCaCertificateRepository', () => {
           provide: getRepositoryToken(CaCertificateEntity),
           useFactory: mockRepository,
         },
+        {
+          provide: getRepositoryToken(DatabaseEntity),
+          useFactory: mockRepository,
+        },
         {
           provide: EncryptionService,
           useFactory: mockEncryptionService,
@@ -38,6 +45,7 @@ describe('LocalCaCertificateRepository', () => {
     }).compile();
 
     repository = await module.get(getRepositoryToken(CaCertificateEntity));
+    databaseRepository = await module.get(getRepositoryToken(DatabaseEntity));
     encryptionService = await module.get(EncryptionService);
     service = await module.get(LocalCaCertificateRepository);
 
@@ -100,24 +108,37 @@ describe('LocalCaCertificateRepository', () => {
   });
 
   describe('delete', () => {
-    it('should delete ca certificate', async () => {
-      const result = await service.delete(mockCaCertificate.id);
-
-      expect(result).toEqual(undefined);
+    it('should delete ca certificate and return affected databases', async () => {
+      const mockId = 'mock-ca-cert-id';
+      const mockAffectedDatabases = ['db1', 'db2'];
+
+      // Mock findOneBy to return a certificate
+      repository.findOneBy.mockResolvedValue(mockCaCertificate);
+      databaseRepository.createQueryBuilder().getMany.mockResolvedValue(mockAffectedDatabases.map((id) => ({ id })));
+
+      // Mock delete operation
+      repository.delete.mockResolvedValue(undefined);
+
+      const result = await service.delete(mockId);
+
+      expect(result).toEqual({ affectedDatabases: mockAffectedDatabases });
+      expect(repository.findOneBy).toHaveBeenCalledWith({ id: mockId });
+      expect(databaseRepository.createQueryBuilder).toHaveBeenCalledWith('d');
+      expect(databaseRepository.createQueryBuilder().leftJoinAndSelect).toHaveBeenCalledWith('d.caCert', 'c');
+      expect(databaseRepository.createQueryBuilder().where).toHaveBeenCalledWith({ caCert: mockId });
+      expect(databaseRepository.createQueryBuilder().select).toHaveBeenCalledWith(['d.id']);
+      expect(repository.delete).toHaveBeenCalledWith(mockId);
     });
 
-    it('should throw an error when trying to delete non-existing ca certificate', async () => {
-      repository.findOneBy.mockResolvedValueOnce(null);
+    it('should throw NotFoundException when trying to delete non-existing ca certificate', async () => {
+      const mockId = 'non-existent-id';
 
-      try {
-        await service.delete(mockCaCertificate.id);
-        fail();
-      } catch (e) {
-        expect(e).toBeInstanceOf(NotFoundException);
-        // todo: why such message?
-        expect(e.message).toEqual('Not Found');
-        expect(repository.delete).not.toHaveBeenCalled();
-      }
+      // Mock findOneBy to return null (certificate not found)
+      repository.findOneBy.mockResolvedValue(null);
+
+      await expect(service.delete(mockId)).rejects.toThrow(NotFoundException);
+      expect(repository.findOneBy).toHaveBeenCalledWith({ id: mockId });
+      expect(repository.delete).not.toHaveBeenCalled();
     });
   });
 });

redisinsight/api/src/modules/certificate/repositories/local.ca-certificate.repository.ts

@@ -13,6 +13,7 @@ import ERROR_MESSAGES from 'src/constants/error-messages';
 import { classToClass } from 'src/utils';
 import { EncryptionService } from 'src/modules/encryption/encryption.service';
 import { ModelEncryptor } from 'src/modules/encryption/model.encryptor';
+import { DatabaseEntity } from 'src/modules/database/entities/database.entity';
 
 @Injectable()
 export class LocalCaCertificateRepository extends CaCertificateRepository {
@@ -23,6 +24,8 @@ export class LocalCaCertificateRepository extends CaCertificateRepository {
   constructor(
     @InjectRepository(CaCertificateEntity)
     private readonly repository: Repository<CaCertificateEntity>,
+    @InjectRepository(DatabaseEntity)
+    private readonly databaseRepository: Repository<DatabaseEntity>,
     private readonly encryptionService: EncryptionService,
   ) {
     super();
@@ -71,7 +74,7 @@ export class LocalCaCertificateRepository extends CaCertificateRepository {
   /**
    * @inheritDoc
    */
-  async delete(id: string): Promise<void> {
+  async delete(id: string): Promise<{ affectedDatabases: string[] }> {
     this.logger.log(`Deleting certificate. id: ${id}`);
 
     // todo: 1. why we need to check if entity exists?
@@ -84,7 +87,16 @@ export class LocalCaCertificateRepository extends CaCertificateRepository {
       throw new NotFoundException();
     }
 
+    const affectedDatabases = (await this.databaseRepository
+      .createQueryBuilder('d')
+      .leftJoinAndSelect('d.caCert', 'c')
+      .where({ caCert: id })
+      .select(['d.id'])
+      .getMany()).map((e) => e.id);
+
     await this.repository.delete(id);
     this.logger.log(`Succeed to delete ca certificate: ${id}`);
+
+    return { affectedDatabases };
   }
 }