Merge pull request #5318 from hypothesis/add-update-user-permission

Add update-user permission; use AuthClient auth'n for `PATCH /api/user/{username}`

h/auth/policy.py

@@ -17,6 +17,7 @@
     ('api.groups', 'POST'),
     ('api.group_member', 'POST'),
     ('api.users', 'POST'),
+    ('api.user', 'PATCH'),
 ]
 
 

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 '<User: %s>' % self.username

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')

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()

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))
 

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',

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

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'),

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):

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')