Unable to stay logged in during upgrade from D6 -> D7

I am subscribing because I am unable to stay logged in after changing hosts from shared to vps.

Drupal 6 but still interested if you get any responses.

Your comment about .htaccess caused me to recall I had disabled boost module which had some .htaccess modifications.

Enabling the module resolved my problem.

So thank you and hope you get or got a resolution.

I just tried an upgrade (in order to test #949102: Polish UPGRADE.txt), and I got logged out too.

1. Logged in as user 1
2. Set the site in maintenance mode
3. Replaced files as described in http://drupal.org/files/issues/UPGRADE_20.txt
4. Went to mysite.com/update.php
5. Got the following message:

Access denied
Access denied. You are not authorized to access this page. Log in using either an account with the administer software updates permission or the site maintenance account (the account you created during installation). If you cannot log in, you will have to edit settings.php to bypass this access check. To do this:

1. With a text editor find the settings.php file on your system. From the main Drupal directory that you installed all the files into, go to sites/your_site_name if such directory exists, or else to sites/default which applies otherwise.
2. There is a line inside your settings.php file that says $update_free_access = FALSE;. Change it to $update_free_access = TRUE;.
3. As soon as the update.php script is done, you must change the settings.php file back to its original form with $update_free_access = FALSE;.
4. To avoid having this problem in the future, remember to log in to your website using either an account with the administer software updates permission or the site maintenance account (the account you created during installation) before you backup your database at the beginning of the update process.

6. Tried mysite.com/user but got an error message
7. Changed settings.php as described
8. Ran update.php successfully

I think the problem is that we are rehashing all user passwords during the upgrade #29706: More secure password hashing and keeping the session cookie after this is done is rather difficult.

I've been bitten by this, too. I seem to have to set $update_free_access = TRUE every time I attempt an upgrade. I don't think this has to do with .htaccess at all, however. It would be nice to work this out since setting $update_free_access = TRUE is neither convenient nor secure.

This has been annoying me too.

I believe it resulted from #723802: convert to sha-256 and hmac from md5 and sha1, which changed Drupal's session generation code from using md5() to sha256 hashes. So the Drupal 6 session you have in your cookie is totally unrecognized by Drupal 7.

In particular, compare the last lines of these functions:
Drupal 6: http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/conf_i...
Drupal 7: http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...

Not sure if we could fix this - maybe update.php could have some code which (only for D6->D7 upgrades, and only for user 1), sets a new cookie if it sees you have a valid old cookie that matches a session in the database?

@David that sounds about right. Here's a quickie rewrite of update_access_allowed() as a possible approach.

function update_access_allowed() {
  global $update_free_access, $user, $base_url;

  if (!empty($update_free_access) || $user->uid == 1) {
    return TRUE;
  }

  // Check for a valid D6 session.
  list( , $session_name) = explode('://', $base_url, 2);
  if (ini_get('session.cookie_secure')) {
    $session_name .= 'SSL';
  }
  $session_name = 'SESS' . md5($session_name);

  if (!empty($_COOKIE[$session_name])) {
    $is_admin = db_query("SELECT 1 FROM {sessions} WHERE uid = 1 AND sid = :sid",
      array(':sid' => $_COOKIE[$session_name])
    )->fetchField();
    if ($is_admin) {
      $user = db_query('SELECT * FROM {users} WHERE uid = 1')->fetchObject();
      $user->data = ...
      $user->roles = ...
      drupal_session_regenerate();
      return TRUE;
    }
  }

  // Check for the 'administer software updates' permission.
  try {
    require_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'user') . '/user.module';
    return user_access('administer software updates');
  }
  catch (Exception $e) {
    // Ignore exceptions which may happen on an un-upgraded site.
  }
  
  return FALSE;
}

I'm wondering if this could be a more widespread problem than this report indicates - I'm trying to upgrade a test system from D6.16 to D7b3 on a local linux box. Upgraded successfully to 6.18, then followed UPGRADE.txt for D7. upgrade.php ran successfully, but I was unable to access admin pages (404 errors). I have a separate D7b3 install (different directory and DB), so cut and pasted urls from there, modifying for the correct Drupal installation, and got the same (404) error, regardless of the admin or user page sought.

The upgraded site shows up correctly, but I can not access the admin screens. One page I tried did contain the login fields, but my login (correct) was not accepted and I could not log in for about 45 minutes. Now I can log in, but clicking on any admin link (such as Configuration) gives 404 error.

Could this be related to clean URLs? I tried to reset them (they were being used on the D6.16 system), but I can not access the pages to alter them after the upgrade.

@SolaFide001, 404 (page not found) errors are not to be confused with 403 (access denied) errors, so that is not entirely relevant here. It sounds to me like you have the clean URL option enabled but the .htaccess file is missing or not properly rewriting those URLs. If you need more help, please open a support ticket or try IRC #drupal-support.

Thanks @carlos8f - I posted this originally because I was getting access denied errors, which later became 404 errors.

As an aside, I was using the default .htaccess file from D7b3. I removed it to see if there was any change, and there was not.

@carlos8f it looks like your approach introduces a requirement that the upgrade be performed by UID 1 which we don't currently have.

Also you may as well use user_load() rather than building up the user from scratch. We don't do that during a regular page load for performance sake, but here the code efficiency is probably worth it.

@dalin, for an upgrade, we do require either a UID 1 session or $update_free_access=TRUE. So my approach is not introducing anything new. Due to hashing changes, D6 sessions no longer work once the D7 code is uploaded, which means you must use $update_free_access. I think setting that variable should be a last resort, not a requirement to upgrade.

I'd like to raise this to 'major', as it touches the upgrade path and affects anyone attempting to upgrade.

Also, user_load() is probably broken at this point in update.php since we have D7 code and a D6 database. That's my reasoning for building $user from scratch.

Ah of course, I obviously need to take a nap. Everything that you've said is sound.

Well I was kind-of right. Your approach doesn't work if the user is not UID 1, but $update_free_access == TRUE. I also found what may be an easier way of loading the user object - by calling _drupal_session_read(). I manually tested an upgrade and it works as expected - you stay logged in.

Patch looks great. Let's get some other opinions though. I think this is close to RTBC.

Hm, this didn't seem to work for me when I tested it out :( I didn't have time to debug further, but will try to soon.

Apart from that, this looks good except for a couple of things:

• I think a function like update_access_allowed() ideally shouldn't have side effects. Plus, we don't need to run this session stuff every time update.php runs, just during the D6->D7 upgrade. Is there a reason this couldn't run in update_prepare_d7_bootstrap() - or if not there, then in a separate function that runs a little after? In update_prepare_d7_bootstrap(), there is already code like:
  

    $system_schema = drupal_get_installed_schema_version('system');
    if ($system_schema < 7000) {
      ...
    }
  

  which could be used to make sure this only can happen during the D6->D7 upgrade, never again.

• Looking at how the D6 code builds up the session name, if the global $cookie_domain variable is set it also uses that, so it seems we would need to reproduce that here also so we know we are checking the correct session name in all cases?

My only other thought is that it seems this patch will leave an odd backdoor where if any user of the site hits update.php before updates are run (even without access to it), they can regenerate their session and prevent themselves from getting logged out during the D6->D7 upgrade? I guess that ought to be completely harmless, but somehow it makes me feel a tiny bit uncomfortable anyway :)

Tried the patch in #14

In my case it didn't work. This concerns more the session variable than anything on my attempt. Since the other issue was closed as duplicate I posted here.

Senario,

A d5 system updated to 6.19
user 1 is NOT logged in, $update_free_access is set to TRUE.
system clogs around updating (extremely slow)
update completes.

Ajax error box pop's up with

* Notice: Undefined index: update_success in update_results_page() (line 162 of C:\sites\d6d7\update.php).
* Warning: reset(): Passed variable is not an array or object in update_results_page() (line 166 of C:\sites\d6d7\update.php).
* Warning: array_pop(): The argument should be an array in update_results_page() (line 166 of C:\sites\d6d7\update.php).

I can log into the updated site. it ask me to change my timezone setting and works. This is an old bug and every patch I've tried hasn't worked.

It occurred to me that we might be able to work this into the upgrade tests -- instead of creating a user #1 session with D7 hashing in UpgradePathTestCase::setUp(), we could create one with D6 hashing and have the upgrade convert that session.

This patch combines the ideas of #16 and #18, and works in theory (the upgrade tests are passing, but I haven't tried an actual upgrade).

I tried an actual upgrade and it didn't work. I found the cause to be that $cookie_domain causes the session name assignment to not be idempotent. So you get two different session names running that logic twice, if $cookie_domain isn't set in $conf.

I removed support for $cookie_domain then, which should only affect a minority of sites. I successfully stayed logged in through an upgrade with this patch.

Applied the patch in #19 to dec. 4 tarball, same results

* Notice: Undefined index: update_success in update_results_page() (line 162 of C:\sites\d6d7\update.php).
* Warning: reset(): Passed variable is not an array or object in update_results_page() (line 166 of C:\sites\d6d7\update.php).
* Warning: array_pop(): The argument should be an array in update_results_page() (line 166 of C:\sites\d6d7\update.php).

Could this possibly be related to ajax losing data? just pondering, not familiar enough with bootstrapping to know where to start looking.

[note: since this is an edit to correct wrong patch #, currently running update on #20 now, will post back results]

@ctmattice1,

Since the other issue was closed as duplicate I posted here.

Can you specify what issue was closed as a duplicate? From the looks of your error report it doesn't look related to this issue at all.

@carlos8F - the other is #826640: Update results page missing session data, causing "Undefined index: update_success" notice and "aborted prematurely" message

but in that issue I have a user 0 until I run the upgrade. the unassigned index $update_sucess that shows in that issue is repeated in this issue. There are others as well, can't remember, some open, some not.

I REALLY don't care if this get's fixed now or not. It is a nuance more than anything else.

I picked this issue as it was similar.

Yes on update I am logged out.
Yes $update_free_access is set to TRUE
Yes I run the update and get errors
Yes this is probably D8 material now.
No I do not have a user 0 after upgrading but can add it to correct other issues.
No Patch in #20 did not work either on this particular db update attempt.
Yes it updates, yes I can log in, yes it is still in maintenance mode (this was never a issue I experienced)

Still annoying and I haven't been able to figure out this one at all so I keep searching. Any suggestions??

@ctmattice1, if you have no user 0, something went wrong when you copied the database, so this is not a bug in Drupal. I created an issue for D8, #989852: Kill uid=0 requirement, to get rid of this dependency since it's very commonly missing in databases copied with phpMyAdmin, etc.

The patch in #20 worked for me.

I thought we could make the cookie domain stuff work by removing the part of patch #19 that makes it non-idempotent (since this code isn't necessary for what we're doing anyway):

+    if (!empty($_SERVER['HTTP_HOST'])) {
+      $cookie_domain = check_plain($_SERVER['HTTP_HOST']);
+      // Strip leading periods, www., and port numbers from cookie domain.
+      $cookie_domain = ltrim($cookie_domain, '.');
+      if (strpos($cookie_domain, 'www.') === 0) {
+        $cookie_domain = substr($cookie_domain, 4);
+      }
+      $cookie_domain = explode(':', $cookie_domain);
+      $cookie_domain = '.' . $cookie_domain[0];
+    }

But the deeper part of the problem is that drupal_settings_initialize() in Drupal 7 will set $cookie_domain for everyone as soon as it's called, so we can't (easily) distinguish between people who actually had $cookie_domain defined in their settings.php and people who did not. By the time our code gets called, everyone has it defined.

There's probably a way to fix that, but I guess it's not worth the effort unless someone clamors for it.

#20 works for me. Patch generally looks good. I do see that this code runs on every page load during the update process. Would it make sense to make it just run once?

@carlos8f - "if you have no user 0, something went wrong when you copied the database"

Thanks, I checked my user table and found that the copied database did not have uid 0. I added it, ran the update and it took care of the error. I guess I should have picked up on this on the other issues but took for granted that phpmyadmin would do an exact copy. Ha, think I need to think twice about making assumptions. I'll follow the new issue #989852: Kill uid=0 requirement you started.

@David, that was what I was trying to say (at 3am) but you explained it better -- at this point in the bootstrap, it's hard to tell whether $cookie_domain is set in $conf or not. This latest patch attempts to load settings.php and only use $cookie_domain if it is set there.

@dalin, it would make sense to regenerate the session only once, but there are complications. For example under SSL, D7's session handling requires a new 'ssid' field that doesn't exist at the time of our code. I messed around with various approaches and this was the best I could come up with that actually works.

Oops, missed a semicolon.

+  // $cookie_domain may be specifically set in settings.php. Load it here
+  // because $cookie_domain can be overwritten in drupal_settings_initialize().
+  if (file_exists(DRUPAL_ROOT . '/' . conf_path() . '/settings.php')) {
+    include DRUPAL_ROOT . '/' . conf_path() . '/settings.php';
+  }

If this is the case shouldn't we be calling drupal_settings_initialize() again at the end of this function to set things back.

No, update_get_d6_session_name() does not modify any globals. It's a read-only function, and it tries to read the original value of $cookie_domain from settings.php, but doesn't change it.

Ah I see now.

I think this is good-to-go.

Can't including settings.php twice on the same page request have unintended side effects? I think it can. (I've seen people put all sorts of interesting custom logic in settings.php for various reasons.)

An alternative might be to have drupal_settings_initialize() store the original cookie domain from settings.php in a separate global variable, $cookie_domain_original or something, before it thinks about modifying it. That would allow anyone who needed the same information that we need here to get at it also, with no side effects.

Ah right. I've run into that exact trouble before so I should've caught that. This one uses $cookie_domain_original.

+++ includes/bootstrap.inc	13 Dec 2010 09:50:02 -0000
@@ -592,6 +592,12 @@ function drupal_settings_initialize() {
+    // hard-coded in settings.php. ¶

Trailing whitespace.

+++ includes/bootstrap.inc	13 Dec 2010 09:50:02 -0000
@@ -592,6 +592,12 @@ function drupal_settings_initialize() {
+    if ($cookie_domain_original === NULL) {

I would use !isset($cookie_domain_original) here. === NULL is a little hard to grok unless you realize that $cookie_domain_original may be initialized to NULL when declared a global.

+++ includes/update.inc	13 Dec 2010 09:50:03 -0000
@@ -156,6 +156,19 @@ function update_prepare_d7_bootstrap() {
+      $session = _drupal_session_read($sid);
+      var_export($session, TRUE);

Er...

+++ includes/update.inc	13 Dec 2010 09:50:03 -0000
@@ -829,6 +842,41 @@ function update_parse_db_url($db_url, $d
+  if (!empty($cookie_domain_original)) {

Since you made sure that $cookie_domain_original is initialized, you can simply do if ($cookie_domain_original)

+    if ($cookie_domain_original === NULL) {
+      $cookie_domain_original = $cookie_domain;
+    }

As @carlos8f said, at least this should use isset(), but do we even need the check at all? I can't think of a scenario where we want or expect this variable to have been set by someone else beforehand.

     // HTTP_HOST can be modified by a visitor, but we already sanitized it
     // in drupal_settings_initialize().
     if (!empty($_SERVER['HTTP_HOST'])) {
+      $cookie_domain_original = '';
       $cookie_domain = $_SERVER['HTTP_HOST'];

I'm thinking we probably don't need that either. Couldn't we just leave $cookie_domain_original unset if it's not being used?

Otherwise, this patch looks pretty good - I think it's close to being ready. I haven't dug 100% deeply into the session stuff yet, but I know from previous patches (and the test) that it seems to be working :)

@David, sounds good. I'll give it a re-roll tonight.

Here are the changes:

- Addressed #35 and #36.
- $cookie_domain_is_custom instead of $cookie_domain_original (TRUE flag instead of a semi-useless copy)
- Ripped out superfluous comments in update_get_d6_session_name() copied from drupal_settings_initialize().

I think this is pretty close to RTBC too.

Ah I tried to beat Carlos to the patch but ran out of time. My reasoning for the apparent oddness with NULL vs. ''; Is that drupal_settings_initialize() can be called twice in one page load. Doing so a second time without the funny logic would overwrite a previous value. While it would be odd to do so, there is nothing that prevents calling it twice.

@dalin I don't see the tangible benefit from calling drupal_settings_initialize() twice, or bending over backwards to facilitate that :)

I tested the patch in #38 on a clean D6.19 install and it works fine: I stay logged in as an admin and can complete the upgrade without having to edit settings.php.

I still see some minor code clean up to be done (removing excess space around string constants)

@george, not sure what you mean about excess space.

My bad, seems my information wasn't up to date: http://drupal.org/coding-standards#concat (sometimes still living in D4.6 times :)

I was confused about $cookie_domain_is_custom for a second but then realized that it's because $cookie_domain never gets modified in the case where the user provides it in settings.php (only in the other case). So that definitely makes sense.

Considering that calling drupal_settings_initialize() twice can currently cause problems anyway, I agree we don't have to try to start supporting it here.

I think this one is good!

Not close to ready.

+      require_once DRUPAL_ROOT . '/includes/session.inc';

"session.inc" is pluggeable. You need to use the proper variable here.

+      _drupal_session_read($sid);

This is a private function of the session implementation. You just cannot call it directly. I think you want session_start() here.

The rest of the patch is not very very pretty. What we actually care about is $session_name... if $cookie_domain is the session name (that we can check by recomputing the hash), we should just use that, if it isn't we should rebuild it using the D6 algorithm (explode('://', $base_url, 2)). I'm not convinced there is any need to modify drupal_settings_initialize() at all.

I don't think session_start() will work here. And supporting this behavior for people using pluggable sessions during the D6->D7 upgrade seems like a nice-to-have :)

But..... is there any reason we can't just remove those two lines from the patch entirely? I.e. these lines:

  require_once DRUPAL_ROOT . '/includes/session.inc';
...
  _drupal_session_read($sid);

I tried it quickly and it seemed to work. Essentially, we'd still be modifying the cookie/etc in this part of the code, but then let Drupal read the session at the same part of the bootstrap that it normally would.

The idea of checking $cookie_domain by recomputing the hash also seems to make a lot of sense.

Wow, thanks @Damien and @David, this patch got a lot simpler. I ran the tests successfully after setting a custom $cookie_domain, so it looks like that part is working. Nice.

+  global $base_url, $cookie_domain, $is_https;
....
+  if (($is_https ? 'SSESS' : 'SESS') . substr(hash('sha256', $cookie_domain), 0, 32) == session_name()) {

Are you sure $is_https is the right thing to check here? Looking at the code in drupal_settings_initialize() it seems ini_get('session.cookie_secure') would be better.

Otherwise, looks great to me. And I think it addresses all of Damien's concerns.

Good catch, we do an ini_set() if $is_https=TRUE in drupal_settings_initialize(), but otherwise fall back on the ini_get().

Now the patch is pretty (tm)

Issue summary: this patch allows the Drupal 6 session cookie (that has a name in the form 'SESS' . md5($session_name)) to work during upgrade.

The code looks sounds and this is an important usability improvement for the upgrade process.

Just a correction, it is not only a usability improvement, but also fixes several PHP warnings during the upgrade that make the upgrade appear to have failed:
#986846: PHP warning during upgrade causes apparent premature abortion

@dalin: no. Those warnings are caused by a missing uid = 0.

This issue is just an usability improvement.

Nice work on this, folks.

Committed to HEAD.