From 37c95b08b69e54fcd0cec91083a82814f23c652b Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 3 Nov 2016 11:11:51 +0000 Subject: [PATCH 1/7] Refactor saving user profile --- app/dao/users_dao.py | 9 ++++++--- app/user/rest.py | 10 +++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 1a240fecc..264481bfe 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -7,6 +7,11 @@ from app import db from app.models import (User, VerifyCode) +def _remove_values_for_keys_if_present(dict, keys): + for key in keys: + dict.pop(key, None) + + def create_secret_code(): return ''.join(map(str, random.sample(range(9), 5))) @@ -16,9 +21,7 @@ def save_model_user(usr, update_dict={}, pwd=None): usr.password = pwd usr.password_changed_at = datetime.utcnow() if update_dict: - if update_dict.get('id'): - del update_dict['id'] - update_dict.pop('password_changed_at') + _remove_values_for_keys_if_present(update_dict, ['id', 'password', 'password_changed_at']) db.session.query(User).filter_by(id=usr.id).update(update_dict) else: db.session.add(usr) diff --git a/app/user/rest.py b/app/user/rest.py index 2e6e6b021..7aee5ac3f 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -55,14 +55,10 @@ def create_user(): def update_user(user_id): user_to_update = get_model_users(user_id=user_id) req_json = request.get_json() - update_dct, errors = user_schema_load_json.load(req_json) pwd = req_json.get('password', None) - # TODO password validation, it is already done on the admin app - # but would be good to have the same validation here. - if pwd is not None and not pwd: - errors.update({'password': ['Invalid data for field']}) - raise InvalidRequest(errors, status_code=400) - save_model_user(user_to_update, update_dict=update_dct, pwd=pwd) + if not pwd: + raise InvalidRequest('Invalid entry for password', status_code=400) + save_model_user(user_to_update, update_dict=req_json, pwd=pwd) return jsonify(data=user_schema.dump(user_to_update).data), 200 From a41e7d5a2f68cdd70438635bd44d34cb648f1caa Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 3 Nov 2016 11:46:58 +0000 Subject: [PATCH 2/7] Add the right password check --- app/user/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/user/rest.py b/app/user/rest.py index 7aee5ac3f..4fd21e202 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -56,7 +56,7 @@ def update_user(user_id): user_to_update = get_model_users(user_id=user_id) req_json = request.get_json() pwd = req_json.get('password', None) - if not pwd: + if pwd is not None and not pwd: raise InvalidRequest('Invalid entry for password', status_code=400) save_model_user(user_to_update, update_dict=req_json, pwd=pwd) return jsonify(data=user_schema.dump(user_to_update).data), 200 From 8a126c738770ce09c961e76db64ac903d223888e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Nov 2016 17:41:49 +0000 Subject: [PATCH 3/7] Add a schema to validate a single user attr 'strictly' --- app/schemas.py | 36 ++++++++++++++++++++++++++++++++++++ tests/app/test_schemas.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index 6f5934114..b9f75cb2c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -105,6 +105,41 @@ class UserSchema(BaseSchema): strict = True +class UserUpdateAttributeSchema(BaseSchema): + + class Meta: + model = models.User + exclude = ( + "updated_at", "created_at", "user_to_service", + "_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(e.message) + + @validates('mobile_number') + def validate_mobile_number(self, value): + try: + validate_phone_number(value) + except InvalidPhoneError as error: + raise ValidationError('Invalid phone number: {}'.format(error)) + + @validates_schema(pass_original=True) + def check_unknown_fields(self, data, original_data): + for key in original_data: + if key not in self.fields: + raise ValidationError('Unknown field name {}'.format(key)) + + class ProviderDetailsSchema(BaseSchema): class Meta: model = models.ProviderDetails @@ -529,6 +564,7 @@ class UnarchivedTemplateSchema(BaseSchema): user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) +user_update_schema_load_json = UserUpdateAttributeSchema(load_json=True, partial=True) service_schema = ServiceSchema() service_schema_load_json = ServiceSchema(load_json=True) detailed_service_schema = DetailedServiceSchema() diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py index 4ba1c98b5..5f2bc4371 100644 --- a/tests/app/test_schemas.py +++ b/tests/app/test_schemas.py @@ -1,3 +1,6 @@ +import pytest + + def test_job_schema_doesnt_return_notifications(sample_notification_with_job): from app.schemas import job_schema @@ -22,3 +25,34 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key) data = notification_with_template_schema.dump(sample_notification_with_api_key).data assert data['key_name'] == 'Test key' + + +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', 'New User'), + ('email_address', 'newuser@mail.com'), + ('mobile_number', '+4407700900460') +]) +def test_user_schema_accepts_valid_attributes(user_attribute, user_value): + update_dict = { + user_attribute: user_value + } + from app.schemas import user_update_schema_load_json + + data, errors = user_update_schema_load_json.load(update_dict) + assert not errors + + +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', None), + ('name', ''), + ('email_address', 'bademail@...com'), + ('mobile_number', '+44077009') +]) +def test_user_schema_rejects_invalid_attributes(user_attribute, user_value): + from app.schemas import user_update_schema_load_json + update_dict = { + user_attribute: user_value + } + + with pytest.raises(Exception): + data, errors = user_update_schema_load_json.load(update_dict) From 461d8a9b2c4850c6caa74936dfcf945f3df34a91 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Nov 2016 17:42:23 +0000 Subject: [PATCH 4/7] Add separate endpoint to update a single user attr --- app/service/rest.py | 8 ++++---- app/user/rest.py | 36 +++++++++++++++++++++++++----------- tests/app/user/test_rest.py | 24 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 15 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 2cc7dab90..fb1ea29e1 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -34,7 +34,7 @@ from app.dao.service_whitelist_dao import ( ) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count -from app.dao.users_dao import get_model_users +from app.dao.users_dao import get_user_by_id from app.errors import ( register_errors, InvalidRequest @@ -88,7 +88,7 @@ def create_service(): errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) - user = get_model_users(data['user_id']) + user = get_user_by_id(data['user_id']) data.pop('user_id', None) valid_service = service_schema.load(request.get_json()).data dao_create_service(valid_service, user) @@ -148,7 +148,7 @@ def get_users_for_service(service_id): @service_blueprint.route('//users/', methods=['POST']) def add_user_to_service(service_id, user_id): service = dao_fetch_service_by_id(service_id) - user = get_model_users(user_id=user_id) + user = get_user_by_id(user_id=user_id) if user in service.users: error = 'User id: {} already part of service id: {}'.format(user_id, service_id) @@ -163,7 +163,7 @@ def add_user_to_service(service_id, user_id): @service_blueprint.route('//users/', methods=['DELETE']) def remove_user_from_service(service_id, user_id): service = dao_fetch_service_by_id(service_id) - user = get_model_users(user_id=user_id) + user = get_user_by_id(user_id=user_id) if user not in service.users: error = 'User not found' raise InvalidRequest(error, status_code=404) diff --git a/app/user/rest.py b/app/user/rest.py index 2e6e6b021..e41e10877 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,7 +4,7 @@ from datetime import datetime from flask import (jsonify, request, Blueprint, current_app) from app import encryption, DATETIME_FORMAT from app.dao.users_dao import ( - get_model_users, + get_user_by_id, save_model_user, create_user_code, get_user_code, @@ -12,7 +12,8 @@ from app.dao.users_dao import ( increment_failed_login_count, reset_failed_login_count, get_user_by_email, - create_secret_code + create_secret_code, + save_user_attribute ) from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id @@ -22,8 +23,10 @@ from app.schemas import ( email_data_request_schema, user_schema, request_verify_code_schema, + permission_schema, user_schema_load_json, - permission_schema) + user_update_schema_load_json +) from app.celery.tasks import ( send_sms, @@ -53,7 +56,7 @@ def create_user(): @user.route('/', methods=['PUT']) def update_user(user_id): - user_to_update = get_model_users(user_id=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) pwd = req_json.get('password', None) @@ -66,9 +69,20 @@ def update_user(user_id): return jsonify(data=user_schema.dump(user_to_update).data), 200 +@user.route('//update-attribute', methods=['PUT']) +def update_user_attribute(user_id): + user_to_update = get_user_by_id(user_id=user_id) + req_json = request.get_json() + update_dct, errors = user_update_schema_load_json.load(req_json) + if errors: + raise InvalidRequest(errors, status_code=400) + save_user_attribute(user_to_update, update_dict=update_dct) + return jsonify(data=user_schema.dump(user_to_update).data), 200 + + @user.route('//verify/password', methods=['POST']) def verify_user_password(user_id): - user_to_verify = get_model_users(user_id=user_id) + user_to_verify = get_user_by_id(user_id=user_id) txt_pwd = None try: @@ -92,7 +106,7 @@ def verify_user_password(user_id): @user.route('//verify/code', methods=['POST']) def verify_user_code(user_id): - user_to_verify = get_model_users(user_id=user_id) + user_to_verify = get_user_by_id(user_id=user_id) txt_code = None resp_json = request.get_json() @@ -120,7 +134,7 @@ def verify_user_code(user_id): @user.route('//sms-code', methods=['POST']) def send_user_sms_code(user_id): - user_to_send_to = get_model_users(user_id=user_id) + user_to_send_to = get_user_by_id(user_id=user_id) verify_code, errors = request_verify_code_schema.load(request.get_json()) secret_code = create_secret_code() @@ -149,7 +163,7 @@ def send_user_sms_code(user_id): @user.route('//change-email-verification', methods=['POST']) def send_user_confirm_new_email(user_id): - user_to_send_to = get_model_users(user_id=user_id) + user_to_send_to = get_user_by_id(user_id=user_id) email, errors = email_data_request_schema.load(request.get_json()) if errors: raise InvalidRequest(message=errors, status_code=400) @@ -178,7 +192,7 @@ def send_user_confirm_new_email(user_id): @user.route('//email-verification', methods=['POST']) def send_user_email_verification(user_id): - user_to_send_to = get_model_users(user_id=user_id) + user_to_send_to = get_user_by_id(user_id=user_id) secret_code = create_secret_code() create_user_code(user_to_send_to, secret_code, 'email') @@ -230,7 +244,7 @@ def send_already_registered_email(user_id): @user.route('/', methods=['GET']) @user.route('', methods=['GET']) def get_user(user_id=None): - users = get_model_users(user_id=user_id) + users = get_user_by_id(user_id=user_id) result = user_schema.dump(users, many=True) if isinstance(users, list) else user_schema.dump(users) return jsonify(data=result.data) @@ -239,7 +253,7 @@ def get_user(user_id=None): def set_permissions(user_id, service_id): # TODO fix security hole, how do we verify that the user # who is making this request has permission to make the request. - user = get_model_users(user_id=user_id) + user = get_user_by_id(user_id=user_id) service = dao_fetch_service_by_id(service_id=service_id) permissions, errors = permission_schema.load(request.get_json(), many=True) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 14b26d387..d9261fbb8 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -1,4 +1,5 @@ import json +import pytest from flask import url_for, current_app from freezegun import freeze_time @@ -180,6 +181,29 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service): assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', 'New User'), + ('email_address', 'newuser@mail.com'), + ('mobile_number', '+4407700900460') +]) +def test_put_user_attribute(client, sample_user, user_attribute, user_value): + assert getattr(sample_user, user_attribute) != user_value + update_dict = { + user_attribute: user_value + } + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + + resp = client.put( + url_for('user.update_user_attribute', user_id=sample_user.id), + data=json.dumps(update_dict), + headers=headers) + + assert resp.status_code == 200 + json_resp = json.loads(resp.get_data(as_text=True)) + assert json_resp['data'][user_attribute] == user_value + + def test_put_user_update_password(notify_api, notify_db, notify_db_session, From 3f10e59db3fd072ff28a774f22e13ff0196fef61 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Nov 2016 17:42:39 +0000 Subject: [PATCH 5/7] Add user dao method to update a single user attr --- app/dao/users_dao.py | 16 ++++++++++++---- tests/app/dao/test_users_dao.py | 29 ++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 1a240fecc..6654234a5 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -7,18 +7,26 @@ from app import db from app.models import (User, VerifyCode) +def _remove_values_for_keys_if_present(dict, keys): + for key in keys: + dict.pop(key, None) + + def create_secret_code(): return ''.join(map(str, random.sample(range(9), 5))) +def save_user_attribute(usr, update_dict={}): + db.session.query(User).filter_by(id=usr.id).update(update_dict) + db.session.commit() + + def save_model_user(usr, update_dict={}, pwd=None): if pwd: usr.password = pwd usr.password_changed_at = datetime.utcnow() if update_dict: - if update_dict.get('id'): - del update_dict['id'] - update_dict.pop('password_changed_at') + _remove_values_for_keys_if_present(update_dict, ['id', 'password_changed_at']) db.session.query(User).filter_by(id=usr.id).update(update_dict) else: db.session.add(usr) @@ -74,7 +82,7 @@ def delete_user_verify_codes(user): db.session.commit() -def get_model_users(user_id=None): +def get_user_by_id(user_id=None): if user_id: return User.query.filter_by(id=user_id).one() return User.query.filter_by().all() diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py index 7afcf72ce..7d1901f0d 100644 --- a/tests/app/dao/test_users_dao.py +++ b/tests/app/dao/test_users_dao.py @@ -7,12 +7,13 @@ import pytest from app.dao.users_dao import ( save_model_user, - get_model_users, + save_user_attribute, + get_user_by_id, delete_model_user, increment_failed_login_count, reset_failed_login_count, get_user_by_email, - delete_codes_older_created_more_than_a_day_ago + delete_codes_older_created_more_than_a_day_ago, ) from tests.app.conftest import sample_user as create_sample_user @@ -37,13 +38,13 @@ def test_create_user(notify_api, notify_db, notify_db_session): def test_get_all_users(notify_api, notify_db, notify_db_session, sample_user): assert User.query.count() == 1 - assert len(get_model_users()) == 1 + assert len(get_user_by_id()) == 1 email = "another.notify@digital.cabinet-office.gov.uk" another_user = create_sample_user(notify_db, notify_db_session, email=email) assert User.query.count() == 2 - assert len(get_model_users()) == 2 + assert len(get_user_by_id()) == 2 def test_get_user(notify_api, notify_db, notify_db_session): @@ -51,12 +52,12 @@ def test_get_user(notify_api, notify_db, notify_db_session): another_user = create_sample_user(notify_db, notify_db_session, email=email) - assert get_model_users(user_id=another_user.id).email_address == email + assert get_user_by_id(user_id=another_user.id).email_address == email def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid): try: - get_model_users(user_id=fake_uuid) + get_user_by_id(user_id=fake_uuid) pytest.fail("NoResultFound exception not thrown.") except NoResultFound as e: pass @@ -64,7 +65,7 @@ def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid def test_get_user_invalid_id(notify_api, notify_db, notify_db_session): try: - get_model_users(user_id="blah") + get_user_by_id(user_id="blah") pytest.fail("DataError exception not thrown.") except DataError: pass @@ -131,3 +132,17 @@ def make_verify_code(user, age=timedelta(hours=0), code="12335"): ) db.session.add(verify_code) db.session.commit() + + +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', 'New User'), + ('email_address', 'newuser@mail.com'), + ('mobile_number', '+4407700900460') +]) +def test_update_user_attribute(client, sample_user, user_attribute, user_value): + assert getattr(sample_user, user_attribute) != user_value + update_dict = { + user_attribute: user_value + } + save_user_attribute(sample_user, update_dict) + assert getattr(sample_user, user_attribute) == user_value From 8b64aa7e79f63a33aa4e9436f6ba1631f6b66575 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 10 Nov 2016 12:07:29 +0000 Subject: [PATCH 6/7] Use POST endpoint for updating a user attr --- app/user/rest.py | 2 +- tests/app/user/test_rest.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index e41e10877..a188f162e 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -69,7 +69,7 @@ def update_user(user_id): return jsonify(data=user_schema.dump(user_to_update).data), 200 -@user.route('//update-attribute', methods=['PUT']) +@user.route('/', methods=['POST']) def update_user_attribute(user_id): user_to_update = get_user_by_id(user_id=user_id) req_json = request.get_json() diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index d9261fbb8..951931e01 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -186,7 +186,7 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service): ('email_address', 'newuser@mail.com'), ('mobile_number', '+4407700900460') ]) -def test_put_user_attribute(client, sample_user, user_attribute, user_value): +def test_post_user_attribute(client, sample_user, user_attribute, user_value): assert getattr(sample_user, user_attribute) != user_value update_dict = { user_attribute: user_value @@ -194,7 +194,7 @@ def test_put_user_attribute(client, sample_user, user_attribute, user_value): auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( + resp = client.post( url_for('user.update_user_attribute', user_id=sample_user.id), data=json.dumps(update_dict), headers=headers) From f85ee547075baa83f20431b6fa9a022827f5abff Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 10 Nov 2016 13:09:25 +0000 Subject: [PATCH 7/7] Refactor stuff + stricter validation for updating only ALLOWED user attrs --- app/schemas.py | 5 +++-- tests/app/dao/test_users_dao.py | 5 +---- tests/app/test_schemas.py | 25 ++++++++++++++++++++++--- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index b9f75cb2c..7f52b9541 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -110,8 +110,9 @@ class UserUpdateAttributeSchema(BaseSchema): class Meta: model = models.User exclude = ( - "updated_at", "created_at", "user_to_service", - "_password", "verify_codes") + 'id', 'updated_at', 'created_at', 'user_to_service', + '_password', 'verify_codes', 'logged_in_at', 'password_changed_at', + 'failed_login_count', 'state', 'platform_admin') strict = True @validates('name') diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py index 7d1901f0d..cc814b160 100644 --- a/tests/app/dao/test_users_dao.py +++ b/tests/app/dao/test_users_dao.py @@ -64,11 +64,8 @@ def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid def test_get_user_invalid_id(notify_api, notify_db, notify_db_session): - try: + with pytest.raises(DataError): get_user_by_id(user_id="blah") - pytest.fail("DataError exception not thrown.") - except DataError: - pass def test_delete_users(notify_api, notify_db, notify_db_session, sample_user): diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py index 5f2bc4371..1d66f3290 100644 --- a/tests/app/test_schemas.py +++ b/tests/app/test_schemas.py @@ -1,5 +1,7 @@ import pytest +from marshmallow import ValidationError + def test_job_schema_doesnt_return_notifications(sample_notification_with_job): from app.schemas import job_schema @@ -32,7 +34,7 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key) ('email_address', 'newuser@mail.com'), ('mobile_number', '+4407700900460') ]) -def test_user_schema_accepts_valid_attributes(user_attribute, user_value): +def test_user_update_schema_accepts_valid_attribute_pairs(user_attribute, user_value): update_dict = { user_attribute: user_value } @@ -48,11 +50,28 @@ def test_user_schema_accepts_valid_attributes(user_attribute, user_value): ('email_address', 'bademail@...com'), ('mobile_number', '+44077009') ]) -def test_user_schema_rejects_invalid_attributes(user_attribute, user_value): +def test_user_update_schema_rejects_invalid_attribute_pairs(user_attribute, user_value): from app.schemas import user_update_schema_load_json update_dict = { user_attribute: user_value } - with pytest.raises(Exception): + with pytest.raises(ValidationError): data, errors = user_update_schema_load_json.load(update_dict) + + +@pytest.mark.parametrize('user_attribute', [ + 'id', 'updated_at', 'created_at', 'user_to_service', + '_password', 'verify_codes', 'logged_in_at', 'password_changed_at', + 'failed_login_count', 'state', 'platform_admin' +]) +def test_user_update_schema_rejects_disallowed_attribute_keys(user_attribute): + update_dict = { + user_attribute: 'not important' + } + from app.schemas import user_update_schema_load_json + + with pytest.raises(ValidationError) as excinfo: + data, errors = user_update_schema_load_json.load(update_dict) + + assert excinfo.value.messages['_schema'][0] == 'Unknown field name {}'.format(user_attribute)