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
=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py 2019-04-11 20:08:26 +0000
+++ src/api/v20/handlers.py 2019-05-09 19:12:12 +0000
@@ -35,8 +35,8 @@
35from identityprovider import auth, emailutils35from identityprovider import auth, emailutils
36from identityprovider.fields import FIELD_REQUIRED36from identityprovider.fields import FIELD_REQUIRED
37from identityprovider.forms import (37from identityprovider.forms import (
38 VALID_NAME_RE,
39 AccountManageForm,38 AccountManageForm,
39 valid_username,
40)40)
41from identityprovider.login import (41from identityprovider.login import (
42 AccountDeactivated,42 AccountDeactivated,
@@ -213,7 +213,7 @@
213 if account is None:213 if account is None:
214 return errors.RESOURCE_NOT_FOUND()214 return errors.RESOURCE_NOT_FOUND()
215215
216 valid = bool(VALID_NAME_RE.match(account.username))216 valid = valid_username(account.username)
217217
218 identifier = urlparse.urljoin(218 identifier = urlparse.urljoin(
219 settings.SSO_ROOT_URL, account.get_absolute_url())219 settings.SSO_ROOT_URL, account.get_absolute_url())
220220
=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py 2019-04-11 20:08:26 +0000
+++ src/api/v20/tests/test_handlers.py 2019-05-09 19:12:12 +0000
@@ -28,7 +28,7 @@
28from api.v20 import handlers, whitelist28from api.v20 import handlers, whitelist
29from api.v20.utils import install_error_format_checker29from api.v20.utils import install_error_format_checker
30from identityprovider.auth import build_discharge_macaroon30from identityprovider.auth import build_discharge_macaroon
31from identityprovider.forms import INVALID_NAME31from identityprovider.forms import INVALID_USERNAME_MSG
32from identityprovider.login import PasswordPolicyError32from identityprovider.login import PasswordPolicyError
33from identityprovider.models import (33from identityprovider.models import (
34 Account,34 Account,
@@ -275,7 +275,7 @@
275 {'username': 'foo.bar'}, token=self.super_token, status_code=400)275 {'username': 'foo.bar'}, token=self.super_token, status_code=400)
276 self.assertEqual(body['message'], 'Invalid request data')276 self.assertEqual(body['message'], 'Invalid request data')
277 self.assertEqual(body['code'], 'INVALID_DATA')277 self.assertEqual(body['code'], 'INVALID_DATA')
278 self.assertEqual(body['extra'], {'username': [INVALID_NAME]})278 self.assertEqual(body['extra'], {'username': [INVALID_USERNAME_MSG]})
279279
280 @override_settings(LP_API_URL='test.com')280 @override_settings(LP_API_URL='test.com')
281 def test_patch_username_valid(self):281 def test_patch_username_valid(self):
@@ -927,7 +927,7 @@
927 {'username': 'foo.bar'}, token=self.super_token, status_code=400)927 {'username': 'foo.bar'}, token=self.super_token, status_code=400)
928 self.assertEqual(body['message'], 'Invalid request data')928 self.assertEqual(body['message'], 'Invalid request data')
929 self.assertEqual(body['code'], 'INVALID_DATA')929 self.assertEqual(body['code'], 'INVALID_DATA')
930 self.assertEqual(body['extra'], {'username': [INVALID_NAME]})930 self.assertEqual(body['extra'], {'username': [INVALID_USERNAME_MSG]})
931931
932 @override_settings(LP_API_URL='test.com')932 @override_settings(LP_API_URL='test.com')
933 def test_patch_username_valid(self):933 def test_patch_username_valid(self):
934934
=== modified file 'src/api/v20/tests/test_registration.py'
--- src/api/v20/tests/test_registration.py 2018-08-17 18:38:29 +0000
+++ src/api/v20/tests/test_registration.py 2019-05-09 19:12:12 +0000
@@ -7,7 +7,7 @@
77
8from api.v20 import registration8from api.v20 import registration
9from identityprovider.fields import FIELD_REQUIRED, INVALID_EMAIL9from identityprovider.fields import FIELD_REQUIRED, INVALID_EMAIL
10from identityprovider.forms import INVALID_NAME10from identityprovider.forms import INVALID_USERNAME_MSG
11from identityprovider.login import PasswordPolicyError11from identityprovider.login import PasswordPolicyError
12from identityprovider.models import Account12from identityprovider.models import Account
13from identityprovider.models.const import (13from identityprovider.models.const import (
@@ -272,7 +272,7 @@
272 self.sso_root_url, email=self.email, password=None,272 self.sso_root_url, email=self.email, password=None,
273 displayname='displayname', username=username)273 displayname='displayname', username=username)
274 e = ctx.exception274 e = ctx.exception
275 self.assertEqual(e.message_dict['username'], [INVALID_NAME])275 self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
276 # Make sure no account is created.276 # Make sure no account is created.
277 assert not Account.objects.exists()277 assert not Account.objects.exists()
278278
@@ -348,7 +348,7 @@
348 e = ctx.exception348 e = ctx.exception
349 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])349 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])
350 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])350 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])
351 self.assertEqual(e.message_dict['username'], [INVALID_NAME])351 self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
352 # Make sure no account is created.352 # Make sure no account is created.
353 assert not Account.objects.exists()353 assert not Account.objects.exists()
354354
@@ -378,7 +378,7 @@
378 e = ctx.exception378 e = ctx.exception
379 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])379 self.assertEqual(e.message_dict['displayname'], [FIELD_REQUIRED])
380 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])380 self.assertEqual(e.message_dict['email'], [INVALID_EMAIL])
381 self.assertEqual(e.message_dict['username'], [INVALID_NAME])381 self.assertEqual(e.message_dict['username'], [INVALID_USERNAME_MSG])
382 self.assertEqual(e.message_dict['password'], [FIELD_REQUIRED])382 self.assertEqual(e.message_dict['password'], [FIELD_REQUIRED])
383 # Make sure no account is created.383 # Make sure no account is created.
384 assert not Account.objects.exists()384 assert not Account.objects.exists()
385385
=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py 2019-04-11 03:38:11 +0000
+++ src/identityprovider/forms.py 2019-05-09 19:12:12 +0000
@@ -54,13 +54,29 @@
54 ),54 ),
55}55}
5656
57# Should match SCA ``short_namespace_re` expression.57# Username validation should match SCA and LP.
58VALID_NAME_RE = re.compile(r"(?!^[\d-]+$)^[a-z\d](-?[a-z\d]){2,31}$")58INVALID_USERNAME_MSG = _(
59
60INVALID_NAME = _(
61 'Usernames must be 3 to 32 characters long, using lower-case letters or '59 'Usernames must be 3 to 32 characters long, using lower-case letters or '
62 'digits and the character - can be used as a separator. Numbers-only '60 'digits and the character - can be used as a separator. Numbers-only '
63 'terms and consecutive separators are not allowed.')61 'terms and consecutive separators are not allowed.')
62USERNAME_VALID_PATTERN = re.compile(r"^[a-z0-9](-?[a-z0-9])+$")
63# Block digits-and-dashes-only terms.
64USERNAME_BLOCKED_PATTERN = re.compile(r"^[0-9-]+$")
65
66
67def valid_username(username):
68 """Return True if the username is valid, otherwise False."""
69 length = len(username)
70 if length < 3 or length > 32:
71 return False
72
73 if not USERNAME_VALID_PATTERN.match(username):
74 return False
75
76 if USERNAME_BLOCKED_PATTERN.match(username):
77 return False
78
79 return True
6480
6581
66class DeleteAccountForm(forms.Form):82class DeleteAccountForm(forms.Form):
@@ -206,8 +222,8 @@
206 username = self.cleaned_data['username'].strip()222 username = self.cleaned_data['username'].strip()
207 if not username:223 if not username:
208 return username224 return username
209 if not VALID_NAME_RE.match(username):225 if not valid_username(username):
210 raise forms.ValidationError(INVALID_NAME)226 raise forms.ValidationError(INVALID_USERNAME_MSG)
211227
212 # Check with LP if the `username` can be used to continue with the228 # Check with LP if the `username` can be used to continue with the
213 # registration. We don't have the openid_identifier for this229 # registration. We don't have the openid_identifier for this
@@ -342,8 +358,8 @@
342 # are now considered "distasteful".358 # are now considered "distasteful".
343 if username == self.instance.person_name:359 if username == self.instance.person_name:
344 return username360 return username
345 if not VALID_NAME_RE.match(username):361 if not valid_username(username):
346 raise forms.ValidationError(INVALID_NAME)362 raise forms.ValidationError(INVALID_USERNAME_MSG)
347 if username and username != self.instance.person_name:363 if username and username != self.instance.person_name:
348 # The username is going to change, so ask LP to do a dry364 # The username is going to change, so ask LP to do a dry
349 # run to see if it will fail.365 # run to see if it will fail.
@@ -675,8 +691,8 @@
675 username = self.cleaned_data['username'].strip()691 username = self.cleaned_data['username'].strip()
676 if not username:692 if not username:
677 return ''693 return ''
678 if not VALID_NAME_RE.match(username):694 if not valid_username(username):
679 raise forms.ValidationError(INVALID_NAME)695 raise forms.ValidationError(INVALID_USERNAME_MSG)
680 return username696 return username
681697
682 def clean_status(self):698 def clean_status(self):
683699
=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py 2019-04-10 13:24:11 +0000
+++ src/identityprovider/tests/test_forms.py 2019-05-09 19:12:12 +0000
@@ -35,8 +35,7 @@
35 PreferredEmailField,35 PreferredEmailField,
36)36)
37from identityprovider.forms import (37from identityprovider.forms import (
38 INVALID_NAME,38 INVALID_USERNAME_MSG,
39 VALID_NAME_RE,
40 AccountManageForm,39 AccountManageForm,
41 DeviceRenameForm,40 DeviceRenameForm,
42 EditAccountForm,41 EditAccountForm,
@@ -52,6 +51,7 @@
52 TwoFactorSyncForm,51 TwoFactorSyncForm,
53 UserAttribsRequestForm,52 UserAttribsRequestForm,
54 tos_error,53 tos_error,
54 valid_username,
55)55)
56from identityprovider.models import Account, AccountPassword, OpenIDRPConfig56from identityprovider.models import Account, AccountPassword, OpenIDRPConfig
57from identityprovider.models.const import AccountStatus, EmailStatus57from identityprovider.models.const import AccountStatus, EmailStatus
@@ -69,6 +69,7 @@
69 # Or chars and digits69 # Or chars and digits
70 'f00',70 'f00',
71 # One or more dashes followed by at least one chars or digits.71 # One or more dashes followed by at least one chars or digits.
72 'a-a',
72 'fo-o'73 'fo-o'
73 'f-oo',74 'f-oo',
74 'foo-1',75 'foo-1',
@@ -79,7 +80,7 @@
79 ]80 ]
80 for n in names:81 for n in names:
81 self.assertTrue(82 self.assertTrue(
82 VALID_NAME_RE.match(n), '%r is unexpectedly invalid.' % n)83 valid_username(n), '%r is unexpectedly invalid.' % n)
8384
84 def test_invalid_names(self):85 def test_invalid_names(self):
85 names = [86 names = [
@@ -92,8 +93,6 @@
92 '111',93 '111',
93 # No dashes-only.94 # No dashes-only.
94 '---',95 '---',
95 # No single-char separator
96 'a-a',
97 # No consecutive dashes.96 # No consecutive dashes.
98 'a--a',97 'a--a',
99 # No digit+hyphen-only.98 # No digit+hyphen-only.
@@ -110,7 +109,7 @@
110 ]109 ]
111 for n in names:110 for n in names:
112 self.assertFalse(111 self.assertFalse(
113 VALID_NAME_RE.match(n), '%r is unexpectedly valid.' % n)112 valid_username(n), '%r is unexpectedly valid.' % n)
114113
115114
116class FormBaseTestCase(SSOBaseTestCase):115class FormBaseTestCase(SSOBaseTestCase):
@@ -445,7 +444,7 @@
445 form = self.get_form(self.data, require_username=True)444 form = self.get_form(self.data, require_username=True)
446445
447 self.assertFalse(form.is_valid())446 self.assertFalse(form.is_valid())
448 self.assertEqual(form.errors, {'username': [INVALID_NAME]})447 self.assertEqual(form.errors, {'username': [INVALID_USERNAME_MSG]})
449448
450 @switches(USERNAME_UI=True)449 @switches(USERNAME_UI=True)
451 def test_username_not_required_for_api_when_feature_flag_enabled(self):450 def test_username_not_required_for_api_when_feature_flag_enabled(self):
@@ -1417,7 +1416,7 @@
1417 data = {'username': 'no+pluses'}1416 data = {'username': 'no+pluses'}
1418 form = self.get_form(data=data)1417 form = self.get_form(data=data)
1419 self.assertFalse(form.is_valid())1418 self.assertFalse(form.is_valid())
1420 self.assertEqual(form.errors['username'], [INVALID_NAME])1419 self.assertEqual(form.errors['username'], [INVALID_USERNAME_MSG])
14211420
14221421
1423class MacaroonRequestFormTestCase(SSOBaseTestCase):1422class MacaroonRequestFormTestCase(SSOBaseTestCase):
14241423
=== modified file 'src/webui/tests/test_views_account.py'
--- src/webui/tests/test_views_account.py 2019-04-02 21:18:12 +0000
+++ src/webui/tests/test_views_account.py 2019-05-09 19:12:12 +0000
@@ -20,7 +20,7 @@
20from pyquery import PyQuery20from pyquery import PyQuery
2121
22from identityprovider.const import SESSION_TOKEN_KEY, SESSION_TOKEN_NAME22from identityprovider.const import SESSION_TOKEN_KEY, SESSION_TOKEN_NAME
23from identityprovider.forms import INVALID_NAME23from identityprovider.forms import INVALID_USERNAME_MSG
24from identityprovider.models import (24from identityprovider.models import (
25 Account,25 Account,
26 AuthToken,26 AuthToken,
@@ -404,7 +404,7 @@
404 self.assertEqual(r.status_code, 200)404 self.assertEqual(r.status_code, 200)
405 self.assertIsNone(self.account.person)405 self.assertIsNone(self.account.person)
406 self.assertContains(r, 'Usernames must be')406 self.assertContains(r, 'Usernames must be')
407 self.assertContains(r, INVALID_NAME)407 self.assertContains(r, INVALID_USERNAME_MSG)
408408
409 @switches(USERNAME_UI=True)409 @switches(USERNAME_UI=True)
410 def test_edit_username_preserves_bad_usernames(self):410 def test_edit_username_preserves_bad_usernames(self):
411411
=== modified file 'src/webui/tests/test_views_registration.py'
--- src/webui/tests/test_views_registration.py 2019-01-15 17:32:45 +0000
+++ src/webui/tests/test_views_registration.py 2019-05-09 19:12:12 +0000
@@ -22,7 +22,10 @@
22 INVALID_EMAIL,22 INVALID_EMAIL,
23 ONE_TIME_PWD_ERROR,23 ONE_TIME_PWD_ERROR,
24)24)
25from identityprovider.forms import INVALID_NAME, TwoFactorSyncForm25from identityprovider.forms import (
26 INVALID_USERNAME_MSG,
27 TwoFactorSyncForm,
28)
26from identityprovider.models import Account29from identityprovider.models import Account
27from identityprovider.models.twofactor import (30from identityprovider.models.twofactor import (
28 TWOFACTOR_LOGIN,31 TWOFACTOR_LOGIN,
@@ -290,7 +293,7 @@
290 self.TESTDATA.pop('username')293 self.TESTDATA.pop('username')
291294
292 self.assertFalse(response.context_data['form'].is_valid())295 self.assertFalse(response.context_data['form'].is_valid())
293 error = {'username': [INVALID_NAME]}296 error = {'username': [INVALID_USERNAME_MSG]}
294 self.assertEqual(error, response.context_data['form'].errors)297 self.assertEqual(error, response.context_data['form'].errors)
295 self.assertIsNone(response.context_data['form'].clean_username())298 self.assertIsNone(response.context_data['form'].clean_username())
296299