[PATCH 2 of 5] auth: return early in LoginRequired on invalid IP

[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 1427271075 -3600
> #      Wed Mar 25 09:11:15 2015 +0100
> # Node ID 43b0bc9088d3eedacf7610ffbf2f4b429ae98c45
> # Parent  c5828585502f1a061f162abe8cbd181c17039843
> auth: return early in LoginRequired on invalid IP
>
> Simplify the code of the LoginRequired decorator by returning early when an
> unacceptable condition is met.
>
> diff --git a/kallithea/lib/auth.py b/kallithea/lib/auth.py
> --- a/kallithea/lib/auth.py
> +++ b/kallithea/lib/auth.py
> @@ -739,13 +739,19 @@
>           user = cls.authuser
>           loc = "%s:%s" % (cls.__class__.__name__, func.__name__)
>   
> +        def redirect_to_login():
> +            p = url.current()
> +            log.debug('redirecting to login page with %s' % p)
> +            return redirect(url('login_home', came_from=p))

Nested function definitions always makes me wonder which bindings the 
function has to the surrounding scope. Is it a pure function where 
everything is passed as parameters, or will it also use some "global" state?

It seems like this is a general function that it could be nice to have 
in helpers.py or here in auth.py ... and use it everywhere instead of 
direct redirect to login_home.

> +
>           # check if our IP is allowed
> -        ip_access_valid = True
>           if not user.ip_allowed:
>               from kallithea.lib import helpers as h
>               h.flash(h.literal(_('IP %s not allowed' % (user.ip_addr))),
>                       category='warning')
> -            ip_access_valid = False
> +            log.warning('Access to %s from unallowed IP address for user %s'
> +                     % (loc, user))

(It would be nice if h.flash automatically would log messages. (Messages 
might in some cases have different severity for the user and for the 
server admin - that could perhaps be controlled by an extra parameter to 
flash ...))

> +            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
> @@ -770,21 +776,17 @@
>           log.debug('Checking if %s is authenticated @ %s' % (user.username, loc))
>           reason = 'RegularAuth' if user.is_authenticated else 'APIAuth'
>   
> -        if ip_access_valid and (user.is_authenticated or api_access_valid):
> +        if user.is_authenticated or api_access_valid:
>               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: '
> -                     'IP_ACCESS:%s API_ACCESS:%s'
> -                     % (user, reason, loc, ip_access_valid, api_access_valid)
> +                     'API_ACCESS:%s'
> +                     % (user, reason, loc, api_access_valid)
>               )
> -            p = url.current()
> -
> -            log.debug('redirecting to login page with %s' % p)
> -            return redirect(url('login_home', came_from=p))
> -
> +            return redirect_to_login()
>   
>

/Mads