diff --git a/h/auth/policy.py b/h/auth/policy.py index 0f836a9f21c..f8444ac3f1f 100644 --- a/h/auth/policy.py +++ b/h/auth/policy.py @@ -17,6 +17,7 @@ ('api.groups', 'POST'), ('api.group_member', 'POST'), ('api.users', 'POST'), + ('api.user', 'PATCH'), ] diff --git a/h/models/user.py b/h/models/user.py index 2740b14bde8..1957c0b7cd0 100644 --- a/h/models/user.py +++ b/h/models/user.py @@ -7,6 +7,7 @@ import sqlalchemy as sa from sqlalchemy.ext.hybrid import Comparator, hybrid_property from sqlalchemy.ext.declarative import declared_attr +from pyramid import security from h._compat import string_types from h.db import Base @@ -314,5 +315,17 @@ def get_by_username(cls, session, username, authority): cls.authority == authority, ).first() + def __acl__(self): + terms = [] + + # auth_clients that have the same authority as the user + # may update the user + user_update_principal = "client_authority:{}".format(self.authority) + terms.append((security.Allow, user_update_principal, 'update')) + + terms.append(security.DENY_ALL) + + return terms + def __repr__(self): return '' % self.username diff --git a/h/routes.py b/h/routes.py index 06e098654cf..723b9ca5840 100644 --- a/h/routes.py +++ b/h/routes.py @@ -118,7 +118,10 @@ def includeme(config): config.add_route('api.users', '/api/users', factory='h.traversal.UserRoot') - config.add_route('api.user', '/api/users/{username}') + config.add_route('api.user', + '/api/users/{username}', + factory='h.traversal.UserRoot', + traverse='/{username}') config.add_route('badge', '/api/badge') config.add_route('token', '/api/token') config.add_route('oauth_authorize', '/oauth/authorize') diff --git a/h/traversal/roots.py b/h/traversal/roots.py index 25f6e872a27..f400ac1a097 100644 --- a/h/traversal/roots.py +++ b/h/traversal/roots.py @@ -76,6 +76,7 @@ from h.models import Group from h.models import Organization from h.auth import role +from h.auth.util import client_authority from h.interfaces import IGroupService from h.traversal import contexts @@ -220,8 +221,8 @@ def __init__(self, request): self.user_svc = self.request.find_service(name='user') def __getitem__(self, username): - # FIXME: At present, this fetch would never work for third-party users - user = self.user_svc.fetch(username, self.request.default_authority) + authority = client_authority(self.request) or self.request.default_authority + user = self.user_svc.fetch(username, authority) if not user: raise KeyError() diff --git a/h/views/api/users.py b/h/views/api/users.py index 3a934045ad6..05099496a55 100644 --- a/h/views/api/users.py +++ b/h/views/api/users.py @@ -2,9 +2,7 @@ from __future__ import unicode_literals -from pyramid.exceptions import HTTPNotFound - -from h.auth.util import request_auth_client, client_authority +from h.auth.util import client_authority from h.exceptions import PayloadError, ConflictError from h.presenters import UserJSONPresenter from h.schemas.api.user import CreateUserAPISchema, UpdateUserAPISchema @@ -58,22 +56,16 @@ def create(request): return presenter.asdict() -@json_view(route_name='api.user', request_method='PATCH') -def update(request): +@json_view(route_name='api.user', + request_method='PATCH', + permission='update') +def update(user, request): """ Update a user. This API endpoint allows authorised clients (those able to provide a valid Client ID and Client Secret) to update users in their authority. """ - client = request_auth_client(request) - - user_svc = request.find_service(name='user') - user = user_svc.fetch(request.matchdict['username'], - client.authority) - if user is None: - raise HTTPNotFound() - schema = UpdateUserAPISchema() appstruct = schema.validate(_json_payload(request)) diff --git a/tests/functional/api/test_users.py b/tests/functional/api/test_users.py index 6379cf9980c..95a2498d6d7 100644 --- a/tests/functional/api/test_users.py +++ b/tests/functional/api/test_users.py @@ -66,6 +66,46 @@ def test_it_returns_409_if_user_conflict(self, app, user_payload, auth_client_he assert res.status_code == 409 +@pytest.mark.functional +class TestUpdateUser(object): + + def test_it_returns_http_200_when_successful(self, app, auth_client_header, user, patch_user_payload): + url = "/api/users/{username}".format(username=user.username) + + res = app.patch_json(url, patch_user_payload, headers=auth_client_header) + + assert res.status_code == 200 + + def test_it_returns_updated_user_when_successful(self, app, auth_client_header, user, patch_user_payload): + url = "/api/users/{username}".format(username=user.username) + + res = app.patch_json(url, patch_user_payload, headers=auth_client_header) + + assert res.json_body['email'] == patch_user_payload['email'] + assert res.json_body['display_name'] == patch_user_payload['display_name'] + + def test_it_returns_http_404_if_auth_client_missing(self, app, user, patch_user_payload): + url = "/api/users/{username}".format(username=user.username) + + res = app.patch_json(url, patch_user_payload, expect_errors=True) + + assert res.status_code == 404 + + def test_it_returns_http_404_if_user_not_in_client_authority(self, + app, + auth_client_header, + user, + patch_user_payload, + db_session): + user.authority = 'somewhere.com' + db_session.commit() + url = "/api/users/{username}".format(username=user.username) + + res = app.patch_json(url, patch_user_payload, headers=auth_client_header, expect_errors=True) + + assert res.status_code == 404 + + @pytest.fixture def user_payload(): return { @@ -75,6 +115,14 @@ def user_payload(): } +@pytest.fixture +def patch_user_payload(): + return { + "email": "filip@example2.com", + "display_name": "Filip Pilip", + } + + @pytest.fixture def auth_client(db_session, factories): auth_client = factories.ConfidentialAuthClient(authority='example.com', diff --git a/tests/h/models/user_test.py b/tests/h/models/user_test.py index 3dff9647684..3286791da26 100644 --- a/tests/h/models/user_test.py +++ b/tests/h/models/user_test.py @@ -3,6 +3,7 @@ import pytest from sqlalchemy import exc +from pyramid.authorization import ACLAuthorizationPolicy from h import models @@ -268,3 +269,29 @@ def users(self, db_session, factories): } db_session.flush() return users + + +class TestUserACL(object): + def test_auth_client_with_matching_authority_may_update_user(self, user, authz_policy): + user.authority = 'weewhack.com' + + assert authz_policy.permits(user, ['flip', 'client_authority:weewhack.com'], 'update') + + def test_auth_client_without_matching_authority_may_not_update_user(self, user, authz_policy): + user.authority = 'weewhack.com' + + assert not authz_policy.permits(user, ['flip', 'client_authority:2weewhack.com'], 'update') + + def test_user_with_authority_may_not_update_user(self, user, authz_policy): + user.authority = 'fabuloso.biz' + + assert not authz_policy.permits(user, ['flip', 'authority:fabuloso.biz'], 'update') + + @pytest.fixture + def authz_policy(self): + return ACLAuthorizationPolicy() + + @pytest.fixture + def user(self): + user = models.User(username='filip', authority='example.com') + return user diff --git a/tests/h/routes_test.py b/tests/h/routes_test.py index a59306ab363..437ff872238 100644 --- a/tests/h/routes_test.py +++ b/tests/h/routes_test.py @@ -96,7 +96,7 @@ def test_includeme(): 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', factory='h.traversal.UserRoot'), - call('api.user', '/api/users/{username}'), + call('api.user', '/api/users/{username}', factory='h.traversal.UserRoot', traverse='/{username}'), call('badge', '/api/badge'), call('token', '/api/token'), call('oauth_authorize', '/oauth/authorize'), diff --git a/tests/h/traversal/roots_test.py b/tests/h/traversal/roots_test.py index 3f043a36206..c50cf5f1abf 100644 --- a/tests/h/traversal/roots_test.py +++ b/tests/h/traversal/roots_test.py @@ -274,7 +274,8 @@ def group_factory(self, pyramid_request): return GroupRoot(pyramid_request) -@pytest.mark.usefixtures('user_service') +@pytest.mark.usefixtures('user_service', + 'client_authority') class TestUserRoot(object): def test_it_does_not_assign_create_permission_without_auth_client_role(self, pyramid_config, pyramid_request): @@ -300,6 +301,18 @@ def test_it_fetches_the_requested_user(self, pyramid_request, user_factory, user user_service.fetch.assert_called_once_with("bob", pyramid_request.default_authority) + def test_it_proxies_to_client_authority(self, pyramid_request, user_factory, client_authority, user_service): + user_factory["bob"] + + client_authority.assert_called_once_with(pyramid_request) + user_service.fetch.assert_called_once_with("bob", pyramid_request.default_authority) + + def test_it_fetches_with_client_authority_if_present(self, pyramid_request, user_factory, client_authority, user_service): + client_authority.return_value = 'something.com' + user_factory["bob"] + + user_service.fetch.assert_called_once_with("bob", client_authority.return_value) + def test_it_raises_KeyError_if_the_user_does_not_exist(self, user_factory, user_service): @@ -323,6 +336,12 @@ def user_service(self, pyramid_config): pyramid_config.register_service(user_service, name='user') return user_service + @pytest.fixture + def client_authority(self, patch): + client_authority = patch('h.traversal.roots.client_authority') + client_authority.return_value = None + return client_authority + @pytest.fixture def organizations(factories): diff --git a/tests/h/views/api/users_test.py b/tests/h/views/api/users_test.py index b11a0cf64de..cf5e0807f58 100644 --- a/tests/h/views/api/users_test.py +++ b/tests/h/views/api/users_test.py @@ -5,8 +5,6 @@ import pytest import mock -from pyramid.exceptions import HTTPNotFound - from h.exceptions import PayloadError, ConflictError from h.models.auth_client import GrantType from h.schemas import ValidationError @@ -116,19 +114,18 @@ def valid_payload(self): @pytest.mark.usefixtures('auth_client', - 'request_auth_client', 'user_svc', 'user') class TestUpdate(object): def test_it_updates_display_name(self, pyramid_request, valid_payload, user): pyramid_request.json_body = valid_payload - update(pyramid_request) + update(user, pyramid_request) assert user.display_name == 'Jeremy Weyland' def test_it_updates_email(self, pyramid_request, valid_payload, user): pyramid_request.json_body = valid_payload - update(pyramid_request) + update(user, pyramid_request) assert user.email == 'jeremy@weylandtech.com' @@ -141,7 +138,7 @@ def test_you_can_update_the_displayname_of_a_user_who_has_no_email( valid_payload['display_name'] = 'new_display_name' pyramid_request.json_body = valid_payload - update(pyramid_request) + update(user, pyramid_request) assert user.display_name == 'new_display_name' assert user.email is None @@ -155,51 +152,45 @@ def test_you_can_add_an_email_to_a_user_who_has_no_email( valid_payload['email'] = 'new@new.com' pyramid_request.json_body = valid_payload - update(pyramid_request) + update(user, pyramid_request) assert user.email == 'new@new.com' def test_it_presents_user(self, pyramid_request, valid_payload, user, presenter): pyramid_request.json_body = valid_payload - update(pyramid_request) + update(user, pyramid_request) presenter.assert_called_once_with(user) def test_it_returns_presented_user(self, pyramid_request, valid_payload, presenter): pyramid_request.json_body = valid_payload - result = update(pyramid_request) + result = update(user, pyramid_request) assert result == presenter.return_value.asdict() - def test_raises_404_when_user_not_found(self, pyramid_request, valid_payload): - pyramid_request.matchdict['username'] = 'missing' - - with pytest.raises(HTTPNotFound): - update(pyramid_request) - - def test_it_validates_the_input(self, pyramid_request, valid_payload, UpdateUserAPISchema): + def test_it_validates_the_input(self, user, pyramid_request, valid_payload, UpdateUserAPISchema): update_schema = UpdateUserAPISchema.return_value update_schema.validate.return_value = valid_payload pyramid_request.json_body = valid_payload - update(pyramid_request) + update(user, pyramid_request) update_schema.validate.assert_called_once_with(valid_payload) - def test_raises_when_schema_validation_fails(self, pyramid_request, valid_payload, UpdateUserAPISchema): + def test_raises_when_schema_validation_fails(self, user, pyramid_request, valid_payload, UpdateUserAPISchema): update_schema = UpdateUserAPISchema.return_value update_schema.validate.side_effect = ValidationError('validation failed') pyramid_request.json_body = valid_payload with pytest.raises(ValidationError): - update(pyramid_request) + update(user, pyramid_request) - def test_raises_for_invalid_json_body(self, pyramid_request, patch): + def test_raises_for_invalid_json_body(self, user, pyramid_request, patch): type(pyramid_request).json_body = mock.PropertyMock(side_effect=ValueError()) with pytest.raises(PayloadError): - update(pyramid_request) + update(user, pyramid_request) @pytest.fixture def pyramid_request(self, pyramid_request, user): @@ -233,13 +224,6 @@ def auth_client(factories): grant_type=GrantType.client_credentials) -@pytest.fixture -def request_auth_client(patch, auth_client): - request_auth_client = patch('h.views.api.users.request_auth_client') - request_auth_client.return_value = auth_client - return request_auth_client - - @pytest.fixture def validate_auth_client_authority(patch): return patch('h.views.api.users.validate_auth_client_authority')