Canonical SSO provider

Merge lp:~cprov/canonical-identity-provider/username-validation-ng into lp:canonical-identity-provider/release

  1. username-validation-ng
  2. Merge into trunk
Proposed by Celso Providelo
Status: Merged
Approved by: Celso Providelo
Approved revision: no longer in the source branch.
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~cprov/canonical-identity-provider/username-validation-ng
Merge into: lp:canonical-identity-provider/release
Diff against target: 285 lines (+49/-31)
7 files modified
src/api/v20/handlers.py (+2/-2)
src/api/v20/tests/test_handlers.py (+3/-3)
src/api/v20/tests/test_registration.py (+4/-4)
src/identityprovider/forms.py (+26/-10)
src/identityprovider/tests/test_forms.py (+7/-8)
src/webui/tests/test_views_account.py (+2/-2)
src/webui/tests/test_views_registration.py (+5/-2)
To merge this branch: bzr merge lp:~cprov/canonical-identity-provider/username-validation-ng
Reviewer Review Type Date Requested Status
Daniel Manrique (community) Approve
Review via email: mp+367193@code.launchpad.net

Commit message

Consolidating username validation with LP.

Description of the change

Moving away from the overloaded regexp to a more flexible `valid_username()` function matching LP:

https://code.launchpad.net/~cprov/launchpad/cleaner-usernames/+merge/367101

The new checker fixes the problem with arbitrary number of dashes ('x...x-x') and introduces a way to block bad patterns in a cleaner way (than the mysterious look-ahead).

To post a comment you must log in.
Revision history for this message
Daniel Manrique (roadmr) wrote :

LGTM, I like the encapsulation/simplification of validation.

See a couple of optionals inline.

review: Approve
Revision history for this message
Celso Providelo (cprov) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/api/v20/handlers.py'
2--- src/api/v20/handlers.py 2019-04-11 20:08:26 +0000
3+++ src/api/v20/handlers.py 2019-05-09 19:12:12 +0000
4@@ -35,8 +35,8 @@
5 from identityprovider import auth, emailutils
6 from identityprovider.fields import FIELD_REQUIRED
7 from identityprovider.forms import (
8- VALID_NAME_RE,
9 AccountManageForm,
10+ valid_username,
11 )
12 from identityprovider.login import (
13 AccountDeactivated,
14@@ -213,7 +213,7 @@
15 if account is None:
16 return errors.RESOURCE_NOT_FOUND()
17
18- valid = bool(VALID_NAME_RE.match(account.username))
19+ valid = valid_username(account.username)
20
21 identifier = urlparse.urljoin(
22 settings.SSO_ROOT_URL, account.get_absolute_url())
23
24=== modified file 'src/api/v20/tests/test_handlers.py'
25--- src/api/v20/tests/test_handlers.py 2019-04-11 20:08:26 +0000
26+++ src/api/v20/tests/test_handlers.py 2019-05-09 19:12:12 +0000
27@@ -28,7 +28,7 @@
28 from api.v20 import handlers, whitelist
29 from api.v20.utils import install_error_format_checker
30 from identityprovider.auth import build_discharge_macaroon
31-from identityprovider.forms import INVALID_NAME
32+from identityprovider.forms import INVALID_USERNAME_MSG
33 from identityprovider.login import PasswordPolicyError
34 from identityprovider.models import (
35 Account,
36@@ -275,7 +275,7 @@
37 {'username': 'foo.bar'}, token=self.super_token, status_code=400)
38 self.assertEqual(body['message'], 'Invalid request data')
39 self.assertEqual(body['code'], 'INVALID_DATA')
40- self.assertEqual(body['extra'], {'username': [INVALID_NAME]})
41+ self.assertEqual(body['extra'], {'username': [INVALID_USERNAME_MSG]})
42
43 @override_settings(LP_API_URL='test.com')
44 def test_patch_username_valid(self):
45@@ -927,7 +927,7 @@
46 {'username': 'foo.bar'}, token=self.super_token, status_code=400)
47 self.assertEqual(body['message'], 'Invalid request data')
48 self.assertEqual(body['code'], 'INVALID_DATA')
49- self.assertEqual(body['extra'], {'username': [INVALID_NAME]})
50+ self.assertEqual(body['extra'], {'username': [INVALID_USERNAME_MSG]})
51
52 @override_settings(LP_API_URL='test.com')
53 def test_patch_username_valid(self):
54
55=== modified file 'src/api/v20/tests/test_registration.py'
56--- src/api/v20/tests/test_registration.py 2018-08-17 18:38:29 +0000
57+++ src/api/v20/tests/test_registration.py 2019-05-09 19:12:12 +0000
58@@ -7,7 +7,7 @@
59
60 from api.v20 import registration
61 from identityprovider.fields import FIELD_REQUIRED, INVALID_EMAIL
62-from identityprovider.forms import INVALID_NAME
63+from identityprovider.forms import INVALID_USERNAME_MSG
64 from identityprovider.login import PasswordPolicyError
65 from identityprovider.models import Account
66 from identityprovider.models.const import (
67@@ -272,7 +272,7 @@
68 self.sso_root_url, email=self.email, password=None,
69 displayname='displayname', username=username)
70 e = ctx.exception
71- self.assertEqual(e.message_dict['username'], [INVALID_NAME])
72+ self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
73 # Make sure no account is created.
74 assert not Account.objects.exists()
75
76@@ -348,7 +348,7 @@
77 e = ctx.exception
78 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])
79 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])
80- self.assertEqual(e.message_dict['username'], [INVALID_NAME])
81+ self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
82 # Make sure no account is created.
83 assert not Account.objects.exists()
84
85@@ -378,7 +378,7 @@
86 e = ctx.exception
87 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])
88 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])
89- self.assertEqual(e.message_dict['username'], [INVALID_NAME])
90+ self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
91 self.assertEqual(e.message_dict['password'], [FIELD_REQUIRED])
92 # Make sure no account is created.
93 assert not Account.objects.exists()
94
95=== modified file 'src/identityprovider/forms.py'
96--- src/identityprovider/forms.py 2019-04-11 03:38:11 +0000
97+++ src/identityprovider/forms.py 2019-05-09 19:12:12 +0000
98@@ -54,13 +54,29 @@
99 ),
100 }
101
102-# Should match SCA ``short_namespace_re` expression.
103-VALID_NAME_RE = re.compile(r"(?!^[\d-]+$)^[a-z\d](-?[a-z\d]){2,31}$")
104-
105-INVALID_NAME = _(
106+# Username validation should match SCA and LP.
107+INVALID_USERNAME_MSG = _(
108 'Usernames must be 3 to 32 characters long, using lower-case letters or '
109 'digits and the character - can be used as a separator. Numbers-only '
110 'terms and consecutive separators are not allowed.')
111+USERNAME_VALID_PATTERN = re.compile(r"^[a-z0-9](-?[a-z0-9])+$")
112+# Block digits-and-dashes-only terms.
113+USERNAME_BLOCKED_PATTERN = re.compile(r"^[0-9-]+$")
114+
115+
116+def valid_username(username):
117+ """Return True if the username is valid, otherwise False."""
118+ length = len(username)
119+ if length < 3 or length > 32:
120+ return False
121+
122+ if not USERNAME_VALID_PATTERN.match(username):
123+ return False
124+
125+ if USERNAME_BLOCKED_PATTERN.match(username):
126+ return False
127+
128+ return True
129
130
131 class DeleteAccountForm(forms.Form):
132@@ -206,8 +222,8 @@
133 username = self.cleaned_data['username'].strip()
134 if not username:
135 return username
136- if not VALID_NAME_RE.match(username):
137- raise forms.ValidationError(INVALID_NAME)
138+ if not valid_username(username):
139+ raise forms.ValidationError(INVALID_USERNAME_MSG)
140
141 # Check with LP if the `username` can be used to continue with the
142 # registration. We don't have the openid_identifier for this
143@@ -342,8 +358,8 @@
144 # are now considered "distasteful".
145 if username == self.instance.person_name:
146 return username
147- if not VALID_NAME_RE.match(username):
148- raise forms.ValidationError(INVALID_NAME)
149+ if not valid_username(username):
150+ raise forms.ValidationError(INVALID_USERNAME_MSG)
151 if username and username != self.instance.person_name:
152 # The username is going to change, so ask LP to do a dry
153 # run to see if it will fail.
154@@ -675,8 +691,8 @@
155 username = self.cleaned_data['username'].strip()
156 if not username:
157 return ''
158- if not VALID_NAME_RE.match(username):
159- raise forms.ValidationError(INVALID_NAME)
160+ if not valid_username(username):
161+ raise forms.ValidationError(INVALID_USERNAME_MSG)
162 return username
163
164 def clean_status(self):
165
166=== modified file 'src/identityprovider/tests/test_forms.py'
167--- src/identityprovider/tests/test_forms.py 2019-04-10 13:24:11 +0000
168+++ src/identityprovider/tests/test_forms.py 2019-05-09 19:12:12 +0000
169@@ -35,8 +35,7 @@
170 PreferredEmailField,
171 )
172 from identityprovider.forms import (
173- INVALID_NAME,
174- VALID_NAME_RE,
175+ INVALID_USERNAME_MSG,
176 AccountManageForm,
177 DeviceRenameForm,
178 EditAccountForm,
179@@ -52,6 +51,7 @@
180 TwoFactorSyncForm,
181 UserAttribsRequestForm,
182 tos_error,
183+ valid_username,
184 )
185 from identityprovider.models import Account, AccountPassword, OpenIDRPConfig
186 from identityprovider.models.const import AccountStatus, EmailStatus
187@@ -69,6 +69,7 @@
188 # Or chars and digits
189 'f00',
190 # One or more dashes followed by at least one chars or digits.
191+ 'a-a',
192 'fo-o'
193 'f-oo',
194 'foo-1',
195@@ -79,7 +80,7 @@
196 ]
197 for n in names:
198 self.assertTrue(
199- VALID_NAME_RE.match(n), '%r is unexpectedly invalid.' % n)
200+ valid_username(n), '%r is unexpectedly invalid.' % n)
201
202 def test_invalid_names(self):
203 names = [
204@@ -92,8 +93,6 @@
205 '111',
206 # No dashes-only.
207 '---',
208- # No single-char separator
209- 'a-a',
210 # No consecutive dashes.
211 'a--a',
212 # No digit+hyphen-only.
213@@ -110,7 +109,7 @@
214 ]
215 for n in names:
216 self.assertFalse(
217- VALID_NAME_RE.match(n), '%r is unexpectedly valid.' % n)
218+ valid_username(n), '%r is unexpectedly valid.' % n)
219
220
221 class FormBaseTestCase(SSOBaseTestCase):
222@@ -445,7 +444,7 @@
223 form = self.get_form(self.data, require_username=True)
224
225 self.assertFalse(form.is_valid())
226- self.assertEqual(form.errors, {'username': [INVALID_NAME]})
227+ self.assertEqual(form.errors, {'username': [INVALID_USERNAME_MSG]})
228
229 @switches(USERNAME_UI=True)
230 def test_username_not_required_for_api_when_feature_flag_enabled(self):
231@@ -1417,7 +1416,7 @@
232 data = {'username': 'no+pluses'}
233 form = self.get_form(data=data)
234 self.assertFalse(form.is_valid())
235- self.assertEqual(form.errors['username'], [INVALID_NAME])
236+ self.assertEqual(form.errors['username'], [INVALID_USERNAME_MSG])
237
238
239 class MacaroonRequestFormTestCase(SSOBaseTestCase):
240
241=== modified file 'src/webui/tests/test_views_account.py'
242--- src/webui/tests/test_views_account.py 2019-04-02 21:18:12 +0000
243+++ src/webui/tests/test_views_account.py 2019-05-09 19:12:12 +0000
244@@ -20,7 +20,7 @@
245 from pyquery import PyQuery
246
247 from identityprovider.const import SESSION_TOKEN_KEY, SESSION_TOKEN_NAME
248-from identityprovider.forms import INVALID_NAME
249+from identityprovider.forms import INVALID_USERNAME_MSG
250 from identityprovider.models import (
251 Account,
252 AuthToken,
253@@ -404,7 +404,7 @@
254 self.assertEqual(r.status_code, 200)
255 self.assertIsNone(self.account.person)
256 self.assertContains(r, 'Usernames must be')
257- self.assertContains(r, INVALID_NAME)
258+ self.assertContains(r, INVALID_USERNAME_MSG)
259
260 @switches(USERNAME_UI=True)
261 def test_edit_username_preserves_bad_usernames(self):
262
263=== modified file 'src/webui/tests/test_views_registration.py'
264--- src/webui/tests/test_views_registration.py 2019-01-15 17:32:45 +0000
265+++ src/webui/tests/test_views_registration.py 2019-05-09 19:12:12 +0000
266@@ -22,7 +22,10 @@
267 INVALID_EMAIL,
268 ONE_TIME_PWD_ERROR,
269 )
270-from identityprovider.forms import INVALID_NAME, TwoFactorSyncForm
271+from identityprovider.forms import (
272+ INVALID_USERNAME_MSG,
273+ TwoFactorSyncForm,
274+)
275 from identityprovider.models import Account
276 from identityprovider.models.twofactor import (
277 TWOFACTOR_LOGIN,
278@@ -290,7 +293,7 @@
279 self.TESTDATA.pop('username')
280
281 self.assertFalse(response.context_data['form'].is_valid())
282- error = {'username': [INVALID_NAME]}
283+ error = {'username': [INVALID_USERNAME_MSG]}
284 self.assertEqual(error, response.context_data['form'].errors)
285 self.assertIsNone(response.context_data['form'].clean_username())
286