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

Simplify and optimize IllegalSequenceRule #89

Merged
merged 1 commit into from Mar 1, 2019

Conversation

amichair
Copy link
Contributor

@amichair amichair commented Feb 25, 2019

Rewrote IllegalSequenceRule to be much shorter and more efficient.

Other than the refactor, the functionality has also changed in one respect: if the illegal sequence length is set to e.g. 5 numerals, and the input password contains the sequence 1234567, then the new implementation will return the full illegal sequence 1234567 in the error result whereas the old implementation would return 3 illegal sequences 12345, 23456, 34567 (and not the full illegal sequence).

While it would be easy to support the previous functionality also after the refactor, I think the new behavior is more intuitive and informative, and the previously returned multiple subsequences, while technically correct, are not as useful (of course for every invalid sequence all subsequences of the configured length are also invalid... but the main sequence is the more interesting one to know about, which the old implementation misses.)

Also updated the tests for the new behavior, and added a few more interesting cases to the tests.

…l invalid sequence instead of multiple invalid subsequences.
@dfish3r
Copy link
Member

dfish3r commented Mar 1, 2019

Change in IllegalSequenceRule behavior did not break tests in other projects and I agree the new behavior is more desirable.

@dfish3r dfish3r merged commit c8dd70b into vt-middleware:master Mar 1, 2019
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.

None yet

2 participants