Implement recovery key support for user storage providers#2
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| return false; | ||
| } | ||
|
|
||
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); |
There was a problem hiding this comment.
Recovery code validation accepts any code, not sequential
Medium Severity
BackwardsCompatibilityUserStorage.isValid for recovery codes uses generatedKeys.stream().anyMatch(...) which validates the input against ALL stored codes. The real RecoveryAuthnCodesCredentialProvider.isValid only validates against the next sequential unused code via getNextRecoveryAuthnCode(). This means the test provider accepts codes out of order and allows code reuse, since it also never consumes/removes used codes. This undermines the backwards compatibility test's ability to verify correct sequential recovery code behavior with user storage.
| myUser.recoveryCodes.getCreatedDate(), | ||
| myUser.recoveryCodes.getUserLabel() | ||
| ); | ||
| credentialModels.add(model); |
There was a problem hiding this comment.
Federated credential returned without preserving stored ID
Medium Severity
BackwardsCompatibilityUserStorage.getCredentials() creates a RecoveryAuthnCodesCredentialModel via createFromValues but never copies the ID from myUser.recoveryCodes.getId() onto the new model. The returned model has a null ID. This contrasts with the OTP handling in the same method which returns myUser.getOtp() with its ID intact. Production code in RecoveryAuthnCodesFormAuthenticator calls removeStoredCredentialById(recoveryCodeCredentialModel.getId()) when all codes are used, which would pass null.


Test 9nnn## Summary by CodeRabbitnn## New Featuresnn* Recovery Codes Authentication: Added support for recovery codes as an authentication credential type, enabling users to set up and use recovery codes during login as an additional authentication method.n* Recovery Code Verification: Implemented recovery code validation and verification within the authentication flow.n* Backwards Compatibility: Extended legacy storage systems to support recovery codes credentials alongside existing authentication methods.nn✏️ Tip: You can customize this high-level summary in your review settings.nnn---nReplicated from ai-code-review-evaluation/keycloak-coderabbit#9
Note
Medium Risk
Touches authentication credential creation/lookup paths and adds federated-storage handling for recovery codes, which could impact login if storage implementations or serialization differ. Changes are scoped and backed by integration tests, but still affect security-sensitive flows.
Overview
Adds end-to-end support for persisting recovery authentication codes in user storage providers: a new
CredentialHelper.createRecoveryCodesCredentialwrites codes to federated storage when supported (falling back to local credential provider), andRecoveryAuthnCodesUtils.getCredentialcentralizes lookup across federated vs local storage.Updates the recovery-codes login/required-action flows to use the federated-aware lookup and creation paths, and extends the backwards-compatibility user storage test provider and integration tests to store, expose, and validate recovery codes (including a new browser flow test that exercises recovery-code login).
Written by Cursor Bugbot for commit 134437a. Configure here.