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.

Sign up for GitHub

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

Jump to bottom

Feature remove registry SAML #6197

Draft

Osara-B wants to merge 16 commits into wso2:master from Osara-B:fix-codecov-coverage

Contributor

Osara-B commented Dec 11, 2024

Proposed changes in this pull request

This is raised early in the development to validate incremental improvements done on the feature branch to test against the product-is integration tests using the pr builder action

When should this PR be merged

This is a draft PR. Do not merge this.

codecov bot commented Dec 11, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 72.79822% with 244 lines in your changes missing coverage. Please review.

Project coverage is 45.90%. Comparing base (b492a3d) to head (c90315f).
Report is 25 commits behind head on master.

Files with missing lines	Patch %	Lines
...ore/dao/RegistrySAMLSSOServiceProviderDAOImpl.java	65.97%	103 Missing and 44 partials ⚠️
...ty/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java	86.47%	29 Missing and 14 partials ⚠️
.../identity/core/model/SAMLSSOServiceProviderDO.java	61.53%	3 Missing and 32 partials ⚠️
...y/core/persistence/IdentityPersistenceManager.java	0.00%	18 Missing ⚠️
...ity/core/dao/SAMLSSOPersistenceManagerFactory.java	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6197      +/-   ##
============================================
+ Coverage     45.66%   45.90%   +0.23%     
- Complexity    14040    14352     +312     
============================================
  Files          1632     1640       +8     
  Lines        100532   102823    +2291     
  Branches      17421    16885     -536     
============================================
+ Hits          45911    47198    +1287     
- Misses        47931    48815     +884     
- Partials       6690     6810     +120

Flag	Coverage Δ
unit	`28.61% <72.79%> (+0.30%)`	⬆️

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.

Osara-B force-pushed the fix-codecov-coverage branch from 9833bf2 to 33847a3 Compare

December 17, 2024 10:13

darshanasbg reviewed

View reviewed changes

...ty.core/src/main/java/org/wso2/carbon/identity/core/dao/SAMLSSOServiceProviderConstants.java Outdated Show resolved Hide resolved

darshanasbg reviewed

View reviewed changes

...ty.core/src/main/java/org/wso2/carbon/identity/core/dao/SAMLSSOServiceProviderConstants.java Show resolved Hide resolved

jenkins-is-staging commented Dec 19, 2024

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

jenkins-is-staging commented Dec 19, 2024

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

darshanasbg reviewed

View reviewed changes

....core/src/main/java/org/wso2/carbon/identity/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java

+              import static org.wso2.carbon.identity.core.dao.SAMLSSOServiceProviderConstants.SAML2TableColumns.PROPERTY_VALUE;
+              import static org.wso2.carbon.identity.core.dao.SAMLSSOServiceProviderConstants.SAML2TableColumns.SP_ID;
+              public class JDBCSAMLSSOServiceProviderDAOImpl implements SAMLSSOServiceProviderDAO {

Contributor

darshanasbg Dec 19, 2024

Lets add a class comment.

darshanasbg reviewed

View reviewed changes

....core/src/main/java/org/wso2/carbon/identity/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java Outdated


		public class JDBCSAMLSSOServiceProviderDAOImpl implements SAMLSSOServiceProviderDAO {

		private static final Log log = LogFactory.getLog(JDBCSAMLSSOServiceProviderDAOImpl.class);

Contributor

darshanasbg Dec 19, 2024

Suggested change

      
                private static final Log log = LogFactory.getLog(JDBCSAMLSSOServiceProviderDAOImpl.class);
          
                private static final Log LOG = LogFactory.getLog(JDBCSAMLSSOServiceProviderDAOImpl.class);

darshanasbg reviewed

View reviewed changes

....core/src/main/java/org/wso2/carbon/identity/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java Outdated

+              public class JDBCSAMLSSOServiceProviderDAOImpl implements SAMLSSOServiceProviderDAO {
+                  private static final Log log = LogFactory.getLog(JDBCSAMLSSOServiceProviderDAOImpl.class);
+                  private int tenantId;

Contributor

darshanasbg Dec 19, 2024

Do we need this instance variable? Having a instance variable for tenantId is could lead to lot of issues when same object are reused across many tenants.

darshanasbg reviewed

View reviewed changes

....core/src/main/java/org/wso2/carbon/identity/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java

Comment on lines +85 to +104

+                  private static final String QUERY_TO_GET_APPLICATION_CERTIFICATE_ID = "SELECT " +
+                          "META.VALUE FROM SP_INBOUND_AUTH INBOUND, SP_APP SP, SP_METADATA META WHERE SP.ID = INBOUND.APP_ID AND " +
+                          "SP.ID = META.SP_ID AND META.NAME = ? AND INBOUND.INBOUND_AUTH_KEY = ? AND META.TENANT_ID = ?";
+                  private static final String QUERY_TO_GET_APPLICATION_CERTIFICATE_ID_H2 = "SELECT " +
+                          "META.`VALUE` FROM SP_INBOUND_AUTH INBOUND, SP_APP SP, SP_METADATA META WHERE SP.ID = INBOUND.APP_ID AND " +
+                          "SP.ID = META.SP_ID AND META.NAME = ? AND INBOUND.INBOUND_AUTH_KEY = ? AND META.TENANT_ID = ?";

Contributor

darshanasbg Dec 19, 2024

Shouldn't access the SP_ tables directly


          Create interface SAMLSSOServiceProviderDAORegistryImpl

bd8feb5

darshanasbg reviewed

View reviewed changes

....core/src/main/java/org/wso2/carbon/identity/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java Outdated

+                      return certificateId != null ? certificateId : -1;
+                  }
+              }

Contributor

darshanasbg Dec 19, 2024

Add a new line

darshanasbg reviewed

View reviewed changes

...core/src/main/java/org/wso2/carbon/identity/core/persistence/IdentityPersistenceManager.java Outdated

-                      SAMLSSOServiceProviderDAO serviceProviderDAO = new SAMLSSOServiceProviderDAO(registry);
-                      return serviceProviderDAO.addServiceProvider(serviceProviderDO);
+                      int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
+                      SAMLSSOServiceProviderDAO serviceProviderDAO = new RegistrySAMLSSOServiceProviderDAOImpl();

Contributor

darshanasbg Dec 19, 2024

Lets change this to use Factory.

hwupathum and others added 9 commits

December 19, 2024 14:57


          Create database implementation of SAML configs

a918b3f


          Update SAMLSSOServiceProviderDAOImpl and SAMLSSOServiceProviderConsta…

da638f4

…nts to use named parameters


          Add ability to switch SAML storage type via config

2520be6


          Update code to use new database schema

81cbc56


          Refactor Code

319fff6


          Update code to use Named JDBC Templates

49e0179


          Add SIGN_ASSERTIONS property to IDN_SAML2_SERVICE_PROVIDER table

1b24d81


          Add unit tests for SAMLSSOServiceProviderManager JDBC Implementation

c67895f


          Update uploadServiceProvider in SAMLSSOServiceProviderDAOImpl.java

dc1635e

darshanasbg reviewed

View reviewed changes

...e/src/main/java/org/wso2/carbon/identity/core/dao/RegistrySAMLSSOServiceProviderDAOImpl.java Outdated

+                          "META.`VALUE` FROM SP_INBOUND_AUTH INBOUND, SP_APP SP, SP_METADATA META WHERE SP.ID = INBOUND.APP_ID AND " +
+                          "SP.ID = META.SP_ID AND META.NAME = ? AND INBOUND.INBOUND_AUTH_KEY = ? AND META.TENANT_ID = ?";
+                  private static Log log = LogFactory.getLog(SAMLSSOServiceProviderDAO.class);

Contributor

darshanasbg Dec 19, 2024

Suggested change

      
                private static Log log = LogFactory.getLog(SAMLSSOServiceProviderDAO.class);
          
                private static Log log = LogFactory.getLog(RegistrySAMLSSOServiceProviderDAOImpl.class);

darshanasbg reviewed

View reviewed changes

...y.core/src/main/java/org/wso2/carbon/identity/core/dao/SAMLSSOPersistenceManagerFactory.java Show resolved Hide resolved

darshanasbg reviewed

View reviewed changes

...y.core/src/main/java/org/wso2/carbon/identity/core/dao/SAMLSSOPersistenceManagerFactory.java Outdated

+                  private static final String HYBRID = "hybrid";
+                  private static final String REGISTRY = "registry";
+                  public SAMLSSOServiceProviderDAO buildSSOServiceProviderManager(){

Contributor

darshanasbg Dec 19, 2024

Fix formatting

Osara-B added 4 commits

December 19, 2024 15:14


          Add IDN_SAML2_SERVICE_PROVIDER and IDN_SAML2_SP_PROPERTIES

60ee2d3

tables to dbscripts


          Address review comments

59aab15


          Add SAMLSSOPersistenceManagerFactory class and refactor code

95929a8


          Add tests for SAMLSSOPersistenceManagerFactory

b2e7bdf

Osara-B force-pushed the fix-codecov-coverage branch from 6d89915 to 1d8ffdd Compare

December 19, 2024 09:47

darshanasbg reviewed

View reviewed changes

...y.core/src/main/java/org/wso2/carbon/identity/core/dao/SAMLSSOPersistenceManagerFactory.java Outdated

+                                  break;
+                              case REGISTRY:
+                                  samlSSOServiceProviderDAO = new RegistrySAMLSSOServiceProviderDAOImpl();
+                                  LOG.info("Registry based SAML storage initialized.");

Contributor

darshanasbg Dec 19, 2024

Lets change this to warn log

darshanasbg reviewed

View reviewed changes

...y.core/src/main/java/org/wso2/carbon/identity/core/dao/SAMLSSOPersistenceManagerFactory.java Show resolved Hide resolved

Osara-B force-pushed the fix-codecov-coverage branch 2 times, most recently from 8313f3f to 914a7de Compare

December 19, 2024 16:14

Osara-B added 2 commits

December 19, 2024 22:11


          Add VERSION, CREATED_AT, UPDATED_AT columns for IDN_SAML2_SERVICE_PRO…

c3c0587

…VIDER table


          Address review comments

4b9f7e5

Osara-B force-pushed the fix-codecov-coverage branch from 914a7de to 4b9f7e5 Compare

December 19, 2024 16:41

sonarqubecloud bot commented Dec 19, 2024

Quality Gate failed

Failed conditions
5.8% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet