Add ResolveExistingUserBeforeConsentPrompt config to link existing local users before JIT prompt-consent redirect#8194
Conversation
📝 WalkthroughWalkthroughThe PR adds a new JIT provisioning configuration flag, exposes it in server configuration, and updates ChangesJIT provisioning resolve-before-consent flow
Sequence Diagram(s)sequenceDiagram
participant JITProvisioningPostAuthenticationHandler
participant ExternalIdPConfig
participant AccountLookup
participant LocalUserStore
JITProvisioningPostAuthenticationHandler->>ExternalIdPConfig: read prompt-consent, associate-local-user, resolve-before-consent
alt resolve-before-consent enabled and no associated local user
JITProvisioningPostAuthenticationHandler->>AccountLookup: resolve existing local user
alt no account lookup mappings
AccountLookup->>LocalUserStore: match email claim to local username
else account lookup mappings configured
AccountLookup->>LocalUserStore: find user by mapped attributes
end
JITProvisioningPostAuthenticationHandler->>JITProvisioningPostAuthenticationHandler: create federated association
end
Related Issues: Suggested Labels: bug, config, area/identity-server Suggested Reviewers: Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java (2)
410-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the fixed cross-IdP linking scenario.
The PR description notes no automated tests were added, only manual verification. This fix addresses a real correctness bug (existing consent redirect test at
JITProvisioningPostAuthenticationHandlerTest.java:330-365only coverspromptConsent=truewith no prior association). Consider adding a test whereassociateLocalUserandpromptConsentare both enabled and a matching local account already exists, assertingcallDefaultProvisioningHandler/association is invoked andsendRedirectis NOT called.Want me to draft this test case?
🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java` around lines 410 - 423, The fixed cross-IdP linking flow needs a regression test in JITProvisioningPostAuthenticationHandlerTest to cover the case where both associateLocalUser and promptConsent are enabled and a matching local account already exists. Add a test around the JITProvisioningPostAuthenticationHandler logic that verifies the association/default provisioning path is taken via callDefaultProvisioningHandler (or the relevant association helper) and that sendRedirect is not invoked, since this scenario should not fall into the consent redirect branch.
410-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a decision-point log for this fixed branch.
Since this exact reordering is the fix for the cross-IdP JIT linking bug, a log line right before this check stating whether an existing association was found (and therefore whether the consent redirect is skipped or triggered) would help confirm correct behavior in production and aid future debugging. As per path instructions, "Add logs around meaningful business logic decisions/branches (e.g., which path is chosen: local account association vs prompt-consent redirect)."
💡 Suggested addition
+ if (log.isDebugEnabled()) { + log.debug("Associated local user resolution result for IDP " + + externalIdPConfig.getIdPName() + ": " + (StringUtils.isNotBlank(associatedLocalUser) + ? "found, skipping consent redirect" : "not found")); + } if (StringUtils.isEmpty(associatedLocalUser) && externalIdPConfig.isPromptConsentEnabled()) {🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java` around lines 410 - 423, Add a decision-point debug log in JITProvisioningPostAuthenticationHandler around the existing associatedLocalUser and externalIdPConfig.isPromptConsentEnabled branch so it’s clear whether the flow found a local association or is taking the consent redirect path. Log the branch outcome immediately before the check in the post-authentication logic, using the existing context from sequenceConfig.getAuthenticatedUser(), externalIdPConfig, and associatedLocalUser to indicate whether the redirect will be skipped or triggered. Keep the message focused on this business decision so production debugging can confirm the fixed cross-IdP JIT linking behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java`:
- Around line 410-423: The fixed cross-IdP linking flow needs a regression test
in JITProvisioningPostAuthenticationHandlerTest to cover the case where both
associateLocalUser and promptConsent are enabled and a matching local account
already exists. Add a test around the JITProvisioningPostAuthenticationHandler
logic that verifies the association/default provisioning path is taken via
callDefaultProvisioningHandler (or the relevant association helper) and that
sendRedirect is not invoked, since this scenario should not fall into the
consent redirect branch.
- Around line 410-423: Add a decision-point debug log in
JITProvisioningPostAuthenticationHandler around the existing associatedLocalUser
and externalIdPConfig.isPromptConsentEnabled branch so it’s clear whether the
flow found a local association or is taking the consent redirect path. Log the
branch outcome immediately before the check in the post-authentication logic,
using the existing context from sequenceConfig.getAuthenticatedUser(),
externalIdPConfig, and associatedLocalUser to indicate whether the redirect will
be skipped or triggered. Keep the message focused on this business decision so
production debugging can confirm the fixed cross-IdP JIT linking behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0f96dc6d-5e27-4f8a-83fb-dd26a2e05c05
📒 Files selected for processing (1)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8194 +/- ##
=========================================
Coverage 52.98% 52.98%
+ Complexity 21087 21040 -47
=========================================
Files 2197 2197
Lines 129494 129494
Branches 19370 19370
=========================================
Hits 68612 68612
Misses 52506 52506
Partials 8376 8376
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:
|
…foreConsentPrompt config
0e4b3af to
33911f4
Compare
KaveeshaPiumini
changed the title
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java (2)
801-874: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo automated test coverage for the new pre-consent resolution path.
This method encapsulates the core fix for the reported bug (existing local account being ignored due to consent-redirect ordering), but the PR adds no unit/integration tests for it — only manual verification is noted. Given this touches authentication/account-linking logic, consider adding test cases for: (1) email-based match found before consent redirect, (2) attribute-mapping match found before consent redirect, (3) no match found → falls through to signup redirect, and (4) flag disabled → legacy behavior preserved.
🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java` around lines 801 - 874, Add automated tests for the new pre-consent user resolution flow in resolveAndAssociateExistingLocalUser to cover the account-linking paths added by this change. Create cases for email-username lookup, mapped-attribute lookup, and the no-match path that should fall through to signup redirect, plus a regression case where the pre-consent flag is disabled to confirm legacy behavior remains unchanged. Use JITProvisioningPostAuthenticationHandler and resolveAndAssociateExistingLocalUser as the main anchors for locating the logic.
343-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a log at this flag-driven decision point.
The reorder logic is correct, but this branch — gating whether resolution happens before the consent redirect — has no log statement of its own (only the inner helper logs its sub-branches). This is precisely the kind of "reorder/flag-driven path decision" the logging guidelines call out.
📝 Proposed log addition
if (isResolveExistingUserBeforeConsentPromptEnabled() && externalIdPConfig.isAssociateLocalUserEnabled() && StringUtils.isEmpty(associatedLocalUser) && externalIdPConfig.isPromptConsentEnabled()) { associatedLocalUser = resolveAndAssociateExistingLocalUser(externalIdPConfig, context, localClaimValues, federatedClaimValues, stepConfig, externalSubject); + if (log.isDebugEnabled()) { + log.debug("Attempted to resolve an existing local user before the consent prompt for " + + "IdP: " + externalIdPConfig.getIdPName() + ", match found: " + + StringUtils.isNotEmpty(associatedLocalUser)); + } }As per path instructions, "Add logs around major method execution and key business-logic branches/decision points (e.g., the reorder/flag-driven path decisions)."
🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java` around lines 343 - 352, Add a log statement at the flag-driven branch inside JITProvisioningPostAuthenticationHandler’s resolve-and-associate flow so the decision to run before the consent prompt is visible. Keep the existing condition intact, but log when isResolveExistingUserBeforeConsentPromptEnabled(), isAssociateLocalUserEnabled(), and isPromptConsentEnabled() drive the branch before calling resolveAndAssociateExistingLocalUser, since the helper already logs its internals and this outer decision point currently has no trace.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.java`:
- Around line 801-874: Add automated tests for the new pre-consent user
resolution flow in resolveAndAssociateExistingLocalUser to cover the
account-linking paths added by this change. Create cases for email-username
lookup, mapped-attribute lookup, and the no-match path that should fall through
to signup redirect, plus a regression case where the pre-consent flag is
disabled to confirm legacy behavior remains unchanged. Use
JITProvisioningPostAuthenticationHandler and
resolveAndAssociateExistingLocalUser as the main anchors for locating the logic.
- Around line 343-352: Add a log statement at the flag-driven branch inside
JITProvisioningPostAuthenticationHandler’s resolve-and-associate flow so the
decision to run before the consent prompt is visible. Keep the existing
condition intact, but log when
isResolveExistingUserBeforeConsentPromptEnabled(),
isAssociateLocalUserEnabled(), and isPromptConsentEnabled() drive the branch
before calling resolveAndAssociateExistingLocalUser, since the helper already
logs its internals and this outer decision point currently has no trace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fd43b6d1-c06f-4173-99b4-563979d95cc6
📒 Files selected for processing (3)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/JITProvisioningPostAuthenticationHandler.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkConstants.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2
1 participant
Purpose
Fixes wso2/product-is#27811.
With JIT provisioning in a prompt scheme (Provisioning with Consent / with Password & Consent / with Username, Password & Consent), a user who already has a local account and authenticates through a second federated IdP is redirected to the account-create (sign-up) form instead of being linked to their existing account and logged in. No new account is created — the existing user is simply, and misleadingly, shown a sign-up form.
Root cause
In
JITProvisioningPostAuthenticationHandler.handleRequestFlow(), theisPromptConsentEnabled()→redirectToAccountCreateUI()block runs before theisAssociateLocalUserEnabled()existing-account lookup. Under a prompt scheme the consent flow short-circuits to sign-up (return INCOMPLETE) before any existing local account can be matched and linked.Fix
Resolve and link the federated identity to an existing local account before the prompt-consent redirect; the sign-up redirect then fires only when no local account is matched. The change is gated by two flags:
isResolveExistingUserBeforeConsentPromptEnabled()— a new server-level config; the master switch for the behaviour (default off).externalIdPConfig.isAssociateLocalUserEnabled()— the existing per-IdP setting; selects which IdPs the behaviour applies to.So the behaviour is off unless the server config is enabled and the IdP has associate-local-user enabled — per-IdP control rather than a server-wide all-or-nothing change. The duplicated inline lookup is collapsed into a shared
resolveAndAssociateExistingLocalUser(...)helper (covering both the email-username and account-lookup-attribute-mapping paths).Configuration
New server-level config
JITProvisioning.ResolveExistingUserBeforeConsentPrompt, disabled by default and emitted intoidentity.xmlonly when the operator defines the key:Behavioural change
With the server config enabled, an existing local user authenticating through a federated IdP that has associate-local-user enabled, under a prompt scheme, is linked to their existing account and logged in instead of being shown the sign-up form. With the config disabled (default) — or for IdPs without associate-local-user — behaviour is unchanged.
Testing
Verified at runtime on a dual-IS OIDC-federation setup (primary + a second WSO2 IS as IdP-B, both JIT "Password & Consent" with associate-local-user + email→username lookup), against an existing local user provisioned earlier via Google:
... coming from <IdP> do have a local account, with the username <user>./accountrecoveryendpoint/signup.doexactly as before; theResolveExistingUserBeforeConsentPromptelement is absent fromidentity.xmland the new block is a no-op. Behaviour identical to the unpatched build.Related issue
promptConsentenabled redirects existing local users to signup when authenticating via a second federated IdP product-is#27811Developer checklist
impact/behavioral-changeaddedJITProvisioning.ResolveExistingUserBeforeConsentPrompt(default off).configadded🤖 Generated with Claude Code