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

[Mads Kiilerich]

On 03/25/2015 12:01 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1427274589 -3600
> #      Wed Mar 25 10:09:49 2015 +0100
> # Node ID 32ced9e32b506c7441339f67da3c4237de0e6fa3
> # Parent  43b0bc9088d3eedacf7610ffbf2f4b429ae98c45
> 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 (401 Unauthorized), 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, or redirecting to a login page, 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
> @@ -58,7 +58,7 @@
>       get_user_group_slug, conditional_cache
>   from kallithea.lib.caching_query import FromCache
>   
> -from webob.exc import HTTPUnauthorized
> +from webob.exc import HTTPUnauthorized, HTTPForbidden
>   
>   log = logging.getLogger(__name__)
>   
> @@ -738,6 +738,7 @@
>           cls = fargs[0]
>           user = cls.authuser
>           loc = "%s:%s" % (cls.__class__.__name__, func.__name__)
> +        log.debug('Checking access for user %s @ %s' % (user, loc))
>   
>           def redirect_to_login():
>               p = url.current()
> @@ -754,37 +755,34 @@
>               return redirect_to_login()
>   
>           # 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', '')

I think we should use request.GET.get('api_key') and thus get None if no 
key is provided (empty or not). (And let's get rid of the leading _ 
while touching it.)

> -        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:])
> -            else:
> -                api_access_valid = False
> -                if not _api_key:
> -                    log.debug("API KEY *NOT* present in request")
> +        if _api_key:

- and here we should use: if _api_key is not None

That would make it more obvious that an empty key is an (invalid) key.

/Mads

> +            # 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 *NOT* valid" % _api_key[-4:])
> +                    log.warning('API key ****%s is NOT valid' % _api_key[-4:])
>                       headers = [('WWW-Authenticate', 'APIKEY realm="Kallithea"')]
>                       raise HTTPUnauthorized(headers=headers)
> +            else:
> +                # controller does not allow API access
> +                log.warning('API access to %s is not allowed' % loc)
> +                raise HTTPForbidden()
>   
>           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()
>   
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> http://lists.sfconservancy.org/mailman/listinfo/kallithea-general