On 05/13/2015 02:19 PM, Thomas De Schampheleire wrote:
On May 13, 2015 1:16:17 AM CEST, Mads Kiilerich <m...@kiilerich.com> wrote:
On 05/10/2015 08:22 PM, Thomas De Schampheleire wrote:
# HG changeset patch
# User Thomas De Schampheleire <thomas.de.schamphele...@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
_______________________________________________
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
