Merge lp:~vasilev/psiphon/sprint3-457431 into lp:psiphon

[Robert Vasilev]

Fixes for https://bugs.launchpad.net/psiphon/+bug/457431

1. Added cleanup_unused_accounts_cron.php script for deleting inactive user accounts and related data (bookmarks, cookies, etc).
2. Added 3 configuration directives in config.php (inactivity periods for non-disabled and disabled users, and affected user classes list).
3. Updated crontab sample to invoke cleanup_unused_accounts_cron.php.

Upd:
Rod's comments have been implemented.

[Rod (rod-psiphon)]

1. Should not hard code user class numbers:

$config["unused_account_cleanup_affected_user_classes"] = array(2, 3, 4, 6, 7);

In includes/user_class_helpers.php, we have helper functions like get_power_user_class_id().

2. If the series of DELETE statements in delete_user_and_related_data fails in the middle (e.g., server crash) then we could be left with bookmark, invite, userproxy, etc. referring to non-existing user. This is probably bad.

Recommend doing "DELETE FROM user" last instead of first to minimize the risk of inconsistency in the database (we can't guarantee consistency with the MyISAM engine which lacks transactions)/

3. It appears that the SQL works like this:

php array = SELECT userid WHERE ...
DELETE WHERE id in (php array rendered as string) [repeat 6 times]

We recommend this pattern instead:

DELETE WHERE id in (SELECT userid WHERE ...) [repeat 6 times]

Reasoning: there is far less data sent between PHP and the database engine in the second approach; we don't know how big the user id list could be; MySQL ought to cache the results of the SELECT so redoing it in each DELETE should not incur full overhead.

review: Needs Fixing

[Rod (rod-psiphon)]

review: Approve

[Adam Kruger (adam-kruger)]

review: Approve

=== added file 'trunk/cronjobs/cleanup_unused_accounts_cron.php'
--- trunk/cronjobs/cleanup_unused_accounts_cron.php	1970-01-01 00:00:00 +0000
+++ trunk/cronjobs/cleanup_unused_accounts_cron.php	2010-09-14 12:50:56 +0000
@@ -0,0 +1,95 @@
+<?
+/*
+    Psiphon Circumvention Platform
+    Copyright (C) 2009 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/>.
+*/
+
+// Add to crontab (to have it run every 1 minute):
+//
+// * * * * * /usr/local/bin/php /home/ppcron/cronjobs/cleanup_unused_accounts_cron.php
+//
+
+// pwd of a cronjob seems to be the cron user's (ppcron) home directory.
+// This messes up includes, so we're going to explicitly use the directory
+// of this file.
+$current_directory = substr($argv[0], 0, strrpos($argv[0], "/"));
+
+//
+// Remove unused user accounts.
+// Any user account of configured classes with last_activity older than account_ttl is deleted.
+//
+$_SERVER["DOCUMENT_ROOT"] = $current_directory;
+include($current_directory."/includes/user_class_helpers.php");
+include($current_directory."/config.php");
+include($current_directory."/includes/sql_error_cli.php");
+include($current_directory."/includes/cr.php");
+include($current_directory."/includes/database_helpers.php");
+
+// Don't run if this job is already running.
+
+if ($f = @fopen($config["cleanup_unused_accounts_pid_file"], "r"))
+{
+    $pid = (int)fgets($f);
+    fclose($f);
+    exit("{$argv[0]} is already running, pid {$pid}\n");
+}
+
+if ($f = @fopen($config["cleanup_unused_accounts_pid_file"], "w"))
+{
+    $pid = getmypid();
+    fputs($f, $pid."\n");
+    fclose($f);
+}
+else
+{
+    echo "Can't create {$config["cleanup_unused_accounts_pid_file"]}, keep running anyway.\n";
+}
+
+
+// Construct SQL condition of user classes configured for account cleanup.
+$param_string = "";
+$param_array = array();
+string_list_to_params(implode(",", $config["unused_account_cleanup_affected_user_classes"]), ",", "p_", $param_string, $param_array);
+$query_inactive_user_ids_from_part = " FROM user WHERE
+user.last_activity < (unix_timestamp() - :unused_account_ttl)
+AND
+user.locked = :locked
+AND
+user.grp IN (".$param_string.")
+";
+
+// Cleanup unused non-disabled accounts
+$param_array[':locked'] = 0;
+$param_array[':unused_account_ttl'] = $config["unused_account_ttl"];
+delete_user_and_all_related_data($config, $query_inactive_user_ids_from_part, $param_array);
+
+// Cleanup unused disabled accounts
+$param_array[':locked'] = 1;
+$param_array[':unused_account_ttl'] = $config["unused_disabled_account_ttl"];
+delete_user_and_all_related_data($config, $query_inactive_user_ids_from_part, $param_array);
+
+@unlink($config["cleanup_unused_accounts_pid_file"]);
+
+exit(0);
+
+function delete_user_and_all_related_data($config, $query_inactive_user_ids_from_part, Array $param_array) {
+    // Call function from database_helpers.php
+    if (!delete_user_and_related_data($config, $query_inactive_user_ids_from_part, $param_array)) {
+        @unlink($config["cleanup_unused_accounts_pid_file"]);
+        sql_error_cli("DELETE " . $query_inactive_user_ids_from_part, $config["sql"]);
+    }
+}
+?>

=== modified file 'trunk/cronjobs/crontab'
--- trunk/cronjobs/crontab	2010-06-29 20:33:54 +0000
+++ trunk/cronjobs/crontab	2010-09-14 12:50:56 +0000
@@ -1,4 +1,4 @@
 3    2 * * * #dest#/php/bin/php #dest#/ppcron/cleanup_invitations_cron.php
 4    2 * * * #dest#/php/bin/php #dest#/ppcron/cleanup_email_candidates_cron.php
 6    5 * * * #dest#/php/bin/php #dest#/ppcron/cleanup_sessions_cron.php
-
+5    4 * * * #dest#/php/bin/php #dest#/ppcron/cleanup_unused_accounts_cron.php

=== modified file 'trunk/www/config.php'
--- trunk/www/config.php	2010-06-28 20:44:20 +0000
+++ trunk/www/config.php	2010-09-14 12:50:56 +0000
@@ -54,6 +54,17 @@
 
 $config["session_ttl"] = 60*60; // session expires after 1 idle hour
 
+// unused non-disabled user accounts are deleted after 2 years of inactivity
+$config["unused_account_ttl"] = 86400 * 365 * 2;
+
+// unused disabled user accounts are deleted after 5 months since disabled
+$config["unused_disabled_account_ttl"] = 86400 * 30 * 5;
+
+// user groups/classes affected by automatic unused account cleanup
+$config["unused_account_cleanup_affected_user_classes"] = array(get_power_user_class_id(), 
+    get_normal_user_class_id(), get_anonymous_user_class_id(),
+    get_propagator_user_class_id(), get_reporter_user_class_id() );
+
 $config[min_pass_length] = 6;
 $config[max_login_delay] = 30; // 30 seconds
 
@@ -66,6 +77,7 @@
 $config["cleanup_email_candidates_pid_file"] = "/var/run/ppcron/psiphon_cleanup_email_candidates.pid";
 $config["cleanup_invitations_pid_file"] = "/var/run/ppcron/psiphon_cleanup_invitations.pid";
 $config["cleanup_sessions_pid_file"] = "/var/run/ppcron/psiphon_cleanup_sessions.pid";
+$config["cleanup_unused_accounts_pid_file"] = "/var/run/ppcron/psiphon_cleanup_unused_accounts.pid";
 
 $config["psiphon_web_resource_page"] = "https://docs.google.com/View?id=dccxjzk4_0f76vf8g8";
 

=== 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-09-14 12:50:56 +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,40 @@
     return $user_record;
 }
 
+/**
+ * Deletes users and all related data
+ *
+ * @param  $config
+ * @param  $query_affected_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_affected_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_affected_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_affected_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.
  *