diff --git a/CHANGELOG.md b/CHANGELOG.md index 30e36ff803..f39d71c796 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [4.4.2] - 2023-11-29 +### Improved +- PB-27616 As a user I should see improved performances when retrieving resources on the GET resources.json entry point + +### Fixed +- PB-28991 As a user emails should be resent if the first attempt failed + +## [4.4.2-test.1] - 2023-11-27 +### Improved +- PB-27616 As a user I should see improved performances when retrieving resources on the GET resources.json entry point + ## [4.4.1] - 2023-11-20 ### Improved - PB-28521 Alter the gpgkeys.uid column length to 769 to enable the synchronisation of user with very long names diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 4d9987c429..8fd756c5b7 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,18 +1,16 @@ -Release song: https://youtu.be/RbmS3tQJ7Os?si=lp8QM5B-R65C8Jek +Release song: https://youtu.be/6JNwqRF32ZI -Passbolt v4.4.1 is a maintenance release aimed at addressing issues reported by the community, which were introduced in the previous release. +Passbolt version 4.4.2 has been released, primarily as a maintenance update to address specific issues reported by users. This version includes two main fixes. -The update addresses an issue concerning user roles in email notifications. Previously, administrators received notifications when another administrator was deleted. However, the deletion of any user, regardless of their administrator status, was incorrectly reported as an administrator deletion. This issue has been resolved. +The first fix concerns the Time-based One-Time Password (TOTP) feature. In the previous version, there was an issue where users could accidentally delete the TOTP secret for a resource while editing its description from the sidebar. This has been corrected in the latest update. -We extend our gratitude to the community members who reported these issues. Your support and patience are greatly appreciated. +The second fix improves the performance of the application, specifically when users are retrieving their resources. This update is part of an ongoing effort to enhance the overall performance of the application, with further improvements planned for future releases. -## [4.4.1] - 2023-11-20 +We extend our gratitude to the community member who reported this issue. + +## [4.4.2] - 2023-11-28 ### Improved -- PB-28521 Alter the gpgkeys.uid column length to 769 to enable the synchronisation of user with very long names +- PB-27616 As a user I should see improved performances when retrieving resources on the GET resources.json entry point ### Fixed -- PB-27957 As an administrator I should be notified that an administrator was deleted only when an administrator has been deleted, and not a regular user - -### Maintenance -- PB-27927 Remove unused user_agents table -- PB-28616 Refactor the email digest plugin code for easier usage and maintainability +- PB-28991 As a user emails should be resent if the first attempt failed diff --git a/config/version.php b/config/version.php index 7ec1ae1658..d16b31b63f 100644 --- a/config/version.php +++ b/config/version.php @@ -1,8 +1,8 @@ [ - 'version' => '4.4.1', - 'name' => 'Gimme Shelter', + 'version' => '4.4.2', + 'name' => 'Is It Because I\'m Black?', ], 'php' => [ 'minVersion' => '7.4', diff --git a/plugins/PassboltCe/EmailDigest/src/Utility/Factory/EmailPreviewFactory.php b/plugins/PassboltCe/EmailDigest/src/Utility/Factory/EmailPreviewFactory.php index 9bcc634f41..9d8f19c1fd 100644 --- a/plugins/PassboltCe/EmailDigest/src/Utility/Factory/EmailPreviewFactory.php +++ b/plugins/PassboltCe/EmailDigest/src/Utility/Factory/EmailPreviewFactory.php @@ -90,6 +90,7 @@ public function buildSingleEmailDigest(Entity $emailQueueEntity): EmailDigest return (new EmailDigest()) ->addEmailData($emailQueueEntity) ->setSubject($emailQueueEntity->get('subject')) + ->setEmailIds([$emailQueueEntity->id]) ->setEmailRecipient($emailQueueEntity->get('email')); } @@ -111,6 +112,8 @@ public function buildMultipleEmailDigest(Digest $digest): EmailDigest ->setEmailRecipient($emailQueueEntity->get('email')); } + $emailDigest->setEmailIds($digest->getEmailQueueIds()); + return $emailDigest; } diff --git a/plugins/PassboltCe/EmailDigest/tests/Factory/EmailQueueFactory.php b/plugins/PassboltCe/EmailDigest/tests/Factory/EmailQueueFactory.php index 9df70921d8..c64963e94e 100644 --- a/plugins/PassboltCe/EmailDigest/tests/Factory/EmailQueueFactory.php +++ b/plugins/PassboltCe/EmailDigest/tests/Factory/EmailQueueFactory.php @@ -146,11 +146,12 @@ public function setFullBaseUrl(string $fullBaseUrl) } /** - * @param string $subject locale of the email - * @return EmailQueueFactory + * @param ?string $subject locale of the email + * @return $this */ - public function setSubject(string $subject) + public function setSubject(?string $subject = null) { + $subject = $subject ?? $this->getFaker()->sentence(3); $this->setField('template_vars.body.subject', $subject); return $this->setField('subject', $subject); @@ -164,4 +165,15 @@ public function setLocale(string $locale) { return $this->setField('template_vars.locale', $locale); } + + /** + * Sets the id of the email. Useful in unit testing, when an ID should be defined, but no persistence needed + * + * @param int|null $id email Id + * @return $this + */ + public function setId(?int $id = null) + { + return $this->setField('id', $id ?? $this->getFaker()->randomNumber(4)); + } } diff --git a/plugins/PassboltCe/EmailDigest/tests/Factory/GroupUserDeleteEmailQueueFactory.php b/plugins/PassboltCe/EmailDigest/tests/Factory/GroupUserDeleteEmailQueueFactory.php index c9d7a0c02c..475918686f 100644 --- a/plugins/PassboltCe/EmailDigest/tests/Factory/GroupUserDeleteEmailQueueFactory.php +++ b/plugins/PassboltCe/EmailDigest/tests/Factory/GroupUserDeleteEmailQueueFactory.php @@ -23,9 +23,11 @@ class GroupUserDeleteEmailQueueFactory extends EmailQueueFactory protected function setDefaultTemplate(): void { parent::setDefaultTemplate(); - $this->setTemplate(GroupUserDeleteEmailRedactor::TEMPLATE); - $this->setGroup(); - $this->setOperator(); + $this + ->setSubject() + ->setTemplate(GroupUserDeleteEmailRedactor::TEMPLATE) + ->setGroup() + ->setOperator(); } /** diff --git a/plugins/PassboltCe/EmailDigest/tests/Factory/ResourceDeleteEmailQueueFactory.php b/plugins/PassboltCe/EmailDigest/tests/Factory/ResourceDeleteEmailQueueFactory.php index 36ae6b315b..e75cf88b0d 100644 --- a/plugins/PassboltCe/EmailDigest/tests/Factory/ResourceDeleteEmailQueueFactory.php +++ b/plugins/PassboltCe/EmailDigest/tests/Factory/ResourceDeleteEmailQueueFactory.php @@ -28,6 +28,7 @@ protected function setDefaultTemplate(): void { parent::setDefaultTemplate(); $this + ->setSubject() ->setTemplate(ResourceDeleteEmailRedactor::TEMPLATE) ->setResource() ->setOperator() diff --git a/plugins/PassboltCe/EmailDigest/tests/TestCase/Command/SenderCommandTest.php b/plugins/PassboltCe/EmailDigest/tests/TestCase/Command/SenderCommandTest.php index ecf3d9548f..424dd644de 100644 --- a/plugins/PassboltCe/EmailDigest/tests/TestCase/Command/SenderCommandTest.php +++ b/plugins/PassboltCe/EmailDigest/tests/TestCase/Command/SenderCommandTest.php @@ -24,10 +24,12 @@ use App\Test\Factory\UserFactory; use App\Test\Lib\AppIntegrationTestCase; use App\Test\Lib\Utility\EmailTestTrait; +use App\Utility\UuidFactory; use Cake\Chronos\Chronos; use Cake\Console\TestSuite\ConsoleIntegrationTestTrait; use Cake\I18n\I18n; use Cake\Mailer\Mailer; +use Cake\Network\Exception\SocketException; use Passbolt\EmailDigest\Test\Factory\EmailQueueFactory; use Passbolt\EmailDigest\Test\Lib\EmailDigestMockTestTrait; use Passbolt\EmailDigest\Utility\Digest\DigestTemplateRegistry; @@ -124,17 +126,86 @@ public function testSenderCommandLocale() $this->assertMailBodyStringCount(1, ''); } - public function testSenderCommand_NoRowsAreLockedWhenThresholdIsExceeded() + public function noRowsLockedProvider(): array { - $nResourcesAdded = 15; - $operator = UserFactory::make()->withAvatar()->persist(); + return [ + ['single_email' => 1], + ['multiple_emails_in_digest' => 2], + ['threshold_exceeded' => 11], + ]; + } + + /** + * @dataProvider noRowsLockedProvider + */ + public function testSenderCommand_EmailQueueRowsAreUnlockedAfterSuccessfulSend(int $nResourcesAdded) + { + $operator = UserFactory::make()->withAvatar()->getEntity(); + $recipient = 'john@test.test'; + EmailQueueFactory::make($nResourcesAdded) + ->setRecipient($recipient) + ->setTemplate(ResourceCreateEmailRedactor::TEMPLATE) + ->setField('template_vars.body.user', $operator) + ->setField('template_vars.body.resource', [ + // Dummy data to render email without any warnings + 'id' => UuidFactory::uuid(), + 'created' => 'now', + 'name' => 'My pass', + 'secrets' => [['data' => 'foo bar baz']], + ]) + ->setField('template_vars.body.showUsername', false) + ->setField('template_vars.body.showUri', false) + ->setField('template_vars.body.showDescription', false) + ->setField('template_vars.body.showSecret', false) + ->persist(); + + $this->exec('passbolt email_digest send'); + + $this->assertExitSuccess(); + $this->assertMailCount(1); + $this->assertMailSentToAt(0, [$recipient => $recipient]); + // Make sure email queue entries are not locked + $count = EmailQueueFactory::find()->where(['locked' => true])->count(); + $this->assertEquals(0, $count); + // Make sure email queue entries are sent + $count = EmailQueueFactory::find()->where(['sent' => true, 'locked' => false])->count(); + $this->assertEquals($nResourcesAdded, $count); + } + + /** + * @dataProvider noRowsLockedProvider + */ + public function testSenderCommand_EmailQueueRowsAreUnlockedAfterFailedSend(int $nResourcesAdded) + { + $operator = UserFactory::make()->withAvatar()->getEntity(); $recipient = 'john@test.test'; EmailQueueFactory::make($nResourcesAdded) ->setRecipient($recipient) ->setTemplate(ResourceCreateEmailRedactor::TEMPLATE) ->setField('template_vars.body.user', $operator) + ->setField('template_vars.body.resource', [ + // Dummy data to render email without any warnings + 'id' => UuidFactory::uuid(), + 'created' => 'now', + 'name' => 'My pass', + 'secrets' => [['data' => 'foo bar baz']], + ]) + ->setField('template_vars.body.showUsername', false) + ->setField('template_vars.body.showUri', false) + ->setField('template_vars.body.showDescription', false) + ->setField('template_vars.body.showSecret', false) ->persist(); + $mockedEmailQueue = $this->getMockForModel( + 'EmailQueue.EmailQueue', + ['success',], + ['table' => 'email_queue',] + ); + $mockedEmailQueue + ->expects($this->once()) + ->method('success') + ->willThrowException(new SocketException()); + $this->exec('passbolt email_digest send'); $this->assertExitSuccess(); @@ -143,6 +214,9 @@ public function testSenderCommand_NoRowsAreLockedWhenThresholdIsExceeded() // Make sure email queue entries are not locked $count = EmailQueueFactory::find()->where(['locked' => true])->count(); $this->assertEquals(0, $count); + // Make sure email queue entries are not sent + $count = EmailQueueFactory::find()->where(['sent' => false, 'locked' => false])->count(); + $this->assertEquals($nResourcesAdded, $count); } public function testSenderCommandMultipleDigests() @@ -174,6 +248,9 @@ public function testSenderCommandMultipleDigests() $this->exec('passbolt email_digest send'); $this->assertExitSuccess(); + $sentCount = EmailQueueFactory::find()->where(['sent' => true])->count(); + $this->assertSame($nEmailsSent * 2, $sentCount); + $this->assertMailCount(2); $subject = $user->profile->full_name . ' has made changes on several resources'; $this->assertMailSubjectContainsAt(0, $subject); diff --git a/plugins/PassboltCe/EmailDigest/tests/TestCase/Service/SendEmailBatchServiceUnitTest.php b/plugins/PassboltCe/EmailDigest/tests/TestCase/Service/SendEmailBatchServiceUnitTest.php index 9e9cb665fd..1bd313c9c4 100644 --- a/plugins/PassboltCe/EmailDigest/tests/TestCase/Service/SendEmailBatchServiceUnitTest.php +++ b/plugins/PassboltCe/EmailDigest/tests/TestCase/Service/SendEmailBatchServiceUnitTest.php @@ -34,6 +34,9 @@ use Passbolt\EmailDigest\Utility\Digest\DigestTemplateRegistry; use Passbolt\Locale\LocalePlugin; +/** + * @covers \Passbolt\EmailDigest\Service\SendEmailBatchService + */ class SendEmailBatchServiceUnitTest extends TestCase { use EmailTestTrait; @@ -64,13 +67,13 @@ public function tearDown(): void parent::tearDown(); } - public function testSendEmailBatchService_On_No_Email() + public function testSendEmailBatchServiceUnitTest_On_No_Email() { $this->service->sendNextEmailsBatch([]); $this->assertMailCount(0); } - public function testSendEmailBatchService_On_Email_With_Unknown_Template() + public function testSendEmailBatchServiceUnitTest_On_Email_With_Unknown_Template() { $email = EmailQueueFactory::make()->setTemplate('foo')->getEntity(); $this->expectException(MissingTemplateException::class); @@ -90,7 +93,7 @@ public function withAndWithoutDigestTemplate(): array /** * @dataProvider withAndWithoutDigestTemplate */ - public function testSendEmailBatchService_On_One_Email_Translated(bool $withDigestTemplate) + public function testSendEmailBatchServiceUnitTest_On_One_Email_Translated(bool $withDigestTemplate) { if (!$withDigestTemplate) { DigestTemplateRegistry::clearInstance(); @@ -99,6 +102,7 @@ public function testSendEmailBatchService_On_One_Email_Translated(bool $withDige $resourceDeleted = ResourceFactory::make()->getEntity(); $subjectTranslated = 'Le sujet'; $email = ResourceDeleteEmailQueueFactory::make() + ->setId() ->setOperator($operator) ->setResource($resourceDeleted) ->setSubject($subjectTranslated) @@ -117,7 +121,7 @@ public function testSendEmailBatchService_On_One_Email_Translated(bool $withDige /** * @dataProvider withAndWithoutDigestTemplate */ - public function testSendEmailBatchService_On_Multiple_Emails_Same_Recipient_Below_Threshold( + public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Same_Recipient_Below_Threshold( bool $withDigestTemplate ) { if (!$withDigestTemplate) { @@ -130,6 +134,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Same_Recipient_Belo $recipient = 'foo@passbolt.com'; $nEmails = rand(2, 10); $emails = ResourceDeleteEmailQueueFactory::make($nEmails) + ->setId() ->setRecipient($recipient) ->setOperator($operator) ->setResource($resourceDeleted) @@ -159,7 +164,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Same_Recipient_Belo /** * @dataProvider withAndWithoutDigestTemplate */ - public function testSendEmailBatchService_On_Same_Digest_Template_Various_Recipients( + public function testSendEmailBatchServiceUnitTest_On_Same_Digest_Template_Various_Recipients( bool $withDigestTemplate ) { if (!$withDigestTemplate) { @@ -173,12 +178,14 @@ public function testSendEmailBatchService_On_Same_Digest_Template_Various_Recipi $nEmails1 = rand(2, 10); $nEmails2 = rand(2, 10); $emails1 = ResourceDeleteEmailQueueFactory::make($nEmails1) + ->setId() ->setRecipient($recipient1) ->setOperator($operator) ->setResource($resourceDeleted1) ->setSubject($subject) ->getEntities(); $emails2 = ResourceDeleteEmailQueueFactory::make($nEmails2) + ->setId() ->setRecipient($recipient2) ->setOperator($operator) ->setResource($resourceDeleted2) @@ -222,7 +229,7 @@ public function testSendEmailBatchService_On_Same_Digest_Template_Various_Recipi /** * @dataProvider withAndWithoutDigestTemplate */ - public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_Below_Threshold( + public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Multiple_Operators_Below_Threshold( bool $withDigestTemplate ) { if (!$withDigestTemplate) { @@ -236,6 +243,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ $nEmails1 = rand(2, 10); $nEmails2 = rand(2, 10); $emails1 = ResourceDeleteEmailQueueFactory::make($nEmails1) + ->setId() ->setRecipient($recipient) ->setOperator($operator1) ->setResource($resourceDeleted1) @@ -243,6 +251,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ ->setLocale('fr-FR') ->getEntities(); $emails2 = ResourceDeleteEmailQueueFactory::make($nEmails2) + ->setId() ->setRecipient($recipient) ->setOperator($operator2) ->setResource($resourceDeleted2) @@ -287,7 +296,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ /** * @dataProvider withAndWithoutDigestTemplate */ - public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_Below_And_Above_Threshold( + public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Multiple_Operators_Below_And_Above_Threshold( bool $withDigestTemplate ) { if (!$withDigestTemplate) { @@ -297,10 +306,11 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ [$resourceDeleted1, $resourceDeleted2] = ResourceFactory::make(2)->getEntities(); $subjectTranslated = 'Le sujet'; $recipient = 'foo@passbolt.com'; - $nEmails1 = 10 + rand(2, 10); - $nEmails2 = rand(2, 10); + $nEmails1 = 12; + $nEmails2 = 2; // These emails are above the threshold $emails1 = ResourceDeleteEmailQueueFactory::make($nEmails1) + ->setId() ->setRecipient($recipient) ->setOperator($operator1) ->setResource($resourceDeleted1) @@ -308,6 +318,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ ->setLocale('fr-FR') ->getEntities(); $emails2 = ResourceDeleteEmailQueueFactory::make($nEmails2) + ->setId() ->setRecipient($recipient) ->setOperator($operator2) ->setResource($resourceDeleted2) @@ -350,7 +361,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ } } - public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_Multiple_Digest_Templates_Below_And_Above_Threshold() + public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Multiple_Operators_Multiple_Digest_Templates_Below_And_Above_Threshold() { DigestTemplateRegistry::clearInstance(); // Emails of the Group User delete template should be sent first @@ -416,7 +427,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_ $this->assertMailSubjectContainsAt(3, $operator2->profile->full_name . ' has made changes on several resources'); } - public function testSendEmailBatchService_On_Multiple_Full_Base_Url() + public function testSendEmailBatchServiceUnitTest_On_Multiple_Full_Base_Url() { $recipient = 'test@passbolt.com'; $subject = 'Some subject'; diff --git a/plugins/PassboltCe/Folders/src/Model/Behavior/FolderizableBehavior.php b/plugins/PassboltCe/Folders/src/Model/Behavior/FolderizableBehavior.php index 683d6cdb06..298d96d9e5 100644 --- a/plugins/PassboltCe/Folders/src/Model/Behavior/FolderizableBehavior.php +++ b/plugins/PassboltCe/Folders/src/Model/Behavior/FolderizableBehavior.php @@ -114,7 +114,7 @@ public function initialize(array $config): void * @return \Cake\ORM\Query * @see $_defaultConfig */ - public function findFolderParentId(Query $query, array $options) + public function findFolderParentId(Query $query, array $options): Query { return $this->formatResults($query, $options['user_id']); } @@ -126,20 +126,22 @@ public function findFolderParentId(Query $query, array $options) * @param string $userId The user id for whom the request has been executed. * @return \Cake\ORM\Query */ - public function formatResults(Query $query, string $userId) + public function formatResults(Query $query, string $userId): Query { return $query->formatResults(function (CollectionInterface $results) use ($userId) { $itemsIds = $results->extract('id')->toArray(); $itemsFolderParentIdsHash = $this->getItemsFolderParentIdHash($itemsIds, $userId); $itemsUsageHash = $this->getItemsUsageHash($itemsIds); - return $results->map(function (EntityInterface $entity) use ($itemsFolderParentIdsHash, $itemsUsageHash) { - if (array_key_exists($entity->get('id'), $itemsFolderParentIdsHash)) { - $folderParentId = $itemsFolderParentIdsHash[$entity->get('id')]; + // The entity passed may be an entity interface, or an array if the hydration has been disabled + return $results->map(function ($entity) use ($itemsFolderParentIdsHash, $itemsUsageHash) { + $entityId = $entity['id']; + if (array_key_exists($entityId, $itemsFolderParentIdsHash)) { + $folderParentId = $itemsFolderParentIdsHash[$entityId]; $entity = $this->addFolderParentIdProperty($entity, $folderParentId); } - if (array_key_exists($entity->get('id'), $itemsUsageHash)) { - $isPersonal = $itemsUsageHash[$entity->get('id')] === 1; + if (array_key_exists($entityId, $itemsUsageHash)) { + $isPersonal = $itemsUsageHash[$entityId] === 1; $entity = $this->addPersonalStatusProperty($entity, $isPersonal); } @@ -214,14 +216,18 @@ private function getItemsUsageHash(array $itemsIds) /** * Add the folder_parent_id property to an entity * - * @param \Cake\Datasource\EntityInterface $entity The target entity + * @param array|\Cake\Datasource\EntityInterface $entity The target entity * @param string|null $folderParentId The folder parent id - * @return \Cake\Datasource\EntityInterface + * @return array|\Cake\Datasource\EntityInterface */ - private function addFolderParentIdProperty(EntityInterface $entity, ?string $folderParentId = null) + private function addFolderParentIdProperty($entity, ?string $folderParentId = null) { - $entity->setVirtual([self::FOLDER_PARENT_ID_PROPERTY], true); - $entity->set(self::FOLDER_PARENT_ID_PROPERTY, $folderParentId); + if ($entity instanceof EntityInterface) { + $entity->setVirtual([self::FOLDER_PARENT_ID_PROPERTY], true); + $entity->set(self::FOLDER_PARENT_ID_PROPERTY, $folderParentId); + } else { + $entity[self::FOLDER_PARENT_ID_PROPERTY] = $folderParentId; + } return $entity; } @@ -229,14 +235,18 @@ private function addFolderParentIdProperty(EntityInterface $entity, ?string $fol /** * Add the personal status property to an entity * - * @param \Cake\Datasource\EntityInterface $entity The target entity + * @param array|\Cake\Datasource\EntityInterface $entity The target entity * @param bool $isPersonal The status - * @return \Cake\Datasource\EntityInterface + * @return array|\Cake\Datasource\EntityInterface */ - private function addPersonalStatusProperty(EntityInterface $entity, bool $isPersonal) + private function addPersonalStatusProperty($entity, bool $isPersonal) { - $entity->setVirtual([self::PERSONAL_PROPERTY], true); - $entity->set(self::PERSONAL_PROPERTY, $isPersonal); + if ($entity instanceof EntityInterface) { + $entity->setVirtual([self::PERSONAL_PROPERTY], true); + $entity->set(self::PERSONAL_PROPERTY, $isPersonal); + } else { + $entity[self::PERSONAL_PROPERTY] = $isPersonal; + } return $entity; } diff --git a/plugins/PassboltCe/Log/src/Model/Table/SecretAccessesTable.php b/plugins/PassboltCe/Log/src/Model/Table/SecretAccessesTable.php index 8b1a9c4a9a..9f78dabbe8 100644 --- a/plugins/PassboltCe/Log/src/Model/Table/SecretAccessesTable.php +++ b/plugins/PassboltCe/Log/src/Model/Table/SecretAccessesTable.php @@ -131,20 +131,32 @@ public function buildEntity(array $data): SecretAccess } /** - * Create a new SecretAccess + * Create a new SecretAccess from a secret entity * - * @param \App\Model\Entity\Secret $secret the secret entity * @param \App\Utility\UserAccessControl $uac user access control object + * @param \App\Model\Entity\Secret $secret the secret entity * @return bool|\Cake\Datasource\EntityInterface|false|mixed */ - public function create(Secret $secret, UserAccessControl $uac) + public function createFromSecretEntity(UserAccessControl $uac, Secret $secret) { - $userId = $uac->getId(); + return $this->createFromSecretDetails($uac, $secret->resource_id, $secret->id); + } + /** + * Create a new SecretAccess from a secret entity + * + * @param \App\Utility\UserAccessControl $uac user access control object + * @param string $resourceId The resource identifier + * @param string $secretId The secret identifier + * @return bool|\Cake\Datasource\EntityInterface|false|mixed + * @throws \App\Error\Exception\ValidationException + */ + public function createFromSecretDetails(UserAccessControl $uac, string $resourceId, string $secretId) + { $data = [ - 'user_id' => $userId, - 'resource_id' => $secret->resource_id, - 'secret_id' => $secret->id, + 'user_id' => $uac->getId(), + 'resource_id' => $resourceId, + 'secret_id' => $secretId, ]; // Check validation rules. diff --git a/src/Controller/Component/ApiPaginationComponent.php b/src/Controller/Component/ApiPaginationComponent.php index 95d958340b..4b70b1a5ba 100644 --- a/src/Controller/Component/ApiPaginationComponent.php +++ b/src/Controller/Component/ApiPaginationComponent.php @@ -16,23 +16,18 @@ */ namespace App\Controller\Component; +use App\Datasource\Paging\NumericCountAwarePaginator; use BryanCrowe\ApiPagination\Controller\Component\ApiPaginationComponent as BaseApiComponent; use Cake\Event\Event; use Cake\Http\Exception\BadRequestException; use Cake\Http\ServerRequest; use Cake\Utility\Inflector; -/** - * @property \Authentication\Controller\Component\AuthenticationComponent $Authentication - */ class ApiPaginationComponent extends BaseApiComponent { public const MAX_LIMIT = 1000000; - /** - * @var array - */ - public $defaultConfig = [ + public array $defaultConfig = [ 'visible' => [ 'count', 'limit', @@ -40,10 +35,7 @@ class ApiPaginationComponent extends BaseApiComponent ], ]; - /** - * @var array - */ - public $paginate; + public array $paginate; /** * @inheritDoc @@ -57,6 +49,7 @@ public function initialize(array $config): void $order = $this->parseQuery(); $this->setPaginationOptions($order); $this->removeSortingPaginationFromRequestQuery($order); + $this->getController()->paginate['className'] = NumericCountAwarePaginator::class; } /** diff --git a/src/Controller/Resources/ResourcesIndexController.php b/src/Controller/Resources/ResourcesIndexController.php index 2fbebfb15a..842494a1c2 100644 --- a/src/Controller/Resources/ResourcesIndexController.php +++ b/src/Controller/Resources/ResourcesIndexController.php @@ -20,6 +20,8 @@ use App\Controller\AppController; use Cake\Core\Configure; use Cake\Http\Exception\InternalErrorException; +use Cake\ORM\Query; +use Cake\Utility\Hash; /** * @property \BryanCrowe\ApiPagination\Controller\Component\ApiPaginationComponent $ApiPagination @@ -83,32 +85,43 @@ public function index() $options = $this->QueryString->get($whitelist); // Retrieve the resources. - $resources = $this->Resources->findIndex($this->User->id(), $options); - $this->_logSecretAccesses($resources->all()->toArray()); + $resources = $this->Resources->findIndex($this->User->id(), $options)->disableHydration(); $this->paginate($resources); + $this->_logSecretAccesses($resources, $options); $this->success(__('The operation was successful.'), $resources); } /** * Log secrets accesses in secretAccesses table. * - * @param array $resources resources + * @param \Cake\ORM\Query $resources resources + * @param array $queryOptions The query options * @return void */ - protected function _logSecretAccesses(array $resources) + protected function _logSecretAccesses(Query $resources, array $queryOptions) { + $containSecret = (bool)Hash::get($queryOptions, 'contain.secret'); + if (!$containSecret) { + return; + } + if (!$this->Resources->getAssociation('Secrets')->hasAssociation('SecretAccesses')) { return; } foreach ($resources as $resource) { - if (!isset($resource->secrets)) { + $secrets = Hash::get($resource, 'secrets'); + if (!isset($secrets)) { continue; } - foreach ($resource->secrets as $secret) { + foreach ($secrets as $secret) { try { - $this->Resources->Secrets->SecretAccesses->create($secret, $this->User->getAccessControl()); + $this->Resources->Secrets->SecretAccesses->createFromSecretDetails( + $this->User->getAccessControl(), + Hash::get($secret, 'resource_id'), + Hash::get($secret, 'id'), + ); } catch (\Exception $e) { throw new InternalErrorException('Could not log secret access entry.', 500, $e); } diff --git a/src/Controller/Resources/ResourcesViewController.php b/src/Controller/Resources/ResourcesViewController.php index 57d73ee963..5881d762cb 100644 --- a/src/Controller/Resources/ResourcesViewController.php +++ b/src/Controller/Resources/ResourcesViewController.php @@ -98,7 +98,7 @@ protected function _logSecretAccesses(Resource $resource): void try { /** @var \Passbolt\Log\Model\Table\SecretAccessesTable $SecretAccesses */ $SecretAccesses = $Secrets->getAssociation('SecretAccesses'); - $SecretAccesses->create($secret, $this->User->getAccessControl()); + $SecretAccesses->createFromSecretEntity($this->User->getAccessControl(), $secret); } catch (Exception $e) { throw new InternalErrorException('Could not log secret access entry.', 500, $e); } diff --git a/src/Controller/Secrets/SecretsViewController.php b/src/Controller/Secrets/SecretsViewController.php index 3834bdbe3f..70a3c36433 100644 --- a/src/Controller/Secrets/SecretsViewController.php +++ b/src/Controller/Secrets/SecretsViewController.php @@ -84,7 +84,7 @@ protected function _logSecretAccesses(Secret $secret, UserAccessControl $uac) { try { if ($this->Secrets->hasAssociation('SecretAccesses')) { - $this->Secrets->SecretAccesses->create($secret, $uac); + $this->Secrets->SecretAccesses->createFromSecretEntity($uac, $secret); } } catch (Exception $e) { throw new InternalErrorException('Could not log secret access entry.', 500, $e); diff --git a/src/Datasource/Paging/NumericCountAwarePaginator.php b/src/Datasource/Paging/NumericCountAwarePaginator.php new file mode 100644 index 0000000000..5c6f0a5a5d --- /dev/null +++ b/src/Datasource/Paging/NumericCountAwarePaginator.php @@ -0,0 +1,43 @@ +applyOptions([self::IS_COUNTING => true]); + + return parent::getCount($query, $data); + } +} diff --git a/src/Model/Entity/Avatar.php b/src/Model/Entity/Avatar.php index d9b18a03ed..0474ae591c 100644 --- a/src/Model/Entity/Avatar.php +++ b/src/Model/Entity/Avatar.php @@ -16,8 +16,6 @@ */ namespace App\Model\Entity; -use App\View\Helper\AvatarHelper; -use Cake\Core\Configure; use Cake\ORM\Entity; use Psr\Http\Message\StreamInterface; @@ -28,12 +26,9 @@ * @property \Cake\I18n\FrozenTime $created_at * @property \Cake\I18n\FrozenTime|null $updated_at * @property \App\Model\Entity\Profile $profile - * @property-read array $url */ class Avatar extends Entity { - protected $_virtual = ['url']; - /** * The avatar data never needs to be served. it is stored in cache. * @@ -57,27 +52,6 @@ class Avatar extends Entity '*' => true, ]; - /** - * Url virtual field implementation. - * - * @return array - */ - protected function _getUrl() - { - $sizes = Configure::read('FileStorage.imageSizes.Avatar'); - $avatarsPath = []; - // Add path for each available size. - foreach ($sizes as $size => $filters) { - $avatarsPath[$size] = AvatarHelper::getAvatarUrl([ - 'id' => $this->id, - 'data' => $this->data, - ], $size); - } - - // Transform original model to add paths. - return $avatarsPath; - } - /** * Get data in string format. * diff --git a/src/Model/Event/TableFindIndexBefore.php b/src/Model/Event/TableFindIndexBefore.php index 26274ea13c..d81e2a858a 100644 --- a/src/Model/Event/TableFindIndexBefore.php +++ b/src/Model/Event/TableFindIndexBefore.php @@ -92,9 +92,9 @@ private function setQuery(Query $query) * @param \Cake\ORM\Query $query Query * @param \App\Model\Table\Dto\FindIndexOptions $options Options * @param \Cake\ORM\Table $table Table - * @return \App\Model\Event\TableFindIndexBefore + * @return self */ - public static function create(Query $query, FindIndexOptions $options, Table $table) + public static function create(Query $query, FindIndexOptions $options, Table $table): self { return new static(static::EVENT_NAME, $table, [ 'query' => $query, diff --git a/src/Model/Table/AvatarsTable.php b/src/Model/Table/AvatarsTable.php index a693c8a095..d9b94b9361 100644 --- a/src/Model/Table/AvatarsTable.php +++ b/src/Model/Table/AvatarsTable.php @@ -193,16 +193,32 @@ public function afterDelete(Event $event, Avatar $avatar, \ArrayObject $options) * Used mainly to populate an avatar when no there is no result with the default avatar url. * * @param \Cake\Collection\CollectionInterface $avatars list of avatars (\App\Model\Entity\Avatar) + * @param bool $isHydrationEnabled if hydration is enabled, return an Avatar object, otherwise an array * @return mixed - * @deprecated the fallback avatar url is handled by the AvatarHelper. */ - public static function formatResults(CollectionInterface $avatars) + private static function formatResults(CollectionInterface $avatars, bool $isHydrationEnabled) { - return $avatars->map(function ($avatar) { + return $avatars->map(function ($avatar) use ($isHydrationEnabled) { + $sizes = Configure::read('FileStorage.imageSizes.Avatar'); + $url = []; + // Add path for each available size. + foreach ($sizes as $size => $filters) { + $url[$size] = AvatarHelper::getAvatarUrl([ + 'id' => $avatar['id'] ?? null, + ], $size); + } + if (empty($avatar)) { // If avatar is empty, we instantiate one. // The virtual field will take care of retrieving the default avatar. - $avatar = new Avatar(); + $avatar = $isHydrationEnabled ? new Avatar() : []; + } + + if ($avatar instanceof Avatar) { + $avatar->setVirtual(['url'], true); + $avatar->set('url', (object)$url); + } else { + $avatar['url'] = $url; } return $avatar; @@ -222,8 +238,8 @@ public static function addContainAvatar(): array // Formatter for empty avatars. return $q ->select(['Avatars.id', 'Avatars.profile_id', 'Avatars.created', 'Avatars.modified']) - ->formatResults(function (CollectionInterface $avatars) { - return AvatarsTable::formatResults($avatars); + ->formatResults(function (CollectionInterface $avatars, Query $mainQuery) { + return AvatarsTable::formatResults($avatars, $mainQuery->isHydrationEnabled()); }); }, ]; diff --git a/src/Model/Traits/Resources/ResourcesFindersTrait.php b/src/Model/Traits/Resources/ResourcesFindersTrait.php index 01d924b680..f9c4e218a0 100644 --- a/src/Model/Traits/Resources/ResourcesFindersTrait.php +++ b/src/Model/Traits/Resources/ResourcesFindersTrait.php @@ -55,7 +55,6 @@ public function findIndex(string $userId, ?array $options = []) $event = TableFindIndexBefore::create($query, $findIndexOptions, $this); - /** @var \App\Model\Event\TableFindIndexBefore $event */ $this->getEventManager()->dispatch($event); // Filter out deleted resources diff --git a/tests/TestCase/Controller/Avatars/AvatarsViewControllerTest.php b/tests/TestCase/Controller/Avatars/AvatarsViewControllerTest.php index 3e5fa414a0..4730dc5c2b 100644 --- a/tests/TestCase/Controller/Avatars/AvatarsViewControllerTest.php +++ b/tests/TestCase/Controller/Avatars/AvatarsViewControllerTest.php @@ -111,13 +111,6 @@ public function testViewAvatarsViewController_ViewExistentAvatar(string $format) $this->get('avatars/view/' . $avatar->id . '/' . $format . AvatarHelper::IMAGE_EXTENSION); $this->assertResponseEquals($expectedFileContent); - - // Ensure that the virtual field is correctly constructed. - $virtualField = [ - AvatarsConfigurationService::FORMAT_MEDIUM => AvatarHelper::getAvatarUrl($avatar->toArray(), AvatarsConfigurationService::FORMAT_MEDIUM), - AvatarsConfigurationService::FORMAT_SMALL => AvatarHelper::getAvatarUrl($avatar->toArray()), - ]; - $this->assertSame($virtualField, $avatar->url); } /** @@ -142,13 +135,6 @@ public function testAvatarsViewController_ViewExistentAvatarWithDeletedFile(stri $this->get('avatars/view/' . $avatar->id . '/' . $format . AvatarHelper::IMAGE_EXTENSION); $this->assertResponseEquals($expectedFileContent); - - // Ensure that the virtual field is correctly constructed. - $virtualField = [ - AvatarsConfigurationService::FORMAT_MEDIUM => AvatarHelper::getAvatarUrl($avatar->toArray(), AvatarsConfigurationService::FORMAT_MEDIUM), - AvatarsConfigurationService::FORMAT_SMALL => AvatarHelper::getAvatarUrl($avatar->toArray()), - ]; - $this->assertSame($virtualField, $avatar->url); } /** diff --git a/tests/TestCase/Model/Table/Avatars/AddContainAvatarTest.php b/tests/TestCase/Model/Table/Avatars/AddContainAvatarTest.php index e76abb1b15..6fd6563c64 100644 --- a/tests/TestCase/Model/Table/Avatars/AddContainAvatarTest.php +++ b/tests/TestCase/Model/Table/Avatars/AddContainAvatarTest.php @@ -35,9 +35,18 @@ public function setUp(): void { parent::setUp(); $this->Users = $this->fetchTable('Users'); + $this->loadRoutes(); } - public function testAvatarsTableAddContainAvatar_Should_Not_Retrieve_Avatar_Data() + public function enableHydration(): array + { + return [[true], [false]]; + } + + /** + * @dataProvider enableHydration + */ + public function testAvatarsTableAddContainAvatar_Should_Not_Retrieve_Avatar_Data(bool $isHydrationEnabled) { $user = UserFactory::make()->withAvatar()->user()->persist(); @@ -47,14 +56,18 @@ public function testAvatarsTableAddContainAvatar_Should_Not_Retrieve_Avatar_Data ->contain([ 'Profiles' => AvatarsTable::addContainAvatar(), ]) + ->enableHydration($isHydrationEnabled) ->firstOrFail(); - $this->assertSame($user->profile->avatar->id, $retrievedUser->profile->avatar->id); + $this->assertSame($user->profile->avatar->id, $retrievedUser['profile']['avatar']['id']); $this->assertNotNull($user->profile->avatar->data); - $this->assertNull($retrievedUser->profile->avatar->data ?? null); + $this->assertNull($retrievedUser['profile']['avatar']['data'] ?? null); } - public function testAvatarsTableAddContainAvatar_On_Empty_Avatar() + /** + * @dataProvider enableHydration + */ + public function testAvatarsTableAddContainAvatar_On_Empty_Avatar(bool $isHydrationEnabled) { $user = UserFactory::make()->user()->persist(); @@ -64,12 +77,17 @@ public function testAvatarsTableAddContainAvatar_On_Empty_Avatar() ->contain([ 'Profiles' => AvatarsTable::addContainAvatar(), ]) + ->enableHydration($isHydrationEnabled) ->firstOrFail(); $this->assertNull($user->profile->avatar->id ?? null); $this->assertNull($user->profile->avatar->data ?? null); - $this->assertNull($retrievedUser->profile->avatar->id ?? null); - $this->assertNull($retrievedUser->profile->avatar->data ?? null); - $this->assertIsArray($retrievedUser->profile->avatar->url); + $this->assertNull($retrievedUser['profile']['avatar']['id'] ?? null); + $this->assertNull($retrievedUser['profile']['avatar']['data'] ?? null); + if ($isHydrationEnabled) { + $this->assertIsObject($retrievedUser['profile']['avatar']['url']); + } else { + $this->assertIsArray($retrievedUser['profile']['avatar']['url']); + } } } diff --git a/tests/TestCase/Model/Table/Permissions/FindViewAcoPermissionsTest.php b/tests/TestCase/Model/Table/Permissions/FindViewAcoPermissionsTest.php index f77a3ce571..2d322ab31f 100644 --- a/tests/TestCase/Model/Table/Permissions/FindViewAcoPermissionsTest.php +++ b/tests/TestCase/Model/Table/Permissions/FindViewAcoPermissionsTest.php @@ -81,12 +81,7 @@ public function testContainUserProfileAvatar() $permission = Hash::extract($permissions->toArray(), '{n}[aro=User]')[0]; $this->assertPermissionAttributes($permission); $this->assertProfileAttributes($permission->user->profile); - $this->assertObjectHasAttribute('avatar', $permission->user->profile); - $this->assertArrayHasKey('url', $permission->user->profile->avatar); - $this->assertArrayHasKey('medium', $permission->user->profile->avatar->url); - $this->assertArrayHasKey('small', $permission->user->profile->avatar->url); - $this->assertStringContainsString('/img/avatar/user_medium.png', $permission->user->profile->avatar->url['medium']); - $this->assertStringContainsString('/img/avatar/user.png', $permission->user->profile->avatar->url['small']); + $this->assertObjectHasAttributes(['small', 'medium'], $permission->user->profile->avatar->url); } public function testPermissions()