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 password validator for short domains #10201

Merged
merged 4 commits into from Feb 21, 2023

Conversation

JoonasAapro
Copy link
Contributor

🎩 What? Why?

Decidim's password validator had been updated, which caused some changes in the method of checking the similarity with the user's email address' domain. This caused problems with registration if the user had an email address with a short domain.
For example if the email was "example@1.lvh.me", the validator would pick the first part of the domain, which in this case is "1", and check if your password had a string that matches the domain's first part, so even a password like "RndmG3951984" would fail.
After this fix the validator checks all parts of the email domain - In this example "1", "lvh" and "me"
and compares the length of the strings to a configurable number (default "4"). If the part of the domain is shorter than the number set, the validator ignores it, in this example it wouldn't validate any part of the domain with the default value.
If the domain is for example "@decidim.com" and you try to register with a password like "eX4mpl3DeCiDim", it will tell you it's too similar with the email, the validation is not case sensitive.

The domain length check was already in use with another method which compared the password to the site's domain. The checker has now been moved to it's own method that is used in both validations - email domain and site's domain. The length variable has also been made configurable

Testing

To reproduce the bug in Decidim, go to the "Sign up" -page, fill in the user information. In email field put "1.lvh.me" for example, and try to make a password containing the number "1", this will fail.

♥️ Thank you!

@JoonasAapro JoonasAapro changed the title Fix/password validator Fixed password validator for short domains Jan 5, 2023
@alecslupu alecslupu added this to Pending review from Product in Maintainers via automation Jan 20, 2023
@alecslupu alecslupu added module: core type: fix PRs that implement a fix for a bug labels Jan 20, 2023
@alecslupu alecslupu requested a review from a team February 7, 2023 21:32
@alecslupu
Copy link
Contributor

Tested on local, and i could confirm the fix.

i have tried with the following credentials :
system@1.example.tld => password1password1 (initially i got the error )

After Applying the patch,

system@1.example.tld => password1password1 (worked )
test@1.example.tld => password1tespassword1tes (worked)
test@1.example.tld => password1testpassword1test (successfully seen the error message "to similar....")

@alecslupu alecslupu added type: change PRs that implement a change for an existing feature and removed type: fix PRs that implement a fix for a bug labels Feb 7, 2023
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

@JoonasAapro I have changed this PR to a change since you are adding a new parameter.
The functionality works as expected, but, in order to merge this PR, we would need a few words about this change in the RELEASE_NOTES.md file

I would say that this section from RELEASE_NOTES.md is a good example on how structure your data.

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Hi @JoonasAapro, welcome to Decidim!
Great PR and fix 👏🏽 👏🏽
I found a couple of changes to be made and that'd be good to go for me.

decidim-core/spec/validators/password_validator_spec.rb Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
@alecslupu alecslupu self-assigned this Feb 14, 2023
RELEASE_NOTES.md Outdated Show resolved Hide resolved
decidim-core/spec/validators/password_validator_spec.rb Outdated Show resolved Hide resolved
@andreslucena andreslucena changed the title Fixed password validator for short domains Fix password validator for short domains Feb 15, 2023
@alecslupu alecslupu dismissed andreslucena’s stale review February 21, 2023 07:27

I am dismissing Andres's review in order to merge, as the changes requested have been implemented.

@alecslupu alecslupu merged commit 41ceb47 into decidim:develop Feb 21, 2023
Maintainers automation moved this from Pending review from Product to Done Feb 21, 2023
@ahukkanen ahukkanen deleted the fix/password-validator branch February 21, 2023 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core type: change PRs that implement a change for an existing feature
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants