Merge lp:~nurlan0000/psiphon/sprint3-610547 into lp:psiphon

[Nurlan Turdaliev]

Fixes commited.

[Rod (rod-psiphon)]

This was a team review. Here are our comments:

- Lack of comments makes review harder (and future maintenance).

- Our pattern is to add a permission (see config.php) and check permissions instead of checking user class when accessing functionality. In this branch, this should be done in profile.php and in delete_account.php.

- Please update the copyright notice year to the current year when a file is edited.

- Include 403 exits automatically. The exit statement is redundant.

- The prompt looks odd: "Delete account Delete account [X]". One "Delete account" as a link is sufficient.

- Why escape (using htmlentities) a msg value? This isn't user input, so escape is unnecessary and we don't do that elsewhere.

- DELETE FROM USER after the other tables to to minimize consistency problems. See the related comment in https://code.launchpad.net/~vasilev/psiphon/sprint3-457431/+merge/34185.

- Both this branch and "https://code.launchpad.net/~vasilev/psiphon/sprint3-457431/+merge/34185" delete from a bunch of related tables when deleting a user. Could refactor both to use a central config list of dependencies. Otherwise, every time someone adds a table that references user ids, they will have to know to go to these two places and add extra DELETEs.

- The new file user.php seems a bit random. Suggest either refactor all user helper functions to this file or add user table helpers to the existing database helper.

- Do not need header_link_prefix in "href="<?php echo $header_link_prefix; ?>delete_account.php">" in profile.php. Also "header('Location: '.$header_link_prefix.'profile.php');" in delete_account.php. $header_link_prefix is for other code that's added in separately and doesn't apply to anything on Launchpad.

- This is a bug: after a delete, the logout redirect isn't correct. The user ends up at e.g., "<proxy>/logout.php?lu=/001". They should end up at "<proxy>/001". As a result of this, the user can't immediately re-login to a different account; instead another redirect occurs.

review: Needs Fixing

[e.fryntov (e-fryntov)]

Regarding the "...<proxy>/logout.php?lu=/001". They should end up at "<proxy>/001"...." problem:

The last GET logout.php returns HTTP 200 instead of 302.

I suggest to add the 30x HTTP code implicitly to all redirects.

The following code is taken from php.net user comments on header function:

<?php
// 301 Moved Permanently
header("Location: /foo.php",TRUE,301);

// 302 Found
header("Location: /foo.php",TRUE,302);
header("Location: /foo.php");

// 303 See Other
header("Location: /foo.php",TRUE,303);

// 307 Temporary Redirect
header("Location: /foo.php",TRUE,307);
?>

[lp:~nurlan0000/psiphon/sprint3-610547]

122. By Nurlan <ubuntu@ubuntu-desktop> on 2010-09-14

Implemented Rod's and Eugene's recomendations

[Nurlan Turdaliev (nurlan0000)]

review: Needs Resubmitting

[Adam Kruger (adam-kruger)]

Code review comments:

- Diff line 112: header('Location: /001', true, 302);
"/001" should not be hard-coded. This should be retrieved from the database.
You might be able to use something like: $proxy_record = lookup_proxy($config, $record_user['current_proxy']);

Or see logout.php:

$query = "SELECT * FROM proxy WHERE hostname = :hostname";
$result = db_query_execute($config, $query, convert_null_array(array(":hostname" => $_SERVER[HTTP_HOST])));
$proxy_record = db_fetch_result($config, $result);

if (!$proxy_record || $proxy_record['login_url'] != $_GET[lu])
{
    include($_SERVER[DOCUMENT_ROOT]."/http-errors/404.php");
}

$login_url = $proxy_record['login_url'];

Your original code was using this style, but our objection was to the redirect part:
header('Location: logout.php?lu='.$login_url);
We want something like:
header('Location: logout.php?lu='.$login_url, true, 302);
exit;

review: Needs Fixing (group review)

[lp:~nurlan0000/psiphon/sprint3-610547]

123. By Nurlan Turdaliev <ubuntu@ubuntu-desktop> on 2010-10-21

Hardcoded redirect removed

[Nurlan Turdaliev (nurlan0000)]

review: Needs Resubmitting

=== modified file 'trunk/www/config.php'
--- trunk/www/config.php	2010-06-28 20:44:20 +0000
+++ trunk/www/config.php	2010-10-25 09:04:03 +0000
@@ -1,7 +1,7 @@
 <?
 /*
     Psiphon Circumvention Platform
-    Copyright (C) 2009 Psiphon Inc.
+    Copyright (C) 2010 Psiphon Inc.
 
     This program is free software: you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
@@ -99,6 +99,7 @@
         'view_propagator_info' => 1<<19,
         'multi_proxy'       => 1<20,
         'custom_user_agents' => 1<<21,
+        'delete_account'    => 1 << 22, 
     );
 
 // This defines the order in which the user groups/classes should be displayed. Generally, least-to-highest privilege.
@@ -140,7 +141,8 @@
        | $config['perm']['send_ticket']
        | $config['perm']['bookmarks']
        | $config['perm']['logout']
-       | $config['perm']['can_grant_grp_3'],
+       | $config['perm']['can_grant_grp_3']
+       | $config['perm']['delete_account'],
 
     // User
     // NOTE: '3' is the grp default in the invitation table. So if the 3 changes here, it'll have to change there.
@@ -150,7 +152,8 @@
        | $config['perm']['send_ticket']
        | $config['perm']['bookmarks']
        | $config['perm']['logout']
-       | $config['perm']['can_grant_grp_3'],
+       | $config['perm']['can_grant_grp_3']
+       | $config['perm']['delete_account'],
 
     // Anonymous User
 
@@ -166,14 +169,16 @@
        | $config['perm']['can_grant_grp_3']
        | $config['perm']['invite']
        | $config['perm']['view_propagator_info']
-       | $config['perm']['multi_proxy'],
+       | $config['perm']['multi_proxy']
+       | $config['perm']['delete_account'],
 
     // Reporter
 
     7 => $config['perm']['profile']
        | $config['perm']['logout']
        | $config['perm']['reports']
-       | $config['perm']['other_nodes'],
+       | $config['perm']['other_nodes']
+       | $config['perm']['delete_account'],
 );
 
 /*

=== added file 'trunk/www/delete_account.php'
--- trunk/www/delete_account.php	1970-01-01 00:00:00 +0000
+++ trunk/www/delete_account.php	2010-10-25 09:04:03 +0000
@@ -0,0 +1,77 @@
+<?php
+/*
+    Psiphon Circumvention Platform
+    Copyright (C) 2010 Psiphon Inc.
+
+    This program is free software: you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+
+
+include_once($_SERVER[DOCUMENT_ROOT]."/includes/common_includes.php");
+include($_SERVER[DOCUMENT_ROOT]."/check_user.php");
+include($_SERVER[DOCUMENT_ROOT]."/includes/lang_set.php");
+
+// check whether user can delete his own account
+$user_can_delete =  user_has_permission($config, $record_user, 'delete_account');
+
+if ( ! $user_can_delete )
+{
+    include($_SERVER[DOCUMENT_ROOT]."/http-errors/403.php");
+}
+
+$user_deleted = false;
+
+
+if ($_POST['submit_btn'])
+{
+    if ( ! $_POST['delete'] )
+    {
+        header('Location: /profile.php', true, 302);
+        die;
+    }
+
+    // Delete user and related data
+    $res = delete_user_and_related_data($config, ' FROM user WHERE id = :userid', array(':userid' => $record_user['id']));
+
+    // Redirect back to proxy
+    $query = "SELECT * FROM proxy WHERE hostname = :hostname";
+    $result = db_query_execute($config, $query, convert_null_array(array(":hostname" => $_SERVER[HTTP_HOST])));
+    $proxy_record = db_fetch_result($config, $result);
+
+    header("Location: ".$config["ret"].$proxy_record['login_url'], true, 302);
+    exit;
+}
+
+
+include($_SERVER[DOCUMENT_ROOT]."/header.php");
+
+?>
+    <h3><?php echo msg('delete_account'); ?></h3>
+
+    <div>
+        <?php echo msg('confirm_delete'); ?>
+    </div>
+    <form action="<?=getenv("SCRIPT_NAME")?>" method="post">
+        <label for="delete"><?php echo msg('yes'); ?></label>
+        <input type="radio" id="delete" name="delete" value="1" />
+
+        <label for="dont_delete"><?php echo msg('no'); ?></label>
+        <input type="radio" id="dont_delete" name="delete" value="0" checked="checked" /><br />
+
+        <input name="submit_btn" type="submit" value="<?php echo msg('submit'); ?>" />
+        <input name="csrf_token" value="<?=$record_user['csrf_token']?>" type="hidden" />
+    </form>
+
+<? include($_SERVER[DOCUMENT_ROOT]."/footer.php"); ?>

=== modified file 'trunk/www/includes/database_helpers.php'
--- trunk/www/includes/database_helpers.php	2010-06-23 19:21:47 +0000
+++ trunk/www/includes/database_helpers.php	2010-10-25 09:04:03 +0000
@@ -1,7 +1,7 @@
 <?php
 /*
     Psiphon Circumvention Platform
-    Copyright (C) 2009 Psiphon Inc.
+    Copyright (C) 2010 Psiphon Inc.
 
     This program is free software: you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
@@ -173,6 +173,41 @@
     return $user_record;
 }
 
+/**
+ * Deletes users and all related data
+ *
+ * @param  $config
+ * @param  $query_inactive_user_ids_from_part           SQL conditions to  select affected user ids with placeholders 
+ * @param  $param_array                                             placeholder values
+ * @return bool                                                             true on success, false on error
+ */
+function delete_user_and_related_data($config, $query_inactive_user_ids_from_part, Array $param_array) {
+    //   We don't know how big the user id list could be; So, we will use subselect here (Rod's recommendation)
+    $query_affected_user_ids = 'SELECT user.id ' . $query_inactive_user_ids_from_part;
+
+    $query_delete_bookmark      = "DELETE FROM bookmark WHERE user IN (" . $query_affected_user_ids . ")";
+    $query_delete_cookie        = "DELETE FROM cookie WHERE user IN (" . $query_affected_user_ids . ")";
+    $query_delete_invitation    = "DELETE FROM invitation WHERE user IN (" . $query_affected_user_ids . ")";
+    $query_delete_ticket        = "DELETE FROM ticket WHERE user IN (" . $query_affected_user_ids . ")";
+    $query_delete_userproxy     = "DELETE FROM userproxy WHERE userid IN (" . $query_affected_user_ids . ")";
+    // Delete from user table only after deleting all other data
+    $query_delete_user          = "DELETE " . $query_inactive_user_ids_from_part;
+
+    if (
+        !db_query_execute($config, $query_delete_bookmark, convert_null_array($param_array), true) ||
+        !db_query_execute($config, $query_delete_cookie, convert_null_array($param_array), true) ||
+        !db_query_execute($config, $query_delete_invitation, convert_null_array($param_array), true) ||
+        !db_query_execute($config, $query_delete_ticket, convert_null_array($param_array), true) ||
+        !db_query_execute($config, $query_delete_userproxy, convert_null_array($param_array), true) ||
+        !db_query_execute($config, $query_delete_user, convert_null_array($param_array), true)
+    )
+    {
+        return false;
+    }
+
+    return true;
+}
+
 /* Turns a (comma-separated, usually) list of values into a values that can be used in a
  * parameterized SQL query.
  *

=== modified file 'trunk/www/includes/lang.php'
--- trunk/www/includes/lang.php	2010-10-14 16:26:29 +0000
+++ trunk/www/includes/lang.php	2010-10-25 09:04:03 +0000
@@ -371,6 +371,32 @@
         'be' => "Регистрация",
     ),
 
+    'delete_account' => array(
+        'en' => "Delete account",
+        'fa' => "Delete account",
+        'ru' => "Delete account",
+        'uz' => "Delete account",
+        'tk' => "Delete account",
+        'cn' => "Delete account",
+        'ar' => "Delete account",
+        'vt' => "Delete account",
+        'fr' => "Delete account",
+        'es' => "Delete account",
+    ),
+
+    'confirm_delete' => array(
+        'en' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'fa' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'ru' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'uz' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'tk' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'cn' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'ar' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'vt' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'fr' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+        'es' => 'Are you sure that you want to completely delete your account? This action cannot be undone.',
+    ),
+    
     'invalid_uname_format' => array(
         'en' => "Error: Invalid username format",
         'fa' => "اشتباه: اسم کاربر درست نیست",

=== modified file 'trunk/www/profile.php'
--- trunk/www/profile.php	2010-07-16 17:53:05 +0000
+++ trunk/www/profile.php	2010-10-25 09:04:03 +0000
@@ -1,7 +1,7 @@
 <?
 /*
     Psiphon Circumvention Platform
-    Copyright (C) 2009 Psiphon Inc.
+    Copyright (C) 2010 Psiphon Inc.
 
     This program is free software: you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
@@ -245,6 +245,10 @@
     break;
 }
 
+// check whether user can delete his own account
+$user_can_delete = user_has_permission($config, $record_user, 'delete_account');
+
+
 include($_SERVER[DOCUMENT_ROOT]."/header.php");
 
 ?>
@@ -313,5 +317,10 @@
 </table>
 
 </form>
+<?php if ($user_can_delete) : ?>
+<br />
+        
+&nbsp;&nbsp;<a id="delete_href" href="/delete_account.php"><?php echo msg('delete_account');?> [X]</a>
 
+<?php endif; ?>
 <? include($_SERVER[DOCUMENT_ROOT]."/footer.php"); ?>