Merge branch 'master' into add_proxy_header_check

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
 ```
 

app.py

@@ -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()

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)

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)

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)

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(

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/<service_id>/notification/<uuid:notification_id>")
 @login_required
 @user_has_permissions('view_activity', admin_override=True)

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(

app/templates/views/manage-users/permissions.html

@@ -23,5 +23,13 @@
 </div>
 
 {% if service_has_email_auth %}
-  {{ radios(form.login_authentication, disable=['sms_auth' if user_has_no_mobile_number]) }}
-{% endif %}
+  {% 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&nbsp;number to their profile'|safe}
+    ) }}
+  {% else %}
+    {{ radios(form.login_authentication) }}
+  {% endif %}
+{% endif %}

app/templates/views/service-settings.html

@@ -262,7 +262,7 @@
         {% endif %}
         <li class="bottom-gutter">
           <a href="{{ url_for('.service_switch_email_auth', service_id=current_service.id) }}" class="button">
-            {{ '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' }}
           </a>
         </li>
         {% if current_service.active %}

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)

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

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

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
 

scripts/run_app.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
 
 source environment.sh
-python3 app.py runserver
+flask run -p 6012

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 == ''
 
 

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')

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', [

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')