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

Improve usage of this library #194

Closed
MrWook opened this issue May 6, 2023 · 5 comments · Fixed by #244
Closed

Improve usage of this library #194

MrWook opened this issue May 6, 2023 · 5 comments · Fixed by #244
Labels
BreakingChange Indicates that this feature needs a major release enhancement New feature or request
Milestone

Comments

@MrWook
Copy link
Collaborator

MrWook commented May 6, 2023

I'm not as happy as in the beginning with the current usage of the library and how to set the options. The singleton of the options are limiting the usage of the library in some cases which can be annoying for developers.
In the beginning i wanted to use this approach to keep the general usage of the library as close to the original as possible.
Currently you can just use zxcvbn from the core package without setting any options and it will just work.
As this library is getting furthor away from the original library because of improved algorythm more matcher better dictinaries and so on. I think we should have a simpler approach to how to use this library and the highly customization of it.
I thought about two different approaches

1. Use zxcvbn as a class

I think we could directly use a class instead of making a little turn for a singleton options class

import { zxcvbnFactory } from '@zxcvbn-ts/core'
import * as zxcvbnCommonPackage from '@zxcvbn-ts/language-common'
import * as zxcvbnEnPackage from '@zxcvbn-ts/language-en'

const options = {
  translations: zxcvbnEnPackage.translations,
  graphs: zxcvbnCommonPackage.adjacencyGraphs,
  dictionary: {
    ...zxcvbnCommonPackage.dictionary,
    ...zxcvbnEnPackage.dictionary,
  },
}
const zxcvn = new zxcvbnFactory(options)

const password = 'somePassword'
zxcvn.check(password)
// or
await zxcvn.checkAsync(password)

To update the options we could use the same as previously:

zxcvn.setOptions(options)

2. Make heavy options function public

Another approach would be too get rid of the classes and make the heavy functions public. For example building the correct rankedDictionaries is really heavy and could be a functions outside

const rankedDictionaries = makeRankedDictionaries({
    ...zxcvbnCommonPackage.dictionary,
    ...zxcvbnEnPackage.dictionary,
    userInputs: []
  })
const l33tTabel = makeL33tTable(l33tTableOption)

const options = {
  translations: zxcvbnEnPackage.translations,
  graphs: zxcvbnCommonPackage.adjacencyGraphs,
  rankedDictionaries,
  l33tTable,
}
const password = 'somePassword'
zxcvbn(password, options)
@MrWook MrWook added the enhancement New feature or request label May 6, 2023
@MrWook MrWook pinned this issue Jun 5, 2023
@balassy
Copy link

balassy commented Jun 6, 2023

I think in its current form it is very easy to misuse this library. Consumers may assume that it is okay to call setOptions once, and subsequent calls will be using the same configuration. However because the extendUserInputsDictionary function in Options.ts user inputs are persisted in the configuration, so they can impact the future password strength calculation.

This issue can fix the current behavior too.

See:

if (this.dictionary.userInputs) {

@balassy
Copy link

balassy commented Jun 10, 2023

@MrWook , thank you very much for mentioning me in the security advisory, I appreciate it! (And sorry, I couldn't find a better channel for this message :)

@MrWook
Copy link
Collaborator Author

MrWook commented Jun 13, 2023

@balassy don't worry, i saw that there were no way to report those issue too and i added the SECURITY.md file to the repo for those cases.

@camerondm9
Copy link

I wouldn't mind if zxcvbn was a class that could be constructed with different options.
It would be really nice if the userInputs was separate from the other dictionaries, so you could change userInputs without rebuilding everything.
I think my ideal API would have the zxcvbn object be completely immutable once constructed, so the options are locked in. It would be reusable without changing any configuration:

import { zxcvbnFactory } from '@zxcvbn-ts/core'
import * as zxcvbnCommonPackage from '@zxcvbn-ts/language-common'
import * as zxcvbnEnPackage from '@zxcvbn-ts/language-en'

const options = {
  translations: zxcvbnEnPackage.translations,
  graphs: zxcvbnCommonPackage.adjacencyGraphs,
  dictionary: {
    ...zxcvbnCommonPackage.dictionary,
    ...zxcvbnEnPackage.dictionary,
  },
}
const zxcvbn = new zxcvbnFactory(options) //zxcvbn object is immutable and read-only (construct a new one if you want different options)

const password = 'somePassword'
const userInputs = ['email', 'first-name', 'last-name']
zxcvbn.check(password, userInputs) //doesn't save password or userInputs, computes using function inputs only

@MrWook MrWook added the BreakingChange Indicates that this feature needs a major release label Sep 17, 2023
@MrWook MrWook added this to the 4.0.0 milestone Dec 8, 2023
@OneHoopyFrood
Copy link

OneHoopyFrood commented Dec 15, 2023

Being able to separate instances in this way would enable me to have 2 separate validators, which would be an acceptable workaround for the problem I described in #246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BreakingChange Indicates that this feature needs a major release enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants