Get discoverable applciations from root and sub organizations #5934

asha15 · 2024-09-12T07:59:05Z

Proposed changes in this pull request

currently we're retrieving the discoverable application only from the reverent tenant. However when it comes to sub organization the application should be fetched against root org and sub organization.

Tested with H2 DB.

When should this PR be merged

[Please describe any preconditions that need to be addressed before we
can merge this pull request.]

Follow up actions

[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

codecov · 2024-09-12T08:21:10Z

Codecov Report

Attention: Patch coverage is 0% with 120 lines in your changes missing coverage. Please review.

Project coverage is 21.05%. Comparing base (647776b) to head (c0fd487).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...y/application/mgt/dao/impl/ApplicationDAOImpl.java	0.00%	99 Missing ⚠️
...ernal/impl/DiscoverableApplicationManagerImpl.java	0.00%	14 Missing ⚠️
...n/identity/application/mgt/ApplicationMgtUtil.java	0.00%	3 Missing ⚠️
...n/identity/application/mgt/dao/ApplicationDAO.java	0.00%	2 Missing ⚠️
...cation/mgt/dao/impl/CacheBackedApplicationDAO.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5934      +/-   ##
============================================
- Coverage     21.07%   21.05%   -0.02%     
  Complexity     5626     5626              
============================================
  Files          1561     1561              
  Lines         99201    99298      +97     
  Branches      15188    15201      +13     
============================================
+ Hits          20904    20905       +1     
- Misses        75276    75374      +98     
+ Partials       3021     3019       -2

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

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

AnuradhaSK · 2024-09-15T02:11:42Z

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

+ *
+ * @param tenantDomain
+ * @return true if the tenant is a sub org
+ * @throws OrganizationManagementException


Complete the exception description.

AnuradhaSK · 2024-09-15T02:13:33Z

...g/wso2/carbon/identity/application/mgt/internal/impl/DiscoverableApplicationManagerImpl.java

- return appDAO.getDiscoverableApplicationBasicInfo(limit, offset, filter, sortOrder, sortBy, tenantDomain);
+ try {
+ ApplicationDAO appDAO = ApplicationMgtSystemConfig.getInstance().getApplicationDAO();
+ if (ApplicationMgtUtil.isSubOrg(tenantDomain)) {


Wny requirement to wrap this OrganizationManagementUtil.isOrganization(tenantDomain) with using an another util function?
Since the OrganizationManagementException is also handled here, we can directly use OrganizationManagementUtil.isOrganization(tenantDomain)

AnuradhaSK · 2024-09-15T02:15:01Z

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

+ * @return parent organization id
+ * @throws OrganizationManagementServerException
+ */
+ public static String getParentOrgId(String tenantDomain)


Change the method name and comments as getPrimaryOrgId / getRootOrgId.
Parent org means the immediate org in up, but primary means the root org the tree.

AnuradhaSK · 2024-09-15T02:15:53Z

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

+ default List<ApplicationBasicInfo> getDiscoverableAppsInfoFromRootAndSubOrg(int limit, int offset, String filter,
+ String sortOrder, String sortBy, String
+ tenantDomain, String primaryOrgID)


is this correctly formatted?

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

AnuradhaSK · 2024-09-15T02:16:48Z

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

+ * @param tenantDomain Tenant domain
+ * @param primaryOrgId Primary organization ID
+ * @return Count of discoverable applications.
+ * @throws IdentityApplicationManagementException


complete the exception description

...g/wso2/carbon/identity/application/mgt/internal/impl/DiscoverableApplicationManagerImpl.java

AnuradhaSK · 2024-09-15T02:35:56Z

...ication.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ApplicationConstants.java

+ public static class ApplicationSPSharedTableColumns {
+
+ public static final String SHARED_IDP_ID = "SHARED_IDP_ID";
+ public static final String OWNER_ORG_ID = "OWNER_ORG_ID";
+
+ private ApplicationSPSharedTableColumns() {
+
+ }


since SP_SHARED_APP table related queries are not managed in framework, it is not recommended to redefine column names of SP_SHARED_APP here.
Hope you can't even use import the constants from other repo due to cyclic dependencies.

Can't we take an approach like this.

Implement another manager of https://github.com/wso2/carbon-identity-framework/blob/master/components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/DiscoverableApplicationManager.java (name suggestion: sharedAppDiscoverableManager) in organizationApplication

From the places where it invokes DiscoverableApplicationManager methods, pick the correct implementation. Evaluate whether we can do this without any cyclic dependency

Couldn't we define the constants in the organization-mgt-core repo and use that instead of moving the logic to the organization management repo?. Was this comment or any other concern lead to move the logic. @asha15

I restructured the code in a way we don't need this constant anymore. However having SP_SHARED_APP related queries in framework repo will give maintainability issues. that's why we moved it to organization management repo.

…n.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ApplicationMgtUtil.java Co-authored-by: Anuradha Karunarathna <anuradha199528@gmail.com>

Co-authored-by: Anuradha Karunarathna <anuradha199528@gmail.com>

…n.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/internal/impl/DiscoverableApplicationManagerImpl.java Co-authored-by: Anuradha Karunarathna <anuradha199528@gmail.com>

Co-authored-by: Anuradha Karunarathna <anuradha199528@gmail.com>

SujanSanjula96 · 2024-09-16T03:01:25Z

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

@@ -273,6 +273,19 @@ default List<ApplicationBasicInfo> getDiscoverableApplicationBasicInfo(int limit
 return null;
 }

+ /**
+ * Get discoverable applications from root and sub org.
+ *


params are missing in the java doc comment

SujanSanjula96 · 2024-09-16T03:12:06Z

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

+ getDBQueryForDiscoverableAppsByName(databaseVendorType))) {
+ statement.setString(1, tenantDomain);
+ statement.setString(2, filterResolvedForSQL);
+ statement.setString(3, primaryOrgID);


Suggested change

statement.setString(3, primaryOrgID);

statement.setString(3, primaryOrgID);

SujanSanjula96 · 2024-09-16T03:21:01Z

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

+ validateForUnImplementedSortingAttributes(sortOrder, sortBy);
+ validateAttributesForPagination(offset, limit);
+
+ // TODO: 11/5/19 : Enforce a max limit


11/5/19 is a date it seems and shall we update that

asha15 · 2024-09-17T19:53:36Z

Closing the PR as the improvement has been moved to wso2-extensions/identity-organization-management#392

get discoverable applciations

887d67e

asha15 added 3 commits September 13, 2024 00:04

update DB query

3da0e50

refactor the code

2131144

update the comment

c0fd487