Implement recovery key support for user storage providers#2
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce recovery codes authentication support by adding utility helpers for credential management (creation and retrieval), refactoring existing code to use these utilities, extending backwards-compatibility storage with recovery codes handling, and adding comprehensive test coverage for recovery code setup and authentication flows. Changes
Sequence DiagramssequenceDiagram
participant User
participant RecoveryAction as RecoveryAuthnCodesAction
participant CredHelper as CredentialHelper
participant Provider as RecoveryAuthnCodes Provider
participant UserStorage as User Storage
User->>RecoveryAction: Complete recovery codes setup
activate RecoveryAction
RecoveryAction->>CredHelper: createRecoveryCodesCredential(...)
activate CredHelper
CredHelper->>CredHelper: Serialize codes to JSON
CredHelper->>UserStorage: Try update credential
alt Update succeeds
UserStorage-->>CredHelper: Success
CredHelper-->>RecoveryAction: Credential created
else Update fails
CredHelper->>Provider: Create via provider
Provider-->>CredHelper: Credential created
CredHelper-->>RecoveryAction: Credential created
end
deactivate CredHelper
RecoveryAction->>User: Setup complete
deactivate RecoveryAction
sequenceDiagram
participant Browser
participant Authenticator as RecoveryAuthnCodesFormAuthenticator
participant Utils as RecoveryAuthnCodesUtils
participant CredManager as Credential Manager
participant FedCreds as Federated Credentials
Browser->>Authenticator: Check for recovery codes
activate Authenticator
Authenticator->>Utils: getCredential(user)
activate Utils
Utils->>FedCreds: Check federated credentials first
alt Federated credential found
FedCreds-->>Utils: CredentialModel
else Federated not found
Utils->>CredManager: Fall back to stored credentials
CredManager-->>Utils: CredentialModel
end
Utils-->>Authenticator: Optional<CredentialModel>
deactivate Utils
Authenticator->>Authenticator: Process credential
Authenticator-->>Browser: Response
deactivate Authenticator
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 0
🧹 Nitpick comments (8)
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (2)
76-85: Confirm intentional change to factory lookup ordering
getConfigurableAuthenticatorFactorynow triesFormActionFactorybeforeClientAuthenticatorFactory. This can change behavior if aproviderIdis implemented as both a form action and a client authenticator, as the resolved factory will differ from previous ordering. If that scenario is possible in your deployment or tests, it’s worth double-checking this change is deliberate and, if so, adding a short Javadoc note explaining the intended priority to avoid future regressions.
35-42: Recovery‑codes helper is consistent with OTP pattern; consider tightening error handlingThe new
createRecoveryCodesCredentialhelper cleanly centralizes recovery‑code creation and mirrors the existing OTP helper: try user‑storage viaupdateCredential, log when that path is used, otherwise delegate to theCredentialProvider. That looks correct and aligns with the PR’s goal of user‑storage support.Two minor polish suggestions:
- The
IOExceptionfromJsonSerialization.writeValueAsStringis wrapped in a bareRuntimeException, which makes debugging harder. Consider adding context, e.g.new RuntimeException("Failed to serialize recovery authn codes", e).- If you expect
generatedCodesto be small, this is fine; if not, a brief comment noting that the JSON is the canonical storage format for user‑storage providers would help future readers understand why the plain list is serialized here rather than reusing data fromcredentialModel.These don’t block the change, but would improve debuggability and readability.
Also applies to: 112-131
services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java (1)
16-22: Avoid rawOptional.get()to make failures clearer when credential is missing or exhaustedThe new use of
RecoveryAuthnCodesUtils.getCredential(user)is good, but callingcredentialModelOpt.get()(and latergetNextRecoveryAuthnCode().get()) assumes the credential and a next code are always present. If either assumption is violated (misconfiguration, race with credential removal, all codes already used, etc.), you’ll get aNoSuchElementExceptionwith little context.You can keep the same semantics while making the failure mode more explicit and diagnosable:
- Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); - - RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); - - this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); + Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); + + RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = + RecoveryAuthnCodesCredentialModel.createFromCredentialModel( + credentialModelOpt.orElseThrow( + () -> new IllegalStateException("Missing recovery-authn-codes credential for user " + user.getId()) + ) + ); + + this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode() + .map(code -> code.getNumber()) + .orElseThrow( + () -> new IllegalStateException("No remaining recovery authn codes for user " + user.getId()) + );This preserves the invariant (“we should only construct this bean when a valid credential with remaining codes exists”) but produces clearer diagnostics if that invariant is ever broken.
server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (1)
50-62: getCredential helper looks good; tiny Javadoc nitImplementation (federated-first with local fallback) is clear and consistent with the intended lookup behavior. Only minor suggestion is to fix the Javadoc grammar:
- * @return - a optional credential model + * @return - an optional credential modeltestsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (1)
110-120: Recovery-codes wiring in the backwards-compat provider is consistent; consider documenting non-consuming semanticsThe additions for
RecoveryAuthnCodesCredentialModel.TYPEinsupportsCredentialType,updateCredential,getCredentials,isConfiguredFor,isValid, andMyUserare internally consistent:
updateCredentialstores recovery codes as aCredentialModelwith generated ID, type,credentialData, andcreatedDate.getCredentialsdeserializesmyUser.recoveryCodes.getCredentialData()into aRecoveryAuthnCodesCredentialModeland returns it alongside OTP when present.isValiddeserializes the same list and checks whetherinput.getChallengeResponse()matches one of the stored values.Given this is a backwards-compatibility test provider, it’s reasonable that validation does not remove the used code. If you want this provider to more closely mirror production semantics (one-time use of a recovery code) or to make intent explicit, consider adding a short comment near the recovery-codes branch in
isValidexplaining that codes are intentionally non-consuming here for simplicity.If you’d like to tighten types a bit, you could also switch the raw
Listdeserialization to a typed variant, but that’s non-essential for tests. For example:- List generatedKeys; + List<String> generatedKeys; try { - generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); + generatedKeys = JsonSerialization.readValue( + storedRecoveryKeys.getCredentialData(), + new org.keycloak.util.JsonSerialization.TypeReference<List<String>>() {} + );()
Also applies to: 169-205, 230-252, 273-281, 326-341, 422-440
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (3)
74-106: Flow configuration for recovery codes is clear; fix minor alias typoThe new browser flow setup using
FlowUtiland the dedicated constant forBROWSER_FLOW_WITH_RECOVERY_AUTHN_CODESis readable and nicely encapsulated inconfigureBrowserFlowWithRecoveryAuthnCodes.One tiny nit: the alias for the delayed authenticator has a typo, which may make debugging harder later:
- .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { - config.setAlias("delayed-suthenticator-config"); + .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { + config.setAlias("delayed-authenticator-config");Also applies to: 123-143
240-269: Avoid hard-coding the expected recovery code index in the helper
testRecoveryKeysSetupAndLogincurrently calls:enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys);and
enterRecoveryCodesasserts that the page always asks for code index0. This makes the test brittle if the authenticator’s selection strategy changes (for example, random or non-zero index) even though the overall behavior remains correct.You can make the helper resilient by letting it adapt to whichever index the page requests, only asserting that the index is within bounds:
- private void enterRecoveryCodes(EnterRecoveryAuthnCodePage enterRecoveryAuthnCodePage, WebDriver driver, - int expectedCode, List<String> generatedRecoveryAuthnCodes) { + private void enterRecoveryCodes(EnterRecoveryAuthnCodePage enterRecoveryAuthnCodePage, WebDriver driver, + List<String> generatedRecoveryAuthnCodes) { enterRecoveryAuthnCodePage.setDriver(driver); enterRecoveryAuthnCodePage.assertCurrent(); - int requestedCode = enterRecoveryAuthnCodePage.getRecoveryAuthnCodeToEnterNumber(); - org.junit.Assert.assertEquals("Incorrect code presented to login", expectedCode, requestedCode); - enterRecoveryAuthnCodePage.enterRecoveryAuthnCode(generatedRecoveryAuthnCodes.get(requestedCode)); + int requestedCode = enterRecoveryAuthnCodePage.getRecoveryAuthnCodeToEnterNumber(); + org.junit.Assert.assertTrue("Requested code index is out of range", + requestedCode >= 0 && requestedCode < generatedRecoveryAuthnCodes.size()); + enterRecoveryAuthnCodePage.enterRecoveryAuthnCode(generatedRecoveryAuthnCodes.get(requestedCode)); }and at the call site:
- enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys); + enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, recoveryKeys);This still verifies correctness while decoupling the test from a specific index choice.
Also applies to: 476-483
410-436: Required action string for recovery keys: align with existing pattern (optional)
setupRecoveryKeysForUserWithRequiredActionusesname()for the required action, while the OTP helper above usestoString():userRep.setRequiredActions(Arrays.asList( UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name()));Given
RequiredActionis an enum, bothname()andtoString()are currently equivalent, but for consistency with the OTP path and to reduce surprise iftoString()were ever customized, you could align them:- userRep.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name())); + userRep.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.toString()));Purely stylistic; behavior today is the same.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java(3 hunks)server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java(2 hunks)services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java(1 hunks)services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java(3 hunks)services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java(1 hunks)testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java(12 hunks)testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.java(1 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java(8 hunks)
🔇 Additional comments (4)
services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java (1)
28-29: Good reuse of shared helper for recovery‑codes creationSwitching
processActionto useCredentialHelper.createRecoveryCodesCredential(...)is a nice consolidation: you gain the user‑storage‑aware behavior and keep the localCredentialProvidercreation path in one place. The flow (validate inputs, optionally logout sessions, create credential, thensuccess()) remains clear and unchanged from the caller’s perspective.Also applies to: 109-118
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.java (1)
53-57: Test helper for recovery codes mirrors existing OTP helper
hasRecoveryCodesis a simple, clear mirror ofhasUserOTP, with the same null‑safe pattern over the shareduserPasswordsmap. This should integrate cleanly with tests needing to assert recovery‑codes presence in the backwards‑compatibility storage.services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (1)
80-93: Utility‑based credential lookup improves coverage for different storage backendsUsing
RecoveryAuthnCodesUtils.getCredential(authenticatedUser)here is a solid improvement over the previous directcredentialManagerstream: it centralizes the lookup logic (including federated vs. local credentials) and keeps the rest of the flow the same:
- If a credential is found, you reconstruct
RecoveryAuthnCodesCredentialModeland remove it when all codes are used.- If no credential is found or all codes are used, you add the
CONFIGURE_RECOVERY_AUTHN_CODESrequired action.This keeps the authenticator logic readable while making it more robust across different user‑storage implementations.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (1)
456-463: Storage assertion and account REST helper look consistent
assertUserHasRecoveryKeysCredentialInUserStoragemirrors the existing OTP helper and cleanly reusesBackwardsCompatibilityUserStorageFactory.hasRecoveryCodes("otp1")viatestingClient.server().fetch(...). This is a good pattern for asserting against custom user storage without leaking implementation details into the test.- The use of
new TypeReference<>() {}ingetOtpCredentialFromAccountRESTkeeps the type signature of the local variable authoritative while simplifying the generic boilerplate.No changes needed here.
Also applies to: 466-473
2 participants
Test 9
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#9