In this PR, I added various tests on the CredentialCollection class, all of them currently failing and illustrating what I think are bugs.
Issues can be grouped in two categories:
those linked to the additional_publics array
those linked to the password_spray option
I added some comments on each test explaining what it is currently yielding (instead of the result I think should be expected - let me know if I misunderstood anything).
Refactoring opportunity
You will see that there are many bugs linked to the password_spray option.
It seems that this branch of the code has been copy/pasted from the original branch (that didn't handle password spraying) and adapted.
There was not a lot of tests on this part of the code + the structure is difficult to maintain which probably explains those issues.
The original code branch (without spraying: #each_unfiltered_username_first) is also a bit complex with a lot of duplicated code.
I think there are some opportunities to simplify and clarify the code a lot with some refactoring.
Non-regression tests
For this, I created 2 additional "non-regression" tests that activate all options and show how the credentials should be yielded.
There is one test with password spraying, one without.
I don't know if we want to keep them after the refactoring, but they will surely help.
Also, even if a part of the order in which the credentials are yielded should be "fixed", there are questions around others.
For example:
when do we want to yield the credentials generated by the user_as_pass option in case of password_spraying?
when do we want to yield credentials generated by the "userpass" file?
Additional question
As a side note, the nil_passwords, blank_passwords and user_as_pass options do not apply to the userpass file.
Is it expected behaviour or should we extend this behaviour to users yielded in the userpass file?
Next steps
[x] 1. I first plan to fix the easiest bugs (wrong var names for example) to make more tests pass
[x] 2. Then I would tackle the cleaning of the password spraying code branch, so as to make all tests pass.
[ ] 3. Then in a third commit, I would refactor both branches to simplify the code and reduce the probability to have such bugs in the future (also facilitating maintenance)
Draft PR linked to issue #19652
Summary
In this PR, I added various tests on the CredentialCollection class, all of them currently failing and illustrating what I think are bugs.
Issues can be grouped in two categories:
additional_publicsarraypassword_sprayoptionI added some comments on each test explaining what it is currently yielding (instead of the result I think should be expected - let me know if I misunderstood anything).
Refactoring opportunity
You will see that there are many bugs linked to the
password_sprayoption.It seems that this branch of the code has been copy/pasted from the original branch (that didn't handle password spraying) and adapted. There was not a lot of tests on this part of the code + the structure is difficult to maintain which probably explains those issues.
The original code branch (without spraying:
#each_unfiltered_username_first) is also a bit complex with a lot of duplicated code.I think there are some opportunities to simplify and clarify the code a lot with some refactoring.
Non-regression tests
For this, I created 2 additional "non-regression" tests that activate all options and show how the credentials should be yielded. There is one test with password spraying, one without.
I don't know if we want to keep them after the refactoring, but they will surely help.
Also, even if a part of the order in which the credentials are yielded should be "fixed", there are questions around others. For example:
user_as_passoption in case ofpassword_spraying?Additional question
As a side note, the
nil_passwords,blank_passwordsanduser_as_passoptions do not apply to the userpass file. Is it expected behaviour or should we extend this behaviour to users yielded in the userpass file?Next steps