IForbiddenAppFactory -> IForbiddenResponseFactory.

5 files changed, 144 insertions, 73 deletions

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