Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement minimum length for passwords in account creation #4353

Merged
merged 12 commits into from
Aug 13, 2024
51 changes: 20 additions & 31 deletions contentcuration/contentcuration/forms.py
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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)
Expand All @@ -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 = {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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):
Expand All @@ -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
62 changes: 53 additions & 9 deletions contentcuration/contentcuration/frontend/accounts/pages/Create.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->
Expand Down Expand Up @@ -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',
Expand All @@ -219,7 +229,6 @@
return {
valid: true,
registrationFailed: false,
emailErrors: [],
form: {
first_name: '',
last_name: '',
Expand All @@ -237,6 +246,13 @@
accepted_policy: false,
accepted_tos: false,
},
errors: {
first_name: [],
last_name: [],
email: [],
password1: [],
password2: [],
},
};
},
computed: {
Expand All @@ -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;
Expand Down Expand Up @@ -294,10 +313,12 @@
];
},
usageRules() {
return [() => (this.form.uses.length ? true : this.$tr('fieldRequiredMessage'))];
/* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */
Copy link
Member

@akolson akolson Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is nolonger necessary as fieldRequired exists. There's a few other places to clean this up too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is required. The linting rule does not properly understand this is an external translator

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;
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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 {
Expand All @@ -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:
Expand All @@ -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)',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: '',
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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) });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

<script>
import commonStrings from 'shared/translator';
export default {
name: 'EmailField',
props: {
Expand Down Expand Up @@ -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',
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -17,6 +18,8 @@

<script>
import commonStrings from 'shared/translator';
export default {
name: 'PasswordField',
props: {
Expand All @@ -40,6 +43,12 @@
type: Boolean,
default: true,
},
errorMessages: {
type: Array,
default() {
return [];
},
},
},
data() {
return {
Expand All @@ -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',
},
};
Expand Down
2 changes: 1 addition & 1 deletion contentcuration/contentcuration/tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Loading