Automated Test: feature-recovery-keys-implementation #308
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
📝 WalkthroughWalkthroughThis pull request introduces support for recovery codes credential management in Keycloak. It adds helper methods to create and retrieve recovery codes credentials with fallback logic between user storage and credential providers, updates authentication flows to use these utilities, and extends test infrastructure to support recovery codes in backwards-compatible user storage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action as RecoveryAuthnCodesAction
participant Helper as CredentialHelper
participant UserStore as UserStorage
participant Provider as CredentialProvider
User->>Action: Generate Recovery Codes
Action->>Helper: createRecoveryCodesCredential(session, realm, user, model, codes)
Helper->>Helper: Serialize codes to JSON
Helper->>UserStore: Store credential in user storage
alt Storage Succeeds
UserStore-->>Helper: Credential stored
else Storage Fails
Helper->>Provider: Create via recovery-authn-codes provider
Provider-->>Helper: Credential created
end
Helper-->>Action: Credential persisted
Action-->>User: Codes ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (1)
80-89:⚠️ Potential issue | 🟠 MajorFederated credential removal may fail due to API mismatch.
getCredential()retrieves credentials from two sources:getFederatedCredentialsStream()(which usesUserStorageProvider.CredentialInputUpdater) and local storage (UserCredentialStore). When a recovery code credential is found in federated storage,removeStoredCredentialById()attempts removal viaUserCredentialStore.removeStoredCredential(). These are distinct APIs—there is no guarantee that a credential ID obtained from the federated provider API can be properly removed via the credential store API, potentially leaving stale all-used credentials in federated storage.Verify that the federated storage provider handles credential removal correctly, or consider whether an additional removal path is needed to address credentials found in federated storage.
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`:
- Around line 17-19: The code calls credentialModelOpt.get() without checking
presence, which can throw NoSuchElementException; in
RecoveryAuthnCodeInputLoginBean replace the direct get with a presence check or
use credentialModelOpt.orElseThrow(...) and throw a clear, specific runtime
exception (e.g. IllegalStateException) with a descriptive message like "Recovery
codes credential missing for user" before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel; reference
RecoveryAuthnCodesUtils.getCredential, the credentialModelOpt variable, and
RecoveryAuthnCodesCredentialModel.createFromCredentialModel when making the
change.
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`:
- Around line 326-341: The recovery-code validation in
BackwardsCompatibilityUserStorage accepts a matching code but doesn't consume
it; modify the block handling RecoveryAuthnCodesCredentialModel.TYPE so that
when generatedKeys.stream().anyMatch(...) finds a match you remove that specific
key from the generatedKeys list, serialize the updated list back into
storedRecoveryKeys.getCredentialData() (e.g., via
JsonSerialization.writeValueAsString) and persist the updated CredentialModel
(myUser.recoveryCodes) so the used code can't be reused; ensure you still return
true for successful validation and return false/fail on
serialization/persistence errors.
- Around line 230-252: getCredentials currently calls getMyUser(user) without
null-check and then dereferences myUser (accessing myUser.recoveryCodes and
myUser.otp), which can cause NPE; update the getCredentials method to guard
against a null myUser (similar to getDisableableCredentialTypesStream) by
returning an empty Stream when getMyUser(user) is null, or by early-returning
credentialModels.stream() before accessing fields, and keep existing
deserialization/error logging for recoveryCodes and adding otp only when myUser
!= null.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`:
- Line 135: The config alias contains a typo: change the string passed to
config.setAlias from "delayed-suthenticator-config" to
"delayed-authenticator-config" in BackwardsCompatibilityUserStorageTest so the
alias is spelled correctly; locate the call to config.setAlias(...) and update
the literal accordingly to maintain a clear, consistent alias name.
- Line 264: The logout call at testAppHelper.logout() is not asserted; change it
to assertTrue(testAppHelper.logout()) so the test fails if logout fails and
prevents stale session state; locate the call to testAppHelper.logout() in
BackwardsCompatibilityUserStorageTest (method containing that line) and wrap it
with assertTrue(...) to match other tests in this class.
🧹 Nitpick comments (4)
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (1)
118-122: WrapIOExceptionwith a descriptive message.The bare
RuntimeException(e)loses context about what failed. A short message aids debugging.Proposed fix
- } catch (IOException e) { - throw new RuntimeException(e); + } catch (IOException e) { + throw new RuntimeException("Failed to serialize recovery codes to JSON", e); }testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (3)
240-269: Test coverage is limited to the happy path.The test verifies setup + single successful login with recovery code index 0, but doesn't cover:
- Invalid/wrong recovery code entry
- Recovery code reuse (second login with already-used code)
This mirrors the existing OTP test structure for backwards compatibility, so it may be intentional. Consider whether additional coverage for error paths would be valuable here given that this is testing a new credential type integration with user storage.
123-143: Method visibility is package-private while all other helpers are private.Minor inconsistency:
configureBrowserFlowWithRecoveryAuthnCodeslacks an access modifier (package-private), whereas every other helper in this class (setupOTPForUserWithRequiredAction,assertUserDontHaveDBCredentials, etc.) isprivate. Since it's only called from within this class, it should beprivatefor consistency.♻️ Proposed fix
- void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { + private void configureBrowserFlowWithRecoveryAuthnCodes(long delay) {Note:
testingClientis also an inherited field, similar to the parameter discussion inenterRecoveryCodes.
476-483: Remove redundant parameters; use instance fields directly like other helper methods in this class.The
enterRecoveryCodesmethod acceptsenterRecoveryAuthnCodePageanddriveras parameters, but both are already instance fields (lines 104-105 and inherited). This contradicts the pattern used by similar methods likesetupOTPForUserWithRequiredAction(lines 382-408) andsetupRecoveryKeysForUserWithRequiredAction(lines 410-436), which reference page objects directly. The call site at line 259 passes the instance fields, making the parameters unnecessary.Additionally, the
setDriver(driver)call on line 478 is suspicious for an Arquillian@Page-injected object—the driver should already be set by the framework.♻️ Simplified version using instance fields
- private void enterRecoveryCodes(EnterRecoveryAuthnCodePage enterRecoveryAuthnCodePage, WebDriver driver, - int expectedCode, List<String> generatedRecoveryAuthnCodes) { - enterRecoveryAuthnCodePage.setDriver(driver); + private void enterRecoveryCodes(int expectedCode, List<String> generatedRecoveryAuthnCodes) { enterRecoveryAuthnCodePage.assertCurrent(); int requestedCode = enterRecoveryAuthnCodePage.getRecoveryAuthnCodeToEnterNumber(); org.junit.Assert.assertEquals("Incorrect code presented to login", expectedCode, requestedCode); enterRecoveryAuthnCodePage.enterRecoveryAuthnCode(generatedRecoveryAuthnCodes.get(requestedCode)); }Update the call site at line 259:
- enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys); + enterRecoveryCodes(0, recoveryKeys);
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | ||
|
|
||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); |
There was a problem hiding this comment.
Unchecked Optional.get() will throw NoSuchElementException if credential is missing.
credentialModelOpt.get() on Line 19 is called without a presence check. While this bean should only be constructed when the user has recovery codes configured, a race condition or storage inconsistency could leave the Optional empty, resulting in a 500 error during login.
Proposed fix — fail with a clear message
- Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
-
- RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
+ CredentialModel credentialModel = RecoveryAuthnCodesUtils.getCredential(user)
+ .orElseThrow(() -> new IllegalStateException("Recovery codes credential not found for user"));
+
+ RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | |
| CredentialModel credentialModel = RecoveryAuthnCodesUtils.getCredential(user) | |
| .orElseThrow(() -> new IllegalStateException("Recovery codes credential not found for user")); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`
around lines 17 - 19, The code calls credentialModelOpt.get() without checking
presence, which can throw NoSuchElementException; in
RecoveryAuthnCodeInputLoginBean replace the direct get with a presence check or
use credentialModelOpt.orElseThrow(...) and throw a clear, specific runtime
exception (e.g. IllegalStateException) with a descriptive message like "Recovery
codes credential missing for user" before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel; reference
RecoveryAuthnCodesUtils.getCredential, the credentialModelOpt variable, and
RecoveryAuthnCodesCredentialModel.createFromCredentialModel when making the
change.
| @Override | ||
| public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) { | ||
| var myUser = getMyUser(user); | ||
| RecoveryAuthnCodesCredentialModel model; | ||
| List<CredentialModel> credentialModels = new ArrayList<>(); | ||
| if (myUser.recoveryCodes != null) { | ||
| try { | ||
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | ||
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | ||
| myUser.recoveryCodes.getCreatedDate(), | ||
| myUser.recoveryCodes.getUserLabel() | ||
| ); | ||
| credentialModels.add(model); | ||
| } catch (IOException e) { | ||
| log.error("Could not deserialize credential of type: recovery-codes"); | ||
| } | ||
| } | ||
| if (myUser.otp != null) { | ||
| credentialModels.add(myUser.getOtp()); | ||
| } | ||
|
|
||
| return credentialModels.stream(); | ||
| } |
There was a problem hiding this comment.
Null-pointer risk: getMyUser(user) can return null.
getMyUser returns null when the username is not in the map. Accessing myUser.recoveryCodes on Line 235 or myUser.otp on Line 247 will throw a NullPointerException. Other methods in this class (e.g., getDisableableCredentialTypesStream at Line 258) guard against this.
Proposed fix
public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) {
var myUser = getMyUser(user);
+ if (myUser == null) return Stream.empty();
RecoveryAuthnCodesCredentialModel model;
List<CredentialModel> credentialModels = new ArrayList<>();🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`
around lines 230 - 252, getCredentials currently calls getMyUser(user) without
null-check and then dereferences myUser (accessing myUser.recoveryCodes and
myUser.otp), which can cause NPE; update the getCredentials method to guard
against a null myUser (similar to getDisableableCredentialTypesStream) by
returning an empty Stream when getMyUser(user) is null, or by early-returning
credentialModels.stream() before accessing fields, and keep existing
deserialization/error logging for recoveryCodes and adding otp only when myUser
!= null.
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | ||
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | ||
| if (storedRecoveryKeys == null) { | ||
| log.warnf("Not found credential for the user %s", user.getUsername()); | ||
| return false; | ||
| } | ||
| List generatedKeys; | ||
| try { | ||
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | ||
| } catch (IOException e) { | ||
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | ||
| return false; | ||
| } | ||
|
|
||
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); | ||
| } else { |
There was a problem hiding this comment.
Recovery code validation doesn't consume the used code.
The standard RecoveryAuthnCodesCredentialProvider marks a code as used after successful validation and advances the counter. This test storage accepts any stored code without consumption, meaning a single code can be reused indefinitely. If tests rely on code exhaustion behavior (e.g., triggering CONFIGURE_RECOVERY_AUTHN_CODES required action), they may pass incorrectly.
This may be acceptable for the current test scope — flagging for awareness.
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`
around lines 326 - 341, The recovery-code validation in
BackwardsCompatibilityUserStorage accepts a matching code but doesn't consume
it; modify the block handling RecoveryAuthnCodesCredentialModel.TYPE so that
when generatedKeys.stream().anyMatch(...) finds a match you remove that specific
key from the generatedKeys list, serialize the updated list back into
storedRecoveryKeys.getCredentialData() (e.g., via
JsonSerialization.writeValueAsString) and persist the updated CredentialModel
(myUser.recoveryCodes) so the used code can't be reused; ensure you still return
true for successful validation and return false/fail on
serialization/persistence errors.
| .addSubFlowExecution("Recovery-Authn-Codes subflow", AuthenticationFlow.BASIC_FLOW, AuthenticationExecutionModel.Requirement.ALTERNATIVE, altSubFlow -> altSubFlow | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, RecoveryAuthnCodesFormAuthenticatorFactory.PROVIDER_ID) | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { | ||
| config.setAlias("delayed-suthenticator-config"); |
There was a problem hiding this comment.
Typo in config alias: "delayed-suthenticator-config" → "delayed-authenticator-config".
This likely won't break functionality since it's just a config alias name, but it will be confusing if anyone needs to reference this config by alias later.
🔤 Proposed fix
- config.setAlias("delayed-suthenticator-config");
+ config.setAlias("delayed-authenticator-config");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config.setAlias("delayed-suthenticator-config"); | |
| config.setAlias("delayed-authenticator-config"); |
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`
at line 135, The config alias contains a typo: change the string passed to
config.setAlias from "delayed-suthenticator-config" to
"delayed-authenticator-config" in BackwardsCompatibilityUserStorageTest so the
alias is spelled correctly; locate the call to config.setAlias(...) and update
the literal accordingly to maintain a clear, consistent alias name.
|
|
||
| appPage.assertCurrent(); | ||
|
|
||
| testAppHelper.logout(); |
There was a problem hiding this comment.
Logout result is not asserted.
Other tests in this class wrap the logout call with assertTrue(testAppHelper.logout()) (e.g., lines 237, 405). Silently ignoring a failed logout could leave stale session state and cause flaky subsequent tests.
Proposed fix
- testAppHelper.logout();
+ assertTrue(testAppHelper.logout());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testAppHelper.logout(); | |
| assertTrue(testAppHelper.logout()); |
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`
at line 264, The logout call at testAppHelper.logout() is not asserted; change
it to assertTrue(testAppHelper.logout()) so the test fails if logout fails and
prevents stale session state; locate the call to testAppHelper.logout() in
BackwardsCompatibilityUserStorageTest (method containing that line) and wrap it
with assertTrue(...) to match other tests in this class.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Tests