Fixing caching and HTTP basic auth behavior

doc/topics/configuration.rst

@@ -21,9 +21,6 @@ default.
 backend, and serve it. User must have ``cache_update`` permissions. If not,
 returns 404.
 
-``cache redirect`` - If the user has ``cache_update`` permissions, download and
-cache the package. If not, return a 302 to the ``fallback_url``.
-
 ``none`` - Return a 404
 
 ``pypi.fallback_url``

pypicloud/__init__.py

@@ -4,13 +4,14 @@
 import logging
 from pyramid.config import Configurator
 from pyramid.renderers import JSON, render
-from pyramid.settings import asbool, aslist
+from pyramid.settings import asbool
 from pyramid_beaker import session_factory_from_settings
 from six.moves.urllib.parse import urlencode  # pylint: disable=F0401,E0611
 
 from .cache import get_cache_impl
 from .route import Root
 
+
 __version__ = '0.2.1'
 LOG = logging.getLogger(__name__)
 
@@ -64,18 +65,17 @@ def includeme(config):
     config.registry.fallback_url = settings.get('pypi.fallback_url',
                                                 default_url)
 
-    fallback_modes = aslist(settings.get('pypi.fallback', ['redirect']))
+    fallback_mode = settings.get('pypi.fallback', 'redirect')
     # Compatibility with the deprecated pypi.use_fallback option
     if 'pypi.fallback' not in settings and 'pypi.use_fallback' in settings:
         LOG.warn("Using deprecated option 'pypi.use_fallback'")
         use_fallback = asbool(settings['pypi.use_fallback'])
-        fallback_modes = ['redirect'] if use_fallback else []
+        fallback_mode = 'redirect' if use_fallback else 'none'
     modes = ('redirect', 'cache', 'none')
-    for mode in fallback_modes:
-        if mode not in modes:
-            raise ValueError("Invalid value for 'pypi.fallback'. "
-                             "Must be one of %s" % ', '.join(modes))
-    config.registry.fallback = fallback_modes
+    if fallback_mode not in modes:
+        raise ValueError("Invalid value for 'pypi.fallback'. "
+                         "Must be one of %s" % ', '.join(modes))
+    config.registry.fallback = fallback_mode
 
     # CACHING DATABASE SETTINGS
     cache_impl = get_cache_impl(settings)

pypicloud/auth.py

@@ -7,7 +7,8 @@
 from pyramid.security import Everyone, authenticated_userid
 
 
-# Copied from http://docs.pylonsproject.org/projects/pyramid_cookbook/en/latest/auth/basic.html
+# Copied from
+# http://docs.pylonsproject.org/projects/pyramid_cookbook/en/latest/auth/basic.html
 def get_basicauth_credentials(request):
     """ Get the user/password from HTTP basic auth """
     authorization = AUTHORIZATION(request.environ)
@@ -148,6 +149,7 @@ def includeme(config):
     config.add_authentication_policy(BasicAuthenticationPolicy())
     config.add_request_method(authenticated_userid, name='userid',
                               reify=True)
+    config.add_request_method(_forbid, name='forbid')
 
     settings = config.get_settings()
     realm = settings.get('pypi.realm', 'pypi')

pypicloud/views/api.py

@@ -63,10 +63,10 @@ def download_package(context, request):
     """ Download package, or redirect to the download link """
     package = request.db.fetch(context.filename)
     if not package:
-        if 'cache' not in request.registry.fallback:
+        if request.registry.fallback != 'cache':
             return HTTPNotFound()
         if not request.access.can_update_cache():
-            return HTTPForbidden()
+            return request.forbid()
         # If we are caching pypi, download the package from pypi and save it
         locator = FilenameScrapingLocator(request.registry.fallback_url)
         dists = locator.get_project(context.name)

pypicloud/views/login.py

@@ -1,5 +1,4 @@
 """ Render views for logging in and out of the web interface """
-from paste.httpheaders import WWW_AUTHENTICATE
 from pyramid.httpexceptions import HTTPForbidden, HTTPFound, HTTPBadRequest
 from pyramid.security import NO_PERMISSION_REQUIRED, remember, forget
 from pyramid.view import view_config
@@ -11,36 +10,21 @@
 @view_config(context=Root, name='login', request_method='GET',
              permission=NO_PERMISSION_REQUIRED, subpath=(),
              renderer='login.jinja2')
-@view_config(context=HTTPForbidden, permission=NO_PERMISSION_REQUIRED,
-             renderer='login.jinja2')
 def get_login_page(request):
     """ Catch login and redirect to login wall """
-    login_url = request.app_url('login')
     if request.userid is not None:
         # User is logged in and fetching /login, so redirect to /
-        if request.url == login_url:
-            return HTTPFound(location=request.app_url())
-        else:
-            # If user is not authorized, hide the page
-            request.response.status_code = 404
-            return request.response
-    if request.url != login_url:
-        # If pip requested a protected package and it's not authed, prompt for
-        # credentials
-        if (request.path.startswith('/simple') or
-                request.path.startswith('/pypi')):
-            request.response.status_code = 401
-            realm = WWW_AUTHENTICATE.tuples('Basic realm="%s"' %
-                                            request.registry.realm)
-            request.response.headers.update(realm)
-            return request.response
-        else:
-            # If user is not logged in, hide the page
-            request.response.status_code = 404
-            return request.response
+        return HTTPFound(location=request.app_url())
     return {}
 
 
+@view_config(context=HTTPForbidden, permission=NO_PERMISSION_REQUIRED,
+             renderer='login.jinja2')
+def do_forbidden(request):
+    """ Intercept 403's and return 401's when necessary """
+    return request.forbid()
+
+
 @view_config(context=Root, name='login', request_method='POST', subpath=(),
              renderer='json', permission=NO_PERMISSION_REQUIRED)
 @argify

pypicloud/views/simple.py

@@ -1,7 +1,6 @@
 """ Views for simple pip interaction """
 import six
-from pyramid.httpexceptions import (HTTPBadRequest, HTTPFound, HTTPForbidden,
-                                    HTTPNotFound)
+from pyramid.httpexceptions import HTTPBadRequest, HTTPFound, HTTPNotFound
 from pyramid.view import view_config
 
 import posixpath
@@ -10,18 +9,17 @@
 from pyramid_duh import argify, addslash
 
 
-@view_config(context=Root, request_method='POST', subpath=(),
-             permission='login', renderer='json')
+@view_config(context=Root, request_method='POST', subpath=(), renderer='json')
 @view_config(context=SimpleResource, request_method='POST', subpath=(),
-             permission='login', renderer='json')
+             renderer='json')
 @argify
 def upload(request, name, version, content):
     """ Handle update commands """
     action = request.param(':action')
     name = normalize_name(name)
     if action == 'file_upload':
         if not request.access.has_permission(name, 'write'):
-            return HTTPForbidden()
+            return request.forbid()
         try:
             return request.db.upload(content.filename, content.file, name=name,
                                      version=version)
@@ -55,28 +53,30 @@ def package_versions(context, request):
     normalized_name = normalize_name(context.name)
 
     packages = request.db.all(normalized_name)
-    if not packages:
-        if ('cache' in request.registry.fallback and
-                request.access.can_update_cache()):
-            locator = FilenameScrapingLocator(request.registry.fallback_url)
-            dists = locator.get_project(context.name)
-            if not dists:
-                return HTTPNotFound()
-            pkgs = {}
-            for dist in six.itervalues(dists):
-                filename = posixpath.basename(dist.source_url)
-                url = request.app_url('api', 'package', dist.name, filename)
-                pkgs[filename] = url
-            return {'pkgs': pkgs}
-        elif 'redirect' in request.registry.fallback:
-            redirect_url = "%s/%s/" % (
-                request.registry.fallback_url.rstrip('/'), context.name)
-            return HTTPFound(location=redirect_url)
-        else:
+    if packages:
+        if not request.access.has_permission(normalized_name, 'read'):
+            return request.forbid()
+        pkgs = {}
+        for package in packages:
+            pkgs[package.filename] = package.get_url(request)
+        return {'pkgs': pkgs}
+
+    elif request.registry.fallback == 'cache':
+        if not request.access.can_update_cache():
+            return request.forbid()
+        locator = FilenameScrapingLocator(request.registry.fallback_url)
+        dists = locator.get_project(context.name)
+        if not dists:
             return HTTPNotFound()
-    if not request.access.has_permission(normalized_name, 'read'):
-        raise HTTPForbidden()
-    pkgs = {}
-    for package in packages:
-        pkgs[package.filename] = package.get_url(request)
-    return {'pkgs': pkgs}
+        pkgs = {}
+        for dist in six.itervalues(dists):
+            filename = posixpath.basename(dist.source_url)
+            url = request.app_url('api', 'package', dist.name, filename)
+            pkgs[filename] = url
+        return {'pkgs': pkgs}
+    elif request.registry.fallback == 'redirect':
+        redirect_url = "%s/%s/" % (
+            request.registry.fallback_url.rstrip('/'), context.name)
+        return HTTPFound(location=redirect_url)
+    else:
+        return HTTPNotFound()

tests/__init__.py

@@ -115,6 +115,7 @@ def setUp(self):
         self.request.userid = None
         self.db = self.request.db = DummyCache(self.request)
         self.request.path_url = '/path/'
+        self.request.forbid = MagicMock()
         self.params = {}
         self.request.param = lambda x: self.params[x]
 

tests/test_api.py

@@ -114,7 +114,7 @@ def test_download_fallback_cache_no_perm(self):
         db.fetch.return_value = None
         context = MagicMock()
         ret = api.download_package(context, self.request)
-        self.assertEqual(ret.status_code, 403)
+        self.assertEqual(ret, self.request.forbid())
 
     @patch('pypicloud.views.api.FilenameScrapingLocator')
     def test_download_fallback_cache_missing(self, locator):

tests/test_auth.py

@@ -55,6 +55,17 @@ def test_valid(self):
         creds = auth.get_basicauth_credentials(self.request)
         self.assertEqual(creds, {'login': username, 'password': password})
 
+    def test_forbid(self):
+        """ When not logged in, forbid() returns 401 """
+        ret = auth._forbid(self.request)
+        self.assertEqual(ret.status_code, 401)
+
+    def test_forbid_logged_in(self):
+        """ When logged in, forbid() returns 403 """
+        self.request.userid = 'abc'
+        ret = auth._forbid(self.request)
+        self.assertEqual(ret.status_code, 403)
+
 
 class TestBasicAuthPolicy(MockServerTest):
 

tests/test_login.py

@@ -21,32 +21,12 @@ def test_user_fetch_login(self):
         self.assertEqual(ret.status_code, 302)
         self.assertEqual(ret.location, '/')
 
-    def test_user_fetch_other(self):
-        """ If a logged-in user if fetching NOT /login, return 404 """
-        self.request.userid = 'dsa'
-        self.request.url = '/abc'
-        ret = login.get_login_page(self.request)
-        self.assertEqual(ret.status_code, 404)
-
     def test_anon_fetch_login(self):
         """ Anonymous user fetching /login renders login page """
         self.request.url = '/login'
         ret = login.get_login_page(self.request)
         self.assertEqual(ret, {})
 
-    def test_anon_fetch_other(self):
-        """ Anonymous user fetching NOT /login returns 404 """
-        self.request.url = '/wibbles'
-        ret = login.get_login_page(self.request)
-        self.assertEqual(ret.status_code, 404)
-
-    def test_anon_fetch_simple(self):
-        """ Anonymous user fetching /simple/* returns 401 """
-        self.request.url = '/simple/requests'
-        self.request.path = '/simple/requests'
-        ret = login.get_login_page(self.request)
-        self.assertEqual(ret.status_code, 401)
-
 
 class TestLogin(MockServerTest):
 

tests/test_security.py

@@ -92,7 +92,7 @@ def test_api_pkg_unauthed(self):
         """ /api/package/<pkg> requires read perms """
         response = self.app.get('/api/package/%s/' % self.package.name,
                                 expect_errors=True)
-        self.assertEqual(response.status_int, 404)
+        self.assertEqual(response.status_int, 401)
 
     def test_api_pkg_authed(self):
         """ /api/package/<pkg> requires read perms """
@@ -109,7 +109,7 @@ def test_api_pkg_versions_unauthed(self):
                                        self.package.filename)
         response = self.app.post(url, params, expect_errors=True,
                                  headers=_simple_auth('user', 'user'))
-        self.assertEqual(response.status_int, 404)
+        self.assertEqual(response.status_int, 403)
 
     def test_api_pkg_versions_authed(self):
         """ /api/package/<pkg>/<filename> requires write perms """
@@ -128,7 +128,7 @@ def test_api_delete_unauthed(self):
                                       self.package.filename)
         response = self.app.delete(url, expect_errors=True,
                                    headers=_simple_auth('user', 'user'))
-        self.assertEqual(response.status_int, 404)
+        self.assertEqual(response.status_int, 403)
 
     def test_api_delete_authed(self):
         """ delete /api/package/<pkg>/<filename> requires write perms """

tests/test_simple.py

@@ -48,7 +48,7 @@ def test_upload_no_write_permission(self):
         content.filename = 'foo-1.2.tar.gz'
         self.request.access.has_permission.return_value = False
         response = upload(self.request, name, version, content)
-        self.assertEqual(response.status_code, 403)
+        self.assertEqual(response, self.request.forbid())
 
     def test_upload_duplicate(self):
         """ Uploading a duplicate package returns 400 """
@@ -110,10 +110,10 @@ def setUp(self):
         self.context = SimplePackageResource(self.request, 'mypkg')
 
     def test_no_perms(self):
-        """ If not in the update_cache groups, return 404 """
+        """ If not in the update_cache groups, return 403 """
         self.request.access.can_update_cache.return_value = False
         result = package_versions(self.context, self.request)
-        self.assertEqual(result.status_code, 404)
+        self.assertEqual(result, self.request.forbid())
 
     @patch('pypicloud.views.simple.FilenameScrapingLocator')
     def test_no_dists(self, locator):