diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index 326bafdc7..ea065e9d6 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -14,6 +14,8 @@ def insert_user(user): db.session.commit() +# TODO Would be better to have a generic get and update for user +# something that replicates the sql functionality. def get_user_by_id(id): return User.query.filter_by(id=id).first() diff --git a/app/main/forms.py b/app/main/forms.py index 28238e74e..1aceb8465 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -26,6 +26,12 @@ verify_code = '^\d{5}$' class RegisterUserForm(Form): + + def __init__(self, existing_email_addresses, existing_mobile_numbers, *args, **kwargs): + self.existing_emails = existing_email_addresses + self.existing_mobiles = existing_mobile_numbers + super(RegisterUserForm, self).__init__(*args, **kwargs) + name = StringField('Full name', validators=[DataRequired(message='Name can not be empty')]) email_address = StringField('Email address', validators=[ @@ -42,6 +48,16 @@ class RegisterUserForm(Form): Length(10, 255, message='Password must be at least 10 characters'), Blacklist(message='That password is blacklisted, too common')]) + def validate_email_address(self, field): + # Validate email address is unique. + if field.data in self.existing_emails: + raise ValidationError('Email address already exists') + + def validate_mobile_number(self, field): + # Validate mobile number is unique + if field.data in self.existing_mobiles: + raise ValidationError('Mobile number already exists') + class TwoFactorForm(Form): sms_code = StringField('sms code', validators=[DataRequired(message='Please enter your code'), diff --git a/app/main/views/register.py b/app/main/views/register.py index cd8830db9..43581721e 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -12,14 +12,19 @@ from app.main.views import send_sms_code, send_email_code from app.models import User -@main.route("/register", methods=['GET']) -def render_register(): - return render_template('views/register.html', form=RegisterUserForm()) - - -@main.route('/register', methods=['POST']) +# TODO how do we handle duplicate unverifed email addresses? +# malicious or otherwise. +@main.route('/register', methods=['GET', 'POST']) def process_register(): - form = RegisterUserForm() + try: + existing_emails, existing_mobiles = zip( + *[(x.email_address, x.mobile_number) for x in + users_dao.get_all_users()]) + except ValueError: + # Value error is raised if the db is empty. + existing_emails, existing_mobiles = [], [] + + form = RegisterUserForm(existing_emails, existing_mobiles) if form.validate_on_submit(): user = User(name=form.name.data, @@ -28,16 +33,16 @@ def process_register(): password=form.password.data, created_at=datetime.now(), role_id=1) - try: - users_dao.insert_user(user) - send_sms_code(user_id=user.id, mobile_number=form.mobile_number.data) - send_email_code(user_id=user.id, email=form.email_address.data) - session['expiry_date'] = str(datetime.now() + timedelta(hours=1)) - session['user_id'] = user.id - except AdminApiClientException as e: - return jsonify(admin_api_client_error=e.value) - except SQLAlchemyError: - return jsonify(database_error='encountered database error'), 400 - else: - return jsonify(form.errors), 400 - return redirect('/verify') + users_dao.insert_user(user) + # TODO possibly there should be some exception handling + # for sending sms and email codes. + # How do we report to the user there is a problem with + # sending codes apart from service unavailable? + # at the moment i believe http 500 is fine. + send_sms_code(user_id=user.id, mobile_number=form.mobile_number.data) + send_email_code(user_id=user.id, email=form.email_address.data) + session['expiry_date'] = str(datetime.now() + timedelta(hours=1)) + session['user_id'] = user.id + return redirect('/verify') + + return render_template('views/register.html', form=form) diff --git a/app/templates/views/register.html b/app/templates/views/register.html index 526344763..610516f3b 100644 --- a/app/templates/views/register.html +++ b/app/templates/views/register.html @@ -14,24 +14,13 @@ GOV.UK Notify | Create an account
{{ form.hidden_tag() }} -

- - {{ form.name(class="form-control-2-3", autocomplete="off") }}
-

-

- - {{ form.email_address(class="form-control-2-3", autocomplete="off") }}
+ + {{ render_field(form.name, class='form-control-2-3') }} + {{ render_field(form.email_address, class='form-control-2-3') }} Your email address must end in .gov.uk -

-

- - {{ form.mobile_number(class="form-control-1-4", autocomplete="off") }}
-

-

- - {{ form.password(class="form-control-1-4", autocomplete="off") }}
+ {{ render_field(form.mobile_number, class='form-control-2-3') }} + {{ render_field(form.password, class='form-control-2-3') }} Your password must have at least 10 characters -

diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index a1cc52192..d85159f51 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -4,14 +4,11 @@ from app.main.forms import RegisterUserForm def test_should_raise_validation_error_for_password(notifications_admin): - form = RegisterUserForm() + form = RegisterUserForm([], []) form.name.data = 'test' form.email_address.data = 'teset@example.gov.uk' form.mobile_number.data = '+441231231231' form.password.data = 'password1234' - try: - form.validate() - fail() - except: - assert 'That password is blacklisted, too common' in form.errors['password'] + form.validate() + assert 'That password is blacklisted, too common' in form.errors['password'] diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 8fffb9d8a..666e9f86d 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -30,7 +30,7 @@ def test_process_register_returns_400_when_mobile_number_is_invalid(notification 'mobile_number': 'not good', 'password': 'validPassword!'}) - assert response.status_code == 400 + assert response.status_code == 200 assert 'Please enter a +44 mobile number' in response.get_data(as_text=True) @@ -45,7 +45,7 @@ def test_should_return_400_when_email_is_not_gov_uk(notifications_admin, 'mobile_number': '+44123412345', 'password': 'validPassword!'}) - assert response.status_code == 400 + assert response.status_code == 200 assert 'Please enter a gov.uk email address' in response.get_data(as_text=True) @@ -73,5 +73,5 @@ def test_should_return_400_if_password_is_blacklisted(notifications_admin, notif 'mobile_number': '+44123412345', 'password': 'password1234'}) - response.status_code == 400 + response.status_code == 200 assert 'That password is blacklisted, too common' in response.get_data(as_text=True)