Implement recovery key support for user storage providers#9
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces support for Recovery Codes credentials in Keycloak. It adds centralized helper methods for creating and retrieving recovery code credentials, refactors existing authentication and action components to use these helpers, extends the backwards-compatibility storage layer to support the new credential type, and provides integration test coverage for the complete flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java:
- Around line 17-21: The code calls RecoveryAuthnCodesUtils.getCredential(user)
and immediately dereferences the Optional with credentialModelOpt.get(), which
can throw NoSuchElementException; update RecoveryAuthnCodeInputLoginBean to
first check credentialModelOpt.isPresent() (or use
credentialModelOpt.orElseThrow(...) with a clear IllegalStateException) before
passing to RecoveryAuthnCodesCredentialModel.createFromCredentialModel, and
handle the absent case by either setting codeNumber to a safe default or
throwing a descriptive exception so the class does not crash at runtime.
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java:
- Around line 123-143: Typo in the config alias: in
configureBrowserFlowWithRecoveryAuthnCodes update the mistaken string
"delayed-suthenticator-config" to "delayed-authenticator-config" where
config.setAlias(...) is called so the delayed authenticator configuration alias
matches the authenticator name.
🧹 Nitpick comments (4)
server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (1)
56-62: Consider adding null-check for theuserparameter.If
useris null, callinguser.credentialManager()will throw aNullPointerException. Consider adding a null check for consistency with other methods in this class (e.g.,hashRawCodeusesObjects.requireNonNull).♻️ Suggested improvement
public static Optional<CredentialModel> getCredential(UserModel user) { + Objects.requireNonNull(user, "user cannot be null"); return user.credentialManager() .getFederatedCredentialsStream() .filter(c -> RecoveryAuthnCodesCredentialModel.TYPE.equals(c.getType())) .findFirst() .or(() -> user.credentialManager().getStoredCredentialsByTypeStream(RecoveryAuthnCodesCredentialModel.TYPE).findFirst()); }testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (1)
230-252: Use parameterized type for JSON deserialization.Line 238 uses raw
List.classwhich loses type information and causes unchecked conversion warnings. While this is test code, using aTypeReferencewould be more type-safe.♻️ Suggested improvement
- model = RecoveryAuthnCodesCredentialModel.createFromValues( - JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), + @SuppressWarnings("unchecked") + List<String> codes = JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class); + model = RecoveryAuthnCodesCredentialModel.createFromValues( + codes, myUser.recoveryCodes.getCreatedDate(), myUser.recoveryCodes.getUserLabel() );Or alternatively, use Jackson's
TypeReference:List<String> codes = JsonSerialization.readValue( myUser.recoveryCodes.getCredentialData(), new TypeReference<List<String>>() {} );testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (2)
240-269: Consider using a distinct username for this test.The test uses
"otp1"as the username (line 245, 258), which is also used in OTP-related tests. Consider using a more descriptive username like"recovery1"to improve test readability and avoid potential confusion when debugging test failures.The test structure with
try-finallyfor flow cleanup is good practice.
410-436: Hardcoded username reduces reusability.Line 420 uses hardcoded
"otp1"instead of deriving the username from theuserIdparameter. This limits the method's reusability for different users.♻️ Suggested improvement
Consider fetching the username from the user resource or passing it as a parameter:
- private List<String> setupRecoveryKeysForUserWithRequiredAction(String userId, boolean logoutOtherSessions) throws URISyntaxException, IOException { + private List<String> setupRecoveryKeysForUserWithRequiredAction(String userId, String username, boolean logoutOtherSessions) throws URISyntaxException, IOException { // Add required action to the user to reset RecoveryKeys UserResource user = testRealm().users().get(userId); UserRepresentation userRep = user.toRepresentation(); userRep.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name())); user.update(userRep); TestAppHelper testAppHelper = new TestAppHelper(oauth, loginPage, appPage); // Login as the user and setup RecoveryKeys - testAppHelper.startLogin("otp1", "pass"); + testAppHelper.startLogin(username, "pass");Note: The same issue exists in
setupOTPForUserWithRequiredAction(line 392).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.javaserver-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.javaservices/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.javaservices/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.javaservices/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.javatestsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.javatestsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java
🧰 Additional context used
🧬 Code graph analysis (6)
server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (1)
server-spi/src/main/java/org/keycloak/credential/CredentialModel.java (1)
CredentialModel(36-355)
services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java (1)
server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (1)
RecoveryAuthnCodesUtils(20-63)
services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (1)
server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (1)
RecoveryAuthnCodesUtils(20-63)
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (2)
server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java (1)
KeycloakModelUtils(103-1230)core/src/main/java/org/keycloak/util/JsonSerialization.java (1)
JsonSerialization(40-126)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (4)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java (1)
BrowserFlowTest(68-1298)testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/EnterRecoveryAuthnCodePage.java (1)
EnterRecoveryAuthnCodePage(15-64)testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/SetupRecoveryAuthnCodesPage.java (1)
SetupRecoveryAuthnCodesPage(14-78)common/src/main/java/org/keycloak/common/Profile.java (1)
Profile(45-483)
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (1)
core/src/main/java/org/keycloak/util/JsonSerialization.java (1)
JsonSerialization(40-126)
🔇 Additional comments (14)
services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java (1)
116-116: Clean refactor to centralized credential creation.The change correctly delegates recovery codes credential creation to
CredentialHelper.createRecoveryCodesCredential, improving code maintainability and consistency across the codebase.testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.java (1)
53-57: LGTM!The new
hasRecoveryCodesmethod follows the same pattern ashasUserOTP, maintaining consistency in the test utility. The null checks are correctly implemented.services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (2)
80-80: Good use of centralized utility.The refactor to use
RecoveryAuthnCodesUtils.getCredential()correctly handles the Optional with properisPresent()check before accessing the value, unlike the similar change inRecoveryAuthnCodeInputLoginBean.
80-89: Use separate removal logic for federated vs. stored recovery code credentials.
RecoveryAuthnCodesUtils.getCredential()checks federated credentials first, then stored credentials. However, callingremoveStoredCredentialById()on lines 86-87 only removes stored credentials. If the credential is federated, it will not be removed correctly. Check if federated recovery code credentials are possible and handle removal usingremoveFederatedIdentity()if needed.server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (2)
76-85: LGTM!The refactored method maintains the same logic with improved readability through explicit null checks.
112-131: Return type mismatch: consider whether callers need feedback on credential creation success.The
createRecoveryCodesCredentialmethod returnsvoid, while the similarcreateOTPCredentialmethod returnsbooleanindicating whether credential creation/validation succeeded. If callers need to know whether recovery codes were successfully created, this method should return a status value. If recovery codes don't require validation like OTP does, the current void return is acceptable—clarify the design intent with a comment.Likely an incorrect or invalid review comment.
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (5)
111-120: LGTM!The
supportsCredentialTypemethod correctly extends support to includeRecoveryAuthnCodesCredentialModel.TYPE.
193-201: LGTM!The recovery codes credential creation follows the established pattern used for OTP credentials, correctly setting the ID, type, credential data, and creation date.
275-276: LGTM!The
isConfiguredFormethod correctly extends to check for recovery codes credentials using the same pattern as OTP credentials.
326-341: LGTM with minor note.The validation logic correctly checks if the provided challenge response matches any of the stored recovery keys. The use of raw
Listtype (line 332) is consistent with the rest of the file, though parameterized typing would be preferable.
427-439: LGTM!The
recoveryCodesfield and its getter follow the established pattern used for other credentials in theMyUserclass.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (3)
82-86: LGTM!The
@EnableFeatureannotation correctly enables theRECOVERY_CODESfeature for this test class withskipRestart = trueto avoid unnecessary server restarts.
456-463: LGTM!The assertion method follows the same pattern as
assertUserHasOTPCredentialInUserStorage. Note that the hardcoded"otp1"username (line 460) is consistent with other methods but limits flexibility.
476-483: Remove this suggestion—thesetDrivercall is necessary and follows the established test pattern.The
setDriver(driver)call on line 478 is not redundant. WhileenterRecoveryAuthnCodePageis injected via@Page, the page object receives aWebDriverparameter that may differ from the injected instance. This pattern is consistently used throughout the test suite (seeRecoveryAuthnCodesAuthenticatorTest.javalines 275, 283, 293, 301 for identical usage), ensuring the page object operates with the correct driver instance when passed as a parameter. The call is intentional and necessary.
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | ||
|
|
||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | ||
|
|
||
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); |
There was a problem hiding this comment.
Missing Optional.isPresent() check will cause NoSuchElementException.
RecoveryAuthnCodesUtils.getCredential(user) returns an Optional that may be empty if no recovery codes credential exists. Calling .get() on line 19 without checking isPresent() will throw NoSuchElementException at runtime.
🐛 Proposed fix
public RecoveryAuthnCodeInputLoginBean(KeycloakSession session, RealmModel realm, UserModel user) {
Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
- RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
-
- this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber();
+ if (credentialModelOpt.isEmpty()) {
+ throw new IllegalStateException("Recovery authentication codes credential not found for user");
+ }
+ RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
+ this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode()
+ .orElseThrow(() -> new IllegalStateException("No recovery authentication code available"))
+ .getNumber();
}📝 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()); | |
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); | |
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| if (credentialModelOpt.isEmpty()) { | |
| throw new IllegalStateException("Recovery authentication codes credential not found for user"); | |
| } | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | |
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode() | |
| .orElseThrow(() -> new IllegalStateException("No recovery authentication code available")) | |
| .getNumber(); |
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java
around lines 17 - 21, The code calls RecoveryAuthnCodesUtils.getCredential(user)
and immediately dereferences the Optional with credentialModelOpt.get(), which
can throw NoSuchElementException; update RecoveryAuthnCodeInputLoginBean to
first check credentialModelOpt.isPresent() (or use
credentialModelOpt.orElseThrow(...) with a clear IllegalStateException) before
passing to RecoveryAuthnCodesCredentialModel.createFromCredentialModel, and
handle the absent case by either setting codeNumber to a safe default or
throwing a descriptive exception so the class does not crash at runtime.
| void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { | ||
| final String newFlowAlias = BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES; | ||
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); | ||
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) | ||
| .selectFlow(newFlowAlias) | ||
| .inForms(forms -> forms | ||
| .clear() | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, UsernamePasswordFormFactory.PROVIDER_ID) | ||
| .addSubFlowExecution(AuthenticationExecutionModel.Requirement.REQUIRED, reqSubFlow -> reqSubFlow | ||
| .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"); | ||
| config.setConfig(Map.of("delay", Long.toString(delay))); | ||
| }) | ||
| ) | ||
| ) | ||
| ) | ||
| .defineAsBrowserFlow() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix typo in config alias.
Line 135 has a typo: "delayed-suthenticator-config" should be "delayed-authenticator-config".
🔤 Suggested fix
.addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> {
- config.setAlias("delayed-suthenticator-config");
+ config.setAlias("delayed-authenticator-config");
config.setConfig(Map.of("delay", Long.toString(delay)));
})📝 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.
| void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { | |
| final String newFlowAlias = BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES; | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) | |
| .selectFlow(newFlowAlias) | |
| .inForms(forms -> forms | |
| .clear() | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, UsernamePasswordFormFactory.PROVIDER_ID) | |
| .addSubFlowExecution(AuthenticationExecutionModel.Requirement.REQUIRED, reqSubFlow -> reqSubFlow | |
| .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"); | |
| config.setConfig(Map.of("delay", Long.toString(delay))); | |
| }) | |
| ) | |
| ) | |
| ) | |
| .defineAsBrowserFlow() | |
| ); | |
| } | |
| void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { | |
| final String newFlowAlias = BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES; | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) | |
| .selectFlow(newFlowAlias) | |
| .inForms(forms -> forms | |
| .clear() | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, UsernamePasswordFormFactory.PROVIDER_ID) | |
| .addSubFlowExecution(AuthenticationExecutionModel.Requirement.REQUIRED, reqSubFlow -> reqSubFlow | |
| .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-authenticator-config"); | |
| config.setConfig(Map.of("delay", Long.toString(delay))); | |
| }) | |
| ) | |
| ) | |
| ) | |
| .defineAsBrowserFlow() | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java
around lines 123 - 143, Typo in the config alias: in
configureBrowserFlowWithRecoveryAuthnCodes update the mistaken string
"delayed-suthenticator-config" to "delayed-authenticator-config" where
config.setAlias(...) is called so the delayed authenticator configuration alias
matches the authenticator name.
2 participants
Test 9
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#9