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.

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

Optimize "/ids/Users" endpoint #2704

Merged
merged 22 commits into from
Mar 10, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1326570
Add getter for maxUsers in ScimUserEndpoints
adrianhoelzl-sap Feb 2, 2024
dd5af1c
Fix lookup of users in "ids/Users" endpoint
adrianhoelzl-sap Feb 2, 2024
d7cd323
Revert removing generic wildcard from findUsers response type
adrianhoelzl-sap Feb 2, 2024
395dee5
Move reading all users for a given filter to a separate method in Sci…
adrianhoelzl-sap Feb 2, 2024
95457f1
Remove unused import
adrianhoelzl-sap Feb 2, 2024
ef07b01
Merge branch 'fix/filter-check-in-ids-users-endpoint' into fix/optimi…
adrianhoelzl-sap Feb 9, 2024
c2dc58d
Add javadoc comment to SimpleSearchQueryConverter#getWhereClause
adrianhoelzl-sap Feb 12, 2024
8d139c8
Add method JdbcScimUserProvisioning#retrieveByScimFilter for filterin…
adrianhoelzl-sap Feb 12, 2024
8a75e22
Fix visibility of AbstractQueryable#pagingListFactory
adrianhoelzl-sap Feb 12, 2024
2598f64
Use new method for filtering SCIM users in UserIdConversionEndpoints
adrianhoelzl-sap Feb 12, 2024
cf64562
Revert obsolete changes to ScimUserEndpoints
adrianhoelzl-sap Feb 12, 2024
b4d9050
Fix Sonar finding: use constant instead of duplicating String literal
adrianhoelzl-sap Feb 12, 2024
cea1963
Fix Sonar finding: replace .collect(toList()) with .toList()
adrianhoelzl-sap Feb 12, 2024
b8a8b7f
Fix Sonar finding: replace IdentityZoneHolder usage
adrianhoelzl-sap Feb 12, 2024
28c59d1
Fix Sonar finding: introduce parameterized test
adrianhoelzl-sap Feb 12, 2024
3df2648
Add additional unit tests for JdbcScimUserProvisioning#retrieveByScim…
adrianhoelzl-sap Feb 12, 2024
3d0a7e9
Refactor
adrianhoelzl-sap Feb 12, 2024
9d70584
Fix table alias in WHERE clause translated from SCIM filter
adrianhoelzl-sap Feb 13, 2024
09a489d
Merge branch 'develop' into fix/optimize-ids-users-endpoint
adrianhoelzl-sap Feb 26, 2024
709a372
Rework
adrianhoelzl-sap Mar 7, 2024
2644f53
Rework
adrianhoelzl-sap Mar 8, 2024
d0cf81f
Rework
adrianhoelzl-sap Mar 8, 2024
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
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;

private JdbcPagingListFactory pagingListFactory;
protected final JdbcPagingListFactory pagingListFactory;

protected RowMapper<T> rowMapper;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
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 @@ -125,9 +126,12 @@ private String generateParameterPrefix(String filter) {
}
}

/**
* @return the WHERE and (optional) ORDER BY clauses WITHOUT the "WHERE" keyword in the beginning
*/
private String getWhereClause(
final String filter,
final String sortBy,
@Nullable final String filter,
@Nullable final String sortBy,
final boolean ascending,
final Map<String, Object> values,
final AttributeNameMapper mapper,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@
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 @@ -120,6 +122,7 @@ 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
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
package org.cloudfoundry.identity.uaa.scim.endpoints;

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 javax.servlet.http.HttpServletRequest;

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

import org.cloudfoundry.identity.uaa.resources.SearchResultsFactory;
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.IdentityZoneHolder;
import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.InitializingBean;
Expand All @@ -28,61 +32,98 @@
import org.springframework.web.servlet.View;
import org.springframework.web.util.HtmlUtils;

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

@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 IdentityProviderProvisioning provisioning;
private final ScimUserProvisioning scimUserProvisioning;
private final SecurityContextAccessor securityContextAccessor;
private final ScimUserEndpoints scimUserEndpoints;

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;
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
) {
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") String sortOrder,
@RequestParam(required = false, defaultValue = "ascending") final String sortOrder,
@RequestParam(required = false, defaultValue = "1") int startIndex,
@RequestParam(required = false, defaultValue = "100") int count,
@RequestParam(required = false, defaultValue = "false") boolean includeInactive) {
@RequestParam(required = false, defaultValue = "false") final 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);

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 + " )";
// 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()
);
}

return new ResponseEntity<>(scimUserEndpoints.findUsers("id,userName,origin", filter, "userName", sortOrder, startIndex, count), HttpStatus.OK);
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
);
}

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

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 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.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 @@ -39,32 +52,25 @@
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 @@ -170,6 +176,69 @@ 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,
swalchemist marked this conversation as resolved.
Show resolved Hide resolved
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
Loading