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

[Mads Kiilerich]

On 05/13/2015 02:19 PM, Thomas De Schampheleire wrote:
> 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?

Probably just different libraries/frameworks that do the same thing. It 
is probably possible to figure out which one is "better" and more or 
less use that consisently.

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

Probably just because I thought it used functionality from the first 
changesets ...

/Mads