From 7f268c0ab3f13c05289392831c12cd29323cb3a4 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 1 Mar 2018 12:08:22 +0000
Subject: [PATCH] don't allow us to create permissions decorator without
 permissions

ie: not for an organisation, and not for a service
---
 app/notify_client/models.py         | 15 +++++++-----
 app/templates/components/table.html |  2 +-
 tests/app/main/test_permissions.py  | 36 +++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/app/notify_client/models.py b/app/notify_client/models.py
index 2523bd286..55dbd2f50 100644
--- a/app/notify_client/models.py
+++ b/app/notify_client/models.py
@@ -121,20 +121,23 @@ class User(UserMixin):
         if unknown_permissions:
             raise TypeError('{} are not valid permissions'.format(list(unknown_permissions)))
 
-        # 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
-
         # Service id is always set on the request for service specific views.
         service_id = _get_service_id_from_view_args()
         org_id = _get_org_id_from_view_args()
 
+        if not service_id and not org_id:
+            # we shouldn't have any pages that require permissions, but don't specify a service or organisation.
+            # use @user_is_platform_admin for platform admin only pages
+            raise NotImplementedError
+
+        # 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
+
         if org_id:
             return org_id in self.organisations
         elif service_id:
             return any(x in self._permissions.get(service_id, []) for x in permissions)
-        else:
-            return False
 
     def has_permission_for_service(self, service_id, permission):
         return permission in self._permissions.get(service_id, [])
diff --git a/app/templates/components/table.html b/app/templates/components/table.html
index 7e30a35e9..380d595d0 100644
--- a/app/templates/components/table.html
+++ b/app/templates/components/table.html
@@ -107,7 +107,7 @@
 
 {% macro edit_field(text, link, permissions=[]) -%}
   {% call field(align='right') %}
-    {% if current_user.has_permissions(*permissions) or not permissions %}
+    {% if not permissions or current_user.has_permissions(*permissions) %}
       <a href="{{ link }}">{{ text }}</a>
     {% endif %}
   {% endcall %}
diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py
index 3f5b885ed..08ac18bd5 100644
--- a/tests/app/main/test_permissions.py
+++ b/tests/app/main/test_permissions.py
@@ -156,6 +156,42 @@ def test_user_has_permissions_for_organisation(
     index()
 
 
+def test_platform_admin_can_see_orgs_they_dont_have(
+    client,
+    platform_admin_user,
+    mocker,
+):
+    platform_admin_user.organisations = []
+    mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user)
+    client.login(platform_admin_user)
+
+    request.view_args = {'org_id': 'org_2'}
+
+    @user_has_permissions()
+    def index():
+        pass
+
+    index()
+
+
+def test_cant_use_decorator_without_view_args(
+    client,
+    platform_admin_user,
+    mocker,
+):
+    mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user)
+    client.login(platform_admin_user)
+
+    request.view_args = {}
+
+    @user_has_permissions()
+    def index():
+        pass
+
+    with pytest.raises(NotImplementedError):
+        index()
+
+
 def test_user_doesnt_have_permissions_for_organisation(
     client,
     mocker,