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

[Thomas De Schampheleire]

On Wed, Mar 25, 2015 at 8:03 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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
> ...))
>

Good feedback, I will look deeper into this and fix it in the next iteration.