Canonical SSO provider

Merge lp:~matiasb/canonical-identity-provider/updated-username-error-msg into lp:canonical-identity-provider/release

  1. updated-username-error-msg
  2. Merge into trunk
Proposed by Matias Bordese
Status: Merged
Approved by: Matias Bordese
Approved revision: no longer in the source branch.
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~matiasb/canonical-identity-provider/updated-username-error-msg
Merge into: lp:canonical-identity-provider/release
Diff against target: 208 lines (+38/-22)
6 files modified
src/api/v20/tests/test_handlers.py (+8/-4)
src/api/v20/tests/test_registration.py (+10/-5)
src/identityprovider/forms.py (+7/-6)
src/identityprovider/tests/test_forms.py (+6/-3)
src/webui/tests/test_views_account.py (+4/-2)
src/webui/tests/test_views_registration.py (+3/-2)
To merge this branch: bzr merge lp:~matiasb/canonical-identity-provider/updated-username-error-msg
Reviewer Review Type Date Requested Status
Daniel Manrique (community) Approve
Maximiliano Bertacchini Approve
Review via email: mp+369005@code.launchpad.net

Commit message

Updated invalid username error message in account registration/edit form.

Description of the change

FWIW, as can be seen in the screenshots attached to the bug, besides the error message the field in error is already highlighted in red. This adds extra information in the message to help identify the field.
Changing the UX (to include labels or sth else) should probably go through the web team, who defined the current login/register form.

To post a comment you must log in.
Revision history for this message
Maximiliano Bertacchini (maxiberta) wrote :

Looks good to me.

A quick grep for INVALID_USERNAME_MSG shows there are no unformatted strings. Also, locally ran and checked for a simple code injection attack with username "<script>alert('Injected!');</script>".

review: Approve
Revision history for this message
Daniel Manrique (roadmr) wrote :

LGTM! Yes, this solution works and is less invasive than adding names to the fields. FWIW I'm not entirely sure the web team defined the current form, based on the tsunami of bugs filed by MPT on the entire registration process; and in any case, they will have a look at SSO this year, so any changes we would make here would be mostly temporary. But this solution works; I expect users will notice "ahh, this is where I typed that string". Time (and a hopefully reduced number of reports) will tell :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/api/v20/tests/test_handlers.py'
2--- src/api/v20/tests/test_handlers.py 2019-05-16 12:19:20 +0000
3+++ src/api/v20/tests/test_handlers.py 2019-06-18 20:24:38 +0000
4@@ -271,11 +271,13 @@
5 self.assertEqual(response.get('code', ''), 'INVALID_DATA')
6
7 def test_patch_username_invalid(self):
8+ username = 'foo.bar'
9 body = self.do_patch(
10- {'username': 'foo.bar'}, token=self.super_token, status_code=400)
11+ {'username': username}, token=self.super_token, status_code=400)
12 self.assertEqual(body['message'], 'Invalid request data')
13 self.assertEqual(body['code'], 'INVALID_DATA')
14- self.assertEqual(body['extra'], {'username': [INVALID_USERNAME_MSG]})
15+ self.assertEqual(
16+ body['extra'], {'username': [INVALID_USERNAME_MSG % username]})
17
18 @override_settings(LP_API_URL='test.com')
19 def test_patch_username_valid(self):
20@@ -923,11 +925,13 @@
21 self.assertEqual(response.get('code', ''), 'INVALID_DATA')
22
23 def test_patch_username_invalid(self):
24+ username = 'foo.bar'
25 body = self.do_patch(
26- {'username': 'foo.bar'}, token=self.super_token, status_code=400)
27+ {'username': username}, token=self.super_token, status_code=400)
28 self.assertEqual(body['message'], 'Invalid request data')
29 self.assertEqual(body['code'], 'INVALID_DATA')
30- self.assertEqual(body['extra'], {'username': [INVALID_USERNAME_MSG]})
31+ self.assertEqual(
32+ body['extra'], {'username': [INVALID_USERNAME_MSG % username]})
33
34 @override_settings(LP_API_URL='test.com')
35 def test_patch_username_valid(self):
36
37=== modified file 'src/api/v20/tests/test_registration.py'
38--- src/api/v20/tests/test_registration.py 2019-05-09 13:24:37 +0000
39+++ src/api/v20/tests/test_registration.py 2019-06-18 20:24:38 +0000
40@@ -272,7 +272,8 @@
41 self.sso_root_url, email=self.email, password=None,
42 displayname='displayname', username=username)
43 e = ctx.exception
44- self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
45+ self.assertEqual(
46+ e.message_dict['username'], [INVALID_USERNAME_MSG % username])
47 # Make sure no account is created.
48 assert not Account.objects.exists()
49
50@@ -341,14 +342,16 @@
51
52 @switches(USERNAME_UI=True)
53 def test_form_validation_with_valid_pw_with_feature_flag_enabled(self):
54+ username = 'InvalidUsername'
55 with self.assertRaises(ValidationError) as ctx:
56 registration.register(
57 self.sso_root_url, email='bad.email', password='TheSecret1',
58- displayname=None, username='InvalidUsername')
59+ displayname=None, username=username)
60 e = ctx.exception
61 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])
62 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])
63- self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
64+ self.assertEqual(
65+ e.message_dict['username'], [INVALID_USERNAME_MSG % username])
66 # Make sure no account is created.
67 assert not Account.objects.exists()
68
69@@ -370,15 +373,17 @@
70
71 @switches(USERNAME_UI=True)
72 def test_form_validation_with_no_pw_and_with_feature_flag_enabled(self):
73+ username = 'InvalidUsername'
74 with self.assertRaises(ValidationError) as ctx:
75 registration.register(
76 self.sso_root_url, email='bad.email', password=None,
77- displayname=None, username='InvalidUsername')
78+ displayname=None, username=username)
79
80 e = ctx.exception
81 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])
82 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])
83- self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
84+ self.assertEqual(
85+ e.message_dict['username'], [INVALID_USERNAME_MSG % username])
86 self.assertEqual(e.message_dict['password'], [FIELD_REQUIRED])
87 # Make sure no account is created.
88 assert not Account.objects.exists()
89
90=== modified file 'src/identityprovider/forms.py'
91--- src/identityprovider/forms.py 2019-05-09 19:11:07 +0000
92+++ src/identityprovider/forms.py 2019-06-18 20:24:38 +0000
93@@ -56,9 +56,10 @@
94
95 # Username validation should match SCA and LP.
96 INVALID_USERNAME_MSG = _(
97- 'Usernames must be 3 to 32 characters long, using lower-case letters or '
98- 'digits and the character - can be used as a separator. Numbers-only '
99- 'terms and consecutive separators are not allowed.')
100+ "'%s' is not a valid username. Usernames must be 3 to 32 characters long, "
101+ "using lower-case letters or digits and the character - can be used as a "
102+ "separator. Numbers-only terms and consecutive separators are not allowed."
103+)
104 USERNAME_VALID_PATTERN = re.compile(r"^[a-z0-9](-?[a-z0-9])+$")
105 # Block digits-and-dashes-only terms.
106 USERNAME_BLOCKED_PATTERN = re.compile(r"^[0-9-]+$")
107@@ -223,7 +224,7 @@
108 if not username:
109 return username
110 if not valid_username(username):
111- raise forms.ValidationError(INVALID_USERNAME_MSG)
112+ raise forms.ValidationError(INVALID_USERNAME_MSG % username)
113
114 # Check with LP if the `username` can be used to continue with the
115 # registration. We don't have the openid_identifier for this
116@@ -359,7 +360,7 @@
117 if username == self.instance.person_name:
118 return username
119 if not valid_username(username):
120- raise forms.ValidationError(INVALID_USERNAME_MSG)
121+ raise forms.ValidationError(INVALID_USERNAME_MSG % username)
122 if username and username != self.instance.person_name:
123 # The username is going to change, so ask LP to do a dry
124 # run to see if it will fail.
125@@ -692,7 +693,7 @@
126 if not username:
127 return ''
128 if not valid_username(username):
129- raise forms.ValidationError(INVALID_USERNAME_MSG)
130+ raise forms.ValidationError(INVALID_USERNAME_MSG % username)
131 return username
132
133 def clean_status(self):
134
135=== modified file 'src/identityprovider/tests/test_forms.py'
136--- src/identityprovider/tests/test_forms.py 2019-05-09 13:24:37 +0000
137+++ src/identityprovider/tests/test_forms.py 2019-06-18 20:24:38 +0000
138@@ -444,7 +444,8 @@
139 form = self.get_form(self.data, require_username=True)
140
141 self.assertFalse(form.is_valid())
142- self.assertEqual(form.errors, {'username': [INVALID_USERNAME_MSG]})
143+ self.assertEqual(
144+ form.errors, {'username': [INVALID_USERNAME_MSG % username]})
145
146 @switches(USERNAME_UI=True)
147 def test_username_not_required_for_api_when_feature_flag_enabled(self):
148@@ -1413,10 +1414,12 @@
149 self.assertTrue(form.is_valid())
150
151 def test_username_field_invalid(self):
152- data = {'username': 'no+pluses'}
153+ username = 'no+pluses'
154+ data = {'username': username}
155 form = self.get_form(data=data)
156 self.assertFalse(form.is_valid())
157- self.assertEqual(form.errors['username'], [INVALID_USERNAME_MSG])
158+ self.assertEqual(
159+ form.errors['username'], [INVALID_USERNAME_MSG % username])
160
161
162 class MacaroonRequestFormTestCase(SSOBaseTestCase):
163
164=== modified file 'src/webui/tests/test_views_account.py'
165--- src/webui/tests/test_views_account.py 2019-05-09 13:24:37 +0000
166+++ src/webui/tests/test_views_account.py 2019-06-18 20:24:38 +0000
167@@ -16,6 +16,7 @@
168 from django.test.utils import override_settings
169 from django.urls import reverse
170 from django.urls.exceptions import NoReverseMatch
171+from django.utils.html import escape
172 from gargoyle.testutils import switches
173 from pyquery import PyQuery
174
175@@ -400,11 +401,12 @@
176
177 @switches(USERNAME_UI=True)
178 def test_index_edit_username_errors_if_invalid(self):
179- r = self.post_username_change('BAD')
180+ username = 'BAD'
181+ r = self.post_username_change(username)
182 self.assertEqual(r.status_code, 200)
183 self.assertIsNone(self.account.person)
184 self.assertContains(r, 'Usernames must be')
185- self.assertContains(r, INVALID_USERNAME_MSG)
186+ self.assertContains(r, escape(INVALID_USERNAME_MSG % username))
187
188 @switches(USERNAME_UI=True)
189 def test_edit_username_preserves_bad_usernames(self):
190
191=== modified file 'src/webui/tests/test_views_registration.py'
192--- src/webui/tests/test_views_registration.py 2019-05-09 13:24:37 +0000
193+++ src/webui/tests/test_views_registration.py 2019-06-18 20:24:38 +0000
194@@ -288,12 +288,13 @@
195
196 @switches(USERNAME_UI=True)
197 def test_post_with_invalid_username_with_feature_flag_enabled(self):
198- self.TESTDATA['username'] = 'TheUserName'
199+ username = 'TheUserName'
200+ self.TESTDATA['username'] = username
201 response = self.post(**self.TESTDATA)
202 self.TESTDATA.pop('username')
203
204 self.assertFalse(response.context_data['form'].is_valid())
205- error = {'username': [INVALID_USERNAME_MSG]}
206+ error = {'username': [INVALID_USERNAME_MSG % username]}
207 self.assertEqual(error, response.context_data['form'].errors)
208 self.assertIsNone(response.context_data['form'].clean_username())
209