test: improve unit tests coverage for AccountService (Infection)

[diogopaz]

Part of #8053

📝 Summary

This PR adds extensive PHPUnit test coverage to AccountServiceTest to eliminate escaped mutants and improve the mutation testing score with Infection, as part of #8053.

🧪 How to test

Run the focused unit test suite for AccountService:

composer test:unit -- --filter AccountServiceTest

Optionally run Infection on this specific pair to verify mutant coverage:

vendor-bin/infection/vendor/bin/infection --filter=AccountService

⚙️ API / Back‑end changes

• Unit tests added to cover validateCreateToSign, getFileByUuid, createToSign, saveVisibleElement(s), uploadPfx, updatePfxPassword, readPfxData, and config fallbacks.
• Unit and/or integration tests added – required for backend changes

✅ Checklist

• I have read and followed the contribution guide.

🤖 AI (if applicable)

• The content of this PR was partially or fully generated using AI

[welcome]

Thanks for opening your first pull request in this repository! ✌️

[vitormattos]

@diogopaz the problems in the unit tests aren't caused by your PR. I'll fix this in a separate PR.

But have linter issues that you need to solve:

https://github.com/LibreSign/libresign/actions/runs/33325571740/job/100018743935?pr=8142

[vitormattos]

Once you fix the linter issue and push the changes, the unit tests will work fine.

[diogopaz]

Hi @vitormattos,

Sorry for the delay! I've fixed the linter issues (missing imports, multiline array trailing commas, and formatting) and updated the commit.

Thanks for the review!

[vitormattos]

I checked the PHPUnit failures.

There are two different groups of problems:

1. The IUserFolder errors in ValidateHelperTest and SignFileServiceTest are not caused by this PR. They come from tests outside the changed file and appear in the Nextcloud master jobs. We should handle this compatibility change separately.

2. The failures in AccountServiceTest are from the tests added in this PR and need to be fixed here before merge. I added inline comments for the main cases.

So you do not need to fix the unrelated IUserFolder failures as part of this PR.

[vitormattos]

I think we should test one more case before making this cache behavior part of the expected behavior.

AccountService caches signRequest, fileData and fileToSign, but the cache does not store which UUID the data belongs to.

This test calls getFileByUuid() twice with the same UUID, so it does not detect what happens with different UUIDs.

Could we also test something like:

$service->getFileByUuid('uuid-a');
$result = $service->getFileByUuid('uuid-b');

and verify that the result belongs to uuid-b?

If this fails with the current implementation, we found a bug and should fix it instead of protecting this behavior with the mutation test. This is also tracked in #8328.

[vitormattos]

The previous comment became outdated after the rebase, but the point is still valid.

This test only checks one UUID. Could you also use the same AccountService instance with two different UUIDs and verify that the second call returns the data for the second UUID?

For example:

$service = $this->getService();
$service->getFileByUuid('uuid-a');
$result = $service->getFileByUuid('uuid-b');

This is important because AccountService caches the sign request and file data. We should make sure the test does not protect a cache behavior that returns data from the previous UUID.

[vitormattos]

@diogopaz, can you rebase your branch on top of main?

I fixed part of the failing tests in this PR:

• test: use IUserFolder mocks for Nextcloud 36 #8329

[maia-andre]

Heads-up after #8366 was merged (this morning): AccountService no longer receives ValidateHelper. AccountServiceTest on main now mocks IdentityDocumentValidator ($this->identityDocumentValidator) and FileInputValidator ($this->fileInputValidator) and passes them to the constructor; the $this->validateHelper property and the use OCA\Libresign\Helper\ValidateHelper; import are gone.

After rebasing, five expectations in this PR will reference a property that no longer exists. The one-to-one mapping against current main:

This PR	On main
$this->validateHelper->method('userCanApproveValidationDocuments')->with(null, false)	$this->identityDocumentValidator->method('userCanApproveValidationDocuments')->with(null, false)
$this->validateHelper->method('userCanApproveValidationDocuments')->with($user, false) (×2)	$this->identityDocumentValidator->method(...)->with($user, false)
$this->validateHelper->expects($this->once())->method('validateBase64')->with('fake_png_data', ValidateHelper::TYPE_VISIBLE_ELEMENT_USER)	$this->fileInputValidator->expects($this->once())->method('validateBase64')->with('fake_png_data', FileInputValidator::TYPE_VISIBLE_ELEMENT_USER)

The production calls are lib/Service/AccountService.php:218 and :354 (identityDocumentValidator->userCanApproveValidationDocuments($user, false)) and :522/:525 (fileInputValidator->validateBase64(..., FileInputValidator::TYPE_VISIBLE_ELEMENT_USER)). The imports for both validators are already in the test file on main, so the use ValidateHelper line can simply be dropped. Everything else in the PR is additive and should not conflict beyond the use block.

Sorry for the extra step — happy to review the rebase if useful.

[vitormattos]

SignRequest uses dynamic getters from Entity, so PHPUnit cannot configure getId(), getFileId() or getDisplayName() on this mock. This is causing several errors in the PHPUnit job.

Could you use a real SignRequest here and set the needed values with setId(), setFileId(), setDisplayName(), etc.? The same fix is needed in the other new tests that mock these getters.

You also can look other examples with getId at other tests.

[vitormattos]

ICertificateEngine does not exist in the current code. CertificateEngineFactory::getEngine() returns IEngineHandler.

Could you use IEngineHandler in these two tests instead?

[vitormattos]

$this->policyAuthorizationService is a real PolicyAuthorizationService, not a PHPUnit mock, so ->method() cannot be used here.

For a null user, the real service already returns false from canUserManageGroupPolicies() and [] from getManageablePolicyGroupIds(). I think these two stubs can just be removed.