Skip to content

Update password strength check#8455

Merged
andreslucena merged 25 commits intodecidim:developfrom
mainio:refactor/password-strength-check
Dec 20, 2021
Merged

Update password strength check#8455
andreslucena merged 25 commits intodecidim:developfrom
mainio:refactor/password-strength-check

Conversation

@lahdeero
Copy link
Copy Markdown
Contributor

@lahdeero lahdeero commented Nov 2, 2021 •
edited
Loading

🎩 What? Why?

Removed nobspw dependency and wrote similar solution into Decidim.

Includes also rake tasks to update common passwords dictionary. Usage: bundle exec rake decidim:common_passwords:update in root directory.

📌 Related Issues

#8453
#7234

Testing

  1. Go to sign up form
  2. Try different passwords

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

image

♥️ Thank you!

@lahdeero lahdeero changed the title Refactor password strength check Update password strength check Nov 2, 2021
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Nov 2, 2021

I believe the blacklist option is still missing from this implementation compared to nobspw.

See:
https://github.com/cmer/nobspw/blob/21144cb50d5940e3b89df9a8dde0d77c05f2bf8e/lib/nobspw/validation_methods.rb#L53-L65

ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 2, 2021
View reviewed changes
@andreslucena andreslucena mentioned this pull request Nov 12, 2021
Closed
6 tasks
ahukkanen
ahukkanen reviewed Nov 23, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Nov 23, 2021
View reviewed changes
ahukkanen
ahukkanen reviewed Dec 14, 2021
View reviewed changes
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Dec 14, 2021

@andreslucena This is ready for review.

andreslucena
andreslucena requested changes Dec 17, 2021
View reviewed changes
Copy link
Copy Markdown
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.

I think I found a bug with a spec, can you check it 🙏🏽 ?

ahukkanen
ahukkanen reviewed Dec 17, 2021
View reviewed changes
decidim-core/spec/lib/common_passwords_spec.rb Outdated

describe "#passwords" do
it "returns passwords" do
expect(subject.instance.passwords).to be_kind_of(Array)
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen Dec 17, 2021

Choose a reason for hiding this comment

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

@lahdeero I noticed another issue with this after the latest changes.

If this test is run before the .update_passwords! test, the password file may not exist at the overridden path and it may throw an exception.

If you run the following:

$ cd decidim-core
$ rm -f ../spec/decidim_dummy_app/tmp/common-passwords.txt
$ bundle exec rspec spec/lib/common_passwords_spec.rb --seed 18259

This would throw the following error:

  1) Decidim::CommonPasswords when file exists and request returns body #passwords returns passwords
     Failure/Error: raise FileNotFoundError unless File.exist?(self.class.common_passwords_path)
     
     Decidim::CommonPasswords::FileNotFoundError:
       Decidim::CommonPasswords::FileNotFoundError
     # ./lib/decidim/common_passwords.rb:16:in `initialize'
     # ./spec/lib/common_passwords_spec.rb:31:in `block (4 levels) in <module:Decidim>'

So, maybe we should call subject.update_passwords! also in this test?

Copy link
Copy Markdown
Contributor Author

@lahdeero lahdeero Dec 17, 2021

Choose a reason for hiding this comment

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

Then they would be almost identical tests. I just merged these tests.

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Dec 20, 2021

Great job @lahdeero!! 👏🏽 👏🏽

@andreslucena andreslucena merged commit a663377 into decidim:develop Dec 20, 2021
@andreslucena andreslucena added the type: change PRs that implement a change for an existing feature label Dec 20, 2021
@ahukkanen ahukkanen deleted the refactor/password-strength-check branch December 20, 2021 09:55
@andreslucena andreslucena linked an issue Dec 21, 2021 that may be closed by this pull request
Replace libraries (gems) with small communities with alternatives #8516
Closed
6 tasks
@andreslucena andreslucena removed the type: change PRs that implement a change for an existing feature label Dec 21, 2021
@davidbeig davidbeig mentioned this pull request Oct 26, 2022
Merged
basicavisual added a commit to CodeandoMexico/decidim-monterrey that referenced this pull request Nov 8, 2022
@basicavisual
Implemented decidim-module-friendly_signup
4f093b2 
The list of changes:
1. Removed dependency NOBSPW reference in registrations view as per PR decidim/decidim#8455
2. Rewrote view to be compatible to decidim-module-friendly_signup (see https://github.com/OpenSourcePolitics/decidim-module-friendly_signup/blob/main/app/views/decidim/devise/registrations/new.html.erb )
3. Included the gem's locales at config/locales/firendly_signup/es.yml, so parse all subfolders and files under config/locales/ at application.rb:19
4. Added the config/initializers/friendly_signup.rb to specify the features we are going to use
5. Updated version.rb for decidim-module-ine so that it accepts version 0.26.X
@alecslupu alecslupu added this to the 0.26.0 milestone Jul 14, 2023
@ahukkanen ahukkanen mentioned this pull request Sep 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen left review comments

@andreslucena andreslucena andreslucena approved these changes

Assignees

No one assigned

Labels

in-review target: developer-experience

Projects

None yet

Milestone

0.26.0

Development

Successfully merging this pull request may close these issues.

Replace libraries (gems) with small communities with alternatives

4 participants

@lahdeero @ahukkanen @andreslucena @alecslupu