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

adrianhoelzl-sap · 2024-04-24T15:15:30Z

see issue #2505

Reject deletion if an IdP with alias exists in the zone to be deleted. With this restriction, we want to avoid that callers of the endpoint accidentally remove IdPs (and thereby the associated users) from the alias zone. Now the callers must actively delete the IdPs with alias beforehand (which will also remove all associated users, their alias users and the alias IdP in the "uaa" zone).

…leteByIdentityZone

cf-gitbot · 2024-04-24T15:15:37Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187492139

The labels on this github issue will be updated when the story is started.

strehle · 2024-04-25T12:47:24Z

Sonar issues should be solved @adrianhoelzl-sap
https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2663

bruce-ricard · 2024-05-02T21:53:04Z

This appears to be a pretty major feature change. It looks like we are trying to merge this to develop, which would imply that it's going to be shipped in a new minor release in a few weeks.

It seems to me like this should be in a major release. I am uncertain of the ramifications of this change. The code was clearly set to do the opposite of what you are trying to do.

strehle · 2024-05-03T10:53:12Z

I had not checked the logic before only code style and sonar issues, but now I see the impact because @bruce-ricard comments.

So I do not see a need for this PR. If you have the power to delete a zone , then we should not prevent it because some values in the zone are not as expected.

I do not understand the relation to mentioned issue #2505 .

Tallicia

Looks good

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

server/src/test/java/org/cloudfoundry/identity/uaa/zone/IdentityZoneEndpointsTests.java

Tallicia

Un-approving this following review of the comments. The change appears to break existing behavior. I thought it was a fix, not a change.

server/src/main/java/org/cloudfoundry/identity/uaa/zone/IdentityZoneEndpoints.java

strehle

I know #2505 but during the review process we defined that this new feature should come with extra options, but for all others UAA should not change.

We have an option for this, e.g. aliasEntitiesEnabled and therefore with this aliasEntitiesEnabled = false there should not be any differences to UAAs before

strehle · 2024-05-07T18:32:31Z

server/src/main/java/org/cloudfoundry/identity/uaa/zone/IdentityZoneEndpoints.java

+
+            /* 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());


Checked again and I think we should add here check for aliasEntitiesEnabled = true then you can do this extra check and return 422.

Btw. I search a while for aliasEntitiesEnabled in https://github.com/cloudfoundry/uaa-release , e.g. https://github.com/cloudfoundry/uaa-release/blob/develop/spec/input/all-properties-set.yml for this parameter and its documentation, but did not found it.
So we should add it to https://github.com/cloudfoundry/uaa-release/blob/develop/spec/input/all-properties-set.yml and with this parameter we can do some flows in UAA different ,
But for all CF usages without alias entry in IdP or User this SELECT is done

We must also perform this check when aliasEntitiesEnabled is false, since it is possible to enable it, create some entities with alias and then disable it again. At this point, there would still be entities with alias left in the DB which should block the deletion of the zone.

strehle · 2024-05-07T18:33:19Z

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

@@ -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       |


Unprocessable Entity (e.g., deleting IdP with alias while aliasEntitiesEnabled is true)

Found similar fragments in https://docs.cloudfoundry.org/api/uaa/version/77.8.0/index.html#delete

see my comment above - here, the flag is not relevant

strehle · 2024-05-07T18:35:12Z

...r/src/main/java/org/cloudfoundry/identity/uaa/provider/JdbcIdentityProviderProvisioning.java

@@ -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=?";


see now that this is a fix but the issue was
https://github.com/cloudfoundry/uaa/pull/2637/files#diff-8e7d64a1881ae78ed33d745ed1fd3a922312b89e740a3e1c75d1284b6a041b31R43

Was this concern from @strehle ever addressed?

We revert the SQL back to the state it was in before the alias feature for identity providers was integrated. This means that during the deletion of a zone, we no longer delete all identity providers in other zones that have an alias to the deleted zone.
Instead, we reject the deletion of the zone if identity providers with an alias exist in the zone to be deleted and thereby force the administrator to delete these IdPs beforehand (which will then also delete the users associated to the original and alias IdPs).

…ists in zone

…oning method

…lias-present

hsinn0 · 2024-05-22T21:40:09Z

...r/src/main/java/org/cloudfoundry/identity/uaa/provider/JdbcIdentityProviderProvisioning.java

@@ -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=?";


Was this concern from @strehle ever addressed?

hsinn0 · 2024-05-22T21:54:32Z

...r/src/main/java/org/cloudfoundry/identity/uaa/provider/JdbcIdentityProviderProvisioning.java

+        }
+        return numberOfIdpsWithAlias > 0;
+    }
+


Why do you do count query instead of exists query here?

I tried to use exists in two different ways:

VALUES (EXISTS (SELECT ...)) (see e19a904#diff-8e7d64a1881ae78ed33d745ed1fd3a922312b89e740a3e1c75d1284b6a041b31R39) → supported by PostgreSQL and HSQL, but not MySQL

SELECT EXISTS (SELECT ...) → supported by PostgreSQL and MySQL, but not HSQL

Therefore, I decided to use the count function.

Should I add a database migration that creates an index for the alias_zid column?

I tried to use exists in two different ways:
...
Therefore, I decided to use the count function.

I see. HSQL is the hindrance here. In that case, how about using limit clause as follows?
select 1 from identity_provider idp where idp.identity_zone_id = ? and idp.alias_zid is not null and idp.alias_zid <> '' limit 1
That would return 1 if the record exists or null otherwise. It should be better than select count(*).

Then you can change your Java code to return as follows (no need for additional check for null):
return 1 == idpWithAliasExists;

Taking over this PR review as Alicia is out of office

adrianhoelzl-sap added 3 commits April 24, 2024 16:38

Remove deletion of alias IdP from JdbcIdentityProviderProvisioning.de…

b51e355

…leteByIdentityZone

Reject deletion of identity zone if an IdP with alias exists in the zone

5a7fd8c

Add new status code to identity zone deletion documentation

a6966f8

cf-gitbot added the unscheduled label Apr 24, 2024

adrianhoelzl-sap mentioned this pull request Apr 24, 2024

Alias ID and Alias ZID for Users #2663

Open

adrianhoelzl-sap marked this pull request as ready for review April 24, 2024 15:19

adrianhoelzl-sap requested a review from a team April 25, 2024 12:39

adrianhoelzl-sap added 2 commits April 25, 2024 16:04

Fix Sonar: remove unnecessary clause in if statement

ce46e69

Add unit test for zone deletion success case

b74a620

Tallicia approved these changes May 7, 2024

View reviewed changes

uaa/slateCustomizations/source/index.html.md.erb Show resolved Hide resolved

server/src/test/java/org/cloudfoundry/identity/uaa/zone/IdentityZoneEndpointsTests.java Show resolved Hide resolved

Tallicia previously requested changes May 7, 2024

View reviewed changes

Tallicia added in progress clarification needed The issue is not accepted but we need clarification DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels May 7, 2024

strehle reviewed May 7, 2024

View reviewed changes

server/src/main/java/org/cloudfoundry/identity/uaa/zone/IdentityZoneEndpoints.java Outdated Show resolved Hide resolved

strehle reviewed May 7, 2024

View reviewed changes

adrianhoelzl-sap added 5 commits May 15, 2024 09:50

Add MockMVC test: deletion of zone should fail when IdP with alias ex…

cda7f1c

…ists in zone

Add method idpWithAliasExistsInZone to JdbcIdentityProviderProvisioning

e19a904

Replace filtering for IdP with alias in Java code with new IdpProvisi…

59905c1

…oning method

Merge branch 'develop' into feature/reject-idz-deletion-if-idp-with-a…

887239d

…lias-present

Fix query in JdbcIdentityProviderProvisioning.idpWithAliasExistsInZone

4f05c87

adrianhoelzl-sap requested a review from strehle May 15, 2024 13:22

hsinn0 reviewed May 22, 2024

View reviewed changes

hsinn0 requested review from Tallicia and removed request for Tallicia May 22, 2024 22:24

hsinn0 removed in progress clarification needed The issue is not accepted but we need clarification DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels May 22, 2024

hsinn0 removed the request for review from Tallicia May 22, 2024 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adrianhoelzl-sap commented Apr 24, 2024

cf-gitbot commented Apr 24, 2024

strehle commented Apr 25, 2024

bruce-ricard commented May 2, 2024

strehle commented May 3, 2024

Tallicia left a comment

Tallicia left a comment

strehle left a comment

strehle May 7, 2024

adrianhoelzl-sap May 8, 2024

strehle May 7, 2024

adrianhoelzl-sap May 8, 2024

strehle May 7, 2024

hsinn0 May 22, 2024

adrianhoelzl-sap May 29, 2024

hsinn0 May 22, 2024

hsinn0 May 22, 2024

adrianhoelzl-sap May 29, 2024

adrianhoelzl-sap May 29, 2024

hsinn0 May 29, 2024

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

Are you sure you want to change the base?

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

Conversation

adrianhoelzl-sap commented Apr 24, 2024

cf-gitbot commented Apr 24, 2024

strehle commented Apr 25, 2024

bruce-ricard commented May 2, 2024

strehle commented May 3, 2024

Tallicia left a comment

Choose a reason for hiding this comment

Tallicia left a comment

Choose a reason for hiding this comment

strehle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment