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

fix client admin rest call with empty secret - alternative PR #2572

Closed
wants to merge 6 commits into from

Conversation

strehle
Copy link
Member

@strehle strehle commented Oct 24, 2023

This PR simply uses PasswordValidatorUtil but set the password minLength to 1 which was the default because it was in code The minLength for clients should be 0 and this was the case before

Same Tests as before, means, with Default Policy the empty secret is allowed, with the strictPolicy not, see testEmptyClientSecret

This PR simply uses PasswordValidatorUtil but set the password
minLength to 1 which was the default because it was in code
The minLength for clients should be 0 and this was the case
before

Same Tests as before, means, with Default Policy the
empty secret is allowed, with the strictPolicy not, see
testEmptyClientSecret
@cf-gitbot
Copy link

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

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

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

@strehle
Copy link
Member Author

strehle commented Oct 24, 2023

After thinking about I see that minLength for passwords and secrets is 0 by default so then I would go with it, if you have no concerns and completly rely on the PasswordValidor rules, means I will update again to minLength:0

Documentation allows minLength with 0 even for passwords
Copy link
Contributor

@bruce-ricard bruce-ricard left a comment

Choose a reason for hiding this comment

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

Thanks Markus. This is IMO a lot better. I just have a couple small comments.

@strehle strehle requested a review from bruce-ricard October 24, 2023 21:17
@peterhaochen47
Copy link
Member

This PR is indeed simpler than the first #2571, but it does change the current behavior of UAA (and the 4 failing tests, before this commit was made to remove/modify the existing tests, are evidence). The behavior in question (not allowing user password to be an empty string) seems contradictory to our uaa-release doc, but its addition seems deliberate based on the original tracker story associated with this restriction. Hence, I suspect that this change (removing the restriction) might introduce a new regression (possibly the one that the original tracker story tries to address).

@strehle
Copy link
Member Author

strehle commented Oct 25, 2023

close this , now, was related to #2570 but no successful tests

@strehle strehle closed this Oct 25, 2023
@strehle strehle deleted the fix/issue/2570-small branch October 25, 2023 18:38
@cf-gitbot cf-gitbot added accepted Accepted the issue and removed delivered labels Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the issue
Projects
Development

Successfully merging this pull request may close these issues.

4 participants