Use platform OCSP implementation by default

[mrts]

WE2-1030

Signed-off-by: Mart Somermaa mrts@users.noreply.github.com

The best way to fix this problem is to remove the unused parameter from the method signature and update all method calls to stop passing the unnecessary argument. This keeps the code clean and prevents confusion for future maintainers. Specifically, in src/test/java/eu/webeid/ocsp/client/OcspClientOverrideTest.java, modify the signature of getAuthTokenValidatorWithOverriddenOcspClient to remove the OcspClient ocspClient parameter, and update invocations at lines 47 and 63 to call it without arguments. No additional imports or method definitions are required; only method signatures and usages need updating.

[svenzik]

This class name is OcspResponderUriMessage, but content is used like formatter.

First method argument is never null, Is this class even needed?

If it is, then

I suggest we rename the method formatResponderUri and class to OcspResponderUriMessageFormatter or similar.

I would suggest we improve API by having two methods, one for each usecase:

static String formatResponderUri(URI ocspResponderUri)
static String formatResponderUri(String message, URI ocspResponderUri)

Usage in code:
formatResponderUri(ocspResponderUri)
formatResponderUri("User certificate has been revoked", ocspResponderUri)

[svenzik]

Assigning a variable revocationInfoList and returning it later is a code smell.

This switch should be refactored into a separate method.

For example:

            List<RevocationInfo> revocationInfoList = getRevocationInfos(revocationMode, certificateRevocationChecker, customPkixRevocationChecker, pkixBuilderParameters, certPathBuilder);

This allows the code flow at the end of the method to be more readable:

            if (revocationMode == RevocationMode.CUSTOM_CHECKER) {
                if (!certificate.getIssuerX500Principal().equals(trustedCACert.getSubjectX500Principal())) {
                    throw new IllegalStateException(
                            "Trust anchor is not the issuer of the subject certificate, check your configured certificate authorities. " +
                                    "Subject issuer=" + certificate.getIssuerX500Principal() +
                                    ", trust anchor subject=" + trustedCACert.getSubjectX500Principal()
                    );
                }
                return certificateRevocationChecker.validateCertificateNotRevoked(certificate, trustedCACert);
            } else {
                return getRevocationInfos(revocationMode, certificateRevocationChecker, customPkixRevocationChecker, pkixBuilderParameters, certPathBuilder);
            }

This will make code readable, the method responsiblity is now to choose between custom or default implementation and default implementation is properly isolated into method.

NB! Spring usually introduces a delegator pattern, which only purpose is to choose which implementation to use. Currently this class chooses the implementation and is the default implementation.

[mrts]

I agree that the current revocationInfoList assignment isn’t perfect.

That said, I’m slightly concerned about the proposed structure making CUSTOM_CHECKER look like the “main” path and everything else somehow "secondary" in the else branch. Conceptually, CUSTOM_CHECKER is intended to be the more of a secondary, special case and the primary logic is the switch itself.

So I’d prefer to leave it as-is for now, unless you feel strongly otherwise.

(Also, I’m not sure the Spring delegator pattern applies cleanly here, since this is a utility performing PKIX validation rather than a DI-selected strategy, though I agree the modes resemble strategies in a sense.)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
14 Accepted issues

Measures
0 Security Hotspots
84.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud