From 4aeb57567a0489de3a8e0e5586b95938926eb896 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 6 Nov 2017 13:07:21 +0000 Subject: [PATCH 1/7] remove flask-script flask-script has been deprecated by the internal flask.cli module, but making this carries a few changes with it * you should add FLASK_APP=application.py and FLASK_DEBUG=1 to your environment.sh. * instead of using `python app.py runserver`, now you must run `flask run -p 6012`. The -p command is important - the port must be set before the config is loaded, so that it can live reload nicely. (https://github.com/pallets/flask/issues/2113#issuecomment-268014481) * find available commands by just running `flask`. * run them using flask. eg `flask list_routes` * define new tasks by giving them the decorator `@app.cli.command('task-name')`. Task name isn't needed if it's just the same as the function name. Alternatively, if app isn't available in the current scope, you can invoke the decorator directly, as seen in app/commands.py --- README.md | 3 ++ app.py | 20 ------- app/__init__.py | 69 ++++++++++++------------- app/commands.py | 11 ++++ application.py | 6 +++ requirements.txt | 1 - scripts/run_app.sh | 2 +- tests/app/main/test_choose_time_form.py | 2 +- tests/conftest.py | 13 +++-- wsgi.py | 10 ++-- 10 files changed, 69 insertions(+), 68 deletions(-) delete mode 100644 app.py create mode 100644 app/commands.py create mode 100644 application.py diff --git a/README.md b/README.md index cca5660fe..15975b530 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,9 @@ export DANGEROUS_SALT='dev-notify-salt' export SECRET_KEY='notify-secret-key' export DESKPRO_API_HOST="some-host" export DESKPRO_API_KEY="some-key" +export FLASK_APP=application.py +export FLASK_DEBUG=1 +export WERKZEUG_DEBUG_PIN=off "> environment.sh ``` diff --git a/app.py b/app.py deleted file mode 100644 index 2b856fa3f..000000000 --- a/app.py +++ /dev/null @@ -1,20 +0,0 @@ -import os -from flask_script import Manager, Server -from app import create_app - - -application = create_app() -manager = Manager(application) -port = int(os.environ.get('PORT', 6012)) -manager.add_command("runserver", Server(host='0.0.0.0', port=port)) - - -@manager.command -def list_routes(): - """List URLs of all application routes.""" - for rule in sorted(application.url_map.iter_rules(), key=lambda r: r.rule): - print("{:10} {}".format(", ".join(rule.methods - set(['OPTIONS', 'HEAD'])), rule.rule)) - - -if __name__ == '__main__': - manager.run() diff --git a/app/__init__.py b/app/__init__.py index 8cf3c7147..816ef58a7 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -7,7 +7,6 @@ import itertools import ago from itsdangerous import BadSignature from flask import ( - Flask, session, render_template, make_response, @@ -37,6 +36,7 @@ from werkzeug.exceptions import abort from werkzeug.local import LocalProxy from app import proxy_fix +from app.config import configs from app.asset_fingerprinter import AssetFingerprinter from app.its_dangerous_session import ItsdangerousSessionInterface from app.notify_client.service_api_client import ServiceAPIClient @@ -54,8 +54,8 @@ from app.notify_client.models import AnonymousUser from app.notify_client.letter_jobs_client import LetterJobsClient from app.notify_client.inbound_number_client import InboundNumberClient from app.notify_client.billing_api_client import BillingAPIClient +from app.commands import setup_commands from app.utils import get_cdn_domain - from app.utils import gmt_timezones login_manager = LoginManager() @@ -82,10 +82,8 @@ billing_api_client = BillingAPIClient() current_service = LocalProxy(partial(_lookup_req_object, 'service')) -def create_app(): - from app.config import configs - - application = Flask(__name__) +def create_app(application): + setup_commands(application) notify_environment = os.environ['NOTIFY_ENVIRONMENT'] @@ -128,40 +126,12 @@ def create_app(): application.session_interface = ItsdangerousSessionInterface() - application.add_template_filter(format_datetime) - application.add_template_filter(format_datetime_24h) - application.add_template_filter(format_datetime_normal) - application.add_template_filter(format_datetime_short) - application.add_template_filter(format_time) - application.add_template_filter(valid_phone_number) - application.add_template_filter(linkable_name) - application.add_template_filter(format_date) - application.add_template_filter(format_date_normal) - application.add_template_filter(format_date_short) - application.add_template_filter(format_datetime_relative) - application.add_template_filter(format_delta) - application.add_template_filter(format_notification_status) - application.add_template_filter(format_notification_status_as_time) - application.add_template_filter(format_notification_status_as_field_status) - application.add_template_filter(format_notification_status_as_url) - application.add_template_filter(formatted_list) - application.add_template_filter(nl2br) - application.add_template_filter(format_phone_number_human_readable) - - application.after_request(useful_headers_after_request) - application.after_request(save_service_after_request) - application.before_request(load_service_before_request) - - @application.context_processor - def _attach_current_service(): - return {'current_service': current_service} + add_template_filters(application) register_errorhandlers(application) setup_event_handlers() - return application - def init_csrf(application): csrf.init_app(application) @@ -185,6 +155,13 @@ def init_csrf(application): def init_app(application): + application.after_request(useful_headers_after_request) + application.after_request(save_service_after_request) + application.before_request(load_service_before_request) + + @application.context_processor + def _attach_current_service(): + return {'current_service': current_service} @application.before_request def record_start_time(): @@ -519,3 +496,25 @@ def setup_event_handlers(): from app.event_handlers import on_user_logged_in user_logged_in.connect(on_user_logged_in) + + +def add_template_filters(application): + application.add_template_filter(format_datetime) + application.add_template_filter(format_datetime_24h) + application.add_template_filter(format_datetime_normal) + application.add_template_filter(format_datetime_short) + application.add_template_filter(format_time) + application.add_template_filter(valid_phone_number) + application.add_template_filter(linkable_name) + application.add_template_filter(format_date) + application.add_template_filter(format_date_normal) + application.add_template_filter(format_date_short) + application.add_template_filter(format_datetime_relative) + application.add_template_filter(format_delta) + application.add_template_filter(format_notification_status) + application.add_template_filter(format_notification_status_as_time) + application.add_template_filter(format_notification_status_as_field_status) + application.add_template_filter(format_notification_status_as_url) + application.add_template_filter(formatted_list) + application.add_template_filter(nl2br) + application.add_template_filter(format_phone_number_human_readable) diff --git a/app/commands.py b/app/commands.py new file mode 100644 index 000000000..c13dfee18 --- /dev/null +++ b/app/commands.py @@ -0,0 +1,11 @@ +from flask import current_app + + +def list_routes(): + """List URLs of all application routes.""" + for rule in sorted(current_app.url_map.iter_rules(), key=lambda r: r.rule): + print("{:10} {}".format(", ".join(rule.methods - set(['OPTIONS', 'HEAD'])), rule.rule)) + + +def setup_commands(application): + application.cli.command('list-routes')(list_routes) diff --git a/application.py b/application.py new file mode 100644 index 000000000..f46d830c6 --- /dev/null +++ b/application.py @@ -0,0 +1,6 @@ +from flask import Flask +from app import create_app + +app = Flask('app') + +create_app(app) diff --git a/requirements.txt b/requirements.txt index 27ed7fe20..398a94219 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,5 @@ ago==0.0.92 Flask==0.12.2 -Flask-Script==2.0.5 Flask-WTF==0.14.2 Flask-Login==0.4.0 diff --git a/scripts/run_app.sh b/scripts/run_app.sh index 1e3c4bf73..67eb6dd17 100755 --- a/scripts/run_app.sh +++ b/scripts/run_app.sh @@ -1,4 +1,4 @@ #!/bin/bash source environment.sh -python3 app.py runserver +flask run -p 6012 diff --git a/tests/app/main/test_choose_time_form.py b/tests/app/main/test_choose_time_form.py index f67af5402..77e9449f0 100644 --- a/tests/app/main/test_choose_time_form.py +++ b/tests/app/main/test_choose_time_form.py @@ -34,7 +34,7 @@ def test_form_contains_next_24h(): @freeze_time("2016-01-01 11:09:00.061258") -def test_form_defaults_to_now(): +def test_form_defaults_to_now(client): assert ChooseTimeForm().scheduled_for.data == '' diff --git a/tests/conftest.py b/tests/conftest.py index d3a2a1a7d..8f9273fc1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,7 +5,7 @@ from unittest.mock import Mock import pytest from notifications_python_client.errors import HTTPError -from flask import url_for +from flask import url_for, Flask from bs4 import BeautifulSoup from app import create_app @@ -31,17 +31,16 @@ from . import ( @pytest.fixture(scope='session') def app_(request): - app = create_app() + app = Flask('app') + create_app(app) ctx = app.app_context() ctx.push() - def teardown(): - ctx.pop() - - request.addfinalizer(teardown) app.test_client_class = TestClient - return app + yield app + + ctx.pop() @pytest.fixture(scope='function') diff --git a/wsgi.py b/wsgi.py index 35092e5f3..4fc722299 100644 --- a/wsgi.py +++ b/wsgi.py @@ -1,13 +1,17 @@ -from whitenoise import WhiteNoise import os -from app import create_app # noqa +from flask import Flask +from whitenoise import WhiteNoise + +from app import create_app PROJECT_ROOT = os.path.dirname(os.path.abspath(__file__)) STATIC_ROOT = os.path.join(PROJECT_ROOT, 'app', 'static') STATIC_URL = 'static/' -application = WhiteNoise(create_app(), STATIC_ROOT, STATIC_URL) +app = Flask(__name__) +create_app(app) +application = WhiteNoise(app, STATIC_ROOT, STATIC_URL) if __name__ == "__main__": application.run() From 05357027072e809d891b228bb5704fb62791fd17 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Nov 2017 11:23:15 +0000 Subject: [PATCH 2/7] ensure flask app has right name --- wsgi.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/wsgi.py b/wsgi.py index 4fc722299..f849e9c8c 100644 --- a/wsgi.py +++ b/wsgi.py @@ -9,9 +9,6 @@ PROJECT_ROOT = os.path.dirname(os.path.abspath(__file__)) STATIC_ROOT = os.path.join(PROJECT_ROOT, 'app', 'static') STATIC_URL = 'static/' -app = Flask(__name__) +app = Flask('app') create_app(app) application = WhiteNoise(app, STATIC_ROOT, STATIC_URL) - -if __name__ == "__main__": - application.run() From 2cd77e628e102b1a2ebc64295abc62f844d58563 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Nov 2017 11:50:13 +0000 Subject: [PATCH 3/7] remove wsgi.py - always serve with whitenoise We're now running our app as a wsgi app locally, so don't need to distinguish between the two processes by having wsgi and application.py whitenoise just serves static files nicely - we don't lose anything by doing that locally. --- app/main/views/notifications.py | 17 ----------------- application.py | 9 +++++++++ manifest-base.yml | 2 +- manifest-prototype-base.yml | 2 +- wsgi.py | 14 -------------- 5 files changed, 11 insertions(+), 33 deletions(-) delete mode 100644 wsgi.py diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index cb52d5b34..4fa389987 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -4,7 +4,6 @@ from flask import ( jsonify, request, url_for, - current_app ) from flask_login import login_required @@ -21,27 +20,11 @@ from app.utils import ( get_template, get_time_left, get_letter_timings, - REQUESTED_STATUSES, FAILURE_STATUSES, - SENDING_STATUSES, DELIVERED_STATUSES, ) -def get_status_arg(filter_args): - if 'status' not in filter_args or not filter_args['status']: - return REQUESTED_STATUSES - elif filter_args['status'] == 'sending': - return SENDING_STATUSES - elif filter_args['status'] == 'delivered': - return DELIVERED_STATUSES - elif filter_args['status'] == 'failed': - return FAILURE_STATUSES - else: - current_app.logger.info('Unrecognised status filter: {}'.format(filter_args['status'])) - return REQUESTED_STATUSES - - @main.route("/services//notification/") @login_required @user_has_permissions('view_activity', admin_override=True) diff --git a/application.py b/application.py index f46d830c6..f4ddca8f7 100644 --- a/application.py +++ b/application.py @@ -1,6 +1,15 @@ +import os + from flask import Flask +from whitenoise import WhiteNoise + from app import create_app +PROJECT_ROOT = os.path.dirname(os.path.abspath(__file__)) +STATIC_ROOT = os.path.join(PROJECT_ROOT, 'app', 'static') +STATIC_URL = 'static/' + app = Flask('app') create_app(app) +application = WhiteNoise(app, STATIC_ROOT, STATIC_URL) diff --git a/manifest-base.yml b/manifest-base.yml index bf16b7af8..9f6d0d60f 100644 --- a/manifest-base.yml +++ b/manifest-base.yml @@ -1,7 +1,7 @@ --- buildpack: python_buildpack -command: scripts/run_app_paas.sh gunicorn -c /home/vcap/app/gunicorn_config.py --error-logfile /home/vcap/logs/gunicorn_error.log -w 5 -b 0.0.0.0:$PORT wsgi +command: scripts/run_app_paas.sh gunicorn -c /home/vcap/app/gunicorn_config.py --error-logfile /home/vcap/logs/gunicorn_error.log -w 5 -b 0.0.0.0:$PORT application services: - notify-aws - notify-config diff --git a/manifest-prototype-base.yml b/manifest-prototype-base.yml index 0cd3aac50..cf28cbb6a 100644 --- a/manifest-prototype-base.yml +++ b/manifest-prototype-base.yml @@ -1,7 +1,7 @@ --- buildpack: python_buildpack -command: scripts/run_app_paas.sh gunicorn -w 5 -b 0.0.0.0:$PORT wsgi +command: scripts/run_app_paas.sh gunicorn -w 5 -b 0.0.0.0:$PORT application services: - notify-aws - notify-config diff --git a/wsgi.py b/wsgi.py deleted file mode 100644 index f849e9c8c..000000000 --- a/wsgi.py +++ /dev/null @@ -1,14 +0,0 @@ -import os - -from flask import Flask -from whitenoise import WhiteNoise - -from app import create_app - -PROJECT_ROOT = os.path.dirname(os.path.abspath(__file__)) -STATIC_ROOT = os.path.join(PROJECT_ROOT, 'app', 'static') -STATIC_URL = 'static/' - -app = Flask('app') -create_app(app) -application = WhiteNoise(app, STATIC_ROOT, STATIC_URL) From ca373e9d88e57946d2565d78bfefe6c944d81875 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 15:54:49 +0000 Subject: [PATCH 4/7] add templates in a loop --- app/__init__.py | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 816ef58a7..434f148c8 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -499,22 +499,25 @@ def setup_event_handlers(): def add_template_filters(application): - application.add_template_filter(format_datetime) - application.add_template_filter(format_datetime_24h) - application.add_template_filter(format_datetime_normal) - application.add_template_filter(format_datetime_short) - application.add_template_filter(format_time) - application.add_template_filter(valid_phone_number) - application.add_template_filter(linkable_name) - application.add_template_filter(format_date) - application.add_template_filter(format_date_normal) - application.add_template_filter(format_date_short) - application.add_template_filter(format_datetime_relative) - application.add_template_filter(format_delta) - application.add_template_filter(format_notification_status) - application.add_template_filter(format_notification_status_as_time) - application.add_template_filter(format_notification_status_as_field_status) - application.add_template_filter(format_notification_status_as_url) - application.add_template_filter(formatted_list) - application.add_template_filter(nl2br) - application.add_template_filter(format_phone_number_human_readable) + for fn in [ + format_datetime, + format_datetime_24h, + format_datetime_normal, + format_datetime_short, + format_time, + valid_phone_number, + linkable_name, + format_date, + format_date_normal, + format_date_short, + format_datetime_relative, + format_delta, + format_notification_status, + format_notification_status_as_time, + format_notification_status_as_field_status, + format_notification_status_as_url, + formatted_list, + nl2br, + format_phone_number_human_readable, + ]: + application.add_template_filter(fn) From ab4504f5173bad0acece089a57785a53caf806ff Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 15 Nov 2017 14:59:03 +0000 Subject: [PATCH 5/7] fix logged in user not having auth type set from invite --- app/main/views/invites.py | 2 ++ tests/app/main/views/test_accept_invite.py | 32 ++++++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 4f1c88f7a..59ff29227 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -72,6 +72,8 @@ def accept_invite(token): if existing_user in service_users: return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) else: + if invited_user.auth_type != existing_user.auth_type: + user_api_client.update_user_attribute(existing_user.id, auth_type=invited_user.auth_type) user_api_client.add_user_to_service(invited_user.service, existing_user.id, invited_user.permissions) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index aa2f0de30..bbbc115e0 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -24,7 +24,6 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( mocker.patch('app.main.views.invites.check_token') expected_service = service_one['id'] - expected_redirect_location = 'http://localhost/services/{}/dashboard'.format(expected_service) expected_permissions = ['send_messages', 'manage_service', 'manage_api_keys'] response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) @@ -35,7 +34,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id, expected_permissions) assert response.status_code == 302 - assert response.location == expected_redirect_location + assert response.location == url_for('main.service_dashboard', service_id=expected_service, _external=True) def test_existing_user_with_no_permissions_accept_invite( @@ -396,3 +395,32 @@ def test_gives_message_if_token_has_expired( assert response.status_code == 400 assert 'Your invitation to GOV.UK Notify has expired' in page.find('h1').text assert not mock_check_invite_token.called + + +def test_existing_user_accept_sets_email_auth( + client_request, + api_user_active, + service_one, + sample_invite, + mock_get_user_by_email, + mock_get_users_by_service, + mock_accept_invite, + mock_update_user_attribute, + mock_add_user_to_service, + mocker +): + mocker.patch('app.main.views.invites.check_token') + sample_invite['email_address'] = api_user_active.email_address + sample_invite['auth_type'] = 'email_auth' + invited_user = InvitedUser(**sample_invite) + mocker.patch('app.invite_api_client.check_token', return_value=invited_user) + + response = client_request.get( + 'main.accept_invite', + token='thisisnotarealtoken', + _expected_status=302, + _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), + ) + + mock_update_user_attribute.assert_called_with(api_user_active.id, auth_type='email_auth') + mock_add_user_to_service.assert_called_with(ANY, api_user_active.id, ANY) From 8f4081bdb461c3e1fa66a729e2619542061babf6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 15 Nov 2017 16:04:50 +0000 Subject: [PATCH 6/7] Add a hint to explain why SMS auth is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we’re going to ‘disable’ radio buttons then we should always tell users why the radio button is disabled. This is what we found with the API key choices anyway. --- app/templates/views/manage-users/permissions.html | 12 ++++++++++-- tests/app/main/views/test_manage_users.py | 14 +++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index e23eef804..5d98cd0f2 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -23,5 +23,13 @@ {% if service_has_email_auth %} - {{ radios(form.login_authentication, disable=['sms_auth' if user_has_no_mobile_number]) }} -{% endif %} \ No newline at end of file + {% if user_has_no_mobile_number %} + {{ radios( + form.login_authentication, + disable=['sms_auth'], + option_hints={'sms_auth': 'Not available because this team member hasn’t added a phone number to their profile'|safe} + ) }} + {% else %} + {{ radios(form.login_authentication) }} + {% endif %} +{% endif %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index c54da8a70..3fb303e31 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -114,20 +114,29 @@ def test_manage_users_page_shows_member_auth_type_if_service_has_email_auth_acti assert bool(page.select_one('.tick-cross-list-hint')) == displays_auth_type -@pytest.mark.parametrize('user, sms_option_disabled', [ +@pytest.mark.parametrize('user, sms_option_disabled, expected_label', [ ( active_user_no_mobile, True, + """ + Text message code + Not available because this team member hasn’t added a + phone number to their profile + """, ), ( active_user_with_permissions, False, + """ + Text message code + """, ), ]) def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( client_request, user, sms_option_disabled, + expected_label, service_one, mocker ): @@ -142,6 +151,9 @@ def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( sms_auth_radio_button = page.select_one('input[value="sms_auth"]') assert sms_auth_radio_button.has_attr("disabled") == sms_option_disabled + assert normalize_spaces( + page.select_one('label[for=login_authentication-0]').text + ) == normalize_spaces(expected_label) @pytest.mark.parametrize('endpoint, extra_args, expected_checkboxes', [ From ddf88b70c06e21be2883e6b3d2ceb494da8f9813 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 15 Nov 2017 17:19:32 +0000 Subject: [PATCH 7/7] clean up logic around existing users accepting invites * if the service issuing the invite does not have permission to edit auth types, don't let them do anything. This will stop them turning existing email_auth users back to sms auth * if the user hasn't got a mobile number, but the invite is for sms login, don't do anything either. They won't have a phone number if they signed up via an email_auth invite previously. in these cases, we accept the invite and add the user to the service as normal, however, just don't update the user's auth type. --- app/main/views/invites.py | 10 +- app/main/views/manage_users.py | 5 +- app/main/views/user_profile.py | 9 +- app/templates/views/service-settings.html | 2 +- tests/app/main/views/test_accept_invite.py | 114 +++++++++++++++++++-- 5 files changed, 122 insertions(+), 18 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 59ff29227..43e994e06 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -72,7 +72,15 @@ def accept_invite(token): if existing_user in service_users: return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) else: - if invited_user.auth_type != existing_user.auth_type: + service = service_api_client.get_service(invited_user.service)['data'] + # if the service you're being added to can modify auth type, then check if this is relevant + if 'email_auth' in service['permissions'] and ( + # they have a phone number, we want them to start using it. if they dont have a mobile we just + # ignore that option of the invite + (existing_user.mobile_number and invited_user.auth_type == 'sms_auth') or + # we want them to start sending emails. it's always valid, so lets always update + invited_user.auth_type == 'email_auth' + ): user_api_client.update_user_attribute(existing_user.id, auth_type=invited_user.auth_type) user_api_client.add_user_to_service(invited_user.service, existing_user.id, diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 40e1dc047..440ed6031 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -102,10 +102,7 @@ def edit_user_permissions(service_id, user_id): permissions=set(get_permissions_from_form(form)), ) if service_has_email_auth: - user_api_client.update_user_attribute( - user_id, - auth_type=form.login_authentication.data - ) + user_api_client.update_user_attribute(user_id, auth_type=form.login_authentication.data) return redirect(url_for('.manage_users', service_id=service_id)) return render_template( diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index d063f9a91..0d583e4b2 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -47,8 +47,7 @@ def user_profile_name(): form = ChangeNameForm(new_name=current_user.name) if form.validate_on_submit(): - user_api_client.update_user_attribute(current_user.id, - name=form.new_name.data) + user_api_client.update_user_attribute(current_user.id, name=form.new_name.data) return redirect(url_for('.user_profile')) return render_template( @@ -114,8 +113,7 @@ def user_profile_email_confirm(token): token_data = json.loads(token_data) user_id = token_data['user_id'] new_email = token_data['email'] - user_api_client.update_user_attribute(user_id, - email_address=new_email) + user_api_client.update_user_attribute(user_id, email_address=new_email) session.pop(NEW_EMAIL, None) return redirect(url_for('.user_profile')) @@ -183,8 +181,7 @@ def user_profile_mobile_number_confirm(): mobile_number = session[NEW_MOBILE] del session[NEW_MOBILE] del session[NEW_MOBILE_PASSWORD_CONFIRMED] - user_api_client.update_user_attribute(current_user.id, - mobile_number=mobile_number) + user_api_client.update_user_attribute(current_user.id, mobile_number=mobile_number) return redirect(url_for('.user_profile')) return render_template( diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 1eb75ee21..b3262b1c3 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -262,7 +262,7 @@ {% endif %}
  • - {{ 'Stop email auth' if 'email_auth' in current_service.permissions else 'Allow email auth' }} + {{ 'Stop editing user auth' if 'email_auth' in current_service.permissions else 'Allow editing user auth' }}
    • {% if current_service.active %} diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index bbbc115e0..ae94139c8 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -4,8 +4,8 @@ from unittest.mock import ANY from itsdangerous import SignatureExpired import app - from app.notify_client.models import InvitedUser + from tests.conftest import sample_invite as create_sample_invite from tests.conftest import mock_check_invite_token as mock_check_token_invite @@ -19,6 +19,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( mock_get_users_by_service, mock_accept_invite, mock_add_user_to_service, + mock_get_service, mocker, ): mocker.patch('app.main.views.invites.check_token') @@ -47,6 +48,7 @@ def test_existing_user_with_no_permissions_accept_invite( mock_get_user_by_email, mock_get_users_by_service, mock_add_user_to_service, + mock_get_service, ): mocker.patch('app.main.views.invites.check_token') @@ -397,7 +399,7 @@ def test_gives_message_if_token_has_expired( assert not mock_check_invite_token.called -def test_existing_user_accept_sets_email_auth( +def test_existing_user_accepts_and_sets_email_auth( client_request, api_user_active, service_one, @@ -411,11 +413,13 @@ def test_existing_user_accept_sets_email_auth( ): mocker.patch('app.main.views.invites.check_token') sample_invite['email_address'] = api_user_active.email_address - sample_invite['auth_type'] = 'email_auth' - invited_user = InvitedUser(**sample_invite) - mocker.patch('app.invite_api_client.check_token', return_value=invited_user) - response = client_request.get( + service_one['permissions'].append('email_auth') + sample_invite['auth_type'] = 'email_auth' + mocker.patch('app.main.views.invites.service_api_client.get_service', return_value={'data': service_one}) + mocker.patch('app.invite_api_client.check_token', return_value=InvitedUser(**sample_invite)) + + client_request.get( 'main.accept_invite', token='thisisnotarealtoken', _expected_status=302, @@ -424,3 +428,101 @@ def test_existing_user_accept_sets_email_auth( mock_update_user_attribute.assert_called_with(api_user_active.id, auth_type='email_auth') mock_add_user_to_service.assert_called_with(ANY, api_user_active.id, ANY) + + +def test_existing_user_doesnt_get_auth_changed_by_service_without_permission( + client_request, + api_user_active, + service_one, + sample_invite, + mock_get_user_by_email, + mock_get_users_by_service, + mock_accept_invite, + mock_update_user_attribute, + mock_add_user_to_service, + mocker +): + mocker.patch('app.main.views.invites.check_token') + sample_invite['email_address'] = api_user_active.email_address + + assert 'email_auth' not in service_one['permissions'] + + sample_invite['auth_type'] = 'email_auth' + mocker.patch('app.main.views.invites.service_api_client.get_service', return_value={'data': service_one}) + mocker.patch('app.invite_api_client.check_token', return_value=InvitedUser(**sample_invite)) + + client_request.get( + 'main.accept_invite', + token='thisisnotarealtoken', + _expected_status=302, + _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), + ) + + assert not mock_update_user_attribute.called + + +def test_existing_email_auth_user_without_phone_cannot_set_sms_auth( + client_request, + api_user_active, + service_one, + sample_invite, + mock_get_users_by_service, + mock_accept_invite, + mock_update_user_attribute, + mock_add_user_to_service, + mocker +): + mocker.patch('app.main.views.invites.check_token') + sample_invite['email_address'] = api_user_active.email_address + + service_one['permissions'].append('email_auth') + + api_user_active.auth_type = 'email_auth' + api_user_active.mobile_number = None + sample_invite['auth_type'] = 'sms_auth' + + mocker.patch('app.main.views.invites.user_api_client.get_user_by_email', return_value=api_user_active) + mocker.patch('app.main.views.invites.service_api_client.get_service', return_value={'data': service_one}) + mocker.patch('app.invite_api_client.check_token', return_value=InvitedUser(**sample_invite)) + + client_request.get( + 'main.accept_invite', + token='thisisnotarealtoken', + _expected_status=302, + _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), + ) + + assert not mock_update_user_attribute.called + + +def test_existing_email_auth_user_with_phone_can_set_sms_auth( + client_request, + api_user_active, + service_one, + sample_invite, + mock_get_users_by_service, + mock_accept_invite, + mock_update_user_attribute, + mock_add_user_to_service, + mocker +): + mocker.patch('app.main.views.invites.check_token') + sample_invite['email_address'] = api_user_active.email_address + + service_one['permissions'].append('email_auth') + sample_invite['auth_type'] = 'sms_auth' + api_user_active.auth_type = 'email_auth' + api_user_active.mobile_number = '07700900001' + + mocker.patch('app.main.views.invites.user_api_client.get_user_by_email', return_value=api_user_active) + mocker.patch('app.main.views.invites.service_api_client.get_service', return_value={'data': service_one}) + mocker.patch('app.invite_api_client.check_token', return_value=InvitedUser(**sample_invite)) + + client_request.get( + 'main.accept_invite', + token='thisisnotarealtoken', + _expected_status=302, + _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), + ) + + mock_update_user_attribute.assert_called_with(api_user_active.id, auth_type='sms_auth')