Merge pull request #3465 from luhn/security-policy

Security policy implementation

2 files changed, 222 insertions, 278 deletions

diff --git a/docs/narr/advanced-features.rst b/docs/narr/advanced-features.rst
index b24208bc4..8d99f7291 100644
--- a/docs/narr/advanced-features.rst
+++ b/docs/narr/advanced-features.rst
@@ -104,13 +104,14 @@ For example, if you want to reuse an existing application that already has a bun
 Authenticate Users Your Way
 ---------------------------
 
-:app:`Pyramid` ships with prebuilt, well-tested authentication and authorization schemes out of the box. Using a scheme is a matter of configuration. So if you need to change approaches later, you need only update your configuration.
-
-In addition, the system that handles authentication and authorization is flexible and pluggable. If you want to use another security add-on, or define your own, you can. And again, you need only update your application configuration to make the change.
+:app:`Pyramid` has a powerful security system that can be tailored to your
+needs.  Build your own security policy tailored to your needs, or use one of
+the many helpers provided to easily implement common authentication and
+authorization patterns.
 
 .. seealso::
 
-   See also :ref:`enabling_authorization_policy`.
+   See also :ref:`writing_security_policy`.
 
 Build Trees of Resources
 ------------------------
diff --git a/docs/narr/security.rst b/docs/narr/security.rst
index 2b0a2f032..2a7034a19 100644
--- a/docs/narr/security.rst
+++ b/docs/narr/security.rst
@@ -6,17 +6,12 @@
 Security
 ========
 
-:app:`Pyramid` provides an optional, declarative, security system. Security in
-:app:`Pyramid` is separated into authentication and authorization. The two
-systems communicate via :term:`principal` identifiers. Authentication is merely
-the mechanism by which credentials provided in the :term:`request` are resolved
-to one or more :term:`principal` identifiers. These identifiers represent the
-users and groups that are in effect during the request. Authorization then
-determines access based on the :term:`principal` identifiers, the requested
-:term:`permission`, and a :term:`context`.
-
-The :app:`Pyramid` authorization system can prevent a :term:`view` from being
-invoked based on an :term:`authorization policy`. Before a view is invoked, the
+:app:`Pyramid` provides an optional, declarative security system.  The system
+determines the identity of the current user (authentication) and whether or not
+the user has access to certain resources (authorization).
+
+The :app:`Pyramid` security system can prevent a :term:`view` from being
+invoked based on the :term:`security policy`. Before a view is invoked, the
 authorization system can use the credentials in the :term:`request` along with
 the :term:`context` resource to determine if access will be allowed.  Here's
 how it works at a high level:
@@ -37,89 +32,144 @@ how it works at a high level:
 - A :term:`view callable` is located by :term:`view lookup` using the context
   as well as other attributes of the request.
 
-- If an :term:`authentication policy` is in effect, it is passed the request.
-  It will return some number of :term:`principal` identifiers. To do this, the
-  policy would need to determine the authenticated :term:`userid` present in
-  the request.
+- If a :term:`security policy` is in effect, it is passed the request and
+  returns the :term:`identity` of the current user.
 
-- If an :term:`authorization policy` is in effect and the :term:`view
+- If a :term:`security policy` is in effect and the :term:`view
   configuration` associated with the view callable that was found has a
-  :term:`permission` associated with it, the authorization policy is passed the
-  :term:`context`, some number of :term:`principal` identifiers returned by the
-  authentication policy, and the :term:`permission` associated with the view;
-  it will allow or deny access.
+  :term:`permission` associated with it, the policy is passed the
+  :term:`context`, the current :term:`identity`, and the :term:`permission`
+  associated with the view; it will allow or deny access.
 
-- If the authorization policy allows access, the view callable is invoked.
+- If the security policy allows access, the view callable is invoked.
 
-- If the authorization policy denies access, the view callable is not invoked.
+- If the security policy denies access, the view callable is not invoked.
   Instead the :term:`forbidden view` is invoked.
 
-Authorization is enabled by modifying your application to include an
-:term:`authentication policy` and :term:`authorization policy`. :app:`Pyramid`
-comes with a variety of implementations of these policies.  To provide maximal
-flexibility, :app:`Pyramid` also allows you to create custom authentication
-policies and authorization policies.
+The security system is enabled by modifying your application to include a
+:term:`security policy`. :app:`Pyramid` comes with a variety of helpers to
+assist in the creation of this policy.
 
 .. index::
-   single: authorization policy
-
-.. _enabling_authorization_policy:
+   single: security policy
 
-Enabling an Authorization Policy
---------------------------------
+.. _writing_security_policy:
 
-:app:`Pyramid` does not enable any authorization policy by default.  All views
-are accessible by completely anonymous users.  In order to begin protecting
-views from execution based on security settings, you need to enable an
-authorization policy.
+Writing a Security Policy
+-------------------------
 
-Enabling an Authorization Policy Imperatively
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+:app:`Pyramid` does not enable any security policy by default.  All views are
+accessible by completely anonymous users.  In order to begin protecting views
+from execution based on security settings, you need to write a security policy.
 
-Use the :meth:`~pyramid.config.Configurator.set_authorization_policy` method of
-the :class:`~pyramid.config.Configurator` to enable an authorization policy.
+Security policies are simple classes implementing a
+:class:`pyramid.interfaces.ISecurityPolicy`, defined as follows:
 
-You must also enable an :term:`authentication policy` in order to enable the
-authorization policy.  This is because authorization, in general, depends upon
-authentication.  Use the
-:meth:`~pyramid.config.Configurator.set_authentication_policy` method during
-application setup to specify the authentication policy.
+.. autointerface:: pyramid.interfaces.ISecurityPolicy
+  :members:
 
-For example:
+A simple security policy might look like the following:
 
 .. code-block:: python
     :linenos:
 
-    from pyramid.config import Configurator
-    from pyramid.authentication import AuthTktAuthenticationPolicy
-    from pyramid.authorization import ACLAuthorizationPolicy
-    authn_policy = AuthTktAuthenticationPolicy('seekrit', hashalg='sha512')
-    authz_policy = ACLAuthorizationPolicy()
-    config = Configurator()
-    config.set_authentication_policy(authn_policy)
-    config.set_authorization_policy(authz_policy)
+    from pyramid.security import Allowed, Denied
 
-.. note:: The ``authentication_policy`` and ``authorization_policy`` arguments
-   may also be passed to their respective methods mentioned above as
-   :term:`dotted Python name` values, each representing the dotted name path to
-   a suitable implementation global defined at Python module scope.
+    class SessionSecurityPolicy:
+        def identify(self, request):
+            """ Return the user ID stored in the session. """
+            return request.session.get('userid')
 
-The above configuration enables a policy which compares the value of an "auth
-ticket" cookie passed in the request's environment which contains a reference
-to a single :term:`userid`, and matches that userid's :term:`principals
-<principal>` against the principals present in any :term:`ACL` found in the
-resource tree when attempting to call some :term:`view`.
+        def permits(self, request, context, identity, permission):
+            """ Allow access to everything if signed in. """
+            if identity is not None:
+                return Allowed('User is signed in.')
+            else:
+                return Denied('User is not signed in.')
 
-While it is possible to mix and match different authentication and
-authorization policies, it is an error to configure a Pyramid application with
-an authentication policy but without the authorization policy or vice versa. If
-you do this, you'll receive an error at application startup time.
+        def remember(request, userid, **kw):
+            request.session.get('userid')
+            return []
+
+        def forget(request):
+            del request.session['userid']
+            return []
+
+Use the :meth:`~pyramid.config.Configurator.set_security_policy` method of
+the :class:`~pyramid.config.Configurator` to enforce the security policy on
+your application.
 
 .. seealso::
 
-    See also the :mod:`pyramid.authorization` and :mod:`pyramid.authentication`
-    modules for alternative implementations of authorization and authentication
-    policies.
+    For more information on implementing the ``permits`` method, see
+    :ref:`security_policy_permits`.
+
+Writing a Security Policy Using Helpers
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+To assist in writing common security policies, Pyramid provides several
+helpers.  The following authentication helpers assist with implementing
+``identity``, ``remember``, and ``forget``.
+
++-------------------------------+-------------------------------------------------------------------+
+| Use Case                      | Helper                                                            |
++===============================+===================================================================+
+| Store the :term:`userid`      | :class:`pyramid.authentication.SessionAuthenticationHelper`       |
+| in the :term:`session`.       |                                                                   |
++-------------------------------+-------------------------------------------------------------------+
+| Store the :term:`userid`      | :class:`pyramid.authentication.AuthTktCookieHelper`               |
+| with an "auth ticket" cookie. |                                                                   |
++-------------------------------+-------------------------------------------------------------------+
+| Retrieve user credentials     | Use :func:`pyramid.authentication.extract_http_basic_credentials` |
+| using HTTP Basic Auth.        | to retrieve credentials.                                          |
++-------------------------------+-------------------------------------------------------------------+
+| Retrieve the :term:`userid`   | ``REMOTE_USER`` can be accessed with                              |
+| from ``REMOTE_USER`` in the   | ``request.environ.get('REMOTE_USER')``.                           |
+| WSGI environment.             |                                                                   |
++-------------------------------+-------------------------------------------------------------------+
+
+For example, our above security policy can leverage these helpers like so:
+
+.. code-block:: python
+    :linenos:
+
+    from pyramid.security import Allowed, Denied
+    from pyramid.authentication import SessionAuthenticationHelper
+
+    class SessionSecurityPolicy:
+        def __init__(self):
+            self.helper = SessionAuthenticationHelper()
+
+        def identify(self, request):
+            """ Return the user ID stored in the session. """
+            return self.helper.identify(request)
+
+        def permits(self, request, context, identity, permission):
+            """ Allow access to everything if signed in. """
+            if identity is not None:
+                return Allowed('User is signed in.')
+            else:
+                return Denied('User is not signed in.')
+
+        def remember(request, userid, **kw):
+            return self.helper.remember(request, userid, **kw)
+
+        def forget(request):
+            return self.helper.forget(request)
+
+Helpers are intended to be used with application-specific code, so perhaps your
+authentication also queries the database to ensure the identity is valid.
+
+.. code-block:: python
+    :linenos:
+
+        def identify(self, request):
+            """ Return the user ID stored in the session. """
+            user_id = self.helper.identify(request)
+            if validate_user_id(user_id):
+                return user_id
+            else:
+                return None
 
 .. index::
    single: permissions
@@ -165,11 +215,53 @@ performed via the ``@view_config`` decorator:
         pass
 
 As a result of any of these various view configuration statements, if an
-authorization policy is in place when the view callable is found during normal
-application operations, the requesting user will need to possess the ``add``
-permission against the :term:`context` resource in order to be able to invoke
-the ``blog_entry_add_view`` view.  If they do not, the :term:`Forbidden view`
-will be invoked.
+security policy is in place when the view callable is found during normal
+application operations, the security policy will be queried to see if the
+requesting user is allowed the ``add`` permission within the current
+:term:`context`.  If the policy allows access, ``blog_entry_add_view`` will be
+invoked.  If not, the :term:`Forbidden view` will be invoked.
+
+.. _security_policy_permits:
+
+Allowing and Denying Access With a Security Policy
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+To determine whether access is allowed to a view with an attached permission,
+Pyramid calls the ``permits`` method of the security policy.  ``permits``
+should return an instance of :class:`pyramid.security.Allowed` or
+:class:`pyramid.security.Denied`.  Both classes accept a string as an argument,
+which should detail why access was allowed or denied.
+
+A simple ``permits`` implementation that grants access based on a user role
+might look like so:
+
+.. code-block:: python
+    :linenos:
+
+    from pyramid.security import Allowed, Denied
+
+    class SecurityPolicy:
+        def permits(self, request, context, identity, permission):
+            if identity is None:
+                return Denied('User is not signed in.')
+            if identity.role == 'admin':
+                allowed = ['read', 'write', 'delete']
+            elif identity.role == 'editor':
+                allowed = ['read', 'write']
+            else:
+                allowed = ['read']
+            if permission in allowed:
+                return Allowed(
+                    'Access granted for user %s with role %s.',
+                    identity,
+                    identity.role,
+                )
+            else:
+                return Denied(
+                    'Access denied for user %s with role %s.',
+                    identity,
+                    identity.role,
+                )
 
 .. index::
    pair: permission; default
@@ -180,7 +272,7 @@ Setting a Default Permission
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 If a permission is not supplied to a view configuration, the registered view
-will always be executable by entirely anonymous users: any authorization policy
+will always be executable by entirely anonymous users: any security policy
 in effect is ignored.
 
 In support of making it easier to configure applications which are "secure by
@@ -217,16 +309,39 @@ When a default permission is registered:
 
 .. _assigning_acls:
 
-Assigning ACLs to Your Resource Objects
----------------------------------------
+Implementing ACL Authorization
+------------------------------
+
+A common way to implement authorization is using an :term:`ACL`.  An ACL is a
+:term:`context`-specific list of access control entries, which allow or deny
+access to permissions based on a user's principals.
+
+Pyramid provides :class:`pyramid.authorization.ACLHelper` to assist with an
+ACL-based implementation of ``permits``.  Application-specific code should
+construct a list of principals for the user and call
+:meth:`pyramid.authorization.ACLHelper.permits`, which will return an
+:class:`pyramid.security.ACLAllowed` or :class:`pyramid.security.ACLDenied`
+object.  An implementation might look like this:
+
+.. code-block:: python
+    :linenos:
+
+    from pyramid.security import Everyone, Authenticated
+    from pyramid.authorization import ACLHelper
+
+    class SecurityPolicy:
+        def permits(self, request, context, identity, permission):
+            principals = [Everyone]
+            if identity is not None:
+                principals.append(Authenticated)
+                principals.append('user:' + identity.id)
+                principals.append('group:' + identity.group)
+            return ACLHelper().permits(context, principals, permission)
 
-When the default :app:`Pyramid` :term:`authorization policy` determines whether
-a user possesses a particular permission with respect to a resource, it
-examines the :term:`ACL` associated with the resource.  An ACL is associated
-with a resource by adding an ``__acl__`` attribute to the resource object.
-This attribute can be defined on the resource *instance* if you need
-instance-level security, or it can be defined on the resource *class* if you
-just need type-level security.
+To associate an ACL with a resource, add an ``__acl__`` attribute to the
+resource object.  This attribute can be defined on the resource *instance* if
+you need instance-level security, or it can be defined on the resource *class*
+if you just need type-level security.
 
 For example, an ACL might be attached to the resource for a blog via its class:
 
@@ -334,11 +449,9 @@ matches.  The second element is a :term:`principal`.  The third argument is a
 permission or sequence of permission names.
 
 A principal is usually a user id, however it also may be a group id if your
-authentication system provides group information and the effective
-:term:`authentication policy` policy is written to respect group information.
-See :ref:`extending_default_authentication_policies`.
+authentication system provides group information.
 
-Each ACE in an ACL is processed by an authorization policy *in the order
+Each ACE in an ACL is processed by the ACL helper *in the order
 dictated by the ACL*.  So if you have an ACL like this:
 
 .. code-block:: python
@@ -353,9 +466,9 @@ dictated by the ACL*.  So if you have an ACL like this:
         (Deny, Everyone, 'view'),
         ]
 
-The default authorization policy will *allow* everyone the view permission,
-even though later in the ACL you have an ACE that denies everyone the view
-permission.  On the other hand, if you have an ACL like this:
+The ACL helper will *allow* everyone the view permission, even though later in
+the ACL you have an ACE that denies everyone the view permission.  On the other
+hand, if you have an ACL like this:
 
 .. code-block:: python
     :linenos:
@@ -369,7 +482,7 @@ permission.  On the other hand, if you have an ACL like this:
         (Allow, Everyone, 'view'),
         ]
 
-The authorization policy will deny everyone the view permission, even though
+The ACL helper will deny everyone the view permission, even though
 later in the ACL, there is an ACE that allows everyone.
 
 The third argument in an ACE can also be a sequence of permission names instead
@@ -388,6 +501,7 @@ can collapse this into a single ACE, as below.
         (Allow, 'group:editors', ('add', 'edit')),
         ]
 
+.. _special_principals:
 
 .. index::
    single: principal
@@ -445,8 +559,7 @@ permissions in :data:`pyramid.security.DENY_ALL`.  This ACE is often used as
 the *last* ACE of an ACL to explicitly cause inheriting authorization policies
 to "stop looking up the traversal tree" (effectively breaking any inheritance).
 For example, an ACL which allows *only* ``fred`` the view permission for a
-particular resource, despite what inherited ACLs may say when the default
-authorization policy is in effect, might look like so:
+particular resource, despite what inherited ACLs may say, might look like so:
 
 .. code-block:: python
     :linenos:
@@ -472,11 +585,10 @@ following:
 ACL Inheritance and Location-Awareness
 --------------------------------------
 
-While the default :term:`authorization policy` is in place, if a resource
-object does not have an ACL when it is the context, its *parent* is consulted
-for an ACL.  If that object does not have an ACL, *its* parent is consulted for
-an ACL, ad infinitum, until we've reached the root and there are no more
-parents left.
+While the ACL helper is in place, if a resource object does not have an ACL
+when it is the context, its *parent* is consulted for an ACL.  If that object
+does not have an ACL, *its* parent is consulted for an ACL, ad infinitum, until
+we've reached the root and there are no more parents left.
 
 In order to allow the security machinery to perform ACL inheritance, resource
 objects must provide *location-awareness*.  Providing *location-awareness*
@@ -567,184 +679,16 @@ denied or allowed.  Introspecting this information in the debugger or via print
 statements when a call to :meth:`~pyramid.request.Request.has_permission` fails
 is often useful.
 
-.. index::
-   single: authentication policy (extending)
-
-.. _extending_default_authentication_policies:
-
-Extending Default Authentication Policies
------------------------------------------
-
-Pyramid ships with some built in authentication policies for use in your
-applications. See :mod:`pyramid.authentication` for the available policies.
-They differ on their mechanisms for tracking authentication credentials between
-requests, however they all interface with your application in mostly the same
-way.
-
-Above you learned about :ref:`assigning_acls`. Each :term:`principal` used in
-the :term:`ACL` is matched against the list returned from
-:meth:`pyramid.interfaces.IAuthenticationPolicy.effective_principals`.
-Similarly, :meth:`pyramid.request.Request.authenticated_userid` maps to
-:meth:`pyramid.interfaces.IAuthenticationPolicy.authenticated_userid`.
-
-You may control these values by subclassing the default authentication
-policies. For example, below we subclass the
-:class:`pyramid.authentication.AuthTktAuthenticationPolicy` and define extra
-functionality to query our database before confirming that the :term:`userid`
-is valid in order to avoid blindly trusting the value in the cookie (what if
-the cookie is still valid, but the user has deleted their account?).  We then
-use that :term:`userid` to augment the ``effective_principals`` with
-information about groups and other state for that user.
-
-.. code-block:: python
-    :linenos:
-
-    from pyramid.authentication import AuthTktAuthenticationPolicy
-
-    class MyAuthenticationPolicy(AuthTktAuthenticationPolicy):
-        def authenticated_userid(self, request):
-            userid = self.unauthenticated_userid(request)
-            if userid:
-                if request.verify_userid_is_still_valid(userid):
-                    return userid
-
-        def effective_principals(self, request):
-            principals = [Everyone]
-            userid = self.authenticated_userid(request)
-            if userid:
-                principals += [Authenticated, str(userid)]
-            return principals
-
-In most instances ``authenticated_userid`` and ``effective_principals`` are
-application-specific, whereas ``unauthenticated_userid``, ``remember``, and
-``forget`` are generic and focused on transport and serialization of data
-between consecutive requests.
-
-.. index::
-   single: authentication policy (creating)
-
-.. _creating_an_authentication_policy:
-
-Creating Your Own Authentication Policy
----------------------------------------
-
-:app:`Pyramid` ships with a number of useful out-of-the-box security policies
-(see :mod:`pyramid.authentication`).  However, creating your own authentication
-policy is often necessary when you want to control the "horizontal and
-vertical" of how your users authenticate.  Doing so is a matter of creating an
-instance of something that implements the following interface:
-
-.. code-block:: python
-    :linenos:
-
-    class IAuthenticationPolicy(object):
-        """ An object representing a Pyramid authentication policy. """
-
-        def authenticated_userid(self, request):
-            """ Return the authenticated :term:`userid` or ``None`` if
-            no authenticated userid can be found. This method of the
-            policy should ensure that a record exists in whatever
-            persistent store is used related to the user (the user
-            should not have been deleted); if a record associated with
-            the current id does not exist in a persistent store, it
-            should return ``None``.
-            """
-
-        def unauthenticated_userid(self, request):
-            """ Return the *unauthenticated* userid.  This method
-            performs the same duty as ``authenticated_userid`` but is
-            permitted to return the userid based only on data present
-            in the request; it needn't (and shouldn't) check any
-            persistent store to ensure that the user record related to
-            the request userid exists.
-
-            This method is intended primarily a helper to assist the
-            ``authenticated_userid`` method in pulling credentials out
-            of the request data, abstracting away the specific headers,
-            query strings, etc that are used to authenticate the request.
-            """
-
-        def effective_principals(self, request):
-            """ Return a sequence representing the effective principals
-            typically including the :term:`userid` and any groups belonged
-            to by the current user, always including 'system' groups such
-            as ``pyramid.security.Everyone`` and
-            ``pyramid.security.Authenticated``.
-            """
-
-        def remember(self, request, userid, **kw):
-            """ Return a set of headers suitable for 'remembering' the
-            :term:`userid` named ``userid`` when set in a response.  An
-            individual authentication policy and its consumers can
-            decide on the composition and meaning of **kw.
-            """
-
-        def forget(self, request):
-            """ Return a set of headers suitable for 'forgetting' the
-            current user on subsequent requests.
-            """
-
-After you do so, you can pass an instance of such a class into the
-:class:`~pyramid.config.Configurator.set_authentication_policy` method at
-configuration time to use it.
-
-.. index::
-   single: authorization policy (creating)
-
-.. _creating_an_authorization_policy:
-
-Creating Your Own Authorization Policy
---------------------------------------
-
-An authorization policy is a policy that allows or denies access after a user
-has been authenticated.  Most :app:`Pyramid` applications will use the default
-:class:`pyramid.authorization.ACLAuthorizationPolicy`.
-
-However, in some cases, it's useful to be able to use a different authorization
-policy than the default :class:`~pyramid.authorization.ACLAuthorizationPolicy`.
-For example, it might be desirable to construct an alternate authorization
-policy which allows the application to use an authorization mechanism that does
-not involve :term:`ACL` objects.
-
-:app:`Pyramid` ships with only a single default authorization policy, so you'll
-need to create your own if you'd like to use a different one.  Creating and
-using your own authorization policy is a matter of creating an instance of an
-object that implements the following interface:
-
-.. code-block:: python
-    :linenos:
-
-    class IAuthorizationPolicy(Interface):
-        """ An object representing a Pyramid authorization policy. """
-        def permits(context, principals, permission):
-            """ Return an instance of :class:`pyramid.security.Allowed` if any
-            of the ``principals`` is allowed the ``permission`` in the current
-            ``context``, else return an instance of
-            :class:`pyramid.security.Denied`.
-            """
-
-        def principals_allowed_by_permission(context, permission):
-            """ Return a set of principal identifiers allowed by the
-            ``permission`` in ``context``.  This behavior is optional; if you
-            choose to not implement it you should define this method as
-            something which raises a ``NotImplementedError``.  This method
-            will only be called when the
-            ``pyramid.security.principals_allowed_by_permission`` API is
-            used."""
-
-After you do so, you can pass an instance of such a class into the
-:class:`~pyramid.config.Configurator.set_authorization_policy` method at
-configuration time to use it.
-
 .. _admonishment_against_secret_sharing:
 
 Admonishment Against Secret-Sharing
 -----------------------------------
 
 A "secret" is required by various components of Pyramid.  For example, the
-:term:`authentication policy` below uses a secret value ``seekrit``::
+helper below might be used for a security policy and uses a secret value
+``seekrit``::
 
-  authn_policy = AuthTktAuthenticationPolicy('seekrit', hashalg='sha512')
+  helper = AuthTktCookieHelper('seekrit', hashalg='sha512')
 
 A :term:`session factory` also requires a secret::
 
@@ -752,9 +696,8 @@ A :term:`session factory` also requires a secret::
 
 It is tempting to use the same secret for multiple Pyramid subsystems.  For
 example, you might be tempted to use the value ``seekrit`` as the secret for
-both the authentication policy and the session factory defined above.  This is
-a bad idea, because in both cases, these secrets are used to sign the payload
-of the data.
+both the helper and the session factory defined above.  This is a bad idea,
+because in both cases, these secrets are used to sign the payload of the data.
 
 If you use the same secret for two different parts of your application for
 signing purposes, it may allow an attacker to get his chosen plaintext signed,