Problem/Motivation

When running a manual test of Drupal 6 upgrade to Drupal 8 using the migrate_upgrade UI, with a D6 site having several hundred user accounts and thus requiring more than one batch to import all the users, when proceeding to the second batch of uses the migration dies with a 404 for the path '/'. The 404 is a red herring - the main point is that importing the admin account data changes the admin account password, which regenerates the user session, which changes the CSRF token used by the batch API.

Proposed resolution

Either

  1. Fix it in batch API: #2607478: Batches broken by password change, or
  2. Make sure we don't change the admin password on migration - this would probably be less surprising to the admin user than having it set to the legacy site password, or
  3. Both.

Remaining tasks

Decide whether we should change the migrate side.

User interface changes

N/A

API changes

Presumably none

Data model changes

Presumably none

CommentFileSizeAuthor
#20 interdiff.txt1.28 KBmikeryan
#20 migration_of_uid_1-2606466-20.patch6.05 KBmikeryan
#18 interdiff.txt2.18 KBmikeryan
#18 migration_of_uid_1-2606466-18.patch6.14 KBmikeryan
#12 interdiff.txt806 bytesmikeryan
#12 migration_of_uid_1-2606466-12.patch6.12 KBmikeryan
#8 migration_of_uid_1-2606466-8.patch5.2 KBmikeryan
#7 migration_of_uid_1-2606466-7-FAIL.patch3.68 KBmikeryan

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Actually, at least some of the problem here is not having views enabled in D8, so when the migration sets the front page to /node (which is a view) then the front page is a failure. Doing something intelligent when the front page configuration we're importing leads to a non-existent path is one issue - another is the UI breaking if uid 1 is updated, not quite sure yet to what extent they're related.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

OK, the front page was a red herring - with views enabled, I have no front page problems but I still get the process interrupted. I've reduced the user table I'm migrating from so it fits in one batch, but what is happening now is that the next migration (d6_node_type) hangs and ultimately fails when *it* goes to a second batch. Logging to dblog shows that when attempting to load the next batch after the user migration, it goes into a loop reloading the batch page _batch_page() repeatedly. Each time this calls \Drupal::service('batch.storage')->load($request_id) (with $request_id == 2, which exists in the batch table) and the load fails. BatchStorage::load() looks up by both the request ID and by csrf_token - I don't know how/where that token is generated and provided, but I'm betting that saving the admin account changes it. Hmmm, I wonder if you edited the account manually in a separate tab while a batch is running if it would have the same effect... Also, symptomatically this issue recalls #2508888: Fatal error during batch operations causes endless test loop - the fix there seems unlikely in this case, but it may be worth a shot for completeness.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Title: Migration of uid 1 breaks site front page » Migration of uid 1 breaks batch upgrade process

Yep - changing the user migration back to skipping uid 1, if while migration is running I change the admin user's password in a different tab it hoses the batch processing, because a password change causes the session to be recreated. I expect this will happen with any batch processing, not just migration - I'll open a batch system issue for that, but will not be optimistic about a fix in the forseeable future.

In the meantime, it's arguable whether migration should change the admin user's password - you've just created this D8 site with a given admin password, don't you expect that password to continue to work after migration? I think the user destination should not alter the admin password anyway.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Related issues: +#2607478: Batches broken by password change

Opened issue against the batch API at #2607478: Batches broken by password change.

My original thought was that it would be hard to fix this while maintaining CSRF safety, but then it occurred to me that User::postSave() could fix up the batch table. Need some discussion about whether to pursue that for 8.0.0, or as suggested above have EntityUser::import() make sure the admin account password isn't changed.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue summary: View changes
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
StatusFileSize
new migration_of_uid_1-2606466-7-FAIL.patch3.68 KB

Pursuing the skip-admin-password approach, here's a test-only patch.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new migration_of_uid_1-2606466-8.patch5.2 KB

With the fix. Note this requires adding a method removeDestinationProperty() to the Row class, which seems sensible.

The last submitted patch, 7: migration_of_uid_1-2606466-7-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: migration_of_uid_1-2606466-8.patch, failed testing.

The last submitted patch, 8: migration_of_uid_1-2606466-8.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new migration_of_uid_1-2606466-12.patch6.12 KB
new interdiff.txt806 bytes

The existing D6 user migration test was expecting the admin password to be migrated, changed to only test non-admin passwords.

Status: Needs review » Needs work

The last submitted patch, 12: migration_of_uid_1-2606466-12.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review

OK, ready for manual review.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: +rc target triage

The last submitted patch, 7: migration_of_uid_1-2606466-7-FAIL.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
  1. +++ b/core/modules/migrate/src/Row.php
@@ -223,6 +223,17 @@ public function setDestinationProperty($property, $value) {
+   * Remove destination property.

    Nit: s/Remove/Removes/ 

  2. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
@@ -90,6 +90,10 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+    // Do not overwrite the admin account password.

    Don't we usually say "root"? 

  3. +++ b/core/modules/user/src/Tests/Migrate/MigrateUserAdminPassTest.php
@@ -0,0 +1,117 @@
+ * Tests rolling back of imports.

    C/P remnant. 

  4. +++ b/core/modules/user/src/Tests/Migrate/MigrateUserAdminPassTest.php
@@ -0,0 +1,117 @@
+   * @var array

    string[] 

  5. +++ b/core/modules/user/src/Tests/Migrate/MigrateUserAdminPassTest.php
@@ -0,0 +1,117 @@
+   * @var array

    string[] 

  6. +++ b/core/modules/user/src/Tests/Migrate/d6/MigrateUserTest.php
@@ -119,8 +119,11 @@ public function testUser() {
+      // conform the Drupal >= 7 for non-admin users.

    "conform the Drupal 7" sounds strange. s/the/to/ ?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new migration_of_uid_1-2606466-18.patch6.14 KB
new interdiff.txt2.18 KB

All feedback addressed, thanks!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Looks good, but two minor things -- 

  1. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
@@ -90,6 +90,10 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+    if ($row->getDestinationProperty('uid') == 1 && $row->getDestinationProperty('pass')) {
+      $row->removeDestinationProperty('pass');
+    }

    I don't think we need to check if the 'pass' property is set. Let's just unconditionally kill it if it's uid 1. 

  2. +++ b/core/modules/user/src/Tests/Migrate/MigrateUserAdminPassTest.php
@@ -0,0 +1,117 @@
+      'migration_tags' => ['Admin password test'],

    We're tagging test migrations now? :) 

  3. +++ b/core/modules/user/src/Tests/Migrate/MigrateUserAdminPassTest.php
@@ -0,0 +1,117 @@
+    $executable = new MigrateExecutable($migration, $this);
+    $executable->import();

    Should be $this->executeMigration($migration)

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new migration_of_uid_1-2606466-20.patch6.05 KB
new interdiff.txt1.28 KB

We're tagging test migrations now? :)

As we have been, yes. The strict schema checking insists migration_tags be set - if it's there anyway, no reason not to put a value in.

Other requested changes have been made.

quietone’s picture

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

Tested and confirmed this works. Since phenaproxima and Wim Leers have looked at this, I'll RTBC.

The last submitted patch, 7: migration_of_uid_1-2606466-7-FAIL.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -rc target triage +rc eligible

This seems to be isolated to Migrate so should be RC eligible. Also gets the UI working again, which is fantastic.

Committed and pushed to 8.0.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Oopsie, credit.

  • webchick committed b841a56 on 8.0.x 
    Issue #2606466 by mikeryan, Wim Leers, phenaproxima, quietone: Migration...

  • webchick committed b841a56 on 8.1.x 
    Issue #2606466 by mikeryan, Wim Leers, phenaproxima, quietone: Migration...

Status: Fixed » Closed (fixed)

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