SSF: Revise SSF event structure (#50792) by thomasdarimont · Pull Request #50793 · keycloak/keycloak

thomasdarimont · 2026-07-10T14:12:21Z

Move the CAEP-specific claims (subject, event_timestamp, initiating_entity, reason_admin, reason_user) from SsfEvent down to CaepEvent so the base event only carries the generic SET fields: event type, alias and the extension attributes map. Introduce shared EVENT_TYPE_BASE_URI constants for the CAEP and SSF stream event type URIs and an (eventType, alias) constructor on SsfEvent.

Rework toString across the event hierarchy: SsfEvent renders the fields subclasses contribute to a LinkedHashMap via the new appendFields hook, filtering null values, quoting String values uniformly and appending the extension attributes map only when non-empty. This replaces the per-class string concatenation, which rendered unset fields as null placeholders.

SsfEventsTest constructs every registry-contributed event type plus the GenericSsfEvent fallback and asserts toString neither throws nor renders unset fields.

Fixes #50792

Copilot

Pull request overview

Refactors SSF events so generic and CAEP-specific fields live in the appropriate hierarchy and standardizes diagnostic rendering.

Changes:

Moves CAEP claims from SsfEvent to CaepEvent.
Introduces shared event-type URI constants and constructors.
Centralizes null-safe toString() rendering with regression tests.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`SsfEvent.java`	Simplifies base fields and centralizes rendering.
`GenericSsfEvent.java`	Adds event-type construction and shared rendering.
`CaepEvent.java`	Owns common CAEP claims and rendering.
`CaepCredentialChange.java`	Uses shared URI and field-rendering hook.
`CaepSessionRevoked.java`	Uses shared CAEP URI constant.
`SsfStreamEvent.java`	Defines the SSF event-type base URI.
`SsfStreamUpdatedEvent.java`	Uses shared URI and renders event fields.
`SsfStreamVerificationEvent.java`	Uses shared URI and renders state safely.
`SsfEventsTest.java`	Tests built-in and generic event rendering.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

ssilvert · 2026-08-04T19:35:00Z

@thomasdarimont Claude suggests splitting this into two PR's and I tend to agree if it's not too much rework. Basically, the toString() stuff is a separate (and simpler) change that can be split out and merged quickly. The other part may need more rework.

I'll have Claude post a full review then we can discuss where to go from here and if this PR should be merged before mine. I don't mind waiting and I'm thinking you should probably go first if we can get everything resolved quickly.

ssilvert

Nice cleanup. The toString() rework in particular fixes a real gap: SsfEvent has no toString() on main today, so e.g. SsfStreamUpdatedEvent currently logs as org.keycloak...@1a2b3c4d. The appendFields template hook is the right shape for that.

Two things I'd like to discuss before this lands, both on the field-relocation half rather than the toString() half. My main suggestion is to split this into two PRs — the toString() rework looks ready to merge with only cosmetic nits, while moving the event claims between classes deserves its own discussion.

1. `subject` and `event_timestamp` may be one level too deep

initiating_entity, reason_admin and reason_user are genuinely CAEP §3.1 event-specific claims, so pushing those down to CaepEvent makes sense to me. But subject is an SSF/SET-level concept that non-CAEP event families carry too, and RISC 1.0's Credential Compromise event explicitly defines event_timestamp, reason_admin and reason_user.

Concretely this collides with the RISC work in #48900: RiscEvent extends SsfEvent and RiscAccountDisabled / RiscAccountEnabled use all five claims, plus there's an applyInitiatingEntity(Event, AdminEvent, SsfEvent) helper in SecurityEventTokenMapper that takes the base type. After this change those would have to either redeclare every field verbatim on RiscEvent or extend CaepEvent, which would be semantically wrong.

Would you consider keeping subject and event_timestamp on SsfEvent and pushing down only the three CAEP reason/initiator claims? Or introducing an intermediate class, if you'd rather keep SsfEvent strictly minimal.

2. `SsfEvent` is documented extension surface

The javadoc on SsfEventProviderFactory explicitly invites third parties to contribute custom events via META-INF/services. Removing five getter/setter pairs from SsfEvent is a breaking change for any non-CAEP custom event that already exists out there. SSF is still a preview feature so this is probably acceptable, but it seems worth calling out in the PR description / release notes.

Test coverage

The new tests are well written — lazy suppliers for the failure messages, good assertion text. But all four target toString(), which is the low-risk half of the change. The behaviourally risky part is relocating @JsonProperty fields between classes, and nothing here covers the wire format. A JSON round-trip test asserting that CaepCredentialChange still serialises subject / event_timestamp / reason_admin, and that SsfStreamVerificationEvent emits only state, would guard exactly what this PR moves.

(For what it's worth, dropping those fields from the stream events looks spec-correct to me — SSF §8.1.4 verification carries only state, and stream-updated carries status / reason.)

Smaller notes

The TYPE constant derivation is safe: EVENT_TYPE_BASE_URI + "credential-change" is still a compile-time constant expression, so it stays inlined and there's no class-init ordering hazard. 👍
The re-indent of the IllegalArgumentException continuation line and the DECLARED_JSON_PROPERTIES field move in SsfEvent.java are unrelated to either stated goal and add a bit of diff noise.
Rest of the notes inline below.

ssilvert · 2026-08-04T19:37:34Z

+     */
+    public static final String EVENT_TYPE_BASE_URI = "https://schemas.openid.net/secevent/caep/event-type/";
+
+    @JsonProperty("subject")


subject reads as generic rather than CAEP-specific to me — it's the SET/SSF subject, and RISC events carry it too. Same for event_timestamp, which RISC 1.0 defines on Credential Compromise alongside reason_admin / reason_user.

If all five stay here, RiscEvent (#48900) has to redeclare them verbatim. Keeping subject + event_timestamp on SsfEvent and pushing down only initiating_entity / reason_admin / reason_user would avoid that duplication while still getting you the lean base class.

The subject field within the event is the legacy CAEP SSE subject attribute for backwards compatibility. In the old OpenID Shared Signals and Events Framework Specification 1.0 - draft 01 the subject attribute was used to identify the subject of the event.
Is currently necessary for compatibility with Apple Business.

In SSF 1.0 the subject is encoded as the "sub_id" claim in the Security Event Token (SET).

Therefore I'd recommend to leave the subject field as is in the CaepEvent class and add a suitable comment.
Regarding the event_timestamp I'm fine with moving this up since it is also used by some RISC events, e.g.: Credential Compromise, however I'm not sure if SsfEvent (or another intermediate class) is better here... some SSF Events, e.g. SCIM events don't use the event_timestamp attribute: https://www.rfc-editor.org/rfc/rfc9967.html#name-scim-provisioning-events

Following up on event_timestamp: after checking the specs again more closely I'd now keep it on CaepEvent too, and leave the hierarchy as it is in this PR:

CAEP 1.0 defines event_timestamp, reason_admin and reason_user as profile-common optional claims (Section 2), which is exactly what CaepEvent models.

RISC 1.0 final defines them on only one of its fourteen event types: Credential Compromise, so they're per-event there, not profile-common.
The subject is identified via the SET-level sub_id claim in RISC final, with no in-event subject at all. Your RiscEvent in Emit RISC account-disabled/account-enabled SSF events #51318 reflects this already: it carries
no fields, and the mapper sets the subject on the token, so nothing actually needs hoisting to SsfEvent to support it.

Other SET profiles that may flow through SSF streams, e.g. SCIM Events (RFC 9967), define none of these claims, which is another reason to keep the base lean.

I also considered a shared intermediate class (something like SsfCommonEvent with event_timestamp/reason_admin/reason_user, extended by CaepEvent and RiscEvent)
and decided against it: for CAEP it adds nothing over CaepEvent, and because of single inheritance RiscEvent would have to extend it, giving the thirteen RISC event
types that don't define these claims typed fields for them anyway.
When Credential Compromise gets implemented it can declare its three optional claims itself; if a genuinely shared set ever emerges, extracting an intermediate
base at that point is a non-breaking pull-up refactor, whereas placing fields too high now is hard to undo once extensions subclass these types.

So the resulting rule for me is: SsfEvent carries only SET member semantics (type, alias, extension attributes), profile-common claims live on the profile base
class, and per-event claims live on the event class.

ssilvert · 2026-08-04T19:37:34Z

+        this(null);
+    }
+
+    public GenericSsfEvent(String eventType) {


Two things here:

This constructor doesn't appear to be called anywhere except from the no-arg one just above — was it intended for something specific, or is it speculative API?

More substantively: because GenericSsfEvent extends SsfEvent directly, unknown event types now lose typed subject parsing. Previously subject went through SubjectIdJsonDeserializer into a typed SubjectId; now it lands in attributes as a raw LinkedHashMap and getSubjectId() is gone entirely. Nothing on main reads it today (SubjectSubscriptionFilter uses the token's subject), but it is a capability regression for extension/unknown events.

This is ctor is intended to be used by custom implementations that don't want to implement their own event classes.

As I mentioned in #50793 (comment) the subject attribute of an event is not used by modern events and exists only for backwards compatbility with implementations that used the older SSE CAEP structures (such as Apple).

ssilvert · 2026-08-04T19:37:34Z

    }
+
+    @Override
+    public String toString() {


A subclass that overrides toString() instead of appendFields() would silently drop every inherited field, with no compile-time signal. The javadoc on appendFields says the right thing, but making this final would actually enforce it — and since every subclass in the tree now uses the hook, nothing would break.

I'd keep toString() overridable since custom/extension events may legitimately want to fully control their rendering rather than go through appendFields.
The javadoc on appendFields documents the contract for subclasses that only contribute fields; I'd rather keep the escape hatch than enforce it at compile time.

ssilvert · 2026-08-04T19:37:34Z

+                    () -> "toString() must not throw for fresh " + entry.getKey());
+            assertNotNull(rendered, () -> "toString() must not return null for " + entry.getKey());
+            assertFalse(rendered.isBlank(), () -> "toString() must not be blank for " + entry.getKey());
+            assertFalse(rendered.contains("null"),


contains("null") is quite broad — it will fire on any future field name or attribute value that merely contains the substring (nullable, annulled, a URI with null in a path segment). contains("=null") targets the actual failure mode you care about here.

contains("=null") make sense - I'll adjust the tests.

Move the CAEP-specific claims (subject, event_timestamp, initiating_entity, reason_admin, reason_user) from SsfEvent down to CaepEvent so the base event only carries the generic SET fields: event type, alias and the extension attributes map. Introduce shared EVENT_TYPE_BASE_URI constants for the CAEP and SSF stream event type URIs and an (eventType, alias) constructor on SsfEvent. Rework toString across the event hierarchy: SsfEvent renders the fields subclasses contribute to a LinkedHashMap via the new appendFields hook, filtering null values, quoting String values uniformly and appending the extension attributes map only when non-empty. This replaces the per-class string concatenation, which rendered unset fields as null placeholders. SsfEventsTest constructs every registry-contributed event type plus the GenericSsfEvent fallback and asserts toString neither throws nor renders unset fields. Fixes keycloak#50792 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

…vents Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

- Render toString in the conventional Name{...} shape and quote String values recursively at every nesting level, including extension attributes and reason_admin/reason_user entries - Tighten toString test assertions to "=null" and genuinely cover the unknown-type registry fallback via a stubbed resolveRegistry(), renaming the no-session degradation test to what it actually asserts Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ssf/core/src/main/java/org/keycloak/ssf/event/SsfEvent.java:180

The recursive renderer has no cycle detection, although setAttributeValue/setAttributes accept arbitrary programmatic values. A self-referential map or iterable now causes toString() to overflow the stack (where JDK collection toString() handles direct self-reference); track values by identity while descending and emit a cycle marker when revisiting one.

        if (value instanceof Map<?, ?> map) {
            StringJoiner joiner = new StringJoiner(", ", "{", "}");
            for (var entry : map.entrySet()) {
                joiner.add(entry.getKey() + "=" + render(entry.getValue()));
            }

keycloak-github-bot · 2026-08-06T09:33:10Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

Uh oh!

Conversation

thomasdarimont commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ssilvert commented Aug 4, 2026

Uh oh!

ssilvert left a comment

Choose a reason for hiding this comment

1. subject and event_timestamp may be one level too deep

2. SsfEvent is documented extension surface

Test coverage

Smaller notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasdarimont Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

keycloak-github-bot Bot commented Aug 6, 2026

Unreported flaky test detected

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employee2Test

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestCertSubjectAsKeyNameInKeyInfo

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testUserAttributeStatementMapperUserGroupsAggregate

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testUserAttributeStatementMapperGroupsNoAggregate

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#singleLoginAndLogoutSAMLTest

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostTestCompositeRoleForUser

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEmptyConsumerPostURL

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testUserAttributeStatementMapperGroupsAggregate

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testReloginWithInvalidAuthSessionCookie

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostTest

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostSigTransientTest

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testUserAttributeStatementMapperUserGroupsNoAggregate

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostSigTest

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#disabledClientTest

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testMultipleTabsParallelLogin

Uh oh!

keycloak-github-bot Bot left a comment

thomasdarimont commented Jul 10, 2026 •

edited

Loading

1. `subject` and `event_timestamp` may be one level too deep

2. `SsfEvent` is documented extension surface

thomasdarimont Aug 6, 2026 •

edited

Loading