Merge lp:~mvo/click/lp1358294-config-removal into lp:click/devel

[Michael Vogt]

This branch implements removal of the data when a application is removed.

[lp:~mvo/click/lp1358294-config-removal]

504. By Michael Vogt on 2015-01-21

remove XDG_DATA_HOME as well (as requested in #1358294)

505. By Michael Vogt on 2015-01-21

merged lp:click/devel

[Michael Vogt (mvo)]

@Colin: do you think you could give this branch a review? If you are too busy I will try to find someone else but I would love to get a review before landing it.

[Colin Watson (cjwatson)]

This is a code review only; I think deleting user data in this way is fundamentally misguided, but since I'm stepping back from the click project in general I don't really want to get into that argument. The "Needs Fixing" applies only to making the tests more robust, as suggested in inline comments.

It is not at all obvious to me from the bug trail that Jamie's comment #15 has been resolved. Comment #13 only says that we "can" display a warning, but there's no bug task for that (unity-scope-click?) and no indication that this has in fact been done. I think it's mandatory to land a warning indication no later than the point where we land this change.

This branch also doesn't seem to do anything to address the "Users should not be allowed to delete the key apps that ship with the phone" point from the specification in the bug description. I would personally recommend and prefer that this sort of policy should be implemented in the scope rather than in click itself, but it's up to you.

review: Needs Fixing

[lp:~mvo/click/lp1358294-config-removal]

506. By Michael Vogt on 2015-03-11

merged lp:click

507. By Michael Vogt on 2015-03-11

address review comments (thanks Colin!) and push environment manipulation before the glib import

508. By Michael Vogt on 2015-03-11

update tests to address issues raised by Colin (thanks)

509. By Michael Vogt on 2015-03-11

rename rmtree() to rmtree_onefs() and use rm -rf --one-file-system (thanks Colin)

510. By Michael Vogt on 2015-03-11

refactor user data removal tests into their own TestCase

[Michael Vogt (mvo)]

Thanks Colin! I addressed your code review comments.

I agree with the concerns about deleting user data and will followup on that to ensure this only lands when everything else is in place.

[Alejandro J. Cura (alecu)]

I'd like to suggest adding an extra test to check that the user data is not deleted when a package is upgraded. A quick look thru click code seems to suggest it's not, but I think it's better if that's checked going forward.

review: Needs Fixing

[dobey (dobey)]

I'm inclined to reject this, because removal of user data when uninstalling an app is a generally very bad idea. There is no precedent set anywhere in the industry that suggests it is a good idea, while there is plenty of precedent showing it is a very bad idea.

If I uninstall the Camera or Gallery apps, should all the photos I ever took on my phone be deleted? Contacts? Calendar? No, of course not. And this patch won't do that, because it isn't a complete solution, as that data is stored elsewhere.

review: Needs Information

[dobey (dobey)]

Actually, yes, I will reject this. Automatic implicit deletion of anything other than the cache directory, is harmful to the user and we should never perform such actions. Deletion of any data beyond the cache, must be explicit.

review: Disapprove

Unmerged revisions

510. By Michael Vogt on 2015-03-11

refactor user data removal tests into their own TestCase

509. By Michael Vogt on 2015-03-11

rename rmtree() to rmtree_onefs() and use rm -rf --one-file-system (thanks Colin)

508. By Michael Vogt on 2015-03-11

update tests to address issues raised by Colin (thanks)

507. By Michael Vogt on 2015-03-11

address review comments (thanks Colin!) and push environment manipulation before the glib import

506. By Michael Vogt on 2015-03-11

merged lp:click

505. By Michael Vogt on 2015-01-21

merged lp:click/devel

504. By Michael Vogt on 2015-01-21

remove XDG_DATA_HOME as well (as requested in #1358294)

503. By Michael Vogt on 2014-08-22

merged from lp:click/devel

502. By Michael Vogt on 2014-08-22

only remove XDG_{CACHE,CONFIG}_DIR

501. By Michael Vogt on 2014-08-20

remove data on ap removal

=== modified file 'click/tests/test_user.py'
--- click/tests/test_user.py	2014-10-02 06:57:37 +0000
+++ click/tests/test_user.py	2015-03-11 08:21:00 +0000
@@ -26,10 +26,24 @@
 import json
 import os
 import shutil
+import tempfile
 from textwrap import dedent
 
+
+# Important:
+#
+# glib reads the environment only once and caches it then
+# so we need to create the
+#
+# ensure that the xdg dirs point to a different place as
+# registry.remove() will remove stuff here
+xdg_tempdir = tempfile.mkdtemp()
+for xdgdir in ("XDG_CONFIG_HOME", "XDG_CACHE_HOME", "XDG_DATA_HOME"):
+    os.environ[xdgdir] = os.path.join(xdg_tempdir, xdgdir)
+# ensure this is imported after the environment manipulation
 from gi.repository import Click, GLib
 
+
 from click.json_helpers import json_array_to_python, json_object_to_python
 from click.tests.gimock_types import Passwd
 from click.tests.helpers import (
@@ -42,6 +56,7 @@
 
 
 class TestClickUser(TestCase):
+
     def setUp(self):
         super(TestClickUser, self).setUp()
         self.use_temp_dir()
@@ -589,3 +604,92 @@
         # ensure that stop was called with the right app
         with open(self.fake_app_stop_output) as f:
             self.assertEqual("meep_a-app_2.0", f.read().strip())
+
+
+class UserDataRemovalTestCase(TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        cls.class_temp_dir = xdg_tempdir
+
+    def tearDown(self):
+        super(UserDataRemovalTestCase, self).tearDown()
+        for entry in os.listdir(self.class_temp_dir):
+            shutil.rmtree(os.path.join(self.class_temp_dir, entry))
+
+    def setUp(self):
+        super(UserDataRemovalTestCase, self).setUp()
+        self.use_temp_dir()
+        self.db = Click.DB()
+        self.db.add(self.temp_dir)
+        # add app
+        self.appname = "some-app"
+        self.registry = Click.User.for_user(self.db, "user")
+        os.makedirs(self.registry.get_overlay_db())
+        path = os.path.join(self.registry.get_overlay_db(), self.appname)
+        os.symlink("/1.0", path)
+
+    def test_remove_removes_config(self):
+        """
+        Ensure that all user data is removed if a click is removed
+        (LP: #1358294)
+        """
+        fake_config_dir = os.path.join(
+            os.environ["XDG_CONFIG_HOME"], self.appname)
+        fake_cache_dir = os.path.join(
+            os.environ["XDG_CACHE_HOME"], self.appname)
+        fake_data_dir = os.path.join(
+            os.environ["XDG_DATA_HOME"], self.appname)
+        os.makedirs(fake_config_dir)
+        os.makedirs(fake_cache_dir)
+        os.makedirs(fake_data_dir)
+
+        self.registry.remove(self.appname)
+
+        self.assertFalse(os.path.exists(fake_config_dir))
+        self.assertFalse(os.path.exists(fake_cache_dir))
+        self.assertFalse(os.path.exists(fake_data_dir))
+
+    def test_remove_does_not_follow_symlinks_inside_dir(self):
+        """
+        Ensure that symlinks that are inside a user-data dir get removed,
+        but that the symlink targets are not touched
+        """
+        fake_config_dir = os.path.join(
+            os.environ["XDG_CONFIG_HOME"], self.appname)
+        os.makedirs(fake_config_dir)
+        # setup symlink inside the config dir that that points
+        # to a file that is outside of the config dir
+        canary = os.path.join(self.temp_dir, "canary")
+        touch(canary)
+        os.symlink(canary, os.path.join(fake_config_dir, "canary"))
+        self.assertTrue(os.path.islink(os.path.join(fake_config_dir, "canary")))
+
+        self.registry.remove(self.appname)
+
+        # ensure that the config dir is gone
+        self.assertFalse(os.path.exists(fake_config_dir))
+        # ... but the canary file is still there
+        self.assertTrue(os.path.exists(canary))
+
+    def test_remove_does_not_follow_if_xdg_dir_is_symlink(self):
+        """
+        Ensure that if the user-data dir is a symlink that symlink is
+        removed  but that the symlink target is not touched
+        """
+        fake_config_dir = os.path.join(
+            os.environ["XDG_CONFIG_HOME"], self.appname)
+        os.makedirs(os.environ["XDG_CONFIG_HOME"])
+
+        # setup the config dir as a symlink to a other dir
+        canary = os.path.join(self.temp_dir, "canary")
+        os.mkdir(canary)
+        os.symlink(canary, fake_config_dir)
+        self.assertTrue(os.path.islink(fake_config_dir))
+
+        self.registry.remove(self.appname)
+
+        # ensure that the config dir is gone
+        self.assertFalse(os.path.exists(fake_config_dir))
+        # ... but the canary dir is still there
+        self.assertTrue(os.path.exists(canary))

=== modified file 'lib/click/database.vala'
--- lib/click/database.vala	2014-09-29 11:40:56 +0000
+++ lib/click/database.vala	2015-03-11 08:21:00 +0000
@@ -341,18 +341,8 @@
 		if (show_messages ())
 			message ("Removing %s", version_path);
 		package_remove_hooks (master_db, package, version);
-		/* In Python, we used shutil.rmtree(version_path,
-		 * ignore_errors=True), but GLib doesn't have an obvious
-		 * equivalent.  I could write a recursive version with GLib,
-		 * but this isn't performance-critical and it isn't worth
-		 * the hassle for now, so just call out to "rm -rf" instead.
-		 */
-		string[] argv = { "rm", "-rf", version_path };
-		int exit_status;
-		Process.spawn_sync (null, argv, null, SpawnFlags.SEARCH_PATH,
-				    null, null, null, out exit_status);
-		Process.check_exit_status (exit_status);
-
+		rmtree_onefs(version_path);
+		
 		var package_path = Path.build_filename (root, package);
 		var current_path = Path.build_filename
 			(package_path, "current");

=== modified file 'lib/click/osextras.vala'
--- lib/click/osextras.vala	2014-03-07 11:08:05 +0000
+++ lib/click/osextras.vala	2015-03-11 08:21:00 +0000
@@ -68,6 +68,28 @@
 }
 
 /**
+ * rmtree_onefs:
+ * @path: A path to delete
+ *
+ * Errors when the directory can not be removed
+ */
+public void
+rmtree_onefs (string path) throws Error
+{
+	/* In Python, we used shutil.rmtree(version_path,
+	 * ignore_errors=True), but GLib doesn't have an obvious
+	 * equivalent.  I could write a recursive version with GLib,
+	 * but this isn't performance-critical and it isn't worth
+	 * the hassle for now, so just call out to "rm -rf" instead.
+	 */
+        string[] argv = { "rm", "--one-file-system", "-rf", path };
+	int exit_status;
+	Process.spawn_sync (null, argv, null, SpawnFlags.SEARCH_PATH,
+						null, null, null, out exit_status);
+	Process.check_exit_status (exit_status);
+}
+
+/**
  * unlink_force:
  * @path: A path to unlink.
  *

=== modified file 'lib/click/user.vala'
--- lib/click/user.vala	2015-02-23 07:20:41 +0000
+++ lib/click/user.vala	2015-03-11 08:21:00 +0000
@@ -768,11 +768,37 @@
 		}
 
 		if (! is_pseudo_user)
+		{
 			package_remove_hooks (db, package, old_version, name);
+			package_remove_user_config (package);
+		}
 
-		// run user hooks for all logged in users
+                // run user hooks for all logged in users
 		if (name == ALL_USERS)
 			run_user_remove_hooks_for_all_logged_in_users (package, old_version);
+        }
+       
+	private void
+	package_remove_user_config (string package) throws Error
+	{
+		drop_privileges ();
+		try {
+			string[] dirs = {
+				Path.build_filename (
+					Environment.get_user_config_dir (), package),
+				Path.build_filename (
+					Environment.get_user_cache_dir (), package),
+				Path.build_filename (
+					Environment.get_user_data_dir (), package),
+			};
+			foreach(string dir in dirs)
+			{
+				if (exists(dir))
+					rmtree_onefs (dir);
+			}
+		} finally {
+			regain_privileges ();
+		}
 	}
 
 	/**