Skip to content

Commit

Permalink
Delete dependent relationships when deleting group
Browse files Browse the repository at this point in the history 
[#115293245] https://www.pivotaltracker.com/story/show/115293245
  • Loading branch information
Jeremy Coffield committed Mar 24, 2016
1 parent 9e2017a commit 27717d1
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 23 deletions.
3 changes: 3 additions & 0 deletions .../src/main/java/org/cloudfoundry/identity/uaa/scim/ScimGroupExternalMembershipManager.java
View file
Expand Up @@ -33,4 +33,7 @@ List<ScimGroupExternalMember> getExternalGroupMapsByExternalGroup(String externa

List<ScimGroupExternalMember> getExternalGroupMapsByGroupName(String groupName, String origin)
throws ScimResourceNotFoundException;

void unmapAll(String groupId)
throws ScimResourceNotFoundException;
}
18 changes: 18 additions & 0 deletions .../java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupExternalMembershipManager.java
View file
Expand Up @@ -79,6 +79,9 @@ public class JdbcScimGroupExternalMembershipManager extends AbstractQueryable<Sc
String.format("delete from %s where group_id=? and lower(external_group)=lower(?) and origin=?",
EXTERNAL_GROUP_MAPPING_TABLE);

public static final String DELETE_ALL_MAPPINGS_FOR_GROUP_SQL =
String.format("delete from %s where group_id = ?", EXTERNAL_GROUP_MAPPING_TABLE);

private final RowMapper<ScimGroupExternalMember> rowMapper = new ScimGroupExternalMemberRowMapper();

private ScimGroupProvisioning scimGroupProvisioning;
Expand Down Expand Up @@ -238,6 +241,21 @@ public void setValues(PreparedStatement ps) throws SQLException {
}
}

@Override
public void unmapAll(String groupId) throws ScimResourceNotFoundException {
ScimGroup group = scimGroupProvisioning.retrieve(groupId);
if (null == group) {
throw new ScimResourceNotFoundException("Group not found for ID " + groupId);
}

jdbcTemplate.update(DELETE_ALL_MAPPINGS_FOR_GROUP_SQL, new PreparedStatementSetter() {
@Override
public void setValues(PreparedStatement ps) throws SQLException {
ps.setString(1, groupId);
}
});
}

@Override
public List<ScimGroupExternalMember> getExternalGroupMapsByExternalGroup(final String externalGroup,
final String origin)
Expand Down
10 changes: 0 additions & 10 deletions ...src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupMembershipManager.java
View file
Expand Up @@ -152,16 +152,6 @@ public int delete(String filter) {
}
}

@Override
public List<ScimGroupMember> query(String filter) {
return super.query(filter);
}

@Override
public List<ScimGroupMember> query(String filter, String sortBy, boolean ascending) {
return super.query(filter, sortBy, ascending);
}

@Override
protected String getQuerySQL(String filter, SearchQueryConverter.ProcessedFilter where) {
boolean containsWhereClause = getBaseSqlQuery().contains(" where ");
Expand Down
11 changes: 11 additions & 0 deletions server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java
View file
Expand Up @@ -17,6 +17,7 @@
import org.cloudfoundry.identity.uaa.audit.event.EntityDeletedEvent;
import org.cloudfoundry.identity.uaa.audit.event.SystemDeletable;
import org.cloudfoundry.identity.uaa.resources.jdbc.AbstractQueryable;
import org.cloudfoundry.identity.uaa.resources.jdbc.DefaultLimitSqlAdapter;
import org.cloudfoundry.identity.uaa.resources.jdbc.JdbcPagingListFactory;
import org.cloudfoundry.identity.uaa.scim.ScimGroup;
import org.cloudfoundry.identity.uaa.scim.ScimGroupProvisioning;
Expand Down Expand Up @@ -48,7 +49,9 @@
public class JdbcScimGroupProvisioning extends AbstractQueryable<ScimGroup>
implements ScimGroupProvisioning, ApplicationListener<EntityDeletedEvent<?>>, SystemDeletable {

private JdbcScimGroupExternalMembershipManager externalGroupMappingManager;
private JdbcTemplate jdbcTemplate;
private JdbcScimGroupMembershipManager membershipManager;

private final Log logger = LogFactory.getLog(getClass());

Expand Down Expand Up @@ -90,6 +93,12 @@ public Log getLogger() {

public JdbcScimGroupProvisioning(JdbcTemplate jdbcTemplate, JdbcPagingListFactory pagingListFactory) {
super(jdbcTemplate, pagingListFactory, new ScimGroupRowMapper());

this.membershipManager = new JdbcScimGroupMembershipManager(jdbcTemplate, pagingListFactory);
this.membershipManager.setScimGroupProvisioning(this);
this.externalGroupMappingManager = new JdbcScimGroupExternalMembershipManager(jdbcTemplate, pagingListFactory);
this.externalGroupMappingManager.setScimGroupProvisioning(this);

Assert.notNull(jdbcTemplate);
this.jdbcTemplate = jdbcTemplate;
setQueryConverter(new ScimSearchQueryConverter());
Expand Down Expand Up @@ -192,6 +201,8 @@ public void setValues(PreparedStatement ps) throws SQLException {
@Override
public ScimGroup delete(String id, int version) throws ScimResourceNotFoundException {
ScimGroup group = retrieve(id);
membershipManager.removeMembersByGroupId(id);
externalGroupMappingManager.unmapAll(id);
int deleted;
if (version > 0) {
deleted = jdbcTemplate.update(DELETE_GROUP_SQL + " and version=?;", id, IdentityZoneHolder.get().getId(),version);
Expand Down
3 changes: 0 additions & 3 deletions ...st/java/org/cloudfoundry/identity/uaa/scim/bootstrap/ScimExternalGroupBootstrapTests.java
View file
Expand Up @@ -12,14 +12,11 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.scim.bootstrap;

import java.util.ArrayList;
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.Set;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
Expand Down
10 changes: 9 additions & 1 deletion .../org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupExternalMembershipManagerTests.java
View file
Expand Up @@ -91,6 +91,14 @@ public void addExternalMappingToGroup() {
createGroupMapping();
}

@Test
public void deleteGroupAndMappings() {
createGroupMapping();
gdao.delete("g1-"+IdentityZoneHolder.get().getId(), -1);
int mappingsCount = jdbcTemplate.queryForObject("select count(1) from " + JdbcScimGroupExternalMembershipManager.EXTERNAL_GROUP_MAPPING_TABLE, Integer.class);
assertEquals(0, mappingsCount);
}

private void createGroupMapping() {
ScimGroup group = gdao.retrieve("g1-"+IdentityZoneHolder.get().getId());
assertNotNull(group);
Expand All @@ -101,7 +109,7 @@ private void createGroupMapping() {

List<ScimGroupExternalMember> externalMapping = edao.getExternalGroupMapsByGroupId("g1-"+IdentityZoneHolder.get().getId(), origin);

assertEquals(externalMapping.size(), 1);
assertEquals(1, externalMapping.size());
}

@Test(expected = ScimResourceNotFoundException.class)
Expand Down
73 changes: 64 additions & 9 deletions ...src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioningTests.java
View file
Expand Up @@ -12,9 +12,13 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.scim.jdbc;

import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.resources.jdbc.DefaultLimitSqlAdapter;
import org.cloudfoundry.identity.uaa.resources.jdbc.JdbcPagingListFactory;
import org.cloudfoundry.identity.uaa.scim.ScimGroup;
import org.cloudfoundry.identity.uaa.scim.ScimGroupMember;
import org.cloudfoundry.identity.uaa.scim.ScimUser;
import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning;
import org.cloudfoundry.identity.uaa.scim.exception.ScimResourceNotFoundException;
import org.cloudfoundry.identity.uaa.scim.test.TestUtils;
import org.cloudfoundry.identity.uaa.test.JdbcTestBase;
Expand All @@ -25,52 +29,68 @@
import java.sql.Timestamp;
import java.util.Arrays;
import java.util.List;
import java.util.UUID;

import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.springframework.util.StringUtils.hasText;

public class JdbcScimGroupProvisioningTests extends JdbcTestBase {

private JdbcScimGroupProvisioning dao;
private JdbcScimGroupMembershipManager memberships;
private ScimUserProvisioning users;

private static final String SQL_INJECTION_FIELDS = "displayName,version,created,lastModified";

private int existingGroupCount = -1;
private ScimGroup g1;
private ScimGroup g2;
private ScimGroup g3;

@Before
public void initJdbcScimGroupProvisioningTests() {
memberships = new JdbcScimGroupMembershipManager(jdbcTemplate, new JdbcPagingListFactory(jdbcTemplate, new DefaultLimitSqlAdapter()));
dao = new JdbcScimGroupProvisioning(jdbcTemplate, new JdbcPagingListFactory(jdbcTemplate, limitSqlAdapter));
memberships.setScimGroupProvisioning(dao);
users = mock(ScimUserProvisioning.class);
memberships.setScimUserProvisioning(users);

addGroup("g1", "uaa.user");
addGroup("g2", "uaa.admin");
addGroup("g3", "openid");
g1 = addGroup("g1", "uaa.user");
g2 = addGroup("g2", "uaa.admin");
g3 = addGroup("g3", "openid");

validateGroupCount(3);
}

private void validateGroupCount(int expected) {
existingGroupCount = jdbcTemplate.queryForObject("select count(id) from groups where identity_zone_id='"+IdentityZoneHolder.get().getId()+"'", Integer.class);
assertEquals(expected, existingGroupCount);
}

private void validateGroup(ScimGroup group, String name, String zoneId) {

}
private void validateGroup(ScimGroup group, String name, String zoneId, String description) {
assertNotNull(group);
assertNotNull(group.getId());
assertNotNull(group.getDisplayName());
if (hasText(name)) {
assertEquals(name, group.getDisplayName());
}
if (hasText(description)) {
assertEquals(description, group.getDescription());
}
if (hasText(zoneId)) {
assertEquals(zoneId, group.getZoneId());
}
}

private void validateGroup(ScimGroup group, String name, String zoneId, String description) {
validateGroup(group, name, zoneId);
if (hasText(description)) {
assertEquals(description, group.getDescription());
}
}

@Test
public void canRetrieveGroups() throws Exception {
List<ScimGroup> groups = dao.retrieveAll();
Expand Down Expand Up @@ -185,8 +205,27 @@ public void canUpdateGroup() throws Exception {

@Test
public void canRemoveGroup() throws Exception {
addUserToGroup(g1.getId(), "joe@example.com");
addUserToGroup(g1.getId(), "mary@example.com");
ScimGroupMember bill = addUserToGroup(g2.getId(), "bill@example.com");

dao.delete("g1", 0);
validateGroupCount(2);
List<ScimGroupMember> remainingMemberships = memberships.query("");
assertEquals(1, remainingMemberships.size());
ScimGroupMember survivor = remainingMemberships.get(0);
assertThat(survivor.getType(), is(ScimGroupMember.Type.USER));
assertEquals(bill.getMemberId(), survivor.getMemberId());
}

@Test
public void deleteGroupWithNestedMembers() {
ScimGroup appUsers = addGroup("appuser", "app.user");
addGroupToGroup(appUsers.getId(), g1.getId());
dao.delete(appUsers.getId(), 0);

List<ScimGroupMember> remainingMemberships = memberships.query("");
assertEquals(0, remainingMemberships.size());
}

private ScimGroup addGroup(String id, String name) {
Expand All @@ -204,6 +243,22 @@ private ScimGroup addGroup(String id, String name) {
return dao.retrieve(id);
}

private ScimGroupMember<ScimUser> addUserToGroup(String groupId, String username) {
String userId = UUID.randomUUID().toString();
ScimUser scimUser = new ScimUser(userId, username, username, username);
scimUser.setZoneId(OriginKeys.UAA);
when(users.retrieve(userId)).thenReturn(scimUser);
ScimGroupMember<ScimUser> member = new ScimGroupMember<>(scimUser);
memberships.addMember(groupId, member);
return member;
}

private ScimGroupMember addGroupToGroup(String parentGroupId, String childGroupId) {
ScimGroupMember<ScimGroup> member = new ScimGroupMember<>(dao.retrieve(childGroupId));
memberships.addMember(parentGroupId, member);
return member;
}

@Test(expected = IllegalArgumentException.class)
public void sqlInjectionAttack1Fails() {
dao.query("displayName='something'; select " + SQL_INJECTION_FIELDS
Expand Down

0 comments on commit 27717d1

Please sign in to comment.