Create recovery keys in user storage or local - #1
Open
linxia0415 wants to merge 1 commit into
Open
Conversation
Signed-off-by: rtufisi <rtufisi@phasetwo.io>
📝 WalkthroughWalkthroughThis PR enables custom UserStorageProviders to manage recovery-authn-codes credentials alongside passwords and OTP. It introduces abstraction utilities for credential retrieval and creation, refactors existing components to use these centralized helpers, and extends a test user storage provider to demonstrate full recovery-codes credential lifecycle support including creation, validation, and retrieval. ChangesRecovery Codes User Storage Delegation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (1)
326-341: 💤 Low valueRecovery code is not consumed after successful validation.
In production recovery-code implementations, codes are one-time use and should be removed or marked as consumed after successful validation. This implementation allows the same code to be reused indefinitely.
This is acceptable for a test provider, but if you intend this to more accurately simulate real user storage behavior, consider removing the matched code from the list after successful validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java` around lines 326 - 341, The validation currently reads storedRecoveryKeys (myUser.recoveryCodes) into generatedKeys and returns true if any key equals input.getChallengeResponse() but never removes the matched code; update the logic in BackwardsCompatibilityUserStorage (the RecoveryAuthnCodesCredentialModel.TYPE branch) so that after finding a matching key you remove that key from generatedKeys, reserialize the updated list (use JsonSerialization.readValue/ writeValueAsString on storedRecoveryKeys.getCredentialData()), set the new credential data on myUser.recoveryCodes, and persist the change via your user/credential update path so the code is consumed and cannot be reused.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (1)
135-135: 💤 Low valueTypo in config alias: "suthenticator" should be "authenticator".
Minor typo in the configuration alias name.
Suggested fix
- config.setAlias("delayed-suthenticator-config"); + config.setAlias("delayed-authenticator-config");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java` at line 135, The alias string passed to config.setAlias contains a typo; update the alias in BackwardsCompatibilityUserStorageTest so the call config.setAlias("delayed-suthenticator-config") is corrected to config.setAlias("delayed-authenticator-config") to use the proper "authenticator" spelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`:
- Around line 17-19: The code calls credentialModelOpt.get() without checking
the Optional returned by RecoveryAuthnCodesUtils.getCredential in
RecoveryAuthnCodeInputLoginBean, which can throw NoSuchElementException; update
the code to handle the absent case (either check credentialModelOpt.isPresent()
before calling RecoveryAuthnCodesCredentialModel.createFromCredentialModel, or
use credentialModelOpt.orElseThrow(...) to throw a clear, specific exception, or
return/handle a missing-credential state) so that
RecoveryAuthnCodesCredentialModel.createFromCredentialModel is only invoked with
a present CredentialModel.
---
Nitpick comments:
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`:
- Around line 326-341: The validation currently reads storedRecoveryKeys
(myUser.recoveryCodes) into generatedKeys and returns true if any key equals
input.getChallengeResponse() but never removes the matched code; update the
logic in BackwardsCompatibilityUserStorage (the
RecoveryAuthnCodesCredentialModel.TYPE branch) so that after finding a matching
key you remove that key from generatedKeys, reserialize the updated list (use
JsonSerialization.readValue/ writeValueAsString on
storedRecoveryKeys.getCredentialData()), set the new credential data on
myUser.recoveryCodes, and persist the change via your user/credential update
path so the code is consumed and cannot be reused.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`:
- Line 135: The alias string passed to config.setAlias contains a typo; update
the alias in BackwardsCompatibilityUserStorageTest so the call
config.setAlias("delayed-suthenticator-config") is corrected to
config.setAlias("delayed-authenticator-config") to use the proper
"authenticator" spelling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fd45d0c-f1bb-4732-96f2-a1d1a924e939
📒 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
Comment on lines
+17
to
+19
| 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() may throw NoSuchElementException.
Calling .get() on credentialModelOpt without checking isPresent() will throw a NoSuchElementException if no credential is found. While this bean is presumably only instantiated when a credential exists, defensive handling would be safer.
🛡️ Proposed fix to handle missing credential gracefully
- Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
-
- RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
+ CredentialModel credentialModel = RecoveryAuthnCodesUtils.getCredential(user)
+ .orElseThrow(() -> new IllegalStateException("Recovery authentication codes credential not found for user"));
+ RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`
around lines 17 - 19, The code calls credentialModelOpt.get() without checking
the Optional returned by RecoveryAuthnCodesUtils.getCredential in
RecoveryAuthnCodeInputLoginBean, which can throw NoSuchElementException; update
the code to handle the absent case (either check credentialModelOpt.isPresent()
before calling RecoveryAuthnCodesCredentialModel.createFromCredentialModel, or
use credentialModelOpt.orElseThrow(...) to throw a clear, specific exception, or
return/handle a missing-credential state) so that
RecoveryAuthnCodesCredentialModel.createFromCredentialModel is only invoked with
a present CredentialModel.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create recovery keys in user storage or local. Closes keycloak/keycloak#38445
Summary by CodeRabbit
Release Notes
New Features
Tests