feat: setup check implementation by guilhermercarvalho · Pull Request #7070 · LibreSign/libresign

guilhermercarvalho · 2026-03-04T02:18:54Z

Related Issue

Issue Number: #6590

Pull Request Type

Feature
Refactoring

Pull request checklist

Did you explain or provide a way of how can we test your code ?
If your pull request is related to frontend modifications provide a print of before and after screen
Did you provide a general summary of your changes ?
Try to limit your pull request to one type, submit multiple pull requests if needed
I implemented tests that cover my contribution

Summary of Changes

This PR replaces the custom ConfigureCheckService with a set of specialized classes implementing Nextcloud's native ISetupCheck interface. This improves maintainability, reuses core APIs, and makes the checks visible in the admin overview and available via occ setupchecks:check.

The following checks were created, preserving all original logic from ConfigureCheckService:

Check Name	Priority	Category	Source Method
`JavaSetupCheck`	must-have	`system`	`ConfigureCheckService::checkJava()`
`JSignPdfSetupCheck`	must-have	`system`	`ConfigureCheckService::checkJSignPdf()`
`PDFtkSetupCheck`	must-have	`system`	`ConfigureCheckService::checkPdftk()`
`CertificateEngineSetupCheck`	must-have	`security`	`ConfigureCheckService::checkCertificate()`
`PopplerSetupCheck`	nice-to-have	`system`	`ConfigureCheckService::checkPoppler()`
`ImagickSetupCheck`	nice-to-have	`system`	`ConfigureCheckService::checkImagick()`

All checks are registered in Application.php. The old ConfigureCheckService has been removed (not just deprecated) as it is fully replaced by the new implementation.

Key Changes

Command occ libresign:configure:check now uses SetupCheckResultService to aggregate results, preserving the existing --sign (system category) and --certificate (security category) filters.
AdminController endpoints and SSE events now use the new service, removing the need for manual cache clearing (disableCache() is gone).
Certificate engine handler (AEngineHandler.php): the filterNameValue method now returns OU values as a comma-separated string instead of an array. This change was made to align with the new setup check structure and to fix a failing test in AdminControllerTest. Note: I'm not entirely sure about the full impact of this change on certificate generation; a careful review of this part is highly appreciated.
Tests were completely rewritten to mock exec() and file_exists() inside the SetupCheck namespace, ensuring reliable unit testing without affecting global functions.

Screenshots

How to Test

Via web interface
- Navigate to Administration settings → Overview.
- Scroll to the "Setup checks" section.
- Verify that all six LibreSign checks are present in the list (Java, JSignPdf, PDFtk, Poppler, Imagick, CertificateEngine).
- Their statuses will reflect your actual system configuration:
  - Poppler and Imagick should show success if installed (or info if missing).
  - CertificateEngine will likely show error until configured (e.g., openssl engine not set up).
- This confirms the checks are properly registered and executing.
Via CLI
```
occ setupchecks:check
```
The output should include all LibreSign checks (Java, JSignPdf, PDFtk, Poppler, Imagick, CertificateEngine).

Run unit tests

composer test:unit -- --filter SetupCheck

All tests should pass.

Before / After CLI Output

Before (using occ libresign:configure:check):

...
success   java                Java version: openjdk version "21.0.8" 2025-07-15 LTS
success   java                Java binary: /path/to/java
success   pdftk               PDFtk version: 3.3.3
success   pdftk               PDFtk path: /path/to/pdftk.jar
...
error    openssl-configure   OpenSSL (root certificate) not configured.

After (same command, now using new checks):

...
success   Java                Java version: openjdk version "21.0.8" 2025-07-15 LTS
                             Java binary: /path/to/java
success   JSignPdf            JSignPdf version: 2.3.0
                             JSignPdf path: /path/to/jsignpdf.jar
success   PDFtk               PDFtk version: 3.3.3
                             PDFtk path: /path/to/pdftk.jar
success   Poppler             pdfsig version: 25.03.0, pdfinfo version: 25.03.0
success   Imagick             Imagick extension is loaded
error    CertificateEngine   [ERROR] OpenSSL (root certificate) not configured.

Additional Notes for Reviewers

Backward compatibility: The legacy API endpoint /api/v1/admin/configure-check now returns data via SetupCheckResultService::getLegacyFormattedChecks(), ensuring no breakage for existing frontend code.
Cache handling: The removed disableCache() method was only used during SSE updates; the new implementation calls the setup check manager directly, which already handles caching appropriately.
Category mapping: --sign shows checks with category system, --certificate shows security. This matches the original intent and is clearly documented.
Testing approach: Global functions exec and file_exists are mocked by redefining them in the OCA\Libresign\SetupCheck namespace only during tests – a safe and isolated method.
OU formatting change: The modification in AEngineHandler (returning a string instead of an array for OrganizationalUnit) was necessary to pass tests but may have unforeseen consequences. Please review this part carefully, especially regarding certificate generation and compatibility with different certificate engines.

Adds trait with verifyResourceIntegrity and getErrorAndTipFromVerify methods used by multiple setup checks. Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Add verifyResourceIntegrity method - [x] Add getErrorAndTipFromVerify method with translated messages - [x] Declare required properties (signSetupService, urlGenerator, appManager, logger) - [x] Use IL10N for all user-facing strings Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds ExecMock, FileSystemMock, SetupCheckFunctions to mock system calls, and updates bootstrap.php to load them. Related issue: LibreSign#6590 Type: Test Checklist: - [x] Create ExecMock class - [x] Create FileSystemMock class - [x] Create SetupCheckFunctions with file_exists and exec overrides - [x] Update bootstrap.php to include new files Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds JavaSetupCheck to verify Java installation, path, version and encoding. Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Implement ISetupCheck interface - [x] Use SetupCheckUtils trait - [x] Verify Java path exists - [x] Check Java version matches required version - [x] Check native.encoding for UTF-8 - [x] Return SetupResult with appropriate severity and translated messages - [x] Add unit tests covering all scenarios Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds JSignPdfSetupCheck to verify JSignPdf binary, integrity, Java dependency and version. Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Implement ISetupCheck interface - [x] Use SetupCheckUtils trait - [x] Verify JSignPdf path is configured and exists - [x] Verify Java is available - [x] Check JSignPdf version against required version - [x] Return SetupResult with appropriate severity and translated messages - [x] Add unit tests covering all scenarios Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds PDFtkSetupCheck to verify PDFtk binary, integrity, Java dependency and version. Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Implement ISetupCheck interface - [x] Use SetupCheckUtils trait - [x] Verify PDFtk path is configured and exists - [x] Verify Java is available - [x] Check PDFtk version matches required version - [x] Return SetupResult with appropriate severity and translated messages - [x] Add unit tests covering all scenarios Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds PopplerSetupCheck to verify pdfsig and pdfinfo utilities (optional). Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Implement ISetupCheck interface - [x] Check pdfsig installation and version - [x] Check pdfinfo installation and version - [x] Return info severity when missing (optional check) - [x] Return success when both tools are working - [x] Add unit tests covering all scenarios Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds ImagickSetupCheck to verify imagick PHP extension (optional). Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Implement ISetupCheck interface - [x] Check if imagick extension is loaded - [x] Return info severity when not loaded (optional check) - [x] Return success when loaded - [x] Add unit tests covering both states Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Adds CertificateEngineSetupCheck to process certificate engine configuration results. Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Implement ISetupCheck interface - [x] Use CertificateEngineFactory to get current engine - [x] Process ConfigureCheckHelper results from engine - [x] Convert to SetupResult with aggregated messages and tips - [x] Handle engine not defined (error) - [x] Add unit tests covering success, warning, error, and mixed results Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Registers all six setup check classes in Application.php. Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Add registerSetupCheck for JavaSetupCheck - [x] Add registerSetupCheck for JSignPdfSetupCheck - [x] Add registerSetupCheck for PDFtkSetupCheck - [x] Add registerSetupCheck for PopplerSetupCheck - [x] Add registerSetupCheck for ImagickSetupCheck - [x] Add registerSetupCheck for CertificateEngineSetupCheck Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

@deprecated

Adds @deprecated annotation to ConfigureCheckService pointing to the new individual setup checks. Related issue: LibreSign#6590 Type: Maintenance Checklist: - [x] Add @deprecated tag with version 13.0.4 - [x] Mention replacement classes (JavaSetupCheck, JSignPdfSetupCheck, etc.) Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

vitormattos

Run the command at LibreSign folder inside container:
composer cs:fix
Some files have a wrong indent pattern.

vitormattos · 2026-03-04T19:34:53Z

lib/Service/Install/ConfigureCheckService.php

+/**
+* @deprecated 13.0.4 Use the individual SetupCheck classes instead
+*             (JavaSetupCheck, JSignPdfSetupCheck, etc.).
+*/


Why this, and why not solve?

The @deprecated annotation was originally added to maintain backward compatibility as suggested in the issue discussion. However, after further analysis and the reviewer's comment, I realized that fully removing the class is cleaner and more aligned with the goal of this refactor. The old ConfigureCheckService is now completely replaced by the new SetupCheckResultService and the individual ISetupCheck implementations. There are no remaining usages of the old service in the codebase, so keeping a deprecated stub would only add unnecessary maintenance burden. Therefore, I removed the class entirely. This should resolve the concern – the service is gone, not just deprecated.

vitormattos · 2026-03-04T19:45:27Z

tests/php/Unit/SetupCheck/JavaSetupCheckTest.php

+use OCA\Libresign\Service\Install\InstallService;
+use OCA\Libresign\SetupCheck\JavaSetupCheck;
+use OCA\Libresign\SetupCheck\FileSystemMock;
+use OCA\Libresign\SetupCheck\ExecMock;


I think that by this way, moving the mock to be an external file of test class, isn't good. Maybe would be best to move the mock files to a new namespace before the row 8 with the mocks.

I've implemented a solution where global functions exec and file_exists are redefined only within the OCA\Libresign\SetupCheck namespace during tests, and they delegate to dedicated mock classes (ExecMock and FileSystemMock) located in tests/php/Mock/. This keeps the test code clean and avoids polluting the global namespace, while still allowing us to simulate command execution and file existence in a controlled way.

Could you please evaluate whether this approach meets your expectations? If you see any issues or have a concrete suggestion for improvement (e.g., a different structure or better isolation), I'd be happy to adjust. Feel free to provide code examples if you have a preferred pattern.

guilhermercarvalho · 2026-03-05T16:46:53Z

Thanks for the feedback! I'm working on the suggestions and will update the PR soon.

This refactoring consolidates and organizes test classes related to SetupChecks, while also applying standardization improvements and best practices in the main codebase. - Move `ExecMock` and `FileSystemMock` classes to the new `Mock` namespace under `tests/php/Unit/SetupCheck/` and create a `functions.php` file to hold the overridden global functions (`file_exists`, `exec`). - Update `bootstrap.php` to load mocks from the new location and remove the obsolete `SetupCheckFunctions.php` file. - Apply code style fixes (indentation, spacing, trailing commas) to all files touched by the refactoring. Related issue: LibreSign#6590 Type: Maintenance Checklist: - [x] Move mocks to subdirectory and update bootstrap - [x] Apply code style fixes Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

This commit applies automated code style fixes using `composer cs:fix` and addresses issues identified by Psalm static analysis across the SetupCheck classes and their related tests. The changes include: - Normalizing indentation (tabs vs spaces) and adding missing trailing commas in argument lists across all SetupCheck classes and `SetupCheckUtils.php`. - Fixing the formatting of the `@deprecated` comment in `ConfigureCheckService.php`. - Adding `#[\Override]` attributes to all methods implementing `ISetupCheck`. Related issue: LibreSign#6590 Type: Maintenance Checklist: - [x] Run `composer cs:fix` to apply coding standards - [x] Address Psalm static analysis findings - [x] Add `#[\Override]` attributes to interface methods - [x] Fix deprecation comment formatting Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

- Add `@var list<string>` type annotation to `$output` variable in `JavaSetupCheck.php` to satisfy Psalm's type checking. - Improve empty output check in `PDFtkSetupCheck.php` by verifying `empty($versionOutput)` in addition to the return code, preventing false positives when the command returns an empty string but a zero exit code. These changes enhance code quality and the reliability of the setup checks. Related issue: LibreSign#6590 Type: Bug fix Checklist: - [x] Add type annotation for Psalm in JavaSetupCheck - [x] Fix empty output check in PDFtkSetupCheck Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

The previous fix for empty output in PDFtkSetupCheck changed the error message from "PDFtk binary is invalid" to "Failure to check PDFtk version". This commit updates the corresponding test assertion to align with the new behavior. Related issue: LibreSign#6590 Type: Test Checklist: - [x] Update assertion in PDFtkSetupCheckTest Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Move ExecMock and FileSystemMock from Unit/SetupCheck to the Mock/ directory to centralize test doubles and follow standard testing practices. Consolidate function overrides (file_exists, exec) from multiple test files into a single functions.php file to improve maintainability and reduce duplication. This refactoring addresses the reviewer's request by: - Placing mocks in the proper namespace (OCA\Libresign\Tests\Mock) before the function overrides, aligning with PHP namespace fallback mechanics. - Centralizing mock logic in dedicated, reusable classes. - Making test setup cleaner and more maintainable. Related issue: LibreSign#6590 Type: Test Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Refactor the libresign:configure:check command to consume the new setup check classes via ISetupCheckManager, instead of the deprecated ConfigureCheckService. The command now: - Filters only checks from the LibreSign namespace - Displays friendly resource names (e.g. "Java" instead of FQCN) - Respects the --sign (system category) and --certificate (security) filtering options Related issue: LibreSign#6590 Type: Feature Checklist: - [x] Replace ConfigureCheckService with ISetupCheckManager - [x] Add namespace filtering for LibreSign checks - [x] Improve resource name display - [x] Preserve option-based filtering by category Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

…ultService Centralize the logic for filtering and formatting LibreSign setup check results using the new SetupCheckResultService. This service internally uses the ISetupCheckManager and the individual SetupCheck classes, resolving the deprecation of the old ConfigureCheckService. Related issue: LibreSign#6590 Type: Refactor Checklist: - [x] Update `Check` command to use `SetupCheckResultService` - [x] Update `AdminController` to use `SetupCheckResultService` - [x] Remove direct dependency on `ConfigureCheckService` from controllers - [x] Create new `SetupCheckResultService` with methods for formatted results - [x] Update `@psalm-type LibresignConfigureCheck` to include 'info' status - [x] Ensure legacy API output (`getLegacyFormattedChecks`) remains unchanged Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

… data The API endpoint GET /api/v1/admin/certificate was returning the Organizational Unit (OU) field as an array when the certificate was already generated. This violated the OpenAPI schema which expects a string, causing test failures in AdminControllerTest. This change ensures that for the OU field, any array value is always converted to a comma-separated string. The CA ID is preserved when the certificate is generated, and removed only when it is not (matching the original behavior of the filter). Although discovered during refactoring of ConfigureCheckService, this issue was pre-existing and is fixed here following the "leave it better than you found it" principle. Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

The ConfigureCheckService has been fully replaced by the new ISetupCheck implementations (JavaSetupCheck, JSignPdfSetupCheck, etc.) and the SetupCheckResultService aggregator. This cleanup removes the old service and its test. Refs: LibreSign#6590 Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Add comprehensive test coverage for the new SetupCheckResultService, which formats and filters ISetupCheck results for LibreSign. The tests verify: - Filtering of checks by LibreSign namespace - Correct formatting of results with category - Legacy format without category - Severity mapping (warning → info, etc.) Related issue: LibreSign#6590 Type: Test Checklist: - [x] Add tests for getFormattedChecks filtering - [x] Add tests for getLegacyFormattedChecks format - [x] Add tests for severity mapping - [x] Use data providers for multiple scenarios - [x] Mock ISetupCheckManager and SetupResult Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

The toArray() method in AEngineHandler now returns the Organizational Unit as a string (or null) instead of an array, to match the API contract. This commit updates the test data provider to expect the correct new return types. Related issue: LibreSign#6590 Type: Tests Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

vitormattos

This is a partial review. I need to conduct a more thorough check soon.

vitormattos · 2026-03-07T23:13:17Z

lib/Service/SetupCheckResultService.php

Is pendign the SPDX header

vitormattos · 2026-03-07T23:13:56Z

lib/Service/SetupCheckResultService.php

+	/**
+	 * Get formatted checks without category, suitable for legacy API.
+	 *
+	 * @return list<array{
+	 *     status: 'error'|'info'|'success',
+	 *     resource: string,
+	 *     message: string,
+	 *     tip: string
+	 * }>
+	 */
+	public function getLegacyFormattedChecks(): array {
+		$checks = $this->getFormattedChecks();
+		return array_map(function ($check) {
+			unset($check['category']);
+			return $check;
+		}, $checks);
+	}


We don't need to have legacy compatibility.

vitormattos · 2026-03-07T23:23:34Z

lib/Service/SetupCheckResultService.php

+				$formatted[] = [
+					'status' => $this->mapSeverityToStatus($result->getSeverity()),
+					'resource' => $this->formatResourceName($checkName),
+					'message' => (string)$result->getDescription(),
+					'tip' => $result->getLinkToDoc() ?? '',
+					'category' => $category,
+				];


The previous structure, which uses a class to define the elements of a formatted result, sounds more testable and offers a better contract. It also removes the necessity to specify a type for the @return.

vitormattos · 2026-03-07T23:24:29Z

lib/Service/SetupCheckResultService.php

+	}
+
+	/**
+	 * Get formatted results of all LibreSign setup checks.


When we have a good method name, wasn't necessary a docblock to describe what is the method used for?

vitormattos · 2026-03-07T23:29:23Z

lib/SetupCheck/CertificateEngineSetupCheck.php

+
+	#[\Override]
+	public function getName(): string {
+		return $this->l10n->t('Certificate engine');


I'm not sure about making all the setup check sentences translatable. It might be too technical and could be kept in English instead. It would be good to check what Nextcloud is doing with the sentences that are used. What do you think?

github-project-automation bot moved this to 0. Needs triage in Roadmap Mar 4, 2026

github-project-automation bot added this to Roadmap Mar 4, 2026

guilhermercarvalho added 10 commits March 3, 2026 23:21

guilhermercarvalho force-pushed the feat/6590-setup-check-implementation branch from 3015ad8 to 2b31973 Compare March 4, 2026 02:22

guilhermercarvalho changed the title ~~Feat/6590 setup check implementation~~ feat: setup check implementation Mar 4, 2026

vitormattos requested changes Mar 4, 2026

View reviewed changes

github-project-automation bot moved this from 0. Needs triage to 1. to do in Roadmap Mar 4, 2026

vitormattos reviewed Mar 4, 2026

View reviewed changes

guilhermercarvalho added 12 commits March 5, 2026 18:08

chore(service): remove redundant comment in SetupCheckResultService

0ac2fed

Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

chore(service): remove redundant comments

502124e

Signed-off-by: Guilherme Carvalho <dev.gcarvalho@gmail.com>

vitormattos requested changes Mar 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: setup check implementation#7070

feat: setup check implementation#7070
guilhermercarvalho wants to merge 23 commits intoLibreSign:mainfrom
guilhermercarvalho:feat/6590-setup-check-implementation

guilhermercarvalho commented Mar 4, 2026 •

edited

Loading

Uh oh!

vitormattos left a comment

Uh oh!

vitormattos Mar 4, 2026

Uh oh!

guilhermercarvalho Mar 7, 2026

Uh oh!

vitormattos Mar 4, 2026

Uh oh!

guilhermercarvalho Mar 7, 2026

Uh oh!

guilhermercarvalho commented Mar 5, 2026

Uh oh!

vitormattos left a comment

Uh oh!

vitormattos Mar 7, 2026

Uh oh!

vitormattos Mar 7, 2026

Uh oh!

vitormattos Mar 7, 2026

Uh oh!

vitormattos Mar 7, 2026

Uh oh!

vitormattos Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

guilhermercarvalho commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue

Pull Request Type

Pull request checklist

Summary of Changes

Key Changes

How to Test

Before / After CLI Output

Additional Notes for Reviewers

Uh oh!

vitormattos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermercarvalho commented Mar 5, 2026

Uh oh!

vitormattos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

guilhermercarvalho commented Mar 4, 2026 •

edited

Loading