[PATCH 5 of 6 v2] auth: return early in LoginRequired on API key validation

[Thomas De Schampheleire]

# HG changeset patch
# User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
# Date 1427743622 -7200
#      Mon Mar 30 21:27:02 2015 +0200
# Node ID b8ff1ec9f8e70a4540ab03db822367cde8ea1df2
# Parent  126d600ac54455fc07d40b65f511b73577090757
auth: return early in LoginRequired on API key validation

Simplify the logic in the LoginRequired decorator when Kallithea is accessed
using an API key. Either:
- the key is valid and API access is allowed for the accessed method
  (continue), or
- the key is invalid (redirect to login page), or
- the accessed method does not allow API access (403 Forbidden)

In none of these cases does it make sense to continue checking for user
authentication, so return early.

diff --git a/kallithea/lib/auth.py b/kallithea/lib/auth.py
--- a/kallithea/lib/auth.py
+++ b/kallithea/lib/auth.py
@@ -59,6 +59,7 @@ from kallithea.lib.utils import get_repo
     get_user_group_slug, conditional_cache
 from kallithea.lib.caching_query import FromCache
 
+from webob.exc import HTTPForbidden
 
 log = logging.getLogger(__name__)
 
@@ -746,31 +747,31 @@ class LoginRequired(object):
         cls = fargs[0]
         user = cls.authuser
         loc = "%s:%s" % (cls.__class__.__name__, func.__name__)
+        log.debug('Checking access for user %s @ %s' % (user, loc))
 
         # check if our IP is allowed
         if not user.ip_allowed:
             return redirect_to_login(_('IP %s not allowed' % (user.ip_addr)))
 
-        # check if we used an APIKEY and it's a valid one
-        # defined whitelist of controllers which API access will be enabled
-        _api_key = request.GET.get('api_key', '')
-        api_access_valid = allowed_api_access(loc, api_key=_api_key)
-
-        # explicit controller is enabled or API is in our whitelist
-        if self.api_access or api_access_valid:
-            log.debug('Checking API KEY access for %s' % cls)
-            if _api_key and _api_key in user.api_keys:
-                api_access_valid = True
-                log.debug('API KEY ****%s is VALID' % _api_key[-4:])
+        # check if we used an API key and it's a valid one
+        api_key = request.GET.get('api_key')
+        if api_key is not None:
+            # explicit controller is enabled or API is in our whitelist
+            if self.api_access or allowed_api_access(loc, api_key=api_key):
+                if api_key in user.api_keys:
+                    log.info('user %s authenticated with API key ****%s @ %s'
+                             % (user, api_key[-4:], loc))
+                    return func(*fargs, **fkwargs)
+                else:
+                    log.warning('API key ****%s is NOT valid' % api_key[-4:])
+                    return redirect_to_login(_('Invalid API key'))
             else:
-                api_access_valid = False
-                if not _api_key:
-                    log.debug("API KEY *NOT* present in request")
-                else:
-                    log.warning("API KEY ****%s *NOT* valid" % _api_key[-4:])
+                # controller does not allow API access
+                log.warning('API access to %s is not allowed' % loc)
+                raise HTTPForbidden()
 
         # CSRF protection - POSTs with session auth must contain correct token
-        if request.POST and user.is_authenticated and not api_access_valid:
+        if request.POST and user.is_authenticated:
             token = request.POST.get(secure_form.token_key)
             if not token or token != secure_form.authentication_token():
                 log.error('CSRF check failed')
@@ -779,15 +780,14 @@ class LoginRequired(object):
         log.debug('Checking if %s is authenticated @ %s' % (user.username, loc))
         reason = 'RegularAuth' if user.is_authenticated else 'APIAuth'
 
-        if user.is_authenticated or api_access_valid:
+        if user.is_authenticated:
             log.info('user %s authenticating with:%s IS authenticated on func %s '
                      % (user, reason, loc)
             )
             return func(*fargs, **fkwargs)
         else:
             log.warning('user %s authenticating with:%s NOT authenticated on func: %s: '
-                     'API_ACCESS:%s'
-                     % (user, reason, loc, api_access_valid)
+                     % (user, reason, loc)
             )
             return redirect_to_login()
 
diff --git a/kallithea/tests/functional/test_login.py b/kallithea/tests/functional/test_login.py
--- a/kallithea/tests/functional/test_login.py
+++ b/kallithea/tests/functional/test_login.py
@@ -319,7 +319,7 @@ class TestLoginController(TestController
                 self.app.get(url(controller='changeset',
                                  action='changeset_raw',
                                  repo_name=HG_REPO, revision='tip', api_key=api_key),
-                             status=302)
+                             status=403)
 
     @parameterized.expand([
         ('none', None, 302),