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"

Comments

aohrvetpv’s picture

Issue summary: View changes
nancydru’s picture

If 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.

aohrvetpv’s picture

Issue summary: View changes
aohrvetpv’s picture

If 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.

Are 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".

nancydru’s picture

Nope, 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.

aohrvetpv’s picture

Are you still suggesting a change to the text? I am unsure what the requested change is currently.

nancydru’s picture

StatusFileSize
new27.32 KB

Yes, the text should reflect the check box status. See image.

aohrvetpv’s picture

Got it, thanks for the screenshot.

aohrvetpv’s picture

Status: Active » Needs review
StatusFileSize
new679 bytes

Using "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.

aohrvetpv’s picture

Update patch to apply on latest code.

nancydru’s picture

Status: Needs review » Needs work
StatusFileSize
new11.96 KB
new12.06 KB

The ON message is okay, but the OFF message is still wrong.

How about "Blacklist not checked"

nancydru’s picture

Status: Needs work » Needs review
StatusFileSize
new814 bytes

How about this?

nancydru’s picture

StatusFileSize
new814 bytes

Oops, backwards.

aohrvetpv’s picture

When 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_".

nancydru’s picture

It 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.

aohrvetpv’s picture

That 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.

nancydru’s picture

Okay, 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.

nancydru’s picture

Status: Needs review » Needs work
aohrvetpv’s picture

I am not a JS expert (barely a beginner), but I can wrestle with it, or you can with your much greater knowledge.

I'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.

nancydru’s picture

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.

I 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?"

nancydru’s picture

Status: Needs work » Needs review
StatusFileSize
new1.13 KB

Okay, let's try this.

aohrvetpv’s picture

StatusFileSize
new1.15 KB

- 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.

nancydru’s picture

Status: Needs review » Reviewed & tested by the community

Works 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.

  • AohRveTPV committed 70334e2 on 7.x-2.x authored by NancyDru
    Issue #2497701 by NancyDru, AohRveTPV: Indicate in settings summary when...
nancydru’s picture

Woohoo! Thanks.

aohrvetpv’s picture

Status: Reviewed & tested by the community » Fixed

Thanks.

aohrvetpv’s picture

Status: Fixed » Needs review
StatusFileSize
new827 bytes

My 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.

  • AohRveTPV committed 23f5df2 on 7.x-2.x
    Revert "Issue #2497701 by NancyDru, AohRveTPV: Indicate in settings...

  • AohRveTPV committed f51c40c on 7.x-2.x authored by NancyDru
    Issue #2497701 by NancyDru, AohRveTPV: Indicate in settings summary when...
aohrvetpv’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.