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

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

adrianhoelzl-sap
Copy link
Contributor

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).

@cf-gitbot
Copy link

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.

@adrianhoelzl-sap adrianhoelzl-sap marked this pull request as ready for review April 24, 2024 15:19
@adrianhoelzl-sap adrianhoelzl-sap requested a review from a team April 25, 2024 12:39
@strehle
Copy link
Member

strehle commented Apr 25, 2024

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

@bruce-ricard
Copy link
Contributor

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
Copy link
Member

strehle commented May 3, 2024

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 .

Copy link
Contributor

@Tallicia Tallicia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Contributor

@Tallicia Tallicia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Tallicia 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
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hsinn0 hsinn0 requested review from Tallicia and removed request for Tallicia May 22, 2024 22:24
@hsinn0 hsinn0 dismissed Tallicia’s stale review May 22, 2024 22:26

Taking over this PR review as Alicia is out of office

@hsinn0 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 hsinn0 removed the request for review from Tallicia May 22, 2024 22:27
Copy link
Contributor

@hsinn0 hsinn0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, assuming other concerns from @strehle has been actually resolved (despite they are not shown as resolved in this PR).

@strehle
Copy link
Member

strehle commented Jun 4, 2024

Approving, assuming other concerns from @strehle has been actually resolved (despite they are not shown as resolved in this PR).

mainly resolved with the indexed search now @adrianhoelzl-sap , please ensure happy sonar
https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2850

@strehle strehle linked an issue Jun 9, 2024 that may be closed by this pull request
…re/reject-idz-deletion-if-idp-with-alias-present

# Conflicts:
#	uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/zones/IdentityZoneEndpointsMockMvcTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending Merge | Prioritized
Development

Successfully merging this pull request may close these issues.

Missing user management from custom IdZ to CC
6 participants