From 000d6c3a48d520c67ca669f7326e2dd1611033e6 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Feb 2019 16:56:43 +0000 Subject: [PATCH] Replace platform admin service setting buttons with forms Most of the existing platform admin buttons on the service settings page used to issue GET requests to switch service settings. This means they weren't protected by CSRF. On top of that as our number of service permissions increases over time a lot of buttons on the page made it hard to work with. To fix these issues we replace most of the buttons with rows in the platform admin settings table. Each setting has a 'Change' link that leads to a page with an On/Off switch form. This removes "research mode" switch completely since we're planning to deprecate it in the future and we don't expect to switch any new services into research mode at the moment. Most service permissions are now handled by a shared endpoint that is parameterized with the permission name. Some permissions that require some additional logic before they can be toggled (like document upload, which requires setting a contact address) have separate initial endpoints that redirect to `set_service_permission`. "Archive", "Suspend" and "Resume" actions are kept as buttons since they display a confirmation banner (which is a CSRF-protected form) and they're not easily represented as an On/Off switch. --- app/main/views/service_settings.py | 89 +++++++++------ app/navigation.py | 16 +-- app/templates/views/service-settings.html | 54 +++------ .../service-settings/set-service-setting.html | 26 +++++ .../test_service_setting_permissions.py | 104 +++++++++++++----- tests/app/main/views/test_service_settings.py | 99 +++-------------- 6 files changed, 200 insertions(+), 188 deletions(-) create mode 100644 app/templates/views/service-settings/set-service-setting.html diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 0774c1e5d..f77833b7d 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1,3 +1,4 @@ +from collections import OrderedDict from datetime import datetime import pytz @@ -42,6 +43,7 @@ from app.main.forms import ( ServiceEditInboundNumberForm, ServiceInboundNumberForm, ServiceLetterContactBlockForm, + ServiceOnOffSettingForm, ServicePreviewBranding, ServiceReplyToEmailForm, ServiceSetEmailBranding, @@ -60,11 +62,22 @@ from app.utils import ( ) +PLATFORM_ADMIN_SERVICE_PERMISSIONS = OrderedDict([ + ('inbound_sms', {'title': 'Receive inbound SMS', 'requires': 'sms', 'endpoint': '.service_set_inbound_number'}), + ('precompiled_letter', {'title': 'Send precompiled letters', 'requires': 'letter'}), + ('email_auth', {'title': 'User auth type editing'}), + ('upload_document', {'title': 'Uploading documents', 'endpoint': '.service_switch_can_upload_document'}), +]) + + @main.route("/services//service-settings") @login_required @user_has_permissions('manage_service', 'manage_api_keys') def service_settings(service_id): - return render_template('views/service-settings.html') + return render_template( + 'views/service-settings.html', + service_permissions=PLATFORM_ADMIN_SERVICE_PERMISSIONS + ) @main.route("/services//service-settings/name", methods=['GET', 'POST']) @@ -211,54 +224,64 @@ def submit_request_to_go_live(service_id): return render_template('views/service-settings/submit-request-to-go-live.html', form=form) -@main.route("/services//service-settings/switch-live") +@main.route("/services//service-settings/switch-live", methods=["GET", "POST"]) @login_required @user_is_platform_admin def service_switch_live(service_id): - current_service.update( - # TODO This limit should be set depending on the agreement signed by - # with Notify. - message_limit=250000 if current_service.trial_mode else 50, - restricted=(not current_service.trial_mode) + form = ServiceOnOffSettingForm( + name="Make service live", + enabled=not current_service.trial_mode + ) + + if form.validate_on_submit(): + current_service.update( + # TODO This limit should be set depending on the agreement signed by + # with Notify. + message_limit=250000 if form.enabled.data else 50, + restricted=(not form.enabled.data) + ) + return redirect(url_for('.service_settings', service_id=service_id)) + + return render_template( + 'views/service-settings/set-service-setting.html', + title="Make service live", + form=form, ) - return redirect(url_for('.service_settings', service_id=service_id)) -@main.route("/services//service-settings/research-mode") +@main.route("/services//service-settings/permissions/", methods=["GET", "POST"]) @login_required @user_is_platform_admin -def service_switch_research_mode(service_id): - current_service.toggle_research_mode() - return redirect(url_for('.service_settings', service_id=service_id)) +def service_set_permission(service_id, permission): + if permission not in PLATFORM_ADMIN_SERVICE_PERMISSIONS: + abort(404) + title = PLATFORM_ADMIN_SERVICE_PERMISSIONS[permission]['title'] + form = ServiceOnOffSettingForm( + name=title, + enabled=current_service.has_permission(permission) + ) -@main.route("/services//service-settings/email-auth") -@login_required -@user_is_platform_admin -def service_switch_email_auth(service_id): - current_service.switch_permission('email_auth') - return redirect(url_for('.service_settings', service_id=service_id)) + if form.validate_on_submit(): + current_service.force_permission(permission, on=form.enabled.data) + return redirect(url_for(".service_settings", service_id=service_id)) -@main.route("/services//service-settings/can-send-precompiled-letter") -@login_required -@user_is_platform_admin -def service_switch_can_send_precompiled_letter(service_id): - current_service.switch_permission('precompiled_letter') - return redirect(url_for('.service_settings', service_id=service_id)) + return render_template( + 'views/service-settings/set-service-setting.html', + title=title, + form=form, + ) @main.route("/services//service-settings/can-upload-document", methods=['GET', 'POST']) @login_required @user_is_platform_admin def service_switch_can_upload_document(service_id): - form = ServiceContactDetailsForm() + if current_service.contact_link: + return redirect(url_for('.service_set_permission', service_id=service_id, permission='upload_document')) - # If turning the permission off, or turning it on and the service already has a contact_link, - # don't show the form to add the link - if current_service.has_permission('upload_document') or current_service.contact_link: - current_service.switch_permission('upload_document') - return redirect(url_for('.service_settings', service_id=service_id)) + form = ServiceContactDetailsForm() if form.validate_on_submit(): contact_type = form.contact_details_type.data @@ -266,8 +289,8 @@ def service_switch_can_upload_document(service_id): current_service.update( contact_link=form.data[contact_type] ) - current_service.switch_permission('upload_document') - return redirect(url_for('.service_settings', service_id=service_id)) + + return redirect(url_for('.service_set_permission', service_id=service_id, permission='upload_document')) return render_template('views/service-settings/contact_link.html', form=form) @@ -425,6 +448,7 @@ def service_set_inbound_number(service_id): form = ServiceInboundNumberForm( inbound_number_choices=inbound_numbers_value_and_label ) + if form.validate_on_submit(): service_api_client.add_sms_sender( current_service.id, @@ -434,6 +458,7 @@ def service_set_inbound_number(service_id): ) current_service.force_permission('inbound_sms', on=True) return redirect(url_for('.service_settings', service_id=service_id)) + return render_template( 'views/service-settings/set-inbound-number.html', form=form, diff --git a/app/navigation.py b/app/navigation.py index e5dbca7ab..6bcaf66ae 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -246,11 +246,9 @@ class HeaderNavigation(Navigation): 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', - 'service_switch_can_send_precompiled_letter', 'service_switch_can_upload_document', - 'service_switch_email_auth', 'service_switch_live', - 'service_switch_research_mode', + 'service_set_permission', 'services_or_dashboard', 'set_free_sms_allowance', 'set_organisation_type', @@ -479,11 +477,9 @@ class MainNavigation(Navigation): 'service_dashboard_updates', 'service_delete_email_reply_to', 'service_delete_sms_sender', - 'service_switch_can_send_precompiled_letter', 'service_switch_can_upload_document', - 'service_switch_email_auth', 'service_switch_live', - 'service_switch_research_mode', + 'service_set_permission', 'services_or_dashboard', 'show_accounts_or_dashboard', 'sign_in', @@ -707,11 +703,9 @@ class CaseworkNavigation(Navigation): 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', - 'service_switch_can_send_precompiled_letter', 'service_switch_can_upload_document', - 'service_switch_email_auth', 'service_switch_live', - 'service_switch_research_mode', + 'service_set_permission', 'services_or_dashboard', 'set_free_sms_allowance', 'service_set_letter_branding', @@ -943,11 +937,9 @@ class OrgNavigation(Navigation): 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', - 'service_switch_can_send_precompiled_letter', 'service_switch_can_upload_document', - 'service_switch_email_auth', 'service_switch_live', - 'service_switch_research_mode', + 'service_set_permission', 'services_or_dashboard', 'set_free_sms_allowance', 'service_set_letter_branding', diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 92ecbc8a3..896066802 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -285,6 +285,13 @@ field_headings_visible=False, caption_visible=False ) %} + + {% call row() %} + {{ text_field('Live')}} + {{ boolean_field(not current_service.trial_mode) }} + {{ edit_field('Change', url_for('.service_switch_live', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} {{ text_field('Organisation')}} {{ optional_text_field(current_service.organisation_name) }} @@ -319,45 +326,20 @@ {% endcall %} {{ edit_field('Change', url_for('.data_retention', service_id=current_service.id)) }} {% endcall %} + + {% for permission in service_permissions %} + {% if not service_permissions[permission].requires or current_service.has_permission(service_permissions[permission].requires) %} + {% call row() %} + {{ text_field(service_permissions[permission].title)}} + {{ boolean_field(current_service.has_permission(permission)) }} + {{ edit_field('Change', url_for(service_permissions[permission].endpoint or '.service_set_permission', service_id=current_service.id, permission=permission if not service_permissions[permission].endpoint else None)) }} + {% endcall %} + {% endif %} + {% endfor %} + {% endcall %}