548435 - livechat_id is not set by tiki-register_livechat.php

Status: VERIFIED FIXED

(support.mozilla.org Graveyard :: Chat, defect)

Description

[Vishal Kamdar [:vish_moz]]

"Invalid Live Chat credentials" when manually migrating existing Live Chat account

Pre-condition: You should have a  Live Chat account (open fire account). You should not have a SUMO account with the same name.

STR:
   1.  Log into Openfire (Spark) normally with Openfire credentials (server: chat-support-stage.mozilla.com)
   2.  the server should ask you to migrate your account after you sign in.
   3. Visit https://support-stage.mozilla.org/tiki-livechat_migration.php as instructed, and follow the steps to create a new TikiWiki account and password. (Note: use new username & password than your existing openfire account) This password should be different than your Openfire password to verify migration
   4. Log out of Openfire, then log back in using your new TikiWiki username and password. 

Actual result:
Username/password invalid when logging into Openfire (Spark).
You are able to log into SUMO with this new account.

Expected result:
You should be able to log-in with your new migrated account into Openfire.

Comment 1

[Matthew Middleton (:zzxc)]

I tested this with some of my accounts, and the result was that livechat_id was always null after using tiki-register_livechat.php.  I migrated zzxc2 (to zzxc2) and zzxc4 (to zzxcfour), and livechat_id is null for both accounts.

Comment 2

[Paul Craciunoiu [:paulc]]

livechat_id is not set by tiki-register-livechat.php because it's set when returning to tiki-livechat_migration.php, since the latter also does the cleanup (from the Openfire database).
Does the migration process not return to this page?

I can change the register script to create the livechat_id when the user account is created. Would it be better to delete the openfire account there, too? James, Laura: thoughts?

Comment 3

[Matthew Middleton (:zzxc)]

The user is being deleted from ofUser as expected, but livechat_id in the newly registered account is not being set.

Comment 4

[Paul Craciunoiu [:paulc]]

Interesting. Will investigate tomorrow.

Comment 5

[Paul Craciunoiu [:paulc]]

Attachment: v1, makes tiki-register_livechat use behavior from tiki-register

Indeed, the livechat_id is not set when migrating a livechat account only (i.e. the 2nd option). Sorry everyone.

This patch copies over the relevant code from tiki-register.php (literally), since it's already been verified to work.

Comment 6

[Matthew Middleton (:zzxc)]

(In reply to comment #5)
> Created an attachment (id=429145) [details]
> v1, makes tiki-register_livechat use behavior from tiki-register
> 
> Indeed, the livechat_id is not set when migrating a livechat account only (i.e.
> the 2nd option). Sorry everyone.
> 
> This patch copies over the relevant code from tiki-register.php (literally),
> since it's already been verified to work.

This patch seems to generate a new livechat_id based on the newly registered username.  It should be using the existing Openfire username (the of_user parameter) as the livechat_id.

Comment 7

[Paul Craciunoiu [:paulc]]

(In reply to comment #6)
> This patch seems to generate a new livechat_id based on the newly registered
> username.  It should be using the existing Openfire username (the of_user
> parameter) as the livechat_id.
You're right.

I got together with James and set a better workflow for this, depending on availability of the old ofUser->username to Tiki:

* if the livechat_id and SUMO login are both available, use these and do not allow the user to change them in the migration process. Mark the form to show that the username is available.

* otherwise, show a message acknowledging that the login is taken, and allow the user to enter a new SUMO login. This will go through validation with the patch above.


There's one case that is probably infrequent, but worth noting: if the SUMO login is not available, but the livechat_id is, we still don't allow registration. Matthew: can you justify why it would be worth doing otherwise?

Comment 8

[Matthew Middleton (:zzxc)]

When a user with an existing Openfire account registers - which is always the case in tiki-register_livechat.php - this Openfire account name must become the livechat_id.

In the case where the livechat_id is available as a SUMO username, it should automatically be used.  In this case, the SUMO 'login' and 'livechat_id' will be identical.

In the case where the livechat_id is not available as a SUMO username, the user needs to be prompted to create one.  In this case, 'login' is the SUMO login name, and 'livechat_id' is the old Openfire account username.

The key is that the user's Openfire username is retained as livechat_id in whichever SUMO account is used for migration.

Comment 9

[James Socol [:jsocol, :james]]

(In reply to comment #8)
> When a user with an existing Openfire account registers - which is always the
> case in tiki-register_livechat.php - this Openfire account name must become the
> livechat_id.

What's the justification for "must" here? Maybe I've forgotten something.

> In the case where the livechat_id is not available as a SUMO username, the user
> needs to be prompted to create one.  In this case, 'login' is the SUMO login
> name, and 'livechat_id' is the old Openfire account username.

What about the case where the livechat_id is not available as a livechat_id? Are collisions possible post migration/normal registration, or would it be impossible to have a livechat_id in the SUMO database match an unmigrated account in the OF database?

Comment 10

[Matthew Middleton (:zzxc)]

(In reply to comment #9) 
> What's the justification for "must" here? Maybe I've forgotten something.

Openfire settings are stored in the database based on Jabber ID.  If an existing user's Jabber ID (which is generated using livechat_id) changes after migration, then all the user's live chat settings are lost.

> What about the case where the livechat_id is not available as a livechat_id?
> Are collisions possible post migration/normal registration, or would it be
> impossible to have a livechat_id in the SUMO database match an unmigrated
> account in the OF database?

Collisions with other Openfire accounts are not possible, since the migration script requires an existing Openfire account matching this livechat_id to exist.  This Openfire account is deleted during the migration.

Collisions among livechat_id's, however, are possible.  Thus it would be a good idea to clear any other instances of this livechat_id before continuing the migration.  These collisions are possible because tiki-register.php does not check for collisions in Openfire before setting the livechat_id.  (Openfire handles these collisions intelligently for unmigrated accounts by only allowing the older account to log in.)

To handle the latter case, SQL like this should be executed right before setting the user's lviechat_id:
UPDATE users_users SET livechat_id=NULL WHERE livechat_id=?;

Comment 11

[Paul Craciunoiu [:paulc]]

(In reply to comment #10)
> (In reply to comment #9) 
> To handle the latter case, SQL like this should be executed right before
> setting the user's lviechat_id:
> UPDATE users_users SET livechat_id=NULL WHERE livechat_id=?;
So, what you're saying is, remove the conflicting livechat_id from the Tiki user if the other user is coming from Openfire, right?

Comment 12

[James Socol [:jsocol, :james]]

What happens to the Tiki user if their livechat_id is removed after we've run the automatic migration?

Comment 13

[Paul Craciunoiu [:paulc]]

(In reply to comment #12)
> What happens to the Tiki user if their livechat_id is removed after we've run
> the automatic migration?
I checked just now. The migration script actually does not create livechat_ids for Tiki users with identical usernames in the Openfire database. So this shouldn't happen.

Comment 14

[Matthew Middleton (:zzxc)]

(In reply to comment #11)
> So, what you're saying is, remove the conflicting livechat_id from the Tiki
> user if the other user is coming from Openfire, right?

Exactly.  An existing Openfire user is always be older than a SUMO account with
a conflicting livechat_id.  No new Openfire accounts can be created after
migration starts.

Comment 15

[Paul Craciunoiu [:paulc]]

Attachment: v2, keeps livechat_id from Openfire at all costs

Comment 16

[Matthew Middleton (:zzxc)]

Comment on attachment 429234 [details] [diff] [review]
v2, keeps livechat_id from Openfire at all costs

This part of the migration is now destructive, so the hashcode needs to be checked.  Since it isn't verified before clearing duplicates or registering the user, anyone could take over any live chat account by forging a link.

Comment 17

[Paul Craciunoiu [:paulc]]

Attachment: v3, verifies Openfire information before setting livechat_id

This moves livechat_id creation to the migration script rather than on registration, and verifies livechat_id before setting it in the Tiki table for the newly created Tiki login.

Comment 18

[James Socol [:jsocol, :james]]

Comment on attachment 429556 [details] [diff] [review]
v3, verifies Openfire information before setting livechat_id


>Index: webroot/tiki-livechat_migration.php
>===================================================================
>--- webroot/tiki-livechat_migration.php	(revision 63482)
>+++ webroot/tiki-livechat_migration.php	(working copy)
>@@ -221,6 +221,29 @@
>         if (sha1($of_user . $encryptedPassword_2) != $_GET['hashcode']) {
>             throw new Exception(MESSAGE_LIVECHAT_DELETE);
>         }
>+
>+        /* bug 548435 - ensure livechat_id is unique */
>+        $query = '
>+            UPDATE
>+                users_users
>+            SET
>+                livechat_id = NULL
>+            WHERE
>+                livechat_id = ?
>+            LIMIT 1';
>+        get_result($db_tiki, $query, array($of_user));

I'm still confused about why this is OK. What happens to any users this matches? And if it can't match, why is this here?

Comment 19

[Paul Craciunoiu [:paulc]]

(In reply to comment #18)
> I'm still confused about why this is OK. What happens to any users this
> matches? And if it can't match, why is this here?
It can match if, by some coincidence, someone registers a Tiki account with the same livechat_id, before the Openfire user migrates.

Comment 20

[James Socol [:jsocol, :james]]

(In reply to comment #19)
> It can match if, by some coincidence, someone registers a Tiki account with the
> same livechat_id, before the Openfire user migrates.

And what happens to that user?

Comment 21

[Paul Craciunoiu [:paulc]]

(In reply to comment #20)
> And what happens to that user?
They would need to contact an admin to have their livechat_id set to something else. Alternatively we can change this script's behavior to append a number to the ID instead of setting it to null.

I imagine this situation is a rare case.

Comment 22

[James Socol [:jsocol, :james]]

(In reply to comment #21)
> They would need to contact an admin to have their livechat_id set to something
> else. Alternatively we can change this script's behavior to append a number to
> the ID instead of setting it to null.

With no messaging or notification, how would they know that?

Maybe, for now, we need to check that livechat_ids created at registration don't collide with anything in the openfire database?

> I imagine this situation is a rare case.

I could easily see someone with a livechat account getting confused, registering the same account name on SUMO, then taking the wrong option instead of merging the two.

Either way, I don't like leaving this edge case.

Comment 23

[Paul Craciunoiu [:paulc]]

(In reply to comment #22)
> Maybe, for now, we need to check that livechat_ids created at registration
> don't collide with anything in the openfire database?
Yes we could do that and avoid this edge case entirely.

Comment 24

[James Socol [:jsocol, :james]]

Sorry to make more work for you, but I'd rather do that than just blank out someone's livechat_id.

Comment 25

[Matthew Middleton (:zzxc)]

Checking the Openfire database when registering a new user would prevent this edge case entirely.  As it is, an affected user (one with the *newer* conflicting account) is simply unmigrated.  The affected user can fix this by migrating again - which we can document in the contributor documentation.

Note that affected users will have never been able to log into Live Chat, as Openfire always denies access to the newer account, if there is a conflict.  So, setting this to null isn't changing the user's experience ('access denied' message) at all.

Obviously, it would be best to avoid account conflicts altogether in tiki-register.php so that this never happens, but this is a rare case that will only be possible while migration is underway.

Comment 26

[Paul Craciunoiu [:paulc]]

Attachment: v4, adds check for livechat_id in tiki-register.php -- per comment 23

Comment 27

[James Socol [:jsocol, :james]]

Paul, is this truly v4, or do I need v3, as well? I wasn't sure if this completely supersedes the previous solution or adds on to it.

Comment 28

[Paul Craciunoiu [:paulc]]

(In reply to comment #27)
> Paul, is this truly v4, or do I need v3, as well? I wasn't sure if this
> completely supersedes the previous solution or adds on to it.
It should supersede.

Comment 29

[James Socol [:jsocol, :james]]

Comment on attachment 430195 [details] [diff] [review]
v4, adds check for livechat_id in tiki-register.php -- per comment 23

r+ on this, just a general note.

>Index: webroot/tiki-register.php
>===================================================================
>--- webroot/tiki-register.php	(revision 63683)
>+++ webroot/tiki-register.php	(working copy)
>@@ -180,12 +180,40 @@
>     /* create livechat_id -- no punctuation or spaces */
>     $livechat_id = mb_ereg_replace('[ `!@#\$%\^&\*\(\)<>\?:\"\{\}\|~\[\];\',\.\/\-=_\+\\\]+', '_', $_REQUEST['name']);
>     /* check livechat_id is unique (user OR livechat_id) */
>-    $livechat_id_exists = $tikilib->getOne("SELECT login
>-        FROM users_users
>-        WHERE
>-            login = ?
>-         OR livechat_id = ?", array($livechat_id, $livechat_id));
>-    if (isset($livechat_id_exists)) {
>+    try {
>+        // check the tiki database
>+        $livechat_id_exists = $tikilib->getOne("SELECT login
>+            FROM users_users
>+            WHERE
>+                login = ?
>+            OR livechat_id = ?", array($livechat_id, $livechat_id));
>+
>+        if (!isset($livechat_id_exists)) {

This section could be less branchy if you reversed this and threw an exception, eg:

    if(isset($livechat_id_exists)) {
        throw new Exception()
    }

Then the rest of it doesn't need to be nested. Nesting the conditions like this turns the catch block into a glorified else, rather than taking advantage of it.

>+            // check the openfire database
>+            require('db/local.php');
>+            $db_of = new PDO("mysql:host=$host_of;dbname=$dbs_of", $user_of, $pass_of);
>+            $query = "
>+                    SELECT
>+                        username, email, encryptedPassword
>+                    FROM
>+                        ofUser
>+                    WHERE
>+                        username = ?
>+                    LIMIT 1
>+            ";
>+
>+            $statement = $db_of->prepare($query);
>+            $statement->execute(array($livechat_id));
>+
>+            $rows = $statement->fetchAll();
>+            if ((is_array($rows) && $rows[0]['username'])) {
>+                throw new Exception();
>+            }
>+
>+        } else {
>+            throw new Exception();
>+        }
>+    } catch(Exception $e) {
>         $smarty->assign('msg',tra("User already exists"));
>         $smarty->display("error.tpl");
>         die;

The nice thing about doing errors in a try block is linearizing the conditions (instead of nesting), so:

    try {
        if not condition 1
            throw exception
        if not condition 2
            throw exception
...etc...

        all conditions met here, do what you gotta do
    } catch exception {
        handle the error
    }

Comment 30

[Matthew Middleton (:zzxc)]

Comment on attachment 430195 [details] [diff] [review]
v4, adds check for livechat_id in tiki-register.php -- per comment 23

All the tests at https://wiki.mozilla.org/Support/Live_Chat/SUMO_login/Testing are now working with the latest patch applied.

Comment 31

[Paul Craciunoiu [:paulc]]

r63764
Thanks to both James and Matthew!

Comment 32

[Vishal Kamdar [:vish_moz]]

Verified, FIXED.