Merge lp:~matiasb/canonical-identity-provider/phoneid-as-emails into lp:canonical-identity-provider/release
- phoneid-as-emails
- Merge into trunk
Proposed by
Matias Bordese
Status: | Merged |
---|---|
Approved by: | Natalia Bidart |
Approved revision: | no longer in the source branch. |
Merged at revision: | 657 |
Proposed branch: | lp:~matiasb/canonical-identity-provider/phoneid-as-emails |
Merge into: | lp:canonical-identity-provider/release |
Diff against target: |
447 lines (+319/-1) 8 files modified
api/urls.py (+3/-0) api/v20/handlers.py (+64/-1) api/v20/tests/test_phone_login.py (+155/-0) identityprovider/login.py (+1/-0) identityprovider/models/emailaddress.py (+32/-0) identityprovider/tests/unit/test_models_emailaddress.py (+32/-0) webui/templates/account/emails.html (+2/-0) webui/tests/test_views_account.py (+30/-0) |
To merge this branch: | bzr merge lp:~matiasb/canonical-identity-provider/phoneid-as-emails |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Natalia Bidart (community) | Approve | ||
Review via email: mp+149055@code.launchpad.net |
Commit message
Added new login API (behind a flag) to allow login with a phone id.
Description of the change
Added new login API (behind a flag) to allow login with a phone id.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'api/urls.py' |
2 | --- api/urls.py 2013-02-04 11:13:04 +0000 |
3 | +++ api/urls.py 2013-02-18 16:43:22 +0000 |
4 | @@ -42,6 +42,7 @@ |
5 | handler=v20.AccountsHandler, authentication=ApiOAuthAuthentication()) |
6 | v2emails = Resource(handler=v20.EmailsHandler) |
7 | v2login = Resource(handler=v20.AccountLoginHandler) |
8 | +v2login_phone = Resource(handler=v20.AccountPhoneLoginHandler) |
9 | v2registration = Resource(handler=v20.AccountRegistrationHandler) |
10 | v2requests = Resource(handler=v20.RequestsHandler) |
11 | |
12 | @@ -78,6 +79,8 @@ |
13 | url(r'^v2/tokens$', v2login, name='api-login'), |
14 | url(r'^v2/accounts/(\w+)$', v2accounts, name='api-account'), |
15 | url(r'^v2/requests/validate$', v2requests, name='api-requests'), |
16 | + # login from phone, with a phone user id |
17 | + url(r'^v2/tokens/phone$', v2login_phone, name='api-login-phone'), |
18 | # temporarily hooked up so we can do reverse() |
19 | url(r'^v2/emails/(.*)$', v2emails, name='api-email'), |
20 | url(r'v2/tokens/(.*)$', v2login, name='api-token'), |
21 | |
22 | === modified file 'api/v20/handlers.py' |
23 | --- api/v20/handlers.py 2013-02-06 15:14:39 +0000 |
24 | +++ api/v20/handlers.py 2013-02-18 16:43:22 +0000 |
25 | @@ -20,7 +20,7 @@ |
26 | AuthenticationError, |
27 | authenticate_user, |
28 | ) |
29 | -from identityprovider.models import Account, twofactor |
30 | +from identityprovider.models import Account, EmailAddress, twofactor |
31 | from identityprovider.models.captcha import Captcha, VerifyCaptchaError |
32 | from identityprovider.store import SSODataStore |
33 | |
34 | @@ -137,6 +137,69 @@ |
35 | return response |
36 | |
37 | |
38 | +class AccountPhoneLoginHandler(BaseHandler): |
39 | + allowed_methods = ('POST', ) |
40 | + |
41 | + @require_mime('json') |
42 | + def create(self, request): |
43 | + if not gargoyle.is_active('LOGIN_BY_PHONE'): |
44 | + return errors.RESOURCE_NOT_FOUND() |
45 | + |
46 | + data = request.data |
47 | + # email should be available the first time, to bind account/phone |
48 | + # after that, get email from phoneid in the token name |
49 | + email = data.get('email') |
50 | + try: |
51 | + phone_id = data['phone_id'] |
52 | + password = data['password'] |
53 | + token_name = data['token_name'] |
54 | + except KeyError: |
55 | + expected = set(('phone_id', 'password', 'token_name')) |
56 | + missing = dict((k, 'field required') for k in expected - set(data)) |
57 | + return errors.INVALID_DATA(**missing) |
58 | + |
59 | + try: |
60 | + phone_email = EmailAddress.objects.get_from_phone_id(phone_id) |
61 | + login_email = phone_email.email |
62 | + except EmailAddress.DoesNotExist: |
63 | + phone_email = None |
64 | + login_email = email |
65 | + |
66 | + if login_email is None: |
67 | + return errors.INVALID_DATA(phone_id='invalid value') |
68 | + |
69 | + try: |
70 | + account = authenticate_user(login_email, password) |
71 | + except AccountSuspended: |
72 | + return errors.ACCOUNT_SUSPENDED() |
73 | + except AccountDeactivated: |
74 | + return errors.ACCOUNT_DEACTIVATED() |
75 | + except AuthenticationError: |
76 | + return errors.INVALID_CREDENTIALS() |
77 | + |
78 | + # TODO: here we should check for 2fa, avoiding for demo |
79 | + |
80 | + if phone_email is None: |
81 | + # user is authenticated through email, |
82 | + # adding phone id as email for future login |
83 | + EmailAddress.objects.create_from_phone_id(phone_id, account) |
84 | + |
85 | + token = account.get_or_create_oauth_token(token_name) |
86 | + response = rc.ALL_OK |
87 | + response.content = { |
88 | + "consumer_key": token.consumer.key, |
89 | + "consumer_secret": token.consumer.secret, |
90 | + "token_key": token.token, |
91 | + "token_secret": token.token_secret, |
92 | + "token_name": token_name, |
93 | + "date_created": token.created_at, |
94 | + "date_updated": token.updated_at, |
95 | + "href": reverse('api-token', args=(token.token,)), |
96 | + "openid": account.openid_identifier, |
97 | + } |
98 | + return response |
99 | + |
100 | + |
101 | class EmailsHandler(BaseHandler): |
102 | allowed_methods = ('GET',) |
103 | |
104 | |
105 | === added file 'api/v20/tests/test_phone_login.py' |
106 | --- api/v20/tests/test_phone_login.py 1970-01-01 00:00:00 +0000 |
107 | +++ api/v20/tests/test_phone_login.py 2013-02-18 16:43:22 +0000 |
108 | @@ -0,0 +1,155 @@ |
109 | + |
110 | +from django.core.urlresolvers import reverse |
111 | +from gargoyle.testutils import switches |
112 | +from mock import patch |
113 | + |
114 | +from identityprovider.login import ( |
115 | + AuthenticationError, |
116 | + AccountDeactivated, |
117 | + AccountSuspended, |
118 | +) |
119 | +from identityprovider.models.emailaddress import PHONE_EMAIL_DOMAIN |
120 | +from identityprovider.tests import DEFAULT_USER_PASSWORD |
121 | +from identityprovider.tests.utils import ( |
122 | + SSOBaseTestCase, |
123 | +) |
124 | + |
125 | +from api.v20 import handlers |
126 | +from api.v20.tests.utils import call |
127 | + |
128 | + |
129 | +handler = handlers.AccountPhoneLoginHandler() |
130 | +API_URL = reverse('api-login-phone') |
131 | +API_DATA = dict( |
132 | + email='foo@bar.com', phone_id='tel:+1234567890', |
133 | + password='foobar', token_name='token_name') |
134 | + |
135 | + |
136 | +class PhoneLoginHandlerTestCase(SSOBaseTestCase): |
137 | + fixtures = ['test'] |
138 | + |
139 | + def test_disabled_by_default(self): |
140 | + response, json_body = call(handler.create, API_URL, API_DATA) |
141 | + self.assertEqual(response.status_code, 404) |
142 | + |
143 | + @switches(LOGIN_BY_PHONE=True) |
144 | + def test_login_required_parameters(self): |
145 | + response, json_body = call(handler.create, API_URL, {}) |
146 | + self.assertEqual(response.status_code, 400) |
147 | + self.assertEqual(json_body, { |
148 | + 'code': 'INVALID_DATA', |
149 | + 'extra': { |
150 | + 'phone_id': 'field required', 'password': 'field required', |
151 | + 'token_name': 'field required' |
152 | + }, |
153 | + 'message': 'Invalid request data'}) |
154 | + |
155 | + @switches(LOGIN_BY_PHONE=True) |
156 | + def test_account_suspended(self): |
157 | + self.assert_failed_login('ACCOUNT_SUSPENDED', AccountSuspended) |
158 | + |
159 | + @switches(LOGIN_BY_PHONE=True) |
160 | + def test_account_deactivated(self): |
161 | + self.assert_failed_login('ACCOUNT_DEACTIVATED', AccountDeactivated) |
162 | + |
163 | + @switches(LOGIN_BY_PHONE=True) |
164 | + def test_failed_login(self): |
165 | + self.assert_failed_login('INVALID_CREDENTIALS', AuthenticationError) |
166 | + |
167 | + @switches(LOGIN_BY_PHONE=True) |
168 | + def assert_failed_login(self, code, exception): |
169 | + status_code = 403 |
170 | + if exception is AuthenticationError: |
171 | + status_code = 401 |
172 | + |
173 | + with patch('api.v20.handlers.authenticate_user') as mock_authenticate: |
174 | + mock_authenticate.side_effect = exception |
175 | + response, json_body = call(handler.create, API_URL, API_DATA) |
176 | + |
177 | + self.assertEqual(response.status_code, status_code) |
178 | + mock_authenticate.assert_called_once_with('foo@bar.com', 'foobar') |
179 | + self.assertEqual(json_body['code'], code) |
180 | + |
181 | + @switches(LOGIN_BY_PHONE=True) |
182 | + @switches(ALLOW_UNVALIDATED=True) |
183 | + def test_login_add_phone_email(self): |
184 | + account = self.factory.make_account() |
185 | + email = account.preferredemail |
186 | + # no unverified_emails |
187 | + assert len(account.unverified_emails()) == 0 |
188 | + |
189 | + data = {'phone_id': 'tel:+1234567890', 'email': email.email, |
190 | + 'password': DEFAULT_USER_PASSWORD, 'token_name': 'token_name'} |
191 | + response, json_body = call(handler.create, API_URL, data) |
192 | + |
193 | + self.assertEqual(response.status_code, 200) |
194 | + token = account.get_or_create_oauth_token('token_name') |
195 | + expected_response = { |
196 | + "consumer_key": token.consumer.key, |
197 | + "consumer_secret": token.consumer.secret, |
198 | + "token_key": token.token, |
199 | + "token_secret": token.token_secret, |
200 | + "token_name": 'token_name', |
201 | + "date_created": token.created_at, |
202 | + "href": reverse('api-token', args=(token.token,)), |
203 | + "openid": account.openid_identifier, |
204 | + } |
205 | + # ignore date_updated |
206 | + json_body.pop('date_updated') |
207 | + self.assertEqual(json_body, expected_response) |
208 | + # check added phone email |
209 | + unverified_emails = account.unverified_emails() |
210 | + self.assertEqual(len(unverified_emails), 1) |
211 | + phone_email = unverified_emails[0].email |
212 | + self.assertTrue(phone_email.endswith(PHONE_EMAIL_DOMAIN)) |
213 | + |
214 | + @switches(LOGIN_BY_PHONE=True) |
215 | + @switches(ALLOW_UNVALIDATED=True) |
216 | + def test_login_without_email_or_added_phone_email(self): |
217 | + account = self.factory.make_account() |
218 | + # no unverified_emails |
219 | + assert len(account.unverified_emails()) == 0 |
220 | + |
221 | + data = {'phone_id': 'tel:+1234567890', |
222 | + 'password': DEFAULT_USER_PASSWORD, 'token_name': 'token_name'} |
223 | + response, json_body = call(handler.create, API_URL, data) |
224 | + |
225 | + self.assertEqual(response.status_code, 400) |
226 | + self.assertEqual(json_body, { |
227 | + 'code': 'INVALID_DATA', |
228 | + 'extra': {'phone_id': 'invalid value'}, |
229 | + 'message': 'Invalid request data'}) |
230 | + |
231 | + @switches(LOGIN_BY_PHONE=True) |
232 | + @switches(ALLOW_UNVALIDATED=True) |
233 | + def test_login_with_phone_email(self): |
234 | + account = self.factory.make_account() |
235 | + email = account.preferredemail |
236 | + # no unverified_emails |
237 | + assert len(account.unverified_emails()) == 0 |
238 | + |
239 | + # login first with email and phone id |
240 | + data = {'phone_id': 'tel:+1234567890', 'email': email.email, |
241 | + 'password': DEFAULT_USER_PASSWORD, 'token_name': 'token_name'} |
242 | + response, json_body = call(handler.create, API_URL, data) |
243 | + assert response.status_code == 200 |
244 | + |
245 | + # now we should be able to login with phone id only |
246 | + data = {'phone_id': 'tel:+1234567890', |
247 | + 'password': DEFAULT_USER_PASSWORD, 'token_name': 'token_name'} |
248 | + response, json_body = call(handler.create, API_URL, data) |
249 | + self.assertEqual(response.status_code, 200) |
250 | + token = account.get_or_create_oauth_token('token_name') |
251 | + expected_response = { |
252 | + "consumer_key": token.consumer.key, |
253 | + "consumer_secret": token.consumer.secret, |
254 | + "token_key": token.token, |
255 | + "token_secret": token.token_secret, |
256 | + "token_name": 'token_name', |
257 | + "date_created": token.created_at, |
258 | + "href": reverse('api-token', args=(token.token,)), |
259 | + "openid": account.openid_identifier, |
260 | + } |
261 | + # ignore date_updated |
262 | + json_body.pop('date_updated') |
263 | + self.assertEqual(json_body, expected_response) |
264 | |
265 | === modified file 'identityprovider/login.py' |
266 | --- identityprovider/login.py 2013-01-03 11:41:12 +0000 |
267 | +++ identityprovider/login.py 2013-02-18 16:43:22 +0000 |
268 | @@ -43,6 +43,7 @@ |
269 | else: |
270 | raise AuthenticationError(_("Password didn't match.")) |
271 | |
272 | + # also check for LOGIN_BY_PHONE flag conditional to canonical users |
273 | if not gargoyle.is_active('ALLOW_UNVALIDATED'): |
274 | email_obj = account.emailaddress_set.get(email__iexact=email) |
275 | if email_obj.status == EmailStatus.NEW: |
276 | |
277 | === modified file 'identityprovider/models/emailaddress.py' |
278 | --- identityprovider/models/emailaddress.py 2013-01-25 20:39:28 +0000 |
279 | +++ identityprovider/models/emailaddress.py 2013-02-18 16:43:22 +0000 |
280 | @@ -3,6 +3,7 @@ |
281 | # LICENSE). |
282 | |
283 | import datetime |
284 | +import re |
285 | |
286 | from django.core.urlresolvers import reverse |
287 | from django.core.validators import validate_email |
288 | @@ -18,6 +19,31 @@ |
289 | ) |
290 | |
291 | |
292 | +PHONE_EMAIL_DOMAIN = 'phone.ubuntu' |
293 | +PHONE_EMAIL_INVALID_CHARS = re.compile(r"[^-!#$%&'*+/=?^_`{}|~0-9A-Z\.]", |
294 | + re.IGNORECASE) |
295 | + |
296 | + |
297 | +class EmailAddressManager(models.Manager): |
298 | + |
299 | + def _generate_email_from_phone_id(self, phone_id): |
300 | + # replace chars not validated by django validate_email by # |
301 | + email = '%s@%s' % (PHONE_EMAIL_INVALID_CHARS.sub('#', phone_id), |
302 | + PHONE_EMAIL_DOMAIN) |
303 | + return email |
304 | + |
305 | + def create_from_phone_id(self, phone_id, account): |
306 | + email = self._generate_email_from_phone_id(phone_id) |
307 | + email_address = EmailAddress.objects.create( |
308 | + email=email, account=account, status=EmailStatus.NEW) |
309 | + return email_address |
310 | + |
311 | + def get_from_phone_id(self, phone_id): |
312 | + email = self._generate_email_from_phone_id(phone_id) |
313 | + email_address = self.get(email=email) |
314 | + return email_address |
315 | + |
316 | + |
317 | class EmailAddress(models.Model): |
318 | email = models.TextField(validators=[validate_email]) |
319 | lp_person = models.IntegerField( |
320 | @@ -28,6 +54,8 @@ |
321 | account = models.ForeignKey( |
322 | Account, db_column='account', blank=True, null=True) |
323 | |
324 | + objects = EmailAddressManager() |
325 | + |
326 | class Meta: |
327 | app_label = 'identityprovider' |
328 | db_table = u'emailaddress' |
329 | @@ -39,6 +67,10 @@ |
330 | def is_preferred(self): |
331 | return self.status == EmailStatus.PREFERRED |
332 | |
333 | + def is_verifiable(self): |
334 | + suffix = '@%s' % PHONE_EMAIL_DOMAIN |
335 | + return not self.email.endswith(suffix) |
336 | + |
337 | def is_verified(self): |
338 | return self.status in (EmailStatus.VALIDATED, EmailStatus.PREFERRED) |
339 | |
340 | |
341 | === modified file 'identityprovider/tests/unit/test_models_emailaddress.py' |
342 | --- identityprovider/tests/unit/test_models_emailaddress.py 2013-01-25 21:32:21 +0000 |
343 | +++ identityprovider/tests/unit/test_models_emailaddress.py 2013-02-18 16:43:22 +0000 |
344 | @@ -2,9 +2,34 @@ |
345 | |
346 | from identityprovider.models import Account, EmailAddress |
347 | from identityprovider.models.const import EmailStatus |
348 | +from identityprovider.models.emailaddress import PHONE_EMAIL_DOMAIN |
349 | from identityprovider.tests.utils import SSOBaseTestCase |
350 | |
351 | |
352 | +class EmailAddressManagerTestCase(SSOBaseTestCase): |
353 | + fixtures = ['test'] |
354 | + |
355 | + def test_create_from_phone_id(self): |
356 | + account = Account.objects.get_by_email('test@canonical.com') |
357 | + new_email = EmailAddress.objects.create_from_phone_id('tel:+123', |
358 | + account) |
359 | + self.assertEqual(new_email.status, EmailStatus.NEW) |
360 | + self.assertEqual(new_email.account, account) |
361 | + self.assertEqual(new_email.email, 'tel#+123@%s' % PHONE_EMAIL_DOMAIN) |
362 | + |
363 | + def test_get_from_phone_id(self): |
364 | + account = Account.objects.get_by_email('test@canonical.com') |
365 | + new_email = EmailAddress.objects.create_from_phone_id('tel:+123', |
366 | + account) |
367 | + email = EmailAddress.objects.get_from_phone_id('tel:+123') |
368 | + self.assertEqual(email, new_email) |
369 | + |
370 | + def test_get_from_phone_id_not_exist(self): |
371 | + self.assertRaises( |
372 | + EmailAddress.DoesNotExist, |
373 | + EmailAddress.objects.get_from_phone_id, 'tel:+123') |
374 | + |
375 | + |
376 | class EmailAddressTestCase(SSOBaseTestCase): |
377 | fixtures = ['test'] |
378 | |
379 | @@ -13,6 +38,13 @@ |
380 | email = EmailAddress.objects.get(email='test@canonical.com') |
381 | self.assertEqual(account, email.account) |
382 | |
383 | + def test_emailaddress_is_verifiable(self): |
384 | + email = EmailAddress.objects.get(email='test@canonical.com') |
385 | + self.assertTrue(email.is_verifiable()) |
386 | + unverifiable_email = 'test@%s' % PHONE_EMAIL_DOMAIN |
387 | + email = EmailAddress(email=unverifiable_email) |
388 | + self.assertFalse(email.is_verifiable()) |
389 | + |
390 | def test_emailaddress_is_verified(self): |
391 | email = EmailAddress.objects.get(email='test@canonical.com') |
392 | assert email.status == EmailStatus.PREFERRED |
393 | |
394 | === modified file 'webui/templates/account/emails.html' |
395 | --- webui/templates/account/emails.html 2012-12-19 18:54:44 +0000 |
396 | +++ webui/templates/account/emails.html 2013-02-18 16:43:22 +0000 |
397 | @@ -69,7 +69,9 @@ |
398 | <td class="email">{{ email }}</td> |
399 | {% if not readonly %} |
400 | <td class="actions"> |
401 | + {% if email.is_verifiable %} |
402 | <a href="./+verify-email?id={{ email.id }}" class="btn-sm"><span>{% trans "Verify" %}</span></a> |
403 | + {% endif %} |
404 | <a href="./+remove-email?id={{ email.id }}" class="btn-sm"><span>{% trans "Delete" %}</span></a> |
405 | </td> |
406 | {% endif %} |
407 | |
408 | === modified file 'webui/tests/test_views_account.py' |
409 | --- webui/tests/test_views_account.py 2013-01-25 20:40:15 +0000 |
410 | +++ webui/tests/test_views_account.py 2013-02-18 16:43:22 +0000 |
411 | @@ -36,6 +36,36 @@ |
412 | from identityprovider.utils import validate_launchpad_password |
413 | |
414 | |
415 | +class AccountEmailsViewTestCase(AuthenticatedTestCase): |
416 | + |
417 | + def setUp(self): |
418 | + super(AccountEmailsViewTestCase, self).setUp() |
419 | + account = Account.objects.get(emailaddress__email=self.login_email) |
420 | + self.phone_email = EmailAddress.objects.create_from_phone_id( |
421 | + 'tel:+1234567890', account) |
422 | + |
423 | + def test_phone_id_email_is_not_verifiable(self): |
424 | + url = reverse('account-emails') |
425 | + response = self.client.get(url) |
426 | + tree = PyQuery(response.content) |
427 | + # unverified emails -> second listing table |
428 | + unverified_emails = tree.find('table.listing')[1] |
429 | + emails_td = PyQuery(unverified_emails).find('td.email') |
430 | + for email_td in emails_td: |
431 | + if email_td.text == self.phone_email.email: |
432 | + # get available actions |
433 | + actions = email_td.getnext().getchildren() |
434 | + # only delete option available, is not verifiable |
435 | + self.assertEqual(len(actions), 1) |
436 | + self.assertEqual(actions[0].get('href'), |
437 | + '.%s?id=%d' % (reverse('delete_email'), |
438 | + self.phone_email.id)) |
439 | + else: |
440 | + # get available actions: verify, remove |
441 | + actions = email_td.getnext().getchildren() |
442 | + self.assertEqual(len(actions), 2) |
443 | + |
444 | + |
445 | class AccountViewsUnauthenticatedTestCase(SSOBaseTestCase): |
446 | |
447 | def test_index_unauthenticated(self): |
Looks great, thanks!