HTTPS sessions use an invalid merge query

This patch also passes tests and does

• (sid = SESSION_ID) over HTTP,
• (ssid = SESSION_ID) over HTTPS if double session cookies are not in effect,
• (sid = INSECURE_SESSION_ID AND ssid = SESSION_ID) over HTTPS if double session cookies are in effect

double cookies mean that both a secure session cookie and an insecure session cookie is present and the variable https is on too.

Also it finally allows (sid,ssid) as the primary key. This makes me feel warm and fuzzy.

There can be fun spinoffs from this patch because a) there was never a test for NULL ssid. Surely we need to do that? b) why ->condition does not do the right thing for ->condition('foo', NULL)? Edit: #813540: Comparisons involving NULL must never return true

#6: 813492-session-remix.patch queued for re-testing.

subscribe

MySQL does not allow NULLs in any of the fields of the compound key (boo) but oh well, empty string works too -- it wont be a problem as only (sid,ssid) needs to be unique neither sid nor ssid is mandated to be unique so they do not really need to be NULL.

Damien's problem, I let him either explain better or fix it. I don't get it.

Minus debug

I found this in my IRC log and while I could not ask Damien permission to post I can't imagine what I would breach by doing this so "what needs to be tested is: put something in the session as HTTP, login as HTTPS, verify that the session has been carried over, get back to the original anonymous cookie, verify that this session is now inactive". I will write that test.

Keeping up w HEAD

Now, now.

Committed to CVS HEAD. I'm moving this to head to head.

Looks like the column descriptions in hook_schema() needs to reflect that (sid, ssid) is now a multi-column primary key. I think we should just omit the "Primary key"/"Unique key" prefix - it is redundant, and the convention is not used elsewhere in core.

      'sid' => array(
        'description' => "Primary key: A session ID. The value is generated by PHP's Session API.",
        'type' => 'varchar',
        'length' => 64,
        'not null' => TRUE,
        'default' => '',
      ),
      'ssid' => array(
        'description' => "Unique key: Secure session ID. The value is generated by PHP's Session API.",
        'type' => 'varchar',
        'length' => 64,
        'not null' => TRUE,
        'default' => '',
      ),

BTW, shouldn't we add an index on the ssid column?

oh dang yes we should!

There is nothing to review here I have merely executed the requests of c960657

We're going to need to add that index too on whatever update hook adds the ssid column, no?

There is actually no update function that creates ssids on existing sites. This version of the patch adds an update function to create ssid column as part of the upgrade process and adds an index.

After speaks to chx reworking to move the update to update.inc.

Should be "+ // Add ssid column and index.

Otherwise seems fine, a little disturbing we had this broken for so long.

Fix typo

Sounds a plan. I have removed a stray newline.

Neither patch fully fixes #28.

Remove white space and fix typo (hopefully)

Fix another docs issue

Cool, thanks for that!

Committed to HEAD.

I think this patch might have re-opened #575280: Impersonation when an https session exists.? Or is it just me seeing empty string sid and ssid values in my sessions table...

#2330279: When operating in mixed mode SSL: data from anonymous non-HTTP session is still available after login discovered a bug that seems to exist since this was committed, causing a different behavior than the expected. Your feedback over there would be appreciated.