Skip to content

Commit

Permalink
Fix: performance issue in MySQL
Browse files Browse the repository at this point in the history 
* Revert "Merge pull request #2704 from cloudfoundry/fix/optimize-ids-users-endpoint"

* This reverts commit 1324a81, reversing
  changes made to b8826e1.

* The original PR is suspected to cause the performance issues. We
  will try to bring this change back later.
  • Loading branch information
@bruce-ricard @Tallicia
bruce-ricard authored and Tallicia committed Apr 29, 2024
1 parent 355c059 commit 2ffb7af
Show file tree
Hide file tree
Showing 8 changed files with 251 additions and 543 deletions.
2 changes: 1 addition & 1 deletion server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public abstract class AbstractQueryable<T> implements Queryable<T> {

private NamedParameterJdbcTemplate namedParameterJdbcTemplate;

protected final JdbcPagingListFactory pagingListFactory;
private JdbcPagingListFactory pagingListFactory;

protected RowMapper<T> rowMapper;

Expand Down
8 changes: 2 additions & 6 deletions ...rc/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/SimpleSearchQueryConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import org.cloudfoundry.identity.uaa.util.AlphanumericRandomValueStringGenerator;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.lang.Nullable;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.util.StringUtils;
Expand Down Expand Up @@ -126,12 +125,9 @@ private String generateParameterPrefix(String filter) {
}
}

/**
* @return the WHERE and (optional) ORDER BY clauses WITHOUT the "WHERE" keyword in the beginning
*/
private String getWhereClause(
@Nullable final String filter,
@Nullable final String sortBy,
final String filter,
final String sortBy,
final boolean ascending,
final Map<String, Object> values,
final AttributeNameMapper mapper,
Expand Down
10 changes: 0 additions & 10 deletions server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,6 @@ public interface ScimUserProvisioning extends ResourceManager<ScimUser>, Queryab

List<ScimUser> retrieveByUsernameAndZone(String username, String zoneId);

/**
* Retrieve all users that satisfy the given SCIM filter and stem from active IdPs.
*/
List<ScimUser> retrieveByScimFilterOnlyActive(
String filter,
String sortBy,
boolean ascending,
String zoneId
);

List<ScimUser> retrieveByUsernameAndOriginAndZone(String username, String origin, String zoneId);

void changePassword(String id, String oldPassword, String newPassword, String zoneId) throws ScimResourceNotFoundException;
Expand Down
3 changes: 0 additions & 3 deletions server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@
import static org.cloudfoundry.identity.uaa.codestore.ExpiringCodeType.REGISTRATION;
import static org.springframework.util.StringUtils.isEmpty;

import lombok.Getter;

/**
* User provisioning and query endpoints. Implements the core API from the
* Simple Cloud Identity Management (SCIM)
Expand Down Expand Up @@ -122,7 +120,6 @@ public class ScimUserEndpoints implements InitializingBean, ApplicationEventPubl
private final ExpiringCodeStore codeStore;
private final ApprovalStore approvalStore;
private final ScimGroupMembershipManager membershipManager;
@Getter
private final int userMaxCount;
private final HttpMessageConverter<?>[] messageConverters;
private final AtomicInteger scimUpdates;
Expand Down
117 changes: 38 additions & 79 deletions ...src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,16 @@
package org.cloudfoundry.identity.uaa.scim.endpoints;

import javax.servlet.http.HttpServletRequest;

import java.util.Arrays;
import java.util.List;
import java.util.Map;

import org.cloudfoundry.identity.uaa.resources.SearchResultsFactory;
import com.unboundid.scim.sdk.SCIMException;
import com.unboundid.scim.sdk.SCIMFilter;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.provider.IdentityProvider;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.resources.SearchResults;
import org.cloudfoundry.identity.uaa.scim.ScimCore;
import org.cloudfoundry.identity.uaa.scim.ScimUser;
import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning;
import org.cloudfoundry.identity.uaa.scim.exception.ScimException;
import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor;
import org.cloudfoundry.identity.uaa.util.UaaPagingUtils;
import org.cloudfoundry.identity.uaa.util.UaaStringUtils;
import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.InitializingBean;
Expand All @@ -32,98 +28,61 @@
import org.springframework.web.servlet.View;
import org.springframework.web.util.HtmlUtils;

import com.unboundid.scim.sdk.SCIMException;
import com.unboundid.scim.sdk.SCIMFilter;
import javax.servlet.http.HttpServletRequest;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

@Controller
public class UserIdConversionEndpoints implements InitializingBean {
private static final String FIELD_USERNAME = "userName";
private static final String FIELD_ID = "id";
private static final String FIELD_ORIGIN = "origin";

private final Logger logger = LoggerFactory.getLogger(getClass());

private final ScimUserProvisioning scimUserProvisioning;
private final IdentityProviderProvisioning provisioning;
private final SecurityContextAccessor securityContextAccessor;
private final ScimUserEndpoints scimUserEndpoints;
private final IdentityZoneManager identityZoneManager;
private final boolean enabled;

public UserIdConversionEndpoints(
final SecurityContextAccessor securityContextAccessor,
final ScimUserEndpoints scimUserEndpoints,
final @Qualifier("scimUserProvisioning") ScimUserProvisioning scimUserProvisioning,
final IdentityZoneManager identityZoneManager,
final @Value("${scim.userids_enabled:true}") boolean enabled
) {

private boolean enabled;

public UserIdConversionEndpoints(final @Qualifier("identityProviderProvisioning") IdentityProviderProvisioning provisioning,
final SecurityContextAccessor securityContextAccessor,
final ScimUserEndpoints scimUserEndpoints,
final @Value("${scim.userids_enabled:true}") boolean enabled) {
this.provisioning = provisioning;
this.securityContextAccessor = securityContextAccessor;
this.scimUserEndpoints = scimUserEndpoints;
this.scimUserProvisioning = scimUserProvisioning;
this.identityZoneManager = identityZoneManager;
this.enabled = enabled;
}

@RequestMapping(value = "/ids/Users")
@ResponseBody
public ResponseEntity<Object> findUsers(
@RequestParam(defaultValue = "") String filter,
@RequestParam(required = false, defaultValue = "ascending") final String sortOrder,
@RequestParam(required = false, defaultValue = "ascending") String sortOrder,
@RequestParam(required = false, defaultValue = "1") int startIndex,
@RequestParam(required = false, defaultValue = "100") int count,
@RequestParam(required = false, defaultValue = "false") final boolean includeInactive
) {
@RequestParam(required = false, defaultValue = "false") boolean includeInactive) {
if (!enabled) {
logger.info("Request from user {} received at disabled Id translation endpoint with filter:{}",
UaaStringUtils.getCleanedUserControlString(securityContextAccessor.getAuthenticationInfo()),
UaaStringUtils.getCleanedUserControlString(filter));
UaaStringUtils.getCleanedUserControlString(securityContextAccessor.getAuthenticationInfo()),
UaaStringUtils.getCleanedUserControlString(filter));
return new ResponseEntity<>("Illegal Operation: Endpoint not enabled.", HttpStatus.BAD_REQUEST);
}

if (startIndex < 1) {
startIndex = 1;
}

if (count > scimUserEndpoints.getUserMaxCount()) {
count = scimUserEndpoints.getUserMaxCount();
}

filter = filter.trim();
checkFilter(filter);

// get all users for the given filter and the current page
final boolean ascending = sortOrder.equalsIgnoreCase("ascending");
final List<ScimUser> filteredUsers;
if (includeInactive) {
filteredUsers = scimUserProvisioning.query(
filter, FIELD_USERNAME, ascending, identityZoneManager.getCurrentIdentityZoneId()
);
} else {
filteredUsers = scimUserProvisioning.retrieveByScimFilterOnlyActive(
filter, FIELD_USERNAME, ascending, identityZoneManager.getCurrentIdentityZoneId()
);
List<IdentityProvider> activeIdentityProviders = provisioning.retrieveActive(IdentityZoneHolder.get().getId());

if (!includeInactive) {
if (activeIdentityProviders.isEmpty()) {
return new ResponseEntity<>(new SearchResults<>(Arrays.asList(ScimCore.SCHEMAS), new ArrayList<>(), startIndex, count, 0), HttpStatus.OK);
}
String originFilter = activeIdentityProviders.stream().map(identityProvider -> "".concat("origin eq \"" + identityProvider.getOriginKey() + "\"")).collect(Collectors.joining(" OR "));
filter += " AND (" + originFilter + " )";
}
final List<ScimUser> usersCurrentPage = UaaPagingUtils.subList(filteredUsers, startIndex, count);

// map to result structure
final List<Map<String, String>> result = usersCurrentPage.stream()
.map(scimUser -> Map.of(
FIELD_ID, scimUser.getId(),
FIELD_USERNAME, scimUser.getUserName(),
FIELD_ORIGIN, scimUser.getOrigin()
))
.toList();

return new ResponseEntity<>(
SearchResultsFactory.buildSearchResultFrom(
result,
startIndex,
count,
filteredUsers.size(),
new String[]{FIELD_ID, FIELD_USERNAME, FIELD_ORIGIN},
Arrays.asList(ScimCore.SCHEMAS)
),
HttpStatus.OK
);

return new ResponseEntity<>(scimUserEndpoints.findUsers("id,userName,origin", filter, "userName", sortOrder, startIndex, count), HttpStatus.OK);
}

@ExceptionHandler
Expand Down Expand Up @@ -164,10 +123,10 @@ private boolean containsIdOrUserNameClause(SCIMFilter filter) {
return containsIdOrUserNameClause(filter.getFilterComponents().get(1)) || resultLeftOperand;
case EQUALITY:
String name = filter.getFilterAttribute().getAttributeName();
if (FIELD_ID.equalsIgnoreCase(name) ||
FIELD_USERNAME.equalsIgnoreCase(name)) {
if ("id".equalsIgnoreCase(name) ||
"userName".equalsIgnoreCase(name)) {
return true;
} else if (FIELD_ORIGIN.equalsIgnoreCase(name)) {
} else if (OriginKeys.ORIGIN.equalsIgnoreCase(name)) {
return false;
} else {
throw new ScimException("Invalid filter attribute.", HttpStatus.BAD_REQUEST);
Expand Down
117 changes: 24 additions & 93 deletions server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,19 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.scim.jdbc;

import static java.sql.Types.VARCHAR;
import static java.util.stream.Collectors.joining;
import static org.springframework.util.StringUtils.hasText;

import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Timestamp;
import java.sql.Types;
import java.util.Arrays;
import java.util.Calendar;
import java.util.Date;
import java.util.GregorianCalendar;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.regex.Pattern;
import java.util.stream.Stream;

import org.cloudfoundry.identity.uaa.util.UaaStringUtils;
import org.cloudfoundry.identity.uaa.zone.IdentityZone;
import org.cloudfoundry.identity.uaa.zone.JdbcIdentityZoneProvisioning;
import org.cloudfoundry.identity.uaa.zone.UserConfig;
import org.cloudfoundry.identity.uaa.zone.ZoneDoesNotExistsException;
import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.cloudfoundry.identity.uaa.audit.event.SystemDeletable;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.resources.AttributeNameMapper;
import org.cloudfoundry.identity.uaa.resources.ResourceMonitor;
import org.cloudfoundry.identity.uaa.resources.jdbc.AbstractQueryable;
import org.cloudfoundry.identity.uaa.resources.jdbc.JdbcPagingListFactory;
import org.cloudfoundry.identity.uaa.resources.jdbc.SearchQueryConverter.ProcessedFilter;
import org.cloudfoundry.identity.uaa.resources.jdbc.SimpleSearchQueryConverter;
import org.cloudfoundry.identity.uaa.scim.ScimMeta;
import org.cloudfoundry.identity.uaa.scim.ScimUser;
Expand All @@ -52,25 +39,32 @@
import org.cloudfoundry.identity.uaa.user.JdbcUaaUserDatabase;
import org.cloudfoundry.identity.uaa.util.TimeService;
import org.cloudfoundry.identity.uaa.util.TimeServiceImpl;
import org.cloudfoundry.identity.uaa.util.UaaStringUtils;
import org.cloudfoundry.identity.uaa.zone.IdentityZone;
import org.cloudfoundry.identity.uaa.zone.JdbcIdentityZoneProvisioning;
import org.cloudfoundry.identity.uaa.zone.UserConfig;
import org.cloudfoundry.identity.uaa.zone.ZoneDoesNotExistsException;
import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.dao.DuplicateKeyException;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.dao.IncorrectResultSizeDataAccessException;
import org.springframework.dao.OptimisticLockingFailureException;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.util.Assert;

import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Timestamp;
import java.sql.Types;
import java.util.Calendar;
import java.util.Date;
import java.util.GregorianCalendar;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.regex.Pattern;

import static java.sql.Types.VARCHAR;
import static org.springframework.util.StringUtils.hasText;

public class JdbcScimUserProvisioning extends AbstractQueryable<ScimUser>
implements ScimUserProvisioning, ResourceMonitor<ScimUser>, SystemDeletable {

Expand Down Expand Up @@ -178,69 +172,6 @@ public List<ScimUser> retrieveByUsernameAndZone(String username, String zoneId)
return jdbcTemplate.query(USER_BY_USERNAME_AND_ZONE_QUERY , mapper, username, zoneId);
}

@Override
public List<ScimUser> retrieveByScimFilterOnlyActive(
final String filter,
final String sortBy,
final boolean ascending,
final String zoneId
) {
final SimpleSearchQueryConverter queryConverter = new SimpleSearchQueryConverter();
validateOrderBy(queryConverter.map(sortBy));

/* since the two tables used in the query ('users' and 'identity_provider') have columns with identical names,
* we must ensure that the columns of 'users' are used in the WHERE clause generated for the SCIM filter */
final AttributeNameMapper attributeNameMapper = new AttributeNameMapper() {
@Override
public String mapToInternal(final String attr) {
// in the later query, 'users' will have the alias 'u'
return "u." + attr;
}

@Override
public String[] mapToInternal(final String[] attr) {
return Stream.of(attr).map(this::mapToInternal).toArray(String[]::new);
}

@Override
public String mapFromInternal(final String attr) {
return attr.substring(2);
}

@Override
public String[] mapFromInternal(final String[] attr) {
return Stream.of(attr).map(this::mapFromInternal).toArray(String[]::new);
}
};
queryConverter.setAttributeNameMapper(attributeNameMapper);

// build WHERE clause
final ProcessedFilter where = queryConverter.convert(filter, sortBy, ascending, zoneId);
final String whereClauseScimFilter = where.getSql();
String whereClause = "idp.active is true and (";
if (where.hasOrderBy()) {
whereClause += whereClauseScimFilter.replace(ProcessedFilter.ORDER_BY, ")" + ProcessedFilter.ORDER_BY);
} else {
whereClause += whereClauseScimFilter + ")";
}

final String userFieldsWithPrefix = Arrays.stream(USER_FIELDS.split(","))
.map(field -> "u." + field)
.collect(joining(", "));
final String sql = String.format(
"select %s from users u join identity_provider idp on u.origin = idp.origin_key and u.identity_zone_id = idp.identity_zone_id where %s",
userFieldsWithPrefix,
whereClause
);

if (getPageSize() > 0 && getPageSize() < Integer.MAX_VALUE) {
return pagingListFactory.createJdbcPagingList(sql, where.getParams(), rowMapper, getPageSize());
}

final NamedParameterJdbcTemplate namedParameterJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate);
return namedParameterJdbcTemplate.query(sql, where.getParams(), rowMapper);
}

@Override
public List<ScimUser> retrieveByUsernameAndOriginAndZone(String username, String origin, String zoneId) {
return jdbcTemplate.query(USER_BY_USERNAME_AND_ORIGIN_AND_ZONE_QUERY , mapper, username, origin, zoneId);
Expand Down
Loading

0 comments on commit 2ffb7af

Please sign in to comment.