Provision the user credential from the flow user in password recovery and ask password flows - #1152
Provision the user credential from the flow user in password recovery and ask password flows#1152KD23243 wants to merge 3 commits into
Conversation
… and ask password flows
📝 WalkthroughWalkthroughChangesProvisioning flow changes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UserProvisioningExecutor
participant ThreadLocalFlow
participant IdentityEventService
participant UserStoreManager
UserProvisioningExecutor->>ThreadLocalFlow: enter invited-registration flow
UserProvisioningExecutor->>IdentityEventService: dispatch PRE_ADD_NEW_PASSWORD
UserProvisioningExecutor->>UserStoreManager: update user password
UserProvisioningExecutor->>ThreadLocalFlow: exit flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/PasswordProvisioningExecutor.java (1)
69-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale executor test
PasswordProvisioningExecutorTest.testExecuteWithValidDatastill verifiesupdateCredentialByAdmin(...), butPasswordProvisioningExecutor.execute(...)now only stores the password onFlowUser. Remove that verify here and place it inUserProvisioningExecutorTest, where the password is actually persisted.🤖 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 `@components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/PasswordProvisioningExecutor.java` around lines 69 - 85, The testPasswordProvisioningExecutorTest.testExecuteWithValidData assertion should no longer verify updateCredentialByAdmin, since PasswordProvisioningExecutor.execute only stores credentials on FlowUser. Remove that verification from PasswordProvisioningExecutorTest and add the corresponding verification to UserProvisioningExecutorTest, where password persistence occurs.
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/UserProvisioningExecutor.java (2)
220-220: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
getOrDefaulteagerly generates a throwaway password.
DefaultPasswordGenerator().generatePassword()is evaluated on every call, even whenPASSWORD_KEYis present, wasting work and leaving a generated passwordchar[]in memory that is never zeroized (only the returnedpasswordreference is wiped). ConsidercomputeIfAbsent/an explicit branch so the generator only runs when actually needed.🤖 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 `@components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/UserProvisioningExecutor.java` at line 220, Update the password lookup in UserProvisioningExecutor to avoid eagerly calling DefaultPasswordGenerator.generatePassword when PASSWORD_KEY already exists. Use an explicit presence check or lazy computeIfAbsent approach so generation occurs only when needed, while preserving the existing password reference cleanup behavior.
200-210: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMove
enterFlow()out of thetryto keep enter/exit balanced.
enterFlow()is the first statement inside thetry, butexitFlow()runs infinally. IfenterFlow()throws after (or without) pushing the flow,finallywill still callexitFlow(), popping a flow that was never entered and corrupting the thread-local flow stack.♻️ Suggested restructuring
- try { - enterFlow(); - handlePrePasswordUpdate(context, user, recoveryScenario, confirmationCode); + enterFlow(); + try { + handlePrePasswordUpdate(context, user, recoveryScenario, confirmationCode); updateUserPassword(userCredentials, flowUser, userStoreManager); String userId = ((AbstractUserStoreManager) userStoreManager) .getUserIDFromUserName(flowUser.getUsername()); flowUser.setUserId(userId); flowUser.setUserStoreDomain(user.getUserStoreDomain()); } finally { IdentityContext.getThreadLocalIdentityContext().exitFlow(); }🤖 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 `@components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/UserProvisioningExecutor.java` around lines 200 - 210, Move enterFlow() in the UserProvisioningExecutor flow so it executes immediately before the try block, while leaving the existing exitFlow() call in finally. Keep the password update and user identity-setting operations inside the try, ensuring exitFlow() runs only after a successful enterFlow().
🤖 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.
Outside diff comments:
In
`@components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/PasswordProvisioningExecutor.java`:
- Around line 69-85: The
testPasswordProvisioningExecutorTest.testExecuteWithValidData assertion should
no longer verify updateCredentialByAdmin, since
PasswordProvisioningExecutor.execute only stores credentials on FlowUser. Remove
that verification from PasswordProvisioningExecutorTest and add the
corresponding verification to UserProvisioningExecutorTest, where password
persistence occurs.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/UserProvisioningExecutor.java`:
- Line 220: Update the password lookup in UserProvisioningExecutor to avoid
eagerly calling DefaultPasswordGenerator.generatePassword when PASSWORD_KEY
already exists. Use an explicit presence check or lazy computeIfAbsent approach
so generation occurs only when needed, while preserving the existing password
reference cleanup behavior.
- Around line 200-210: Move enterFlow() in the UserProvisioningExecutor flow so
it executes immediately before the try block, while leaving the existing
exitFlow() call in finally. Keep the password update and user identity-setting
operations inside the try, ensuring exitFlow() runs only after a successful
enterFlow().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7280808d-6bad-436e-946b-7a0562f75f7c
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/PasswordProvisioningExecutor.javacomponents/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/executor/UserProvisioningExecutor.java
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/29738587561
| response.setResult(STATUS_COMPLETE); | ||
| return response; | ||
| } catch (UserStoreException e) { | ||
| response = buildClientErrorResponseForActionFailure(response, e, |
There was a problem hiding this comment.
can we guarantee that only the action failures would occur?
There was a problem hiding this comment.
Yes, buildClientErrorResponseForActionFailure will check UserStoreClientException for an action related error code, and return the existing value if it isn't found.
| ? LoggerUtils.getMaskedContent(user.getUsername()) | ||
| : user.getUsername(); | ||
| LOG.error("Error while updating password for user: " + maskedUsername, e); | ||
| throw new FlowEngineException(null, e.getMessage(), null, e); |
There was a problem hiding this comment.
dont we need error codes?
There was a problem hiding this comment.
This preserves the existing behavior. In the old PasswordProvisioningExecutor, the generic password update failure path used errorResponse(response, e.getMessage()), which only set the message + STATUS_ERROR and not a code, so the null here keeps the same response.
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/29896517209
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1152 +/- ##
============================================
+ Coverage 55.19% 55.30% +0.10%
- Complexity 3337 3339 +2
============================================
Files 317 317
Lines 22200 22156 -44
Branches 4589 4581 -8
============================================
- Hits 12254 12253 -1
+ Misses 8365 8327 -38
+ Partials 1581 1576 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
Purpose
In the password recovery and ask password (invited user registration) flows, the provisioned credential should reflect the value present on the flow user at the time of provisioning, so that a flow extension configured after the password provisioning step can override it. Previously the provisioning step re-read the retained raw user input, discarding any credential set on the flow user by a later extension.
Related Issue
Manual Testing
Implementation
PasswordProvisioningExecutornow only captures the submitted password onto the flow user (FlowUser.setUserCredentials) and returns; it no longer updates the credential itself.UserProvisioningExecutor. It reads the credential from the flow user (getUserCredentials), so a value overridden by a flow extension between the two steps is the value that gets persisted.UserProvisioningExecutor.This covers the server-side credential provisioning behavior; the Console flow builder changes for exposing the flow extension step are handled separately.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests