Merge pull request #1649 from alphagov/existing-user-email-auth-invite

Existing user email auth invite

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_api_client.update_user_attribute(user_id, auth_type=form.login_authentication.data)
-                user_id,
-                auth_type=form.login_authentication.data
-            )
         return redirect(url_for('.manage_users', service_id=service_id))
 
     return render_template(

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,
+        user_api_client.update_user_attribute(current_user.id, name=form.new_name.data)
-                                              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,
+    user_api_client.update_user_attribute(user_id, email_address=new_email)
-                                          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,
+        user_api_client.update_user_attribute(current_user.id, mobile_number=mobile_number)
-                                              mobile_number=mobile_number)
         return redirect(url_for('.user_profile'))
 
     return render_template(

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 %}

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