IForbiddenAppFactory -> IForbiddenResponseFactory.

7 files changed, 199 insertions, 123 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 090c0f412..6df0c8171 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -5,42 +5,44 @@ Features
 --------
 
 - It is now possible to register a custom
-  ``repoze.bfg.interfaces.IForbiddenAppFactory`` for a given
+  ``repoze.bfg.interfaces.IForbiddenResponseFactory`` for a given
   application.  This feature replaces the
   ``repoze.bfg.interfaces.IUnauthorizedAppFactory`` feature previously
-  described in the Hooks chapter.  The IForbiddenAppFactory will be
-  called when the framework detects an authorization failure; it
+  described in the Hooks chapter.  The IForbiddenResponseFactory will
+  be called when the framework detects an authorization failure; it
   should accept a context object and a request object; it should
-  return a WSGI application.  Read the below point for more info and
-  see the Hooks narrative chapter of the BFG docs for more info.
+  return an IResponse object (a webob response, basically).  Read the
+  below point for more info and see the Hooks narrative chapter of the
+  BFG docs for more info; this registration trumps the security policy
+  ``forbidden`` feature when it is registered.
 
 - It is now possible to register a security policy that returns a
-  customized ``Forbidden`` WSGI application when BFG cannot authorize
-  an invocation of a view.  To this end, ISecurityPolicy objects must
-  now have a ``forbidden`` method that accepts two arguments:
-  ``context`` and ``request``.  The ``context`` will be the context
-  found by the router, the ``request`` will be the current request.
-  This method should return a WSGI application.  The returned WSGI
-  application should generate a response which is appropriate when
-  access to a view resource was forbidden by the security policy
-  (e.g. perhaps a login page).  ``repoze.bfg`` is willing to operate
-  with a custom security policy that does not have a ``forbidden``
-  method, but it will issue a warning; eventually security policies
-  without a ``forbidden`` method will cease to work under
-  ``repoze.bfg``.
-
-  Note that the ``forbidden`` WSGI application returned by the
-  security policy is not used if a developer has registered an
-  IForbiddenAppFactory (see the "Hooks" narrative chapter); the
-  explicitly registered IForbiddenAppFactory will be preferred over
-  the (more general) security policy forbidden app factory.
+  customized ``Forbidden`` response when BFG cannot authorize an
+  invocation of a view.  To this end, ISecurityPolicy objects must now
+  have a ``forbidden`` method that accepts two arguments: ``context``
+  and ``request``.  The ``context`` will be the context found by the
+  router, the ``request`` will be the current request.  This method
+  should return an IResponse (a webob response).  The returned
+  response application should be appropriate when access to a view
+  resource was forbidden by the security policy (e.g. perhaps a login
+  page or a general "forbidden" page).  ``repoze.bfg`` is willing to
+  operate with a custom security policy that does not have a
+  ``forbidden`` method, but it will issue a warning; eventually
+  security policies without a ``forbidden`` method will cease to work
+  under ``repoze.bfg``.
+
+  Note that the ``forbidden`` method of a security policy is not used
+  if a developer has registered an IForbiddenResponseFactory (see the
+  "Hooks" narrative chapter); the explicitly registered
+  IForbiddenResponseFactory will be preferred over the (more general)
+  security policy forbidden method.
 
 - All default security policies now have a ``forbidden`` callable
-  attached to them.  This particular callable returns a WSGI
-  application which generates a ``401 Unauthorized`` response for
-  backwards compatibility (had backwards compatibility not been an
-  issue, this callable would have returned a WSGI app that generated a
-  ``403 Forbidden`` response).
+  attached to them.  This particular callable returns an IResponse (a
+  webob response) with a ``401 Unauthorized`` status for backwards
+  compatibility (had backwards compatibility not been an issue, this
+  callable would have returned a WSGI app that generated a ``403
+  Forbidden`` response).
 
 Backwards Incompatibilities
 ---------------------------
@@ -57,8 +59,7 @@ Deprecations
 
 - The ``repoze.bfg.interfaces.IUnauthorizedAppFactory`` interface has
   been deprecated in favor of using the new
-  ``repoze.bfg.interfaces.IForbiddenAppFactory`` mechanism.
-
+  ``repoze.bfg.interfaces.IForbiddenResponseFactory`` mechanism.
 
 0.8.1 (2009-05-21)
 ==================
diff --git a/docs/narr/hooks.rst b/docs/narr/hooks.rst
index aefa95046..6428408e8 100644
--- a/docs/narr/hooks.rst
+++ b/docs/narr/hooks.rst
@@ -124,40 +124,44 @@ sample code that implements a minimal NotFound application factory:
    ``debug_notfound`` environment setting is true than it is when it
    is false.
 
-Changing the Forbidden Application
-----------------------------------
+Changing the Forbidden Response
+-------------------------------
 
 When :mod:`repoze.bfg` can't authorize execution of a view based on
-the security policy in use, it creates and invokes a Forbidden WSGI
-application. The application it invokes can be customized by placing
-something like the following ZCML in your ``configure.zcml`` file.
+the security policy in use, it invokes a "forbidden response factory".
+Usually this forbidden response factory is serviced by the currently
+active :term:`security policy`, but it can be overridden as necessary
+by placing something like the following ZCML in your
+``configure.zcml`` file.
 
 .. code-block:: xml
    :linenos:
 
-   <utility provides="repoze.bfg.interfaces.IForbiddenAppFactory"
-            component="helloworld.factories.forbidden_app_factory"/>
+   <utility provides="repoze.bfg.interfaces.IForbiddenResponseFactory"
+            component="helloworld.factories.forbidden_response_factory"/>
 
 Replace ``helloworld.factories.forbidden_app_factory`` with the Python
-dotted name to the WSGI application factory you want to use.  Here's
-some sample code that implements a minimal Unauthorized application
-factory:
+dotted name to the forbidden response factory you want to use.  The
+response factory must accept two parameters: ``context`` and
+``request``.  The ``context`` is the context found by the router when
+the view invocation was denied.  The ``request`` is the current
+:term:`request` representing the denied action.  Here's some sample
+code that implements a minimal forbidden response factory:
 
 .. code-block:: python
 
    from repoze.bfg.chameleon_zpt import render_template_to_response
 
-   def forbidden_app_factory(context, request):
+   def forbidden_response_factory(context, request):
        return render_template_to_response('templates/login_form.pt')
 
-.. note:: When an Forbidden application factory is invoked, it is
-   passed the WSGI environ and the WSGI ``start_response`` handler by
-   :mod:`repoze.bfg`.  Within the WSGI environ will be a key named
-   ``repoze.bfg.message`` that has a value explaining why the action
-   was forbidden.  This error will be different when the
-   ``debug_authorization`` environment setting is true than it is when
-   it is false.  A WebOb ``Response`` object is a valid WSGI
-   application, by the way.
+.. note:: When an forbidden response factory is invoked, it is passed
+   the request as the second argument.  An attribute of the request is
+   ``environ``, which is the WSGI environment.  Within the WSGI
+   environ will be a key named ``repoze.bfg.message`` that has a value
+   explaining why the current view invocation was forbidden.  This
+   error will be different when the ``debug_authorization``
+   environment setting is true than it is when it is false.
 
 .. warning:: the default forbidden application factory sends a
    response with a ``401 Unauthorized`` status code for backwards
diff --git a/repoze/bfg/interfaces.py b/repoze/bfg/interfaces.py
index 78db34ced..78311962f 100644
--- a/repoze/bfg/interfaces.py
+++ b/repoze/bfg/interfaces.py
@@ -132,14 +132,21 @@ class ISecurityPolicy(Interface):
         ``NotImplementedError`` exception."""
 
     def forbidden(context, request):
-        """ This method should return a WSGI application (a callable
-        accepting ``environ`` and ``start_response``).  This WSGI
-        application will be called by ``repoze.bfg`` when view
-        invocation is denied due to a security policy deny.  The WSGI
-        application should return a response appropriate when access
-        to a view resource was forbidden by the security policy.  Note
-        that the ``repoze.bfg.message`` key in the environ passed to
-        the WSGI app will contain the 'raw' reason that view
+        """ This method should return an IResponse object (an object
+        with the attributes ``status``, ``headerlist``, and
+        ``app_iter``) as a result of a view invocation denial.  The
+        ``forbidden`` method of a security policy will be called by
+        ``repoze.bfg`` when view invocation is denied (usually as a
+        result of the ``permit`` method of the same security policy
+        returning False to the Router).
+
+        The ``forbidden`` method of a security will not be called when
+        an ``IForbiddenResponseFactory`` utility is registered;
+        instead the ``IForbiddenResponseFactory`` utility will serve
+        the forbidden response.
+
+        Note that the ``repoze.bfg.message`` key in the environ passed
+        to the WSGI app will contain the 'raw' reason that view
         invocation was denied by repoze.bfg.  The ``context`` object
         passed in will be the context found by ``repoze.bfg`` when the
         denial was found and the ``request`` will be the request which
@@ -211,23 +218,25 @@ class INotFoundAppFactory(Interface):
         a``message`` key in the WSGI environ provides information
         pertaining to the reason for the notfound."""
 
-class IForbiddenAppFactory(Interface):
-    """ A utility which returns an Forbidden WSGI application
-    factory"""
+class IForbiddenResponseFactory(Interface):
+    """ A utility which returns an IResponse as the result of the
+    denial of a view invocation by a security policy."""
     def __call__(context, request):
-        """ Return a callable which returns an unauthorized WSGI
-        application.  When the WSGI application is invoked, a
-        ``message`` key in the WSGI environ provides information
-        pertaining to the reason for the unauthorized.  The
-        ``context`` passed to the forbidden app factory will be the
-        context found by the repoze.bfg router during traversal or url
-        dispatch.  The ``request`` will be the request object which
-        caused the deny. """
+        """ Return an object implementing IResponse (an object with
+        the status, headerlist, and app_iter attributes) as a result
+        of a view invocation denial by a security policy.
+        
+        Note that the ``message`` key in the WSGI environ
+        (request.environ) provides information pertaining to the
+        reason for the view invocation denial.  The ``context`` passed
+        to the forbidden app factory will be the context found by the
+        repoze.bfg router during traversal or url dispatch.  The
+        ``request`` will be the request object which caused the deny."""
 
 class IUnauthorizedAppFactory(Interface):
     """ A utility which returns an Unauthorized WSGI application
     factory (deprecated in repoze.bfg 0.8.2) in favor of
-    IForbiddenAppFactory """
+    IForbiddenResponseFactory """
     
 class IContextURL(Interface):
     """ An adapter which deals with URLs related to a context.
diff --git a/repoze/bfg/router.py b/repoze/bfg/router.py
index 2af5df4c7..fdd5495fe 100644
--- a/repoze/bfg/router.py
+++ b/repoze/bfg/router.py
@@ -17,7 +17,7 @@ from repoze.bfg.interfaces import IRouter
 from repoze.bfg.interfaces import IRoutesMapper
 from repoze.bfg.interfaces import ISecurityPolicy
 from repoze.bfg.interfaces import ISettings
-from repoze.bfg.interfaces import IForbiddenAppFactory
+from repoze.bfg.interfaces import IForbiddenResponseFactory
 from repoze.bfg.interfaces import IUnauthorizedAppFactory
 from repoze.bfg.interfaces import IView
 from repoze.bfg.interfaces import IViewPermission
@@ -66,18 +66,21 @@ class Router(object):
                 'Instead of registering a utility against the '
                 'repoze.bfg.interfaces.IUnauthorizedAppFactory interface '
                 'to return a custom forbidden response, you should now '
-                'register a "repoze.interfaces.IForbiddenAppFactory".  '
+                'register a "repoze.interfaces.IForbiddenResponseFactory".  '
                 'The IUnauthorizedAppFactory interface was deprecated in '
                 'repoze.bfg 0.8.2 and will be removed in a subsequent version '
                 'of repoze.bfg.  See the "Hooks" chapter of the repoze.bfg '
                 'documentation for more information about '
-                'IForbiddenAppFactory.')
+                'IForbiddenResponseFactory.')
             self.logger and self.logger.warn(warning)
             def forbidden(context, request):
-                return unauthorized_app_factory()
+                app = unauthorized_app_factory()
+                response = request.get_response(app)
+                return response
 
-        self.forbidden_app_factory = registry.queryUtility(IForbiddenAppFactory,
-                                                           default=forbidden)
+        self.forbidden_resp_factory = registry.queryUtility(
+            IForbiddenResponseFactory,
+            default=forbidden)
 
         if security_policy is not None:
             if hasattr(security_policy, 'forbidden'):
@@ -93,10 +96,10 @@ class Router(object):
                            'security policy without a "forbidden" method.' %
                            security_policy)
                 self.logger and self.logger.warn(warning)
-            # allow a specifically-registered IForbiddenAppFactory to
+            # allow a specifically-registered IForbiddenResponseFactory to
             # override the security policy's forbidden
-            self.forbidden_app_factory = (self.forbidden_app_factory or
-                                          security_policy_forbidden)
+            self.forbidden_resp_factory = (self.forbidden_resp_factory or
+                                           security_policy_forbidden)
 
         self.security_policy = security_policy
         self.notfound_app_factory = registry.queryUtility(INotFoundAppFactory,
@@ -193,7 +196,9 @@ class Router(object):
 
                 environ['repoze.bfg.message'] = msg
 
-                return self.forbidden_app_factory()(environ, start_response)
+                response = self.forbidden_resp_factory(context, request)
+                start_response(response.status, response.headerlist)
+                return response.app_iter
 
             response = registry.queryMultiAdapter(
                 (context, request), IView, name=view_name)
diff --git a/repoze/bfg/security.py b/repoze/bfg/security.py
index ba89a80e3..81ecf88ee 100644
--- a/repoze/bfg/security.py
+++ b/repoze/bfg/security.py
@@ -1,3 +1,6 @@
+from webob import Response
+from cgi import escape
+
 from zope.component import queryUtility
 from zope.deprecation import deprecated
 from zope.interface import implements
@@ -7,6 +10,7 @@ from repoze.bfg.location import lineage
 from repoze.bfg.interfaces import ISecurityPolicy
 from repoze.bfg.interfaces import IViewPermission
 from repoze.bfg.interfaces import IViewPermissionFactory
+from repoze.bfg.interfaces import IResponseFactory
 
 from repoze.bfg.wsgi import Unauthorized as UnauthorizedApp
 
@@ -78,7 +82,9 @@ def principals_allowed_by_permission(context, permission):
 
 class ACLSecurityPolicy(object):
     implements(ISecurityPolicy)
-    
+
+    forbidden_status = '401 Unauthorized' # b/c, should be 403
+
     def __init__(self, get_principals):
         self.get_principals = get_principals
 
@@ -148,7 +154,11 @@ class ACLSecurityPolicy(object):
         return []
 
     def forbidden(self, context, request):
-        return UnauthorizedApp()
+        body, headerlist = _forbidden_html(request, self.forbidden_status)
+        response_factory = queryUtility(IResponseFactory, default=Response)
+        return response_factory(status = self.forbidden_status,
+                                headerlist = headerlist,
+                                app_iter = body)
 
 class InheritingACLSecurityPolicy(object):
     """ A security policy which uses ACLs in the following ways:
@@ -194,7 +204,9 @@ class InheritingACLSecurityPolicy(object):
       ``authenticated_userid``).
     """
     implements(ISecurityPolicy)
-    
+
+    forbidden_status = '401 Unauthorized' # b/c, should be 403
+
     def __init__(self, get_principals):
         self.get_principals = get_principals
 
@@ -274,7 +286,11 @@ class InheritingACLSecurityPolicy(object):
         return allowed
 
     def forbidden(self, context, request):
-        return UnauthorizedApp()
+        body, headerlist = _forbidden_html(request, self.forbidden_status)
+        response_factory = queryUtility(IResponseFactory, default=Response)
+        return response_factory(status = self.forbidden_status,
+                                headerlist = headerlist,
+                                app_iter = body)
 
 def get_remoteuser(request):
     user_id = request.environ.get('REMOTE_USER')
@@ -491,6 +507,27 @@ def WhoInheritingACLSecurityPolicy():
     """
     return InheritingACLSecurityPolicy(get_who_principals)
 
+## class StandaloneInheritingACLSecurityPolicy(InheritingACLSecurityPolicy):
+##     def __init__(self, get_principals, login_view_name='login_view',
+##                  forbidden_view_name='forbidden_view'):
+##         self.get_principals = get_principals
+##         self.login_view_name = login_view_name
+##         self.forbidden_view_name = forbidden_view_name
+
+##     def forbidden(self, context, request):
+##         from repoze.bfg.view import render_view_to_response
+##         from webob import Response
+
+##         userid = self.authenticated_userid(request)
+
+##         if userid is None:
+##             view_name = self.login_view_name
+##         else:
+##             view_name = self.forbidden_view_name
+
+##         return render_view_to_response(context, request, name=view_name,
+##                                        secure=False)
+
 class PermitsResult(int):
     def __new__(cls, s, *args):
         inst = int.__new__(cls, cls.boolval)
@@ -603,3 +640,21 @@ class ViewPermissionFactory(object):
 class Unauthorized(Exception):
     pass
 
+def _forbidden_html(request, status):
+    try:
+        msg = escape(request.environ['repoze.bfg.message'])
+    except KeyError:
+        msg = ''
+    html = """
+    <html>
+    <title>%s</title>
+    <body>
+    <h1>%s</h1>
+    <code>%s</code>
+    </body>
+    </html>
+    """ % (status, status, msg)
+    headers = [('Content-Length', str(len(html))),
+               ('Content-Type', 'text/html')]
+    return [html], headers
+
diff --git a/repoze/bfg/tests/test_router.py b/repoze/bfg/tests/test_router.py
index 99316f056..1b76c0b7e 100644
--- a/repoze/bfg/tests/test_router.py
+++ b/repoze/bfg/tests/test_router.py
@@ -78,8 +78,9 @@ class RouterTests(unittest.TestCase):
         from repoze.bfg.interfaces import IViewPermission
         self.registry.registerAdapter(permission, for_, IViewPermission, name)
 
-    def _registerSecurityPolicy(self):
-        secpol = DummySecurityPolicy()
+    def _registerSecurityPolicy(self, secpol=None):
+        if secpol is None:
+            secpol = DummySecurityPolicy()
         from repoze.bfg.interfaces import ISecurityPolicy
         self.registry.registerUtility(secpol, ISecurityPolicy)
         return secpol
@@ -130,8 +131,9 @@ class RouterTests(unittest.TestCase):
         self._registerTraverserFactory(context)
         rootfactory = self._registerRootFactory(None)
         logger = self._registerLogger()
-        secpol = self._registerSecurityPolicy()
-        del secpol.forbidden
+        class Dummy:
+            pass
+        self._registerSecurityPolicy(Dummy())
         router = self._makeOne()
         self.assertEqual(len(logger.messages), 1)
         self.failUnless('which does not have a "forbidden" method'
@@ -150,8 +152,12 @@ class RouterTests(unittest.TestCase):
         self.registry.registerUtility(factory, IUnauthorizedAppFactory)
         router = self._makeOne()
         self.assertEqual(len(logger.messages), 1)
-        self.failUnless('IForbiddenAppFactory' in logger.messages[0])
-        self.assertEqual(router.forbidden_app_factory(None, None), 'yo')
+        self.failUnless('IForbiddenResponseFactory' in logger.messages[0])
+        class DummyRequest:
+            def get_response(self, app):
+                return app
+        req = DummyRequest()
+        self.assertEqual(router.forbidden_resp_factory(None, req), 'yo')
 
     def test_inotfound_appfactory_override(self):
         from repoze.bfg.interfaces import INotFoundAppFactory
@@ -162,31 +168,31 @@ class RouterTests(unittest.TestCase):
         router = self._makeOne()
         self.assertEqual(router.notfound_app_factory, app)
 
-    def test_iforbidden_appfactory_override_withsecpol(self):
-        from repoze.bfg.interfaces import IForbiddenAppFactory
+    def test_iforbidden_respfactory_override_withsecpol(self):
+        from repoze.bfg.interfaces import IForbiddenResponseFactory
         def app():
             """ """
-        self.registry.registerUtility(app, IForbiddenAppFactory)
+        self.registry.registerUtility(app, IForbiddenResponseFactory)
         self._registerSecurityPolicy()
         self._registerRootFactory(None)
         router = self._makeOne()
-        self.assertEqual(router.forbidden_app_factory, app)
+        self.assertEqual(router.forbidden_resp_factory, app)
 
-    def test_iforbidden_appfactory_override_nosecpol(self):
-        from repoze.bfg.interfaces import IForbiddenAppFactory
+    def test_iforbidden_responsefactory_override_nosecpol(self):
+        from repoze.bfg.interfaces import IForbiddenResponseFactory
         def app():
             """ """
-        self.registry.registerUtility(app, IForbiddenAppFactory)
+        self.registry.registerUtility(app, IForbiddenResponseFactory)
         self._registerRootFactory(None)
         router = self._makeOne()
-        self.assertEqual(router.forbidden_app_factory, app)
+        self.assertEqual(router.forbidden_resp_factory, app)
 
-    def test_iforbidden_appfactory_nooverride(self):
+    def test_iforbidden_responsefactory_nooverride(self):
         secpol = self._registerSecurityPolicy()
         context = DummyContext()
         self._registerRootFactory(None)
         router = self._makeOne()
-        self.assertEqual(router.forbidden_app_factory, secpol.forbidden)
+        self.assertEqual(router.forbidden_resp_factory, secpol.forbidden)
 
     def test_call_no_view_registered_no_isettings(self):
         environ = self._makeEnviron()
@@ -782,11 +788,11 @@ class DummyResponse:
     app_iter = ()
     
 class DummySecurityPolicy:
-    def __init__(self):
-        def wsgiapp(environ, start_response):
-            self.environ = environ
-            self.start_response = start_response
-            start_response('401 Unauthorized', [])
-            return 'Unauthorized'
-        self.forbidden = lambda *x: wsgiapp
+    def forbidden(self, context, request):
+        self.request = request
+        ob = DummyResponse()
+        ob.status = '401 Unauthorized'
+        ob.app_iter = ['Unauthorized']
+        ob.headerlist = ()
+        return ob
 
diff --git a/repoze/bfg/tests/test_security.py b/repoze/bfg/tests/test_security.py
index ffac19e0d..b9f9624d4 100644
--- a/repoze/bfg/tests/test_security.py
+++ b/repoze/bfg/tests/test_security.py
@@ -245,14 +245,12 @@ class TestACLSecurityPolicy(unittest.TestCase):
 
     def test_forbidden(self):
         policy = self._makeOne(lambda *arg: None)
-        forbidden_app = policy.forbidden(None, None)
-        environ = {}
-        result = []
-        def start_response(status, headers):
-            result.append((status, headers))
-        response = forbidden_app(environ, start_response)
-        self.assertEqual(result[0][0], '401 Unauthorized')
-        self.failUnless(len(result[0][1]), 2) # headers
+        context = DummyContext()
+        request = DummyRequest({})
+        response = policy.forbidden(context, request)
+        self.failUnless('401 Unauthorized' in response.app_iter[0])
+        self.assertEqual(response.status, '401 Unauthorized')
+        self.assertEqual(len(response.headerlist), 2)
         
 
 class TestInheritingACLSecurityPolicy(unittest.TestCase):
@@ -444,14 +442,12 @@ class TestInheritingACLSecurityPolicy(unittest.TestCase):
 
     def test_forbidden(self):
         policy = self._makeOne(lambda *arg: None)
-        forbidden_app = policy.forbidden(None, None)
-        environ = {}
-        result = []
-        def start_response(status, headers):
-            result.append((status, headers))
-        response = forbidden_app(environ, start_response)
-        self.assertEqual(result[0][0], '401 Unauthorized')
-        self.failUnless(len(result[0][1]), 2) # headers
+        context = DummyContext()
+        request = DummyRequest({})
+        response = policy.forbidden(context, request)
+        self.failUnless('401 Unauthorized' in response.app_iter[0])
+        self.assertEqual(response.status, '401 Unauthorized')
+        self.assertEqual(len(response.headerlist), 2)
 
 class TestAllPermissionsList(unittest.TestCase):
     def setUp(self):