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

Reject IdZ deletion if an IdP with alias exists in the zone #2850

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b51e355
Remove deletion of alias IdP from JdbcIdentityProviderProvisioning.de…
adrianhoelzl-sap Apr 24, 2024
5a7fd8c
Reject deletion of identity zone if an IdP with alias exists in the zone
adrianhoelzl-sap Apr 24, 2024
a6966f8
Add new status code to identity zone deletion documentation
adrianhoelzl-sap Apr 24, 2024
ce46e69
Fix Sonar: remove unnecessary clause in if statement
adrianhoelzl-sap Apr 25, 2024
b74a620
Add unit test for zone deletion success case
adrianhoelzl-sap Apr 25, 2024
cda7f1c
Add MockMVC test: deletion of zone should fail when IdP with alias ex…
adrianhoelzl-sap May 15, 2024
e19a904
Add method idpWithAliasExistsInZone to JdbcIdentityProviderProvisioning
adrianhoelzl-sap May 15, 2024
59905c1
Replace filtering for IdP with alias in Java code with new IdpProvisi…
adrianhoelzl-sap May 15, 2024
887239d
Merge branch 'develop' into feature/reject-idz-deletion-if-idp-with-a…
adrianhoelzl-sap May 15, 2024
4f05c87
Fix query in JdbcIdentityProviderProvisioning.idpWithAliasExistsInZone
adrianhoelzl-sap May 15, 2024
7f52786
Replace count query with query limited to one row
adrianhoelzl-sap Jun 4, 2024
780d9ff
Add indexes on identity_provider.alias_zid and users.alias_zid
adrianhoelzl-sap Jun 4, 2024
a3a72bb
Fix tests
adrianhoelzl-sap Jun 4, 2024
a09808b
Rework
adrianhoelzl-sap Jun 5, 2024
b77cecd
Move method idpWithAliasExistsInZone from JdbcIdentityProviderProvisi…
adrianhoelzl-sap Jun 5, 2024
87c3b2d
Merge remote-tracking branch 'refs/remotes/origin/develop' into featu…
strehle Jun 10, 2024
3acb204
Add tests
strehle Jun 10, 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 @@ -21,6 +21,8 @@ public interface IdentityProviderProvisioning {

IdentityProvider update(IdentityProvider identityProvider, String zoneId);

boolean idpWithAliasExistsInZone(String zoneId);

IdentityProvider retrieve(String id, String zoneId);

List<IdentityProvider> retrieveActive(String zoneId);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.cloudfoundry.identity.uaa.provider;

import static java.sql.Types.VARCHAR;

import org.cloudfoundry.identity.uaa.audit.event.SystemDeletable;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.util.JsonUtils;
Expand All @@ -11,7 +13,6 @@
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.stereotype.Component;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;

import java.sql.ResultSet;
Expand All @@ -34,13 +35,15 @@ public class JdbcIdentityProviderProvisioning implements IdentityProviderProvisi

public static final String IDENTITY_ACTIVE_PROVIDERS_QUERY = IDENTITY_PROVIDERS_QUERY + " and active=?";

public static final String IDP_WITH_ALIAS_EXISTS_QUERY = "select 1 from identity_provider idp where idp.identity_zone_id = ? and idp.alias_zid is not null and idp.alias_zid <> '' limit 1";

public static final String ID_PROVIDER_UPDATE_FIELDS = "version,lastmodified,name,type,config,active,alias_id,alias_zid".replace(",", "=?,") + "=?";

public static final String UPDATE_IDENTITY_PROVIDER_SQL = "update identity_provider set " + ID_PROVIDER_UPDATE_FIELDS + " where id=? and identity_zone_id=?";

public static final String DELETE_IDENTITY_PROVIDER_BY_ORIGIN_SQL = "delete from identity_provider where identity_zone_id=? and origin_key = ?";

public static final String DELETE_IDENTITY_PROVIDER_BY_ZONE_SQL = "delete from identity_provider where identity_zone_id=? or alias_zid=?";
public static final String DELETE_IDENTITY_PROVIDER_BY_ZONE_SQL = "delete from identity_provider where identity_zone_id=?";
strehle marked this conversation as resolved.
Show resolved Hide resolved

public static final String IDENTITY_PROVIDER_BY_ID_QUERY = "select " + ID_PROVIDER_FIELDS + " from identity_provider " + "where id=? and identity_zone_id=?";

Expand All @@ -56,6 +59,18 @@ public JdbcIdentityProviderProvisioning(JdbcTemplate jdbcTemplate) {
this.jdbcTemplate = jdbcTemplate;
}

@Override
public boolean idpWithAliasExistsInZone(final String zoneId) {
final List<Integer> result = jdbcTemplate.queryForList(
IDP_WITH_ALIAS_EXISTS_QUERY,
new Object[]{zoneId},
new int[]{VARCHAR},
Integer.class
);
// if an IdP with alias is present, the list contains a single element, otherwise it is empty
return result.size() == 1;
}

hsinn0 marked this conversation as resolved.
Show resolved Hide resolved
@Override
public IdentityProvider retrieve(String id, String zoneId) {
return jdbcTemplate.queryForObject(IDENTITY_PROVIDER_BY_ID_QUERY, mapper, id, zoneId);
Expand Down Expand Up @@ -150,12 +165,9 @@ protected void validate(IdentityProvider provider) {
}
}

/**
* Delete all identity providers in the given zone as well as all alias identity providers of them.
*/
@Override
public int deleteByIdentityZone(String zoneId) {
return jdbcTemplate.update(DELETE_IDENTITY_PROVIDER_BY_ZONE_SQL, zoneId, zoneId);
return jdbcTemplate.update(DELETE_IDENTITY_PROVIDER_BY_ZONE_SQL, zoneId);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ public IdentityProvider update(IdentityProvider identityProvider, String zoneId)
return providerProvisioning.update(identityProvider, zoneId);
}

@Override
public boolean idpWithAliasExistsInZone(final String zoneId) {
return providerProvisioning.idpWithAliasExistsInZone(zoneId);
}

@Override
public IdentityProvider retrieve(String id, String zoneId) {
IdentityProvider p = providerProvisioning.retrieve(id, zoneId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,15 @@ public ResponseEntity<IdentityZone> deleteIdentityZone(@PathVariable String id)
IdentityZone zone = zoneDao.retrieveIgnoreActiveFlag(id);
// ignore the id in the body, the id in the path is the only one that matters
IdentityZoneHolder.set(zone);
if (publisher != null && zone != null) {

/* reject deletion if an IdP with alias exists in the zone - checking for users with alias is not required
* here, since they can only exist if their origin IdP has an alias as well */
final boolean idpWithAliasExists = idpDao.idpWithAliasExistsInZone(zone.getId());
if (idpWithAliasExists) {
return new ResponseEntity<>(UNPROCESSABLE_ENTITY);
}

if (publisher != null) {
publisher.publishEvent(new EntityDeletedEvent<>(zone, SecurityContextHolder.getContext().getAuthentication(), IdentityZoneHolder.getCurrentZoneId()));
logger.debug("Zone - deleted id[" + zone.getId() + "]");
return new ResponseEntity<>(removeKeys(zone), OK);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX identity_provider_alias_zid__idx ON identity_provider (alias_zid);
CREATE INDEX users_alias_zid__idx ON users (alias_zid);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX identity_provider_alias_zid__idx ON identity_provider (alias_zid);
CREATE INDEX users_alias_zid__idx ON users (alias_zid);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX CONCURRENTLY IF NOT EXISTS identity_provider_alias_zid__idx on identity_provider (alias_zid);
CREATE INDEX CONCURRENTLY IF NOT EXISTS users_alias_zid__idx on users (alias_zid);
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -69,7 +70,7 @@ void deleteProvidersInZone() {
}

@Test
void deleteByIdentityZone_ShouldAlsoDeleteAliasIdentityProviders() {
void deleteByIdentityZone_ShouldNotDeleteAliasIdentityProviders() {
final String originSuffix = generator.generate();

// IdP 1: created in custom zone, no alias
Expand Down Expand Up @@ -105,13 +106,13 @@ void deleteByIdentityZone_ShouldAlsoDeleteAliasIdentityProviders() {
// delete by zone
final int rowsDeleted = jdbcIdentityProviderProvisioning.deleteByIdentityZone(otherZoneId1);

// number should also include the alias IdP
Assertions.assertThat(rowsDeleted).isEqualTo(3);
// number should not include the alias IdP
Assertions.assertThat(rowsDeleted).isEqualTo(2);

// check if all three entries are gone
// the two IdPs in the custom zone should be deleted, the alias should still be present
assertIdentityProviderDoesNotExist(createdIdp1.getId(), otherZoneId1);
assertIdentityProviderDoesNotExist(createdIdp2.getId(), otherZoneId1);
assertIdentityProviderDoesNotExist(createdIdp2Alias.getId(), uaaZoneId);
assertIdentityProviderExists(createdIdp2Alias.getId(), uaaZoneId);
}

private void assertIdentityProviderExists(final String id, final String zoneId) {
Expand Down Expand Up @@ -317,4 +318,26 @@ void retrieveIdentityProviderByOriginInDifferentZone() {
IdentityProvider idp1 = jdbcIdentityProviderProvisioning.create(idp, otherZoneId1);
assertThrows(EmptyResultDataAccessException.class, () -> jdbcIdentityProviderProvisioning.retrieveByOrigin(idp1.getOriginKey(), otherZoneId2));
}

@Test
void testIdpWithAliasExistsInZone_TrueCase() {
final IdentityProvider<AbstractIdentityProviderDefinition> idpWithAlias = MultitenancyFixture.identityProvider(
generator.generate(),
otherZoneId1
);
idpWithAlias.setAliasZid(IdentityZone.getUaaZoneId());
idpWithAlias.setAliasId(UUID.randomUUID().toString());
jdbcIdentityProviderProvisioning.create(idpWithAlias, otherZoneId1);
assertTrue(jdbcIdentityProviderProvisioning.idpWithAliasExistsInZone(otherZoneId1));
}

@Test
void testIdpWithAliasExistsInZone_FalseCase() {
final IdentityProvider<AbstractIdentityProviderDefinition> idp = MultitenancyFixture.identityProvider(
generator.generate(),
otherZoneId2
);
jdbcIdentityProviderProvisioning.create(idp, otherZoneId2);
assertFalse(jdbcIdentityProviderProvisioning.idpWithAliasExistsInZone(otherZoneId2));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.RawExternalOAuthIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.util.AlphanumericRandomValueStringGenerator;
import org.cloudfoundry.identity.uaa.util.UaaRandomStringUtil;
import org.cloudfoundry.identity.uaa.zone.IdentityZone;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.dao.IncorrectResultSizeDataAccessException;
Expand Down Expand Up @@ -56,6 +59,8 @@
@ExtendWith(PollutionPreventionExtension.class)
@ExtendWith(MockitoExtension.class)
class ExternalOAuthProviderConfiguratorTests {
private static final AlphanumericRandomValueStringGenerator RANDOM_STRING_GENERATOR =
new AlphanumericRandomValueStringGenerator(6);

private final String UAA_BASE_URL = "https://localhost:8443/uaa";

Expand Down Expand Up @@ -386,4 +391,12 @@ void testGetIdpAuthenticationUrlAndCheckTokenFormatParameter() {
UriComponentsBuilder.fromUriString(authzUri).build().getQueryParams().toSingleValueMap();
assertThat(queryParams, hasEntry("token_format", "jwt"));
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
void testIdpWithAliasExistsInZone(final boolean resultFromDelegate) {
final String zoneId = RANDOM_STRING_GENERATOR.generate();
when(mockIdentityProviderProvisioning.idpWithAliasExistsInZone(zoneId)).thenReturn(resultFromDelegate);
assertEquals(resultFromDelegate, configurator.idpWithAliasExistsInZone(zoneId));
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
package org.cloudfoundry.identity.uaa.zone;

import org.cloudfoundry.identity.uaa.audit.event.EntityDeletedEvent;
import org.cloudfoundry.identity.uaa.error.UaaException;
import org.cloudfoundry.identity.uaa.extensions.PollutionPreventionExtension;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.provider.JdbcIdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.saml.SamlKey;
import org.cloudfoundry.identity.uaa.scim.ScimGroup;
import org.cloudfoundry.identity.uaa.scim.ScimGroupProvisioning;
import org.cloudfoundry.identity.uaa.util.AlphanumericRandomValueStringGenerator;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.ArgumentCaptor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.validation.BindingResult;

import java.util.List;
Expand Down Expand Up @@ -49,14 +55,22 @@ class IdentityZoneEndpointsTests {
private IdentityZoneValidator mockIdentityZoneValidator;

@Mock
private IdentityProviderProvisioning mockIdentityProviderProvisioning;
private JdbcIdentityProviderProvisioning mockIdentityProviderProvisioning;

@Mock
private IdentityZoneEndpointClientRegistrationService mockIdentityZoneEndpointClientRegistrationService;

@Mock
private ApplicationEventPublisher mockApplicationEventPublisher;

@InjectMocks
private IdentityZoneEndpoints endpoints;

@BeforeEach
void setUp() {
endpoints.setApplicationEventPublisher(mockApplicationEventPublisher);
}

@Test
void create_zone() throws InvalidIdentityZoneDetailsException {
when(mockIdentityZoneProvisioning.create(any())).then(invocation -> invocation.getArgument(0));
Expand Down Expand Up @@ -169,6 +183,45 @@ void reduce_zone_allowed_groups_on_update_should_fail() throws InvalidIdentityZo
is("The identity zone user configuration contains not-allowed groups."));
}

@Test
void deleteIdentityZone_ShouldReject_IfIdpWithAliasExists() {
final IdentityZone idz = new IdentityZone();
final String idzId = new AlphanumericRandomValueStringGenerator(5).generate();
idz.setName(idzId);
idz.setId(idzId);
idz.setSubdomain(idzId);
when(mockIdentityZoneProvisioning.retrieveIgnoreActiveFlag(idzId)).thenReturn(idz);

// arrange IdP with alias exists in zone
when(mockIdentityProviderProvisioning.idpWithAliasExistsInZone(idzId)).thenReturn(true);

final ResponseEntity<IdentityZone> response = endpoints.deleteIdentityZone(idzId);
assertNotNull(response);
assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, response.getStatusCode());
}

@Test
void deleteIdentityZone_ShouldEmitEntityDeletedEvent_WhenNoAliasIdpExists() {
final IdentityZone idz = new IdentityZone();
final String idzId = new AlphanumericRandomValueStringGenerator(5).generate();
idz.setName(idzId);
idz.setId(idzId);
idz.setSubdomain(idzId);
when(mockIdentityZoneProvisioning.retrieveIgnoreActiveFlag(idzId)).thenReturn(idz);

// arrange no IdP with alias exists in zone
when(mockIdentityProviderProvisioning.idpWithAliasExistsInZone(idzId)).thenReturn(false);

final ResponseEntity<IdentityZone> response = endpoints.deleteIdentityZone(idzId);
assertNotNull(response);
assertEquals(HttpStatus.OK, response.getStatusCode());

final ArgumentCaptor<EntityDeletedEvent<IdentityZone>> eventArgument = ArgumentCaptor.forClass(EntityDeletedEvent.class);
verify(mockApplicationEventPublisher).publishEvent(eventArgument.capture());
final var capturedEvent = eventArgument.getValue();
assertEquals(idz, capturedEvent.getDeleted());
}

adrianhoelzl-sap marked this conversation as resolved.
Show resolved Hide resolved
private static IdentityZone createZone() {
IdentityZone zone = MultitenancyFixture.identityZone("id", "subdomain");
IdentityZoneConfiguration config = zone.getConfig();
Expand Down
1 change: 1 addition & 0 deletions uaa/slateCustomizations/source/index.html.md.erb
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,7 @@ _Error Codes_
| 401 | Unauthorized - Invalid token |
| 403 | Forbidden - Insufficient scope (zone admins can only delete their own zone) |
| 404 | Not Found - Zone does not exist |
| 422 | Unprocessable Entity - at least one IdP with alias exists in the zone |
hsinn0 marked this conversation as resolved.
Show resolved Hide resolved
strehle marked this conversation as resolved.
Show resolved Hide resolved


# Identity Providers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@
import org.cloudfoundry.identity.uaa.client.UaaClientDetails;
import org.cloudfoundry.identity.uaa.client.event.ClientCreateEvent;
import org.cloudfoundry.identity.uaa.client.event.ClientDeleteEvent;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils;
import org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils.IdentityZoneCreationResult;
import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants;
import org.cloudfoundry.identity.uaa.oauth.provider.ClientRegistrationService;
import org.cloudfoundry.identity.uaa.provider.IdentityProvider;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.provider.JdbcIdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.scim.*;
import org.cloudfoundry.identity.uaa.scim.event.GroupModifiedEvent;
import org.cloudfoundry.identity.uaa.scim.event.UserModifiedEvent;
Expand Down Expand Up @@ -1697,6 +1699,33 @@ void testDeleteZonePublishesEvent() throws Exception {
assertThat(auditedIdentityZone, containsString(id));
}

@Test
void testDeleteZone_ShouldFail_WhenIdpWithAliasExistsInZone() throws Exception {
// create zone
final String idzId = generator.generate();
createZone(idzId, HttpStatus.CREATED, identityClientToken, new IdentityZoneConfiguration());

// create IdP with alias (via DB, since alias feature is disabled by default)
final JdbcIdentityProviderProvisioning idpProvisioning = webApplicationContext
.getBean(JdbcIdentityProviderProvisioning.class);
final IdentityProvider<OIDCIdentityProviderDefinition> idp = new IdentityProvider<>();
idp.setName("some-idp");
idp.setId(UUID.randomUUID().toString());
idp.setIdentityZoneId(idzId);
idp.setOriginKey("some-origin-key");
idp.setAliasZid(IdentityZone.getUaaZoneId());
idp.setAliasId(UUID.randomUUID().toString());
idp.setType(OriginKeys.OIDC10);
idpProvisioning.create(idp, idzId);

// deleting zone should fail
mockMvc.perform(
delete("/identity-zones/" + idzId)
.header("Authorization", "Bearer " + identityClientToken)
.accept(APPLICATION_JSON)
).andExpect(status().isUnprocessableEntity());
}

@Test
void testCreateAndDeleteLimitedClientInNewZoneUsingZoneEndpoint() throws Exception {
String id = generator.generate();
Expand Down
Loading