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

[Thomas De Schampheleire]

On May 13, 2015 1:16:17 AM CEST, Mads Kiilerich <mads at kiilerich.com> wrote:
>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.

Yes, I wondered about that. What is the difference?

Also, any particular reason why you did not apply 6of6, which was also generic cleanup?

>
>/Mads