Introduce JDBC based persistence for SAML #6241

Osara-B · 2024-12-20T12:35:36Z

Proposed changes in this pull request

Introduce JDBC based persistence for SAML. Related to,

Carbon registry usage removal from SAML product-is#20992

sonarqubecloud · 2024-12-20T12:38:00Z

Quality Gate failed

Failed conditions
6.0% 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

codecov · 2024-12-20T13:48:30Z

Codecov Report

Attention: Patch coverage is 79.61783% with 96 lines in your changes missing coverage. Please review.

Project coverage is 45.87%. Comparing base (44b8576) to head (69291af).
Report is 15 commits behind head on master.

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

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6241      +/-   ##
============================================
+ Coverage     45.66%   45.87%   +0.20%     
- Complexity    14080    14183     +103     
============================================
  Files          1632     1636       +4     
  Lines        100764   101283     +519     
  Branches      17705    17781      +76     
============================================
+ Hits          46016    46462     +446     
- Misses        48036    48058      +22     
- Partials       6712     6763      +51

Flag	Coverage Δ
unit	`28.69% <79.61%> (+0.38%)`	⬆️

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.

darshanasbg · 2024-12-21T05:07:47Z

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

@@ -0,0 +1,193 @@
+/*
+ * Copyright (c) 2023, WSO2 LLC. (http://www.wso2.com).


Fix licenses

darshanasbg · 2024-12-21T05:07:58Z

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

+
+package org.wso2.carbon.identity.core.dao;
+
+public class SAMLSSOServiceProviderConstants {


Add a class comment

darshanasbg · 2024-12-21T05:08:15Z

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

+
+    }
+
+}


Add a new line.

darshanasbg · 2024-12-21T05:09:40Z

...identity.core/src/main/java/org/wso2/carbon/identity/core/model/ServiceProviderProperty.java

+        return this.value;
+    }
+
+}


Add a new line

darshanasbg · 2024-12-21T05:11:00Z

...identity.core/src/main/java/org/wso2/carbon/identity/core/model/ServiceProviderProperty.java

+package org.wso2.carbon.identity.core.model;
+
+/**
+ * This class represents a tuple of key and value.


Can we improve a class comment a bit, indicating how this class related to the SAML

darshanasbg · 2024-12-21T05:11:18Z

...tity.core/src/test/java/org/wso2/carbon/identity/core/SAMLSSOServiceProviderManagerTest.java

+        return StringUtils.isNotBlank(ISSUER_QUALIFIER) ?
+                issuer + IdentityRegistryResources.QUALIFIER_ID + ISSUER_QUALIFIER : issuer;
+    }
+}


Add a new line

darshanasbg · 2024-12-21T05:11:29Z

...carbon.identity.core/src/test/java/org/wso2/carbon/identity/core/constant/TestConstants.java

@@ -0,0 +1,49 @@
+package org.wso2.carbon.identity.core.constant;


Add licenses

darshanasbg · 2024-12-21T05:11:44Z

...carbon.identity.core/src/test/java/org/wso2/carbon/identity/core/constant/TestConstants.java

+    public static final String UPDATED_ISSUER_QUALIFIER = "updatedIssuerQualifier";
+    public static final boolean UPDATED_DO_SINGLE_LOGOUT = false;
+    public static final String[] UPDATED_REQUESTED_RECIPIENTS = {"updatedRecipient1", "updatedRecipient2"};
+}


Add a new line.

darshanasbg · 2024-12-21T05:11:55Z

...rg.wso2.carbon.identity.core/src/test/java/org/wso2/carbon/identity/core/util/TestUtils.java

@@ -0,0 +1,65 @@
+package org.wso2.carbon.identity.core.util;


Add licenses.

darshanasbg · 2024-12-21T05:12:06Z

components/identity-core/org.wso2.carbon.identity.core/src/test/resources/dbscripts/h2.sql

+--SAML--
+CREATE INDEX IDX_SAML2_SP_ISSUER ON IDN_SAML2_SERVICE_PROVIDER (ISSUER, TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);


Add new line

darshanasbg · 2024-12-21T05:12:16Z

features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/db2.sql

+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+/
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);
+/


Add a new line

darshanasbg · 2024-12-21T05:12:29Z

features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/h2.sql

+-- SAML --
+CREATE INDEX IDX_SAML2_SP_ISSUER ON IDN_SAML2_SERVICE_PROVIDER (ISSUER, TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);


Add a new line

darshanasbg · 2024-12-21T05:12:37Z

...res/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sql

@@ -1700,4 +1752,4 @@ GO
 -- Trigger AUTHORIZED_API delete by API_ID on API_RESOURCE deletion by ID --
 CREATE TRIGGER API_RESOURCE_DELETE_TRIGGER ON API_RESOURCE INSTEAD OF DELETE AS BEGIN DELETE FROM AUTHORIZED_API WHERE API_ID IN (SELECT ID FROM DELETED) DELETE FROM API_RESOURCE WHERE ID IN (SELECT ID FROM deleted) END;

-GO
+GO


Add a new line.

darshanasbg · 2024-12-21T05:12:45Z

...tity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql-cluster.sql

+-- SAML --
+CREATE INDEX IDX_SAML2_SP_ISSUER ON IDN_SAML2_SERVICE_PROVIDER (ISSUER, TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);


Add a new line.

darshanasbg · 2024-12-21T05:12:52Z

...res/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql.sql

+--SAML--
+CREATE INDEX IDX_SAML2_SP_ISSUER ON IDN_SAML2_SERVICE_PROVIDER (ISSUER, TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);


Add a new line.

darshanasbg · 2024-12-21T05:12:59Z

...es/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql

+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+/
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);
+/


Add a new line

darshanasbg · 2024-12-21T05:13:07Z

...dentity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle_rac.sql

+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+/
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);
+/


Add a new line.

darshanasbg · 2024-12-21T05:13:16Z

...dentity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/postgresql.sql

+-- SAML --
+CREATE INDEX IDX_SAML2_SP_ISSUER ON IDN_SAML2_SERVICE_PROVIDER (ISSUER, TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_TENANT_ID ON IDN_SAML2_SERVICE_PROVIDER (TENANT_ID);
+CREATE INDEX IDX_SAML2_SP_PROPERTIES ON IDN_SAML2_SP_PROPERTIES (SP_ID);


Add a new line.

darshanasbg · 2024-12-21T05:14:56Z

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

+import static org.wso2.carbon.identity.core.dao.SAMLSSOServiceProviderConstants.SAML2TableColumns.SP_ID;
+
+import static java.time.ZoneOffset.UTC;
+/**


Add a new line.

darshanasbg · 2024-12-21T05:15:14Z

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

+import java.util.Date;
+import java.util.TimeZone;
+
+


Remove new line.

darshanasbg · 2024-12-21T05:17:12Z

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

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


Add a new line.

darshanasbg · 2024-12-21T05:18:10Z

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

-            throws IdentityException {
+    /**

-        Registry registry = getRegistry(tenantId);
+     * Add the service provider information to the registry.
+
+     * @param serviceProviderDO Service provider information object.
+
+     * @return True if addition successful.
+
+     * @throws IdentityException Error while persisting to the registry.
+
+     */
+


Remove the method comment.

darshanasbg · 2024-12-21T05:19:11Z

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

+        registry = getRegistry(tenantId);
+//        Registry registry = getRegistry(tenantId);


registry = getRegistry(tenantId); part should be removed.
Registry registry = getRegistry(tenantId); part should be uncommented.

darshanasbg · 2024-12-21T05:21:23Z

...dentity.core/src/main/java/org/wso2/carbon/identity/core/model/SAMLSSOServiceProviderDO.java

@@ -78,6 +81,8 @@ public class SAMLSSOServiceProviderDO implements Serializable {
    private boolean doFrontChannelLogout;
    private String frontChannelLogoutBinding;

+    private static final String BACKCHANNEL_LOGOUT_BINDING = "BackChannel";


Better to move out this constant to a constants class.

darshanasbg · 2024-12-21T05:31:00Z

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

+/**
+ * Implementation of the SAMLSSOServiceProviderDAO interface for JDBC-based persistence.
+ */
+


Remove the new line.

UdeshAthukorala · 2024-12-21T15:00:21Z

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

+    }
+
+    private void setServiceProviderParameters(NamedPreparedStatement statement,
+                                              SAMLSSOServiceProviderDO serviceProviderDO, int tenantId) throws SQLException {


Let's add a new line here

UdeshAthukorala · 2024-12-21T15:08:43Z

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

+     * @param serviceProviderDO
+     * @param tenant
+     * @return
+     * @throws SQLException
+     * @throws CertificateRetrievingException


Let's add descriptions for parameters and return values

UdeshAthukorala · 2024-12-21T15:09:11Z

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

+    private X509Certificate getApplicationCertificate(SAMLSSOServiceProviderDO serviceProviderDO, Tenant tenant)
+            throws CertificateRetrievingException, DataAccessException {
+
+        // Check whether there is a certificate stored against the service provider (in the database)


Suggested change

// Check whether there is a certificate stored against the service provider (in the database)

// Check whether there is a certificate stored against the service provider (in the database).

UdeshAthukorala · 2024-12-21T15:10:10Z

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

+    /**
+     * Returns the certificate reference ID for the given issuer (Service Provider) if there is one.
+     *
+     * @param issuer
+     * @return
+     * @throws SQLException
+     */


Let's add description for parameters and return statements

UdeshAthukorala · 2024-12-21T15:21:16Z

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

+
+        }
+
+        // IDN_SAML2_SERVICE_PROVIDER table


Suggested change

// IDN_SAML2_SERVICE_PROVIDER table

// IDN_SAML2_SERVICE_PROVIDER table.

UdeshAthukorala · 2024-12-22T08:29:14Z

...carbon.identity.core/src/test/java/org/wso2/carbon/identity/core/constant/TestConstants.java

@@ -0,0 +1,49 @@
+package org.wso2.carbon.identity.core.constant;
+
+public class TestConstants {


Let's add the class comment.

UdeshAthukorala · 2024-12-22T08:29:39Z

...rg.wso2.carbon.identity.core/src/test/java/org/wso2/carbon/identity/core/util/TestUtils.java

+
+import static org.mockito.Mockito.spy;
+
+public class TestUtils {


Let's add the class comment here.

UdeshAthukorala · 2024-12-22T08:29:58Z

...rg.wso2.carbon.identity.core/src/test/java/org/wso2/carbon/identity/core/util/TestUtils.java

+        throw new RuntimeException("No data source initiated for database: " + DB_NAME);
+    }
+
+}


Let's add the missing new line here.

UdeshAthukorala · 2024-12-22T08:31:12Z

...es/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql

+CREATE OR REPLACE TRIGGER IDN_SAML2_SERVICE_PROVIDER_TRIG
+            BEFORE INSERT
+            ON IDN_SAML2_SERVICE_PROVIDER
+            REFERENCING NEW AS NEW
+            FOR EACH ROW
+            BEGIN
+                SELECT IDN_SAML2_SERVICE_PROVIDER_SEQ.nextval INTO :NEW.ID FROM dual;
+            END


Let's fix the indentation to align with other queries.

UdeshAthukorala · 2024-12-22T08:31:19Z

...es/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql

+CREATE OR REPLACE TRIGGER IDN_SAML2_SP_PROPERTIES_TRIG
+            BEFORE INSERT
+            ON IDN_SAML2_SP_PROPERTIES
+            REFERENCING NEW AS NEW
+            FOR EACH ROW
+            BEGIN
+                SELECT IDN_SAML2_SP_PROPERTIES_SEQ.nextval INTO :NEW.ID FROM dual;
+            END


Let's fix the indentation to align with other queries.

Osara-B added 2 commits December 20, 2024 15:15

Introduce SAMLSSOServiceProviderDAO Factory class

7c68916

Introduce JDBC based persistence for SAML

69291af

darshanasbg changed the title ~~Introduce SAMLSSOPersistenceManagerFactory and JDBC based persistence for SAML~~ Introduce JDBC based persistence for SAML Dec 21, 2024

darshanasbg reviewed Dec 21, 2024

View reviewed changes

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

}

}

Copy link

Contributor

darshanasbg Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a new line.

darshanasbg reviewed Dec 21, 2024

View reviewed changes

UdeshAthukorala reviewed Dec 22, 2024

View reviewed changes

		@@ -0,0 +1,193 @@
		/*
		* Copyright (c) 2023, WSO2 LLC. (http://www.wso2.com).


		package org.wso2.carbon.identity.core.dao;

		public class SAMLSSOServiceProviderConstants {

		@@ -0,0 +1,49 @@
		package org.wso2.carbon.identity.core.constant;

		registry = getRegistry(tenantId);
		// Registry registry = getRegistry(tenantId);

	// Check whether there is a certificate stored against the service provider (in the database)
	// Check whether there is a certificate stored against the service provider (in the database).

	// IDN_SAML2_SERVICE_PROVIDER table
	// IDN_SAML2_SERVICE_PROVIDER table.

		@@ -0,0 +1,49 @@
		package org.wso2.carbon.identity.core.constant;

		public class TestConstants {


		import static org.mockito.Mockito.spy;

		public class TestUtils {

Introduce JDBC based persistence for SAML #6241

Are you sure you want to change the base?

Introduce JDBC based persistence for SAML #6241

Conversation

Osara-B commented Dec 20, 2024 • edited by darshanasbg Loading

Proposed changes in this pull request

sonarqubecloud bot commented Dec 20, 2024

Quality Gate failed

codecov bot commented Dec 20, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osara-B commented Dec 20, 2024 •

edited by darshanasbg

Loading