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

Feature/add configuration option for custom dictionaries #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Saoma1
Copy link

@Saoma1 Saoma1 commented Jun 1, 2023

Description

This pull request adds a new configuration option to the Zxcvbn library, allowing users to customize to add custom dictionaries in the form of a text file.

Changes Made

  • Added a new config module
  • Added spec for the config
  • Added config option to the readme

@formigarafa
Copy link
Owner

I like this idea, too. I have been investigating something along these lines since the beginning.

I did not committed with this approach because I am still wondering what can I do to load the dictionaries faster, save memory after loaded, and release memory when no longer needed to keep them loaded.

I understand that these listed ideas listed may not be compatible with each other.
I could not decide what direction to take, yet, and for now I simply left the code similar to the original.

Sorry for taking that long, but I will have a bit more thought around it. Maybe also add a reset/replace option, so you can get a completely fresh dictionary. I am also wondering If I should change the current dictionaries to files to be loaded and have a single interface to everything. What you think?

@Saoma1
Copy link
Author

Saoma1 commented Nov 1, 2023

@formigarafa thanks for writing back.

So right now I use your gem in a rails app, there, I want the dictionaries to be loaded ones and be kept in memory.
Would want to have to reload the dictionaries every time a request comes in, that would take too much time. But that is just my input and maybe I am shooting in a different direction.

I do see the value of a reset/reload method as we might not want to reload the whole application.
Also the idea of a single interface makes a lot of sense.

If I can help out in any way let me know 🙂

@formigarafa
Copy link
Owner

I understand your point and keeping dictionaries in memory is what the gem does atm.
But I wonder if there is a middle ground because usually only sign-ups and password changes need to check the password strength.
Sign-in requests only need if you want to take existing users with weak passwords to update their passwords.
And even in that case you would not want that step to be dragged for too long. If after a good chunk of users have been marked as having a strong password I would just erase the password of the remaining users and let them use a reset password process to set a new and strong one.
The apps in general would be processing a whole lot of other requests that do not need that dictionary in memory.

@formigarafa
Copy link
Owner

@Saoma1 , I feel that with #18 PR your idea of a configurable selection of dictionaries should be much easier to implement without causing any trouble to other functionalities, resetting or unloading.

@Saoma1
Copy link
Author

Saoma1 commented Nov 5, 2024

@formigarafa awesome, that is great to hear & thank you for your work! I will test it out once you merge it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants