Prevent Update and Delete of Entities with Alias if Alias Feature is disabled

server/src/main/java/org/cloudfoundry/identity/uaa/alias/EntityAliasHandler.java

@@ -10,7 +10,6 @@
 import org.cloudfoundry.identity.uaa.zone.ZoneDoesNotExistsException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.dao.DataAccessException;
 import org.springframework.http.HttpStatus;
 import org.springframework.lang.NonNull;
 import org.springframework.lang.Nullable;
@@ -37,8 +36,8 @@ public final boolean aliasPropertiesAreValid(
         final boolean entityAlreadyHasAlias = existingEntity != null && hasText(existingEntity.getAliasZid());
         if (entityAlreadyHasAlias) {
             if (!aliasEntitiesEnabled) {
-                // if the feature is disabled, we only allow setting both alias properties to null
-                return !hasText(requestBody.getAliasId()) && !hasText(requestBody.getAliasZid());
+                // reject ANY update of an entity with an existing alias if the feature is disabled
+                return false;
             }
 
             if (!hasText(existingEntity.getAliasId())) {
@@ -116,58 +115,31 @@ public final boolean aliasPropertiesAreValid(
      *                                    'aliasZid' does not exist
      * @throws EntityAliasFailedException if 'aliasId' and 'aliasZid' are set in the original entity, but the
      *                                    referenced alias entity could not be found
+     * @throws IllegalStateException      if {@code existingEntity} has an alias and 'aliasEntitiesEnabled' is set to
+     *                                    {@code false}
+     * @throws IllegalStateException      if a new alias is about to be created, i.e., {@code originalEntity} has a
+     *                                    non-empty 'aliasZid', and 'aliasEntitiesEnabled' is set to {@code false}
      */
     public final T ensureConsistencyOfAliasEntity(
             @NonNull final T originalEntity,
             @Nullable final T existingEntity
     ) throws EntityAliasFailedException {
-        /* If the entity had an alias before the update and the alias feature is now turned off, we break the reference
-         * between the entity and its alias by setting aliasId and aliasZid to null for both of them. Then, all other
-         * changes are only applied to the original entity. */
         final boolean entityHadAlias = existingEntity != null && hasText(existingEntity.getAliasZid());
-        final boolean referenceBreakRequired = entityHadAlias && !aliasEntitiesEnabled;
-        if (referenceBreakRequired) {
-            if (!hasText(existingEntity.getAliasId())) {
-                LOGGER.warn(
-                        "The state of the entity {} before the update had an aliasZid set, but no aliasId.",
-                        existingEntity.getAliasDescription()
-                );
-                return originalEntity;
-            }
-
-            final Optional<T> aliasEntityOpt = retrieveAliasEntity(existingEntity);
-            if (aliasEntityOpt.isEmpty()) {
-                LOGGER.warn(
-                        "The alias referenced in entity {} does not exist, therefore cannot break reference.",
-                        existingEntity.getAliasDescription()
-                );
-                return originalEntity;
-            }
-
-            final T aliasEntity = aliasEntityOpt.get();
-            aliasEntity.setAliasId(null);
-            aliasEntity.setAliasZid(null);
-
-            try {
-                updateEntity(aliasEntity, aliasEntity.getZoneId());
-            } catch (final DataAccessException e) {
-                throw new EntityAliasFailedException(
-                        String.format(
-                                "Could not break reference to alias in entity %s.",
-                                existingEntity.getAliasDescription()
-                        ), HttpStatus.UNPROCESSABLE_ENTITY.value(), e
-                );
-            }
-
-            // no change required in the original entity since its aliasId and aliasZid were already set to null
-            return originalEntity;
+        if (entityHadAlias && !aliasEntitiesEnabled) {
+            // this should already be caught in the validation method
+            throw new IllegalStateException("Performing update on entity with alias while alias feature is disabled.");
         }
 
         if (!hasText(originalEntity.getAliasZid())) {
             // no alias handling is necessary
             return originalEntity;
         }
 
+        if (!aliasEntitiesEnabled) {
+            // this should already be caught in the validation method
+            throw new IllegalStateException("Trying to create a new alias while alias feature is disabled.");
+        }
+
         final T aliasEntity = buildAliasEntity(originalEntity);
 
         // get the existing alias entity, if present
@@ -225,6 +197,9 @@ private T buildAliasEntity(final T originalEntity) {
     protected abstract T cloneEntity(final T originalEntity);
 
     public final Optional<T> retrieveAliasEntity(final T originalEntity) {
+        if (!hasText(originalEntity.getAliasId()) || !hasText(originalEntity.getAliasZid())) {
+            return Optional.empty();
+        }
         return retrieveEntity(originalEntity.getAliasId(), originalEntity.getAliasZid());
     }
 

server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java

@@ -185,11 +185,19 @@ public ResponseEntity<IdentityProvider> 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());
+        if (idpHasAlias && !aliasEntitiesEnabled) {
+            return new ResponseEntity<>(UNPROCESSABLE_ENTITY);
+        }
+
+        // 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<IdentityProvider<?>> aliasIdpOpt = idpAliasHandler.retrieveAliasEntity(existing);
             if (aliasIdpOpt.isEmpty()) {
                 // ignore dangling reference to alias
@@ -202,16 +210,6 @@ public ResponseEntity<IdentityProvider> 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));
         }

server/src/test/java/org/cloudfoundry/identity/uaa/alias/EntityAliasHandlerValidationTest.java

@@ -87,7 +87,7 @@ protected final boolean isAliasFeatureEnabled() {
         }
 
         @Test
-        final void shouldReturnFalse_NotBothAliasPropsEmptyInReqBody() {
+        final void shouldReturnFalse_UpdatesOfEntitiesWithExistingAliasForbidden() {
             final String initialAliasId = UUID.randomUUID().toString();
             final String initialAliasZid = CUSTOM_ZONE_ID;
 
@@ -124,16 +124,10 @@ final void shouldReturnFalse_NotBothAliasPropsEmptyInReqBody() {
             // (8) alias ID removed, alias ZID changed
             requestBody = buildEntityWithAliasProps(null, "some-other-zid");
             assertThat(aliasHandler.aliasPropertiesAreValid(requestBody, existingEntity)).isFalse();
-        }
 
-        @Test
-        final void shouldReturnTrue_BothAliasPropsEmptyInReqBody() {
-            final T existingEntity = buildEntityWithAliasProps(
-                    UUID.randomUUID().toString(),
-                    CUSTOM_ZONE_ID
-            );
-            final T requestBody = buildEntityWithAliasProps(null, null);
-            assertThat(aliasHandler.aliasPropertiesAreValid(requestBody, existingEntity)).isTrue();
+            // (9) alias ID removed, alias ZID removed
+            requestBody = buildEntityWithAliasProps(null, null);
+            assertThat(aliasHandler.aliasPropertiesAreValid(requestBody, existingEntity)).isFalse();
         }
     }
 

server/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderAliasHandlerEnsureConsistencyTest.java

@@ -2,6 +2,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.cloudfoundry.identity.uaa.constants.OriginKeys.OIDC10;
 import static org.cloudfoundry.identity.uaa.constants.OriginKeys.UAA;
 import static org.mockito.ArgumentMatchers.any;
@@ -24,7 +25,6 @@
 import org.mockito.ArgumentMatcher;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
-import org.springframework.dao.EmptyResultDataAccessException;
 import org.springframework.test.util.ReflectionTestUtils;
 
 @ExtendWith(MockitoExtension.class)
@@ -202,50 +202,7 @@ void setUp() {
             }
 
             @Test
-            void shouldIgnoreDanglingReferenceInExistingEntity_AliasIdEmpty() {
-                final IdentityProvider<?> existingIdp = new IdentityProvider<>();
-                existingIdp.setType(OIDC10);
-                existingIdp.setId(UUID.randomUUID().toString());
-                existingIdp.setIdentityZoneId(UAA);
-                existingIdp.setAliasId(null); // dangling reference: aliasId empty
-                existingIdp.setAliasZid(customZoneId);
-
-                final IdentityProvider<?> originalIdp = shallowCloneIdp(existingIdp);
-                originalIdp.setAliasId(null);
-                originalIdp.setAliasZid(null);
-
-                // should ignore dangling reference
-                assertThat(idpAliasHandler.ensureConsistencyOfAliasEntity(existingIdp, existingIdp))
-                        .isEqualTo(existingIdp);
-            }
-
-            @Test
-            void shouldIgnoreDanglingReference_AliasNotFound() {
-                final String idpId = UUID.randomUUID().toString();
-                final String aliasIdpId = UUID.randomUUID().toString();
-
-                final IdentityProvider<?> existingIdp = new IdentityProvider<>();
-                existingIdp.setType(OIDC10);
-                existingIdp.setId(idpId);
-                existingIdp.setIdentityZoneId(UAA);
-                existingIdp.setAliasId(aliasIdpId);
-                existingIdp.setAliasZid(customZoneId);
-
-                final IdentityProvider<?> originalIdp = shallowCloneIdp(existingIdp);
-                originalIdp.setAliasId(null);
-                originalIdp.setAliasZid(null);
-
-                // dangling reference: alias IdP does not exist
-                when(identityProviderProvisioning.retrieve(aliasIdpId, customZoneId))
-                        .thenThrow(new EmptyResultDataAccessException(1));
-
-                // should ignore dangling reference
-                assertThat(idpAliasHandler.ensureConsistencyOfAliasEntity(existingIdp, existingIdp))
-                        .isEqualTo(existingIdp);
-            }
-
-            @Test
-            void shouldBreakReferenceInAliasIdp() {
+            void shouldThrow_IfExistingEntityHasAlias() {
                 final String idpId = UUID.randomUUID().toString();
                 final String aliasIdpId = UUID.randomUUID().toString();
 
@@ -259,22 +216,11 @@ void shouldBreakReferenceInAliasIdp() {
                 final IdentityProvider<?> originalIdp = shallowCloneIdp(existingIdp);
                 originalIdp.setAliasId(null);
                 originalIdp.setAliasZid(null);
+                originalIdp.setName("some-new-name");
 
-                final IdentityProvider<?> aliasIdp = shallowCloneIdp(existingIdp);
-                aliasIdp.setAliasId(idpId);
-                aliasIdp.setAliasZid(UAA);
-                aliasIdp.setIdentityZoneId(customZoneId);
-                aliasIdp.setId(aliasIdpId);
-                when(identityProviderProvisioning.retrieve(aliasIdpId, customZoneId)).thenReturn(aliasIdp);
-
-                idpAliasHandler.ensureConsistencyOfAliasEntity(originalIdp, existingIdp);
-
-                final IdentityProvider<?> aliasIdpWithEmptyAliasProps = shallowCloneIdp(aliasIdp);
-                aliasIdpWithEmptyAliasProps.setAliasZid(null);
-                aliasIdpWithEmptyAliasProps.setAliasId(null);
-
-                // should break reference in alias IdP
-                verify(identityProviderProvisioning).update(aliasIdpWithEmptyAliasProps, customZoneId);
+                assertThatIllegalStateException().isThrownBy(() ->
+                        idpAliasHandler.ensureConsistencyOfAliasEntity(originalIdp, existingIdp)
+                ).withMessage("Performing update on entity with alias while alias feature is disabled.");
             }
         }
     }

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<?>, 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<EntityDeletedEvent<?>> 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<IdentityProvider> 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<IdentityProvider> 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.UNPROCESSABLE_ENTITY);
             }
 
             private Pair<IdentityProvider<?>, IdentityProvider<?>> arrangeIdpWithAliasExists(final String zone1Id, final String zone2Id) {
@@ -734,7 +711,7 @@ private Pair<IdentityProvider<?>, 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);
             }

uaa/slateCustomizations/source/index.html.md.erb

@@ -1173,10 +1173,10 @@ _Request and Response Fields_
 
 _Error Codes_
 
-| Error Code | Description                                                           |
-|------------|-----------------------------------------------------------------------|
-| 403        | Forbidden - Insufficient scope                                        |
-| 422        | Unprocessable Entity - Invalid config                                 |
+| Error Code | Description                                                                                            |
+|------------|--------------------------------------------------------------------------------------------------------|
+| 403        | Forbidden - Insufficient scope                                                                         |
+| 422        | Unprocessable Entity - Invalid config or updating IdP with alias while `aliasEntitiesEnabled` is false |
 
 ## Delete
 
@@ -1202,10 +1202,10 @@ _Response Fields_
 
 _Error Codes_
 
-| Error Code | Description                                                           |
-|------------|-----------------------------------------------------------------------|
-| 403        | Forbidden - Insufficient scope                                        |
-| 422        | Unprocessable Entity                                                  |
+| Error Code | Description                                                                                |
+|------------|--------------------------------------------------------------------------------------------|
+| 403        | Forbidden - Insufficient scope                                                             |
+| 422        | Unprocessable Entity (e.g., deleting IdP with alias while `aliasEntitiesEnabled` is false) |
 
 ## Force password change for Users
 <%= render('IdentityProviderEndpointDocs/patchIdentityProviderStatus/curl-request.md') %>