Merge lp:~matiasb/canonical-identity-provider/models-validation into lp:canonical-identity-provider/release

[Matias Bordese]

Refactored password and email validation when creating an account using (reusable) validators.

Refactored password and email validation when creating an account using (reusable) validators.
Updated forms/views requiring password validation.
Kept flexibility at the model level (by using set_password or create_account).

[Natalia Bidart (nataliabidart)]

Code looks good. This piece seems buggy to me, perhaps you can fix it and add a test for it as well?

108 - 'errors': [PASSWORD_POLICY_ERROR]
109 + 'errors': e.message

Also, could you please add a couple of more tests to PasswordPolicyValidatorTestCase?

review: Needs Fixing

[Matias Bordese (matiasb)]

> Code looks good. This piece seems buggy to me, perhaps you can fix it and add
> a test for it as well?
>
> 108 - 'errors': [PASSWORD_POLICY_ERROR]
> 109 + 'errors': e.message

Added test, using .messages returns a list (of possible multiple errors).

> Also, could you please add a couple of more tests to
> PasswordPolicyValidatorTestCase?

Added.

[Natalia Bidart (nataliabidart)]

Looks good, thanks.

review: Approve

=== modified file 'api/tests/test_handlers.py'
--- api/tests/test_handlers.py	2012-12-04 13:44:46 +0000
+++ api/tests/test_handlers.py	2012-12-13 20:21:20 +0000
@@ -27,6 +27,7 @@
     EmailStatus,
     LoginTokenType,
 )
+from identityprovider.validators import PASSWORD_POLICY_ERROR
 
 from api.tests.utils import (
     AnonAPITestCase,
@@ -51,8 +52,20 @@
         response = self.api.registrations.set_new_password(
             email='[e-mail]', token=authtoken.token,
             new_password='pass')
-
-        self.assertEqual(response['status'], "error")
+        self.assertEqual(response['status'], "error")
+
+    def test_set_new_password_invalid(self):
+        email_address = self.factory.make_email_address()
+        account = self.factory.make_account(email=email_address)
+        authtoken = AuthTokenFactory().new(account, email_address,
+                                           email_address,
+                                           LoginTokenType.PASSWORDRECOVERY,
+                                           None)
+
+        response = self.api.registrations.set_new_password(
+            email=email_address, token=authtoken.token, new_password='pass')
+        self.assertEqual(response['status'], "error")
+        self.assertEqual(response['errors'], [PASSWORD_POLICY_ERROR])
 
     def test_reset_token_when_email_is_invalid(self):
         try:
 
=== modified file 'api/v10/forms.py'
--- api/v10/forms.py	2012-12-03 16:51:35 +0000
+++ api/v10/forms.py	2012-12-13 20:21:20 +0000
@@ -7,18 +7,14 @@
 from django.forms import fields
 from django.utils.translation import ugettext as _
 
-from identityprovider.utils import password_policy_compliant
 from identityprovider.models.captcha import Captcha
-
-
-PASSWORD_POLICY_ERROR = _("Password must be at least "
-                          "8 characters long, and must contain at least one "
-                          "number and an upper case letter.")
+from identityprovider.validators import validate_password_policy
 
 
 class WebserviceCreateAccountForm(forms.Form):
     email = fields.EmailField()
-    password = fields.CharField(max_length=256)
+    password = fields.CharField(max_length=256,
+                                validators=[validate_password_policy])
     captcha_id = fields.CharField(max_length=1024)
     captcha_solution = fields.CharField(max_length=256)
     remote_ip = fields.CharField(max_length=256)
@@ -28,18 +24,6 @@
         empty_value='desktop', required=False)
     validate_redirect_to = fields.CharField(required=False)
 
-    def clean_password(self):
-        if 'password' in self.cleaned_data:
-            password = self.cleaned_data['password']
-            try:
-                str(password)
-            except UnicodeEncodeError:
-                raise forms.ValidationError(
-                    _("Invalid characters in password"))
-            if not password_policy_compliant(password):
-                raise forms.ValidationError(PASSWORD_POLICY_ERROR)
-            return password
-
     def clean_validate_redirect_to(self):
         validate_redirect_to = self.cleaned_data.get('validate_redirect_to')
         # return None instead of '' as the default value
 
=== modified file 'api/v10/handlers.py'
--- api/v10/handlers.py	2012-12-04 18:51:42 +0000
+++ api/v10/handlers.py	2012-12-13 20:21:20 +0000
@@ -4,6 +4,7 @@
 from datetime import datetime, timedelta
 
 from django.conf import settings
+from django.core.exceptions import ValidationError
 from django.core.serializers.json import DateTimeAwareJSONEncoder
 from django.http import (
     HttpResponseBadRequest,
@@ -36,10 +37,8 @@
     application_token_created,
     application_token_invalidated,
 )
-from identityprovider.utils import (
-    encrypt_launchpad_password,
-    password_policy_compliant,
-)
+from identityprovider.validators import validate_password_policy
+
 from oauth_backend.models import Token
 
 from api.v10.decorators import (
@@ -48,7 +47,6 @@
     named_operation,
 )
 from api.v10.forms import (
-    PASSWORD_POLICY_ERROR,
     WebserviceCreateAccountForm,
 )
 
@@ -199,15 +197,13 @@
 
         platform = cleaned_data['platform']
         redirection_url = cleaned_data['validate_redirect_to']
-        encrypted_password = encrypt_launchpad_password(
-            cleaned_data['password'])
+        password = cleaned_data['password']
         displayname = cleaned_data['displayname']
         email = cleaned_data['email']
         if platform in ['desktop', 'mobile']:
             account = Account.objects.create_account(
-                displayname, email,
-                encrypted_password, password_encrypted=True,
-                email_validated=False)
+                displayname, email, password, email_validated=False)
+            encrypted_password = account.encrypted_password
 
         if platform == 'desktop':
             token = AuthTokenFactory().new_api_email_validation_token(
@@ -275,15 +271,15 @@
                 'status': 'error',
                 'message': "Wrong token, request new one."
             }
-        if not password_policy_compliant(new_password):
+
+        try:
+            validate_password_policy(new_password)
+        except ValidationError, e:
             return {
                 'status': 'error',
-                'errors': [PASSWORD_POLICY_ERROR]
+                'errors': e.messages,
             }
-        password_obj = token.requester.accountpassword
-        password_obj.password = encrypt_launchpad_password(new_password)
-        password_obj.save()
-
+        token.requester.set_password(new_password)
         token.consume()
 
         return {
 
=== modified file 'identityprovider/forms.py'
--- identityprovider/forms.py	2012-10-30 12:15:29 +0000
+++ identityprovider/forms.py	2012-12-13 20:21:20 +0000
@@ -21,7 +21,9 @@
 from identityprovider.models.const import EmailStatus
 from identityprovider.utils import (
     encrypt_launchpad_password,
-    password_policy_compliant,
+)
+from identityprovider.validators import (
+    validate_password_policy,
 )
 from identityprovider.fields import OATHPasswordField
 from identityprovider.widgets import (
@@ -29,8 +31,6 @@
     ROAwareSelect,
 )
 
-PASSWORD_ERROR = _("Password must be at least 8 characters long, and must "
-                   "contain at least one number and an upper case letter.")
 
 logger = logging.getLogger('sso')
 
@@ -81,6 +81,7 @@
 class ResetPasswordForm(forms.Form):
     password = fields.CharField(
         error_messages=default_errors,
+        validators=[validate_password_policy],
         widget=widgets.PasswordInput(attrs={
             'class': 'textType',
             'size': '20',
@@ -94,18 +95,6 @@
         }),
     )
 
-    def clean_password(self):
-        if 'password' in self.cleaned_data:
-            password = self.cleaned_data['password']
-            try:
-                str(password)
-            except UnicodeEncodeError:
-                raise forms.ValidationError(
-                    _("Invalid characters in password"))
-            if not password_policy_compliant(password):
-                raise forms.ValidationError(PASSWORD_ERROR)
-            return password
-
     def clean(self):
         cleaned_data = self.cleaned_data
         password = cleaned_data.get('password')
@@ -171,6 +160,7 @@
     )
     password = fields.CharField(
         required=False,
+        validators=[validate_password_policy],
         widget=widgets.PasswordInput(attrs={
             'class': 'disableAutoComplete textType',
             'size': '20',
@@ -237,14 +227,6 @@
                 (self.data.get('currentpassword') or
                  self.data.get('passwordconfirm'))):
             raise forms.ValidationError(_("Required field"))
-        if password:
-            try:
-                str(password)
-            except UnicodeEncodeError:
-                raise forms.ValidationError(
-                    _("Invalid characters in password"))
-            if not password_policy_compliant(password):
-                raise forms.ValidationError(PASSWORD_ERROR)
         return password
 
     def clean_passwordconfirm(self):
 
=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2012-10-29 12:47:16 +0000
+++ identityprovider/models/account.py	2012-12-13 20:21:20 +0000
@@ -278,6 +278,24 @@
         sname = self.displayname.split(' ')
         return sname[0]
 
+    @property
+    def encrypted_password(self):
+        try:
+            accountpassword = self.accountpassword
+            password = accountpassword.password
+        except AccountPassword.DoesNotExist:
+            password = None
+        return password
+
+    def set_password(self, password, salt=None):
+        try:
+            accountpassword = self.accountpassword
+        except AccountPassword.DoesNotExist:
+            accountpassword = AccountPassword(account=self, password='invalid')
+        accountpassword.password = encrypt_launchpad_password(
+            password, salt=salt)
+        accountpassword.save()
+
     def get_and_delete_messages(self):
         return []
 
 
=== modified file 'identityprovider/models/emailaddress.py'
--- identityprovider/models/emailaddress.py	2012-11-29 14:47:09 +0000
+++ identityprovider/models/emailaddress.py	2012-12-13 20:21:20 +0000
@@ -7,6 +7,7 @@
 from django.conf import settings
 from django.core.mail import send_mail
 from django.core.urlresolvers import reverse
+from django.core.validators import validate_email
 from django.db import models
 from django.template.defaultfilters import force_escape
 from django.template.loader import render_to_string
@@ -22,7 +23,7 @@
 
 
 class EmailAddress(models.Model):
-    email = models.TextField()
+    email = models.TextField(validators=[validate_email])
     lp_person = models.ForeignKey(
         Person, db_column='person', blank=True, null=True, editable=False)
     status = models.IntegerField(choices=EmailStatus._get_choices())
 
=== added file 'identityprovider/tests/unit/test_validators.py'
--- identityprovider/tests/unit/test_validators.py	1970-01-01 00:00:00 +0000
+++ identityprovider/tests/unit/test_validators.py	2012-12-13 20:21:20 +0000
@@ -0,0 +1,26 @@
+from django.core.exceptions import ValidationError
+
+from identityprovider.tests.utils import SSOBaseTestCase
+from identityprovider.validators import validate_password_policy
+
+
+class PasswordPolicyValidatorTestCase(SSOBaseTestCase):
+
+    def test_password_too_short(self):
+        self.assertRaises(
+            ValidationError, validate_password_policy, 'abcd3Fg')
+
+    def test_password_missing_uppercase(self):
+        self.assertRaises(
+            ValidationError, validate_password_policy, 'abcd3fgh')
+
+    def test_password_missing_number(self):
+        self.assertRaises(
+            ValidationError, validate_password_policy, 'abcdEfgh')
+
+    def test_password_invalid_chars(self):
+        self.assertRaises(
+            ValidationError, validate_password_policy, u'abcd\xe1Fgh')
+
+    def test_valid_password(self):
+        self.assertEqual(validate_password_policy('abcD3fgh'), None)
 
=== added file 'identityprovider/validators.py'
--- identityprovider/validators.py	1970-01-01 00:00:00 +0000
+++ identityprovider/validators.py	2012-12-13 20:21:20 +0000
@@ -0,0 +1,21 @@
+from django.core.exceptions import ValidationError
+from django.utils.translation import ugettext_lazy as _
+
+from identityprovider.utils import (
+    password_policy_compliant,
+)
+
+
+PASSWORD_POLICY_ERROR = _("Password must be at least "
+                          "8 characters long, and must contain at least one "
+                          "number and an upper case letter.")
+
+
+def validate_password_policy(password):
+    """Validate password complies with policy."""
+    try:
+        str(password)
+    except UnicodeEncodeError:
+        raise ValidationError(_("Invalid characters in password"))
+    if not password_policy_compliant(password):
+        raise ValidationError(PASSWORD_POLICY_ERROR)
 
=== modified file 'webui/views/ui.py'
--- webui/views/ui.py	2012-12-06 14:31:23 +0000
+++ webui/views/ui.py	2012-12-13 20:21:20 +0000
@@ -48,7 +48,6 @@
 )
 from identityprovider.models import (
     Account,
-    AccountPassword,
     AuthToken,
     AuthTokenFactory,
     EmailAddress,
@@ -1023,15 +1022,7 @@
                 if not created:
                     account.preferredemail = email_obj
             email = account.preferredemail.email
-            try:
-                password_obj = account.accountpassword
-            except AccountPassword.DoesNotExist:
-                password_obj = AccountPassword.objects.create(
-                    account=account,
-                    password='invalid',
-                )
-            password_obj.password = encrypt_launchpad_password(password)
-            password_obj.save()
+            account.set_password(password)
             user = auth.authenticate(username=email, password=password)
             auth.login(request, user)
             atrequest.consume()