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

[Mads Kiilerich]

On 05/10/2015 08:22 PM, Thomas De Schampheleire wrote:
> # 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()

I pushed these two, thanks .. but used abort(403) to be consistent with 
the other 403 in the same function.

/Mads