Modify the core to support removing the necessity of having role_org-application association #5619

DMHP · 2024-04-08T04:12:19Z

Proposed changes in this pull request

Modify the core to support removing the necessity of having role_org-application association

When should this PR be merged

Immediately

CLAassistant · 2024-04-08T04:12:25Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Hasanthi Dissanayake seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

...on/src/main/java/org/wso2/carbon/identity/application/common/model/ApplicationBasicInfo.java

AnuradhaSK · 2024-04-08T04:16:16Z

...on/src/main/java/org/wso2/carbon/identity/application/common/model/ApplicationBasicInfo.java

+     * Get application uuid.
+     * @return uuid application uuid


Suggested change

* Get application uuid.

* @return uuid application uuid

* Get application uuid.

*

* @return uuid application uuid.

Check formatting in other places as well

...mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ApplicationManagementService.java

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

AnuradhaSK · 2024-04-08T04:31:17Z

...src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationMgtDBQueries.java

@@ -78,6 +78,15 @@ public class ApplicationMgtDBQueries {
            "TENANT_ID = ? AND APP_NAME != ? ORDER BY ID DESC";
    public static final String LOAD_APP_NAMES_BY_TENANT_AND_APP_NAME = "SELECT ID, APP_NAME, DESCRIPTION FROM SP_APP " +
            "WHERE TENANT_ID = ? AND APP_NAME != ? AND (%s) ORDER BY ID DESC";
+
+    public static final String LOAD_APP_IDS_BY_SP_PROPERTY_H2 = "SELECT SP_APP.UUID FROM SP_APP  JOIN " +


Suggested change

public static final String LOAD_APP_IDS_BY_SP_PROPERTY_H2 = "SELECT SP_APP.UUID FROM SP_APP JOIN " +

public static final String LOAD_APP_IDS_BY_SP_PROPERTY_H2 = "SELECT SP_APP.UUID FROM SP_APP JOIN " +

AnuradhaSK · 2024-04-08T04:31:31Z

...src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationMgtDBQueries.java

+            "SP_METADATA  ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +
+            "SP_METADATA.`VALUE`=? AND SP_METADATA.TENANT_ID = ?";
+
+    public static final String LOAD_APP_IDS_BY_SP_PROPERTY = "SELECT SP_APP.UUID FROM SP_APP  JOIN " +


Suggested change

public static final String LOAD_APP_IDS_BY_SP_PROPERTY = "SELECT SP_APP.UUID FROM SP_APP JOIN " +

public static final String LOAD_APP_IDS_BY_SP_PROPERTY = "SELECT SP_APP.UUID FROM SP_APP JOIN " +

AnuradhaSK · 2024-04-08T04:45:13Z

...plication.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/ApplicationDAO.java

+     * @param key Name of the sp metadata property key
+     * @param value Value of the sp metadata property
+     * @return ApplicationBasicInfo containing the basic app information
+     * @throws IdentityApplicationManagementException


complete the exception description

AnuradhaSK · 2024-04-08T04:46:31Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

@@ -193,6 +195,8 @@ public class ApplicationDAOImpl extends AbstractApplicationDAOImpl implements Pa

    private static final String SP_PROPERTY_NAME_CERTIFICATE = "CERTIFICATE";
    private static final String APPLICATION_NAME_CONSTRAINT = "APPLICATION_NAME_CONSTRAINT";
+    public static final String AUDIENCE_EQ_ORGANIZATION = "audience eq organization";


Shall we build this filter value by combining existing constants for audience, eq and organization separately

AnuradhaSK · 2024-04-08T04:49:42Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+                List<RoleBasicInfo> listOfAllRoles = roleManagementServiceV2.getRoles(AUDIENCE_EQ_ORGANIZATION,
+                        0, 0, null, null, tenantDomain);
+                List<String> roleIds = listOfAllRoles.stream().map(RoleBasicInfo::getId).collect(Collectors.toList());
+                associatedRolesConfig.setRoles(buildAssociatedRolesWithRoleName(roleIds, tenantDomain));


Are we returning all organization audienced roles as the associated roles, if the app is using organization audienced roles?

AnuradhaSK · 2024-04-08T04:50:13Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+     * @return
+     * @throws IdentityApplicationManagementException


AnuradhaSK · 2024-04-08T04:52:28Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+
+        int tenantID = CarbonContext.getThreadLocalCarbonContext().getTenantId();
+        if (log.isDebugEnabled()) {
+            log.debug("Getting the all applications for the tenant: " + tenantID);


Suggested change

log.debug("Getting the all applications for the tenant: " + tenantID);

log.debug(String.format("Getting the all applications matching to property: %s value equals to %s in the tenant: %d", key, value, tenantID);

AnuradhaSK · 2024-04-08T04:54:11Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+        ResultSet appNameResultSet = null;
+        ArrayList<ApplicationBasicInfo> appInfo = new ArrayList<ApplicationBasicInfo>();
+
+        try {


Shall we use try with resource, making connection and preparedStatement resources which auto closable at the end of usage?

AnuradhaSK · 2024-04-08T05:10:20Z

...src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationMgtDBQueries.java

@@ -78,6 +78,15 @@ public class ApplicationMgtDBQueries {
            "TENANT_ID = ? AND APP_NAME != ? ORDER BY ID DESC";
    public static final String LOAD_APP_NAMES_BY_TENANT_AND_APP_NAME = "SELECT ID, APP_NAME, DESCRIPTION FROM SP_APP " +
            "WHERE TENANT_ID = ? AND APP_NAME != ? AND (%s) ORDER BY ID DESC";
+
+    public static final String LOAD_APP_IDS_BY_SP_PROPERTY_H2 = "SELECT SP_APP.UUID FROM SP_APP  JOIN " +
+            "SP_METADATA  ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +


Suggested change

"SP_METADATA ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +

"SP_METADATA ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +

AnuradhaSK · 2024-04-08T05:10:45Z

...src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationMgtDBQueries.java

+            "SP_METADATA.`VALUE`=? AND SP_METADATA.TENANT_ID = ?";
+
+    public static final String LOAD_APP_IDS_BY_SP_PROPERTY = "SELECT SP_APP.UUID FROM SP_APP  JOIN " +
+            "SP_METADATA  ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +


Suggested change

"SP_METADATA ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +

"SP_METADATA ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +

…g-application association

AnuradhaSK · 2024-04-16T03:48:00Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+            RoleManagementService roleManagementService = holder.getRoleManagementServiceV2();
+            try {
+                if (roleManagementService != null) {
+                    List<RoleBasicInfo> listOfAllRoles = roleManagementService.


Suggested change

List<RoleBasicInfo> listOfAllRoles = roleManagementService.

List<RoleBasicInfo> listOfAllOrgRoles = roleManagementService.

AnuradhaSK · 2024-04-16T03:49:31Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+            try {
+                if (roleManagementService != null) {
+                    List<RoleBasicInfo> listOfAllRoles = roleManagementService.
+                            getRoles(RoleConstants.AUDIENCE + SPACE + RoleConstants.EQ + SPACE +


Is there any max limit defined in role mgt service when request to get all roles matching to a filter?

Seems this is fixed in the same PR. Hence ignore the comment

AnuradhaSK · 2024-04-16T03:52:16Z

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java

+     *
+     * @param key Name of the sp metadata property key
+     * @param value Value of the sp metadata property value
+     * @return


Suggested change

* @return

* @return Filtered applications' basic information.

AnuradhaSK reviewed Apr 8, 2024

View reviewed changes

...on/src/main/java/org/wso2/carbon/identity/application/common/model/ApplicationBasicInfo.java Show resolved Hide resolved

AnuradhaSK reviewed Apr 8, 2024

View reviewed changes

shashimalcse reviewed Apr 8, 2024

View reviewed changes

...mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ApplicationManagementService.java Show resolved Hide resolved

shashimalcse reviewed Apr 8, 2024

View reviewed changes

....mgt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/ApplicationDAOImpl.java Show resolved Hide resolved

AnuradhaSK reviewed Apr 8, 2024

View reviewed changes

DMHP force-pushed the role_org branch 4 times, most recently from a333a8b to 9ae4f20 Compare April 8, 2024 17:32

Modify the core to support removing the necessacity of having role_or…

65bff7e

…g-application association

DMHP force-pushed the role_org branch from 9ae4f20 to 65bff7e Compare April 9, 2024 01:44

DMHP mentioned this pull request Apr 10, 2024

Removing the necessity of associating organisation roles with Applications wso2/product-is#20158

Open

shashimalcse previously approved these changes Apr 11, 2024

View reviewed changes

AnuradhaSK reviewed Apr 16, 2024

View reviewed changes

DMHP dismissed shashimalcse’s stale review via 7f7fb6d April 16, 2024 09:16

Fix review commits

47e71c3

DMHP force-pushed the role_org branch from 7f7fb6d to 47e71c3 Compare April 16, 2024 09:20

AnuradhaSK approved these changes Apr 16, 2024

View reviewed changes

DMHP merged commit 8fd991b into wso2:master Apr 16, 2024
1 of 2 checks passed

DMHP mentioned this pull request Apr 17, 2024

Revert "Modify the core to support removing the necessity of having role_org-application association" #5632

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the core to support removing the necessity of having role_org-application association #5619

Modify the core to support removing the necessity of having role_org-application association #5619

DMHP commented Apr 8, 2024

CLAassistant commented Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

DMHP Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 8, 2024

AnuradhaSK Apr 16, 2024

AnuradhaSK Apr 16, 2024

AnuradhaSK Apr 16, 2024

AnuradhaSK Apr 16, 2024

	public static final String LOAD_APP_IDS_BY_SP_PROPERTY_H2 = "SELECT SP_APP.UUID FROM SP_APP JOIN " +
	public static final String LOAD_APP_IDS_BY_SP_PROPERTY_H2 = "SELECT SP_APP.UUID FROM SP_APP JOIN " +

	log.debug("Getting the all applications for the tenant: " + tenantID);
	log.debug(String.format("Getting the all applications matching to property: %s value equals to %s in the tenant: %d", key, value, tenantID);

	"SP_METADATA ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +
	"SP_METADATA ON SP_APP.ID = SP_METADATA.SP_ID WHERE SP_METADATA.NAME=? and " +

	List<RoleBasicInfo> listOfAllRoles = roleManagementService.
	List<RoleBasicInfo> listOfAllOrgRoles = roleManagementService.

	* @return
	* @return Filtered applications' basic information.

Modify the core to support removing the necessity of having role_org-application association #5619

Modify the core to support removing the necessity of having role_org-application association #5619

Conversation

DMHP commented Apr 8, 2024

Proposed changes in this pull request

When should this PR be merged

CLAassistant commented Apr 8, 2024

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