Fixes #3204, removed LOCK TABLES privilege check from installation. Modified...

Fixes #3204, removed LOCK TABLES privilege check from installation. Modified LogDataPurger so log_actions are not purged if the lock tables privilege is not granted.


git-svn-id: http://dev.piwik.org/svn/trunk@6483 59fd770c-687e-43c8-a1e3-f5a4ff64c105

core/Piwik.php

@@ -2595,4 +2595,38 @@ class Piwik
 		$db = Zend_Registry::get('db');
 		return $db->fetchOne($sql, array($lockName)) == '1';
 	}
+	
+	/**
+	 * Cached result of isLockprivilegeGranted function.
+	 * 
+	 * Public so tests can simulate the situation where the lock tables privilege isn't granted.
+	 * 
+	 * @var bool
+	 */
+	static public $lockPrivilegeGranted = null;
+	
+	/**
+	 * Checks whether the database user is allowed to lock tables.
+	 * 
+	 * @return bool
+	 */
+	static public function isLockPrivilegeGranted()
+	{
+		if (is_null(self::$lockPrivilegeGranted))
+		{
+			try
+			{
+				Piwik_LockTables(Piwik_Common::prefixTable('log_visit'));
+				Piwik_UnlockAllTables();
+				
+				self::$lockPrivilegeGranted = true;
+			}
+			catch (Exception $ex)
+			{
+				self::$lockPrivilegeGranted = false;
+			}
+		}
+		
+		return self::$lockPrivilegeGranted;
+	}
 }

core/PluginsFunctions/Sql.php

@@ -181,7 +181,7 @@ class Piwik_Sql
 	 * @param string|array  $tablesToWrite  The table or tables to obtain 'write' locks on.
 	 * @return Zend_Db_Statement
 	 */
-	static public function lockTables( $tablesToRead, $tablesToWrite )
+	static public function lockTables( $tablesToRead, $tablesToWrite = array() )
 	{
 		if (!is_array($tablesToRead))
 		{
@@ -354,7 +354,7 @@ function Piwik_DropTables( $tables )
  * @param string|array  $tablesToWrite  The table or tables to obtain 'write' locks on.
  * @return Zend_Db_Statement
  */
-function Piwik_LockTables( $tablesToRead, $tablesToWrite )
+function Piwik_LockTables( $tablesToRead, $tablesToWrite = array() )
 {
 	return Piwik_Sql::lockTables($tablesToRead, $tablesToWrite);
 }

plugins/Installation/FormDatabaseSetup.php

@@ -148,7 +148,6 @@ class Piwik_Installation_FormDatabaseSetup extends Piwik_QuickForm2
  * - INSERT
  * - UPDATE
  * - DELETE
- * - LOCK TABLES
  * - DROP
  * - CREATE TEMPORARY TABLES
  * 
@@ -244,7 +243,8 @@ class Piwik_Installation_FormDatabaseSetup_Rule_checkUserPrivileges extends HTML
 	 * array maps privilege names with one or more SQL queries that can be used to test
 	 * if the current user has the privilege.
 	 * 
-	 * NOTE: LOAD DATA INFILE privilege is not **required** so its not checked.
+	 * NOTE: LOAD DATA INFILE & LOCK TABLES privileges are not **required** so they're
+	 * not checked.
 	 * 
 	 * @return array
 	 */
@@ -263,7 +263,6 @@ class Piwik_Installation_FormDatabaseSetup_Rule_checkUserPrivileges extends HTML
 			'INSERT' => 'INSERT INTO '.self::TEST_TABLE_NAME.' (value) VALUES (123)',
 			'UPDATE' => 'UPDATE '.self::TEST_TABLE_NAME.' SET value = 456 WHERE id = 1',
 			'DELETE' => 'DELETE FROM '.self::TEST_TABLE_NAME.' WHERE id = 1',
-			'LOCK TABLES' => array('LOCK TABLES '.self::TEST_TABLE_NAME.' WRITE', 'UNLOCK TABLES'),
 			'DROP' => 'DROP TABLE '.self::TEST_TABLE_NAME,
 			'CREATE TEMPORARY TABLES' => 'CREATE TEMPORARY TABLE '.self::TEST_TEMP_TABLE_NAME.' (
 											id INT AUTO_INCREMENT,

plugins/PrivacyManager/LogDataPurger.php

@@ -73,8 +73,16 @@ class Piwik_PrivacyManager_LogDataPurger
 			}
 		}
 		
-		// delete unused actions from the log_action table
-		$this->purgeUnusedLogActions();
+		// delete unused actions from the log_action table (but only if we can lock tables)
+		if (Piwik::isLockPrivilegeGranted())
+		{
+			$this->purgeUnusedLogActions();
+		}
+		else
+		{
+			$logMessage = get_class($this).": LOCK TABLES privilege not granted; skipping unused actions purge";
+			Piwik::log($logMessage);
+		}
 		
 		// optimize table overhead after deletion
 		Piwik_OptimizeTables($logTables);

plugins/PrivacyManager/tests/PrivacyManager.test.php

@@ -41,6 +41,7 @@ class Test_Piwik_PrivacyManager extends Test_Integration
 		parent::setUp();
 		
 		Piwik_TablePartitioning::$tablesAlreadyInstalled = null;
+		Piwik::$lockPrivilegeGranted = null;
 		
 		// purging depends upon today's date, so 'older_than' parts must be dependent upon today
 		$today = Piwik_Date::factory('today');
@@ -78,6 +79,8 @@ class Test_Piwik_PrivacyManager extends Test_Integration
 	{
 		parent::tearDown();
 		
+		Piwik::$lockPrivilegeGranted = null;
+		
 		// remove archive tables (integration test teardown will only truncate)
 		$archiveTables = $this->getArchiveTableNames();
 		$archiveTables = array_merge($archiveTables['numeric'], $archiveTables['blob']);
@@ -528,6 +531,23 @@ class Test_Piwik_PrivacyManager extends Test_Integration
 		$this->checkReportsAndMetricsPurged($janBlobsRemaining = 6); // 1 segmented blob + 5 day blobs
 	}
 	
+	/** Tests that log actions are not purged when the lock privilege is not granted.
+	 * (Simulates absence of lock privilege.)
+	 */
+	public function test_purgeData_deleteLogsWithoutLockPrivilege()
+	{
+		Piwik::$lockPrivilegeGranted = false;
+		
+		$this->addLogData();
+		
+		// purge data
+		$this->setTimeToRun();
+		$this->instance->deleteLogData();
+		
+		// perform checks
+		$this->checkLogDataPurged($actionsPurged = false);
+	}
+	
 	// --- utility functions follow ---
 	
 	private function addLogData()
@@ -717,7 +737,7 @@ class Test_Piwik_PrivacyManager extends Test_Integration
 		$this->assertEqual(self::FEB_METRIC_ARCHIVE_COUNT + 1, $this->getTableCount($archiveTables['blob'][1])); // February
 	}
 	
-	private function checkLogDataPurged()
+	private function checkLogDataPurged( $actionsPurged = true )
 	{
 		// 3 days removed by purge, so 3 visits, 6 conversions, 6 visit actions, 3 e-commerce orders
 		// & 6 actions removed
@@ -725,7 +745,14 @@ class Test_Piwik_PrivacyManager extends Test_Integration
 		$this->assertEqual(16, $this->getTableCount('log_conversion'));
 		$this->assertEqual(16, $this->getTableCount('log_link_visit_action'));
 		$this->assertEqual(8, $this->getTableCount('log_conversion_item'));
-		$this->assertEqual(21, $this->getTableCount('log_action'));
+		if ($actionsPurged)
+		{
+			$this->assertEqual(21, $this->getTableCount('log_action'));
+		}
+		else
+		{
+			$this->assertEqual(27, $this->getTableCount('log_action'));
+		}
 	}
 	
 	/**