Write tests for: Users with passwords over 60 characters cannot log in via the user login block

Awesome ross9885, the more core developers the merrier!

Some notes from my inital look at the issue (repeating some of your observations):

• User module stores passwords in the "pass" column which has a length limit of 128 characters. (user.install)
• User module provides a block which allows users to enter their username and password in order to login. The block's password textfield has a maximum length restriction of 60 characters. This is incorrect and should be changed to 128. (user.module)
• When installing Drupal, the password field has no validation check for the length of the password. #1344552: Password confirm field type should have support for #maxlength attribute is related but can be worked around by manually validating the length of the password in install_configure_form_validate() (install.core.inc)
• When updating a user password on the user edit page, the password field has no validation check for the length of the password. #1344552: Password confirm field type should have support for #maxlength attribute is related but can be worked around by manually validating the length of the password in user_validate_current_pass() (user.module) which is called by AccountFormController (AccountFormController.php)

Also, patches are made against Drupal 8 and then backported to Drupal 7 in order to prevent regressions.

This is a nasty bug, but I don't think it should be blocking other development.

Really nice find, @ross9885! And great issue summary.

why not remove the size and maxlength because they are already defined in the default password form element declaration ?

this is ok to define a custom value for custom modules but i think that drupal should have the same value everywhere.

i m telling this because i'm building a custom module and i have overrided the form element maxlenght but this is still forced in some drupal's forms like the login form so i need to also use a form alter or preprocess instead of just altering the default form element.

A patch to implement ssm2017 Binder's suggestion.

The login block's password form item doesn't need to declare a maxlength since all password elements have an appropriate default maxlength of 128.

I have tested the patch and confirmed that it does not enforce 60 characters max on the login form.

Because this pointed out a larger problem that all password fields should be consistent, I did a grep to find all '#type' => 'password' instances assuming they would be the only ones that would have max sizes:

grep -r "'#type' => 'password'" *

I reviewed all nine core files found by that grep and none of them, post-patch, has a maxlength attribute so all are now consistently coded.

Looks like this already got committed at some point down the line to 8.x. Moving down to 7.x, which should have that same test by caelon in #8 to ensure that there's only one instance.

The patch from #1772584: Get rid of user_login_block() & user_login() in favour of user_login_form() fixed the issue and was committed three days ago by yourself. :P

See http://drupalcode.org/project/drupal.git/commit/b38996475dd09e5c3709397c...

A patch against D7, since the issue which fixed this in D8 will not be backported.

Just ran ack --type-add php=.inc password -A 10 . --php | grep maxlength to doublecheck if we were missing anything, seems ok.

Looks good to me, but shouldn't we have an automated test here too?

This issue has the "Novice" tag, and it might actually be a good test for someone who is looking to get started with simpletests to work on. Basically, the goal would be to create a user with a password equal to the maximum allowed length and then make sure that user can log in (via both the user login block and the regular /user/login page) without any errors.

This test could go into Drupal 8 too, actually...

Actually, come to think of it, since this is already fixed in Drupal 8 it will be a lot simpler if we just commit it to Drupal 7 now and do the tests as a followup.

So... committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/dafe7ae

@Devin Carlson, feel free to unassign yourself if you don't want to write those tests, by the way :)

@David Carlson, anything I can help with. Happy to help. Thanks

Here is patch for the test case.

I've left a mistake, the length of the valid and invalid case were the same. Apologies.

Thanks! Setting to "needs review" so that the testbot will run it.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -23,6 +23,45 @@ public static function getInfo() {
+      if ($password_length === 128) {
+        $this->assertNoText(t('User login'), 'Logged in with 127 character length password.');

Why does the assertion message say 127 characters here when the password was generated with 128 characters?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -23,6 +23,45 @@ public static function getInfo() {
+      if ($password_length === 129) {
+        $this->assertText(t('Log in'), 'Couldn\'t log in with 255 character long password.');

Similarly, I don't understand the assertion message. Why does it contain 255 characters? I could totally miss something here with those characters, but if that's the case maybe documenting the reason in a comment would be a good idea.
Also, it's just a small thing, but using single quotes and an escaped apostrophe in the string should be avoided (even though this is not inside of t()). I did a quick grep and I learned that it's more common to avoid this situation by using the non-shortened form "could not". Additionally, a super-minor remark: I would also make sure that the two assertion messages have nearly the same wording (i.e.: "length" vs. "long").

Thanks Bálint, I've accidentally left those numbers in. I've updated the test according your recommendations.

I was focusing on those assertion messages, but now I've given more thoughts to the approach of the patch.

Use assertFailedLogin()

There is already a method in the same class for making an unsuccessful login attempt, it would be better to use that.

Consider implementing setUp() method

Since almost all methods in this class need at least one user account to perform tests it would be interesting to consider implementing the setUp() method, creating user accounts there and making them accessible via class properties so that each test method in the class can utilize them.
One of the user accounts could be created with the maximum allowed numbers of characters in the password (other tests will pass just fine), so in the test method introduced by the patch this password could be simply changed when we want to test the login failure scenario.
If the setUp() method is a good idea, maybe refactoring the other methods to use the user object created by that could be a follow-up issue.

Test user login block as well

As @David_Rothstein said the test should cover the login attempts from a login block as well, not only logins from the regular user login page.

Cheers Bálint for the review. This patch contains number of improvements what Bálint suggested:

• UserLoginTest class refactored to use shared user accounts
• additional test to cover the login block cases

Use of assertFailedLogin(). assertFailedLogin() expects wrong user credentials, where Drupal returns with Sorry, unrecognized username or password. Have you forgotten your password?' instead of Password cannot be longer than... which means the credentials were correct but the password has invalid length. That's why I handled this in different way.

Now only I fail left which looks like because a bug in the Blocks module. The testPasswordLengthBlock() should be fine, but the issue is a bit weird.

The login fails because the invalid password length, but in the block header instead t('User login') I get the username. With same block the error message and the form appears as it should. Check the raw response here: http://pastebin.com/97nAzeqW . With cURL the login via the UI works fine.

I'm a little confused that have we committed the code to fix the problem described here?
If the answere is yes, so what we need to do now is to write tests for this scenario?

@smiletrl: I noticed the issue I described in #21 while I was writing tests for log in. I need confirmation, is this a real issue?
To replicate it all you need is to apply the patch I attached and run the test via either Drush (drush test-run UserLoginTest) or UI.

@aries, yeah, this is an interesting problem as in #24. I get the same issue too.

I suggest do this test manually. Create such user in code directly and login via UI. I agree that there could be some problem with user-login form. Or I'm thinking because the length has been designed in db for 128, this new created user in code could lead to unexpected result from db.

      'pass' => array(
        'type' => 'varchar',
        'length' => 128,
        'not null' => TRUE,
        'default' => '',
        'description' => "User's password (hashed).",
      ),

Maybe we could change the way we are testing? Try to create a user with 129 length pass, and get some error? But this page test returns right result:(

Maybe there's problem with drupalPost, like what you assumed. Anyway, let's test this manually, and see what happens:)

@smiletrl: I've tested manually, I saved and put the response to pastebin.com. The link is in #24.

a)

John@JOHN-PC /d/apache/drupal82 (user_login_tests-1777270-29)
$ grep -r 'Password cannot be longer' *   

returns nothing. I'm not sure where does this scentence come from.

b) my manual test returns 'Sorry, unrecognized username or password. Have you forgotten your password?' when I try to log in with 129 length password.

c)I think we could add something like this in user.module

/**
 * Maximum length of user password text field.
 */
const USERPASS_MAX_LENGTH = 128;

and do this for user_login_form and user_register_form

  $form['pass'] = array(
    '#type' => 'password',
    '#title' => t('Password'),
    '#size' => 60,
    '#maxlength' => USERPASS_MAX_LENGTH,  // add constraint here.
    '#description' => t('Enter the password that accompanies your username.'),
    '#required' => TRUE,
  );

This should solve our problem.
We may add some password validate process, like username does

  if (drupal_strlen($name) > USERNAME_MAX_LENGTH) {
    return t('The username %name is too long: it must be %max characters or less.', array('%name' => $name, '%max' => USERNAME_MAX_LENGTH));
  }

But this validate looks redundant to me, since $form['name'] ['#maxlength'] has already defined the max length.

d) I think we need so something like this to place user_login_block.

    // Add user login block.
    $this->adminUser = $this->drupalCreateUser(array('administer blocks'));
    $this->drupalLogin($this->adminUser);
    $this->drupalPlaceBlock('user_login_block');
    $this->drupalLogout($this->adminUser);

But it seems this block is there without such an admin-user. Anyway, just a suggestion.

e) I can't explain why this test returns this wierd result. Perhaps it needs deep exploring code effort:(

@smiletrl: your suggestions are already implemented.

Added info about maxlength attribute.

This issue also affects Drupal 6.x (and is becoming more noticeable as more of our users are adopting xkcd-style password novellas).

Could we apply the attached backport to Drupal 6.x, then resume work on tests for 8.x?

There are still Drupal 8 patches to review above which add automated tests for this.

I took the patch from aries in #24 and refactored it a little to make the tests pass against the latest HEAD. Any improvement suggestions are welcome.

Ooops ... sorry something went wrong with my last patch. Please review this one.

@aries are you still working on this, or can it be unassigned?

Going to assume yes, at least in part because it's been 3 years since @aries worked on it and 3 months since the last patch by @dermario. Plus, it looks like it might be good for someone to review at the Drupal North 2016 code sprint tomorrow.

@aries or @dermario, if you are still working on this, please re-assign it to yourself!

Patch applies to 8.2.x and tests pass, assertion messages are fine.

Added interdiff between the latest two patches, #24 and #37. And retesting latest patch with 8.2.x.

Reviewed the patch in #37. All looks good, at least to me, but I don't know if the changes made in response to #21 are out of scope. Otherwise, I'd RTBC. Anyone care to comment?

Added a summary to the IS.

I re-rolled the patch against 8.3.x. Additionally I changed the last assert calls on both testGlobalLoginFloodControl() and testPerUserLoginFloodControl() to use a valid user, not the invalid one, since that's what the code originally does and it seems to have been changed in error in patch #37.

I ran the tests again against 8.4.x, and it still seems to apply. Hopefully someone finds time to review this.

I found a couple of things in this patch that should be updated to our new standards. I like the idea of the additional coverage!

1. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -13,6 +13,49 @@
   +   * Modules to enable.
   +   *
   +   * @var array
   

   Should be {@inheritdoc}

2. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -13,6 +13,49 @@
   +  public static $modules = array('block');
   

   Short array syntax should be used instead.

3. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -13,6 +13,49 @@
   +   * @var array
   

   This is an array of users, @var \Drupal\user\UserInterface[]

4. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -13,6 +13,49 @@
   +    $password_lengths = array(
   

   ^

5. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -13,6 +13,49 @@
   +      $account = entity_create('user', array(
   +        'name' => $test_name,
   +        'mail' => $test_name . '@example.com',
   +        'pass' => $test_pass,
   +        'status' => 1,
   +      ));
   +      $account->save();
   

   We shouldn't use entity_create here, we can use the createUser method and pass just the pass to it. That should be sufficient for this.

6. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -29,6 +72,62 @@ public function testLoginCacheTagsAndDestination() {
   +    /** @var User $user_with_valid_password_length */
   

   This line can be removed, if we correctly document the $this->userCases parameter.

7. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -29,6 +72,62 @@ public function testLoginCacheTagsAndDestination() {
   +    $auth = array(
   

   Should be short array syntax.

8. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -29,6 +72,62 @@ public function testLoginCacheTagsAndDestination() {
   +    $this->drupalPostForm('user/login', $auth, t('Log in'));
   +    $this->assertNoText(t('User login'), 'Logged in with 128 character long password.');
   +    $this->assertPattern('!<h4.*?' . t('Member for') . '.*?</h4>!', 'Redirected to /user');
   

   No need for the t here, as this page is not translated.

9. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -29,6 +72,62 @@ public function testLoginCacheTagsAndDestination() {
   +    /** @var User $user_with_invalid_password_length */
   

   ^

As part of the Bug smash initiative, we are triaging issues that are marked 'Postponed (maintainer needs more info)'. I started with the referenced issue, #2851784: Long passwords allowed when creating account, but logging in fails and continued on to here.

The tests in the latest patch here specifically test long passwords in the user form and in the user login block. There are are also changes to testLoginCacheTagsAndDestination() and testGlobalLoginFloodControl() but these are variable name changes and do not alter the functionality of those tests. Tests of long passwords were added to core in #2378703: Port denial of service fixes from SA-CORE-2014-006 to Drupal 8 in \Drupal\Tests\Core\Password\PasswordHashingTest and there is a test of user login, \Drupal\Tests\user\Functional\UserLoginTest but that doesn't specifically test long passwords and there isn't a test of logging in from the user login block. So, I reckon the work to do here is to add functional tests for logging in on the user page and via the user login block that specifically test long passwords.

Following on I added a test to UserLoginTest to test the use of long passwords, using PasswordInterface::PASSWORD_MAX_LENGTH for the length, which is 512. Unfortunately the test fails and looking at the html output it reports
Password: may not be longer than 255 characters.

Curious I ran some more tests and found that only passwords with length of < 128 will pass. And further the error message displayed changes depending on the length of the password.

129: Password cannot be longer than 128 characters but is currently 129 characters long.
254: Password cannot be longer than 128 characters but is currently 254 characters long.
512: Password: may not be longer than 255 characters.

Then I went back to review the manual testing I did earlier. I created a password of 521 characters and entered that at user/1/edit and it saved successfully and I could login with that password. I logged out and removed characters from the end of the password to make the password 511 characters long. I could successfully login with that as well. Further testing and found that the max length in 127. At least that agrees with testing results.

The next steps are to understand why the password length is 127 despite what PassowrdInterface states and why the test produces error messages not seen during manual testing (standard profile).

Spoke with larowlan on #bugsmash who lead me to \Drupal\Core\Render\Element\Password::getInfo where the field length is set to 128.

so looking at PasswordItem it extends StringItem
StringItem has a max_length of 255 but that’s for binary => FALSE
passwords are case-sensitive so they have binary=> TRUE
StringItem defaults to “VARCHAR”
nothing in the mysql docs tells me binary has any bearing on the max length
\Drupal\Core\Field\Plugin\Field\FieldType\StringItem::getConstraints is where the message should be coming from
but that would surely be 255
not sure if any of this is useful
so digging deeper
User forms are built with AccountForm
The password field is #type password_confirm
which is PasswordConfirm render element
it has a process callback which adds two fields of #type password
and there we find the culprit (edited) 
\Drupal\Core\Render\Element\Password::getInfo
defaults to max length 128
@quietone ^ stream of consciousness to find what’s going on here
so posting via Rest etc, you could create a password up to 255 (enforced by the data layer)
but via the UI, the max is 128 (enforced by the render element defaults)

So, I have updated the test to verify that passwords of length 128 pass but 129 fail. The IS refers to the login block, but that just uses the accountform anyway, so all should be good here. I don't think an interdiff is needed here.

I intend to make a followup to change the length to 255.

Darn, coding standard errors. And I named the patch in comment #57 with '58' instead of '57. Therefor the new patch is name '58a', couldn't think of a better option right now.

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -154,6 +154,58 @@ public function testPasswordRehashOnLogin() {
+    $session->pageTextNotContains('Password cannot be longer than.');

is the . here giving false positives?

Yes, it was giving false positives. Fixed now.

1. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -152,6 +152,58 @@ public function testPasswordRehashOnLogin() {
   +    // \Drupal\Core\Render\Element\Password::getInfo.
   

   getInfo should end with ().

2. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -152,6 +152,58 @@ public function testPasswordRehashOnLogin() {
   +    $session->pageTextContains('Password cannot be longer than 128 characters but is currently 129 characters long.');
   

   Should we just inject $length and $length + 1 in the string, so when we update $length in the future we only have to do it in one place?

Made suggested changes. Please review.

1. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -152,6 +152,58 @@ public function testPasswordRehashOnLogin() {
   +    $session = $this->assertSession();
   

   Usually we don't extract the session to a variable and always call assertSession() each time, sorry for not spotting this before.

2. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -152,6 +152,58 @@ public function testPasswordRehashOnLogin() {
   +    $session->pageTextContains('Password cannot be longer than @from characters but is currently @to characters long.', ['@from' => $count, '@to' => $count+1]);
   

   This won't work, you just need to put $length and $length + 1 directly into the string.

1. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -152,6 +152,58 @@ public function testPasswordRehashOnLogin() {
   +    $session = $this->assertSession();
   ...
   +    $session->pageTextNotContains('Password cannot be longer than');
   +    $session->pageTextContains('Member for');
   

   I still think we don't need a separate variable for $session, we usually just call $this->assertSession()->... each time.

2. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -152,6 +152,58 @@ public function testPasswordRehashOnLogin() {
   +    $session->pageTextContains('Password cannot be longer than '. $length .' characters but is currently '. ($length+1) .' characters long.');
   

   There are some coding standards issues on this line, . and + operators need spaces either side.

Added changes and applied patch according to the comment in #72
Please review

Patch is applying correctly.
If code review is fine, this could be moved to RTBCa

It was a random test failure, setting it to needs review

Thanks for the updated patch. I've reviewed the issue summary, patch, and recent comments. Based on the following, I'm marking RTBC.

1. Patch handles the issue noted in the issue summary and nothing else.

2. Code is clear and has been updated to address the most recent comments in #72.

3. Patch applies cleanly to 9.3.x and 9.4.x branches.

4. Test passes.

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -152,6 +152,56 @@ public function testPasswordRehashOnLogin() {
+  public function doPasswordLengthLogin($account, $current_password, $length) {

This method could do with some typehints and return typehints and docs.

Also I'm pretty sure the test is using deprecated methods. I.e drupalPostForm()

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -152,6 +152,56 @@ public function testPasswordRehashOnLogin() {
+    $this->assertSession()->responseContains('The changes have been saved.');

I think this should use pageTextContains().

Tested the patch from #73 does not apply with 9.4.x-dev.

Updated the patch to work with 9.4.x-dev
Replaced the deprecated functions and added type hints to functions.

Missed adding the Interdiff

Thanks for the updated patch. Back to RTBC based on:

1. Reviewed the interdiff for the feedback from #81 and everything appears to be addressed along with adding documentation and fixing coding standards.

2. Patch applies cleanly to 9.3.x (with fuzz) and 9.4.x branches.

3. Test passes.

Committed and pushed 34bccfc8138 to 10.0.x and a0ffc4bba21 to 9.4.x. Thanks!

Drupal 7 backports require there own issues as per policy now.