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 5 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 @@ -40,7 +40,7 @@ public class JdbcIdentityProviderProvisioning implements IdentityProviderProvisi

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 Down Expand Up @@ -150,12 +150,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 @@ -336,7 +336,18 @@ 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 List<IdentityProvider> idps = idpDao.retrieveAll(false, zone.getId());
hsinn0 marked this conversation as resolved.
Show resolved Hide resolved
strehle marked this conversation as resolved.
Show resolved Hide resolved
final boolean idpWithAliasExists = idps.stream()
.map(IdentityProvider::getAliasZid)
.anyMatch(UaaStringUtils::isNotEmpty);
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
Expand Up @@ -69,7 +69,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 +105,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
Original file line number Diff line number Diff line change
@@ -1,23 +1,32 @@
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.IdentityProvider;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
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.Collections;
import java.util.List;
import java.util.stream.Collectors;

import static org.cloudfoundry.identity.uaa.constants.OriginKeys.UAA;
import static org.cloudfoundry.identity.uaa.util.AssertThrowsWithMessage.assertThrowsWithMessageThat;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
Expand Down Expand Up @@ -54,9 +63,17 @@ class IdentityZoneEndpointsTests {
@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 +186,53 @@ 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
final IdentityProvider idpWithoutAlias = mock(IdentityProvider.class);
when(idpWithoutAlias.getAliasZid()).thenReturn("");
final IdentityProvider idpWithAlias = mock(IdentityProvider.class);
when(idpWithAlias.getAliasZid()).thenReturn(UAA);
when(mockIdentityProviderProvisioning.retrieveAll(false, idzId))
.thenReturn(List.of(idpWithoutAlias, idpWithAlias));

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
final IdentityProvider idpWithoutAlias = mock(IdentityProvider.class);
when(idpWithoutAlias.getAliasZid()).thenReturn("");
when(mockIdentityProviderProvisioning.retrieveAll(false, idzId))
.thenReturn(Collections.singletonList(idpWithoutAlias));

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
Loading