Merge lp:~nataliabidart/ubuntuone-control-panel/device-list-type-error into lp:ubuntuone-control-panel

Proposed by Natalia Bidart on 2011-03-31
Status: Merged
Approved by: Natalia Bidart on 2011-03-31
Approved revision: 124
Merged at revision: 121
Proposed branch: lp:~nataliabidart/ubuntuone-control-panel/device-list-type-error
Merge into: lp:ubuntuone-control-panel
Diff against target: 349 lines (+106/-19)
6 files modified
ubuntuone/controlpanel/backend.py (+31/-5)
ubuntuone/controlpanel/dbus_client.py (+0/-1)
ubuntuone/controlpanel/dbus_service.py (+11/-8)
ubuntuone/controlpanel/tests/__init__.py (+10/-0)
ubuntuone/controlpanel/tests/test_backend.py (+50/-2)
ubuntuone/controlpanel/webclient.py (+4/-3)
To merge this branch: bzr merge lp:~nataliabidart/ubuntuone-control-panel/device-list-type-error
Reviewer Review Type Date Requested Status
Martin Albisetti (community) Approve on 2011-03-31
Eric Casteleijn (community) 2011-03-31 Approve on 2011-03-31
Review via email: mp+55808@code.launchpad.net

Commit message

- Made the backend robust against possible None values (or any non basestring instance) sent from the API server (LP: #745790).

Description of the change

There is no good way of IRL test this since we would need to force a response from our API servers to send a null device name, for example.

If you insist, you should temporarly change the line 275 in backend.py and set something like:

local_device["name"] = None

and then run both the backend and UI from this branch.

To post a comment you must log in.
Eric Casteleijn (thisfred) wrote :

Looks great!

review: Approve
124. By Natalia Bidart on 2011-03-31

Typo.

Martin Albisetti (beuno) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/backend.py'
2--- ubuntuone/controlpanel/backend.py 2011-03-29 17:10:59 +0000
3+++ ubuntuone/controlpanel/backend.py 2011-03-31 21:12:36 +0000
4@@ -61,6 +61,16 @@
5 return 'True' if value else ''
6
7
8+def filter_field(info, field):
9+ """Return a copy of 'info' where each item has 'field' hidden."""
10+ result = []
11+ for item in info:
12+ item = item.copy()
13+ item[field] = '<hidden>'
14+ result.append(item)
15+ return result
16+
17+
18 class ControlBackend(object):
19 """The control panel backend."""
20
21@@ -178,6 +188,13 @@
22 # di["available_services"] = ""
23 # di["enabled_services"] = ""
24
25+ # make a sanity check
26+ for key, val in di.iteritems():
27+ if not isinstance(val, basestring):
28+ logger.warning('_process_device_web_info: (key %r), '
29+ 'val %r is not a basestring.', key, val)
30+ di[key] = repr(val)
31+
32 if is_local: # prepend the local device!
33 result.insert(0, di)
34 else:
35@@ -197,7 +214,6 @@
36 dtype, did = self.type_n_id(device_id)
37 local_token = yield self.get_token()
38 is_local = (dtype == DEVICE_TYPE_COMPUTER and did == local_token)
39- logger.info('device_is_local: result %r, ', is_local)
40 returnValue(is_local)
41
42 @log_call(logger.debug)
43@@ -238,6 +254,10 @@
44 show_notifs = yield dbus_client.show_all_notifications_enabled()
45 limits = yield dbus_client.get_throttling_limits()
46
47+ logger.debug('devices_info: file sync enabled? %s limit_bw %s, limits '
48+ '%s, show_notifs %s',
49+ enabled, limit_bw, limits, show_notifs)
50+
51 try:
52 devices = yield self.wc.call_api(DEVICES_API)
53 except WebClientError:
54@@ -247,6 +267,8 @@
55 limit_bw, limits,
56 show_notifs)
57 if result is None:
58+ logger.info('devices_info: result is None after calling '
59+ 'devices/ API, building the local device.')
60 credentials = yield dbus_client.get_credentials()
61 local_device = {}
62 local_device["type"] = DEVICE_TYPE_COMPUTER
63@@ -264,6 +286,10 @@
64 local_device[UPLOAD_KEY] = str(upload)
65 local_device[DOWNLOAD_KEY] = str(download)
66 result = [local_device]
67+ else:
68+ logger.info('devices_info: result is not None after calling '
69+ 'devices/ API: %r',
70+ filter_field(result, field='device_id'))
71
72 returnValue(result)
73
74@@ -275,7 +301,7 @@
75 return DEVICE_TYPE_PHONE, device_id[5:]
76 return "No device", device_id
77
78- @log_call(logger.info)
79+ @log_call(logger.info, with_args=False)
80 @inlineCallbacks
81 def change_device_settings(self, device_id, settings):
82 """Change the settings for the given device."""
83@@ -311,7 +337,7 @@
84 # still pending: more work on the settings dict (LP: #673674)
85 returnValue(device_id)
86
87- @log_call(logger.warning)
88+ @log_call(logger.warning, with_args=False)
89 @inlineCallbacks
90 def remove_device(self, device_id):
91 """Remove a device's tokens from the sso server."""
92@@ -322,8 +348,8 @@
93 yield self.wc.call_api(api)
94
95 if is_local:
96- logger.warning('remove_device: device is local, id %r, '
97- 'clearing credentials.', device_id)
98+ logger.warning('remove_device: device is local! removing and '
99+ 'clearing credentials.')
100 yield dbus_client.clear_credentials()
101
102 returnValue(device_id)
103
104=== modified file 'ubuntuone/controlpanel/dbus_client.py'
105--- ubuntuone/controlpanel/dbus_client.py 2011-02-16 15:56:43 +0000
106+++ ubuntuone/controlpanel/dbus_client.py 2011-03-31 21:12:36 +0000
107@@ -64,7 +64,6 @@
108
109 def found_credentials(app_name, creds):
110 """Credentials have been found."""
111- logger.debug('credentials were found for app_name %r.', app_name)
112 if app_name == APP_NAME:
113 logger.info('credentials were found! (%r).', APP_NAME)
114 d.callback(creds)
115
116=== modified file 'ubuntuone/controlpanel/dbus_service.py'
117--- ubuntuone/controlpanel/dbus_service.py 2011-03-28 17:25:21 +0000
118+++ ubuntuone/controlpanel/dbus_service.py 2011-03-31 21:12:36 +0000
119@@ -32,7 +32,8 @@
120 from ubuntuone.controlpanel import (DBUS_BUS_NAME, DBUS_PREFERENCES_PATH,
121 DBUS_PREFERENCES_IFACE)
122 from ubuntuone.controlpanel.backend import (
123- ControlBackend, FILE_SYNC_DISABLED, FILE_SYNC_DISCONNECTED,
124+ ControlBackend, filter_field,
125+ FILE_SYNC_DISABLED, FILE_SYNC_DISCONNECTED,
126 FILE_SYNC_ERROR, FILE_SYNC_IDLE, FILE_SYNC_STARTING, FILE_SYNC_STOPPED,
127 FILE_SYNC_SYNCING,
128 MSG_KEY, STATUS_KEY,
129@@ -85,6 +86,7 @@
130 """
131 def inner(error, _=None):
132 """Do the Failure transformation."""
133+ logger.error('processing failure: %r', error.printTraceback())
134 error_dict = error_handler(error)
135 if _ is not None:
136 result = f(_, error_dict)
137@@ -149,10 +151,11 @@
138 d.addCallback(self.DevicesInfoReady)
139 d.addErrback(transform_failure(self.DevicesInfoError))
140
141- @log_call(logger.debug)
142 @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="aa{ss}")
143 def DevicesInfoReady(self, info):
144 """The info for the devices is available right now."""
145+ logger.debug('DevicesInfoReady: args %r',
146+ filter_field(info, field='device_id'))
147
148 @log_call(logger.error)
149 @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="a{ss}")
150@@ -161,7 +164,7 @@
151
152 #---
153
154- @log_call(logger.info)
155+ @log_call(logger.info, with_args=False)
156 @method(dbus_interface=DBUS_PREFERENCES_IFACE, in_signature="sa{ss}")
157 def change_device_settings(self, device_id, settings):
158 """Configure a given device."""
159@@ -170,19 +173,19 @@
160 d.addErrback(transform_failure(self.DeviceSettingsChangeError),
161 device_id)
162
163- @log_call(logger.info)
164+ @log_call(logger.info, with_args=False)
165 @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="s")
166 def DeviceSettingsChanged(self, device_id):
167 """The settings for the device were changed."""
168
169- @log_call(logger.error)
170+ @log_call(logger.error, with_args=False)
171 @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="sa{ss}")
172 def DeviceSettingsChangeError(self, device_id, error):
173 """Problem changing settings for the device."""
174
175 #---
176
177- @log_call(logger.warning)
178+ @log_call(logger.warning, with_args=False)
179 @method(dbus_interface=DBUS_PREFERENCES_IFACE, in_signature="s")
180 def remove_device(self, device_id):
181 """Remove a given device."""
182@@ -190,12 +193,12 @@
183 d.addCallback(self.DeviceRemoved)
184 d.addErrback(transform_failure(self.DeviceRemovalError), device_id)
185
186- @log_call(logger.warning)
187+ @log_call(logger.warning, with_args=False)
188 @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="s")
189 def DeviceRemoved(self, device_id):
190 """The removal for the device was completed."""
191
192- @log_call(logger.error)
193+ @log_call(logger.error, with_args=False)
194 @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="sa{ss}")
195 def DeviceRemovalError(self, device_id, error):
196 """Problem removing the device."""
197
198=== modified file 'ubuntuone/controlpanel/tests/__init__.py'
199--- ubuntuone/controlpanel/tests/__init__.py 2011-03-29 11:38:39 +0000
200+++ ubuntuone/controlpanel/tests/__init__.py 2011-03-31 21:12:36 +0000
201@@ -110,6 +110,16 @@
202 ]
203 """
204
205+EMPTY_DESCRIPTION_JSON = """
206+[
207+ {
208+ "token": "ABCDEF01234token",
209+ "description": null,
210+ "kind": "Computer"
211+ }
212+]
213+"""
214+
215 LOCAL_DEVICE = {
216 'is_local': 'True',
217 'configurable': 'True',
218
219=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
220--- ubuntuone/controlpanel/tests/test_backend.py 2011-03-29 14:48:28 +0000
221+++ ubuntuone/controlpanel/tests/test_backend.py 2011-03-31 21:12:36 +0000
222@@ -30,6 +30,7 @@
223 from ubuntuone.controlpanel import backend, replication_client
224 from ubuntuone.controlpanel.backend import (bool_str,
225 ACCOUNT_API, DEVICES_API, DEVICE_REMOVE_API, QUOTA_API,
226+ DEVICE_TYPE_COMPUTER,
227 FILE_SYNC_DISABLED,
228 FILE_SYNC_DISCONNECTED,
229 FILE_SYNC_ERROR,
230@@ -41,6 +42,7 @@
231 MSG_KEY, STATUS_KEY,
232 )
233 from ubuntuone.controlpanel.tests import (TestCase,
234+ EMPTY_DESCRIPTION_JSON,
235 EXPECTED_ACCOUNT_INFO,
236 EXPECTED_ACCOUNT_INFO_WITH_CURRENT_PLAN,
237 EXPECTED_DEVICES_INFO,
238@@ -251,7 +253,7 @@
239 self.patch(backend, "WebClient", MockWebClient)
240 self.patch(backend, "dbus_client", MockDBusClient())
241 self.patch(backend, "replication_client", MockReplicationClient())
242- self.local_token = "Computer" + TOKEN["token"]
243+ self.local_token = DEVICE_TYPE_COMPUTER + TOKEN["token"]
244 self.be = backend.ControlBackend()
245
246 self.memento = MementoHandler()
247@@ -380,9 +382,35 @@
248 self.assertEqual(result, expected)
249
250 @inlineCallbacks
251+ def test_devices_info_when_token_name_is_empty(self):
252+ """The devices_info can handle empty token names."""
253+ # pylint: disable=E1101
254+ self.be.wc.results[DEVICES_API] = EMPTY_DESCRIPTION_JSON
255+ result = yield self.be.devices_info()
256+ expected = {'configurable': '',
257+ 'device_id': 'ComputerABCDEF01234token',
258+ 'is_local': '', 'name': 'None',
259+ 'type': DEVICE_TYPE_COMPUTER}
260+ self.assertEqual(result, [expected])
261+ self.assertTrue(self.memento.check_warning('name', 'None'))
262+
263+ @inlineCallbacks
264+ def test_devices_info_does_not_log_device_id(self):
265+ """The devices_info does not log the device_id."""
266+ # pylint: disable=E1101
267+ self.be.wc.results[DEVICES_API] = SAMPLE_DEVICES_JSON
268+ yield self.be.devices_info()
269+
270+ dids = (d['device_id'] for d in EXPECTED_DEVICES_INFO)
271+ device_id_logged = all(all(did not in r.getMessage()
272+ for r in self.memento.records)
273+ for did in dids)
274+ self.assertTrue(device_id_logged)
275+
276+ @inlineCallbacks
277 def test_remove_device(self):
278 """The remove_device method calls the right api."""
279- dtype, did = "Computer", "SAMPLE-TOKEN"
280+ dtype, did = DEVICE_TYPE_COMPUTER, "SAMPLE-TOKEN"
281 device_id = dtype + did
282 apiurl = DEVICE_REMOVE_API % (dtype.lower(), did)
283 # pylint: disable=E1101
284@@ -411,6 +439,16 @@
285 WebClientError)
286
287 @inlineCallbacks
288+ def test_remove_device_does_not_log_device_id(self):
289+ """The remove_device does not log the device_id."""
290+ device_id = DEVICE_TYPE_COMPUTER + TOKEN['token']
291+ yield self.be.remove_device(device_id)
292+
293+ device_id_logged = all(device_id not in r.getMessage()
294+ for r in self.memento.records)
295+ self.assertTrue(device_id_logged)
296+
297+ @inlineCallbacks
298 def test_change_show_all_notifications(self):
299 """The device settings are updated."""
300 backend.dbus_client.show_all_notifications = False
301@@ -465,6 +503,16 @@
302 self.assertEqual(backend.dbus_client.limits["upload"], -1)
303 self.assertEqual(backend.dbus_client.limits["download"], -1)
304
305+ @inlineCallbacks
306+ def test_changing_settings_does_not_log_device_id(self):
307+ """The change_device_settings does not log the device_id."""
308+ device_id = 'yadda-yadda'
309+ yield self.be.change_device_settings(device_id, {})
310+
311+ device_id_logged = all(device_id not in r.getMessage()
312+ for r in self.memento.records)
313+ self.assertTrue(device_id_logged)
314+
315
316 class BackendVolumesTestCase(BackendBasicTestCase):
317 """Volumes tests for the backend."""
318
319=== modified file 'ubuntuone/controlpanel/webclient.py'
320--- ubuntuone/controlpanel/webclient.py 2010-12-18 16:21:36 +0000
321+++ ubuntuone/controlpanel/webclient.py 2011-03-31 21:12:36 +0000
322@@ -48,7 +48,7 @@
323
324 def _handler(self, session, msg, d):
325 """Handle the result of an http message."""
326- logger.debug("WebClient: got http response %d for uri %r",
327+ logger.debug("got http response %d for uri %r",
328 msg.status_code, msg.get_uri().to_string(False))
329 data = msg.response_body.data
330 if msg.status_code == 200:
331@@ -62,7 +62,7 @@
332 """Get a given url from the webservice with credentials."""
333 url = (self.base_url + api_name).encode('utf-8')
334 method = "GET"
335- logger.debug("WebClient: getting url: %s, %s", method, url)
336+ logger.debug("getting url: %s, %s", method, url)
337 msg = Soup.Message.new(method, url)
338 add_oauth_headers(msg.request_headers.append, method, url, credentials)
339 d = defer.Deferred()
340@@ -71,7 +71,8 @@
341
342 def call_api(self, api_name):
343 """Get a given url from the webservice."""
344- logger.debug("WebClient: calling api: %s", api_name)
345+ # this may log device ID's, but only for removals, which is OK
346+ logger.debug("calling api: %s", api_name)
347 d = self.get_credentials()
348 d.addCallback(self._call_api_with_creds, api_name)
349 return d

Subscribers

People subscribed via source and target branches