Skip to content
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

Improve applications list performance #4348

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve applications list performance #4348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b573d32
Fix proposal for applicationMgt list with slow userstore #2114
mxm-tr Nov 22, 2022
7bef81c
Merge branch 'master' of github.com:mxm-tr/carbon-identity-framework
mxm-tr Nov 22, 2022
00bad8f
Add AbstractUserStoreManager support in filterUser for ApplicationMgt…
mxm-tr Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 14 additions & 24 deletions ...main/java/org/wso2/carbon/identity/application/mgt/ApplicationManagementAdminService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
import java.util.regex.Pattern;

/**
* Application management admin service
* Application management admin service.
*/
public class ApplicationManagementAdminService extends AbstractAdmin {

Expand Down Expand Up @@ -112,7 +112,7 @@ public ServiceProvider createApplicationWithTemplate(ServiceProvider serviceProv
}

/**
* Get Service provider information for given application name
* Get Service provider information for given application name.
*
* @param applicationName Application name
* @return service provider
Expand All @@ -136,7 +136,7 @@ public ServiceProvider getApplication(String applicationName) throws IdentityApp
}

/**
* Get all basic application information
* Get all basic application information.
*
* @return Application Basic information array
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand Down Expand Up @@ -182,7 +182,7 @@ public ApplicationBasicInfo[] getApplicationBasicInfo(String filter)
}

/**
* Get all basic application information with paginated manner
* Get all basic application information with paginated manner.
*
* @return Application Basic information array
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand Down Expand Up @@ -411,7 +411,7 @@ private int getSynchronizedApplicationCount(List<String> applicationRoles, Strin
}

/**
* Update application
* Update application.
*
* @param serviceProvider Service provider
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand All @@ -437,7 +437,7 @@ public void updateApplication(ServiceProvider serviceProvider) throws IdentityAp
}

/**
* Delete Application
* Delete Application.
*
* @param applicationName Application name
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand All @@ -459,7 +459,7 @@ public void deleteApplication(String applicationName) throws IdentityApplication
}

/**
* Get identity provider by identity provider name
* Get identity provider by identity provider name.
*
* @param federatedIdPName Federated identity provider name
* @return Identity provider
Expand All @@ -479,7 +479,7 @@ public IdentityProvider getIdentityProvider(String federatedIdPName) throws Iden
}

/**
* Get all identity providers
* Get all identity providers.
*
* @return Identity providers array
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand All @@ -498,7 +498,7 @@ public IdentityProvider[] getAllIdentityProviders() throws IdentityApplicationMa
}

/**
* Get all local authenticators
* Get all local authenticators.
*
* @return local authenticators array
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand All @@ -518,7 +518,7 @@ public LocalAuthenticatorConfig[] getAllLocalAuthenticators() throws IdentityApp
}

/**
* Get all request path authenticator config
* Get all request path authenticator config.
*
* @return Request path authenticator config array
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand All @@ -539,7 +539,7 @@ public RequestPathAuthenticatorConfig[] getAllRequestPathAuthenticators()
}

/**
* Get all local claim uris
* Get all local claim uris.
*
* @return claim uri array
* @throws org.wso2.carbon.identity.application.common.IdentityApplicationManagementException
Expand All @@ -558,7 +558,7 @@ public String[] getAllLocalClaimUris() throws IdentityApplicationManagementExcep
}

/**
* Retrieve the set of authentication templates configured from file system in JSON format
* Retrieve the set of authentication templates configured from file system in JSON format.
*
* @return Authentication templates.
*/
Expand Down Expand Up @@ -803,18 +803,8 @@ private void generateCustomInboundAuthenticatorConfigs() {
private ArrayList<ApplicationBasicInfo> getAuthorizedApplicationBasicInfo(
ApplicationBasicInfo[] applicationBasicInfos, String userName)
throws IdentityApplicationManagementException {

ArrayList<ApplicationBasicInfo> appInfo = new ArrayList<>();
for (ApplicationBasicInfo applicationBasicInfo : applicationBasicInfos) {
if (ApplicationMgtUtil.isUserAuthorized(applicationBasicInfo.getApplicationName(), userName)) {
appInfo.add(applicationBasicInfo);
if (log.isDebugEnabled()) {
log.debug("Retrieving basic information of application: " +
applicationBasicInfo.getApplicationName() + "username: " + userName);
}
}
}
return appInfo;

return ApplicationMgtUtil.filterApplicationsForUser(applicationBasicInfos, userName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ApplicationMgtUtil.filterApplicationsForUser(applicationBasicInfos, userName);
return ApplicationMgtUtil.filterUserAuthorizedApplications(applicationBasicInfos, userName);

Copy link
Contributor

Choose a reason for hiding this comment

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

Calling another method from this method without any additional steps doesn't seem fine. Either the new method could have been call from where the getAuthorizedApplicationBasicInfo method was called.

}

/**
Expand Down
107 changes: 100 additions & 7 deletions ...cation.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/ApplicationMgtUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.wso2.carbon.context.PrivilegedCarbonContext;
import org.wso2.carbon.context.RegistryType;
import org.wso2.carbon.identity.application.common.IdentityApplicationManagementException;
import org.wso2.carbon.identity.application.common.model.ApplicationBasicInfo;
import org.wso2.carbon.identity.application.common.model.ApplicationPermission;
import org.wso2.carbon.identity.application.common.model.InboundAuthenticationRequestConfig;
import org.wso2.carbon.identity.application.common.model.PermissionsAndRoleConfig;
Expand Down Expand Up @@ -64,10 +65,12 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;

import javax.xml.bind.JAXBContext;
Expand Down Expand Up @@ -150,6 +153,96 @@ public static boolean isUserAuthorized(String applicationName, String username,
return true;
}


/**
* @param applicationInfos
* @param username
* @return a filtered list of ApplicationBasicInfo
* @throws IdentityApplicationManagementException
*/
public static ArrayList<ApplicationBasicInfo> filterApplicationsForUser(
ApplicationBasicInfo[] applicationInfos, String username
)
throws IdentityApplicationManagementException {

// Initialize list to return
ArrayList<ApplicationBasicInfo> authorizedAppInfo = new ArrayList<ApplicationBasicInfo>();

// Check whether roles validation is enabled
// If we do not validate the roles, return the whole list of applications
boolean validateRoles = validateRoles();
if (!validateRoles) {
if (log.isDebugEnabled()) {
log.debug(String.format("Validating user with application roles is disabled. Therefore, " +
"user: %s will be authorized for all applications", username));
}

// return new ArrayList<ApplicationBasicInfo>(applicationInfos);
return new ArrayList<ApplicationBasicInfo>(
(List<ApplicationBasicInfo>) Arrays.asList(applicationInfos));

}

// Get user store
try {
UserStoreManager userStoreManager = CarbonContext.getThreadLocalCarbonContext().getUserRealm()
.getUserStoreManager();

// List roles from user store
String[] userRoles = userStoreManager.getRoleListOfUser(username);

// If the user store is an implementation of the AbstractUserStoreManager,
// Get the role lists using its methods
log.debug("User roles" + Arrays.toString(userRoles));
if (userStoreManager instanceof AbstractUserStoreManager) {
try {

String[] userRolesAbstractUSM = (((AbstractUserStoreManager) userStoreManager)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it is required to cast and invoke the getRoleListOfUser method. The results would be same as Line 173.

.getRoleListOfUser(username));

// Merge the lists
Copy link
Contributor

Choose a reason for hiding this comment

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

If the above comment is valid, there is no point of merging. Also these operations will be costly when there large number of roles assigned for the user.

Set<String> userRolesList = new HashSet(Arrays.asList(userRoles));
userRolesList.addAll(Arrays.asList(userRolesAbstractUSM));
userRoles = userRolesList.toArray(String[]::new);

log.debug("AbstractUserStoreManager roles" + Arrays.toString(userRolesAbstractUSM));


} catch (UserStoreException e) {
throw new IdentityApplicationManagementException(
"Error while getting roles for user" + username, e);
}
}

// For each app, check whether the user the corresponding application role
for (ApplicationBasicInfo applicationBasicInfo : applicationInfos) {

String applicationName = applicationBasicInfo.getApplicationName();

String applicationRoleName = getAppRoleName(applicationName);

if (log.isDebugEnabled()) {
log.debug(
"Checking whether user has role : " + applicationRoleName
+ " by retrieving role list of " + "user : " + username);
}

for (String userRole : userRoles) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can improve the code using Java streams APIs

if (applicationRoleName.equals(userRole)) {
authorizedAppInfo.add(applicationBasicInfo);
}
}

}

} catch (UserStoreException e) {
throw new IdentityApplicationManagementException("Error getting roles for user: " +
username, e);
}
return authorizedAppInfo;
}


/**
* @param applicationName
* @param username
Expand Down Expand Up @@ -296,7 +389,7 @@ private static String getAppRoleName(String applicationName) {
}

/**
* Delete the role of the app
* Delete the role of the app.
*
* @param applicationName
* @throws IdentityApplicationManagementException
Expand Down Expand Up @@ -468,7 +561,7 @@ public static void storePermissions(String applicationName, String username,
}

/**
* Updates the permissions of the application
* Updates the permissions of the application.
*
* @param applicationName
* @param permissions
Expand Down Expand Up @@ -542,7 +635,7 @@ private static void addPermission(String applicationNode, ApplicationPermission[
}

/**
* Loads the permissions of the application
* Loads the permissions of the application.
*
* @param applicationName
* @return
Expand Down Expand Up @@ -620,7 +713,7 @@ private static void permissionPath(Registry tenantGovReg, String permissionPath,
}

/**
* Delete the resource
* Delete the resource.
*
* @param applicationName
* @throws IdentityApplicationManagementException
Expand Down Expand Up @@ -690,7 +783,7 @@ public static String getApplicationPermissionPath() {
}

/**
* Validate application name according to the regex
* Validate application name according to the regex.
*
* @return validated or not
*/
Expand All @@ -716,7 +809,7 @@ public static String getSPValidatorRegex() {
}

/**
* Get Property values
* Get Property values.
*
* @param tenantDomain Tenant domain
* @param spIssuer SP Issuer
Expand Down Expand Up @@ -811,7 +904,7 @@ public static boolean isValidApplicationOwner(ServiceProvider serviceProvider)
}

/**
* Get Service provider name from XML configuration file
* Get Service provider name from XML configuration file.
*
* @param spFileStream
* @param tenantDomain
Expand Down