Told you we'd be keen to help out with d8 upgrades ;-)

There was a password blacklist on the d7 version, which needs upgrading to d8.

The blacklist should:
* Prevent users from creating a password that is in the blacklist
* If "match substrings" is enabled, users should not be able to have a password that contains a blacklisted term

Patch coming in a moment! I've tried to keep it as true to the d7 version as possible.

CommentFileSizeAuthor
#16 2678578-16.patch7.39 KBhmendes
#16 2678578-interdiff_14_16.txt1.99 KBhmendes
#15 codingStandards.txt1.44 KBmarcusvsouza
#15 functional.txt533 bytesmarcusvsouza
#14 2678578-14.patch15.43 KBhmendes
#11 2678578_config.png20.03 KBhmendes
#11 2678578_interdiff_2_11.txt3.41 KBhmendes
#11 2678578-11.patch15.42 KBhmendes
#2 password_policy-add_blacklist-2678578-2.patch7.41 KBsophie.sk

Comments

Sophie.SK created an issue. See original summary.

sophie.sk’s picture

sophie.sk
she/her
Primary language English
commented
StatusFileSize
new password_policy-add_blacklist-2678578-2.patch7.41 KB

Patch attached. Includes unit testing.

sophie.sk’s picture

sophie.sk
she/her
Primary language English
commented
Category: Task » Feature request

Changing category to feature request, as it kind of is that.

sophie.sk’s picture

sophie.sk
she/her
Primary language English
commented

An interesting thing to note: I upgraded this because our client has a need for blacklisting the 10,000 most common passwords. These are exported as a single-line string in the config, as
blacklist: one\ntwo\nthree\n....

For me, working locally on Ubuntu 14.04 with a standard LAMP stack, this 112,000-character long string causes Apache to segfault and restart when I try to import the config (as part of a module install).

I had to change the format to

blacklist: |
  one
  two
  three
...

Which then imported fine ... I just can't find a way of editing how the config is exported.

I suppose there's never really going to be many people who have that many blacklisted passwords, but it's something worth bearing in mind and possibly trying to find a way around - I certainly couldn't find anything :(

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Assigned: Unassigned » nerdstein

Thanks for helping out with this, I'll review it. And, I am behind on reviewing the other issue as well. I'll try to take care of both as soon as possible.

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

In the schema...

blacklist:
      type: text
      label: 'Blacklist'

This should likely be a sequence of strings to support an array of blacklisted passwords.

Example found in config/schema/password_policy.schema.yml:

roles:
      type: sequence
      label: 'Roles'
      sequence:
        type: string
        label: 'Role'

You can see the code that saves this sequence in src/Form/PasswordPolicyRolesForm.php

More feedback...

$form['blacklist'] = [
      '#type' => 'textarea',

To support the sequence, you'll need to make this a text field with unlimited cardinality. I might recommend moving up the 'match_substrings' checkbox above it, because this list might get long.

error_message = @Translation("Your password contains restricted words or phrases.")

Does this cover both "match substrings" and "exact match" searches?

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

Can you please take a stab at implementing this feedback?

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Status: Needs review » Needs work

Marking as 'needs work'

sophie.sk’s picture

sophie.sk
she/her
Primary language English
commented

Agree that the schema file might not have been the best (I couldn't find any useful documentation), but... the whole reason I upgraded this constraint was because our client has 10,000 passwords that they want to blacklist. I'd rather not change it so we have to input each password by hand, unless that really is the only way of making the config work.

Will take a look at the rest when I have a moment this week.

bisonbleu’s picture

bisonbleu commented

Hello, is the patch in #2 viable ? I'm wondering what its status is considering the fact that it's been dormant for 3 years.

hmendes’s picture

hmendes commented
Assigned: nerdstein » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2678578-11.patch15.42 KB
new 2678578_interdiff_2_11.txt3.41 KB
new 2678578_config.png20.03 KB

Hello,
I changed the patch to be available to the Drupal9 version, and changed the approach in the patch from #2 to export a array, instead of a string (as shown in the image).

I didn't changed the $form['blacklist'] because it's working without this change and I think that creating a field with unlimited cardinality would be too much information on the screen, with multiple text fields...

Changing this to Needs Review.

hmendes’s picture

hmendes commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 11: 2678578-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hmendes’s picture

hmendes commented
Status: Needs work » Needs review
StatusFileSize
new 2678578-14.patch15.43 KB

Sorry, forgot to change the tests.

marcusvsouza’s picture

marcusvsouza commented
Status: Needs review » Needs work
StatusFileSize
new functional.txt533 bytes
new codingStandards.txt1.44 KB

The patch works as expected

Steps performed:
1. Installed the module
2. Enable the module
3. Create a constraint type blacklist
4. Create a list of forbidden passwords
5. Test the passwords in the list
6. Test the code with phpcs against the module directory
In the last step I found some errors, se the files attached.

hmendes’s picture

hmendes commented
Status: Needs work » Needs review
StatusFileSize
new 2678578-interdiff_14_16.txt1.99 KB
new 2678578-16.patch7.39 KB

Fixing the problems reported in #15 and also removing the old patch from inside the patch.

marcusvsouza’s picture

marcusvsouza commented
Status: Needs review » Reviewed & tested by the community

I tested again with phpcs against the password_policy_blacklist folder inside the module folder and no errors returned.
moving to RTBC

  • paulocs committed 966f272 on 8.x-3.x authored by hmendes 
    Issue #2678578 by hmendes, Sophie.SK, marcusvsouza: Upgrade password...
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Reviewed & tested by the community » Fixed

Fixed error implode(): Argument #2 ($array) must be of type ?array, string given in implode on commit.

Thanks!

Status: Fixed » Closed (fixed)

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