KNOX-3390: Address review feedback for TrustedOidcIssuerService

[smolnar82]

KNOX-3390 - Address review feedback for TrustedOidcIssuerService

What changes were proposed in this pull request?

Follow-up to #1315 addressing review comments on the TrustedOidcIssuerService implementation. Four items:

1. Surface registry-reload failures instead of swallowing them (JdbcTrustedOidcIssuerService). reloadRegistrySnapshot() previously caught, logged, and discarded any exception. Because it runs after a committed insert/delete, a reload failure would leave the in-memory snapshot silently diverged from the database while register/deregister still returned success. It now logs and re-throws as a RuntimeException, which register/deregister propagate (matching the interface's documented "throws RuntimeException on storage error") and which init() continues to wrap into ServiceLifecycleException so startup still fails cleanly.
2. Canonicalize issuer URLs on write and lookup (JdbcTrustedOidcIssuerService). Added trailing-slash normalization (a single trailing / stripped, null-safe, consistent with the stripping OIDCDiscoveryHelper already applies when building the discovery URL). register now persists the canonical form, and all lookup paths (isTrusted, isDynamicJwks, resolveJwksUri, refreshJwksUri, deregister) normalize their argument. This prevents a registration of https://idp.example.com/ from failing to match an iss claim of https://idp.example.com (and vice versa).
3. Remove dead code: dropped the unused count() method and COUNT_SQL constant from TrustedOidcIssuerDatabase (the max-issuers check uses the in-memory snapshot size, so count() had no callers).
4. Restore alphabetical ordering in pom.xml: moved the oauth2-oidc-sdk.version property after nodejs.version.

How was this patch tested?

Existing unit tests plus a new one, run via Maven:

mvn -pl gateway-server test -Dtest='JdbcTrustedOidcIssuerServiceTest,\
  TrustedOidcIssuerServiceFactoryTest,OIDCDiscoveryHelperTest,\
  EmptyTrustedOidcIssuerServiceTest,TrustedOidcIssuersSchemaTest'

Result: 46 tests, 0 failures, 0 errors (JdbcTrustedOidcIssuerServiceTest 17, OIDCDiscoveryHelperTest 12, TrustedOidcIssuerServiceFactoryTest 8, EmptyTrustedOidcIssuerServiceTest 7, TrustedOidcIssuersSchemaTest 2).

Added JdbcTrustedOidcIssuerServiceTest#testIssuerUrlTrailingSlashNormalized, which verifies: registering with a trailing slash and looking up without one (and vice versa) matches; the canonical (slash-stripped) form is what gets persisted; and deregistering via the non-canonical form still removes the entry.

Integration Tests

N/A

UI changes

N/A

[smolnar82]

Cc. @hsheinblatt

[github-actions]

Test Results

42 tests   42 ✅  7s ⏱️
 3 suites   0 💤
 3 files     0 ❌

Results for commit a4ba276.

[hsheinblatt]

Thanks Sandor. See comment below, but I argue to avoid the normalization of the issuer url. In case we want to skip that, I've created a new PR like this one but without that change, and some additional unit tests for the snapshot reload failure paths: #1320

[hsheinblatt]

I like the idea of ensuring that the user-entered issuer URI registered is correct and being somewhat permissive to prevent user-input errors that might cause confusion. However, I'm not sure it's possible.

Checked with the AI again, and this is a standard problem. What matters is the format of the issuer in the iss JWT claim. Our registered issuer must match it exactly, and that value comes from the IdP in use, not us. Every issuer is different. It does seem like omitting the trailing slash is the most common format used by the most-used IdPs, but in general, it's not a required format, and some IdP's have the trailing slash. In particular, k8s for service account projected tokens can include a trailing slash, depending on the config, and some external IdP's customers might use do include it.

So when you register the issuer, you must know what format your IdP uses and ensure that you register the correct URI. We can validate the input URL for the known requirements, like must be 'https://', cannot contain query string parts, and so on, but the trailing slash is not defined in the spec as required or not allowed: https://openid.net/specs/openid-connect-discovery-1_0.html

The problem in the original PR is different: we want to use the issuer as a base to construct the discovery URL. Adding '/.well-known/openid-configuration' is standard, but if applied blindly in all cases can lead to a double slash, which may fail. So to ensure the discovery lookup works for issuer urls that end in a slash as well as those that do not, we want to normalize the base url to have a single slash in the full discovery url. But we can't change the stored issuer URL, since it must match whatever the IdP uses as the issuer claim, and that may include a trailing slash.