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

Proposed by Michael Vogt on 2015-01-19
Status: Needs review
Proposed branch: lp:~mvo/click/lp1358294-config-removal
Merge into: lp:click/devel
Diff against target: 230 lines (+155/-13)
4 files modified
click/tests/test_user.py (+104/-0)
lib/click/database.vala (+2/-12)
lib/click/osextras.vala (+22/-0)
lib/click/user.vala (+27/-1)
To merge this branch: bzr merge lp:~mvo/click/lp1358294-config-removal
Reviewer Review Type Date Requested Status
dobey Disapprove on 2015-06-24
Alejandro J. Cura (community) Needs Fixing on 2015-05-20
Colin Watson 2015-01-19 Needs Fixing on 2015-01-28
Review via email: mp+246918@code.launchpad.net

Description of the change

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

To post a comment you must log in.
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) wrote :

@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) wrote :

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
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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'click/tests/test_user.py'
2--- click/tests/test_user.py 2014-10-02 06:57:37 +0000
3+++ click/tests/test_user.py 2015-03-11 08:21:00 +0000
4@@ -26,10 +26,24 @@
5 import json
6 import os
7 import shutil
8+import tempfile
9 from textwrap import dedent
10
11+
12+# Important:
13+#
14+# glib reads the environment only once and caches it then
15+# so we need to create the
16+#
17+# ensure that the xdg dirs point to a different place as
18+# registry.remove() will remove stuff here
19+xdg_tempdir = tempfile.mkdtemp()
20+for xdgdir in ("XDG_CONFIG_HOME", "XDG_CACHE_HOME", "XDG_DATA_HOME"):
21+ os.environ[xdgdir] = os.path.join(xdg_tempdir, xdgdir)
22+# ensure this is imported after the environment manipulation
23 from gi.repository import Click, GLib
24
25+
26 from click.json_helpers import json_array_to_python, json_object_to_python
27 from click.tests.gimock_types import Passwd
28 from click.tests.helpers import (
29@@ -42,6 +56,7 @@
30
31
32 class TestClickUser(TestCase):
33+
34 def setUp(self):
35 super(TestClickUser, self).setUp()
36 self.use_temp_dir()
37@@ -589,3 +604,92 @@
38 # ensure that stop was called with the right app
39 with open(self.fake_app_stop_output) as f:
40 self.assertEqual("meep_a-app_2.0", f.read().strip())
41+
42+
43+class UserDataRemovalTestCase(TestCase):
44+
45+ @classmethod
46+ def setUpClass(cls):
47+ cls.class_temp_dir = xdg_tempdir
48+
49+ def tearDown(self):
50+ super(UserDataRemovalTestCase, self).tearDown()
51+ for entry in os.listdir(self.class_temp_dir):
52+ shutil.rmtree(os.path.join(self.class_temp_dir, entry))
53+
54+ def setUp(self):
55+ super(UserDataRemovalTestCase, self).setUp()
56+ self.use_temp_dir()
57+ self.db = Click.DB()
58+ self.db.add(self.temp_dir)
59+ # add app
60+ self.appname = "some-app"
61+ self.registry = Click.User.for_user(self.db, "user")
62+ os.makedirs(self.registry.get_overlay_db())
63+ path = os.path.join(self.registry.get_overlay_db(), self.appname)
64+ os.symlink("/1.0", path)
65+
66+ def test_remove_removes_config(self):
67+ """
68+ Ensure that all user data is removed if a click is removed
69+ (LP: #1358294)
70+ """
71+ fake_config_dir = os.path.join(
72+ os.environ["XDG_CONFIG_HOME"], self.appname)
73+ fake_cache_dir = os.path.join(
74+ os.environ["XDG_CACHE_HOME"], self.appname)
75+ fake_data_dir = os.path.join(
76+ os.environ["XDG_DATA_HOME"], self.appname)
77+ os.makedirs(fake_config_dir)
78+ os.makedirs(fake_cache_dir)
79+ os.makedirs(fake_data_dir)
80+
81+ self.registry.remove(self.appname)
82+
83+ self.assertFalse(os.path.exists(fake_config_dir))
84+ self.assertFalse(os.path.exists(fake_cache_dir))
85+ self.assertFalse(os.path.exists(fake_data_dir))
86+
87+ def test_remove_does_not_follow_symlinks_inside_dir(self):
88+ """
89+ Ensure that symlinks that are inside a user-data dir get removed,
90+ but that the symlink targets are not touched
91+ """
92+ fake_config_dir = os.path.join(
93+ os.environ["XDG_CONFIG_HOME"], self.appname)
94+ os.makedirs(fake_config_dir)
95+ # setup symlink inside the config dir that that points
96+ # to a file that is outside of the config dir
97+ canary = os.path.join(self.temp_dir, "canary")
98+ touch(canary)
99+ os.symlink(canary, os.path.join(fake_config_dir, "canary"))
100+ self.assertTrue(os.path.islink(os.path.join(fake_config_dir, "canary")))
101+
102+ self.registry.remove(self.appname)
103+
104+ # ensure that the config dir is gone
105+ self.assertFalse(os.path.exists(fake_config_dir))
106+ # ... but the canary file is still there
107+ self.assertTrue(os.path.exists(canary))
108+
109+ def test_remove_does_not_follow_if_xdg_dir_is_symlink(self):
110+ """
111+ Ensure that if the user-data dir is a symlink that symlink is
112+ removed but that the symlink target is not touched
113+ """
114+ fake_config_dir = os.path.join(
115+ os.environ["XDG_CONFIG_HOME"], self.appname)
116+ os.makedirs(os.environ["XDG_CONFIG_HOME"])
117+
118+ # setup the config dir as a symlink to a other dir
119+ canary = os.path.join(self.temp_dir, "canary")
120+ os.mkdir(canary)
121+ os.symlink(canary, fake_config_dir)
122+ self.assertTrue(os.path.islink(fake_config_dir))
123+
124+ self.registry.remove(self.appname)
125+
126+ # ensure that the config dir is gone
127+ self.assertFalse(os.path.exists(fake_config_dir))
128+ # ... but the canary dir is still there
129+ self.assertTrue(os.path.exists(canary))
130
131=== modified file 'lib/click/database.vala'
132--- lib/click/database.vala 2014-09-29 11:40:56 +0000
133+++ lib/click/database.vala 2015-03-11 08:21:00 +0000
134@@ -341,18 +341,8 @@
135 if (show_messages ())
136 message ("Removing %s", version_path);
137 package_remove_hooks (master_db, package, version);
138- /* In Python, we used shutil.rmtree(version_path,
139- * ignore_errors=True), but GLib doesn't have an obvious
140- * equivalent. I could write a recursive version with GLib,
141- * but this isn't performance-critical and it isn't worth
142- * the hassle for now, so just call out to "rm -rf" instead.
143- */
144- string[] argv = { "rm", "-rf", version_path };
145- int exit_status;
146- Process.spawn_sync (null, argv, null, SpawnFlags.SEARCH_PATH,
147- null, null, null, out exit_status);
148- Process.check_exit_status (exit_status);
149-
150+ rmtree_onefs(version_path);
151+
152 var package_path = Path.build_filename (root, package);
153 var current_path = Path.build_filename
154 (package_path, "current");
155
156=== modified file 'lib/click/osextras.vala'
157--- lib/click/osextras.vala 2014-03-07 11:08:05 +0000
158+++ lib/click/osextras.vala 2015-03-11 08:21:00 +0000
159@@ -68,6 +68,28 @@
160 }
161
162 /**
163+ * rmtree_onefs:
164+ * @path: A path to delete
165+ *
166+ * Errors when the directory can not be removed
167+ */
168+public void
169+rmtree_onefs (string path) throws Error
170+{
171+ /* In Python, we used shutil.rmtree(version_path,
172+ * ignore_errors=True), but GLib doesn't have an obvious
173+ * equivalent. I could write a recursive version with GLib,
174+ * but this isn't performance-critical and it isn't worth
175+ * the hassle for now, so just call out to "rm -rf" instead.
176+ */
177+ string[] argv = { "rm", "--one-file-system", "-rf", path };
178+ int exit_status;
179+ Process.spawn_sync (null, argv, null, SpawnFlags.SEARCH_PATH,
180+ null, null, null, out exit_status);
181+ Process.check_exit_status (exit_status);
182+}
183+
184+/**
185 * unlink_force:
186 * @path: A path to unlink.
187 *
188
189=== modified file 'lib/click/user.vala'
190--- lib/click/user.vala 2015-02-23 07:20:41 +0000
191+++ lib/click/user.vala 2015-03-11 08:21:00 +0000
192@@ -768,11 +768,37 @@
193 }
194
195 if (! is_pseudo_user)
196+ {
197 package_remove_hooks (db, package, old_version, name);
198+ package_remove_user_config (package);
199+ }
200
201- // run user hooks for all logged in users
202+ // run user hooks for all logged in users
203 if (name == ALL_USERS)
204 run_user_remove_hooks_for_all_logged_in_users (package, old_version);
205+ }
206+
207+ private void
208+ package_remove_user_config (string package) throws Error
209+ {
210+ drop_privileges ();
211+ try {
212+ string[] dirs = {
213+ Path.build_filename (
214+ Environment.get_user_config_dir (), package),
215+ Path.build_filename (
216+ Environment.get_user_cache_dir (), package),
217+ Path.build_filename (
218+ Environment.get_user_data_dir (), package),
219+ };
220+ foreach(string dir in dirs)
221+ {
222+ if (exists(dir))
223+ rmtree_onefs (dir);
224+ }
225+ } finally {
226+ regain_privileges ();
227+ }
228 }
229
230 /**

Subscribers

People subscribed via source and target branches

to all changes: