Ubuntu Single Sign On Client

Merge lp:~nataliabidart/ubuntu-sso-client/better-errors into lp:ubuntu-sso-client

Proposed by Natalia Bidart on 2010-08-26
Status: Merged
Approved by: Natalia Bidart on 2010-08-26
Approved revision: 603
Merged at revision: 600
Proposed branch: lp:~nataliabidart/ubuntu-sso-client/better-errors
Merge into: lp:ubuntu-sso-client
Diff against target: 488 lines (+173/-45) 4 files modified
To merge this branch: bzr merge lp:~nataliabidart/ubuntu-sso-client/better-errors
Reviewer Review Type Date Requested Status
John Lenton Approve on 2010-08-26
Vincenzo Di Somma (community) 2010-08-26 Approve on 2010-08-26
Review via email: mp+33846@code.launchpad.net

Commit Message

Errors from SSO servers are being shown now to users, matching error-specific to fields (LP: #616101).
Also, be robust when SSO server answer with a string where it's supposed to be a list (LP: #623447).

Description of the Change

Errors from SSO servers are being shown now to users, matching error-specific to fields.

To post a comment you must log in.
Natalia Bidart (nataliabidart) wrote :

To test this, you should try both registration and login using email address like '@' (that will make the GUI accept the address as valid but SSO will reject it).

You can also try registering with an already registered email address, etc.

Also, you can try logging with non existing accounts, and resetting passwords for non-existing accounts, and resetting password for a valid account but using a bad token.

603. By Natalia Bidart on 2010-08-26

Merged trunk in.

Vincenzo Di Somma (vds) :
review: Approve
John Lenton (chipaca) :
review: Approve

Preview Diff

1=== modified file 'ubuntu_sso/gui.py'
2--- ubuntu_sso/gui.py 2010-08-26 18:44:24 +0000
3+++ ubuntu_sso/gui.py 2010-08-26 22:54:41 +0000
4@@ -547,7 +547,6 @@
5
6 def _build_success_page(self):
7 """Build the success page."""
8- #self.success_vbox.help_text = ''
9 self.success_label.set_markup('<span size="x-large">%s</span>' %
10 self.SUCCESS)
11 return self.success_vbox
12@@ -846,6 +845,17 @@
13
14 # backend callbacks
15
16+ def _build_general_error_message(self, errordict):
17+ """Concatenate __all__ and message from the errordict."""
18+ result = None
19+ msg1 = errordict.get('__all__')
20+ msg2 = errordict.get('message')
21+ if msg1 is not None and msg2 is not None:
22+ result = '\n'.join((msg1, msg2))
23+ else:
24+ result = msg1 if msg1 is not None else msg2
25+ return result
26+
27 @log_call
28 def on_captcha_generated(self, app_name, captcha_id, *args, **kwargs):
29 """Captcha image has been generated and is available to be shown."""
30@@ -868,10 +878,21 @@
31 @log_call
32 def on_user_registration_error(self, app_name, error, *args, **kwargs):
33 """Captcha image generation failed."""
34- self._set_current_page(self.enter_details_vbox,
35- warning_text=self.UNKNOWN_ERROR)
36+ msg = error.get('email')
37+ if msg is not None:
38+ self.email1_entry.set_warning(msg)
39+ self.email2_entry.set_warning(msg)
40+
41+ msg = error.get('password')
42+ if msg is not None:
43+ self.password1_entry.set_warning(msg)
44+ self.password2_entry.set_warning(msg)
45+
46+ msg = self._build_general_error_message(error)
47+ self._set_current_page(self.enter_details_vbox, warning_text=msg)
48+
49 self._gtk_signal_log.append((SIG_REGISTRATION_FAILED, self.app_name,
50- error))
51+ msg))
52
53 @log_call
54 def on_email_validated(self, app_name, email, *args, **kwargs):
55@@ -883,10 +904,15 @@
56 @log_call
57 def on_email_validation_error(self, app_name, error, *args, **kwargs):
58 """User email validation failed."""
59- self._set_current_page(self.verify_email_vbox,
60- warning_text=self.UNKNOWN_ERROR)
61+ msg = error.get('email_token')
62+ if msg is not None:
63+ self.email_token_entry.set_warning(msg)
64+
65+ msg = self._build_general_error_message(error)
66+ self._set_current_page(self.verify_email_vbox, warning_text=msg)
67+
68 self._gtk_signal_log.append((SIG_REGISTRATION_FAILED, self.app_name,
69- error))
70+ msg))
71
72 @log_call
73 def on_logged_in(self, app_name, email, *args, **kwargs):
74@@ -898,10 +924,10 @@
75 @log_call
76 def on_login_error(self, app_name, error, *args, **kwargs):
77 """User was not successfully logged in."""
78- self._set_current_page(self.login_vbox,
79- warning_text=self.UNKNOWN_ERROR)
80+ msg = self._build_general_error_message(error)
81+ self._set_current_page(self.login_vbox, warning_text=msg)
82 self._gtk_signal_log.append((SIG_LOGIN_FAILED, self.app_name,
83- error))
84+ msg))
85
86 @log_call
87 def on_password_reset_token_sent(self, app_name, email, *args, **kwargs):
88@@ -913,8 +939,8 @@
89 @log_call
90 def on_password_reset_error(self, app_name, error, *args, **kwargs):
91 """Password reset failed."""
92- self._set_current_page(self.login_vbox,
93- warning_text=self.UNKNOWN_ERROR)
94+ msg = self._build_general_error_message(error)
95+ self._set_current_page(self.login_vbox, warning_text=msg)
96
97 @log_call
98 def on_password_changed(self, app_name, email, *args, **kwargs):
99@@ -925,5 +951,6 @@
100 @log_call
101 def on_password_change_error(self, app_name, error, *args, **kwargs):
102 """Password reset failed."""
103+ msg = self._build_general_error_message(error)
104 self._set_current_page(self.request_password_token_vbox,
105- warning_text=self.UNKNOWN_ERROR)
106+ warning_text=msg)
107
108=== modified file 'ubuntu_sso/main.py'
109--- ubuntu_sso/main.py 2010-08-26 21:06:36 +0000
110+++ ubuntu_sso/main.py 2010-08-26 22:54:41 +0000
111@@ -139,8 +139,12 @@
112 def _format_webservice_errors(self, errdict):
113 """Turn each list of strings in the errdict into a LF separated str."""
114 result = {}
115- for k in errdict:
116- result[k] = "\n".join(errdict[k])
117+ for k, v in errdict.iteritems():
118+ # workaround until bug #624955 is solved
119+ if isinstance(v, basestring):
120+ result[k] = v
121+ else:
122+ result[k] = "\n".join(v)
123 return result
124
125 def generate_captcha(self, filename):
126@@ -239,7 +243,7 @@
127 result = service(email=email)
128 except HTTPError, e:
129 logger.exception('request_password_reset_token failed with:')
130- raise ResetPasswordTokenError(e.content)
131+ raise ResetPasswordTokenError(e.content.split('\n')[0])
132
133 if result['status'] == 'ok':
134 return email
135@@ -261,7 +265,7 @@
136 new_password=new_password)
137 except HTTPError, e:
138 logger.exception('set_new_password failed with:')
139- raise NewPasswordError(e.content)
140+ raise NewPasswordError(e.content.split('\n')[0])
141
142 if result['status'] == 'ok':
143 return email
144@@ -273,7 +277,6 @@
145 """Turn an exception into a dictionary to return thru DBus."""
146 result = {
147 "errtype": e.__class__.__name__,
148- "errargs": repr(e.args),
149 }
150 if len(e.args) == 0:
151 result["message"] = e.__class__.__doc__
152
153=== modified file 'ubuntu_sso/tests/test_gui.py'
154--- ubuntu_sso/tests/test_gui.py 2010-08-26 18:44:24 +0000
155+++ ubuntu_sso/tests/test_gui.py 2010-08-26 22:54:41 +0000
156@@ -44,7 +44,6 @@
157 CAPTCHA_SOLUTION = 'william Byrd'
158 EMAIL = 'test@example.com'
159 EMAIL_TOKEN = 'B2Pgtf'
160-ERROR = 'Something bad happened!'
161 NAME = 'Juanito Pérez'
162 PASSWORD = 'h3lloWorld'
163 RESET_PASSWORD_TOKEN = '8G5Wtq'
164@@ -344,6 +343,7 @@
165 'tc_browser', 'login', 'request_password_token',
166 'set_new_password')
167 self.ui = self.gui_class(**self.kwargs)
168+ self.ERROR = {'message': self.ui.UNKNOWN_ERROR}
169
170 def tearDown(self):
171 """Clean up."""
172@@ -866,17 +866,39 @@
173 """Init."""
174 super(UserRegistrationErrorTestCase, self).setUp()
175 self.click_join_with_valid_data()
176- self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
177
178 def test_previous_page_is_shown(self):
179 """On UserRegistrationError the previous page is shown."""
180+ self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
181 self.assert_pages_visibility(enter_details=True)
182
183 def test_warning_label_is_shown(self):
184 """On UserRegistrationError the warning label is shown."""
185+ self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
186 self.assert_correct_label_warning(self.ui.warning_label,
187 self.ui.UNKNOWN_ERROR)
188
189+ def test_specific_errors_from_backend_are_shown(self):
190+ """Specific errors from backend are used."""
191+ error = {'errtype': 'RegistrationError',
192+ 'message': 'We\'re so doomed.',
193+ 'email': 'Enter a valid e-mail address.',
194+ 'password': 'I don\'t like your password.',
195+ '__all__': 'Wrong captcha solution.'}
196+
197+ self.ui.on_user_registration_error(app_name=APP_NAME, error=error)
198+
199+ expected = '\n'.join((error['__all__'], error['message']))
200+ self.assert_correct_label_warning(self.ui.warning_label, expected)
201+ self.assert_correct_entry_warning(self.ui.email1_entry,
202+ error['email'])
203+ self.assert_correct_entry_warning(self.ui.email2_entry,
204+ error['email'])
205+ self.assert_correct_entry_warning(self.ui.password1_entry,
206+ error['password'])
207+ self.assert_correct_entry_warning(self.ui.password2_entry,
208+ error['password'])
209+
210
211 class VerifyEmailTestCase(UbuntuSSOClientTestCase):
212 """Test suite for the user registration (verify email page)."""
213@@ -926,19 +948,33 @@
214 self.ui.on_email_validated(app_name=APP_NAME, email=EMAIL)
215 self.assert_pages_visibility(success=True)
216
217- def test_on_email_validated_clears_the_help_text(self):
218- """On email validated the help text is removed."""
219+ def test_on_email_validated_does_not_clear_the_help_text(self):
220+ """On email validated the help text is not removed."""
221 self.ui.on_email_validated(app_name=APP_NAME, email=EMAIL)
222- self.assertEqual('', self.ui.help_label.get_text())
223- test_on_email_validated_clears_the_help_text.skip = 'Maybe this is wrong.'
224+ self.assertEqual(self.ui.verify_email_vbox.help_text,
225+ self.ui.help_label.get_label())
226
227 def test_on_email_validation_error_verify_email_is_shown(self):
228 """On email validation error, the verify_email page is shown."""
229- self.ui.on_email_validation_error(app_name=APP_NAME, error=ERROR)
230+ self.ui.on_email_validation_error(app_name=APP_NAME, error=self.ERROR)
231 self.assert_pages_visibility(verify_email=True)
232 self.assert_correct_label_warning(self.ui.warning_label,
233 self.ui.UNKNOWN_ERROR)
234
235+ def test_specific_errors_from_backend_are_shown(self):
236+ """Specific errors from backend are used."""
237+ error = {'errtype': 'EmailValidationError',
238+ 'message': 'We\'re so doomed.',
239+ 'email_token': 'Enter a valid e-mail address.',
240+ '__all__': 'We all are gonna die.'}
241+
242+ self.ui.on_email_validation_error(app_name=APP_NAME, error=error)
243+
244+ expected = '\n'.join((error['__all__'], error['message']))
245+ self.assert_correct_label_warning(self.ui.warning_label, expected)
246+ self.assert_correct_entry_warning(self.ui.email_token_entry,
247+ error['email_token'])
248+
249 def test_success_label_is_correct(self):
250 """The success message is correct."""
251 self.assertEqual(self.ui.SUCCESS, self.ui.success_label.get_text())
252@@ -1152,20 +1188,31 @@
253 def test_on_login_error_morphs_to_login_page(self):
254 """On user login error, the previous page is shown."""
255 self.click_connect_with_valid_data()
256- self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
257+ self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
258 self.assert_pages_visibility(login=True)
259
260 def test_on_login_error_a_warning_is_shown(self):
261 """On user login error, a warning is shown with proper wording."""
262 self.click_connect_with_valid_data()
263- self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
264+ self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
265 self.assert_correct_label_warning(self.ui.warning_label,
266 self.ui.UNKNOWN_ERROR)
267
268+ def test_specific_errors_from_backend_are_shown(self):
269+ """Specific errors from backend are used."""
270+ error = {'errtype': 'AuthenticationError',
271+ 'message': 'We\'re so doomed.',
272+ '__all__': 'We all are gonna die.'}
273+
274+ self.ui.on_login_error(app_name=APP_NAME, error=error)
275+
276+ expected = '\n'.join((error['__all__'], error['message']))
277+ self.assert_correct_label_warning(self.ui.warning_label, expected)
278+
279 def test_back_to_registration_hides_warning(self):
280 """After user login error, warning is hidden when clicking 'Back'."""
281 self.click_connect_with_valid_data()
282- self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
283+ self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
284 self.ui.login_back_button.clicked()
285 self.assertFalse(self.ui.warning_label.get_property('visible'))
286
287@@ -1317,11 +1364,22 @@
288
289 def test_on_password_reset_error_shows_login_page(self):
290 """When reset token wasn't successfuly sent the login page is shown."""
291- self.ui.on_password_reset_error(app_name=APP_NAME, error=ERROR)
292+ self.ui.on_password_reset_error(app_name=APP_NAME, error=self.ERROR)
293 self.assert_correct_label_warning(self.ui.warning_label,
294 self.ui.UNKNOWN_ERROR)
295 self.assert_pages_visibility(login=True)
296
297+ def test_specific_errors_from_backend_are_shown(self):
298+ """Specific errors from backend are used."""
299+ error = {'errtype': 'ResetPasswordTokenError',
300+ 'message': 'We\'re so doomed.',
301+ '__all__': 'We all are gonna die.'}
302+
303+ self.ui.on_password_reset_error(app_name=APP_NAME, error=error)
304+
305+ expected = '\n'.join((error['__all__'], error['message']))
306+ self.assert_correct_label_warning(self.ui.warning_label, expected)
307+
308
309 class SetNewPasswordTestCase(UbuntuSSOClientTestCase):
310 """Test suite for setting a new password functionality."""
311@@ -1374,11 +1432,22 @@
312
313 def test_on_password_change_error_shows_login_page(self):
314 """When password wasn't changed the reset password page is shown."""
315- self.ui.on_password_change_error(app_name=APP_NAME, error=ERROR)
316+ self.ui.on_password_change_error(app_name=APP_NAME, error=self.ERROR)
317 self.assert_correct_label_warning(self.ui.warning_label,
318 self.ui.UNKNOWN_ERROR)
319 self.assert_pages_visibility(request_password_token=True)
320
321+ def test_specific_errors_from_backend_are_shown(self):
322+ """Specific errors from backend are used."""
323+ error = {'errtype': 'NewPasswordError',
324+ 'message': 'We\'re so doomed.',
325+ '__all__': 'We all are gonna die.'}
326+
327+ self.ui.on_password_change_error(app_name=APP_NAME, error=error)
328+
329+ expected = '\n'.join((error['__all__'], error['message']))
330+ self.assert_correct_label_warning(self.ui.warning_label, expected)
331+
332
333 class DbusTestCase(UbuntuSSOClientTestCase):
334 """Test suite for the dbus calls."""
335@@ -1458,9 +1527,10 @@
336
337 def test_on_user_registration_error_proper_signal_is_emitted(self):
338 """On UserRegistrationError, 'registration-failed' signal is sent."""
339- self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
340+ self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
341 self.ui.on_close_clicked()
342- self.assertEqual((self.ui.window, (APP_NAME, ERROR), {}),
343+ expected = (self.ui.window, (APP_NAME, self.ui.UNKNOWN_ERROR), {})
344+ self.assertEqual(expected,
345 self._called[gui.SIG_REGISTRATION_FAILED])
346
347 def test_on_email_validated_proper_signals_is_emitted(self):
348@@ -1472,10 +1542,11 @@
349
350 def test_on_email_validation_error_proper_signals_is_emitted(self):
351 """On EmailValidationError, 'registration-failed' signal is sent."""
352- self.ui.on_email_validation_error(app_name=APP_NAME, error=ERROR)
353+ self.ui.on_email_validation_error(app_name=APP_NAME, error=self.ERROR)
354 self.ui.on_close_clicked()
355- self.assertEqual((self.ui.window, (APP_NAME, ERROR), {}),
356- self._called[gui.SIG_REGISTRATION_FAILED])
357+ expected = (self.ui.window, (APP_NAME, self.ui.UNKNOWN_ERROR), {})
358+ self.assertEqual(expected,
359+ self._called[gui.SIG_REGISTRATION_FAILED])
360
361 def test_on_logged_in_proper_signals_is_emitted(self):
362 """On LoggedIn, 'login-succeeded' signal is sent."""
363@@ -1487,15 +1558,16 @@
364 def test_on_login_error_proper_signals_is_emitted(self):
365 """On LoginError, 'login-failed' signal is sent."""
366 self.click_connect_with_valid_data()
367- self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
368+ self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
369 self.ui.on_close_clicked()
370- self.assertEqual((self.ui.window, (APP_NAME, ERROR), {}),
371- self._called[gui.SIG_LOGIN_FAILED])
372+ expected = (self.ui.window, (APP_NAME, self.ui.UNKNOWN_ERROR), {})
373+ self.assertEqual(expected,
374+ self._called[gui.SIG_LOGIN_FAILED])
375
376 def test_registration_successfull_even_if_prior_registration_error(self):
377 """Only one signal is sent with the final outcome."""
378 self.click_join_with_valid_data()
379- self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
380+ self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
381 self.click_join_with_valid_data()
382 self.ui.on_email_validated(app_name=APP_NAME, email=EMAIL)
383 self.ui.on_close_clicked()
384@@ -1506,7 +1578,7 @@
385 def test_login_successfull_even_if_prior_login_error(self):
386 """Only one signal is sent with the final outcome."""
387 self.click_connect_with_valid_data()
388- self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
389+ self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
390 self.click_connect_with_valid_data()
391 self.ui.on_logged_in(app_name=APP_NAME, email=EMAIL)
392 self.ui.on_close_clicked()
393@@ -1517,7 +1589,7 @@
394 def test_user_cancelation_even_if_prior_registration_error(self):
395 """Only one signal is sent with the final outcome."""
396 self.click_join_with_valid_data()
397- self.ui.on_user_registration_error(app_name=APP_NAME, error=ERROR)
398+ self.ui.on_user_registration_error(app_name=APP_NAME, error=self.ERROR)
399 self.ui.join_cancel_button.clicked()
400
401 self.assertEqual(len(self._called), 1)
402@@ -1526,7 +1598,7 @@
403 def test_user_cancelation_even_if_prior_login_error(self):
404 """Only one signal is sent with the final outcome."""
405 self.click_connect_with_valid_data()
406- self.ui.on_login_error(app_name=APP_NAME, error=ERROR)
407+ self.ui.on_login_error(app_name=APP_NAME, error=self.ERROR)
408 self.ui.login_cancel_button.clicked()
409
410 self.assertEqual(len(self._called), 1)
411
412=== modified file 'ubuntu_sso/tests/test_main.py'
413--- ubuntu_sso/tests/test_main.py 2010-08-26 21:04:47 +0000
414+++ ubuntu_sso/tests/test_main.py 2010-08-26 22:54:41 +0000
415@@ -54,6 +54,7 @@
416 "Can't reset password for this account"
417 RESET_TOKEN_INVALID_CONTENT = "AuthToken matching query does not exist."
418 EMAIL = 'test@example.com'
419+EMAIL_ALREADY_REGISTERED = 'a@example.com'
420 EMAIL_TOKEN = 'B2Pgtf'
421 HELP = 'help text'
422 PASSWORD = 'be4tiFul'
423@@ -102,7 +103,10 @@
424
425 def register(self, email, password, captcha_id, captcha_solution):
426 """Fake registration. Return a fix result."""
427- if captcha_id is None and captcha_solution is None:
428+ if email == EMAIL_ALREADY_REGISTERED:
429+ return {'status': 'error',
430+ 'errors': {'email': 'Email already registered'}}
431+ elif captcha_id is None and captcha_solution is None:
432 return STATUS_UNKNOWN
433 elif captcha_id != CAPTCHA_ID or captcha_solution != CAPTCHA_SOLUTION:
434 return STATUS_ERROR
435@@ -146,7 +150,10 @@
436 """Fake the email validation. Return a fix result."""
437 if email_token is None:
438 return STATUS_EMAIL_UNKNOWN
439- if email_token != EMAIL_TOKEN:
440+ elif email_token == EMAIL_ALREADY_REGISTERED:
441+ return {'status': 'error',
442+ 'errors': {'email': 'Email already registered'}}
443+ elif email_token != EMAIL_TOKEN:
444 return STATUS_EMAIL_ERROR
445 else:
446 return STATUS_EMAIL_OK
447@@ -234,6 +241,16 @@
448 self.assertIn(k, STATUS_ERROR['errors'])
449 self.assertEqual(v, "\n".join(STATUS_ERROR['errors'][k]))
450
451+ def test_register_user_if_status_error_with_string_message(self):
452+ """Proper error is raised if register fails."""
453+ self.register_kwargs['email'] = EMAIL_ALREADY_REGISTERED
454+ failure = self.assertRaises(RegistrationError,
455+ self.processor.register_user,
456+ **self.register_kwargs)
457+ for k, v in failure.args[0].items():
458+ self.assertIn(k, {'email': 'Email already registered'})
459+ self.assertEqual(v, 'Email already registered')
460+
461 def test_register_user_if_status_unknown(self):
462 """Proper error is raised if register returns an unknown status."""
463 self.register_kwargs['captcha_id'] = None
464@@ -274,6 +291,16 @@
465 self.assertIn(k, STATUS_EMAIL_ERROR['errors'])
466 self.assertEqual(v, "\n".join(STATUS_EMAIL_ERROR['errors'][k]))
467
468+ def test_validate_email_if_status_error_with_string_message(self):
469+ """Proper error is raised if register fails."""
470+ self.login_kwargs['email_token'] = EMAIL_ALREADY_REGISTERED
471+ failure = self.assertRaises(EmailTokenError,
472+ self.processor.validate_email,
473+ **self.login_kwargs)
474+ for k, v in failure.args[0].items():
475+ self.assertIn(k, {'email': 'Email already registered'})
476+ self.assertEqual(v, 'Email already registered')
477+
478 def test_validate_email_if_status_unknown(self):
479 """Proper error is raised if email validation returns unknown."""
480 self.login_kwargs['email_token'] = None
481@@ -797,7 +824,6 @@
482 e = TestExceptToErrdictException(*sample_args)
483 result = except_to_errdict(e)
484 self.assertEqual(result["errtype"], e.__class__.__name__)
485- self.assertEqual(result["errargs"], repr(sample_args))
486
487
488 class KeyringCredentialsTestCase(MockerTestCase):

Subscribers

People subscribed via source and target branches