Problem/Motivation

After running the user migration from D7 to D8 all user passwords seems invalid, e.g. It's impossible to login in D8. I noticed this today because of #2560637: Improve handling of uid 1 during migration.
Before this I was able to login with the user name and pass given during site install by drush si. Now the original (D7) admin user is mapped to be the new admin user in D8 with the original password. I tried to login with some other user accounts too, but with no luck.
After doing a drush uli and enter a new password for the admin I'm able to login back again. I'm using PostgreSQL 9.3 and migrating from a PostgreSQL 9.1 version. So this could be a PostgreSQL only issue! I didn't have the time to see if we have a migration test verifying the successful login after a migration, but guess we don't. I think this is a migrate critical so adding as such.

Questions:

  1. As a first analysis: Is this issue PostgreSQL only? - NO
  2. Is this because of bytea_escaping? - NO

Proposed resolution

This issue is occurring because the PasswordItem will hash all incoming values. This means the Drupal 7 password is hashed again - which breaks it. At the moment we wrap the password service with one that migrate can change its behaviour for Drupal 6 passwords. However this wrapping is always on even if the migrate module is not installed.

The current patch:

  • Adds a flag to PasswordItem to say that the password is already hashed so it won't be hashed by its preSave() method
  • Removes the password service wrapper
  • Uses the new flag for both Drupal 6 and Drupal 7 passwords during a user migration

This patch results in less run-time complexity for all sites and a cleaner approach to migrating passwords because now we have one less stateful service.

This patch is acceptable for 8.1.x because UserServiceProvider is not API and the MigratePassword is part of migrate (although it's in the user module) and therefore experimental. MigratePassword also effects all existing sites because every time a user logs in there are unnecessary method calls.

Remaining tasks

  1. Write patch
  2. Write tests

User interface changes

None.

API changes

  • New pre_hashed flag on PasswordItem
  • Removes MigratePassword and the UserServiceProvider which performed a weird decoration of the password service
Files: 
CommentFileSizeAuthor
#46 2598038-46.patch13.48 KBalexpott
#46 41-46-interdiff.txt1.65 KBalexpott
#41 2598038-41.patch13.37 KBalexpott
#41 38-41-interdiff.txt10.41 KBalexpott
#38 2598038-38.patch7.87 KBalexpott
#38 34-38-interdiff.txt7.25 KBalexpott
#34 2598038-34.patch5.99 KBalexpott
#34 33-34-interdiff.txt1.28 KBalexpott
#33 2598038-33.patch5.47 KBalexpott
#33 32-33-interdiff.txt1.46 KBalexpott
#32 2598038-32.patch4.78 KBalexpott
#26 interdiff-20-26.txt1.87 KBquietone
#26 migrate_user_passwords-2598038-26.patch4.81 KBquietone
#20 interdiff-2598038-19-20.txt1.56 KBquietone
#20 migrate_user_passwords-2598038-20.patch4.8 KBquietone
#19 migrate_user_passwords-2598038-19.patch4.15 KBJaesin
#19 interdiff_2598038_17-19.txt1.73 KBJaesin
#17 interdiff-migrate_user_passwords-2598038-15-17.txt2.26 KBquietone
#17 migrate_user_passwords-2598038-17.patch4.08 KBquietone
#15 migrate_user_passwords-2598038-15.patch2.13 KBsethcardoza

Comments

bzrudi71 created an issue. See original summary.

svendecabooter’s picture

Issue tags: -PostgreSQL

I'm experiencing the same behaviour with MySQL, so doesn't seem PostgreSQL-only.
In the D6 > D8 migration there is an option "md5_passwords" to migrate the D6 MD5 passwords to D8, which works OK in my tests.
In D7 > D8 there doesn't seem to be any special logic to handle the passwords. I'm not familiar enough with the D8 password hashing to suggest a fix...

bzrudi71’s picture

Issue summary: View changes

@svendecabooter, thanks for MySQL testing! So it seems we have a problem here ;) If I get you right, user migration from D6->D8 works, D7->D8 fails. This is because the password hashing changed between D6 and D7. For D6 it's md5 and we take care of that by adding the md5_passwords tag during migrate. I think that is also needed for a D7 to D8 migration, because like in my case, my D7 was migrated from D6 earlier and has a mix of 'old' and new passwords. For the D7 passwords we need to see what is going wrong...

svendecabooter’s picture

Both Drupal 7 and Drupal 8 are using SHA512 encryption, with a hash salt.
I tried modifying my Drupal 8 settings.php, to include the same salt as my Drupal 7 installation:
$settings['hash_salt'] = 'SALT HERE';

Though that doesn't seem to allow me to use my D7 password in my tests.

pedrorocha’s picture

It happened to me on MySQL also, but I migrated on Beta-12, if i'm not wrong, so I thought it would be solved soon.

I tried
* a simple migration
* forcing a new hash, like with 6 to 8
* using the same hash salt

None of these worked, so I ended up:
* copying every password hash to a temporary table on my D8 installation
* created a code on my D8 installation to check on each login if the user is on that D7 users table(I copied D7 login hash code to match the hashes)

It's not the solution for D8 to be really ready, but if anyone needs this for now, can take a look at what I did on https://gist.github.com/pedrorocha-net/c37920043d6fee5d1fee

P.S.: there are some pieces on my code related to user picture that can be ignored

sethcardoza’s picture

I've found that during a D7 to D8 migration, the password is hashed again, because the content is treated as new (as it should), but the password is already hashed. I added a flag to the user entity in the EntityUser class, which gets checked in the PasswordItem::preSave() method, and skips hashing if TRUE. We were successfully able to log in to our D8 site after applying this patch.

sethcardoza’s picture

Status: Active » Needs review
FileSize
1.64 KB

Here's the patch.

davidwbarratt’s picture

Issue tags: +Needs tests

Status: Needs review » Needs work

The last submitted patch, 7: migrate_user_passwords-2598038-7.patch, failed testing.

sethcardoza’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

Updated patch with proper paths.

Status: Needs review » Needs work

The last submitted patch, 10: migrate_user_passwords-2598038-9.patch, failed testing.

sethcardoza’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

Now with the required use statement.

davidwbarratt’s picture

Status: Needs review » Needs work

The last submitted patch, 12: migrate_user_passwords-2598038-12.patch, failed testing.

sethcardoza’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Fixing issue in patch with D6 migration.

davidwbarratt’s picture

quietone’s picture

A simple migration test for the user password.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
@@ -46,7 +46,7 @@ public function preSave() {
     // Update the user password if it has changed.
-    if ($entity->isNew() || (strlen(trim($this->value)) > 0 && $this->value != $entity->original->{$this->getFieldDefinition()->getName()}->value)) {
+    if (!$entity->is_hashed && ($entity->isNew() || (strlen(trim($this->value)) > 0 && $this->value != $entity->original->{$this->getFieldDefinition()->getName()}->value))) {

+++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
@@ -111,6 +112,16 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+  protected function save(ContentEntityInterface $entity, array $old_destination_id_values = array()) {
+    if (!$this->password) {
+      $entity->is_hashed = TRUE;
+    }

I kinda would have expected the flag to be on the field item instead of the entity itself. In general we should add some quick line of documentation.

Jaesin’s picture

The way MigratePassword works, all passwords are re-hashed.

I don't see how you would get "U$H$" or "U$P$" into the DB at all. It will always be "U$S$".

When the password check happens, md5() is ran on the form value before re-hashing against the standard "$S$" (sha512) password algorithm so you would still be able to log in. The only thing "destination: md5_passwords: true" does is append the "U" so the first check gets md5 hashed before running through the sha512 algorithm.

That said, I the attached patch sets the flag on the field instead of the entity.

quietone’s picture

@Jaesin, thx. Tested this today and it works very well.

The only change I made is an attempt to reduce a very long line and add comments in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php. I'm having some odd testing issues locally that I've not sorted out yet. But lets see what the testbot finds.

vasi’s picture

Status: Needs review » Needs work

I feel like there should be a configuration setting for this. It makes sense for D7 password hashes, but this is supposed to be a destination for everything, not just d2d. What if you're migrating in something else with plaintext passwords?

quietone’s picture

@vasi, for me, making the password migration generic is different from fixing a problem specific to D7->D8 migration. Perhaps a new issue?

quietone’s picture

Issue tags: +migrate-d7-d8
benjy’s picture

+++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
@@ -111,6 +112,17 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+    if (!$this->password) {
...
+      $entity->pass->pre_hashed = TRUE;

+++ b/core/modules/user/src/Tests/Migrate/d7/MigrateUserTest.php
@@ -49,6 +49,8 @@ protected function setUp() {
+   * @param string $passwd

We have $passwd, $password and $pass at play here, can any of them be standardised?

alexpott’s picture

Issue tags: +Triaged D8 major

Discussed with @xjm and @catch and we agreed that this is a major bug - if migrate was not experimental it would be critical.

quietone’s picture

Changed all passwd to password in MigrateUserTest.php.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 26: migrate_user_passwords-2598038-26.patch, failed testing.

xjm’s picture

Priority: Major » Critical
Issue tags: -Triaged D8 major
Related issues: +#1921822: Take account of drupal_hash_salt during migration path from 7.x

Discussed with @mikeryan, @alexpott, @weal, @benjifisher and we agreed that this is probably the most critical of outstanding critical issues. @catch and I discussed earlier promoting some Migrate criticals to critical priority for visibility and this one qualifies I think.

mikeryan’s picture

s/@benjy/@benjifisher/ (alas, benjy isn't in NOLA).

alexpott’s picture

After further discussion, we realised that in order to compare passwords we're going to need the d7 hash salt somewhere. This is challenging because the Drupal 8 site already exists so changing the hash salt after install is not possible. We think that we need to add a legacy hash salt to settings.php and prevent user migration until set. During migration we need to prefix passwords so that on login it will check the legacy hash salts and the rehash the password using the current salt.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.78 KB

Rerolled the patch

alexpott’s picture

#31 is wrong. In Drupal 7 we store the salt along with the hash. And as the password hashing and slating has not changed between d7 and d8 we shouldn't need to make changes here. However this means we still need to consider #1921822: Take account of drupal_hash_salt during migration path from 7.x and how to handle that - but that is a much smaller problem.

alexpott’s picture

:( I should have recalled some of the password portability discussions. Anyhow, we can test a little bit more of the d7 migration. The password will be rehashed because we've increased the

alexpott’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
@@ -40,13 +40,18 @@ public function preSave() {
+    // Skip if the password is pre hashed. This is set when migrating users from
+    // Drupal 7.
+    if (!$this->pre_hashed) {

I don't think building this into the generic password item is a good idea. I think we need to swap out the PasswordItem for MigratePasswordItem on the user entity. If something manages to set the pre_hashed flag in other code then plaintext password will be stored in the db.

The last submitted patch, 33: 2598038-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 34: 2598038-34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.25 KB
7.87 KB

So we can take the same approach as the Drupal 6 user migration and alter the MigratePassword service to handle this for us. And then we don't need any changes to PasswordItem - which seems sane.

alexpott’s picture

+++ b/core/modules/user/src/MigratePassword.php
@@ -21,7 +21,15 @@ class MigratePassword implements PasswordInterface {
   /**
    * Indicates if MD5 password prefixing is enabled.
    */
-  protected $enabled = FALSE;
+  protected $md5Prefixing = FALSE;

I made this change because a property called $enabled was just confusing - and even more confusing once enableHashing() and disableHashing() are added.

quietone’s picture

Tested this with a devel generated D7 site. I edited the passwords for two of the users and ran the migration. Only the two users with modified passwords were migrated, those with empty passwords were not. But those two passwords were migrated successfully, that is, I could log in as those users.

alexpott’s picture

Did some more thinking about how password fields are migrated. At the moment the user module replaces the password service with the MigrateUser variant. This happens regardless of whether we're migrating or not. So basically we're running a whole level of unnecessary misdirection on ALL Drupal 8 sites for the password hashing service. The patch attached removes this and introduces the pre_hashed flag to the PasswordItem field type. This is the same as the original direction of the patch BUT it also uses this for Drupal 6 passwords just to make everything consistent and easier to understand. Also the MigrateUser password service is stateful - the last patch in #38 had two different flags to control it's operation. In the patch attached all this complexity is removed.

Status: Needs review » Needs work

The last submitted patch, 41: 2598038-41.patch, failed testing.

The last submitted patch, 41: 2598038-41.patch, failed testing.

mikeryan’s picture

alexpott’s picture

Discussed with @catch, @xjm, @effulgentsia, @Cottser and @webchick. We agreed that this is obviously critical.

Status: Needs review » Needs work

The last submitted patch, 46: 2598038-46.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review

Ignore that round of testing, I had some half-assed idea regarding the random test failures...

alexpott’s picture

Issue summary: View changes
catch’s picture

It's unfortunate that we have to do a schema update in user module to support this. With hindsight adding a new hash prefix to 8.x would have allowed us to detect pre-hashed-ness without the extra property. Do we really not have that?

chx’s picture

Field items are plugins. Swap the plugin class during migration. chx out.

catch’s picture

We should be able to modify the prefix of the 'normal' 7.x passwords to something else during the migration.

Then we can add a line in PhpassHashedPassword::check which looks explicitly for that prefix and hashes based on the old hash salt.

Shouldn't need the extra column - we already have code dealing with multiple differently-hashed passwords for 6-7 and phpbb-7 etc. migrations, and can rely on the same logic here.

Migration itself would still need to suppress the re-hashing, but chx's suggestion in #51 covers that.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
@@ -28,6 +28,8 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
+    $properties['pre_hashed'] = DataDefinition::create('boolean')
+      ->setLabel(new TranslatableMarkup('Determines if a password needs hashing'));

This does not result in an extra column - otherwise we'd have an existing password column everywhere.

I thought about swapping out the password field item class on migrate but that seemed overly complex. In my mind it is a shame that we enforce hashing on a new user. Having the field magically change value on preSave is pretty tricky to deal with sometimes.

alexpott’s picture

I thought about swapping out the PasswordItem class in the migrate module - but then we still need a flag and we'd need to find all the fields using it. I think having the pre_hashed functionality available all the time will prove simpler for all possible password fields not just the user password field. We can't just swap it out when migrate is installed and not do the hashing because then you couldn't create any users through the front-end until migrate is uninstalled.

Re hash salt - user passwords are self salted and the salt is contained in the password so we don't need to worry about the old site's global hash salt and the re-hashing of Drupal 7 users' passwords when they log in to the Drupal 8 site after migration is tested in the new tests.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Yep I had to refresh my memory on a few things.

- 6-7 and phpbb-7 logic is all retained in 8.x, and this patch will cover those too since the hashes are preserved.
- hash_salt isn't used for passwords
- hash iterations/log_count is encoded in the hash so doesn't need any special handling when there's an increment
- properties doesn't mean columns

I can see 'this is already hashed' being useful for some other things, and it's very clear that it should only be used in certain cases, so happy here now. Moving to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: 2598038-46.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review

Hrrm, that failure doesn't look like it has anything to do with the patch - let's retest (and do an 8.2.x test while we're at it).

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

All passing, restoring RTBC.

benjy’s picture

+++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
@@ -83,24 +80,29 @@ public static function create(ContainerInterface $container, array $configuratio
+      if (isset($this->configuration['md5_passwords'])) {
+        $entity->pass->value = 'U' . $this->password->hash($entity->pass->value);
+      }

Are we testing this anywhere?

alexpott’s picture

@benjy yep in MigrateUpgrade6Test we're testing someone with a D6 password logging in and \Drupal\Tests\user\Kernel\Migrate\d6\MigrateUserTest also has existing tests for this. The addition of the U was already occurring in HEAD - just in a different place.

  • catch committed b8a61f2 on 8.2.x
    Issue #2598038 by alexpott, sethcardoza, quietone, Jaesin: Invalid...
catch’s picture

Status: Reviewed & tested by the community » Needs review

Committed/pushed to 8.2.x

Moving to 8.1.x at CNR, I haven't fully thought through the non-migrate changes here for a patch release yet.

  • catch committed b4c3f70 on 8.1.x
    Issue #2598038 by alexpott, sethcardoza, quietone, Jaesin: Invalid...
catch’s picture

Status: Needs review » Fixed

Talked this through with legolasbo and alexpott. The migrate bits should not be relied on, and there's barely theoretical breakage with the user changes. Given the severity of the bug, cherry-picked to 8.1.x - if it turns out we did break something there's still a bit of time before the next patch release to flush it out.

Status: Fixed » Closed (fixed)

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