Merge pull request #231 from alphagov/add_manage_service_permission

Add manage service permission

app/main/dao/users_dao.py

@@ -8,6 +8,11 @@ from app.main.encryption import hashpw
 
 from app import user_api_client
 
+#
+# TODO fix up this, do we really need this class why not just use the clients
+# directly??
+#
+
 
 @login_manager.user_loader
 def load_user(user_id):

app/main/forms.py

@@ -108,7 +108,13 @@ class RegisterUserFromInviteForm(Form):
 
 
 class InviteUserForm(Form):
+
     email_address = email_address('Their email address')
+    # TODO fix this Radio field so we are not having to test for yes or no rather
+    # use operator equality.
+    send_messages = RadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')])
+    manage_service = RadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')])
+    manage_api_keys = RadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')])
 
 
 class TwoFactorForm(Form):

app/main/views/manage_users.py

@@ -12,65 +12,47 @@ from flask_login import (
 )
 
 from notifications_python_client.errors import HTTPError
+from app import user_api_client
 
 from app.main import main
 from app.main.forms import InviteUserForm
-from app.main.dao.services_dao import get_service_by_id_or_404
+from app.main.dao.services_dao import get_service_by_id
 from app import user_api_client
 from app import invite_api_client
-
-fake_users = [
-    {
-        'name': '',
-        'permission_send_messages': True,
-        'permission_manage_service': True,
-        'permission_manage_api_keys': True,
-        'active': True
-    }
-]
+from app.utils import user_has_permissions
 
 
 @main.route("/services/<service_id>/users")
 @login_required
+@user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
 def manage_users(service_id):
-    try:
-        users = user_api_client.get_users_for_service(service_id=service_id)
-        invited_users = invite_api_client.get_invites_for_service(service_id=service_id)
-        return render_template('views/manage-users.html',
-                               service_id=service_id,
-                               users=users,
-                               current_user=current_user,
-                               invited_users=invited_users)
-    except HTTPError as e:
-        if e.status_code == 404:
-            abort(404)
-        else:
-            raise e
+    users = user_api_client.get_users_for_service(service_id=service_id)
+    invited_users = invite_api_client.get_invites_for_service(service_id=service_id)
+    return render_template('views/manage-users.html',
+                           service_id=service_id,
+                           users=users,
+                           current_user=current_user,
+                           invited_users=invited_users)
 
 
 @main.route("/services/<service_id>/users/invite", methods=['GET', 'POST'])
 @login_required
+@user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
 def invite_user(service_id):
 
+    service = get_service_by_id(service_id)
+
     form = InviteUserForm()
     if form.validate_on_submit():
         email_address = form.email_address.data
         permissions = _get_permissions(request.form)
-        try:
-            invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions)
-            flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick')
-            return redirect(url_for('.manage_users', service_id=service_id))
-
-        except HTTPError as e:
-            if e.status_code == 404:
-                abort(404)
-            else:
-                raise e
+        invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions)
+        flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick')
+        return redirect(url_for('.manage_users', service_id=service_id))
 
     return render_template(
         'views/invite-user.html',
-        user={},
-        service=get_service_by_id_or_404(service_id),
+        user=None,
         service_id=service_id,
         form=form
     )
@@ -78,29 +60,53 @@ def invite_user(service_id):
 
 @main.route("/services/<service_id>/users/<user_id>", methods=['GET', 'POST'])
 @login_required
+@user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
 def edit_user_permissions(service_id, user_id):
+    # TODO we should probably using the service id here in the get user
+    # call as well. eg. /user/<user_id>?&service_id=service_id
+    user = user_api_client.get_user(user_id)
+    service = get_service_by_id(service_id)
+    # Need to make the email address read only, or a disabled field?
+    # Do it through the template or the form class?
+    form = InviteUserForm(**{
+        'email_address': user.email_address,
+        'send_messages': 'yes' if user.has_permissions(
+            ['send_texts', 'send_emails', 'send_letters']) else 'no',
+        'manage_service': 'yes' if user.has_permissions(
+            ['manage_users', 'manage_templates', 'manage_settings']) else 'no',
+        'manage_api_keys': 'yes' if user.has_permissions(
+            ['manage_api_keys', 'access_developer_docs']) else 'no'
+        })
 
-    if request.method == 'POST':
+    if form.validate_on_submit():
+        permissions = []
+        permissions.extend(
+            _convert_role_to_permissions('send_messages') if form.send_messages.data == 'yes' else [])
+        permissions.extend(
+            _convert_role_to_permissions('manage_service') if form.manage_service.data == 'yes' else [])
+        permissions.extend(
+            _convert_role_to_permissions('manage_api_keys') if form.manage_api_keys.data == 'yes' else [])
+        user_api_client.set_user_permissions(user_id, service_id, permissions)
         return redirect(url_for('.manage_users', service_id=service_id))
 
     return render_template(
         'views/invite-user.html',
-        user=fake_users[int(user_id)],
-        user_id=user_id,
-        service=get_service_by_id_or_404(service_id),
+        user=user,
+        form=form,
         service_id=service_id
     )
 
 
 @main.route("/services/<service_id>/users/<user_id>/delete", methods=['GET', 'POST'])
 @login_required
+@user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
 def delete_user(service_id, user_id):
+    user = user_api_client.get_user(user_id)
+    service = get_service_by_id(service_id)
 
     if request.method == 'POST':
         return redirect(url_for('.manage_users', service_id=service_id))
 
-    user = fake_users[int(user_id)]
-
     flash(
         'Are you sure you want to delete {}’s account?'.format(user.get('name') or user['email_localpart']),
         'delete'
@@ -109,19 +115,30 @@ def delete_user(service_id, user_id):
     return render_template(
         'views/invite-user.html',
         user=user,
-        user_id=user_id,
-        service=get_service_by_id_or_404(service_id),
         service_id=service_id
     )
 
 
 @main.route("/services/<service_id>/cancel-invited-user/<invited_user_id>", methods=['GET'])
+@user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
 def cancel_invited_user(service_id, invited_user_id):
     invite_api_client.cancel_invited_user(service_id=service_id, invited_user_id=invited_user_id)
 
     return redirect(url_for('main.manage_users', service_id=service_id))
 
 
+def _convert_role_to_permissions(role):
+    if role == 'send_messages':
+        return ['send_texts', 'send_emails', 'send_letters']
+    elif role == 'manage_service':
+        return ['manage_users', 'manage_templates', 'manage_settings']
+    elif role == 'manage_api_keys':
+        return ['manage_api_keys', 'access_developer_docs']
+    return []
+
+
+# TODO replace with method which converts each 'role' into the list
+# of permissions like the method above :)
 def _get_permissions(form):
     permissions = []
     if form.get('send_messages') and form['send_messages'] == 'yes':

app/notify_client/models.py

@@ -133,8 +133,8 @@ class InvitedUser(object):
         self.status = status
         self.created_at = created_at
 
-    def has_permissions(self, permission):
-        return permission in self.permissions
+    def has_permissions(self, permissions):
+        return set(self.permissions) > set(permissions)
 
     def __eq__(self, other):
         return ((self.id,

app/templates/components/yes-no.html

@@ -5,11 +5,11 @@
     </legend>
     <div class='yes-no-fields inline'>
       <label class='block-label'>
-        <input type='radio' name='{{ name }}' value='yes' {% if current_value == True %}checked{% endif %} />
+        <input type='radio' name='{{ name }}' value='yes' {% if current_value == 'yes' %}checked{% endif %} />
         Yes
       </label>
       <label class='block-label'>
-        <input type='radio' name='{{ name }}' value='no' {% if current_value == False %}checked{% endif %} />
+        <input type='radio' name='{{ name }}' value='no' {% if current_value == 'no' %}checked{% endif %} />
         No
       </label>
     </div>

app/templates/views/invite-user.html

@@ -16,22 +16,16 @@ Manage users – GOV.UK Notify
   <div class="grid-row">
     <form method="post" class="column-three-quarters">
 
-      {% if user %}
-        <p class='bottom-gutter'>
-          {{ current_user.email_address }}
-        </p>
-      {% else %}
-        {{ textbox(form.email_address, hint='Email address must end in .gov.uk', width='1-1') }}
-      {% endif %}
+      {{ textbox(form.email_address, hint='Email address must end in .gov.uk', width='1-1') }}
 
       <fieldset class='yes-no-wrapper'>
         <legend class='heading-small'>
           Permissions
         </legend>
         <span class="form-hint">All team members can see message history</span>
-        {{ yes_no('send_messages', 'Send messages', user.permission_send_messages) }}
-        {{ yes_no('manage_service', 'Manage service', user.permission_manage_service) }}
-        {{ yes_no('manage_api_keys', 'Manage API keys', user.permission_manage_api_keys) }}
+        {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }}
+        {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }}
+        {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }}
       </fieldset>
 
       {% if user %}

app/templates/views/manage-users.html

@@ -28,9 +28,14 @@ Manage users – GOV.UK Notify
     {% call field() %}
       {{ item.name }}
     {% endcall %}
-    {{ boolean_field(item.has_permissions(service_id, 'send_messages')) }}
-    {{ boolean_field(item.has_permissions(service_id, 'manage_service')) }}
-    {{ boolean_field(item.has_permissions(service_id, 'manage_api_keys')) }}
+    {{ boolean_field(item.has_permissions(['send_texts', 'send_emails', 'send_letters'], service_id=service_id)) }}
+    {{ boolean_field(item.has_permissions(['manage_users', 'manage_templates', 'manage_settings'], service_id=service_id)) }}
+    {{ boolean_field(item.has_permissions(['manage_api_keys', 'access_developer_docs'], service_id=service_id)) }}
+    {% call field(align='right') %}
+      {% if current_user.id != item.id %}
+        <a href="{{ url_for('.edit_user_permissions', service_id=service_id, user_id=item.id)}}">Edit permission</a>
+      {% endif %}
+    {% endcall %}
   {% endcall %}
 
   {% if invited_users %}
@@ -40,9 +45,9 @@ Manage users – GOV.UK Notify
       {% call field() %}
         {{ item.email_address }}
       {% endcall %}
-      {{ boolean_field(item.has_permissions('send_messages')) }}
-      {{ boolean_field(item.has_permissions('manage_service')) }}
-      {{ boolean_field(item.has_permissions('manage_api_keys')) }}
+      {{ boolean_field(item.has_permissions(['send_texts', 'send_emails', 'send_letters'])) }}
+      {{ boolean_field(item.has_permissions(['manage_users', 'manage_templates', 'manage_settings'])) }}
+      {{ boolean_field(item.has_permissions(['manage_api_keys', 'access_developer_docs'])) }}
       {% if item.status == 'pending' %}
         {% call field(align='right') %}
           <a href="{{ url_for('.cancel_invited_user', service_id=service_id, invited_user_id=item.id)}}">Cancel invitation</a>

tests/app/main/views/test_manage_users.py

@@ -9,7 +9,8 @@ def test_should_show_overview_page(
     mock_login,
     mock_get_service,
     mock_get_users_by_service,
-    mock_get_invites_for_service
+    mock_get_invites_for_service,
+    mock_has_permissions
 ):
     with app_.test_request_context():
         with app_.test_client() as client:
@@ -25,7 +26,8 @@ def test_should_show_page_for_one_user(
     app_,
     api_user_active,
     mock_login,
-    mock_get_service
+    mock_get_service,
+    mock_has_permissions
 ):
     with app_.test_request_context():
         with app_.test_client() as client:
@@ -35,32 +37,84 @@ def test_should_show_page_for_one_user(
         assert response.status_code == 200
 
 
-def test_redirect_after_saving_user(
+def test_edit_user_permissions(
     app_,
     api_user_active,
     mock_login,
     mock_get_service,
     mock_get_users_by_service,
-    mock_get_invites_for_service
+    mock_get_invites_for_service,
+    mock_has_permissions,
+    mock_set_user_permissions
 ):
     with app_.test_request_context():
         with app_.test_client() as client:
+            service_id = '55555'
             client.login(api_user_active)
             response = client.post(url_for(
-                'main.edit_user_permissions', service_id=55555, user_id=0
-            ))
+                'main.edit_user_permissions', service_id=service_id, user_id=api_user_active.id
+            ), data={'email_address': api_user_active.email_address,
+                     'send_messages': 'yes',
+                     'manage_service': 'yes',
+                     'manage_api_keys': 'yes'})
 
         assert response.status_code == 302
         assert response.location == url_for(
-            'main.manage_users', service_id=55555, _external=True
+            'main.manage_users', service_id=service_id, _external=True
         )
+        mock_set_user_permissions.assert_called_with(
+            str(api_user_active.id),
+            service_id,
+            ['send_texts',
+             'send_emails',
+             'send_letters',
+             'manage_users',
+             'manage_templates',
+             'manage_settings',
+             'manage_api_keys',
+             'access_developer_docs'])
+
+
+def test_edit_some_user_permissions(
+    app_,
+    api_user_active,
+    mock_login,
+    mock_get_service,
+    mock_get_users_by_service,
+    mock_get_invites_for_service,
+    mock_has_permissions,
+    mock_set_user_permissions
+):
+    with app_.test_request_context():
+        with app_.test_client() as client:
+            service_id = '55555'
+            client.login(api_user_active)
+            response = client.post(url_for(
+                'main.edit_user_permissions', service_id=service_id, user_id=api_user_active.id
+            ), data={'email_address': api_user_active.email_address,
+                     'send_messages': 'yes',
+                     'manage_service': 'no',
+                     'manage_api_keys': 'no'})
+
+        assert response.status_code == 302
+        assert response.location == url_for(
+            'main.manage_users', service_id=service_id, _external=True
+        )
+        mock_set_user_permissions.assert_called_with(
+            str(api_user_active.id),
+            service_id,
+            ['send_texts',
+             'send_emails',
+             'send_letters'])
 
 
 def test_should_show_page_for_inviting_user(
     app_,
     api_user_active,
     mock_login,
-    mock_get_service
+    mock_get_user,
+    mock_get_service,
+    mock_has_permissions
 ):
     with app_.test_request_context():
         with app_.test_client() as client:
@@ -76,9 +130,12 @@ def test_invite_user(
     service_one,
     api_user_active,
     mock_login,
+    mock_get_user,
+    mock_get_service,
     mock_get_users_by_service,
     mock_create_invite,
-    mock_get_invites_for_service
+    mock_get_invites_for_service,
+    mock_has_permissions
 ):
     from_user = api_user_active.id
     service_id = service_one['id']
@@ -106,7 +163,11 @@ def test_invite_user(
         assert flash_banner == 'Invite sent to test@example.gov.uk'
 
 
-def test_cancel_invited_user_cancels_user_invitations(app_, api_user_active, mock_login, mocker):
+def test_cancel_invited_user_cancels_user_invitations(app_,
+                                                      api_user_active,
+                                                      mock_login,
+                                                      mocker,
+                                                      mock_has_permissions):
     with app_.test_request_context():
         with app_.test_client() as client:
             mocker.patch('app.invite_api_client.cancel_invited_user')

tests/conftest.py

@@ -640,3 +640,8 @@ def mock_add_user_to_service(mocker, service_one, api_user_active):
     def _add_user(service_id, user_id, invited_user):
         return api_user_active
     return mocker.patch('app.user_api_client.add_user_to_service', side_effect=_add_user)
+
+
+@pytest.fixture(scope='function')
+def mock_set_user_permissions(mocker):
+    return mocker.patch('app.user_api_client.set_user_permissions', return_value=None)