From 0bba9cfbba1882cc655d6487a729dd2602efb1a9 Mon Sep 17 00:00:00 2001 From: Hasini Samarathunga Date: Thu, 4 Jul 2024 12:04:25 +0530 Subject: [PATCH] Added review suggestions and remove legacy runtime logic --- .../impl/OrganizationManagementDAOImpl.java | 138 ++---------------- .../service/model/FilterQueryBuilder.java | 8 +- .../service/OrganizationManagerImplTest.java | 8 +- .../OrganizationManagementDAOImplTest.java | 47 ++++-- 4 files changed, 54 insertions(+), 147 deletions(-) diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java index cfa48754..9b8257a8 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java @@ -22,7 +22,6 @@ import org.apache.commons.lang.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.wso2.carbon.CarbonConstants; import org.wso2.carbon.database.utils.jdbc.NamedJdbcTemplate; import org.wso2.carbon.database.utils.jdbc.NamedPreparedStatement; import org.wso2.carbon.database.utils.jdbc.exceptions.DataAccessException; @@ -123,7 +122,6 @@ import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_LAST_MODIFIED_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_NAME_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_ORGANIZATION_ATTRIBUTES_TABLE; -import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_ORGANIZATION_PERMISSION; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_PARENT_ID_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_STATUS_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_TENANT_UUID_COLUMN; @@ -147,17 +145,9 @@ import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_CHILD_ORGANIZATION_IDS; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_BY_NAME; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_LEGACY; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_LEGACY; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_MSSQL; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_MSSQL_LEGACY; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_MSSQL_WITHOUT_PERMISSION_CHECK; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_ORACLE; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_ORACLE_LEGACY; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_ORACLE_WITHOUT_PERMISSION_CHECK; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_TAIL_WITHOUT_PERMISSION_CHECK; -import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_WITHOUT_PERMISSION_CHECK; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_WITH_USER_ASSOCIATIONS; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_WITH_USER_ASSOCIATIONS_TAIL; import static org.wso2.carbon.identity.organization.management.service.constant.SQLConstants.GET_ORGANIZATIONS_WITH_USER_ASSOCIATIONS_TAIL_MSSQL; @@ -742,19 +732,6 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b String sqlStmt = prepareGetOrganizationQuery(authorizedSubOrgsOnly, recursive, sortOrder, applicationAudience, filterQueryBuilder, parentIdFilterQueryBuilder, userID); - List permissions; - String permissionPlaceholder; - if (Boolean.TRUE.equals(CarbonConstants.ENABLE_LEGACY_AUTHZ_RUNTIME)) { - permissionPlaceholder = "PERMISSION_"; - permissions = getAllowedPermissions(VIEW_ORGANIZATION_PERMISSION); - if (authorizedSubOrgsOnly) { - sqlStmt = replacePermissionPlaceholders(sqlStmt, permissions, permissionPlaceholder); - } - } else { - permissionPlaceholder = ""; - permissions = new ArrayList<>(); - } - if (filterQueryBuilder.getHasSubAttribute()) { sqlStmt = String.format(GET_ALL_UM_ORG_ATTRIBUTES, sqlStmt); } @@ -762,7 +739,7 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b Map organizationMap = new HashMap<>(); NamedJdbcTemplate namedJdbcTemplate = Utils.getNewTemplate(); try { - namedJdbcTemplate.executeQuery(sqlStmt, + organizations = namedJdbcTemplate.executeQuery(sqlStmt, (resultSet, rowNumber) -> { if (filterQueryBuilder.getHasSubAttribute()) { String orgId = resultSet.getString(1); @@ -776,19 +753,17 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b organizationAttribute.setValue(resultSet.getString(6)); organization.setAttribute(organizationAttribute); return organization; - } else { - Organization organization = buildOrganization(resultSet); - organizationMap.put(organization.getId(), organization); - return organization; } + return buildOrganization(resultSet); }, namedPreparedStatement -> setPreparedStatementParams(namedPreparedStatement, organizationId, - applicationAudience, limit, filterQueryBuilder, parentIdFilterQueryBuilder, permissions, - permissionPlaceholder, userID)); - organizations = new ArrayList<>(organizationMap.values()); + applicationAudience, limit, filterQueryBuilder, parentIdFilterQueryBuilder, userID)); } catch (DataAccessException e) { throw handleServerException(ERROR_CODE_ERROR_RETRIEVING_ORGANIZATIONS, e); } + if (filterQueryBuilder.getHasSubAttribute()) { + return new ArrayList<>(organizationMap.values()); + } return organizations; } @@ -816,19 +791,6 @@ private List getOrganizationsBasicInfo(boolean authorizedSubO String sqlStmt = prepareGetOrganizationQuery(authorizedSubOrgsOnly, recursive, sortOrder, applicationAudience, filterQueryBuilder, parentIdFilterQueryBuilder, userID); - List permissions; - String permissionPlaceholder; - if (Boolean.TRUE.equals(CarbonConstants.ENABLE_LEGACY_AUTHZ_RUNTIME)) { - permissionPlaceholder = "PERMISSION_"; - permissions = getAllowedPermissions(VIEW_ORGANIZATION_PERMISSION); - if (authorizedSubOrgsOnly) { - sqlStmt = replacePermissionPlaceholders(sqlStmt, permissions, permissionPlaceholder); - } - } else { - permissionPlaceholder = ""; - permissions = new ArrayList<>(); - } - List organizations; NamedJdbcTemplate namedJdbcTemplate = Utils.getNewTemplate(); try { @@ -842,8 +804,7 @@ private List getOrganizationsBasicInfo(boolean authorizedSubO return organization; }, namedPreparedStatement -> setPreparedStatementParams(namedPreparedStatement, organizationId, - applicationAudience, limit, filterQueryBuilder, parentIdFilterQueryBuilder, permissions, - permissionPlaceholder, userID)); + applicationAudience, limit, filterQueryBuilder, parentIdFilterQueryBuilder, userID)); } catch (DataAccessException e) { throw handleServerException(ERROR_CODE_ERROR_RETRIEVING_ORGANIZATIONS, e); } @@ -1546,7 +1507,7 @@ private String prepareGetOrganizationQuery(boolean authorizedSubOrgsOnly, boolea FilterQueryBuilder parentIdFilterQueryBuilder, String userID) throws OrganizationManagementServerException { - String sqlStmt = prepareSqlStatement(authorizedSubOrgsOnly, applicationAudience); + String sqlStmt = getOrgSqlStatement(authorizedSubOrgsOnly, applicationAudience); String getOrgSqlStmtTail = getOrgSqlStmtTail(authorizedSubOrgsOnly, applicationAudience); if (filterQueryBuilder.getHasSubAttribute()) { sqlStmt = sqlStmt.replace("WHERE", INNER_JOIN_UM_ORG_ATTRIBUTE); @@ -1565,66 +1526,20 @@ private String prepareGetOrganizationQuery(boolean authorizedSubOrgsOnly, boolea .map(name -> "'" + name + "'").collect(Collectors.joining(","))); } - private String prepareSqlStatement(boolean authorizedSubOrgsOnly, String applicationAudience) { - - if (Boolean.FALSE.equals(CarbonConstants.ENABLE_LEGACY_AUTHZ_RUNTIME)) { - return prepareSqlStatementForModernRuntime(authorizedSubOrgsOnly, applicationAudience); - } else { - return prepareSqlStatementForLegacyRuntime(authorizedSubOrgsOnly); - } - } - - private String prepareSqlStatementForModernRuntime(boolean authorizedSubOrgsOnly, String applicationAudience) { + private String getOrgSqlStatement(boolean authorizedSubOrgsOnly, String applicationAudience) { if (authorizedSubOrgsOnly) { if (StringUtils.isNotBlank(applicationAudience)) { return GET_ORGANIZATIONS_WITH_USER_ROLE_ASSOCIATIONS; - } else { - return GET_ORGANIZATIONS_WITH_USER_ASSOCIATIONS; } - } else { - return GET_ORGANIZATIONS; - } - } - - private String prepareSqlStatementForLegacyRuntime(boolean authorizedSubOrgsOnly) { - - if (authorizedSubOrgsOnly) { - return GET_ORGANIZATIONS_LEGACY; - } else { - return GET_ORGANIZATIONS_WITHOUT_PERMISSION_CHECK; + return GET_ORGANIZATIONS_WITH_USER_ASSOCIATIONS; } + return GET_ORGANIZATIONS; } private String getOrgSqlStmtTail(boolean authorizedSubOrgsOnly, String applicationAudience) throws OrganizationManagementServerException { - if (Boolean.FALSE.equals(CarbonConstants.ENABLE_LEGACY_AUTHZ_RUNTIME)) { - return getOrgSqlStmtTailForModernRuntime(authorizedSubOrgsOnly, applicationAudience); - } else { - return getOrgSqlStmtTailForLegacyRuntime(authorizedSubOrgsOnly); - } - } - - private String getOrgSqlStmtTailForLegacyRuntime(boolean authorizedSubOrgsOnly) - throws OrganizationManagementServerException { - - String getOrgSqlStmtTail = authorizedSubOrgsOnly ? GET_ORGANIZATIONS_TAIL_LEGACY - : GET_ORGANIZATIONS_TAIL_WITHOUT_PERMISSION_CHECK; - - if (isOracleDB()) { - getOrgSqlStmtTail = authorizedSubOrgsOnly ? GET_ORGANIZATIONS_TAIL_ORACLE_LEGACY - : GET_ORGANIZATIONS_TAIL_ORACLE_WITHOUT_PERMISSION_CHECK; - } else if (isMSSqlDB()) { - getOrgSqlStmtTail = authorizedSubOrgsOnly ? GET_ORGANIZATIONS_TAIL_MSSQL_LEGACY - : GET_ORGANIZATIONS_TAIL_MSSQL_WITHOUT_PERMISSION_CHECK; - } - return getOrgSqlStmtTail; - } - - private String getOrgSqlStmtTailForModernRuntime(boolean authorizedSubOrgsOnly, String applicationAudience) - throws OrganizationManagementServerException { - String getOrgSqlStmtTail = authorizedSubOrgsOnly ? StringUtils.isNotBlank(applicationAudience) ? GET_ORGANIZATIONS_WITH_USER_ROLE_ASSOCIATIONS_TAIL @@ -1660,24 +1575,11 @@ private String appendFilterQueries(String sqlStmt, FilterQueryBuilder filterQuer String.format(getOrgSqlStmtTail, parentIdFilterQuery, recursive ? "> 0" : "= 1", sortOrder); } - private String replacePermissionPlaceholders(String sqlStmt, List permissions, - String permissionPlaceholder) { - - List permissionPlaceholders = new ArrayList<>(); - - // Constructing the placeholders required to hold the permission strings in the named prepared statement. - for (int i = 1; i <= permissions.size(); i++) { - permissionPlaceholders.add(":" + permissionPlaceholder + i + ";"); - } - String placeholder = String.join(", ", permissionPlaceholders); - return sqlStmt.replace(PERMISSION_LIST_PLACEHOLDER, placeholder); - } - private void setPreparedStatementParams(NamedPreparedStatement namedPreparedStatement, String organizationId, String applicationAudience, Integer limit, FilterQueryBuilder filterQueryBuilder, - FilterQueryBuilder parentIdFilterQueryBuilder, List permissions, - String permissionPlaceholder, String userID) throws SQLException { + FilterQueryBuilder parentIdFilterQueryBuilder, String userID) + throws SQLException { namedPreparedStatement.setString(DB_SCHEMA_COLUMN_NAME_USER_ID, userID); namedPreparedStatement.setString(DB_SCHEMA_COLUMN_NAME_USER_DOMAIN, @@ -1690,10 +1592,6 @@ private void setPreparedStatementParams(NamedPreparedStatement namedPreparedStat } setFilterAttributes(namedPreparedStatement, parentIdFilterQueryBuilder.getFilterAttributeValue(), filterQueryBuilder.getFilterAttributeValue(), filterQueryBuilder.getTimestampFilterAttributes()); - - if (Boolean.TRUE.equals(CarbonConstants.ENABLE_LEGACY_AUTHZ_RUNTIME)) { - setPermissions(namedPreparedStatement, permissions, permissionPlaceholder); - } namedPreparedStatement.setInt(DB_SCHEMA_LIMIT, limit); } @@ -1714,16 +1612,6 @@ private void setFilterAttributes(NamedPreparedStatement namedPreparedStatement, } } - private void setPermissions(NamedPreparedStatement namedPreparedStatement, List permissions, - String permissionPlaceholder) throws SQLException { - - int index = 1; - for (String permission : permissions) { - namedPreparedStatement.setString(permissionPlaceholder + index, permission); - index++; - } - } - private String handleViewAttrKeyColumn(ExpressionNode expressionNode, FilterQueryBuilder filterQueryBuilder) { filterQueryBuilder.setHasSubAttribute(true); diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java index d67e647e..6460d74e 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java @@ -97,9 +97,9 @@ public String getFilterQuery() { } /** - * Get the value of containSubAttribute. + * Get the value of hasSubAttribute. * - * @return boolean value indicating if the filter contains subattributes. + * @return boolean value indicating if the filter contains a sub attribute. */ public boolean getHasSubAttribute() { @@ -107,9 +107,9 @@ public boolean getHasSubAttribute() { } /** - * Set the value of containSubAttribute. + * Set the value of hasSubAttribute. * - * @param hasSubAttribute boolean value indicating if the filter contains subattributes. + * @param hasSubAttribute boolean value indicating if the filter contains a sub attributes. */ public void setHasSubAttribute(boolean hasSubAttribute) { diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java index 64393a2a..52033c87 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java @@ -383,8 +383,8 @@ public void testGetOrganizationWithChildren() throws Exception { assertEquals(organization.getChildOrganizations().size(), 1); } - @DataProvider(name = "metaAttributeFilters") - public Object[][] metaAttributeFilters() { + @DataProvider(name = "dataForFilterOrganizationsByMetaAttributes") + public Object[][] dataForFilterOrganizationsByMetaAttributes() { return new Object[][]{ {"attributes.country co S", false}, @@ -393,8 +393,8 @@ public Object[][] metaAttributeFilters() { }; } - @Test(dataProvider = "metaAttributeFilters") - public void testGetOrganizationsWithMetaFilterAttribute(String filter, boolean isEmptyList) throws Exception { + @Test(dataProvider = "dataForFilterOrganizationsByMetaAttributes") + public void testFilterOrganizationsByMetaAttributes(String filter, boolean isEmptyList) throws Exception { TestUtils.mockCarbonContext(SUPER_ORG_ID); List organizations = organizationManager.getOrganizationsList(10, null, null, diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java index baded073..0d5ade02 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java @@ -145,8 +145,7 @@ public void testIsOrganizationExistById(String id) throws Exception { @Test public void testGetOrganizationIdByName() throws Exception { - String organizationId = organizationManagementDAO.getOrganizationIdByName( - ORG_NAME); + String organizationId = organizationManagementDAO.getOrganizationIdByName(ORG_NAME); Assert.assertEquals(organizationId, orgId); } @@ -158,17 +157,18 @@ public void testGetOrganization() throws Exception { Assert.assertEquals(organization.getParent().getId(), SUPER_ORG_ID); } - @DataProvider(name = "dataForGetOrganizationsList") - public Object[][] dataForGetOrganizationsList() { + @DataProvider(name = "dataForFilterOrganizationsByPrimaryAttributes") + public Object[][] dataForFilterOrganizationsByPrimaryAttributes() { return new Object[][]{ - {ORGANIZATION_ATTRIBUTES_FIELD_PREFIX + ATTRIBUTE_KEY, "eq", ATTRIBUTE_VALUE}, - {"name", "co", "XYZ"} + {"name", "co", "XYZ"}, + {"id", "eq", orgId}, + {"description", "sw", "This"} }; } - @Test(dataProvider = "dataForGetOrganizationsList") - public void testGetOrganizationsList(String attributeValue, String operation, String value) + @Test(dataProvider = "dataForFilterOrganizationsByPrimaryAttributes") + public void testFilterOrganizationsByPrimaryAttributes(String attributeValue, String operation, String value) throws OrganizationManagementServerException { TestUtils.mockCarbonContext(SUPER_ORG_ID); @@ -179,13 +179,32 @@ public void testGetOrganizationsList(String attributeValue, String operation, St SUPER_ORG_ID, "DESC", expressionNodes, new ArrayList<>()); Assert.assertEquals(organizations.get(0).getName(), ORG_NAME); + Assert.assertTrue(organizations.get(0).getAttributes().isEmpty()); + } - if (attributeValue.startsWith(ORGANIZATION_ATTRIBUTES_FIELD_PREFIX)) { - Assert.assertEquals(organizations.get(0).getAttributes().get(0).getKey(), ATTRIBUTE_KEY); - Assert.assertEquals(organizations.get(0).getAttributes().get(0).getValue(), ATTRIBUTE_VALUE); - } else { - Assert.assertTrue(organizations.get(0).getAttributes().isEmpty()); - } + @DataProvider(name = "dataForFilterOrganizationsByMetaAttributes") + public Object[][] dataForFilterOrganizationsByMetaAttributes() { + + return new Object[][]{ + {ORGANIZATION_ATTRIBUTES_FIELD_PREFIX + ATTRIBUTE_KEY, "eq", ATTRIBUTE_VALUE}, + {ORGANIZATION_ATTRIBUTES_FIELD_PREFIX + ATTRIBUTE_KEY, "co", "L"} + }; + } + + @Test(dataProvider = "dataForFilterOrganizationsByMetaAttributes") + public void testFilterOrganizationsByMetaAttributes(String attributeValue, String operation, String value) + throws OrganizationManagementServerException { + + TestUtils.mockCarbonContext(SUPER_ORG_ID); + ExpressionNode expressionNode = getExpressionNode(attributeValue, operation, value); + List expressionNodes = new ArrayList<>(); + expressionNodes.add(expressionNode); + List organizations = organizationManagementDAO.getOrganizationsList(false, 10, + SUPER_ORG_ID, "DESC", expressionNodes, new ArrayList<>()); + + Assert.assertEquals(organizations.get(0).getName(), ORG_NAME); + Assert.assertEquals(organizations.get(0).getAttributes().get(0).getKey(), ATTRIBUTE_KEY); + Assert.assertEquals(organizations.get(0).getAttributes().get(0).getValue(), ATTRIBUTE_VALUE); } @DataProvider(name = "dataForHasChildOrganizations")