Fix distinction and optimize (un)authenticated_userid

cliquet/__init__.py

@@ -5,6 +5,7 @@
 from cornice import Service as CorniceService
 from pyramid.settings import aslist
 
+from cliquet import authentication
 from cliquet import errors
 from cliquet import events
 from cliquet.initialization import (  # NOQA
@@ -78,6 +79,7 @@
     'storage_url': '',
     'storage_max_fetch_size': 10000,
     'storage_pool_size': 25,
+    'tm.annotate_user': False,  # Do annotate transactions with the user-id.
     'transaction_per_request': True,
     'userid_hmac_secret': '',
     'version_prefix_redirect_enabled': True,
@@ -155,7 +157,8 @@ def add_api_capability(config, identifier, description="", url="", **kw):
 
     # Custom helpers.
     config.add_request_method(follow_subrequest)
-    config.add_request_method(lambda request: {'id': request.prefixed_userid},
+    config.add_request_method(authentication.prefixed_userid, property=True)
+    config.add_request_method(lambda r: {'id': r.prefixed_userid},
                               name='get_user_info')
     config.add_request_method(current_resource_name, reify=True)
     config.add_request_method(current_service, reify=True)

cliquet/authentication.py

@@ -3,6 +3,20 @@
 from cliquet import utils
 
 
+def prefixed_userid(request):
+    """In Cliquet users ids are prefixed with the policy name that is
+    contained in Pyramid Multiauth.
+    If a custom authn policy is used, without authn_type, this method returns
+    the user id without prefix.
+    """
+    # If pyramid_multiauth is used, a ``authn_type`` is set on request
+    # when a policy succesfully authenticates a user.
+    # (see :func:`cliquet.initializatio.setup_authentication`)
+    authn_type = getattr(request, 'authn_type', None)
+    if authn_type is not None:
+        return authn_type + ':' + request.selected_userid
+
+
 class BasicAuthAuthenticationPolicy(base_auth.BasicAuthAuthenticationPolicy):
     """Basic auth implementation.
 
@@ -17,6 +31,13 @@ def noop_check(*a):
                                                             *args,
                                                             **kwargs)
 
+    def effective_principals(self, request):
+        """Bypass default Pyramid construction of principals because
+        Pyramid multiauth already adds userid, Authenticated and Everyone
+        principals.
+        """
+        return []
+
     def unauthenticated_userid(self, request):
         settings = request.registry.settings
 

cliquet/authorization.py

@@ -1,10 +1,12 @@
 import functools
+
 from pyramid.settings import aslist
 from pyramid.security import IAuthorizationPolicy, Authenticated
 from zope.interface import implementer
 
 from cliquet import utils
 from cliquet.storage import exceptions as storage_exceptions
+from cliquet.authentication import prefixed_userid
 
 # A permission is called "dynamic" when it's computed at request time.
 DYNAMIC = 'dynamic'
@@ -23,15 +25,13 @@ def groupfinder(userid, request):
     if not backend:
         return []
 
-    # Anonymous safety check.
-    if not hasattr(request, 'prefixed_userid'):
-        return []
+    if request.prefixed_userid:
+        userid = request.prefixed_userid
 
     # Query the permission backend only once per request (e.g. batch).
-
-    reify_key = request.prefixed_userid + '_principals'
+    reify_key = userid + '_principals'
     if reify_key not in request.bound_data:
-        principals = backend.user_principals(request.prefixed_userid)
+        principals = backend.user_principals(userid)
         request.bound_data[reify_key] = principals
 
     return request.bound_data[reify_key]
@@ -48,9 +48,10 @@ def permits(self, context, principals, permission):
             return Authenticated in principals
 
         # Add prefixed user id to principals.
-        if context.prefixed_userid:
-            principals = principals + [context.prefixed_userid]
-            prefix, user_id = context.prefixed_userid.split(':', 1)
+        prefixed_userid = context.get_prefixed_userid()
+        if prefixed_userid and ':' in prefixed_userid:
+            principals = principals + [prefixed_userid]
+            prefix, user_id = prefixed_userid.split(':', 1)
             # Remove unprefixed user id to avoid conflicts.
             # (it is added via Pyramid Authn policy effective principals)
             if user_id in principals:
@@ -110,7 +111,7 @@ class RouteFactory(object):
 
     def __init__(self, request):
         # Make it available for the authorization policy.
-        self.prefixed_userid = getattr(request, "prefixed_userid", None)
+        self.get_prefixed_userid = functools.partial(prefixed_userid, request)
 
         self._check_permission = request.registry.permission.check_permission
 

cliquet/initialization.py

@@ -127,24 +127,14 @@ def setup_authentication(config):
 
     # Track policy used, for prefixing user_id and for logging.
     def on_policy_selected(event):
-        event.request.authn_type = event.policy_name.lower()
+        authn_type = event.policy_name.lower()
+        event.request.authn_type = authn_type
+        event.request.selected_userid = event.userid
+        # Add authentication info to context.
+        logger.bind(uid=event.userid, authn_type=authn_type)
 
     config.add_subscriber(on_policy_selected, MultiAuthPolicySelected)
 
-    # Build the prefixed_userid
-    def on_new_request(event):
-        authn_type = getattr(event.request, 'authn_type', None)
-
-        # Prefix the user id with the authn policy type name.
-        user_id = event.request.authenticated_userid
-
-        event.request.prefixed_userid = None
-        if user_id and authn_type:
-            event.request.prefixed_userid = '%s:%s' % (authn_type.lower(),
-                                                       user_id)
-
-    config.add_subscriber(on_new_request, NewRequest)
-
 
 def setup_backoff(config):
     """Attach HTTP requests/responses objects.
@@ -293,7 +283,7 @@ def on_new_response(event):
             request = event.request
 
             # Count unique users.
-            user_id = request.authenticated_userid
+            user_id = request.prefixed_userid
             if user_id:
                 client.count('users', unique=user_id)
 
@@ -367,9 +357,9 @@ def on_new_request(event):
                    path=event.request.path,
                    method=request.method,
                    querystring=dict(request.GET),
-                   uid=request.authenticated_userid,
                    lang=request.headers.get('Accept-Language'),
-                   authn_type=getattr(request, 'authn_type', None),
+                   uid=None,
+                   authn_type=None,
                    errno=None)
 
     config.add_subscriber(on_new_request, NewRequest)

cliquet/resource/__init__.py

@@ -177,7 +177,7 @@ def get_parent_id(self, request):
         :rtype: str
 
         """
-        return getattr(request, 'prefixed_userid', None)
+        return request.prefixed_userid
 
     def _get_known_fields(self):
         """Return all the `field` defined in the ressource mapping."""

cliquet/tests/support.py

@@ -170,7 +170,7 @@ def permits(self, context, principals, permission):
         if Everyone in principals:
             return True
         # Cliquet default authz policy uses prefixed_userid.
-        prefixed = [getattr(context, 'prefixed_userid', None)]
+        prefixed = [context.prefixed_userid]
         return USER_PRINCIPAL in (principals + prefixed)
 
     def principals_allowed_by_permission(self, context, permission):

cliquet/tests/test_authentication.py

@@ -1,4 +1,5 @@
 import mock
+import uuid
 
 from cliquet import authentication
 from cliquet import utils
@@ -62,6 +63,16 @@ def test_user_principals_are_cached_per_user(self):
         self.app.post_json('/batch', batch)
         self.assertEqual(mocked.call_count, 3)
 
+    def test_basicauth_hash_is_computed_only_once(self):
+        # hmac_digest() is used in Basic Authentication only.
+        patch = mock.patch('cliquet.utils.hmac_digest')
+        self.addCleanup(patch.stop)
+        mocked = patch.start()
+        body = {"data": {"name": "haha"}}
+        record_url = self.get_item_url(uuid.uuid4())
+        self.app.put_json(record_url, body, headers=self.headers)
+        self.assertEqual(mocked.call_count, 1)
+
 
 class BasicAuthenticationPolicyTest(unittest.TestCase):
     def setUp(self):

cliquet/tests/test_authorization.py

@@ -3,6 +3,7 @@
 from pyramid.request import Request
 
 from .support import DummyRequest, unittest
+from cliquet import authentication
 from cliquet.authorization import RouteFactory, AuthorizationPolicy
 from cliquet.storage import exceptions as storage_exceptions
 
@@ -86,6 +87,7 @@ def test_attributes_are_none_with_blank_requests(self):
         request = Request.blank(path='/')
         request.registry = mock.Mock(settings={})
         request.authn_type = 'fxa'
+        request.prefixed_userid = property(authentication.prefixed_userid)
         context = RouteFactory(request)
         self.assertIsNone(context.required_permission)
         self.assertIsNone(context.current_record)
@@ -95,12 +97,12 @@ def test_attributes_are_none_with_blank_requests(self):
     def test_attributes_are_none_with_non_resource_requests(self):
         basic_service = object()
         request = Request.blank(path='/')
+        request.prefixed_userid = property(authentication.prefixed_userid)
         request.matched_route = mock.Mock(pattern='foo')
         request.registry = mock.Mock(cornice_services={'foo': basic_service})
         request.registry.settings = {}
 
         context = RouteFactory(request)
-        self.assertIsNone(context.prefixed_userid)
         self.assertIsNone(context.current_record)
         self.assertIsNone(context.required_permission)
         self.assertIsNone(context.resource_name)
@@ -131,7 +133,7 @@ def setUp(self):
         self.authz = AuthorizationPolicy()
         self.authz.get_bound_permissions = mock.sentinel.get_bound_perms
         self.context = mock.MagicMock()
-        self.context.prefixed_userid = None
+        self.context.get_prefixed_userid.return_value = None
         self.context.allowed_principals = []
         self.context.object_id = mock.sentinel.object_id
         self.context.required_permission = 'read'
@@ -191,15 +193,15 @@ def test_permits_takes_route_factory_allowed_principals_into_account(self):
         self.assertTrue(has_permission)
 
     def test_prefixed_userid_is_added_to_principals(self):
-        self.context.prefixed_userid = 'fxa:userid'
+        self.context.get_prefixed_userid.return_value = 'fxa:userid'
         self.authz.permits(self.context, self.principals, 'foobar')
         self.context.check_permission.assert_called_with(
             'foobar',
             self.principals + ['fxa:userid', 'fxa_userid'],
             get_bound_permissions=mock.sentinel.get_bound_perms)
 
     def test_unprefixed_userid_is_removed_from_principals(self):
-        self.context.prefixed_userid = 'fxa:userid'
+        self.context.get_prefixed_userid.return_value = 'fxa:userid'
         self.authz.permits(self.context, ['userid'], 'foobar')
         self.context.check_permission.assert_called_with(
             'foobar',
@@ -222,7 +224,7 @@ def test_permits_returns_true_if_collection_and_shared_records(self):
         allowed = self.authz.permits(self.context, ['userid'], 'dynamic')
         self.context.fetch_shared_records.assert_called_with(
             'read',
-            ['userid', 'basicauth:bob', 'basicauth_bob'],
+            ['userid'],
             get_bound_permissions=mock.sentinel.get_bound_perms)
         self.assertTrue(allowed)
 
@@ -243,6 +245,6 @@ def test_permits_returns_false_if_collection_is_unknown(self):
         allowed = self.authz.permits(self.context, ['userid'], 'dynamic')
         self.context.fetch_shared_records.assert_called_with(
             'read',
-            ['userid', 'basicauth:bob', 'basicauth_bob'],
+            ['userid'],
             get_bound_permissions=mock.sentinel.get_bound_perms)
         self.assertFalse(allowed)

cliquet/tests/test_initialization.py

@@ -338,15 +338,15 @@ def test_statsd_counts_unique_users(self, mocked, digest_mocked):
         cliquet.initialize(self.config, '0.0.1', 'project_name')
         app = webtest.TestApp(self.config.make_wsgi_app())
         headers = {'Authorization': 'Basic bWF0Og=='}
-        app.get('/v0/__heartbeat__', headers=headers)
-        mocked().count.assert_any_call('users', unique='mat')
+        app.get('/v0/', headers=headers)
+        mocked().count.assert_any_call('users', unique='basicauth:mat')
 
     @mock.patch('cliquet.statsd.Client')
     def test_statsd_counts_authentication_types(self, mocked):
         cliquet.initialize(self.config, '0.0.1', 'project_name')
         app = webtest.TestApp(self.config.make_wsgi_app())
         headers = {'Authorization': 'Basic bWF0Og=='}
-        app.get('/v0/__heartbeat__', headers=headers)
+        app.get('/v0/', headers=headers)
         mocked().count.assert_any_call('authn_type.basicauth')
 
 

cliquet/tests/test_views_hello.py

@@ -61,7 +61,7 @@ def test_if_user_authenticated_userid_is_provided(self):
         response = self.app.get('/', headers=self.headers)
         userid = response.json['user']['id']
         self.assertTrue(userid.startswith('basicauth:'),
-                        '"%s" does not starts with "basicauth:"' % userid)
+                        '"%s" does not start with "basicauth:"' % userid)
 
     def test_return_http_api_version_when_set(self):
         with mock.patch.dict(

cliquet/views/errors.py

@@ -1,7 +1,7 @@
 from functools import wraps
 
 from pyramid import httpexceptions
-from pyramid.security import forget, NO_PERMISSION_REQUIRED
+from pyramid.security import forget, NO_PERMISSION_REQUIRED, Authenticated
 from pyramid.view import (
     forbidden_view_config, notfound_view_config, view_config
 )
@@ -32,7 +32,7 @@ def authorization_required(request):
     """Distinguish authentication required (``401 Unauthorized``) from
     not allowed (``403 Forbidden``).
     """
-    if not request.authenticated_userid:
+    if Authenticated not in request.effective_principals:
         error_msg = "Please authenticate yourself to use this endpoint."
         response = http_error(httpexceptions.HTTPUnauthorized(),
                               errno=ERRORS.MISSING_AUTH_TOKEN,

cliquet/views/hello.py

@@ -1,4 +1,4 @@
-from pyramid.security import NO_PERMISSION_REQUIRED
+from pyramid.security import NO_PERMISSION_REQUIRED, Authenticated
 
 from cliquet import Service, PROTOCOL_VERSION
 
@@ -39,8 +39,9 @@ def get_hello(request):
             value = settings[setting]
         data['settings'][setting] = value
 
-    prefixed_userid = getattr(request, 'prefixed_userid', None)
-    if prefixed_userid:
+    # If current user is authenticated, add user info:
+    # (Note: this will call authenticated_userid() with multiauth+groupfinder)
+    if Authenticated in request.effective_principals:
         data['user'] = request.get_user_info()
 
     # Application can register and expose arbitrary capabilities.

setup.py

@@ -21,7 +21,7 @@
     'colander',
     'cornice >= 1.1',  # Fix cache CORS
     'python-dateutil',
-    'pyramid_multiauth >= 0.7',  # Contained policy names.
+    'pyramid_multiauth >= 0.8',  # User on policy selected event.
     'pyramid_tm',
     'redis',  # Default backend
     'requests',