Merge lp:~widelands-dev/widelands-website/fix_REMOTE_ADDR into lp:widelands-website

[kaputtnik]

Fixes getting the real ip address.

Please take a look into the (unchanged) line of http://bazaar.launchpad.net/~widelands-dev/widelands-website/fix_REMOTE_ADDR/view/head:/tracking/utils.py#L19 :

ip_address = request.META.get('HTTP_X_FORWARDED_FOR',
                                  request.META.get('REMOTE_ADDR', '127.0.0.1'))

It looks like the one parameter is used if the previous doesn't exist. But i couldn't find a documentation of the function get() to use it like this. Because we use the tracking only for showing online users (and not all the other things it can do), it needs also a big cleanup or replaced with another, or own, implementation.

I couldn't test this branch on localhost for HTTP_X_FORWARDED_FOR (no proxy), so we might want to test it on the alpha-site.

[lp:~widelands-dev/widelands-website/fix_REMOTE_ADDR]

427. By kaputtnik on 2016-10-13

merged with trunk

[SirVer (sirver)]

lgtm. one possible improvement in a comment further down.

> It looks like the one parameter is used if the previous doesn't exist.

That is exactly right. get() is a method on the built in type dict in Python. documentation (for python 2.7) is here: https://docs.python.org/2/library/stdtypes.html#dict.get

> Because we use the tracking only for showing online users (and not all the other things it can do), it needs also a big cleanup or replaced with another, or own, implementation.

I am not sure what you want to say with this. Why is the current implementation not sufficient?

review: Approve

[kaputtnik (franku)]

Thanks for pointing me to dict.get().... i am too stupid :-S

Regarding the tracking app: It's just my personal view... i don't like shipping an app with the widelands code when much code of it is potentially unused. 'Potentially' because i didn't took a closer look into it, just made it work for Django 1.8.

I like your suggestion for the code change. But i am unsure about returning 'None' if both entries didn't match (the previous code didn't caught this circumstance also). The question i am raising is: Since this function is used now for every request on a users ip, shouldn't it raise an error, or at least a 'fake' ip if both entries didn't match?

I have searched the code and some models containing a 'GenericIPAddressField' haven't set the field option 'blank=True', which means 'could be empty'. I found it only in wlimages and threadedcomments. So all fields without this setting expects a valid ip. This is the reason why e.g. the admin page for pybb Posts doesn't allow to save a change when the ip field isn't set.

Opinions?

[SirVer (sirver)]

> Thanks for pointing me to dict.get().... i am too stupid :-S

you are quite harsh on yourself. missing something in the huge pile of documentation that is django + python is really quite easy.

> Regarding the tracking app: It's just my personal view... i don't like shipping an app with the widelands code when much code of it is potentially unused.

I agree. Unused code should be deleted. But I think there are more useful things in the homepage than removing unused code - that said, if you want to do it I will gladly review it :)

> request variables

A bit of googling says you are probably right: https://www.djangoproject.com/weblog/2009/jul/28/security/#secondary-issue

We cannot rely on any of these variables to be set or even correct. I suggest we return a known false IP that we can recognize in the database instead. For example 192.168.23.42 or something like this. And we should probably not use the IP for anything besides what we maybe require for SPAM services.

[lp:~widelands-dev/widelands-website/fix_REMOTE_ADDR]

428. By kaputtnik on 2016-10-16

reworked wl_utils.py

429. By kaputtnik on 2016-10-17

added comment to wl_utils.py, fixed a missing import

[kaputtnik (franku)]

I have chosen an easier to remember IP: 192.168.255.255 and added a comment.

Django is sometimes really annoying: I have spend now several hours because the rating of maps didn't work anymore. Django showed no errors in debug mode but in console i could see a http500 every time i tried to rate a map. Made several print statements and in the end i thought rating for maps is broken also on widelands.org... no, it isn't. There was only a missing import of wl_utils.py in wlmaps/views.py...

[kaputtnik (franku)]

Merged and deployed now

=== modified file 'djangoratings/templatetags/ratings.py'
--- djangoratings/templatetags/ratings.py	2016-05-18 19:31:46 +0000
+++ djangoratings/templatetags/ratings.py	2016-10-17 20:24:22 +0000
@@ -8,6 +8,7 @@
 from django.db.models import ObjectDoesNotExist
 
 from djangoratings.models import Vote
+from wl_utils import get_real_ip
 
 register = template.Library()
 
@@ -25,7 +26,7 @@
         except (template.VariableDoesNotExist, AttributeError):
             return ''
         try:
-            vote = field.get_rating_for_user(request.user, request.META['REMOTE_ADDR'], request.COOKIES)
+            vote = field.get_rating_for_user(request.user, get_real_ip(request), request.COOKIES)
             context[self.context_var] = vote
         except ObjectDoesNotExist:
             context[self.context_var] = 0

=== modified file 'djangoratings/views.py'
--- djangoratings/views.py	2016-05-18 19:31:46 +0000
+++ djangoratings/views.py	2016-10-17 20:24:22 +0000
@@ -5,6 +5,7 @@
 from exceptions import *
 from django.conf import settings
 from default_settings import RATINGS_VOTES_PER_IP
+from wl_utils import get_real_ip
 
 class AddRatingView(object):
     def __call__(self, request, content_type_id, object_id, field_name, score):
@@ -30,12 +31,12 @@
             'score': score,
         })
         
-        had_voted = bool(field.get_rating_for_user(request.user, request.META['REMOTE_ADDR'], request.COOKIES))
+        had_voted = bool(field.get_rating_for_user(request.user, get_real_ip(request), request.COOKIES))
         
         context['had_voted'] = had_voted
                     
         try:
-            adds = field.add(score, request.user, request.META.get('REMOTE_ADDR'), request.COOKIES)
+            adds = field.add(score, request.user, get_real_ip(request), request.COOKIES)
         except IPLimitReached:
             return self.too_many_votes_from_ip_response(request, context)
         except AuthRequired:

=== modified file 'pybb/views.py'
--- pybb/views.py	2016-10-09 11:17:03 +0000
+++ pybb/views.py	2016-10-17 20:24:22 +0000
@@ -19,6 +19,7 @@
 from pybb import settings as pybb_settings
 from pybb.orm import load_related
 from django.conf import settings
+from wl_utils import get_real_ip
 
 try:
     from notification import models as notification
@@ -155,9 +156,8 @@
         post = get_object_or_404(Post, pk=quote_id)
         quote = quote_text(post.body, post.user, "markdown")
 
-    ip = request.META.get('REMOTE_ADDR', '')
     form = build_form(AddPostForm, request, topic=topic, forum=forum,
-                      user=request.user, ip=ip,
+                      user=request.user, ip=get_real_ip(request),
                       initial={'markup': "markdown", 'body': quote})
 
     if form.is_valid():

=== modified file 'threadedcomments/views.py'
--- threadedcomments/views.py	2016-05-15 14:41:54 +0000
+++ threadedcomments/views.py	2016-10-17 20:24:22 +0000
@@ -8,6 +8,7 @@
 from threadedcomments.forms import FreeThreadedCommentForm, ThreadedCommentForm
 from threadedcomments.models import ThreadedComment, FreeThreadedComment, DEFAULT_MAX_COMMENT_LENGTH
 from threadedcomments.utils import JSONResponse, XMLResponse
+from wl_utils import get_real_ip
 
 def _adjust_max_comment_length(form, field_name='comment'):
     """
@@ -84,7 +85,7 @@
     if form.is_valid():
         new_comment = form.save(commit=False)
         if not edit_id:
-            new_comment.ip_address = request.META.get('REMOTE_ADDR', None)
+            new_comment.ip_address = get_real_ip(request)
             new_comment.content_type = get_object_or_404(ContentType, id = int(content_type))
             new_comment.object_id = int(object_id)
         if model == ThreadedComment:

=== modified file 'wiki/views.py'
--- wiki/views.py	2016-06-20 19:11:03 +0000
+++ wiki/views.py	2016-10-17 20:24:22 +0000
@@ -18,6 +18,8 @@
 from django.contrib.auth.decorators import login_required
 from mainpage.templatetags.wl_markdown import do_wl_markdown
 
+from wl_utils import get_real_ip
+
 # Settings
 #  lock duration in minutes
 try:
@@ -34,16 +36,6 @@
 ALL_ARTICLES = Article.objects.all()
 ALL_CHANGES = ChangeSet.objects.all()
 
-
-def get_real_ip(request):
-    """ Returns the real user IP, even if behind a proxy.
-    Set BEHIND_PROXY to True in your settings if Django is
-    running behind a proxy.
-    """
-    if getattr(settings, 'BEHIND_PROXY', False):
-        return request.META['HTTP_X_FORWARDED_FOR']
-    return request.META['REMOTE_ADDR']
-
 def get_articles_by_group(article_qs, group_slug=None,
                           group_slug_field=None, group_qs=None):
     group = None

=== added file 'wl_utils.py'
--- wl_utils.py	1970-01-01 00:00:00 +0000
+++ wl_utils.py	2016-10-17 20:24:22 +0000
@@ -0,0 +1,8 @@
+
+def get_real_ip(request):
+    """Returns the real user IP, even if behind a proxy."""
+    for key in ('HTTP_X_FORWARDED_FOR', 'REMOTE_ADDR'):
+        if key in request.META:
+            return request.META[key]
+    # No match -> Return a fictional IP to have the model fields not empty
+    return '192.168.255.255'

=== modified file 'wlimages/views.py'
--- wlimages/views.py	2013-06-14 19:23:53 +0000
+++ wlimages/views.py	2016-10-17 20:24:22 +0000
@@ -1,27 +1,13 @@
 from django.contrib.auth.decorators import login_required
 from django.contrib.contenttypes.models import ContentType
-from django.core.urlresolvers import reverse
 from django.http import HttpResponse, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, render_to_response
 from django.template import RequestContext
 
 from models import Image
-from settings import MEDIA_ROOT
-from django.core.files.uploadedfile import SimpleUploadedFile
-from django.conf import settings
-
+from wl_utils import get_real_ip
 from forms import UploadImageForm
 
-
-def get_real_ip(request):
-    """ Returns the real user IP, even if behind a proxy.
-    Set BEHIND_PROXY to True in your settings if Django is
-    running behind a proxy.
-    """
-    if getattr(settings, 'BEHIND_PROXY', False):
-        return request.META['HTTP_X_FORWARDED_FOR']
-    return request.META['REMOTE_ADDR']
-
 def display( request, image, revision ):
     revision = int(revision)
 

=== modified file 'wlmaps/views.py'
--- wlmaps/views.py	2016-05-18 19:31:46 +0000
+++ wlmaps/views.py	2016-10-17 20:24:22 +0000
@@ -8,12 +8,10 @@
 from django.contrib.auth.decorators import login_required
 from django.http import HttpResponseRedirect, HttpResponseNotAllowed, HttpResponse, HttpResponseBadRequest
 from django.core.urlresolvers import reverse
-from django.db import IntegrityError
 import models
 from settings import MAPS_PER_PAGE
-
+from wl_utils import get_real_ip
 import os
-import zipfile
 
 
 #########
@@ -46,7 +44,8 @@
         return HttpResponseBadRequest()
 
     m.rating.add(score=val, user=request.user,
-                 ip_address=request.META['REMOTE_ADDR'])
+                 ip_address=get_real_ip(request))
+    
     # m.save() is not needed
 
     return HttpResponseRedirect(reverse('wlmaps_view', None, {'map_slug': m.slug}))