Skip to content

Implement recovery key support for user storage providers#2

Open
zaibkhan wants to merge 1 commit into
feature-recovery-keys-foundationfrom
feature-recovery-keys-implementation
Open

Implement recovery key support for user storage providers#2
zaibkhan wants to merge 1 commit into
feature-recovery-keys-foundationfrom
feature-recovery-keys-implementation

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

closes #38445

Signed-off-by: rtufisi <rtufisi@phasetwo.io>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Add recovery codes handling, ensure null-safety and flow hygiene
What’s good: Recovery Authentication Codes are integrated across authenticator, required action, and test user storage with proper delegation to user storage or local provider via CredentialHelper.
Review Status: ✅ Safe to merge

Issues (Medium)

Severity Issue Why it matters
Medium Correctness — Unprotected Optional.get() may cause NoSuchElementException …/model/RecoveryAuthnCodeInputLoginBean.java
Possible NoSuchElementException if the recovery codes credential is absent (or removed concurrently), and getNextRecoveryAuthnCode().get() can also be empty when all codes are used. Guard both Optionals to avoid runtime 500s during login.

Showing up to 1 medium issue(s). See inline suggestions for more details.

Key Feedback (click to expand)
  • Needs improvement: Harden Optional handling to avoid NoSuchElementException when credentials are missing or exhausted, and fix a typo in the flow alias for clarity.
  • Testing: Consider adding a test to cover the edge-case where no recovery codes credential is present at the time the form model is constructed (e.g., last code consumed and removed), ensuring the page renders without throwing. Optionally, add a test asserting single-use semantics for recovery codes in the test storage to align with production expectations.
  • Documentation: If recovery codes numbering is zero-based in the UI, a brief note in the template/model would reduce confusion. Also document that the testsuite user storage intentionally stores recovery codes in plaintext for compatibility testing purposes only.
  • Compatibility: Using RecoveryAuthnCodesUtils.getCredential() to search federated first then local improves parity for user storage providers and should preserve behavior across storage types.
  • Security: The testsuite user storage stores recovery codes as plaintext and validates without consuming them; given it's test-only this is acceptable, but ensure the provider isn't reused outside tests.
  • Open questions: Is there a test case covering the scenario where the recovery codes credential is removed between configuredFor() check and form rendering (race)? Should we enforce single-use semantics in the test user storage to better mirror production behavior?

Confidence: 4/5 — Looks good; minor fixes (1 medium)

Sequence Diagram

sequenceDiagram
    participant RA as RecoveryAuthnCodesAction
    participant CH as CredentialHelper
    participant UCM as user.credentialManager()
    participant CP as CredentialProvider(keycloak-recovery-authn-codes)
    RA->>CH: createRecoveryCodesCredential(session, realm, user, model, codes)
    alt userStorage update
        CH->>UCM: updateCredential(UserCredentialModel)
        UCM-->>CH: true
        CH-->>RA: (created in user storage)
    else local storage
        CH->>UCM: updateCredential(UserCredentialModel)
        UCM-->>CH: false
        CH->>CP: createCredential(realm, user, model)
        CP-->>CH: Credential created
        CH-->>RA: (created in local storage)
    end
Loading

React with 👍 or 👎 if you found this review useful.

Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);

RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel);
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Possible NoSuchElementException if the recovery codes credential is absent (or removed concurrently), and getNextRecoveryAuthnCode().get() can also be empty when all codes are used. Guard both Optionals to avoid runtime 500s during login.

.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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Low: Minor typo in the alias string ('suthenticator'); fixing it improves readability and consistency in flow debugging.

Suggested change
.addSubFlowExecution("Recovery-Authn-Codes subflow", AuthenticationFlow.BASIC_FLOW, AuthenticationExecutionModel.Requirement.ALTERNATIVE, altSubFlow -> altSubFlow
config.setAlias("delayed-authenticator-config");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants