Suggested by NancyDru in #1603210: Add token support to blacklist constraint.
My proposal:
"Also disallow passwords containing blacklisted passwords." unchecked:
Summary: "Must not be in blacklist" (same as present)
"Also disallow passwords containing blacklisted passwords." unchecked:
Summary: "Must not contain a string in blacklist"
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 2497701-27.patch | 827 bytes | aohrvetpv |
| #22 | 2497701-22.patch | 1.15 KB | aohrvetpv |
| #21 | 2497701-21.patch | 1.13 KB | nancydru |
| #13 | 2497701-13.patch | 814 bytes | nancydru |
| #12 | 2497701-12.patch | 814 bytes | nancydru |
Comments
Comment #1
aohrvetpv commentedComment #2
nancydruIf it is unchecked, the blacklist is not checked at all, so "Must not be in blacklist" would be misleading, as it is now. Something like "Blacklist not checked" would be more correct.
Comment #3
aohrvetpv commentedComment #4
aohrvetpv commentedAre you sure? I think the blacklist is used if there is text in the "Blacklisted passwords" textarea. If it is empty, the constraint is not enforced and the summary shows "Not enforced".
Comment #5
nancydruNope, I just unchecked that option and put a single word in there, and then set a password containing it.
Then I checked the box and the same password failed.
Before you get upset, this is how, IMHO, it should work.
Comment #6
aohrvetpv commentedAre you still suggesting a change to the text? I am unsure what the requested change is currently.
Comment #7
nancydruYes, the text should reflect the check box status. See image.
Comment #8
aohrvetpv commentedGot it, thanks for the screenshot.
Comment #9
aohrvetpv commentedUsing "member" to avoid the term "string". Concern is that administrators may not know what a "string" is. I could be wrong but "string" seems like more of a programming term.
Comment #10
aohrvetpv commentedUpdate patch to apply on latest code.
Comment #11
nancydruThe ON message is okay, but the OFF message is still wrong.
How about "Blacklist not checked"
Comment #12
nancydruHow about this?
Comment #13
nancydruOops, backwards.
Comment #14
aohrvetpv commentedWhen the checkbox is unchecked and the textarea has text, the constraint is enforced. Passwords that are matches of a password in the textarea are disallowed. I just tried it manually and it seems to work:
1. Edit example policy.
2. Set "Blacklisted passwords" to "foobarbaz", leaving the checkbox unchecked. Save.
3. Browse to "My account".
4. Attempt to change password to "foobarbaz". Blacklist constraint message observed.
Checking the checkbox would also disallow a password containing "foobarbaz", such as "_foobarbaz_".
Comment #15
nancydruIt doesn't do that for me. I did another Git clone this morning, so I have the latest plus these three patches.
I added "foobar" to the list and then tried "Thisis1foobarpassword!" and it changed my password to that.
When I checked the box, the password was rejected.
Comment #16
aohrvetpv commentedThat sounds like expected behavior. "Thisis1foobarpassword!" would only be disallowed if both "foobar" were blacklisted and the setting to also disallow passwords containing blacklisted passwords were enabled.
The purpose of the checkbox is not to enable/disable the constraint. Checking the checkbox makes the constraint stricter so that it disallows passwords not just equaling a blacklisted password, but also containing a blacklisted password.
Case 1
Blacklisted passwords:
Also disallow passwords containing blacklisted passwords: unchecked
Expected results:
"foobar" allowed.
"Thisis1foobarpassword!" allowed.
Case 2
Blacklisted passwords: foobar
Also disallow passwords containing blacklisted passwords: unchecked
Expected results:
"foobar" disallowed.
"Thisis1foobarpassword!" allowed.
Case 3
Blacklisted passwords: foobar
Also disallow passwords containing blacklisted passwords: checked
Expected results:
"foobar" disallowed.
"Thisis1foobarpassword!" disallowed.
Comment #17
nancydruOkay, I see. If there is text in the textarea and the box is not checked, the text must be standalone (sort of like strcmp). And if the box is checked, it's more of a substring check (like strpos).
So, if there is no text in the textarea, it should say something like "Not enforced". If there is text and the box is not checked, maybe something like "Specific text disallowed". And if the box is checked, maybe "May not contain selected texts."
I suppose we should also indicate that tokens are enabled.
I am not a JS expert (barely a beginner), but I can wrestle with it, or you can with your much greater knowledge.
Comment #18
nancydruComment #19
aohrvetpv commentedI'm also a beginner, for some level of beginner. Would be happy to review or try out any patches.
Possibly outside the scope of this issue, but I am wondering if there is ever a desire to disallow certain strings as passwords, but still allow passwords that contain those strings. Maybe the option is not really needed, and the sole behavior should be to disallow passwords containing one of the blacklisted strings.
Comment #20
nancydruI can't imagine why anyone would want a string excluded only when it is stand-alone. But my experience with module writing for D.O is that someone else will always thwart my reasoning. That said, a stand-alone string is self-containing.
Perhaps this is a question for another issue addressed to the community. Or you can just make an executive decision. "Would you ever add 'hoe' to the exclude list, but want 'shoes' to be allowed?"
Comment #21
nancydruOkay, let's try this.
Comment #22
aohrvetpv commented- Change "text string" to "string". ("text string" as opposed to a "non-text string"? "text string" seems redundant and "string" seems sufficient.)
- Move tokens conditional into blacklist conditional. (Avoids a redundant
if (blacklist ...).)- Use "certain" instead of "specific". (One message used "certain" and one message used "specific", but I do not see a distinction in meaning, so choosing one term for consistency.)
I have some reservation about exposing the term "string" to administrators, because to me it seems like a programming term and not all Drupal administrators are programmers. But we can change it later if needed.
Please review.
Comment #23
nancydruWorks for me. I used "specific" for delineated strings and "certain" for containable strings. I thought "text string" might be less programmer-ish. But you are right, if someone objects it can be changed.
Comment #25
nancydruWoohoo! Thanks.
Comment #26
aohrvetpv commentedThanks.
Comment #27
aohrvetpv commentedMy mistake--the tokens part should not have been committed yet because the tokens feature itself is not yet committed. This patch removes it. Will merge the removed code into patch in #1603210: Add token support to blacklist constraint.
Comment #30
aohrvetpv commented