darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-07-18 05:30:21 -04:00
Code Issues Packages Projects Releases Wiki Activity

add restrict_admin_usage arg to admin_override

Browse Source 
rather than allow admins to do everything specifically, we should
only block them from things we conciously don't want them to do.
This is "Don't let platform admins send letters from services they're
not in". Everything else the platform admins can do.

This is step one, adding a restrict_admin_usage flag, and setting that
for those restricted endpoints around creating api keys, uploading CSVs
and sending one-off messages.

Also, this commit separates the two use cases for permissions:
* user.has_permission for access control
* user.has_permission_for_service for user info - this is used for
  showing checkboxes on the manage-users page for example

With this, we can remove the admin_override flag from the permission
decorator.
This commit is contained in:
Leo Hemsted
2018-02-28 18:13:29 +00:00
parent 09824078dd
commit 3ae815528c
12 changed files with 42 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/main/views/api_keys.py
View File

@@ -86,7 +86,7 @@ def api_keys(service_id):
@main.route("/services/<service_id>/api/keys/create", methods=['GET', 'POST'])
@login_required
@user_has_permissions('manage_api_keys')
@user_has_permissions('manage_api_keys', restrict_admin_usage=True)
def create_api_key(service_id):
key_names = [
key['name'] for key in api_key_api_client.get_api_keys(service_id=service_id)['apiKeys']

4
app/main/views/manage_users.py
View File

@@ -80,7 +80,7 @@ def edit_user_permissions(service_id, user_id):
user_has_no_mobile_number = user.mobile_number is None
form = PermissionsForm(
**{role: user.has_permissions(role) for role in roles.keys()},
**{role: user.has_permission_for_service(service_id, role) for role in roles.keys()},
login_authentication=user.auth_type
)
if form.validate_on_submit():
@@ -109,7 +109,7 @@ def remove_user_from_service(service_id, user_id):
# Need to make the email address read only, or a disabled field?
# Do it through the template or the form class?
form = PermissionsForm(**{
role: user.has_permissions(role) for role in roles.keys()
role: user.has_permission_for_service(service_id, role) for role in roles.keys()
})
if request.method == 'POST':

10
app/main/views/send.py
View File

@@ -601,7 +601,7 @@ def check_messages(service_id, template_type, upload_id, row_index=2):
@main.route("/services/<service_id>/<template_type>/check/<upload_id>.<filetype>", methods=['GET'])
@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>.<filetype>", methods=['GET'])
@login_required
@user_has_permissions('send_messages')
@user_has_permissions('send_messages', restrict_admin_usage=True)
def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=2):
if filetype not in ('pdf', 'png'):
abort(404)
@@ -615,7 +615,7 @@ def check_messages_preview(service_id, template_type, upload_id, filetype, row_i
@main.route("/services/<service_id>/<template_type>/check/<upload_id>", methods=['POST'])
@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", methods=['POST'])
@login_required
@user_has_permissions('send_messages')
@user_has_permissions('send_messages', restrict_admin_usage=True)
def recheck_messages(service_id, template_type, upload_id, row_index=0):
if not session.get('upload_data'):
@@ -626,7 +626,7 @@ def recheck_messages(service_id, template_type, upload_id, row_index=0):
@main.route("/services/<service_id>/start-job/<upload_id>", methods=['POST'])
@login_required
@user_has_permissions('send_messages')
@user_has_permissions('send_messages', restrict_admin_usage=True)
def start_job(service_id, upload_id):
upload_data = session['upload_data']
@@ -658,7 +658,7 @@ def start_job(service_id, upload_id):
@main.route("/services/<service_id>/end-tour/<example_template_id>")
@login_required
@user_has_permissions('manage_templates')
@user_has_permissions('manage_templates', admin_override=True)
def go_to_dashboard_after_tour(service_id, example_template_id):
service_api_client.delete_service_template(service_id, example_template_id)
@@ -767,7 +767,7 @@ def get_back_link(service_id, template_id, step_index):
@main.route("/services/<service_id>/template/<template_id>/notification/check", methods=['GET'])
@login_required
@user_has_permissions('send_messages')
@user_has_permissions('send_messages', admin_override=True)
def check_notification(service_id, template_id):
return _check_notification(service_id, template_id)

24
app/notify_client/models.py
View File

@@ -159,29 +159,30 @@ class User(UserMixin):
def permissions(self, permissions):
raise AttributeError("Read only property")
def has_permissions(self, *permissions, any_=False, admin_override=False):
def has_permissions(self, *permissions, any_=False, admin_override=None, restrict_admin_usage=False):
unknown_permissions = set(permissions) - all_permissions
if unknown_permissions:
raise TypeError('{} are not valid permissions'.format(list(unknown_permissions)))
# Only available to the platform admin user
if admin_override and self.platform_admin:
# platform admins should be able to do most things (except eg send messages, or create api keys)
if self.platform_admin and not restrict_admin_usage:
return True
# Not available to the non platform admin users.
# For example the list all-services page is only available to platform admin users and is not service specific
if admin_override and not permissions:
return False
# Service id is always set on the request for service specific views.
service_id = _get_service_id_from_view_args()
if service_id in self._permissions:
if any_:
return any([x in self._permissions[service_id] for x in permissions])
return set(self._permissions[service_id]) >= set(permissions)
has_permissions = any(x in self._permissions[service_id] for x in permissions)
else:
has_permissions = set(self._permissions[service_id]) >= set(permissions)
return has_permissions
return False
def has_permission_for_service(self, service_id, permission):
return permission in self._permissions.get(service_id, [])
@property
def auth_type(self):
return self._auth_type
@@ -249,6 +250,9 @@ class InvitedUser(object):
return False
return set(self.permissions) > set(permissions)
def has_permission_for_service(self, service_id, permission):
return self.status != 'cancelled' and self.service == service_id and permission in self.permissions
def __eq__(self, other):
return ((self.id,
self.service,

2
app/notify_client/user_api_client.py
View File

@@ -136,7 +136,7 @@ class UserApiClient(NotifyAdminAPIClient):
def get_count_of_users_with_permission(self, service_id, permission):
return len([
user for user in self.get_users_for_service(service_id)
if user.has_permissions(permission)
if user.has_permission_for_service(service_id, permission)
])
def get_users_for_organisation(self, org_id):

2
app/templates/views/dashboard/dashboard.html
View File

@@ -19,7 +19,7 @@
{% if not templates %}
{% include 'views/dashboard/write-first-messages.html' %}
{% endif %}
{% elif not current_user.has_permissions('send_messages', 'manage_api_keys', any_=True) %}
{% elif not current_user.has_permissions('send_messages', 'manage_api_keys', any_=True, admin_override=True) %}
{% include 'views/dashboard/no-permissions-banner.html' %}
{% endif %}

8
app/templates/views/manage-users.html
View File

@@ -64,19 +64,19 @@
<ul class="tick-cross-list">
<div class="tick-cross-list-permissions">
{{ tick_cross(
user.has_permissions('send_messages'),
user.has_permission_for_service(current_service.id, 'send_messages'),
'Send messages'
) }}
{{ tick_cross(
user.has_permissions('manage_templates'),
user.has_permission_for_service(current_service.id, 'manage_templates'),
'Add and edit templates'
) }}
{{ tick_cross(
user.has_permissions('manage_service'),
user.has_permission_for_service(current_service.id, 'manage_service'),
'Manage service'
) }}
{{ tick_cross(
user.has_permissions('manage_api_keys'),
user.has_permission_for_service(current_service.id, 'manage_api_keys'),
'Access API keys'
) }}
{% if 'email_auth' in current_service['permissions'] %}

2
app/templates/views/service-settings.html
View File

@@ -227,7 +227,7 @@
</ul>
<p>
{% if current_user.has_permissions('manage_service') %}
{% if current_user.has_permissions('manage_service', admin_override=True) %}
To remove these restrictions
<a href="{{ url_for('.request_to_go_live', service_id=current_service.id) }}">request to go live</a>.
{% else %}

2
app/templates/views/templates/_template.html
View File

@@ -6,7 +6,7 @@
{% else %}
<div class="bottom-gutter-2-3">
<div class="grid-row">
{% if current_user.has_permissions('send_messages') %}
{% if current_user.has_permissions('send_messages', restrict_admin_usage=True) %}
<div class="{{ 'column-half' if template.template_type == 'letter' else 'column-third' }}">
<a href="{{ url_for(".send_messages", service_id=current_service.id, template_id=template.id) }}" class="pill-separate-item">
Upload recipients

5
app/utils.py
View File

@@ -59,15 +59,14 @@ class BrowsableItem(object):
pass
def user_has_permissions(*permissions, admin_override=False, any_=False):
def user_has_permissions(*permissions, **permission_kwargs):
def wrap(func):
@wraps(func)
def wrap_func(*args, **kwargs):
if current_user and current_user.is_authenticated:
if current_user.has_permissions(
*permissions,
admin_override=admin_override,
any_=any_
**permission_kwargs
):
return func(*args, **kwargs)
else:

26
tests/app/main/test_permissions.py
View File

@@ -14,16 +14,16 @@ def _test_permissions(
client,
usr,
permissions,
service_id,
will_succeed,
any_=False,
admin_override=False,
kwargs={}
):
request.view_args.update({'service_id': service_id})
request.view_args.update({'service_id': 'foo'})
if usr:
client.login(usr)
decorator = user_has_permissions(*permissions, any_=any_, admin_override=admin_override)
decorator = user_has_permissions(*permissions, **kwargs)
decorated_index = decorator(index)
if will_succeed:
decorated_index()
else:
@@ -44,7 +44,6 @@ def test_user_has_permissions_on_endpoint_fail(
client,
user,
['send_messages'],
'',
False)
@@ -58,7 +57,6 @@ def test_user_has_permissions_success(
client,
user,
['manage_service'],
'',
True)
@@ -72,9 +70,8 @@ def test_user_has_permissions_or(
client,
user,
['send_messages', 'manage_service'],
'',
True,
any_=True)
kwargs={'any_': True})
def test_user_has_permissions_multiple(
@@ -87,7 +84,6 @@ def test_user_has_permissions_multiple(
client,
user,
['manage_templates', 'manage_service'],
'',
will_succeed=True)
@@ -101,7 +97,6 @@ def test_exact_permissions(
client,
user,
['manage_service', 'manage_templates'],
'',
True)
@@ -115,9 +110,8 @@ def test_platform_admin_user_can_access_page(
client,
platform_admin_user,
[],
'',
will_succeed=True,
admin_override=True)
kwargs={'admin_override': True})
def test_platform_admin_user_can_not_access_page(
@@ -130,9 +124,8 @@ def test_platform_admin_user_can_not_access_page(
client,
platform_admin_user,
[],
'',
will_succeed=False,
admin_override=False)
kwargs={'restrict_admin_usage': True})
def test_no_user_returns_401_unauth(
@@ -144,7 +137,6 @@ def test_no_user_returns_401_unauth(
client,
None,
[],
'',
will_succeed=False)
@@ -158,7 +150,7 @@ def _user_with_permissions():
'mobile_number': '+4412341234',
'state': 'active',
'failed_login_count': 0,
'permissions': {'': ['manage_users', 'manage_templates', 'manage_settings']},
'permissions': {'foo': ['manage_users', 'manage_templates', 'manage_settings']},
'platform_admin': False
}
user = User(user_data)

2
tests/conftest.py
View File

@@ -1931,7 +1931,7 @@ def mock_no_inbound_number_for_service(mocker):
@pytest.fixture(scope='function')
def mock_has_permissions(mocker):
def _has_permission(*permissions, any_=False, admin_override=False):
def _has_permission(*permissions, any_=False, admin_override=False, restrict_admin_usage=False):
return True
return mocker.patch(