[OID4VCI] Omit blank key attestation values from OID4VCI metadata - #51356

@mposolda thanks — moved to the base testsuite as you asked, and doing so surfaced a second bug that I think changes the shape of this fix. Flagging it up front because it widens the diff beyond what #51347 describes.

The integration test found a setter bug

All four key-attestation setters drop their argument:

// CredentialScopeModel:418, :437
// CredentialScopeRepresentation:291, :306
.map(list -> String.join(","))     // `list` is never passed

That is String.join(CharSequence, CharSequence...) with zero elements, so it returns "" and ignores list entirely. Configuring any resistance level writes an empty attribute.

This is where the reported [""] actually comes from. The testsuite's key-attestation-credential scope is set up with List.of(MODERATE) — so iso_18045_moderate was configured, silently discarded on write, and the getter then turned the blank attribute into [""]. Introduced in #51261.

So my original patch was treating the symptom. It made the metadata spec-compliant, but a user configuring iso_18045_high in the Admin Console would still have it silently thrown away — just invisibly rather than visibly. I'd rather not land that.

Why the unit test couldn't see it, and yours can

You were right to push this to the base testsuite, and for a stronger reason than the mocking shape. My unit test stubbed the attribute directly, so it exercised the getter in isolation and could never observe the setter.

The existing integration coverage couldn't catch it either, for a different reason — OID4VCIssuerWellKnownProviderTest:714 builds the expectation from the same getter under test:

expectedKeyAttestationsRequired.setKeyStorage(credScope.getRequiredKeyAttestationKeyStorage());

That assertion holds whether the getter returns [""] or null, so it passes before and after either fix. The new tests assert against literal expected values instead, which is what makes them able to fail.

What changed

• Fixed all four setters to String.join(",", list).
• Applied the blank-filtering getter fix to CredentialScopeRepresentation too — it had the same Arrays.asList(s.split(",")) shape as the model. Without this the two classes disagree on what a blank attribute means.
• Deleted CredentialScopeModelKeyAttestationTest and the java.lang.reflect.Proxy stub with it.
• Added two methods to OID4VCIssuerWellKnownProviderTest:
  • testKeyAttestationsRequiredAdvertisesConfiguredResistanceLevels — asserts the configured iso_18045_moderate actually reaches the metadata. This is the one that pins the setter bug.
  • testKeyAttestationsRequiredOmitsUnconfiguredResistanceLevels — walks the four cases from your issue plus separator-only and padded input, restoring the scope in a finally.

Verification

./mvnw -pl tests/base test -Dtest='OID4VCIssuerWellKnownProviderTest#testKeyAttestationsRequired*'
Tests run: 2, Failures: 0, Errors: 0

Keeping the tests and reverting only the production change:

[ERROR] testKeyAttestationsRequiredAdvertisesConfiguredResistanceLevels
Expected: iterable containing ["iso_18045_moderate"]
     but: was null
Tests run: 2, Failures: 1

I want to be precise about that "1", because it is not 2. Only the first test fails without the fix. The second passes on main too, so it is a spec-compliance guard rather than a regression test for the reported symptom — I would rather say that than imply more coverage than it has.

The reason is worth your attention. On the reverted build the getter returned null, not [""] — so writing "" through ClientScopeResource.update() does not leave a present-but-blank attribute; it comes back absent. I could not reproduce {"key_storage":[""]} through the admin REST path at all.

Which raises a question I could not answer from here: the [""] you saw must come from a write path that does persist a blank value where the admin API does not. If you know which one (Admin Console UI directly? realm import?), I will point the second test at it so it actually pins the getter half. Until then I have left the getter hardening in as defensive — it costs nothing and protects realms that already hold blank attributes — but I am not claiming a test proves it is needed.

DCO is signed off now as well.

One open question: if you would rather the getters stayed strict and blank data were handled as a migration instead, say so and I will drop that half and keep only the setter fix.

Good catch from the Copilot review — the description was stale. I rewrote the code and the tests but left the original Tests/Verification section in place, so it still named CredentialScopeModelKeyAttestationTest and a server-spi-private -am test command. That class is deleted in this PR, so the documented command verified nothing.

The description now describes what is actually here: the two OID4VCIssuerWellKnownProviderTest methods and

./mvnw -pl tests/base test -Dtest='OID4VCIssuerWellKnownProviderTest#testKeyAttestationsRequired*'

(with the note that tests/base needs a prior ./mvnw install -DskipTests to resolve). No code change — description only.

Rebased onto current main (8193dcb → 91ce00de). The diff is unchanged — 3 files, +140/-8.

The four red checks were a stale base, not the change. The branch head was Aug 3 and main was Aug 11, so it was ~100 commits behind; the tell was Keycloak JavaScript CI failing on a Java-only OID4VCI change, with annotations that were bare Process completed with exit code 1 and no source-level error. CodeQL was green on main throughout.

Workflow runs are currently sitting at action_required and need an approval before CI will report.