From 15bf888624bc2fe513b3427eb3c3f5fa8dc3c10f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 14:18:47 +0000 Subject: [PATCH 1/3] 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'] From 6332058781bb556d4cdb6962d165ba125c03ec17 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 19:10:49 +0000 Subject: [PATCH 2/3] remove PUT /user/ --- app/models.py | 2 +- app/user/rest.py | 17 ------ tests/app/user/test_rest.py | 112 ------------------------------------ 3 files changed, 1 insertion(+), 130 deletions(-) diff --git a/app/models.py b/app/models.py index bd6436b05..6adb5abb5 100644 --- a/app/models.py +++ b/app/models.py @@ -563,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/user/rest.py b/app/user/rest.py index acf7e8abb..787c78f44 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -77,23 +77,6 @@ def create_user(): return jsonify(data=user_schema.dump(user_to_create).data), 201 -@user_blueprint.route('/', methods=['PUT']) -def update_user(user_id): - user_to_update = get_user_by_id(user_id=user_id) - req_json = request.get_json() - update_dct, errors = user_schema_load_json.load(req_json) - # TODO don't let password be updated in this PUT method (currently used by the forgot password flow) - pwd = req_json.get('password', None) - if pwd is not None: - if not pwd: - errors.update({'password': ['Invalid data for field']}) - raise InvalidRequest(errors, status_code=400) - else: - reset_failed_login_count(user_to_update) - save_model_user(user_to_update, update_dict=update_dct, pwd=pwd) - return jsonify(data=user_schema.dump(user_to_update).data), 200 - - @user_blueprint.route('/', methods=['POST']) def update_user_attribute(user_id): user_to_update = get_user_by_id(user_id=user_id) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 880cf2cd1..8c97e5d70 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -208,42 +208,6 @@ def test_cannot_create_user_with_empty_strings(admin_request, notify_db_session) } -def test_put_user(client, sample_service): - """ - Tests PUT endpoint '/' to update a user. - """ - assert User.query.count() == 1 - sample_user = sample_service.users[0] - sample_user.failed_login_count = 1 - new_email = 'new@digital.cabinet-office.gov.uk' - data = { - 'name': sample_user.name, - 'email_address': new_email, - 'mobile_number': sample_user.mobile_number - } - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=sample_user.id), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 200 - assert User.query.count() == 1 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['email_address'] == new_email - expected_permissions = default_service_permissions - fetched = json_resp['data'] - - assert str(sample_user.id) == fetched['id'] - assert sample_user.name == fetched['name'] - assert sample_user.mobile_number == fetched['mobile_number'] - assert new_email == fetched['email_address'] - assert sample_user.state == fetched['state'] - assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) - # password wasn't updated, so failed_login_count stays the same - assert sample_user.failed_login_count == 1 - - @pytest.mark.parametrize('user_attribute, user_value', [ ('name', 'New User'), ('email_address', 'newuser@mail.com'), @@ -267,63 +231,6 @@ def test_post_user_attribute(client, sample_user, user_attribute, user_value): assert json_resp['data'][user_attribute] == user_value -def test_put_user_update_password(client, sample_service): - """ - Tests PUT endpoint '/' to update a user including their password. - """ - assert User.query.count() == 1 - sample_user = sample_service.users[0] - new_password = '1234567890' - data = { - 'name': sample_user.name, - 'email_address': sample_user.email_address, - 'mobile_number': sample_user.mobile_number, - 'password': new_password - } - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=sample_user.id), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 200 - assert User.query.count() == 1 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['password_changed_at'] is not None - data = {'password': new_password} - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.post( - url_for('user.verify_user_password', user_id=str(sample_user.id)), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 204 - - -def test_put_user_not_exists(client, sample_user, fake_uuid): - """ - Tests PUT endpoint '/' to update a user doesn't exist. - """ - assert User.query.count() == 1 - new_email = 'new@digital.cabinet-office.gov.uk' - data = {'email_address': new_email} - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=fake_uuid), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 404 - assert User.query.count() == 1 - user = User.query.filter_by(id=str(sample_user.id)).first() - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['result'] == "error" - assert json_resp['message'] == 'No result found' - - assert user == sample_user - assert user.email_address != new_email - - def test_get_user_by_email(client, sample_service): sample_user = sample_service.users[0] header = create_authorization_header() @@ -578,25 +485,6 @@ def test_update_user_password_saves_correctly(client, sample_service): assert resp.status_code == 204 -def test_update_user_resets_failed_login_count_if_updating_password(client, sample_service): - user = sample_service.users[0] - user.failed_login_count = 1 - - resp = client.put( - url_for('user.update_user', user_id=user.id), - data=json.dumps({ - 'name': user.name, - 'email_address': user.email_address, - 'mobile_number': user.mobile_number, - 'password': 'foo' - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] - ) - - assert resp.status_code == 200 - assert user.failed_login_count == 0 - - def test_activate_user(admin_request, sample_user): sample_user.state = 'pending' From 834eecd0f14b51752f6f16d9d5a3557338a8549b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 10 Nov 2017 15:24:37 +0000 Subject: [PATCH 3/3] make sure you can't edit password --- tests/app/user/test_rest.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 8c97e5d70..00444ea64 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -564,3 +564,13 @@ def test_cannot_update_user_with_mobile_number_as_empty_string(admin_request, sa _expected_status=400 ) assert resp['message']['mobile_number'] == ['Invalid phone number: Not enough digits'] + + +def test_cannot_update_user_password_using_attributes_method(admin_request, sample_user): + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'password': 'foo'}, + _expected_status=400 + ) + assert resp['message']['_schema'] == ['Unknown field name password']