Merge pull request #4353 from cerdo03/password-validation-surya

Implement minimum length for passwords in account creation

contentcuration/contentcuration/forms.py

@@ -1,12 +1,12 @@
 import json
-from builtins import object
 
 from django import forms
 from django.conf import settings
 from django.contrib.auth.forms import PasswordResetForm
 from django.contrib.auth.forms import UserChangeForm
 from django.contrib.auth.forms import UserCreationForm
 from django.core import signing
+from django.core.exceptions import ValidationError
 from django.db.models import Q
 from django.template.loader import render_to_string
 
@@ -16,23 +16,16 @@
 REGISTRATION_SALT = getattr(settings, 'REGISTRATION_SALT', 'registration')
 
 
-class ExtraFormMixin(object):
-
-    def check_field(self, field, error):
-        if not self.cleaned_data.get(field):
-            self.errors[field] = self.error_class()
-            self.add_error(field, error)
-            return False
-        return self.cleaned_data.get(field)
-
-
 # LOGIN/REGISTRATION FORMS
 #################################################################
-class RegistrationForm(UserCreationForm, ExtraFormMixin):
+class RegistrationForm(UserCreationForm):
+    CODE_ACCOUNT_ACTIVE = 'account_active'
+    CODE_ACCOUNT_INACTIVE = 'account_inactive'
+
     first_name = forms.CharField(required=True)
     last_name = forms.CharField(required=True)
-    email = forms.CharField(required=True)
-    password1 = forms.CharField(required=True)
+    email = forms.EmailField(required=True)
+    password1 = forms.CharField(required=True, min_length=8)
     password2 = forms.CharField(required=True)
     uses = forms.CharField(required=True)
     other_use = forms.CharField(required=False)
@@ -45,22 +38,18 @@ class RegistrationForm(UserCreationForm, ExtraFormMixin):
     locations = forms.CharField(required=True)
 
     def clean_email(self):
-        email = self.cleaned_data['email'].strip().lower()
-        if User.objects.filter(Q(is_active=True) | Q(deleted=True), email__iexact=email).exists():
-            raise UserWarning
+        # ensure email is lower case
+        email = self.cleaned_data["email"].strip().lower()
+        user_qs = User.objects.filter(email__iexact=email)
+        if user_qs.exists():
+            if user_qs.filter(Q(is_active=True) | Q(deleted=True)).exists():
+                raise ValidationError("Account already active", code=self.CODE_ACCOUNT_ACTIVE)
+            else:
+                raise ValidationError("Already registered.", code=self.CODE_ACCOUNT_INACTIVE)
         return email
 
-    def clean(self):
-        super(RegistrationForm, self).clean()
-
-        # Errors should be caught on the frontend
-        # or a warning should be thrown if the account exists
-        self.errors.clear()
-        return self.cleaned_data
-
     def save(self, commit=True):
-        user = super(RegistrationForm, self).save(commit=commit)
-        user.set_password(self.cleaned_data["password1"])
+        user = super(RegistrationForm, self).save(commit=False)
         user.first_name = self.cleaned_data["first_name"]
         user.last_name = self.cleaned_data["last_name"]
         user.information = {
@@ -165,7 +154,7 @@ def save(self, user):
         return user
 
 
-class StorageRequestForm(forms.Form, ExtraFormMixin):
+class StorageRequestForm(forms.Form):
     # Nature of content
     storage = forms.CharField(required=True)
     kind = forms.CharField(required=True)
@@ -194,7 +183,7 @@ class Meta:
                   "audience", "import_count", "location", "uploading_for", "organization_type", "time_constraint", "message")
 
 
-class IssueReportForm(forms.Form, ExtraFormMixin):
+class IssueReportForm(forms.Form):
     operating_system = forms.CharField(required=True)
     browser = forms.CharField(required=True)
     channel = forms.CharField(required=False)
@@ -204,7 +193,7 @@ class Meta:
         fields = ("operating_system", "browser", "channel", "description")
 
 
-class DeleteAccountForm(forms.Form, ExtraFormMixin):
+class DeleteAccountForm(forms.Form):
     email = forms.CharField(required=True)
 
     def __init__(self, user, *args, **kwargs):
@@ -214,5 +203,5 @@ def __init__(self, user, *args, **kwargs):
     def clean_email(self):
         email = self.cleaned_data['email'].strip().lower()
         if self.user.is_admin or self.user.email.lower() != self.cleaned_data['email']:
-            raise UserWarning
+            raise ValidationError("Not allowed")
         return email

contentcuration/contentcuration/frontend/accounts/pages/Create.vue

@@ -31,31 +31,40 @@
           v-model="form.first_name"
           maxlength="100"
           counter
-          :label="$tr('firstNameLabel')"
           autofocus
+          :label="$tr('firstNameLabel')"
+          :error-messages="errors.first_name"
+          @input="resetErrors('first_name')"
         />
         <TextField
           v-model="form.last_name"
           maxlength="100"
           counter
           :label="$tr('lastNameLabel')"
+          :error-messages="errors.last_name"
+          @input="resetErrors('last_name')"
         />
         <EmailField
           v-model="form.email"
           maxlength="100"
           counter
           :disabled="Boolean($route.query.email)"
-          :error-messages="emailErrors"
-          @input="emailErrors = []"
+          :error-messages="errors.email"
+          @input="resetErrors('email')"
         />
         <PasswordField
           v-model="form.password1"
+          :additionalRules="passwordValidationRules"
           :label="$tr('passwordLabel')"
+          :error-messages="errors.password1"
+          @input="resetErrors('password1')"
         />
         <PasswordField
           v-model="form.password2"
           :additionalRules="passwordConfirmRules"
           :label="$tr('confirmPasswordLabel')"
+          :error-messages="errors.password2"
+          @input="resetErrors('password2')"
         />
 
         <!-- Usage -->
@@ -200,6 +209,7 @@
   import Checkbox from 'shared/views/form/Checkbox';
   import { policies } from 'shared/constants';
   import DropdownWrapper from 'shared/views/form/DropdownWrapper';
+  import commonStrings from 'shared/translator';
 
   export default {
     name: 'Create',
@@ -219,7 +229,6 @@
       return {
         valid: true,
         registrationFailed: false,
-        emailErrors: [],
         form: {
           first_name: '',
           last_name: '',
@@ -237,6 +246,13 @@
           accepted_policy: false,
           accepted_tos: false,
         },
+        errors: {
+          first_name: [],
+          last_name: [],
+          email: [],
+          password1: [],
+          password2: [],
+        },
       };
     },
     computed: {
@@ -247,6 +263,9 @@
       passwordConfirmRules() {
         return [value => (this.form.password1 === value ? true : this.$tr('passwordMatchMessage'))];
       },
+      passwordValidationRules() {
+        return [value => (value.length >= 8 ? true : this.$tr('passwordValidationMessage'))];
+      },
       acceptedAgreement: {
         get() {
           return this.form.accepted_tos && this.form.accepted_policy;
@@ -294,10 +313,12 @@
         ];
       },
       usageRules() {
-        return [() => (this.form.uses.length ? true : this.$tr('fieldRequiredMessage'))];
+        /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
+        return [() => (this.form.uses.length ? true : commonStrings.$tr('fieldRequired'))];
       },
       locationRules() {
-        return [() => (this.form.locations.length ? true : this.$tr('fieldRequiredMessage'))];
+        /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
+        return [() => (this.form.locations.length ? true : commonStrings.$tr('fieldRequired'))];
       },
       sources() {
         return sources;
@@ -359,7 +380,8 @@
         ];
       },
       sourceRules() {
-        return [() => (this.form.source.length ? true : this.$tr('fieldRequiredMessage'))];
+        /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
+        return [() => (this.form.source.length ? true : commonStrings.$tr('fieldRequired'))];
       },
       clean() {
         return data => {
@@ -438,8 +460,27 @@
             .catch(error => {
               if (error.message === 'Network Error') {
                 this.$refs.top.scrollIntoView({ behavior: 'smooth' });
+              } else if (error.response.status === 400) {
+                for (const field of error.response.data) {
+                  if (!Object.prototype.hasOwnProperty.call(this.errors, field)) {
+                    continue;
+                  }
+                  let message = '';
+                  switch (field) {
+                    case 'password1':
+                      message = this.$tr('passwordValidationMessage');
+                      break;
+                    default:
+                      /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
+                      message = commonStrings.$tr('fieldHasError');
+                      break;
+                  }
+                  this.errors[field] = [message];
+                }
+                this.registrationFailed = true;
+                this.valid = false;
               } else if (error.response.status === 403) {
-                this.emailErrors = [this.$tr('emailExistsMessage')];
+                this.errors.email = [this.$tr('emailExistsMessage')];
               } else if (error.response.status === 405) {
                 this.$router.push({ name: 'AccountNotActivated' });
               } else {
@@ -452,12 +493,14 @@
         }
         return Promise.resolve();
       },
+      resetErrors(field) {
+        this.errors[field] = [];
+      },
     },
 
     $trs: {
       backToLoginButton: 'Sign in',
       createAnAccountTitle: 'Create an account',
-      fieldRequiredMessage: 'Field is required',
       errorsMessage: 'Please fix the errors below',
       registrationFailed: 'There was an error registering your account. Please try again',
       registrationFailedOffline:
@@ -470,6 +513,7 @@
       passwordLabel: 'Password',
       confirmPasswordLabel: 'Confirm password',
       passwordMatchMessage: "Passwords don't match",
+      passwordValidationMessage: 'Password should be at least 8 characters long',
 
       // Usage question
       usageLabel: 'How do you plan on using Kolibri Studio (check all that apply)',

contentcuration/contentcuration/frontend/accounts/pages/__tests__/create.spec.js

@@ -17,8 +17,8 @@ const defaultData = {
   first_name: 'Test',
   last_name: 'User',
   email: '[email protected]',
-  password1: 'pass',
-  password2: 'pass',
+  password1: 'tester123',
+  password2: 'tester123',
   uses: ['tagging'],
   storage: '',
   other_use: '',
@@ -126,6 +126,11 @@ describe('create', () => {
         expect(register).not.toHaveBeenCalled();
       });
     });
+    it('should fail if password1 is too short', () => {
+      const wrapper = makeWrapper({ password1: '123' });
+      wrapper.vm.submit();
+      expect(register).not.toHaveBeenCalled();
+    });
     it('should fail if password1 and password2 do not match', () => {
       const wrapper = makeWrapper({ password1: 'some other password' });
       wrapper.vm.submit();
@@ -155,7 +160,7 @@ describe('create', () => {
     it('should say account with email already exists if register returns a 403', async () => {
       wrapper.setMethods({ register: makeFailedPromise(403) });
       await wrapper.vm.submit();
-      expect(wrapper.vm.emailErrors).toHaveLength(1);
+      expect(wrapper.vm.errors.email).toHaveLength(1);
     });
     it('should say account has not been activated if register returns 405', async () => {
       wrapper.setMethods({ register: makeFailedPromise(405) });

contentcuration/contentcuration/frontend/shared/translator.js

@@ -2,6 +2,7 @@ import { createTranslator } from 'shared/i18n';
 
 const MESSAGES = {
   fieldRequired: 'This field is required',
+  fieldHasError: 'This field has an error',
   titleRequired: 'Title is required',
   licenseRequired: 'License is required',
   copyrightHolderRequired: 'Copyright holder is required',

contentcuration/contentcuration/frontend/shared/views/form/EmailField.vue

@@ -17,6 +17,8 @@
 
 <script>
 
+  import commonStrings from 'shared/translator';
+
   export default {
     name: 'EmailField',
     props: {
@@ -46,15 +48,15 @@
       emailRules() {
         // TODO: fix checking if email exists
         return [
-          v => (!this.required || v.trim() ? true : this.$tr('emailRequiredMessage')),
+          /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
+          v => (!this.required || v.trim() ? true : commonStrings.$tr('fieldRequired')),
           v => /.+@.+\..+/.test(v) || this.$tr('validEmailMessage'),
         ];
       },
     },
     $trs: {
       emailLabel: 'Email',
       validEmailMessage: 'Please enter a valid email',
-      emailRequiredMessage: 'Field is required',
     },
   };
 

contentcuration/contentcuration/frontend/shared/views/form/PasswordField.vue

@@ -7,6 +7,7 @@
     :rules="rules"
     :label="label || $tr('passwordLabel')"
     :validate-on-blur="!validate"
+    :error-messages="errorMessages"
     type="password"
     @keyup.enter="validate = true"
     @keydown="validate = false"
@@ -17,6 +18,8 @@
 
 <script>
 
+  import commonStrings from 'shared/translator';
+
   export default {
     name: 'PasswordField',
     props: {
@@ -40,6 +43,12 @@
         type: Boolean,
         default: true,
       },
+      errorMessages: {
+        type: Array,
+        default() {
+          return [];
+        },
+      },
     },
     data() {
       return {
@@ -56,14 +65,14 @@
         },
       },
       rules() {
-        return [v => (!this.required || v ? true : this.$tr('fieldRequiredMessage'))].concat(
+        /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
+        return [v => (!this.required || v ? true : commonStrings.$tr('fieldRequired'))].concat(
           this.additionalRules
         );
       },
     },
     $trs: {
       passwordLabel: 'Password',
-      fieldRequiredMessage: 'Field is required',
     },
   };
 

contentcuration/contentcuration/tests/test_settings.py

@@ -37,7 +37,7 @@ def test_delete_account_invalid(self):
         try:
             DeleteAccountView.as_view()(request)
             self.assertTrue(False)
-        except UserWarning:
+        except Exception:
             self.assertTrue(User.objects.filter(email=self.user.email).exists())
 
     def test_delete_account(self):