Fix hybrid flow migration issue #2575

Thumimku · 2024-09-22T19:08:55Z

Issue

Proposed fix

Before the fix wso2/product-is#20331, applications don't have hybrid flow enabled properties, hence according to the logic hybrid flow is disabled which causes the Backward Incompatibility.

Now with added new fix, if the property is missing which means the application doesn't have hybridFlowEnabled property(probably an application created before the fix), hence providing the previous behaviour, which is enabling the hybrid flow with all allowed response types.

Debug Log:

[2024-09-23 00:35:00,092] [03c5c187-bac3-44c3-8539-0146f38ac019]  DEBUG {org.wso2.carbon.identity.oauth.dao.OAuthAppDAO} - This application with consumer key 70_x1i8nTwzO7qvf82CNdhZvnrEa doesn't have hybridFlowEnabled property, hence providing the previous behaviour, which is enabling the hybrid flow with all allowed response types.

Unit test for Creation
Unit test for Migrated App

Tested Scenarios

Migrated App
- Try code token, code id_token, code id_token token response types and get expected results.
- Disable Hybrid flow - working as expected.
- Try Hybrid flow after disabling the feature - working as expected.
- Try Hybrid flow after disabling the server config feature .
New App
- Try code token, code id_token, code id_token token response types after configuring a selected response type and get expected results. - only configured response type will get successful response.
- Disable Hybrid flow - working as expected.
- Try Hybrid flow after disabling the feature - working as expected.
Working with Server Configuration for existing apps

We have a sever wide config to enable/disable specific response types. current application configuration logic won't support this server wide config (If some disable a response type in server config, he/she still can configure it via UI). This logic partially fix this issue.

If all hybrid flow response types are disabled, then application hybrid flow configuration also disabled.
If one hybrid flow response types is enabled, then application hybrid flow configuration is enabled and configured response type will be set on application retrieval.
If more than one hybrid flow response types are enabled, then application hybrid flow configuration is enabled and configured response type will be "code id_token token". Currently DAO allows to have one string for configured hybrid flow response types so I went for this option to preserve the backward compatibility. But we need to fix it in seperate effort.

UI

Following is the UI for the migrated app regarding hybrid flow.

UI when the hybrid flow is disabled via server config.

UI when only code id_token is enabled via server config. (code token and code id_token token is disabeld)

jenkins-is-staging · 2024-09-22T19:09:20Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10983691062

codecov · 2024-09-22T19:27:06Z

Codecov Report

Attention: Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.

Project coverage is 37.74%. Comparing base (f49ad8a) to head (c1267b9).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
...rg/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java	41.66%	6 Missing and 1 partial ⚠️
...dentity/oauth/config/OAuthServerConfiguration.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2575      +/-   ##
============================================
- Coverage     37.78%   37.74%   -0.05%     
+ Complexity     5321     5298      -23     
============================================
  Files           584      584              
  Lines         44529    44416     -113     
  Branches       6538     6529       -9     
============================================
- Hits          16827    16763      -64     
+ Misses        25351    25310      -41     
+ Partials       2351     2343       -8

Flag	Coverage Δ
unit	`37.74% <47.05%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jenkins-is-staging · 2024-09-22T20:59:43Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10983691062
Status: success

jenkins-is-staging

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/10983691062

Thumimku · 2024-09-23T07:03:15Z

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 37.79%. Comparing base (f49ad8a) to head (aed7f7f).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...rg/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #2575   +/-   ##
=========================================
  Coverage     37.78%   37.79%           
- Complexity     5321     5322    +1     
=========================================
  Files           584      584           
  Lines         44529    44536    +7     
  Branches       6538     6540    +2     
=========================================
+ Hits          16827    16831    +4     
- Misses        25351    25353    +2     
- Partials       2351     2352    +1     
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

Missing lines are due to dubeg logs, it seems debug log level is not activating have to fix with seperate effort.

jenkins-is-staging · 2024-09-24T06:13:43Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11008003195

jenkins-is-staging · 2024-09-24T08:00:21Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11008003195
Status: failure

jenkins-is-staging · 2024-09-24T10:24:57Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11011711257

jenkins-is-staging · 2024-09-24T12:35:36Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11011711257
Status: success

jenkins-is-staging

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/11011711257

jenkins-is-staging · 2024-10-07T03:43:43Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11208280017

jenkins-is-staging · 2024-10-07T05:40:29Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11208280017
Status: success

jenkins-is-staging

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/11208280017

asha15 · 2024-10-08T04:29:00Z

....carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java

+                {true, "code token", true, "code token"},
+                {false, "code id_token", false, "code id_token"},
+        };
+    }


Add a new line

asha15 · 2024-10-08T04:29:54Z

...wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java

+                    oauthApp.setHybridFlowEnabled(true);
+                    oauthApp.setHybridFlowResponseType(configuredHybridResponseTypes.get(0));
+                    break;
+                default:


Here when the user assigned two hybrid flow response types, it will enable all the hybrid flow responses. IMO it's incorrect, we need to improve it.

If this will be reevaluated with a separate effort, +1 to proceed with the current approach.

asha15 · 2024-10-08T04:33:15Z

....carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java

+                     mockStatic(OAuthComponentServiceHolder.class)) {
+            setupMocksForTest(oAuthServerConfiguration, identityTenantUtil, identityUtil);
+            mockUserstore(oAuthComponentServiceHolder);
+            try (Connection connection = getConnection(DB_NAME)) {


Fix the alignment

asha15 · 2024-10-08T04:35:08Z

....carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java

+                    createDeleteQuery(CONSUMER_KEY, "hybridFlowEnabled");
+            final String deleteHybridFlowResponseTypeProperty =
+                    createDeleteQuery(CONSUMER_KEY, "hybridFlowResponseType");
+


Remove the additional line.

fix hybrid flow migration

aed7f7f

jenkins-is-staging approved these changes Sep 22, 2024

View reviewed changes

shashimalcse previously approved these changes Sep 23, 2024

View reviewed changes

support hybrid flow server config partially

00e0b11

Thumimku dismissed shashimalcse’s stale review via 00e0b11 September 24, 2024 05:54

remove app after testing

c1267b9

jenkins-is-staging approved these changes Sep 24, 2024

View reviewed changes

jenkins-is-staging approved these changes Oct 7, 2024

View reviewed changes

asha15 reviewed Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hybrid flow migration issue #2575

Fix hybrid flow migration issue #2575

Thumimku commented Sep 22, 2024 •

edited

Loading

jenkins-is-staging commented Sep 22, 2024

codecov bot commented Sep 22, 2024 •

edited

Loading

jenkins-is-staging commented Sep 22, 2024

jenkins-is-staging left a comment

Thumimku commented Sep 23, 2024

Codecov Report

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging left a comment

jenkins-is-staging commented Oct 7, 2024

jenkins-is-staging commented Oct 7, 2024

jenkins-is-staging left a comment

asha15 Oct 8, 2024

asha15 Oct 8, 2024 •

edited

Loading

asha15 Oct 8, 2024

asha15 Oct 8, 2024

Fix hybrid flow migration issue #2575

Are you sure you want to change the base?

Fix hybrid flow migration issue #2575

Conversation

Thumimku commented Sep 22, 2024 • edited Loading

Issue

Proposed fix

Tested Scenarios

UI

jenkins-is-staging commented Sep 22, 2024

codecov bot commented Sep 22, 2024 • edited Loading

Codecov Report

jenkins-is-staging commented Sep 22, 2024

jenkins-is-staging left a comment

Choose a reason for hiding this comment

Thumimku commented Sep 23, 2024

Codecov Report

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging commented Sep 24, 2024

jenkins-is-staging left a comment

Choose a reason for hiding this comment

jenkins-is-staging commented Oct 7, 2024

jenkins-is-staging commented Oct 7, 2024

jenkins-is-staging left a comment

Choose a reason for hiding this comment

asha15 Oct 8, 2024

Choose a reason for hiding this comment

asha15 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

asha15 Oct 8, 2024

Choose a reason for hiding this comment

asha15 Oct 8, 2024

Choose a reason for hiding this comment

Thumimku commented Sep 22, 2024 •

edited

Loading

codecov bot commented Sep 22, 2024 •

edited

Loading

asha15 Oct 8, 2024 •

edited

Loading