Merge pull request #5288 from hypothesis/add-create-user-permission

Add create user permission and update `POST /api/users` authz

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

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

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

h/views/api/users.py

@@ -4,15 +4,18 @@
 
 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
+from h.schemas import ValidationError
 from h.services.user_unique import DuplicateUserError
 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.
@@ -21,19 +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.
-    """
-    client = request_auth_client(request)
 
+    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
+    """
+    client_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
+    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=client.authority)
+        user_unique_service.ensure_unique(appstruct, authority=client_authority_)
     except DuplicateUserError as err:
         raise ConflictError(err)
 

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)
 
@@ -37,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)