Implement recovery key support for user storage providers#9
Open
wangyue6761 wants to merge 1 commit intofeature-recovery-keys-foundationfrom
Open
Implement recovery key support for user storage providers#9wangyue6761 wants to merge 1 commit intofeature-recovery-keys-foundationfrom
wangyue6761 wants to merge 1 commit intofeature-recovery-keys-foundationfrom
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
Owner
Author
|
@cpgbot review |
wangyue6761
commented
Jan 7, 2026
Owner
Author
wangyue6761
left a comment
There was a problem hiding this comment.
Triggered by @cpgbot review. Total issues: 6.
| public RecoveryAuthnCodeInputLoginBean(KeycloakSession session, RealmModel realm, UserModel user) { | ||
| CredentialModel credentialModel = user.credentialManager().getStoredCredentialsByTypeStream(RecoveryAuthnCodesCredentialModel.TYPE) | ||
| .findFirst().get(); | ||
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); |
Owner
Author
There was a problem hiding this comment.
- ERROR
RiskType.ROBUSTNESS_BOUNDARY_CONDITIONS(confidence 0.85) line (17, 21)- 代码在调用credentialModelOpt.get()(第19行)和recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get()(第21行)之前未检查Optional是否包含值。RecoveryAuthnCodesUtils.getCredential(user)可能返回空的Optional(当用户没有恢复认证码凭证时),RecoveryAuthnCodesCredentialModel.getNextRecoveryAuthnCode()可能返回空的Optional(当所有代码都已使用时)。直接调用Optional.get()会在Optional为空时抛出NoSuchElementException,导致运行时崩溃。
- Suggestion: 建议添加Optional值检查:1) 在调用credentialModelOpt.get()前使用if(credentialModelOpt.isPresent())或credentialModelOpt.orElse(null)处理空值情况;2) 在调用getNextRecoveryAuthnCode().get()前检查Optional是否包含值,或使用orElse()提供默认值。例如:credentialModelOpt.ifPresent(cred -> { ... }) 或 int codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().map(RecoveryAuthnCodeRepresentation::getNumber).orElse(0);
| /** | ||
| * Create RecoveryCodes credential either in userStorage or local storage (Keycloak DB) | ||
| */ | ||
| public static void createRecoveryCodesCredential(KeycloakSession session, RealmModel realm, UserModel user, RecoveryAuthnCodesCredentialModel credentialModel, List<String> generatedCodes) { |
Owner
Author
There was a problem hiding this comment.
- WARNING
RiskType.ROBUSTNESS_BOUNDARY_CONDITIONS(confidence 0.85) line (115, 131)- 方法createRecoveryCodesCredential中,session.getProvider(CredentialProvider.class, "keycloak-recovery-authn-codes")可能返回null(根据DefaultKeycloakSessionFactory.getProviderFactory和DefaultKeycloakSession.getOrCreateProvider的实现),但在第129行直接调用了recoveryCodeCredentialProvider.createCredential()方法,没有进行null检查。如果provider未正确注册或配置,将导致NullPointerException。类似的createOTPCredential方法也存在相同问题。
- Suggestion: 在调用recoveryCodeCredentialProvider.createCredential()之前添加null检查:if (recoveryCodeCredentialProvider != null) { recoveryCodeCredentialProvider.createCredential(realm, user, credentialModel); } else { logger.warnf("Recovery codes credential provider not found for user '%s'", user.getUsername()); }。同时建议检查createOTPCredential方法中的类似问题。
| storedOTPCredential.getPeriod(), realm.getOTPPolicy().getLookAheadWindow()); | ||
| return validator.validateTOTP(otpCredential.getValue(), storedOTPCredential.getValue().getBytes()); | ||
| } else { | ||
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { |
Owner
Author
There was a problem hiding this comment.
- WARNING
RiskType.ROBUSTNESS_BOUNDARY_CONDITIONS(confidence 0.85) line (326, 341)- 在验证恢复码时,直接访问myUser.recoveryCodes的credentialData字段进行JSON反序列化,未检查credentialData是否为null或空字符串。CredentialModel.getCredentialData()可能返回null(如果credentialData字段为null),而JsonSerialization.readValue()在传入null时会抛出NullPointerException。虽然在isValid方法中有try-catch处理IOException,但NullPointerException不是IOException的子类,不会被捕获。同样的问题也存在于getCredentials方法(行237-238)中。
- Suggestion: 在调用JsonSerialization.readValue()之前,检查getCredentialData()的返回值是否为null或空字符串。例如:String credentialData = storedRecoveryKeys.getCredentialData(); if (credentialData == null || credentialData.trim().isEmpty()) { log.warnf(...); return false; }
| * @param user - User model | ||
| * @return - a optional credential model | ||
| */ | ||
| public static Optional<CredentialModel> getCredential(UserModel user) { |
Owner
Author
There was a problem hiding this comment.
- WARNING
RiskType.ROBUSTNESS_BOUNDARY_CONDITIONS(confidence 0.80) line (56, 62)- 方法
getCredential(UserModel user)在第57行直接调用user.credentialManager(),未检查user参数是否为 null。根据AuthenticationFlowContext接口文档(第40-44行),getUser()方法可能返回 null("It can return null if no user has been identified yet")。在RecoveryAuthnCodesFormAuthenticator.java第55行,authenticatedUser从authnFlowContext.getUser()获取后直接传递给此方法,存在 null 风险。当user为 null 时,调用user.credentialManager()会抛出NullPointerException。 - Suggestion: 在方法开始处添加 null 检查:
if (user == null) return Optional.empty();或使用Objects.requireNonNull(user, "User cannot be null");抛出明确的异常。
- 方法
| assertTrue(testAppHelper.logout()); | ||
| } | ||
|
|
||
| @Test |
Owner
Author
There was a problem hiding this comment.
- WARNING
RiskType.LIFECYCLE_STATE_CONSISTENCY(confidence 0.55) line (240, 269)- testRecoveryKeysSetupAndLogin测试方法中,在finally块中调用了BrowserFlowTest.revertFlows来恢复浏览器流,但该方法可能无法完全清理测试期间创建的所有临时资源,特别是当测试在中间步骤失败时。查看revertFlows方法实现,它主要恢复默认浏览器流并删除创建的流,但configureBrowserFlowWithRecoveryAuthnCodes方法可能创建了其他资源(如组件),这些资源在测试失败时可能不会被清理。
- Suggestion: 建议在finally块中添加更全面的清理逻辑,确保测试期间创建的所有临时资源(包括组件、流等)都被正确清理。可以考虑使用TestCleanup机制或确保revertFlows方法能处理所有可能的资源清理场景。
| result = true; | ||
| Optional<CredentialModel> optUserCredentialFound = authenticatedUser.credentialManager().getStoredCredentialsByTypeStream( | ||
| RecoveryAuthnCodesCredentialModel.TYPE).findFirst(); | ||
| Optional<CredentialModel> optUserCredentialFound = RecoveryAuthnCodesUtils.getCredential(authenticatedUser); |
Owner
Author
There was a problem hiding this comment.
- WARNING
RiskType.LIFECYCLE_STATE_CONSISTENCY(confidence 0.45) line (80, 94)- 在成功验证恢复码后,获取并处理凭证模型时,未检查凭证模型是否已过期或处于无效状态,可能导致对已失效凭证进行操作。
- Suggestion: 在验证恢复码成功后,处理凭证模型前,应检查凭证模型的状态。建议检查CredentialModel的createdDate字段或添加过期时间字段,确保凭证在有效期内。例如:if (credentialModel.getCreatedDate() + EXPIRY_TIME < System.currentTimeMillis()) { // 处理过期逻辑 }
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
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.
Recreated from upstream PR ai-code-review-evaluation#9
Original title: Implement recovery key support for user storage providers
Test 9