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

Make scoring thresholds configurable #222

Closed
wants to merge 3 commits into from

Conversation

MrWook
Copy link
Collaborator

@MrWook MrWook commented Jun 26, 2023

@camerondm9
Resolves #217

This would be a breaking change as the attack times need to be configurable too which means the naming need to be changed.

@MrWook MrWook added the enhancement New feature or request label Jun 26, 2023
@MrWook MrWook marked this pull request as draft June 26, 2023 20:40
Copy link

@camerondm9 camerondm9 left a comment

Choose a reason for hiding this comment

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

@MrWook Looks good overall, but might benefit from slightly clearer names (really minor though). I've included suggestions that I think are clearer, but naming is always hard, so feel free to adjust.

I've also made a suggestion on the doc comment. I think it would be good to explain just a little bit about why these values are configurable.

packages/libraries/main/src/types.ts Outdated Show resolved Hide resolved
packages/libraries/main/src/Options.ts Outdated Show resolved Hide resolved
packages/libraries/main/src/TimeEstimates.ts Outdated Show resolved Hide resolved
packages/libraries/main/src/TimeEstimates.ts Outdated Show resolved Hide resolved
@MrWook
Copy link
Collaborator Author

MrWook commented Jun 27, 2023

@camerondm9 thank you very much for the review! I adjusted the code according to your suggestions. I just didn't used the underscore in the naming.
But as this is a breaking change and needs a major version change i would like to add the library usage improvment too for the next major version

Copy link

@camerondm9 camerondm9 left a comment

Choose a reason for hiding this comment

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

One spot where documentation could be a little clearer, and one typo.

}

result.crackTimesDisplay # same keys as result.crackTimesSeconds,
# with friendlier display string values:
# "less than a second", "3 hours", "centuries", etc.

result.score # Integer from 0-4 (useful for implementing a strength bar)
result.score # Integer from 0-4 (useful for implementing a strength bar). The scoring is based on the default configuration.

Choose a reason for hiding this comment

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

"The scoring is based on the default configuration" could be confusing for someone who has changed the configuration. It might be better to just say that this is configurable (using timeEstimationValues) and uses defaults if not configured...

| levenshteinThreshold | number | 2 | Threshold for levenshtein |
| l33tMaxSubstitutions | number | 100 | Indicated the max substituions for the l33t matcher to prevent DOS |
| maxLength | Object | 256 | Defines how many characters of the password are checked |
| timeEstimationValues | number | {} | Define the values to calculate the scoring and attack times |

Choose a reason for hiding this comment

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

Looks like you changed the wrong line here.

maxLength should be a number
timeEstimationValues should be an Object

@MrWook MrWook added this to the 4.0.0 milestone Dec 8, 2023
@MrWook
Copy link
Collaborator Author

MrWook commented Jan 31, 2024

Closed in favor of #250

@MrWook MrWook closed this Jan 31, 2024
@MrWook MrWook deleted the feat!/make-scoring-thresholds-configurable branch February 1, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make time estimate and scoring thresholds configurable
2 participants