On May 13, 2015 1:16:17 AM CEST, Mads Kiilerich <[email protected]> wrote: >On 05/10/2015 08:22 PM, Thomas De Schampheleire wrote: >> # HG changeset patch >> # User Thomas De Schampheleire <[email protected]> >> # 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 _______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
