From 15bf888624bc2fe513b3427eb3c3f5fa8dc3c10f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 14:18:47 +0000 Subject: [PATCH] make user mobile num nullable if user has email_auth enabled --- app/errors.py | 2 +- app/models.py | 9 +- app/schemas.py | 23 +++- app/user/rest.py | 14 ++ .../versions/0136_user_mobile_nullable.py | 28 ++++ tests/app/user/test_rest.py | 126 ++++++++++++++++-- 6 files changed, 185 insertions(+), 17 deletions(-) create mode 100644 migrations/versions/0136_user_mobile_nullable.py diff --git a/app/errors.py b/app/errors.py index a6119d98d..8ff935002 100644 --- a/app/errors.py +++ b/app/errors.py @@ -94,7 +94,7 @@ def register_errors(blueprint): current_app.logger.exception(e) if hasattr(e, 'orig') and hasattr(e.orig, 'pgerror') and e.orig.pgerror and \ ('duplicate key value violates unique constraint "services_name_key"' in e.orig.pgerror or - 'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror): + 'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror): return jsonify( result='error', message={'name': ["Duplicate service name '{}'".format( diff --git a/app/models.py b/app/models.py index 0c64a4581..bd6436b05 100644 --- a/app/models.py +++ b/app/models.py @@ -9,7 +9,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, and_ +from sqlalchemy import UniqueConstraint, CheckConstraint, and_ from sqlalchemy.orm import foreign, remote from notifications_utils.recipients import ( validate_email_address, @@ -97,7 +97,7 @@ class User(db.Model): nullable=True, onupdate=datetime.datetime.utcnow) _password = db.Column(db.String, index=False, unique=False, nullable=False) - mobile_number = db.Column(db.String, index=False, unique=False, nullable=False) + mobile_number = db.Column(db.String, index=False, unique=False, nullable=True) password_changed_at = db.Column(db.DateTime, index=False, unique=False, nullable=False, default=datetime.datetime.utcnow) logged_in_at = db.Column(db.DateTime, nullable=True) @@ -107,6 +107,9 @@ class User(db.Model): current_session_id = db.Column(UUID(as_uuid=True), nullable=True) auth_type = db.Column(db.String, db.ForeignKey('auth_type.name'), index=True, nullable=False, default=SMS_AUTH_TYPE) + # either email auth or a mobile number must be provided + CheckConstraint("auth_type = 'email_auth' or mobile_number is not null") + services = db.relationship( 'Service', secondary='user_to_service', @@ -560,7 +563,7 @@ class Template(db.Model): nullable=False, default=NORMAL ) - + redact_personalisation = association_proxy('template_redacted', 'redact_personalisation') def get_link(self): diff --git a/app/schemas.py b/app/schemas.py index e689bf83f..fe9d29b0f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -105,6 +105,26 @@ class UserSchema(BaseSchema): "_password", "verify_codes") strict = True + @validates('name') + def validate_name(self, value): + if not value: + raise ValidationError('Invalid name') + + @validates('email_address') + def validate_email_address(self, value): + try: + validate_email_address(value) + except InvalidEmailError as e: + raise ValidationError(str(e)) + + @validates('mobile_number') + def validate_mobile_number(self, value): + try: + if value is not None: + validate_phone_number(value, international=True) + except InvalidPhoneError as error: + raise ValidationError('Invalid phone number: {}'.format(error)) + class UserUpdateAttributeSchema(BaseSchema): auth_type = field_for(models.User, 'auth_type') @@ -132,7 +152,8 @@ class UserUpdateAttributeSchema(BaseSchema): @validates('mobile_number') def validate_mobile_number(self, value): try: - validate_phone_number(value, international=True) + if value is not None: + validate_phone_number(value, international=True) except InvalidPhoneError as error: raise ValidationError('Invalid phone number: {}'.format(error)) diff --git a/app/user/rest.py b/app/user/rest.py index 5b559e606..acf7e8abb 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,6 +4,7 @@ from datetime import datetime from urllib.parse import urlencode from flask import (jsonify, request, Blueprint, current_app, abort) +from sqlalchemy.exc import IntegrityError from app.config import QueueNames from app.dao.users_dao import ( @@ -52,6 +53,19 @@ user_blueprint = Blueprint('user', __name__) register_errors(user_blueprint) +@user_blueprint.errorhandler(IntegrityError) +def handle_integrity_error(exc): + """ + Handle integrity errors caused by the auth type/mobile number check constraint + """ + if 'ck_users_mobile_or_email_auth' in str(exc): + # we don't expect this to trip, so still log error + current_app.logger.exception('Check constraint ck_users_mobile_or_email_auth triggered') + return jsonify(result='error', message='Mobile number must be set if auth_type is set to sms_auth'), 400 + + raise + + @user_blueprint.route('', methods=['POST']) def create_user(): user_to_create, errors = user_schema.load(request.get_json()) diff --git a/migrations/versions/0136_user_mobile_nullable.py b/migrations/versions/0136_user_mobile_nullable.py new file mode 100644 index 000000000..8ce4df31a --- /dev/null +++ b/migrations/versions/0136_user_mobile_nullable.py @@ -0,0 +1,28 @@ +""" + +Revision ID: 0136_user_mobile_nullable +Revises: 0135_stats_template_usage +Create Date: 2017-11-08 11:49:05.773974 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.sql import column +from sqlalchemy.dialects import postgresql + +revision = '0136_user_mobile_nullable' +down_revision = '0135_stats_template_usage' + + +def upgrade(): + op.alter_column('users', 'mobile_number', nullable=True) + + op.create_check_constraint( + 'ck_users_mobile_or_email_auth', + 'users', + "auth_type = 'email_auth' or mobile_number is not null" + ) + +def downgrade(): + op.alter_column('users', 'mobile_number', nullable=False) + op.drop_constraint('ck_users_mobile_or_email_auth', 'users') diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 25fd5fde0..880cf2cd1 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -159,6 +159,55 @@ def test_create_user_missing_attribute_password(client, notify_db, notify_db_ses assert {'password': ['Missing data for required field.']} == json_resp['message'] +def test_can_create_user_with_email_auth_and_no_mobile(admin_request, notify_db_session): + data = { + 'name': 'Test User', + 'email_address': 'user@digital.cabinet-office.gov.uk', + 'password': 'password', + 'mobile_number': None, + 'auth_type': EMAIL_AUTH_TYPE + } + + json_resp = admin_request.post('user.create_user', _data=data, _expected_status=201) + + assert json_resp['data']['auth_type'] == EMAIL_AUTH_TYPE + assert json_resp['data']['mobile_number'] is None + + +def test_cannot_create_user_with_sms_auth_and_no_mobile(admin_request, notify_db_session): + data = { + 'name': 'Test User', + 'email_address': 'user@digital.cabinet-office.gov.uk', + 'password': 'password', + 'mobile_number': None, + 'auth_type': SMS_AUTH_TYPE + } + + json_resp = admin_request.post('user.create_user', _data=data, _expected_status=400) + + assert json_resp['message'] == 'Mobile number must be set if auth_type is set to sms_auth' + + +def test_cannot_create_user_with_empty_strings(admin_request, notify_db_session): + data = { + 'name': '', + 'email_address': '', + 'password': 'password', + 'mobile_number': '', + 'auth_type': EMAIL_AUTH_TYPE + } + resp = admin_request.post( + 'user.create_user', + _data=data, + _expected_status=400 + ) + assert resp['message'] == { + 'email_address': ['Not a valid email address'], + 'mobile_number': ['Invalid phone number: Not enough digits'], + 'name': ['Invalid name'] + } + + def test_put_user(client, sample_service): """ Tests PUT endpoint '/' to update a user. @@ -548,18 +597,6 @@ def test_update_user_resets_failed_login_count_if_updating_password(client, samp assert user.failed_login_count == 0 -def test_update_user_auth_type(admin_request, sample_user): - assert sample_user.auth_type == 'sms_auth' - resp = admin_request.post( - 'user.update_user_attribute', - user_id=sample_user.id, - _data={'auth_type': 'email_auth'}, - ) - - assert resp['data']['id'] == str(sample_user.id) - assert resp['data']['auth_type'] == 'email_auth' - - def test_activate_user(admin_request, sample_user): sample_user.state = 'pending' @@ -574,3 +611,68 @@ def test_activate_user_fails_if_already_active(admin_request, sample_user): resp = admin_request.post('user.activate_user', user_id=sample_user.id, _expected_status=400) assert resp['message'] == 'User already active' assert sample_user.state == 'active' + + +def test_update_user_auth_type(admin_request, sample_user): + assert sample_user.auth_type == 'sms_auth' + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'auth_type': 'email_auth'}, + ) + + assert resp['data']['id'] == str(sample_user.id) + assert resp['data']['auth_type'] == 'email_auth' + + +def test_can_set_email_auth_and_remove_mobile_at_same_time(admin_request, sample_user): + sample_user.auth_type = SMS_AUTH_TYPE + + admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={ + 'mobile_number': None, + 'auth_type': EMAIL_AUTH_TYPE, + } + ) + + assert sample_user.mobile_number is None + assert sample_user.auth_type == EMAIL_AUTH_TYPE + + +def test_cannot_remove_mobile_if_sms_auth(admin_request, sample_user): + sample_user.auth_type = SMS_AUTH_TYPE + + json_resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'mobile_number': None}, + _expected_status=400 + ) + + assert json_resp['message'] == 'Mobile number must be set if auth_type is set to sms_auth' + + +def test_can_remove_mobile_if_email_auth(admin_request, sample_user): + sample_user.auth_type = EMAIL_AUTH_TYPE + + admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'mobile_number': None}, + ) + + assert sample_user.mobile_number is None + + +def test_cannot_update_user_with_mobile_number_as_empty_string(admin_request, sample_user): + sample_user.auth_type = EMAIL_AUTH_TYPE + + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'mobile_number': ''}, + _expected_status=400 + ) + assert resp['message']['mobile_number'] == ['Invalid phone number: Not enough digits']