From efec57db010ffbed0c235366ede850608aa3d263 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 6 Mar 2018 17:47:29 +0000 Subject: [PATCH 1/4] replace user_schema with serialize method on user model this is so that we can filter out inactive organisations and services note: can't remove user schema completely, as we still use it in POST /user to create new users --- app/models.py | 32 +++++++++++++++++++++++ app/organisation/rest.py | 7 ++--- app/schemas.py | 8 ++---- app/service/rest.py | 4 +-- app/user/rest.py | 24 ++++++++--------- tests/app/user/test_rest.py | 51 +++++++++++++++++++++++++++---------- 6 files changed, 87 insertions(+), 39 deletions(-) diff --git a/app/models.py b/app/models.py index e78fd18ea..7e064345a 100644 --- a/app/models.py +++ b/app/models.py @@ -133,6 +133,38 @@ class User(db.Model): def check_password(self, password): return check_hash(password, self._password) + def get_permissions(self): + from app.dao.permissions_dao import permission_dao + retval = {} + for x in permission_dao.get_permissions_by_user_id(self.id): + service_id = str(x.service_id) + if service_id not in retval: + retval[service_id] = [] + retval[service_id].append(x.permission) + return retval + + def serialize(self): + return { + 'id': self.id, + 'name': self.name, + 'email_address': self.email_address, + 'auth_type': self.auth_type, + 'current_session_id': self.current_session_id, + 'failed_login_count': self.failed_login_count, + 'logged_in_at': self.logged_in_at.strftime(DATETIME_FORMAT) if self.logged_in_at else None, + 'mobile_number': self.mobile_number, + 'organisations': [x.id for x in self.organisations if x.active], + 'password_changed_at': ( + self.password_changed_at.strftime('%Y-%m-%d %H:%M:%S.%f') + if self.password_changed_at + else None + ), + 'permissions': self.get_permissions(), + 'platform_admin': self.platform_admin, + 'services': [x.id for x in self.services if x.active], + 'state': self.state, + } + user_to_service = db.Table( 'user_to_service', diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 04100ce2a..d436fc656 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -20,7 +20,6 @@ from app.organisation.organisation_schema import ( post_link_service_to_organisation_schema, ) from app.schema_validation import validate -from app.schemas import user_schema organisation_blueprint = Blueprint('organisation', __name__) register_errors(organisation_blueprint) @@ -98,15 +97,13 @@ def get_organisation_services(organisation_id): @organisation_blueprint.route('//users/', methods=['POST']) def add_user_to_organisation(organisation_id, user_id): new_org_user = dao_add_user_to_organisation(organisation_id, user_id) - return jsonify(data=user_schema.dump(new_org_user).data), 200 + return jsonify(data=new_org_user.serialize()) @organisation_blueprint.route('//users', methods=['GET']) def get_organisation_users(organisation_id): org_users = dao_get_users_for_organisation(organisation_id) - - result = user_schema.dump(org_users, many=True) - return jsonify(data=result.data) + return jsonify(data=[x.serialize() for x in org_users]) @organisation_blueprint.route('/unique', methods=["GET"]) diff --git a/app/schemas.py b/app/schemas.py index 2131bb9b7..b9d3b5abf 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -665,19 +665,15 @@ class UnarchivedTemplateSchema(BaseSchema): raise ValidationError('Template has been deleted', 'template') -user_schema = UserSchema() -user_schema_load_json = UserSchema(load_json=True) +# should not be used on its own for dumping - only for loading +create_user_schema = UserSchema() user_update_schema_load_json = UserUpdateAttributeSchema(load_json=True, partial=True) user_update_password_schema_load_json = UserUpdatePasswordSchema(load_json=True, partial=True) service_schema = ServiceSchema() -service_schema_load_json = ServiceSchema(load_json=True) detailed_service_schema = DetailedServiceSchema() template_schema = TemplateSchema() -template_schema_load_json = TemplateSchema(load_json=True) api_key_schema = ApiKeySchema() -api_key_schema_load_json = ApiKeySchema(load_json=True) job_schema = JobSchema() -job_schema_load_json = JobSchema(load_json=True) sms_admin_notification_schema = SmsAdminNotificationSchema() sms_template_notification_schema = SmsTemplateNotificationSchema() job_sms_template_notification_schema = JobSmsTemplateNotificationSchema() diff --git a/app/service/rest.py b/app/service/rest.py index 5ed4a5dee..a36b50664 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -82,7 +82,6 @@ from app.service.send_notification import send_one_off_notification from app.schemas import ( service_schema, api_key_schema, - user_schema, permission_schema, notification_with_template_schema, notifications_filter_schema, @@ -258,8 +257,7 @@ def get_api_keys(service_id, key_id=None): @service_blueprint.route('//users', methods=['GET']) def get_users_for_service(service_id): fetched = dao_fetch_service_by_id(service_id) - result = user_schema.dump(fetched.users, many=True) - return jsonify(data=result.data) + return jsonify(data=[x.serialize() for x in fetched.users]) @service_blueprint.route('//users/', methods=['POST']) diff --git a/app/user/rest.py b/app/user/rest.py index b4011ebe7..c29ec8342 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -31,7 +31,7 @@ from app.notifications.process_notifications import ( ) from app.schemas import ( email_data_request_schema, - user_schema, + create_user_schema, permission_schema, user_update_schema_load_json, user_update_password_schema_load_json @@ -67,13 +67,14 @@ def handle_integrity_error(exc): @user_blueprint.route('', methods=['POST']) def create_user(): - user_to_create, errors = user_schema.load(request.get_json()) + user_to_create, errors = create_user_schema.load(request.get_json()) req_json = request.get_json() if not req_json.get('password', None): errors.update({'password': ['Missing data for required field.']}) raise InvalidRequest(errors, status_code=400) save_model_user(user_to_create, pwd=req_json.get('password')) - return jsonify(data=user_schema.dump(user_to_create).data), 201 + result = user_to_create.serialize() + return jsonify(data=result), 201 @user_blueprint.route('/', methods=['POST']) @@ -84,7 +85,7 @@ def update_user_attribute(user_id): 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 + return jsonify(data=user_to_update.serialize()), 200 @user_blueprint.route('//activate', methods=['POST']) @@ -95,14 +96,14 @@ def activate_user(user_id): user.state = 'active' save_model_user(user) - return jsonify(data=user_schema.dump(user).data), 200 + return jsonify(data=user.serialize()), 200 @user_blueprint.route('//reset-failed-login-count', methods=['POST']) def user_reset_failed_login_count(user_id): user_to_update = get_user_by_id(user_id=user_id) reset_failed_login_count(user_to_update) - return jsonify(data=user_schema.dump(user_to_update).data), 200 + return jsonify(data=user_to_update.serialize()), 200 @user_blueprint.route('//verify/password', methods=['POST']) @@ -324,8 +325,8 @@ def send_already_registered_email(user_id): @user_blueprint.route('', methods=['GET']) def get_user(user_id=None): 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) + result = [x.serialize() for x in users] if isinstance(users, list) else users.serialize() + return jsonify(data=result) @user_blueprint.route('//service//permission', methods=['POST']) @@ -350,9 +351,8 @@ def get_by_email(): error = 'Invalid request. Email query string param required' raise InvalidRequest(error, status_code=400) fetched_user = get_user_by_email(email) - result = user_schema.dump(fetched_user) - - return jsonify(data=result.data) + result = fetched_user.serialize() + return jsonify(data=result) @user_blueprint.route('/reset-password', methods=['POST']) @@ -392,7 +392,7 @@ def update_password(user_id): if errors: raise InvalidRequest(errors, status_code=400) update_user_password(user, pwd) - return jsonify(data=user_schema.dump(user).data), 200 + return jsonify(data=user.serialize()), 200 def _create_reset_password_url(email): diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index cd635cd5f..5b79cd61b 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -36,28 +36,53 @@ def test_get_user_list(admin_request, sample_service): assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) -def test_get_user(client, sample_service): +def test_get_user(admin_request, sample_service, sample_organisation): """ Tests GET endpoint '/' to retrieve a single service. """ sample_user = sample_service.users[0] - header = create_authorization_header() - resp = client.get(url_for('user.get_user', - user_id=sample_user.id), - headers=[header]) - assert resp.status_code == 200 - json_resp = json.loads(resp.get_data(as_text=True)) + sample_user.organisations = [sample_organisation] + json_resp = admin_request.get( + 'user.get_user', + user_id=sample_user.id + ) 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 sample_user.email_address == fetched['email_address'] - assert sample_user.state == fetched['state'] + assert fetched['id'] == str(sample_user.id) + assert fetched['name'] == sample_user.name + assert fetched['mobile_number'] == sample_user.mobile_number + assert fetched['email_address'] == sample_user.email_address + assert fetched['state'] == sample_user.state assert fetched['auth_type'] == SMS_AUTH_TYPE - assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) + assert fetched['permissions'].keys() == {str(sample_service.id)} + assert fetched['services'] == [str(sample_service.id)] + assert fetched['organisations'] == [str(sample_organisation.id)] + assert sorted(fetched['permissions'][str(sample_service.id)]) == sorted(expected_permissions) + + +def test_get_user_doesnt_return_inactive_services_and_orgs(admin_request, sample_service, sample_organisation): + """ + Tests GET endpoint '/' to retrieve a single service. + """ + sample_service.active = False + sample_organisation.active = False + + sample_user = sample_service.users[0] + sample_user.organisations = [sample_organisation] + + json_resp = admin_request.get( + 'user.get_user', + user_id=sample_user.id + ) + + fetched = json_resp['data'] + + assert fetched['id'] == str(sample_user.id) + assert fetched['services'] == [] + assert fetched['organisations'] == [] + assert fetched['permissions'] == {} def test_post_user(client, notify_db, notify_db_session): From 91fa4756458c0a285eef042c4de23fb76d306c4a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 7 Mar 2018 18:31:18 +0000 Subject: [PATCH 2/4] add new endpoint to get organisations and services for a user contains orgs, and unmapped services. the orgs contain nested services - services the user is a part of that belong to that org. the unmapped services are any services that the user is a part of that either don't have an org or have one that the user doesn't know about --- app/models.py | 2 +- app/user/rest.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 7e064345a..a9e63109a 100644 --- a/app/models.py +++ b/app/models.py @@ -120,7 +120,7 @@ class User(db.Model): organisations = db.relationship( 'Organisation', secondary='user_to_organisation', - backref=db.backref('user_to_organisation', lazy='dynamic')) + backref=db.backref('users', lazy='dynamic')) @property def password(self): diff --git a/app/user/rest.py b/app/user/rest.py index c29ec8342..207d19bfe 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -395,6 +395,45 @@ def update_password(user_id): return jsonify(data=user.serialize()), 200 +@user_blueprint.route('//organisations-and-services', methods=['GET']) +def get_organisations_and_services_for_user(user_id): + user = get_user_by_id(user_id=user_id) + + data = { + 'organisations': [ + { + 'name': org.name, + 'id': org.id, + 'services': [ + { + 'id': service.id, + 'name': service.name + } + for service in org.services + if service.active and user in service.users + ] + } + for org in user.organisations + ], + 'services_without_organisations': [ + { + 'id': service.id, + 'name': service.name + } for service in user.services + if ( + service.active and + # include services that either aren't in an organisation, or are in an organisation, + # but not one that the user can see. + ( + not service.organisation or + user not in service.organisation.users + ) + ) + ] + } + return jsonify(data) + + def _create_reset_password_url(email): data = json.dumps({'email': email, 'created_at': str(datetime.utcnow())}) url = '/new-password/' From 5871dee606c7cfa4693017e3a68c752413323b07 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 13 Mar 2018 13:07:02 +0000 Subject: [PATCH 3/4] use joinedload to only hit the database once per request also: * only include active orgs * write lots of tests --- app/dao/users_dao.py | 16 +++++ app/models.py | 4 +- app/user/rest.py | 12 ++-- tests/app/user/test_rest.py | 126 ++++++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 8 deletions(-) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 293c550bf..50113bbf6 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -1,6 +1,9 @@ from random import (SystemRandom) from datetime import (datetime, timedelta) + from sqlalchemy import func +from sqlalchemy.orm import joinedload + from app import db from app.models import (User, VerifyCode) @@ -113,3 +116,16 @@ def update_user_password(user, password): user.password_changed_at = datetime.utcnow() db.session.add(user) db.session.commit() + + +def get_user_and_accounts(user_id): + return User.query.filter( + User.id == user_id + ).options( + # eagerly load the user's services and organisations, and also the service's org and vice versa + # (so we can see if the user knows about it) + joinedload('services'), + joinedload('organisations'), + joinedload('organisations.services'), + joinedload('services.organisation'), + ).one() diff --git a/app/models.py b/app/models.py index a9e63109a..a1d06fa30 100644 --- a/app/models.py +++ b/app/models.py @@ -116,11 +116,11 @@ class User(db.Model): services = db.relationship( 'Service', secondary='user_to_service', - backref=db.backref('user_to_service', lazy='dynamic')) + backref='user_to_service') organisations = db.relationship( 'Organisation', secondary='user_to_organisation', - backref=db.backref('users', lazy='dynamic')) + backref='users') @property def password(self): diff --git a/app/user/rest.py b/app/user/rest.py index 207d19bfe..1b5fbf987 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -19,7 +19,8 @@ from app.dao.users_dao import ( create_secret_code, save_user_attribute, update_user_password, - count_user_verify_codes + count_user_verify_codes, + get_user_and_accounts ) from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id @@ -397,8 +398,7 @@ def update_password(user_id): @user_blueprint.route('//organisations-and-services', methods=['GET']) def get_organisations_and_services_for_user(user_id): - user = get_user_by_id(user_id=user_id) - + user = get_user_and_accounts(user_id) data = { 'organisations': [ { @@ -410,10 +410,10 @@ def get_organisations_and_services_for_user(user_id): 'name': service.name } for service in org.services - if service.active and user in service.users + if service.active and service in user.services ] } - for org in user.organisations + for org in user.organisations if org.active ], 'services_without_organisations': [ { @@ -426,7 +426,7 @@ def get_organisations_and_services_for_user(user_id): # but not one that the user can see. ( not service.organisation or - user not in service.organisation.users + service.organisation not in user.organisations ) ) ] diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 5b79cd61b..fc4d9632d 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -15,6 +15,7 @@ from app.models import ( ) from app.dao.permissions_dao import default_service_permissions from tests import create_authorization_header +from tests.app.db import create_service, create_organisation, create_user def test_get_user_list(admin_request, sample_service): @@ -605,3 +606,128 @@ def test_cannot_update_user_password_using_attributes_method(admin_request, samp _expected_status=400 ) assert resp['message']['_schema'] == ['Unknown field name password'] + + +def test_get_orgs_and_services_nests_services(admin_request, sample_user): + org1 = create_organisation(name='org1') + org2 = create_organisation(name='org2') + service1 = create_service(service_name='service1') + service2 = create_service(service_name='service2') + service3 = create_service(service_name='service3') + + org1.services = [service1, service2] + org2.services = [] + + sample_user.organisations = [org1, org2] + sample_user.services = [service1, service2, service3] + + resp = admin_request.get('user.get_organisations_and_services_for_user', user_id=sample_user.id) + + assert resp == { + 'organisations': [ + { + 'name': org1.name, + 'id': str(org1.id), + 'services': [ + { + 'name': service1.name, + 'id': str(service1.id) + }, + { + 'name': service2.name, + 'id': str(service2.id) + } + ] + }, + { + 'name': org2.name, + 'id': str(org2.id), + 'services': [] + } + ], + 'services_without_organisations': [ + { + 'name': service3.name, + 'id': str(service3.id) + } + ] + } + + +def test_get_orgs_and_services_only_returns_active(admin_request, sample_user): + org1 = create_organisation(name='org1', active=True) + org2 = create_organisation(name='org2', active=False) + + # in an active org + service1 = create_service(service_name='service1', active=True) + service2 = create_service(service_name='service2', active=False) + # active but in an inactive org + service3 = create_service(service_name='service3', active=True) + # not in an org + service4 = create_service(service_name='service4', active=True) + service5 = create_service(service_name='service5', active=False) + + org1.services = [service1, service2] + org2.services = [service3] + + sample_user.organisations = [org1, org2] + sample_user.services = [service1, service2, service3, service4, service5] + + resp = admin_request.get('user.get_organisations_and_services_for_user', user_id=sample_user.id) + + assert resp == { + 'organisations': [ + { + 'name': org1.name, + 'id': str(org1.id), + 'services': [ + { + 'name': service1.name, + 'id': str(service1.id) + } + ] + } + ], + 'services_without_organisations': [ + { + 'name': service4.name, + 'id': str(service4.id) + } + ] + } + + +def test_get_orgs_and_services_only_shows_users_orgs_and_services(admin_request, sample_user): + other_user = create_user(email='other@user.com') + + org1 = create_organisation(name='org1') + org2 = create_organisation(name='org2') + service1 = create_service(service_name='service1') + service2 = create_service(service_name='service2') + + org1.services = [service1] + + sample_user.organisations = [org2] + sample_user.services = [service1] + + other_user.organisations = [org1, org2] + other_user.services = [service1, service2] + + resp = admin_request.get('user.get_organisations_and_services_for_user', user_id=sample_user.id) + + assert resp == { + 'organisations': [ + { + 'name': org2.name, + 'id': str(org2.id), + 'services': [] + } + ], + # service1 belongs to org1, but the user doesn't know about org1 + 'services_without_organisations': [ + { + 'name': service1.name, + 'id': str(service1.id) + } + ] + } From 3ddd43b45be5cf5044cc18269c8a25e476452f7e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 15 Mar 2018 12:21:04 +0000 Subject: [PATCH 4/4] move the dict building up into a separate function --- app/user/rest.py | 62 ++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 1b5fbf987..646c21239 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -399,7 +399,39 @@ def update_password(user_id): @user_blueprint.route('//organisations-and-services', methods=['GET']) def get_organisations_and_services_for_user(user_id): user = get_user_and_accounts(user_id) - data = { + data = get_orgs_and_services(user) + return jsonify(data) + + +def _create_reset_password_url(email): + data = json.dumps({'email': email, 'created_at': str(datetime.utcnow())}) + url = '/new-password/' + return url_with_token(data, url, current_app.config) + + +def _create_verification_url(user): + data = json.dumps({'user_id': str(user.id), 'email': user.email_address}) + url = '/verify-email/' + return url_with_token(data, url, current_app.config) + + +def _create_confirmation_url(user, email_address): + data = json.dumps({'user_id': str(user.id), 'email': email_address}) + url = '/user-profile/email/confirm/' + return url_with_token(data, url, current_app.config) + + +def _create_2fa_url(user, secret_code, next_redir, email_auth_link_host): + data = json.dumps({'user_id': str(user.id), 'secret_code': secret_code}) + url = '/email-auth/' + ret = url_with_token(data, url, current_app.config, base_url=email_auth_link_host) + if next_redir: + ret += '?{}'.format(urlencode({'next': next_redir})) + return ret + + +def get_orgs_and_services(user): + return { 'organisations': [ { 'name': org.name, @@ -431,31 +463,3 @@ def get_organisations_and_services_for_user(user_id): ) ] } - return jsonify(data) - - -def _create_reset_password_url(email): - data = json.dumps({'email': email, 'created_at': str(datetime.utcnow())}) - url = '/new-password/' - return url_with_token(data, url, current_app.config) - - -def _create_verification_url(user): - data = json.dumps({'user_id': str(user.id), 'email': user.email_address}) - url = '/verify-email/' - return url_with_token(data, url, current_app.config) - - -def _create_confirmation_url(user, email_address): - data = json.dumps({'user_id': str(user.id), 'email': email_address}) - url = '/user-profile/email/confirm/' - return url_with_token(data, url, current_app.config) - - -def _create_2fa_url(user, secret_code, next_redir, email_auth_link_host): - data = json.dumps({'user_id': str(user.id), 'secret_code': secret_code}) - url = '/email-auth/' - ret = url_with_token(data, url, current_app.config, base_url=email_auth_link_host) - if next_redir: - ret += '?{}'.format(urlencode({'next': next_redir})) - return ret