Repository navigation
Emit credential_id on UPDATE_CREDENTIAL events from built-in required actions - #50787
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Keycloak’s built-in credential required actions to include Details.CREDENTIAL_ID on UPDATE_CREDENTIAL events, aligning create/update-side auditing with existing REMOVE_CREDENTIAL behavior so event consumers can identify the exact credential row affected.
Changes:
- Emit
Details.CREDENTIAL_IDonUPDATE_CREDENTIALforUpdatePassword,UpdateTotp, andWebAuthnRegister. - Hydrate OTP credential IDs by setting the created ID back onto the passed
OTPCredentialModelduring persistence. - Extend tests to assert
credential_idis present (and for WebAuthn, equals the stored credential representation’s ID).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/WebAuthnIdlessTest.java | Asserts UPDATE_CREDENTIAL includes Details.CREDENTIAL_ID matching the stored WebAuthn credential row. |
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionTotpSetupTest.java | Asserts TOTP UPDATE_CREDENTIAL includes a non-null Details.CREDENTIAL_ID. |
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite 10BC8 /actions/AppInitiatedActionResetPasswordTest.java | Asserts password UPDATE_CREDENTIAL includes a non-null Details.CREDENTIAL_ID. |
| services/src/main/java/org/keycloak/credential/OTPCredentialProvider.java | Sets the generated credential ID back onto the input OTPCredentialModel after storing. |
| services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java | Captures created credential and emits its ID via Details.CREDENTIAL_ID. |
| services/src/main/java/org/keycloak/authentication/requiredactions/UpdateTotp.java | Emits Details.CREDENTIAL_ID from the OTP credential model when available. |
| services/src/main/java/org/keycloak/authentication/requiredactions/UpdatePassword.java | Looks up the password credential post-update and emits its ID via Details.CREDENTIAL_ID when available. |
7d01f41 to
d7ea7a3
Compare
c5b2153 to
41adbd0
Compare
41adbd0 to
3adb8b3
Compare
515f705 to
85eefc5
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new LDAP regression scenario must be moved from the deprecated Arquillian testsuite to the current test framework.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The credential IDs are propagated correctly, legacy events remain unchanged, and storage-specific behavior is covered by tests.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The implementation satisfies the event-correlation requirement and covers local and federated behavior with targeted tests.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@nunofaria11 Could you please rebase? In main, there's a commit that should possibly fix the CI failure for WebAuthn. Thanks! |
There was a problem hiding this comment.
🟢 Approval recommended
The implementation covers the relevant required actions and includes targeted regression tests for local and federated storage paths.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| // local row untouched. The secret data is compared too, as a local reset keeps the row ID and can | ||
| // land on the same createdDate millisecond as the password it replaces. | ||
| PasswordCredentialModel password = passwordProvider.getPassword(context.getRealm(), user); | ||
| if (password != null && (oldPassword == null |
There was a problem hiding this comment.
Might be easier to return the credentialModel from the provider (sim to what was done in WebAuth/TOTP) Change UserCredentialManager::updateCredential() to return an Optional<CredentialModel> populated when the local PasswordCredentialProvider handled the write, empty when a federated provider did. Removes excess db reads, hash comparison and potential race conditions between reads and update.
There was a problem hiding this comment.
Went with a similar approach - added a new updateCredentialAndGet method and kept the signature of updateCredential unchanged.
There was a problem hiding this comment.
🟡 Changes recommended
The upgrade guidance incorrectly generalizes credential-ID propagation beyond the password-update path.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
It changes credential-routing SPIs and binary contracts across several modules despite comprehensive targeted coverage.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| /* | ||
| * Copyright 2026 Red Hat, Inc. and/or its affiliates | ||
| * and other contributors as indicated by the @author tags. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
nitpick - No need to add licenses
| .detail(WebAuthnConstants.PUBKEY_CRED_AAGUID_ATTR, aaguid); | ||
| context.getEvent().clone().event(originalEventType).success(); | ||
| // the deprecated event is emitted first, so that it keeps carrying the same details as before | ||
| context.getEvent().detail(Details.CREDENTIAL_ID, createdCredential.getId()); |
There was a problem hiding this comment.
Is createdCredential always non-null?
There was a problem hiding this comment.
No... it can be null from org.keycloak.models.light.EmptyCredentialManager#createStoredCredential so adding a null check here.
| === `CredentialHelper.createRecoveryCodesCredential` returns the created credential | ||
|
|
||
| `org.keycloak.utils.CredentialHelper#createRecoveryCodesCredential` in the `server-spi-private` module now returns the created `CredentialModel` instead of `void`, so that callers can obtain the ID of the newly stored recovery codes credential. The method returns `null` when the credential was created by a user storage provider, because no {project_name} credential ID exists in that case. | ||
|
|
||
| The return type is part of the method descriptor, so a custom extension that calls this method must be recompiled against this release. An extension that is compiled against an earlier release fails with a `NoSuchMethodError` at runtime. | ||
|
|
||
| === `PasswordCredentialProvider` reports the credential it stores | ||
|
|
||
| `org.keycloak.credential.PasswordCredentialProvider` in the `services` module now implements the new `StoredCredentialInputUpdater` interface, which changed two of its method signatures: | ||
|
|
||
| * `createCredential(RealmModel, UserModel, String)` returns the stored `CredentialModel` instead of `boolean`. It returns `null` instead of `false` when no password hash provider is available for the realm password policy. | ||
| * `updateCredential(RealmModel, UserModel, CredentialInput)` is replaced by `updateCredentialAndGet(RealmModel, UserModel, CredentialInput)`, which returns the stored `CredentialModel` or `null`. The `updateCredential` method inherited from `StoredCredentialInputUpdater` delegates to it, so callers of `updateCredential` keep working. | ||
|
|
||
| A custom extension that calls `createCredential` with a password string must be recompiled against this release, because the return type is part of the method descriptor. An extension compiled against an earlier release fails with a `NoSuchMethodError` at runtime. | ||
|
|
||
| A custom extension that extends `PasswordCredentialProvider` and overrides `updateCredential` must override `updateCredentialAndGet` instead. {project_name} calls `updateCredentialAndGet` for every `StoredCredentialInputUpdater`, so an override of `updateCredential` is no longer invoked on the password update path. | ||
|
|
There was a problem hiding this comment.
Should not be needed as described in other comment.
| === Credential updaters can report the credential they store | ||
|
|
||
| The new `org.keycloak.credential.StoredCredentialInputUpdater` interface in the `server-spi-private` module extends `CredentialInputUpdater` for updaters that write to the {project_name} credential store. Its `updateCredentialAndGet` method returns the credential that was written, or `null` when the updater did not handle the input. `SubjectCredentialManager` gains an `updateCredentialAndGet` default method that performs the update and reports no stored credential; implementations that write to the {project_name} credential store should override it. | ||
|
|
||
| {project_name} uses this to set the `credential_id` detail on the `UPDATE_CREDENTIAL` event only when the credential was written to its own store. A password update handled by a user storage provider, for example LDAP in `WRITABLE` edit mode, therefore no longer risks reporting the ID of an unrelated credential row that is still present in the {project_name} database. | ||
|
|
||
| On the password-update path, a custom credential provider that writes to the {project_name} credential store should implement `StoredCredentialInputUpdater` instead of `CredentialInputUpdater` for the event to carry the ID of the credential it wrote. Implementing `CredentialInputUpdater` remains valid and only means that the password-update event has no `credential_id` detail. | ||
|
|
There was a problem hiding this comment.
We can mention in the release notes (not in the migration guide?), that the credential ID is part of the event attributes. But make it tiny, not so many text like this.
| * A {@link CredentialInputUpdater} writing the credentials it updates to the Keycloak credential store, and therefore | ||
| * able to report which credential it has written. | ||
| */ | ||
| public interface StoredCredentialInputUpdater extends CredentialInputUpdater { |
There was a problem hiding this comment.
I would not introduce this only when we have the use case of adding the credential ID to the event attributes. It's hard to determine what credential should be stored one, and what not (even like the no-import mode for LDAP as we store the OTP, WebAuthn,...).
I'd like to avoid this and also the instanceof checkup of it. We can just directly add the method to the credentialInputUpdater, and possibly handle the null values if it's not stored or we don't have any data about the credential.
There was a problem hiding this comment.
Went with a default method getStoredCredentialupdateCredentialAndGet in CredentialInputUpdater (default returns handled=true, credential=null).
Removed the instance-of checks.
There was a problem hiding this comment.
🟡 Changes recommended
The password ID lookup remains racy, and the breaking helper change and event contract are not documented accurately.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The post-write lookup is race-prone and the recovery helper introduces an avoidable binary incompatibility.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
model/storage/src/main/java/org/keycloak/credential/UserCredentialManager.java:106
- This re-reads the current credential after the update instead of returning the model written by that operation. A concurrent removal/recreation can therefore make
UPDATE_CREDENTIALomit the ID or report another row; have the local updater return the exactCredentialModelfrom the write path.
CredentialInputUpdater updater = update(input);
return updater == null
? Optional.empty()
: Optional.ofNullable(updater.getStoredCredential(realm, user, input.getType()));
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:119
- Changing only this method's return type changes its JVM descriptor, so extensions compiled against the existing
voidmethod will fail withNoSuchMethodError. Preserve the old method and add a differently named returning variant for the required action to call.
public static CredentialModel createRecoveryCodesCredential(KeycloakSession session, RealmModel realm, UserModel user, RecoveryAuthnCodesCredentialModel credentialModel, List<String> generatedCodes) {
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The claimed migration guidance for the extension-facing breaking changes is missing.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Balanced
| * credential store. The default implementation delegates to | ||
| * {@link #updateCredential(RealmModel, UserModel, CredentialInput)} and reports no credential. | ||
| */ | ||
| default CredentialUpdate updateCredentialAndGet(RealmModel realm, UserModel user, CredentialInput input) { |
There was a problem hiding this comment.
Can we use also the Optional<CredentialModel> and not introduce the CredentialUpdate? Do we need to distinguish anywhere whether it's empty due to some error, or comes from the external user storage?
There was a problem hiding this comment.
I think the CredentialUpdate record is useful here. We need to distinguish whether an update took the input and whether this update actually produced a credential.
The scenarios are
- LDAP in WRITABLE mode: writes the password to LDAP (no KC credential, so no id in the event); handled = true
- LDAP in UNSYNCED mode: rejects the update and the next PasswordCredentialProvider takes it; handled = false
If an Optional is used, both are Optional.empty(), and the UserCredentialManager cannot decide whether to move to the next provider for the update.
There was a problem hiding this comment.
🟡 Changes recommended
OTP credential creation now dereferences a null result for transient users.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Balanced
|
@nunofaria11 Sorry for the late response. Would be good to append this: getflip#1 |
… actions UpdatePassword, UpdateTotp and WebAuthnRegister now set Details.CREDENTIAL_ID on the generic UPDATE_CREDENTIAL event, matching REMOVE_CREDENTIAL which already carries the credential id. This lets event consumers identify which credential row was created or updated when a user has multiple credentials of the same type. The OTP credential id is surfaced by hydrating it in OTPCredentialProvider.createCredential; the WebAuthn id is read from the createCredential return value. A password update, however, may be handled entirely by a federated CredentialInputUpdater (e.g. LDAP in WRITABLE edit mode), in which case nothing is written to the Keycloak credential store and there is no id to report. SubjectCredentialManager therefore gains updateCredentialAndGet, which returns the credential written to the Keycloak credential store, or an empty Optional when a user-storage provider handled the write. UpdatePassword emits the detail only when a credential comes back. CredentialInputUpdater gains a default updateCredentialAndGet returning a CredentialUpdate: whether the updater handled the input, and the credential it wrote to the Keycloak credential store, if any. The default delegates to updateCredential and reports no credential, so an updater writing to its own store, or one without data about what it wrote, leaves the event without an id. PasswordCredentialProvider overrides it and hands over the model produced by the write itself: nothing is read back afterwards, so no concurrent password write can make the event report another row, and the required action needs neither a pre-update snapshot nor a hash or timestamp comparison to find out whether the local row changed. UserCredentialManager asks each updater once and keeps the first outcome that was handled, so updateCredential keeps reporting what it reported before. SubjectCredentialManagerCacheAdapter invalidates the cached user for updateCredentialAndGet as it does for every other mutating method, so the write path does not read credentials through the user cache. RecoveryAuthnCodesAction emits the id as well. CredentialHelper.createRecoveryCodesCredential now returns the credential it created in the local storage, or null when the userStorage handled the write, so the action applies the same emit-only-when-locally-stored rule as the other required actions. The tests compare the emitted value with the credential id actually persisted for the user, rather than only asserting that some id is present. RequiredActionUpdatePasswordFederatedTest covers the federated branch: a user whose password is held by a user-storage provider, with a stale password row left in the Keycloak DB, must not get that untouched row's id on the event. WebAuthnRegisterAndLoginTest asserts that the deprecated cloned event still carries the details it had before, as the id is added to the generic event only after that clone is emitted. The release notes mention the new event detail. The migration guide needs no entry, as the touched SPI interfaces only gain default methods. Closes keycloak#50247 Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nuno Faria <nuno.faria@getflip.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
There was a problem hiding this comment.
LGTM, thanks.
@keycloak/core-authn Do you want to review it as well?
|
@msdaly200 I see you have revieweed this before. Can you please take a look now? |
|
Reviewed the changes look good to me too. |
Main change
UpdatePassword,UpdateTotp,WebAuthnRegisterandRecoveryAuthnCodesActionnow setDetails.CREDENTIAL_IDon the genericUPDATE_CREDENTIALevent, matchingREMOVE_CREDENTIALwhich already carries the credential id.This lets event consumers identify which credential row was created or updated when a user has several credentials of the same type. TOTP surfaces the id by hydrating it in
OTPCredentialProvider.createCredential,WebAuthn reads it from thecreateCredentialreturn value (after the deprecatedoriginalEventTypeclone is emitted, so that event keeps its previous details, and only when a credential comes back - a transient user stores nothing), andCredentialHelper.createRecoveryCodesCredentialnow returns the credential it created locally, or null when a user storage provider handled the write.Password updates
A password update is different: it may be handled entirely by a federated
CredentialInputUpdater(LDAP in WRITABLE mode), in which case nothing is written to the Keycloak credential store and there is no id to report - while a stale local password row may still exist, whose id would be wrong to emit. Inspecting the store afterwards cannot answer that reliably either, since a lookup by credential type may return a row written concurrently. The write path therefore reports what it stored.CredentialInputUpdatergains a defaultupdateCredentialAndGetreturning aCredentialUpdate- whether the updater handled the input, plus the credential it wrote to the Keycloak credential store, if any.PasswordCredentialProvideroverrides it and hands over the model produced by the write itself, andUserCredentialManagerasks each updater once and keeps the first handled outcome, so updateCredential keeps reporting what it reported before.SubjectCredentialManagergains a matchingupdateCredentialAndGet, used byUpdatePasswordto emit the detail only when a credential comes back.Both new methods are defaults, so no external implementor is forced to change.
Tests
Tests compare the emitted value against the id actually persisted.
The new
RequiredActionUpdatePasswordFederatedTest(tests/base) covers the federated branch with a stale local row present;WebAuthnRegisterAndLoginTest(tests/webauthn) pins the id to the stored credential and asserts the deprecated clone does not carry it; existing Arquillian tests are only updated.Closes #50247
Aided by Claude Code (opus 4.8 / opus 5).