From 02799c972a8fcf8636f3c2db1ba9f7a98c8036c3 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 21:42:17 +0200 Subject: [PATCH] fix: imported accounts are subject to the password lifetime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every door that creates an account asks the preset service to clamp `passDateChange` to the policy's lifetime — the web form does it, and so do the API's create and edit-pass. The importers do not: `ImportBase::addAccount()` goes straight to `AccountService::create()`, which applies the permission and privacy presets but has never applied the password ones. So a fixed policy saying passwords expire after ninety days said nothing at all about the several thousand accounts that arrived through a CSV, a KeePass file or a sysPass export — which is exactly the population most likely to be carrying old credentials. The clamp is applied on import now, for all three importers, since they share this method. `checkPasswordPreset()` — the one that validates length and character classes — is deliberately *not* applied, and this is the judgement in the change rather than the code. An import is a migration of credentials that already exist somewhere else. Refusing the ones that predate the policy would mean not being able to migrate at all, and the import runs in a single transaction, so one weak password would take the whole file with it. The policy still bites the moment the account is next edited, and the lifetime clamp is what makes somebody get there rather than waiting for it to be noticed. The preset service reaches ImportBase through ImportHelper, which is how it already gets the account, category, client, tag and config services. Checked by dropping the clamp: the new test fails, asserting on the stored DTO rather than on the clamp having been called. The test needed the clamp stub to consult a per-test value rather than being re-stubbed in the test itself — the first stub registered is the one that answers, so a per-test override of a setUp stub silently does nothing. That is the second time that has caught me today. --- .../Import/Ports/ImportHelperInterface.php | 3 + .../Import/Services/ImportBase.php | 17 ++++- .../Import/Services/ImportHelper.php | 9 ++- .../Import/Services/CsvImportTest.php | 66 ++++++++++++++++++- .../Import/Services/KeepassImportTest.php | 12 +++- .../Import/Services/SyspassImportTest.php | 36 +++++++--- 6 files changed, 128 insertions(+), 15 deletions(-) diff --git a/src/Application/Import/Ports/ImportHelperInterface.php b/src/Application/Import/Ports/ImportHelperInterface.php index 58834353e..a854232a0 100644 --- a/src/Application/Import/Ports/ImportHelperInterface.php +++ b/src/Application/Import/Ports/ImportHelperInterface.php @@ -25,6 +25,7 @@ namespace SP\Application\Import\Ports; +use SP\Application\Account\Ports\AccountPresetService; use SP\Application\Account\Ports\AccountService; use SP\Application\Category\Ports\CategoryService; use SP\Application\Client\Ports\ClientService; @@ -39,6 +40,8 @@ interface ImportHelperInterface { public function getAccountService(): AccountService; + public function getAccountPresetService(): AccountPresetService; + /** * @return CategoryService */ diff --git a/src/Application/Import/Services/ImportBase.php b/src/Application/Import/Services/ImportBase.php index 88db3c7af..a8a29c4fb 100644 --- a/src/Application/Import/Services/ImportBase.php +++ b/src/Application/Import/Services/ImportBase.php @@ -31,6 +31,7 @@ use SP\Application\Application; use SP\Domain\Crypt\Hash; use SP\Domain\Account\Dtos\AccountCreateDto; +use SP\Application\Account\Ports\AccountPresetService; use SP\Application\Account\Ports\AccountService; use SP\Domain\Category\Models\Category; use SP\Domain\Category\Models\Category as CategoryModel; @@ -68,6 +69,7 @@ abstract class ImportBase extends Service implements ImportService protected int $version = 0; protected int $counter = 0; protected readonly AccountService $accountService; + protected readonly AccountPresetService $accountPresetService; /** @var CategoryService */ protected readonly CategoryService $categoryService; protected readonly ClientService $clientService; @@ -84,6 +86,7 @@ public function __construct( parent::__construct($application); $this->accountService = $importHelper->getAccountService(); + $this->accountPresetService = $importHelper->getAccountPresetService(); $this->categoryService = $importHelper->getCategoryService(); $this->clientService = $importHelper->getClientService(); $this->tagService = $importHelper->getTagService(); @@ -157,7 +160,19 @@ final protected function addAccount( $dto = $dto->mutate(['pass' => $pass, 'key' => '']); } - $this->accountService->create($dto); + // The password lifetime applies to an imported account too. Every other door that creates + // one — the web form, the API's create and edit-pass — asks the preset service for this; + // the importers went straight to AccountService::create(), which applies the permission + // and privacy presets but has never applied the password ones. So a fixed policy saying + // passwords expire after ninety days said nothing about the several thousand accounts + // that arrived through a CSV. + // + // The lifetime clamp only. checkPasswordPreset(), which validates length and character + // classes, is deliberately not applied here: an import is a migration of credentials that + // already exist elsewhere, and refusing the ones that predate the policy would mean not + // being able to migrate at all. The policy still bites when the account is next edited, + // and the clamp is what makes that happen rather than waiting for someone to notice. + $this->accountService->create($this->accountPresetService->checkPasswordExpiry($dto)); $this->counter++; } diff --git a/src/Application/Import/Services/ImportHelper.php b/src/Application/Import/Services/ImportHelper.php index b8777adf3..26485ddf0 100644 --- a/src/Application/Import/Services/ImportHelper.php +++ b/src/Application/Import/Services/ImportHelper.php @@ -25,6 +25,7 @@ namespace SP\Application\Import\Services; +use SP\Application\Account\Ports\AccountPresetService; use SP\Application\Account\Ports\AccountService; use SP\Application\Category\Ports\CategoryService; use SP\Application\Client\Ports\ClientService; @@ -46,7 +47,8 @@ public function __construct( private CategoryService $categoryService, private ClientService $clientService, private TagService $tagService, - private ConfigService $configService + private ConfigService $configService, + private AccountPresetService $accountPresetService ) { } @@ -55,6 +57,11 @@ public function getAccountService(): AccountService return $this->accountService; } + public function getAccountPresetService(): AccountPresetService + { + return $this->accountPresetService; + } + /** * @return CategoryService */ diff --git a/tests/Unit/Application/Import/Services/CsvImportTest.php b/tests/Unit/Application/Import/Services/CsvImportTest.php index 37f0ddc33..b41e4b97b 100644 --- a/tests/Unit/Application/Import/Services/CsvImportTest.php +++ b/tests/Unit/Application/Import/Services/CsvImportTest.php @@ -31,6 +31,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\MockObject\Rule\InvokedCount; use SP\Domain\Account\Dtos\AccountCreateDto; +use SP\Application\Account\Ports\AccountPresetService; use SP\Application\Account\Ports\AccountService; use SP\Domain\Category\Models\Category; use SP\Application\Category\Ports\CategoryService; @@ -59,6 +60,9 @@ class CsvImportTest extends UnitaryTestCase private AccountService|MockObject $accountService; private MockObject|CategoryService $categoryService; private ClientService|MockObject $clientService; + private AccountPresetService|MockObject $accountPresetService; + /** @var int|null What the lifetime clamp should answer with, when a test cares. */ + private ?int $clampTo = null; private FileHandlerInterface|MockObject $fileHandler; private CsvImport $csvImport; @@ -139,6 +143,52 @@ public function testDoImport() $this->csvImport->doImport($params); } + /** + * An imported account is subject to the password lifetime. + * + * Every other door that creates an account asks the preset service to clamp passDateChange to + * the policy's lifetime — the web form, and the API's create and edit-pass. The importers went + * straight to AccountService::create(), which applies the permission and privacy presets but + * has never applied the password ones, so a fixed policy saying passwords expire after ninety + * days said nothing at all about the several thousand accounts that arrived through a CSV. + * + * The stored account is whatever the clamp returned, not the DTO the importer built. + * + * @throws ImportException + * @throws FileException + */ + public function testAnImportedAccountIsSubjectToThePasswordLifetime() + { + $params = new CsvImportParamsDto($this->fileHandler, 1, 1); + + $this->fileHandler + ->expects(self::once()) + ->method('readFromCsv') + ->willReturnCallback( + static function () { + yield ['Account_name', 'Client_name', 'Category_name', 'a_url', 'a_login', 'a_password', 'a_note']; + } + ); + + $this->clientService->expects(self::once())->method('getByName')->willThrowException(NoSuchItemException::error('test')); + $this->clientService->expects(self::once())->method('create')->willReturn(100); + $this->categoryService->expects(self::once())->method('getByName')->willThrowException(NoSuchItemException::error('test')); + $this->categoryService->expects(self::once())->method('create')->willReturn(200); + + $clamped = 1_234_567_890; + + $this->clampTo = $clamped; + + $this->accountService + ->expects(self::once()) + ->method('create') + ->with( + new Callback(static fn(AccountCreateDto $dto): bool => $dto->passDateChange === $clamped) + ); + + $this->csvImport->doImport($params); + } + /** * @throws ImportException * @throws FileException @@ -396,12 +446,26 @@ protected function setUp(): void $this->categoryService = $this->createMock(CategoryService::class); $this->clientService = $this->createMock(ClientService::class); + $this->accountPresetService = $this->createStub(AccountPresetService::class); + + // The lifetime clamp runs on import. By default it changes nothing; a test that cares sets + // $clampTo. One stub rather than a per-test override, because the first stub registered is + // the one that answers — a later re-stub of the same method silently does nothing. + $this->accountPresetService + ->method('checkPasswordExpiry') + ->willReturnCallback( + fn(AccountCreateDto $dto): AccountCreateDto => $this->clampTo === null + ? $dto + : $dto->mutate(['passDateChange' => $this->clampTo]) + ); + $importHelper = new ImportHelper( $this->accountService, $this->categoryService, $this->clientService, $this->createStub(TagService::class), - $this->createStub(ConfigService::class) + $this->createStub(ConfigService::class), + $this->accountPresetService ); $crypt = $this->createStub(CryptInterface::class); diff --git a/tests/Unit/Application/Import/Services/KeepassImportTest.php b/tests/Unit/Application/Import/Services/KeepassImportTest.php index bb7e07057..a71322130 100644 --- a/tests/Unit/Application/Import/Services/KeepassImportTest.php +++ b/tests/Unit/Application/Import/Services/KeepassImportTest.php @@ -29,6 +29,7 @@ use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\MockObject\MockObject; +use SP\Application\Account\Ports\AccountPresetService; use SP\Application\Account\Ports\AccountService; use SP\Domain\Category\Models\Category; use SP\Application\Category\Ports\CategoryService; @@ -60,6 +61,7 @@ class KeepassImportTest extends UnitaryTestCase private AccountService|MockObject $accountService; private MockObject|CategoryService $categoryService; private ClientService|MockObject $clientService; + private AccountPresetService|MockObject $accountPresetService; /** * @throws Exception @@ -195,7 +197,8 @@ public function testDoImportWithEmptyGroupName() $this->categoryService, $this->clientService, $this->createStub(TagService::class), - $this->createStub(ConfigService::class) + $this->createStub(ConfigService::class), + $this->accountPresetService ); $keepassImport = new KeepassImport( @@ -216,12 +219,17 @@ protected function setUp(): void $this->categoryService = $this->createMock(CategoryService::class); $this->clientService = $this->createMock(ClientService::class); + $this->accountPresetService = $this->createStub(AccountPresetService::class); + // The lifetime clamp runs on import; by default it changes nothing. + $this->accountPresetService->method('checkPasswordExpiry')->willReturnArgument(0); + $importHelper = new ImportHelper( $this->accountService, $this->categoryService, $this->clientService, $this->createStub(TagService::class), - $this->createStub(ConfigService::class) + $this->createStub(ConfigService::class), + $this->accountPresetService ); $crypt = $this->createStub(CryptInterface::class); diff --git a/tests/Unit/Application/Import/Services/SyspassImportTest.php b/tests/Unit/Application/Import/Services/SyspassImportTest.php index cd8d61693..48b5d13e7 100644 --- a/tests/Unit/Application/Import/Services/SyspassImportTest.php +++ b/tests/Unit/Application/Import/Services/SyspassImportTest.php @@ -35,6 +35,7 @@ use RuntimeException; use SP\Infrastructure\Crypt\Crypt; use SP\Domain\Account\Dtos\AccountCreateDto; +use SP\Application\Account\Ports\AccountPresetService; use SP\Application\Account\Ports\AccountService; use SP\Domain\Category\Models\Category; use SP\Application\Category\Ports\CategoryService; @@ -74,6 +75,7 @@ class SyspassImportTest extends UnitaryTestCase private CryptInterface|MockObject $crypt; private SyspassImport $sysPassImport; private ConfigService|MockObject $configService; + private AccountPresetService|MockObject $accountPresetService; /** * @throws ImportException @@ -594,7 +596,8 @@ public function testDoImportWithEncryptedFile() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -679,7 +682,8 @@ public function testDoImportWithEncryptedFileAndCryptoException() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -747,7 +751,8 @@ public function testDoImportRefusesEncryptedFileWithNoPasswordSupplied() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -789,7 +794,8 @@ public function testDoImportDecryptsRawDataForModernFormatWithoutBase64Decoding( $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -841,7 +847,8 @@ public function testDoImportRejectsFilesEncryptedByOldSysPassVersions() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -891,7 +898,8 @@ public function testDoImportTreatsUndecodableDecryptedDataAsWrongPassword() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -951,7 +959,8 @@ public function testDoImportAbortsWhenAccountReferencesUnknownCategory() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -1001,7 +1010,8 @@ public function testDoImportAbortsWhenAccountReferencesUnknownClient() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -1067,7 +1077,8 @@ public function testDoImportRejectsEncryptedAccountsFromOldSysPassVersions() $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $document = new DOMDocument(); @@ -1229,13 +1240,18 @@ protected function setUp(): void $this->clientService = $this->createMock(ClientService::class); $this->tagService = $this->createMock(TagService::class); $this->configService = $this->createMock(ConfigService::class); + $this->accountPresetService = $this->createStub(AccountPresetService::class); + // The lifetime clamp is applied on import; by default it changes nothing. + $this->accountPresetService->method('checkPasswordExpiry') + ->willReturnArgument(0); $importHelper = new ImportHelper( $this->accountService, $this->categoryService, $this->clientService, $this->tagService, - $this->configService + $this->configService, + $this->accountPresetService ); $this->crypt = $this->createMock(CryptInterface::class);