has_permissions() now checks user's orgs for <org_id> view args

view args are parameters within the route. for example,
`/organisation/<org_id>/users`. If there is an org_id, then check that
the user is part of that organisation (users.organisations is a list of
all orgs that user is a member of).

* platform admins ignore this check if restrict_admin_usage=False
* if an endpoint has both org_id and service_id, org_id takes
  precedence, but we should probably revisit this if we ever need
  to create such an endpoint.
* you now call `@user_has_permissions()` with no arguments for
  organisation endpoints - we can look at this if we decide we want
  more clarity.
* you should never call user_has_permissions without any arguments
  for endpoints that aren't organisation-based. We'll raise
  NotImplementedError if you do.
This commit is contained in:
Leo Hemsted
2018-03-01 11:34:53 +00:00
parent 3d589887ce
commit d14f33ea70
3 changed files with 66 additions and 21 deletions

View File

@@ -418,19 +418,17 @@ def load_organisation_before_request():
_request_ctx_stack.top.organisation = None
if request.view_args:
org_id = request.view_args.get('org_id', session.get('org_id'))
else:
org_id = session.get('org_id')
org_id = request.view_args.get('org_id')
if org_id:
try:
_request_ctx_stack.top.organisation = organisations_client.get_organisation(org_id)
except HTTPError as exc:
# if org id isn't real, then 404 rather than 500ing later because we expect org to be set
if exc.status_code == 404:
abort(404)
else:
raise
if org_id:
try:
_request_ctx_stack.top.organisation = organisations_client.get_organisation(org_id)
except HTTPError as exc:
# if org id isn't real, then 404 rather than 500ing later because we expect org to be set
if exc.status_code == 404:
abort(404)
else:
raise
def save_service_after_request(response):

View File

@@ -32,6 +32,10 @@ def _get_service_id_from_view_args():
return request.view_args.get('service_id', None)
def _get_org_id_from_view_args():
return request.view_args.get('org_id', None)
def translate_permissions_from_db_to_admin_roles(permissions):
"""
Given a list of database permissions, return a set of roles
@@ -123,9 +127,14 @@ class User(UserMixin):
# 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:
return any(x in self._permissions[service_id] for x in permissions)
return False
org_id = _get_org_id_from_view_args()
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, [])

View File

@@ -44,7 +44,7 @@ def test_user_has_permissions_on_endpoint_fail(
client,
user,
['send_messages'],
False)
will_succeed=False)
def test_user_has_permissions_success(
@@ -57,7 +57,7 @@ def test_user_has_permissions_success(
client,
user,
['manage_service'],
True)
will_succeed=True)
def test_user_has_permissions_or(
@@ -70,7 +70,7 @@ def test_user_has_permissions_or(
client,
user,
['send_messages', 'manage_service'],
True)
will_succeed=True)
def test_user_has_permissions_multiple(
@@ -96,7 +96,7 @@ def test_exact_permissions(
client,
user,
['manage_service', 'manage_templates'],
True)
will_succeed=True)
def test_platform_admin_user_can_access_page_that_has_no_permissions(
@@ -109,7 +109,7 @@ def test_platform_admin_user_can_access_page_that_has_no_permissions(
client,
platform_admin_user,
[],
True)
will_succeed=True)
def test_platform_admin_user_can_not_access_page(
@@ -138,6 +138,43 @@ def test_no_user_returns_401_unauth(
will_succeed=False)
def test_user_has_permissions_for_organisation(
client,
mocker,
):
user = _user_with_permissions()
user.organisations = ['org_1', 'org_2']
mocker.patch('app.user_api_client.get_user', return_value=user)
client.login(user)
request.view_args = {'org_id': 'org_2'}
@user_has_permissions()
def index():
pass
index()
def test_user_doesnt_have_permissions_for_organisation(
client,
mocker,
):
user = _user_with_permissions()
user.organisations = ['org_1', 'org_2']
mocker.patch('app.user_api_client.get_user', return_value=user)
client.login(user)
request.view_args = {'org_id': 'org_3'}
@user_has_permissions()
def index():
pass
with pytest.raises(Forbidden):
index()
def _user_with_permissions():
from app.notify_client.user_api_client import User
@@ -149,7 +186,8 @@ def _user_with_permissions():
'state': 'active',
'failed_login_count': 0,
'permissions': {'foo': ['manage_users', 'manage_templates', 'manage_settings']},
'platform_admin': False
'platform_admin': False,
'organisations': ['org_1', 'org_2'],
}
user = User(user_data)
return user