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 c14a17fea..7cdb9f4d8 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(request_helper.check_proxy_header_before_request) @@ -522,3 +499,28 @@ 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): + 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) 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/app/main/views/invites.py b/app/main/views/invites.py index 4f1c88f7a..43e994e06 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -72,6 +72,16 @@ def accept_invite(token): if existing_user in service_users: return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) else: + 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, invited_user.permissions) 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/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/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/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/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 528c6a56d..cebf16b43 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/wsgi.py b/application.py similarity index 55% rename from wsgi.py rename to application.py index 35092e5f3..f4ddca8f7 100644 --- a/wsgi.py +++ b/application.py @@ -1,13 +1,15 @@ -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('app') -if __name__ == "__main__": - application.run() +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/requirements.txt b/requirements.txt index 84dc5531f..941f5c1a5 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/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index aa2f0de30..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,12 +19,12 @@ 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') 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 +35,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( @@ -48,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') @@ -396,3 +397,132 @@ 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_accepts_and_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 + + 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, + _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) + + +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') 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', [ diff --git a/tests/conftest.py b/tests/conftest.py index f26951a06..9332ba77d 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 @@ -34,17 +34,16 @@ import json @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')