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

Haveibeenpwnd review #148

Merged
merged 3 commits into from
Sep 11, 2023
Merged

Haveibeenpwnd review #148

merged 3 commits into from
Sep 11, 2023

Conversation

dfish3r
Copy link
Member

@dfish3r dfish3r commented Aug 30, 2023

No description provided.

elektro-wolle and others added 2 commits August 18, 2023 01:05
- Added remote call to API
- configurable parameters: timeout, mandatory check, behaviour on timeouts
- Allow different endpoints
- Returns the number of found occurrences as `CountCategory.Pwnd`
- Adds `-Duser.language=en` to ensure working tests on non-english development platforms

fixes #146
General reanme of HaveIveBeenPwnd to HaveIBeenPwned to match naming from the service.
Provide separate connect and read timeout properties.
Rename mandatory to allowExposed for clarity.
Rename allowPasswordDuringTimeout to allowOnException since its implementation isn't limited solely to timeouts.
Use cryptacular for hashing and encoding.
Modify test class to extend AbstractRuleTest.
Use wiremock for unit tests to avoid external dependencies.
Copy link
Member

@serac serac left a comment

Choose a reason for hiding this comment

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

I believe the searchResponse method is liable to false positives and cited the API docs about best practices for matching.

src/main/java/org/passay/HaveIBeenPwnedRule.java Outdated Show resolved Hide resolved
Co-authored-by: Marvin S. Addison <marvin.addison+github@gmail.com>
@serac serac merged commit 7bf0b40 into main Sep 11, 2023
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.

3 participants