-
Notifications
You must be signed in to change notification settings - Fork 560
Add proper status for failed nodes. #6672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7c4c043
to
45fc74d
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6672 +/- ##
============================================
+ Coverage 47.37% 47.85% +0.48%
- Complexity 15890 16150 +260
============================================
Files 1820 1823 +3
Lines 109188 110133 +945
Branches 20426 20395 -31
============================================
+ Hits 51726 52708 +982
+ Misses 50293 50178 -115
- Partials 7169 7247 +78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Observation on Basic Auth failure response: Without the fix:
With the fix:
This constitutes a breaking change, especially for clients or consumers that relied on the previous 200 OK response pattern. Any logic keyed on status code or "flowStatus" will likely need updating to align with the new error structure. |
However, in the standard (UI-based) login flow, we maintain the current behavior (status code will be |
Since this change only impacts users on version 7.0.0 and above, specifically when using the fail() function in adaptive scripts with app-native authentication, it has been agreed upon by the stakeholders to make a configuration available on demand and proceed with the proposed fix without introducing a config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that when authentication fails, the flow status is explicitly marked as FAIL_COMPLETED
on the request.
- Sets the
FLOW_STATUS
attribute toFAIL_COMPLETED
in thehandleAuthFail
method. - Corrects the flow status handling for failed nodes.
Comments suppressed due to low confidence (2)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/sequence/impl/GraphBasedSequenceHandler.java:458
- Add or update unit/integration tests to verify that the
FLOW_STATUS
attribute is set toFAIL_COMPLETED
on the request when authentication fails.
request.setAttribute(FrameworkConstants.RequestParams.FLOW_STATUS, AuthenticatorFlowStatus.FAIL_COMPLETED);
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/sequence/impl/GraphBasedSequenceHandler.java:458
- [nitpick] Consider extracting status updates into a shared helper or constant-driven utility method to ensure consistency across different failure handling paths.
request.setAttribute(FrameworkConstants.RequestParams.FLOW_STATUS, AuthenticatorFlowStatus.FAIL_COMPLETED);
PR builder started |
PR builder completed |
PR builder started |
PR builder completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/15754568261
Proposed changes in this pull request
Fix flow status in the fail() function
Issue