From ffe30c30706edb13fc06beb0286e117ca0a6d69a Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 29 Feb 2016 17:03:56 +0000 Subject: [PATCH 1/4] Permissions added for templates and send_messages pages. All tests passing. Fix up page heading. --- app/main/views/send.py | 23 ++++++++-- app/main/views/service_settings.py | 9 ++++ app/main/views/templates.py | 4 ++ app/notify_client/user_api_client.py | 4 +- app/templates/main_nav.html | 6 +++ app/templates/views/choose-template.html | 8 ++++ app/utils.py | 4 +- tests/app/main/views/test_send.py | 24 ++++++---- tests/app/main/views/test_service_settings.py | 45 ++++++++++++------- tests/app/main/views/test_templates.py | 12 +++-- tests/conftest.py | 2 +- 11 files changed, 106 insertions(+), 35 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 856403105..a626dfded 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -26,11 +26,17 @@ from app.main.uploader import ( from app.main.dao import templates_dao from app.main.dao import services_dao from app import job_api_client -from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError +from app.utils import ( + validate_recipient, InvalidPhoneError, InvalidEmailError, user_has_permissions) page_headings = { - 'email': 'Send emails', - 'sms': 'Send text messages' + 'manage_service': { + 'email': 'Send emails', + 'sms': 'Send text messages'}, + 'manage_templates': { + 'email': 'Manage templates', + 'sms': 'Manage templates' + } } @@ -42,6 +48,8 @@ def letters_stub(service_id): @main.route("/services//send/", methods=['GET']) +@login_required +@user_has_permissions('send_messages', 'manage_templates', or_=True) def choose_template(service_id, template_type): service = services_dao.get_service_by_id_or_404(service_id) @@ -55,6 +63,9 @@ def choose_template(service_id, template_type): abort(404) else: raise e + # TODO fix up how page_heading is loaded. + page_heading = page_headings['manage_service'][template_type] if current_user.has_permissions(session.get('service_id', ''), 'manage_service') else \ + page_headings['manage_templates'][template_type] return render_template( 'views/choose-template.html', templates=[ @@ -65,7 +76,7 @@ def choose_template(service_id, template_type): if template['template_type'] == template_type ], template_type=template_type, - page_heading=page_headings[template_type], + page_heading=page_heading, service=service, has_jobs=len(jobs), service_id=service_id @@ -74,6 +85,7 @@ def choose_template(service_id, template_type): @main.route("/services//send/", methods=['GET', 'POST']) @login_required +@user_has_permissions('send_messages') def send_messages(service_id, template_id): form = CsvUploadForm() @@ -110,6 +122,7 @@ def send_messages(service_id, template_id): @main.route("/services//send/.csv", methods=['GET']) @login_required +@user_has_permissions('send_messages', 'manage_templates', or_=True) def get_example_csv(service_id, template_id): template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] placeholders = list(Template(template).placeholders) @@ -127,6 +140,7 @@ def get_example_csv(service_id, template_id): @main.route("/services//send//to-self", methods=['GET']) @login_required +@user_has_permissions('send_messages') def send_message_to_self(service_id, template_id): template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] placeholders = list(Template(template).placeholders) @@ -150,6 +164,7 @@ def send_message_to_self(service_id, template_id): @main.route("/services//check/", methods=['GET', 'POST']) @login_required +@user_has_permissions('send_messages') def check_messages(service_id, upload_id): upload_data = session['upload_data'] diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 12b68f307..df2b1f2f7 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -3,6 +3,7 @@ from flask import ( from flask_login import (login_required, current_user) from app.main import main +from app.utils import user_has_permissions from app.main.dao.services_dao import ( get_service_by_id, delete_service, update_service) from app.main.dao.users_dao import verify_password @@ -12,6 +13,7 @@ from notifications_python_client.errors import HTTPError @main.route("/services//service-settings") @login_required +@user_has_permissions('manage_service') def service_settings(service_id): try: service = get_service_by_id(service_id)['data'] @@ -29,6 +31,7 @@ def service_settings(service_id): @main.route("/services//service-settings/name", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_name_change(service_id): try: service = get_service_by_id(service_id)['data'] @@ -53,6 +56,7 @@ def service_name_change(service_id): @main.route("/services//service-settings/name/confirm", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_name_change_confirm(service_id): try: service = get_service_by_id(service_id)['data'] @@ -82,6 +86,7 @@ def service_name_change_confirm(service_id): @main.route("/services//service-settings/request-to-go-live", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_request_to_go_live(service_id): try: service = get_service_by_id(service_id)['data'] @@ -104,6 +109,7 @@ def service_request_to_go_live(service_id): @main.route("/services//service-settings/status", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_status_change(service_id): try: service = get_service_by_id(service_id)['data'] @@ -125,6 +131,7 @@ def service_status_change(service_id): @main.route("/services//service-settings/status/confirm", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_status_change_confirm(service_id): try: service = get_service_by_id(service_id)['data'] @@ -153,6 +160,7 @@ def service_status_change_confirm(service_id): @main.route("/services//service-settings/delete", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_delete(service_id): try: service = get_service_by_id(service_id)['data'] @@ -174,6 +182,7 @@ def service_delete(service_id): @main.route("/services//service-settings/delete/confirm", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_delete_confirm(service_id): try: service = get_service_by_id(service_id)['data'] diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 15f72b113..e178ea13e 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -5,6 +5,7 @@ from notifications_python_client.errors import HTTPError from utils.template import Template from app.main import main +from app.utils import user_has_permissions from app.main.forms import SMSTemplateForm, EmailTemplateForm from app import job_api_client from app.main.dao.services_dao import get_service_by_id_or_404 @@ -20,6 +21,7 @@ form_objects = { @main.route("/services//templates/add-", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_templates') def add_service_template(service_id, template_type): service = sdao.get_service_by_id_or_404(service_id) @@ -51,6 +53,7 @@ def add_service_template(service_id, template_type): @main.route("/services//templates/", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_templates') def edit_service_template(service_id, template_id): template = tdao.get_service_template_or_404(service_id, template_id)['data'] template['template_content'] = template['content'] @@ -78,6 +81,7 @@ def edit_service_template(service_id, template_id): @main.route("/services//templates//delete", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_templates') def delete_service_template(service_id, template_id): template = tdao.get_service_template_or_404(service_id, template_id)['data'] diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index e0d1057d1..46bdbb2d9 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -167,8 +167,10 @@ class User(UserMixin): def permissions(self, permissions): raise AttributeError("Read only property") - def has_permissions(self, service_id, permissions): + def has_permissions(self, service_id, permissions, or_=False): if service_id in self._permissions: + if or_: + return any([x in self._permissions[service_id] for x in permissions]) return set(self._permissions[service_id]) > set(permissions) return False diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 216bec23c..cfc6cd0d1 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -8,6 +8,12 @@
  • Send emails
  • Send letters
    • + {% elif current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% endif %} {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %}
      diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index 7fb87b827..b2d9545f5 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -16,6 +16,7 @@ {% if templates %} {% if not has_jobs %} + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} {{ banner( """ Send yourself a test message @@ -23,6 +24,7 @@ subhead='Next step', type="tip" )}} + {% endif %} {% endif %}
      {% for template in templates %} @@ -39,9 +41,13 @@
      + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} Add recipients Send yourself a test + {% endif %} + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} Edit template + {% endif %}
      {% endfor %} @@ -49,7 +55,9 @@ {% endif %}

      + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} Add a new template + {% endif %}

      diff --git a/app/utils.py b/app/utils.py index 176ff26cb..280b052e1 100644 --- a/app/utils.py +++ b/app/utils.py @@ -94,14 +94,14 @@ def validate_recipient(recipient, template_type): }[template_type](recipient) -def user_has_permissions(*permissions): +def user_has_permissions(*permissions, or_=False): def wrap(func): @wraps(func) def wrap_func(*args, **kwargs): # We are making the assumption that the user is logged in. from flask_login import current_user service_id = session.get('service_id', '') - if current_user and current_user.has_permissions(service_id, permissions): + if current_user and current_user.has_permissions(service_id, permissions, or_=or_): return func(*args, **kwargs) else: abort(403) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e638a5481..28116626c 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -17,7 +17,8 @@ def test_choose_template( mock_get_service, mock_check_verify_code, mock_get_service_templates, - mock_get_jobs + mock_get_jobs, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -40,7 +41,8 @@ def test_upload_empty_csvfile_returns_to_upload_page( mock_get_service, mock_get_service_templates, mock_check_verify_code, - mock_get_service_template + mock_get_service_template, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -64,7 +66,8 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): contents = 'to,name\n+44 123,test1\n+44 456,test2' @@ -95,7 +98,8 @@ def test_upload_csvfile_removes_empty_lines_and_trailing_commas( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): contents = 'to,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' @@ -127,7 +131,8 @@ def test_send_test_message_to_self( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): expected_data = {'data': ['to', '+4412341234'], 'file_name': 'Test run'} @@ -150,7 +155,8 @@ def test_download_example_csv( api_user_active, mock_login, mock_get_service, - mock_get_service_template + mock_get_service_template, + mock_has_permissions ): with app_.test_request_context(): @@ -172,7 +178,8 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): contents = 'to\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986' # noqa @@ -211,7 +218,8 @@ def test_create_job_should_call_api( mock_create_job, mock_get_job, mock_get_service, - mock_get_service_template + mock_get_service_template, + mock_has_permissions ): service_id = service_one['id'] diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 045cc4c86..174935f2a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -6,7 +6,8 @@ def test_should_show_overview(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -25,7 +26,8 @@ def test_should_show_service_name(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -44,7 +46,8 @@ def test_should_redirect_after_change_service_name(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -64,7 +67,8 @@ def test_should_show_service_name_confirmation(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -85,7 +89,8 @@ def test_should_redirect_after_service_name_confirmation(app_, mock_get_user, mock_get_user_by_email, mock_login, - mock_verify_password): + mock_verify_password, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -110,7 +115,8 @@ def test_should_show_request_to_go_live(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -130,7 +136,8 @@ def test_should_redirect_after_request_to_go_live(app_, mock_update_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -151,7 +158,8 @@ def test_should_show_status_page(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -170,7 +178,8 @@ def test_should_show_redirect_after_status_change(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -190,7 +199,8 @@ def test_should_show_status_confirmation(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -211,7 +221,8 @@ def test_should_redirect_after_status_confirmation(app_, mock_get_user, mock_get_user_by_email, mock_login, - mock_verify_password): + mock_verify_password, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -232,7 +243,8 @@ def test_should_show_delete_page(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -250,7 +262,8 @@ def test_should_show_redirect_after_deleting_service(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -269,7 +282,8 @@ def test_should_show_delete_confirmation(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -289,7 +303,8 @@ def test_should_redirect_delete_confirmation(app_, mock_get_user, mock_get_user_by_email, mock_login, - mock_verify_password): + mock_verify_password, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 1ec43d502..cff04bde1 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -9,7 +9,8 @@ def test_should_show_page_for_one_templates(app_, mock_get_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -33,7 +34,8 @@ def test_should_redirect_when_saving_a_template(app_, mock_update_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -65,7 +67,8 @@ def test_should_show_delete_template_page(app_, mock_get_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -91,7 +94,8 @@ def test_should_redirect_when_deleting_a_template(app_, mock_delete_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) diff --git a/tests/conftest.py b/tests/conftest.py index a8614c429..3afba0d1b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -519,7 +519,7 @@ def mock_get_jobs(mocker): @pytest.fixture(scope='function') def mock_has_permissions(mocker): - def _has_permission(service_id, permissions): + def _has_permission(service_id, permissions, or_=False): return True return mocker.patch( 'app.notify_client.user_api_client.User.has_permissions', From efaf826e867235d1cc45d8d3a44257702ec86f11 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 1 Mar 2016 15:53:58 +0000 Subject: [PATCH 2/4] Merge with master. --- app/main/views/send.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index a626dfded..c22d1fb08 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -29,17 +29,26 @@ from app import job_api_client from app.utils import ( validate_recipient, InvalidPhoneError, InvalidEmailError, user_has_permissions) -page_headings = { - 'manage_service': { - 'email': 'Send emails', - 'sms': 'Send text messages'}, - 'manage_templates': { - 'email': 'Manage templates', - 'sms': 'Manage templates' - } + +manage_service_page_headings = { + 'email': 'Send emails', + 'sms': 'Send text messages' } +manage_templates_page_headings = { + 'email': 'Manage templates', + 'sms': 'Manage templates' +} + + +def get_page_headings(template_type): + if current_user.has_permissions(session.get('service_id', ''), 'manage_service'): + return manage_service_page_headings[template_type] + else: + return manage_templates_page_headings[template_type] + + @main.route("/services//send/letters", methods=['GET']) def letters_stub(service_id): return render_template( @@ -63,9 +72,6 @@ def choose_template(service_id, template_type): abort(404) else: raise e - # TODO fix up how page_heading is loaded. - page_heading = page_headings['manage_service'][template_type] if current_user.has_permissions(session.get('service_id', ''), 'manage_service') else \ - page_headings['manage_templates'][template_type] return render_template( 'views/choose-template.html', templates=[ @@ -76,7 +82,7 @@ def choose_template(service_id, template_type): if template['template_type'] == template_type ], template_type=template_type, - page_heading=page_heading, + page_heading=get_page_headings(template_type), service=service, has_jobs=len(jobs), service_id=service_id @@ -188,7 +194,7 @@ def check_messages(service_id, upload_id): 'views/check.html', upload_result=upload_result, template=template, - page_heading=page_headings[template.template_type], + page_heading=get_page_headings(template.template_type), column_headers=['to'] + list(template.placeholders_as_markup), original_file_name=upload_data.get('original_file_name'), service_id=service_id, From 1b59e5c7f11de3d5804b9fc514786ea86106a411 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 09:02:56 +0000 Subject: [PATCH 3/4] Review comments fixed. All tests passing. --- app/main/views/send.py | 2 +- app/notify_client/models.py | 5 +- app/templates/main_nav.html | 12 ++--- app/templates/views/choose-template.html | 8 +-- app/utils.py | 4 +- tests/app/main/test_utils.py | 65 ++++++++++++++++++------ tests/conftest.py | 11 +++- 7 files changed, 76 insertions(+), 31 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 37415b54b..1c3e9f206 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -44,7 +44,7 @@ manage_templates_page_headings = { def get_page_headings(template_type): - if current_user.has_permissions(session.get('service_id', ''), 'manage_service'): + if current_user.has_permissions(['manage_service']): return manage_service_page_headings[template_type] else: return manage_templates_page_headings[template_type] diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 3cf6497b6..b881e8390 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -1,4 +1,5 @@ from flask.ext.login import (UserMixin, login_fresh) +from flask import session class User(UserMixin): @@ -81,7 +82,9 @@ class User(UserMixin): def permissions(self, permissions): raise AttributeError("Read only property") - def has_permissions(self, service_id, permissions, or_=False): + def has_permissions(self, permissions, service_id=None, or_=False): + if service_id is None: + service_id = session.get('service_id', '') if service_id in self._permissions: if or_: return any([x in self._permissions[service_id] for x in permissions]) diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index cfc6cd0d1..96a360727 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -2,26 +2,26 @@

      {{ session.get('service_name', 'Service') }}

      - {% if current_user.has_permissions(session.get('service_id', ''), ['send_messages']) %} + {% if current_user.has_permissions(['send_messages']) %} - {% elif current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% elif current_user.has_permissions(['manage_templates']) %} {% endif %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% if current_user.has_permissions(['manage_service']) %} {% endif %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_api_keys']) %} + {% if current_user.has_permissions(['manage_api_keys']) %}
      • API keys
      • Developer documentation
        • diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index b2d9545f5..bf6f08f12 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -16,7 +16,7 @@ {% if templates %} {% if not has_jobs %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% if current_user.has_permissions(['manage_service']) %} {{ banner( """ Send yourself a test message @@ -41,11 +41,11 @@
        - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% if current_user.has_permissions(['manage_service']) %} Add recipients Send yourself a test {% endif %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% if current_user.has_permissions(['manage_templates']) %} Edit template {% endif %}
        @@ -55,7 +55,7 @@ {% endif %}

        - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% if current_user.has_permissions(['manage_templates']) %} Add a new template {% endif %}

        diff --git a/app/utils.py b/app/utils.py index 965939028..11eb18876 100644 --- a/app/utils.py +++ b/app/utils.py @@ -100,10 +100,8 @@ def user_has_permissions(*permissions, or_=False): def wrap(func): @wraps(func) def wrap_func(*args, **kwargs): - # We are making the assumption that the user is logged in. from flask_login import current_user - service_id = session.get('service_id', '') - if current_user and current_user.has_permissions(service_id, permissions, or_=or_): + if current_user and current_user.has_permissions(permissions, or_=or_): return func(*args, **kwargs) else: abort(403) diff --git a/tests/app/main/test_utils.py b/tests/app/main/test_utils.py index 67262c360..95b435f6e 100644 --- a/tests/app/main/test_utils.py +++ b/tests/app/main/test_utils.py @@ -6,18 +6,53 @@ from app.main.views.index import index from werkzeug.exceptions import Forbidden -# def test_user_has_permissions(app_, -# api_user_active, -# mock_get_user, -# mock_get_user_by_email, -# mock_login): -# with app_.test_request_context(): -# with app_.test_client() as client: -# client.login(api_user_active) -# decorator = user_has_permissions('something') -# decorated_index = decorator(index) -# try: -# response = decorated_index() -# pytest.fail("Failed to throw a forbidden exception") -# except Forbidden: -# pass +def test_user_has_permissions_on_endpoint_fail(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('something') + decorated_index = decorator(index) + try: + response = decorated_index() + pytest.fail("Failed to throw a forbidden exception") + except Forbidden: + pass + + +def test_user_has_permissions_success(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('manage_users') + decorated_index = decorator(index) + response = decorated_index() + + +def test_user_has_permissions_or(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('something', 'manage_users', or_=True) + decorated_index = decorator(index) + response = decorated_index() + + +def test_user_has_permissions_multiple(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('manage_templates', 'manage_users') + decorated_index = decorator(index) + response = decorated_index() diff --git a/tests/conftest.py b/tests/conftest.py index 81c159cf2..a3d4f54aa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -305,6 +305,15 @@ def mock_get_user_by_email(mocker, api_user_active): return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) +@pytest.fixture(scope='function') +def mock_get_user_with_permissions(mocker, api_user_active): + def _get_user(id): + api_user_active._permissions[''] = ['manage_users', 'manage_templates', 'manage_settings'] + return api_user_active + return mocker.patch( + 'app.user_api_client.get_user', side_effect=_get_user) + + @pytest.fixture(scope='function') def mock_dont_get_user_by_email(mocker): @@ -523,7 +532,7 @@ def mock_get_jobs(mocker): @pytest.fixture(scope='function') def mock_has_permissions(mocker): - def _has_permission(service_id, permissions, or_=False): + def _has_permission(permissions, service_id=None, or_=False): return True return mocker.patch( 'app.notify_client.user_api_client.User.has_permissions', From e3fce349e1f822bf3a810da52332e6a8e02715c8 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 09:30:30 +0000 Subject: [PATCH 4/4] Fix tests with master. --- tests/app/main/views/test_send.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e1e4762e9..9c7e2a30f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -156,7 +156,8 @@ def test_send_test_message_to_self( mock_login, mock_get_service, mock_get_service_email_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): expected_data = {'data': ['email address', 'test@user.gov.uk'], 'file_name': 'Test run'}