diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/alias/EntityAliasHandler.java b/server/src/main/java/org/cloudfoundry/identity/uaa/alias/EntityAliasHandler.java index 6636310f3ae..02ab73c5e98 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/alias/EntityAliasHandler.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/alias/EntityAliasHandler.java @@ -225,6 +225,9 @@ private T buildAliasEntity(final T originalEntity) { protected abstract T cloneEntity(final T originalEntity); public final Optional retrieveAliasEntity(final T originalEntity) { + if (!hasText(originalEntity.getAliasId()) || !hasText(originalEntity.getAliasZid())) { + return Optional.empty(); + } return retrieveEntity(originalEntity.getAliasId(), originalEntity.getAliasZid()); } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java index c83d415dd4c..4eed2f59592 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java @@ -185,11 +185,19 @@ public ResponseEntity deleteIdentityProvider(@PathVariable Str return new ResponseEntity<>(UNPROCESSABLE_ENTITY); } + // reject deletion if the IdP has an alias, but alias feature is disabled + final boolean idpHasAlias = hasText(existing.getAliasZid()) || hasText(existing.getAliasId()); + if (idpHasAlias && !aliasEntitiesEnabled) { + return new ResponseEntity<>(BAD_REQUEST); + } + + // delete the IdP existing.setSerializeConfigRaw(rawConfig); publisher.publishEvent(new EntityDeletedEvent<>(existing, authentication, identityZoneId)); redactSensitiveData(existing); - if (hasText(existing.getAliasZid()) && hasText(existing.getAliasId())) { + // delete the alias IdP if present + if (idpHasAlias) { final Optional> aliasIdpOpt = idpAliasHandler.retrieveAliasEntity(existing); if (aliasIdpOpt.isEmpty()) { // ignore dangling reference to alias @@ -202,16 +210,6 @@ public ResponseEntity deleteIdentityProvider(@PathVariable Str } final IdentityProvider aliasIdp = aliasIdpOpt.get(); - if (!aliasEntitiesEnabled) { - // if alias entities are not enabled, just break the reference - aliasIdp.setAliasId(null); - aliasIdp.setAliasZid(null); - identityProviderProvisioning.update(aliasIdp, aliasIdp.getIdentityZoneId()); - - return new ResponseEntity<>(existing, OK); - } - - // also delete the alias IdP aliasIdp.setSerializeConfigRaw(rawConfig); publisher.publishEvent(new EntityDeletedEvent<>(aliasIdp, authentication, identityZoneId)); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpointsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpointsTest.java index 9c7b20c0e3c..7bf03cad3e4 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpointsTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpointsTest.java @@ -670,45 +670,22 @@ void testDeleteIdpWithAlias_DanglingReference() { void testDeleteIdpWithAlias_AliasFeatureDisabled() { arrangeAliasEntitiesEnabled(false); + // ensure event publisher is present + final ApplicationEventPublisher mockEventPublisher = mock(ApplicationEventPublisher.class); + identityProviderEndpoints.setApplicationEventPublisher(mockEventPublisher); + // arrange IdP with alias exists final String customZoneId = UUID.randomUUID().toString(); final Pair, IdentityProvider> idpAndAlias = arrangeIdpWithAliasExists(UAA, customZoneId); final IdentityProvider idp = idpAndAlias.getLeft(); - final IdentityProvider aliasIdp = idpAndAlias.getRight(); - - final ApplicationEventPublisher mockEventPublisher = mock(ApplicationEventPublisher.class); - identityProviderEndpoints.setApplicationEventPublisher(mockEventPublisher); - doNothing().when(mockEventPublisher).publishEvent(any()); - identityProviderEndpoints.deleteIdentityProvider(idp.getId(), true); - - // the original IdP should be deleted - final ArgumentCaptor> entityDeletedEventCaptor = ArgumentCaptor.forClass(EntityDeletedEvent.class); - verify(mockEventPublisher, times(1)).publishEvent(entityDeletedEventCaptor.capture()); - final EntityDeletedEvent event = entityDeletedEventCaptor.getValue(); - Assertions.assertThat(event).isNotNull(); - Assertions.assertThat(event.getIdentityZoneId()).isEqualTo(UAA); - Assertions.assertThat(((IdentityProvider) event.getSource()).getId()).isEqualTo(idp.getId()); - - // instead of being deleted, the alias IdP should just have its reference to the original IdP removed - final ArgumentCaptor updateIdpParamCaptor = ArgumentCaptor.forClass(IdentityProvider.class); - verify(mockIdentityProviderProvisioning).update(updateIdpParamCaptor.capture(), eq(customZoneId)); - final IdentityProvider updateIdpParam = updateIdpParamCaptor.getValue(); - Assertions.assertThat(updateIdpParam).isNotNull(); - Assertions.assertThat(updateIdpParam.getAliasId()).isBlank(); - Assertions.assertThat(updateIdpParam.getAliasZid()).isBlank(); - assertIdpsAreEqualApartFromAliasProperties(updateIdpParam, aliasIdp); - } + final ResponseEntity response = identityProviderEndpoints.deleteIdentityProvider( + idp.getId(), + true + ); - private static void assertIdpsAreEqualApartFromAliasProperties( - final IdentityProvider idp1, - final IdentityProvider idp2 - ) { - idp2.setAliasId(null); - idp1.setAliasId(null); - idp2.setAliasZid(null); - idp1.setAliasZid(null); - Assertions.assertThat(idp1).isEqualTo(idp2); + // deletion should be rejected + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); } private Pair, IdentityProvider> arrangeIdpWithAliasExists(final String zone1Id, final String zone2Id) { @@ -734,7 +711,7 @@ private Pair, IdentityProvider> arrangeIdpWithAliasExists aliasIdp.setIdentityZoneId(zone2Id); aliasIdp.setAliasId(idpId); aliasIdp.setAliasZid(zone1Id); - when(mockIdpAliasHandler.retrieveAliasEntity(idp)).thenReturn(Optional.of(aliasIdp)); + lenient().when(mockIdpAliasHandler.retrieveAliasEntity(idp)).thenReturn(Optional.of(aliasIdp)); return Pair.of(idp, aliasIdp); } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java index 8dbc2863f37..7063db5e278 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java @@ -1091,6 +1091,13 @@ public DeleteBase(final boolean aliasFeatureEnabled) { void setUp() { arrangeAliasFeatureEnabled(aliasFeatureEnabled); } + } + + @Nested + class AliasFeatureEnabled extends DeleteBase { + public AliasFeatureEnabled() { + super(true); + } @Test void shouldIgnoreDanglingReferenceToAliasIdp_UaaToCustomZone() throws Throwable { @@ -1102,7 +1109,10 @@ void shouldIgnoreDanglingReferenceToAliasIdp_CustomToUaaZone() throws Throwable shouldIgnoreDanglingReferenceToAliasIdp(customZone, IdentityZone.getUaa()); } - private void shouldIgnoreDanglingReferenceToAliasIdp(final IdentityZone zone1, final IdentityZone zone2) throws Throwable { + private void shouldIgnoreDanglingReferenceToAliasIdp( + final IdentityZone zone1, + final IdentityZone zone2 + ) throws Throwable { final IdentityProvider originalIdp = executeWithTemporarilyEnabledAliasFeature( aliasFeatureEnabled, () -> createIdpWithAlias(zone1, zone2) @@ -1155,21 +1165,6 @@ private void deletionWithExistingAliasIdp( final MvcResult deleteResult = deleteIdpAndReturnResult(zone1, id); assertThat(deleteResult.getResponse().getStatus()).isEqualTo(HttpStatus.OK.value()); - // alias IdP should still exist, but without reference to original IdP - assertAliasIdpAfterDeletion(aliasId, aliasZid); - } - - protected abstract void assertAliasIdpAfterDeletion(final String aliasId, final String aliasZid) throws Exception; - } - - @Nested - class AliasFeatureEnabled extends DeleteBase { - public AliasFeatureEnabled() { - super(true); - } - - @Override - protected void assertAliasIdpAfterDeletion(final String aliasId, final String aliasZid) throws Exception { // if the alias feature is enabled, the alias should also be removed assertIdpDoesNotExist(aliasId, aliasZid); } @@ -1181,13 +1176,43 @@ public AliasFeatureDisabled() { super(false); } - @Override - protected void assertAliasIdpAfterDeletion(final String aliasId, final String aliasZid) throws Exception { - // if the alias feature is disabled, only the reference should be removed from the alias IdP - assertReferenceWasRemovedFromAlias(aliasId, aliasZid); + @Test + void shouldRejectDeletion_WhenAliasIdpExists_UaaToCustomZone() throws Throwable { + shouldRejectDeletion_WhenAliasIdpExists(IdentityZone.getUaa(), customZone); + } + + @Test + void shouldRejectDeletion_WhenAliasIdpExists_CustomToUaaZone() throws Throwable { + shouldRejectDeletion_WhenAliasIdpExists(customZone, IdentityZone.getUaa()); } - } + private void shouldRejectDeletion_WhenAliasIdpExists( + final IdentityZone zone1, + final IdentityZone zone2 + ) throws Throwable { + // create IdP in zone 1 with alias in zone 2 + final IdentityProvider idpInZone1 = executeWithTemporarilyEnabledAliasFeature( + aliasFeatureEnabled, + () -> createIdpWithAlias(zone1, zone2) + ); + final String id = idpInZone1.getId(); + assertThat(id).isNotBlank(); + final String aliasId = idpInZone1.getAliasId(); + assertThat(aliasId).isNotBlank(); + final String aliasZid = idpInZone1.getAliasZid(); + assertThat(aliasZid).isNotBlank().isEqualTo(zone2.getId()); + + // check if alias IdP is available in zone 2 + final Optional> aliasIdp = readIdpFromZoneIfExists(zone2.getId(), aliasId); + assertThat(aliasIdp).isPresent(); + assertThat(aliasIdp.get().getAliasId()).isNotBlank().isEqualTo(id); + assertThat(aliasIdp.get().getAliasZid()).isNotBlank().isEqualTo(idpInZone1.getIdentityZoneId()); + + // delete IdP in zone 1 -> should be rejected since alias feature is disabled + final MvcResult deleteResult = deleteIdpAndReturnResult(zone1, id); + assertThat(deleteResult.getResponse().getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + } + } private MvcResult deleteIdpAndReturnResult(final IdentityZone zone, final String id) throws Exception { final String accessTokenForZone1 = getAccessTokenForZone(zone.getId());