From 28a96d916877a8193767fa8be6066562540e48c9 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Wed, 12 Sep 2018 15:17:06 -0400 Subject: [PATCH 1/5] Add `create` permission to `GroupRoot` --- h/traversal/roots.py | 4 ++++ tests/h/traversal/roots_test.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/h/traversal/roots.py b/h/traversal/roots.py index 79498083ac5..25f6e872a27 100644 --- a/h/traversal/roots.py +++ b/h/traversal/roots.py @@ -211,6 +211,10 @@ class UserRoot(object): FIXME: This class should return UserContext objects, not User objects. """ + __acl__ = [ + (Allow, role.AuthClient, 'create'), + ] + def __init__(self, request): self.request = request self.user_svc = self.request.find_service(name='user') diff --git a/tests/h/traversal/roots_test.py b/tests/h/traversal/roots_test.py index 8c8aedb0954..3f043a36206 100644 --- a/tests/h/traversal/roots_test.py +++ b/tests/h/traversal/roots_test.py @@ -277,6 +277,24 @@ def group_factory(self, pyramid_request): @pytest.mark.usefixtures('user_service') class TestUserRoot(object): + def test_it_does_not_assign_create_permission_without_auth_client_role(self, pyramid_config, pyramid_request): + policy = pyramid.authorization.ACLAuthorizationPolicy() + pyramid_config.testing_securitypolicy('acct:adminuser@foo') + pyramid_config.set_authorization_policy(policy) + + context = UserRoot(pyramid_request) + + assert not pyramid_request.has_permission('create', context) + + def test_it_assigns_create_permission_to_auth_client_role(self, pyramid_config, pyramid_request): + policy = pyramid.authorization.ACLAuthorizationPolicy() + pyramid_config.testing_securitypolicy('acct:adminuser@foo', groupids=[role.AuthClient]) + pyramid_config.set_authorization_policy(policy) + + context = UserRoot(pyramid_request) + + assert pyramid_request.has_permission('create', context) + def test_it_fetches_the_requested_user(self, pyramid_request, user_factory, user_service): user_factory["bob"] From 307abcdae32b837c1458b1345fae1fd9bea3fa64 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Wed, 12 Sep 2018 15:20:54 -0400 Subject: [PATCH 2/5] Add `UserRoot` as route factory for `/api/users` --- h/routes.py | 4 +++- tests/h/routes_test.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/h/routes.py b/h/routes.py index c029a59ccf6..06e098654cf 100644 --- a/h/routes.py +++ b/h/routes.py @@ -115,7 +115,9 @@ def includeme(config): factory='h.traversal.GroupRoot', traverse='/{pubid}') config.add_route('api.search', '/api/search') - config.add_route('api.users', '/api/users') + config.add_route('api.users', + '/api/users', + factory='h.traversal.UserRoot') config.add_route('api.user', '/api/users/{username}') config.add_route('badge', '/api/badge') config.add_route('token', '/api/token') diff --git a/tests/h/routes_test.py b/tests/h/routes_test.py index 50779042617..a59306ab363 100644 --- a/tests/h/routes_test.py +++ b/tests/h/routes_test.py @@ -95,7 +95,7 @@ def test_includeme(): call('api.debug_token', '/api/debug-token'), call('api.group_member', '/api/groups/{pubid}/members/{userid}', factory='h.traversal.GroupRoot', traverse='/{pubid}'), call('api.search', '/api/search'), - call('api.users', '/api/users'), + call('api.users', '/api/users', factory='h.traversal.UserRoot'), call('api.user', '/api/users/{username}'), call('badge', '/api/badge'), call('token', '/api/token'), From 0fbc240d04be6a506c3bbad4f6a9a5b02c309869 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 13 Sep 2018 10:30:19 -0400 Subject: [PATCH 3/5] Update AuthClientPolicy route whitelisting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The former method of pattern-matching on request.path was kludgy and resulted in at least one bug from oversight. The new whitelist here is easier to read as a human and is matched against the request’s `matched_route` object. --- h/auth/policy.py | 26 +++++---- tests/h/auth/policy_test.py | 108 ++++++++++++++++++++++-------------- 2 files changed, 81 insertions(+), 53 deletions(-) diff --git a/h/auth/policy.py b/h/auth/policy.py index 19588da8e8c..0f836a9f21c 100644 --- a/h/auth/policy.py +++ b/h/auth/policy.py @@ -2,8 +2,6 @@ from __future__ import unicode_literals -import re - import pyramid.compat from pyramid import interfaces from pyramid.authentication import BasicAuthAuthenticationPolicy @@ -13,10 +11,13 @@ from h.auth import util -# As we roll out the new API Auth Policy with Auth Token Policy, we -# want to keep it restricted to certain endpoints -# Currently restricted to `POST /api/groups*` only -AUTH_TOKEN_PATH_PATTERN = r"^/api/groups" +#: List of route name-method combinations that should +#: allow AuthClient authentication +AUTH_CLIENT_API_WHITELIST = [ + ('api.groups', 'POST'), + ('api.group_member', 'POST'), + ('api.users', 'POST'), +] @interface.implementer(interfaces.IAuthenticationPolicy) @@ -359,14 +360,17 @@ def _is_client_request(request): """ Is client_auth authentication valid for the given request? - For initial rollout, authentication should be performed by + Uuthentication should be performed by :py:class:`~h.auth.policy.AuthClientPolicy` only for requests - to the `POST /api/groups` endpoint. + that match the whitelist. + + The whitelist will likely evolve. - This is intended to be temporary. + :rtype: bool """ - return (re.match(AUTH_TOKEN_PATH_PATTERN, request.path) and - request.method == 'POST') + if request.matched_route: + return (request.matched_route.name, request.method) in AUTH_CLIENT_API_WHITELIST + return False def _is_ws_request(request): diff --git a/tests/h/auth/policy_test.py b/tests/h/auth/policy_test.py index a6a60a08e3b..b09509672cc 100644 --- a/tests/h/auth/policy_test.py +++ b/tests/h/auth/policy_test.py @@ -15,6 +15,7 @@ from h.auth.policy import TokenAuthenticationPolicy from h.auth.policy import APIAuthenticationPolicy from h.auth.policy import AuthClientPolicy +from h.auth.policy import AUTH_CLIENT_API_WHITELIST from h.services.user import UserService @@ -31,17 +32,12 @@ '/api/token', ) -AUTH_CLIENT_PATHS = ( - '/api/groups', - '/api/groups/', - '/api/groups/foobar/members/something', -) - -NON_AUTH_CLIENT_PATHS = ( - '/api/badge', - '/api', - '/groups/' -) +AUTH_CLIENT_API_BLACKLIST = [ + ('api.groups', 'GET'), + ('api.user', 'POST'), + ('group_create', 'POST'), + ('api.group_member', 'DELETE') +] class TestAuthenticationPolicy(object): @@ -143,14 +139,16 @@ def test_authenticated_userid_proxies_to_user_policy_first(self, assert client_policy.authenticated_userid.call_count == 0 assert userid == user_policy.authenticated_userid.return_value - @pytest.mark.parametrize('path', AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_WHITELIST) def test_authenticated_userid_proxies_to_client_policy_if_user_fails(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.authenticated_userid.return_value = None userid = api_policy.authenticated_userid(pyramid_request) @@ -159,14 +157,16 @@ def test_authenticated_userid_proxies_to_client_policy_if_user_fails(self, client_policy.authenticated_userid.assert_called_once_with(pyramid_request) assert userid == client_policy.authenticated_userid.return_value - @pytest.mark.parametrize('path', NON_AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_BLACKLIST) def test_authenticated_userid_does_not_proxy_to_client_policy_if_path_mismatch(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.authenticated_userid.return_value = None userid = api_policy.authenticated_userid(pyramid_request) @@ -175,14 +175,16 @@ def test_authenticated_userid_does_not_proxy_to_client_policy_if_path_mismatch(s assert client_policy.authenticated_userid.call_count == 0 assert userid == user_policy.authenticated_userid.return_value - @pytest.mark.parametrize('path', AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_WHITELIST) def test_unauthenticated_userid_proxies_to_user_policy_first(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name userid = api_policy.unauthenticated_userid(pyramid_request) user_policy.unauthenticated_userid.assert_called_once_with(pyramid_request) @@ -202,14 +204,16 @@ def test_unauthenticated_userid_proxies_to_client_policy_if_user_fails(self, client_policy.unauthenticated_userid.assert_called_once_with(pyramid_request) assert userid == client_policy.unauthenticated_userid.return_value - @pytest.mark.parametrize('path', NON_AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_BLACKLIST) def test_unauthenticated_userid_does_not_proxy_to_client_policy_if_path_mismatch(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.unauthenticated_userid.return_value = None userid = api_policy.unauthenticated_userid(pyramid_request) @@ -231,14 +235,16 @@ def test_effective_principals_proxies_to_user_policy_first(self, assert client_policy.effective_principals.call_count == 0 assert principals == user_policy.effective_principals.return_value - @pytest.mark.parametrize('path', AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_WHITELIST) def test_effective_principals_proxies_to_client_if_auth_principal_missing(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.effective_principals.return_value = [Everyone] principals = api_policy.effective_principals(pyramid_request) @@ -247,14 +253,16 @@ def test_effective_principals_proxies_to_client_if_auth_principal_missing(self, client_policy.effective_principals.assert_called_once_with(pyramid_request) assert principals == client_policy.effective_principals.return_value - @pytest.mark.parametrize('path', NON_AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_BLACKLIST) def test_effective_principals_does_not_proxy_to_client_if_path_mismatch(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.effective_principals.return_value = [Everyone] principals = api_policy.effective_principals(pyramid_request) @@ -263,9 +271,15 @@ def test_effective_principals_does_not_proxy_to_client_if_path_mismatch(self, assert client_policy.effective_principals.call_count == 0 assert principals == user_policy.effective_principals.return_value - @pytest.mark.parametrize('path', AUTH_CLIENT_PATHS) - def test_remember_proxies_to_user_policy_first(self, pyramid_request, api_policy, user_policy, path): - pyramid_request.path = path + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_WHITELIST) + def test_remember_proxies_to_user_policy_first(self, + pyramid_request, + api_policy, + user_policy, + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name remembered = api_policy.remember(pyramid_request, 'acct:foo@bar.com') user_policy.remember.assert_called_once_with(pyramid_request, 'acct:foo@bar.com') @@ -280,14 +294,16 @@ def test_remember_proxies_to_client_policy_second(self, pyramid_request, api_pol client_policy.remember.assert_called_once_with(pyramid_request, 'acct:foo@bar.com') assert remembered == client_policy.remember.return_value - @pytest.mark.parametrize('path', NON_AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_BLACKLIST) def test_remember_does_not_proxy_to_client_if_path_mismatch(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.remember.return_value = [] remembered = api_policy.remember(pyramid_request, 'acct:foo@bar.com') @@ -296,9 +312,15 @@ def test_remember_does_not_proxy_to_client_if_path_mismatch(self, assert client_policy.remember.call_count == 0 assert remembered == user_policy.remember.return_value - @pytest.mark.parametrize('path', AUTH_CLIENT_PATHS) - def test_forget_proxies_to_user_policy_first(self, pyramid_request, api_policy, user_policy, path): - pyramid_request.path = path + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_WHITELIST) + def test_forget_proxies_to_user_policy_first(self, + pyramid_request, + api_policy, + user_policy, + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name forgot = api_policy.forget(pyramid_request) user_policy.forget.assert_called_once_with(pyramid_request) @@ -313,14 +335,16 @@ def test_forget_proxies_to_client_policy_second(self, pyramid_request, api_polic client_policy.forget.assert_called_once_with(pyramid_request) assert forgot == client_policy.forget.return_value - @pytest.mark.parametrize('path', NON_AUTH_CLIENT_PATHS) + @pytest.mark.parametrize('route_name,route_method', AUTH_CLIENT_API_BLACKLIST) def test_forget_does_not_proxy_to_client_if_path_mismatch(self, pyramid_request, api_policy, user_policy, client_policy, - path): - pyramid_request.path = path + route_name, + route_method): + pyramid_request.method = route_method + pyramid_request.matched_route.name = route_name user_policy.forget.return_value = [] forgot = api_policy.forget(pyramid_request) @@ -344,7 +368,7 @@ def api_policy(self, client_policy, user_policy): @pytest.fixture def pyramid_request(self, pyramid_request): - pyramid_request.path = '/api/groups' + pyramid_request.matched_route.name = 'api.groups' pyramid_request.method = 'POST' return pyramid_request From 3e617ac273d4bf5551a4eaf3e16ba7e8962aaf68 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 13 Sep 2018 11:21:22 -0400 Subject: [PATCH 4/5] Protect `POST /api/users` with permission instead of view logic Remove view-level auth* checking on auth-client as authn now happens in AuthClientPolicy and authz via a permission on the `UserRoot` traversal root. This endpoint will temporarily return a 404 instead of a 403 on authz fail because of the way that API view exceptions intercept 403s. To be fixed soon --- h/views/api/users.py | 15 ++++++++------- tests/functional/api/test_users.py | 7 +++++++ tests/h/views/api/users_test.py | 10 +++++++--- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/h/views/api/users.py b/h/views/api/users.py index 836c6f71593..0425585f1c0 100644 --- a/h/views/api/users.py +++ b/h/views/api/users.py @@ -4,7 +4,7 @@ from pyramid.exceptions import HTTPNotFound -from h.auth.util import request_auth_client, validate_auth_client_authority +from h.auth.util import request_auth_client, client_authority from h.exceptions import PayloadError, ConflictError from h.presenters import UserJSONPresenter from h.schemas.api.user import CreateUserAPISchema, UpdateUserAPISchema @@ -12,7 +12,9 @@ from h.util.view import json_view -@json_view(route_name='api.users', request_method='POST') +@json_view(route_name='api.users', + request_method='POST', + permission='create') def create(request): """ Create a user. @@ -22,18 +24,17 @@ def create(request): users are created pre-activated, and are unable to log in to the web service directly. """ - client = request_auth_client(request) - + applied_authority = client_authority(request) schema = CreateUserAPISchema() appstruct = schema.validate(_json_payload(request)) - validate_auth_client_authority(client, appstruct['authority']) - appstruct['authority'] = client.authority + # Enforce authority match to currently-active auth-client authority + appstruct['authority'] = applied_authority user_unique_service = request.find_service(name='user_unique') try: - user_unique_service.ensure_unique(appstruct, authority=client.authority) + user_unique_service.ensure_unique(appstruct, authority=applied_authority) except DuplicateUserError as err: raise ConflictError(err) diff --git a/tests/functional/api/test_users.py b/tests/functional/api/test_users.py index 613c71af9bf..e7d17fb2436 100644 --- a/tests/functional/api/test_users.py +++ b/tests/functional/api/test_users.py @@ -25,6 +25,13 @@ def test_it_returns_http_200_when_successful(self, app, auth_client_header, user assert res.status_code == 200 + def test_it_returns_404_if_missing_auth_client(self, app, user_payload): + # FIXME: This should return a 403; our exception views squash it into a 404 + res = app.post_json("/api/users", user_payload, expect_errors=True) + + assert res.status_code == 404 + + @pytest.mark.xfail def test_it_returns_403_if_missing_auth_client(self, app, user_payload): res = app.post_json("/api/users", user_payload, expect_errors=True) diff --git a/tests/h/views/api/users_test.py b/tests/h/views/api/users_test.py index ad3c487babc..e2a1e9df9d9 100644 --- a/tests/h/views/api/users_test.py +++ b/tests/h/views/api/users_test.py @@ -15,9 +15,7 @@ from h.views.api.users import create, update -@pytest.mark.usefixtures('auth_client', - 'request_auth_client', - 'validate_auth_client_authority', +@pytest.mark.usefixtures('client_authority', 'user_signup_service', 'user_unique_svc') class TestCreate(object): @@ -90,6 +88,12 @@ def test_raises_for_invalid_json_body(self, pyramid_request, patch): with pytest.raises(PayloadError): create(pyramid_request) + @pytest.fixture + def client_authority(self, patch): + client_authority = patch('h.views.api.users.client_authority') + client_authority.return_value = 'weylandindustries.com' + return client_authority + @pytest.fixture def valid_payload(self): return { From 065f787ca42671b1e95fb3d9f5002bb592b246f4 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Wed, 26 Sep 2018 09:11:40 -0400 Subject: [PATCH 5/5] raise `ValidationError` if `authority` param mismatch Raise a `ValidationError` if supplied `authority` param does not match the verified auth client authority --- h/views/api/users.py | 22 ++++++++++++++++++---- tests/functional/api/test_users.py | 15 +++++++++++++++ tests/h/views/api/users_test.py | 7 +++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/h/views/api/users.py b/h/views/api/users.py index 0425585f1c0..3a934045ad6 100644 --- a/h/views/api/users.py +++ b/h/views/api/users.py @@ -8,6 +8,7 @@ from h.exceptions import PayloadError, ConflictError from h.presenters import UserJSONPresenter from h.schemas.api.user import CreateUserAPISchema, UpdateUserAPISchema +from h.schemas import ValidationError from h.services.user_unique import DuplicateUserError from h.util.view import json_view @@ -23,18 +24,31 @@ def create(request): Client ID and Client Secret) to create users in their authority. These users are created pre-activated, and are unable to log in to the web service directly. + + Note: the authority-enforcement logic herein is, by necessity, strange. + The API accepts an ``authority`` parameter but the only valid value for + the param is the client's verified authority. If the param does not + match the client's authority, ``ValidationError`` is raised. + + :raises ValidationError: if ``authority`` param does not match client + authority + :raises ConflictError: if user already exists """ - applied_authority = client_authority(request) + client_authority_ = client_authority(request) schema = CreateUserAPISchema() appstruct = schema.validate(_json_payload(request)) - # Enforce authority match to currently-active auth-client authority - appstruct['authority'] = applied_authority + # Enforce authority match + if appstruct['authority'] != client_authority_: + raise ValidationError( + "authority '{auth_param}' does not match client authority".format( + auth_param=appstruct['authority'] + )) user_unique_service = request.find_service(name='user_unique') try: - user_unique_service.ensure_unique(appstruct, authority=applied_authority) + user_unique_service.ensure_unique(appstruct, authority=client_authority_) except DuplicateUserError as err: raise ConflictError(err) diff --git a/tests/functional/api/test_users.py b/tests/functional/api/test_users.py index e7d17fb2436..6379cf9980c 100644 --- a/tests/functional/api/test_users.py +++ b/tests/functional/api/test_users.py @@ -44,6 +44,21 @@ def test_it_returns_400_if_invalid_payload(self, app, user_payload, auth_client_ assert res.status_code == 400 + def test_it_returns_400_if_authority_param_missing(self, app, user_payload, auth_client_header): + del user_payload["authority"] + + res = app.post_json("/api/users", user_payload, headers=auth_client_header, expect_errors=True) + + assert res.status_code == 400 + + def test_it_returns_400_if_authority_mismatch(self, app, user_payload, auth_client_header): + user_payload["authority"] = "mismatch.com" + + res = app.post_json("/api/users", user_payload, headers=auth_client_header, expect_errors=True) + + assert res.status_code == 400 + assert res.json_body['reason'] == "authority 'mismatch.com' does not match client authority" + def test_it_returns_409_if_user_conflict(self, app, user_payload, auth_client_header, user): # user fixture creates user with conflicting username/authority combo res = app.post_json("/api/users", user_payload, headers=auth_client_header, expect_errors=True) diff --git a/tests/h/views/api/users_test.py b/tests/h/views/api/users_test.py index e2a1e9df9d9..b11a0cf64de 100644 --- a/tests/h/views/api/users_test.py +++ b/tests/h/views/api/users_test.py @@ -65,6 +65,13 @@ def test_raises_when_schema_validation_fails(self, pyramid_request, valid_payloa with pytest.raises(ValidationError): create(pyramid_request) + def test_raises_ValidationError_when_authority_mismatch(self, pyramid_request, valid_payload): + valid_payload['authority'] = 'invalid.com' + pyramid_request.json_body = valid_payload + + with pytest.raises(ValidationError, match="does not match client authority"): + create(pyramid_request) + def test_it_proxies_uniqueness_check_to_service(self, valid_payload, pyramid_request, user_unique_svc, CreateUserAPISchema, auth_client): pyramid_request.json_body = valid_payload CreateUserAPISchema().validate.return_value = valid_payload